Only retain external static symbols across LTO #30830
Conversation
| contains_name(attrs, "no_mangle") || | ||
| contains_name(attrs, "link_section") || | ||
| contains_name(attrs, "linkage") || | ||
| contains_name(attrs, "export_name") |
There was a problem hiding this comment.
From #29676, this should use find_export_name_attr to avoid duplicating the literal string "export_name".
There was a problem hiding this comment.
The problem is the diagnostic parameter, and that this has to go through the CrateStore::is_extern_item trait. Constants should be used instead of string literals, but that's a change to be made to the entire compiler. I don't really feel that find_export_name_attr is the right way to go about this. I tried to thread it through and failed, maybe you can advise where I can get an instance of it in each of the two places it's needed?
|
... or nevermindish, removed those two. We can think about linkage later. |
|
How does that look? |
| @@ -299,16 +299,16 @@ pub fn find_crate_name(attrs: &[Attribute]) -> Option<InternedString> { | |||
| } | |||
|
|
|||
| /// Find the value of #[export_name=*] attribute and check its validity. | |||
| pub fn find_export_name_attr(diag: &Handler, attrs: &[Attribute]) -> Option<InternedString> { | |||
| pub fn find_export_name_attr(diag: Option<&Handler>, attrs: &[Attribute]) -> Option<InternedString> { | |||
There was a problem hiding this comment.
Would it be possible to ensure that a &Handler gets threaded through to here on all control flow paths?
There was a problem hiding this comment.
Needs to be done https://github.com/rust-lang/rust/pull/30830/files#diff-896d2c044f21cf78ca23be2b4b6a52a1R240 and https://github.com/rust-lang/rust/pull/30830/files#diff-ccf7f52a9abbf3308db4fa771bd5c0ecR264
Can you advise how to get one from those contexts?
There was a problem hiding this comment.
I think both those contexts have access to a ty::ctxt which I believe should have a handle to this via some methods (probably through the Session itself)?
|
Thanks @arcnmx! Sorry for the delay, but feel free to ping a PR whenever you update it as unfortunately github doesn't send out notifications for that kind of event. Can you be sure to add a test to this PR as well? |
|
☔ The latest upstream changes (presumably #31487) made this pull request unmergeable. Please resolve the merge conflicts. |
arcnmx
force-pushed
the
static-extern
branch
from
33f1c5b
to
0ff055a
Compare
|
... I'm actually not sure how to test this. The test would be to ensure that LTO causes the removal of a symbol, which is mangled so I don't know how to reference it (and using |
The code appears to have been broken in rust-lang/rust#30830. This commit uses no_mangle as a work-around, since that's special-cased by the compiler and always kept. A more proper solution would be to re-export the necessary symbols in the final crate, which should also ensure that they're kept. This would either introduce a platform dependency in the main crate, or require a restructuring of the code so that the kernel binary is produced by the platform code.
See #29676
r? @alexcrichton