Add a test to verify that libstd doesn't use protected symbols by davidlattimore · Pull Request #132432 · rust-lang/rust (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits1 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
labels
This was requested during the review of #131634. I initially had it as a commit on that PR, but have now moved it to a separate PR, since neither commit really depends on the other. Also, there was a test failure when I added the commit to that PR, so by separating it out, I can see if it's related to this change, or if it was just a flake.
This comment has been minimized.
Looks like this is a genuine failure. My guess is that it's because we're passing an extra flag when building std and that's changing a hash that then doesn't match some other build of std where that flag isn't passed. That's just a guess though.
@lqd, before I look into figuring out how to fix it, I'd like to better understand the need for this. Is the idea that we want to explicitly specify the visibility used by std in case we change the default? I don't have any plans at this stage to change the default, so I'm wondering if we should worry about it now.
I'm not sure why there'd be multiple builds of std with different flags, but yes the intent is to avoid accidental changes to bootstrap or rustc that would change the visibility of std symbols, because it's also linked with lld. It's seemingly important that we don't change its visibility, hence using an explicit flag or some kind of assert.
I guess if @Kobzol also agrees that it's not of interest I won't die on that hill.
There are also a couple lf targets which should always use hidden visibility which I think this flag would override.
There are also a couple lf targets which should always use hidden visibility which I think this flag would override.
That would potentially explain the failure - although I couldn't find any platforms that override default_visibility.
I guess rather than setting default_visibility, we could assert that it's not protected.
If we want to ensure that a specific visibility is used for stdlib, we should write a test for that. That's more important than having an explicit call in bootstrap, IMO.
If we want to ensure that a specific visibility is used for stdlib, we should write a test for that. That's more important than having an explicit call in bootstrap, IMO.
That makes sense. I had a quick look for tests that make assertions about libstd, but I couldn't find any. I could write a test that locates libstd, parses it with the object crate, then makes some assertions about the results. Any suggestions for where such a test should live?
Also, any hints for how to correctly locate libstd from such a test would be appreciated.
I was afraid you'd say that 😅 I have no idea. @jieyouxu Do you remember any run-make tests that dealt with examining the stdlib?
I was afraid you'd say that 😅 I have no idea. @jieyouxu Do you remember any run-make tests that dealt with examining the stdlib?
Not std specifically because we didn't have the need for that before. There's
and which is somewhat related, but I'd need to know about what exactly we are trying to test.
I.e. what regression are we trying to catch and what assertion would fail in such a hypothetical test? I'm slightly concerned about what @bjorn3 said
There are also a couple lf targets which should always use hidden visibility which I think this flag would override.
and I personally wouldn't be comfortable landing this unless we have extensive test coverage of some kind for said targets (unfortunately I don't know which). Especially since a test or build did fail here.
Well, right now we don't have any tests for this, and any test would be a best effort (i.e. check that at least the stdlib on x64 Linux has symbol visibility as we expect). The test would examine libstd.so (or libstd.rlib?) and check some ELF data from it, I assume.
@davidlattimore could you kindly elaborate on what properties we would be trying to check for in such a hypothetical test? I would like to understand the use case better before trying to provide a suggestion test infra wise.
I looked through the various subdirectories of tests and most of them it definitely didn't fit in. In the end I went with run-make. I've implemented a test and updated this PR.
Explicitly specify symbol visibility when building std Add a test to verify that libstd doesn't use protected symbols
This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.
cc @jieyouxu
@bors r+
This seems pretty reasonable looking to me, though it's worth noting that it (as any of our tests) doesn't actually check what we distribute, just what we build on other builders. When the build only uses protected symbols with lld, it seems plausible we could diverge unknowingly.
For now though this seems like still an improvement over the prior state.
📌 Commit c1285b4 has been approved by Mark-Simulacrum
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 8 pull requests
Successful merges:
- rust-lang#131523 (Fix asm goto with outputs and move it to a separate feature gate)
- rust-lang#131664 (Support input/output in vector registers of s390x inline assembly (under asm_experimental_reg feature))
- rust-lang#132432 (Add a test to verify that libstd doesn't use protected symbols)
- rust-lang#132502 (Document possibility to set core features in example config.toml)
- rust-lang#132529 (ci(triagebot): add more top-level files to A-meta)
- rust-lang#132533 (Add BorrowedBuf::into_filled{,_mut} methods to allow returning buffer with original lifetime)
- rust-lang#132803 (Fix broken url)
- rust-lang#132982 (alloc: fix
Allocatormethod names inallocfree function docs)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#132432 - davidlattimore:std-interposable, r=Mark-Simulacrum
Add a test to verify that libstd doesn't use protected symbols
Labels
Area: port run-make Makefiles to rmake.rs
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)