Add a test to verify that libstd doesn't use protected symbols #132432
Conversation
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
S-waiting-on-review
|
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.
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. |
That would potentially explain the failure - although I couldn't find any platforms that override I guess rather than setting |
|
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 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? |
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
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 |
|
@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. |
davidlattimore
force-pushed
the
std-interposable
branch
from
deec30b to
c1285b4
Compare
|
I looked through the various subdirectories of |
davidlattimore
changed the title
|
This PR modifies 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. |
bors
added
S-waiting-on-bors
…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 `Allocator` method names in `alloc` free function docs) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#132432 - davidlattimore:std-interposable, r=Mark-Simulacrum Add a test to verify that libstd doesn't use protected symbols
No description provided.