rustc: Allow #[no_mangle] anywhere in a crate
#54451
Merged
Conversation
This file contains 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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I'd still like to confirm that this fixes the issues with cc @japaric |
japaric
reviewed
Sep 22, 2018
src/tools/compiletest/src/main.rs
Outdated
| @@ -809,7 +809,7 @@ fn analyze_gdb(gdb: Option<String>) -> (Option<String>, Option<u32>, bool) { | |||
|
|
|||
| let gdb_native_rust = version.map_or(false, |v| v >= MIN_GDB_WITH_RUST); | |||
|
|
|||
| (Some(gdb.to_owned()), version, gdb_native_rust) | |||
| (Some(gdb.to_owned()), version, gdb_native_rust && false) | |||
There was a problem hiding this comment.
This looks ... suspicious. Perhaps it was used for local testing?
There was a problem hiding this comment.
Oops, indeed a mistake!
alexcrichton
force-pushed
the
no-mangle-extern-linkage
branch
from
9b2fd84 to
74639e9
Compare
japaric
reviewed
Sep 22, 2018
|
|
||
| #![crate_type = "lib"] | ||
|
|
||
| mod private { |
There was a problem hiding this comment.
Could you please add test cases for these patterns?
fn internal() {
#[no_mangle]
static EXTERNAL: fn() = internal;
}
static FOO: u32 = {
#[no_mangle]
static BAR: &u32 = &FOO;
0
};
const PRIVATE: () = {
#[no_mangle]
fn exported() {}
};I can see myself writing attributes / macros that expand into those patterns.
|
I tested this locally with the Cortex-M stuff by making a lot stuff private and linking works perfectly 👌 Though, it seems that the fn foo() {} // WARN function is never used
#[no_mangle]
static FOO: fn() = foo; // WARN static item is never used
struct Bar; // WARN struct is never constructed
#[no_mangle]
static BAR: Bar = Bar; // WARN static item is never used
#[no_mangle]
fn baz() {} // WARN function is never used |
|
@japaric ah I think I may have actually pushed an update after I noticed the same thing myself, if you try out the current state of the PR I think it should fix that. |
alexcrichton
force-pushed
the
no-mangle-extern-linkage
branch
from
74639e9 to
1cb4978
Compare
|
@alexcrichton I can confirm that I get zero warnings with the latest version. 👍 |
|
I am very interested to see if this helps resolve some of the transient linking failures that have been flummoxing me for weeks |
|
I’d like to point out that this is also a breaking change in case anybody depended on For example, doing something like this should end up raising a linker error after this PR: # test.rs
#[no_mangle]
fn foo() -> u32 { // not public!
42
}# test.c {
int foo(void) { return 64; }
int main() { return foo(); }# compile with:
rustc --emit=obj --crate-type=staticlib -o test.o
clang -c test.c -o ctest.o
clang ctest.o test.o
# linker error here because of now genuinely duplicate symbolsAdmittedly, I could not construct a Rust-only case for some reason, but this is still a breaking change nevertheless. |
|
@nagisa #![crate_type = "lib"]
#[no_mangle]
fn foo() -> u32 {
42
}
#[used]
static KEEP: fn() -> u32 = foo;$ nm -C foo.o
0000000000000000 t foo
0000000000000000 d foo::KEEP
No, wait. You can make it work without #![crate_type = "lib"]
#[inline(never)]
#[no_mangle]
fn foo() -> u32 {
unsafe { std::ptr::read_volatile(&42) }
}
pub fn bar() -> u32 {
foo()
}$ rustc +stable --emit=obj -O foo.rs
$ nm -C foo.o
0000000000000000 t foo
0000000000000000 r .Lbyte_str.0
0000000000000000 T foo::barI can't think of a use case for something like but since this is technically a breaking change we may want do a crater run before landing this. |
|
@bors: try |
|
⌛ Trying commit 1cb4978c467ef68b8e453fb25dcf3b7da33effd3 with merge 90cdb0b2660ed428dec2f4a3f9bf083fdc2247e5... |
|
☀️ Test successful - status-travis |
|
I doubt crater will uncover any breakage as it is more pertinent to binary crates which could possibly link multiple different versions of some dependency (both potentially containing the same |
|
@japaric Neither |
|
I'd be interested in whether there even are any crates that legitimately have private |
|
Since there's still way to reset the linkage to internal with |
src/libstd/panicking.rs
Outdated
| @@ -518,7 +518,7 @@ pub fn update_count_then_panic(msg: Box<dyn Any + Send>) -> ! { | |||
|
|
|||
| /// A private no-mangle function on which to slap yer breakpoints. | |||
| #[no_mangle] | |||
| #[allow(private_no_mangle_fns)] // yes we get it, but we like breakpoints | |||
| #[cfg_attr(stage0, allow(private_no_mangle_fns))] | |||
| pub fn rust_panic(mut msg: &mut dyn BoxMeUp) -> ! { | |||
There was a problem hiding this comment.
rust_panic need not to be exposed as an external symbol.
(Also, I'm not sure why it's pub, it's not used anywhere outside of its module.)
There was a problem hiding this comment.
Yeah the #[no_mangle] here is sort of opportunistically trying to keep rust_panic as a symbol in the crate, but the new changes in this PR exported it a bit too aggressively, I've tweaked this although it will likely continue to not work too reliably.
| @@ -352,7 +352,7 @@ impl<'a, 'tcx: 'a> ItemLikeVisitor<'tcx> for CollectPrivateImplItemsVisitor<'a, | |||
| // which are currently akin to allocator symbols. | |||
| let def_id = self.tcx.hir.local_def_id(item.id); | |||
| let codegen_attrs = self.tcx.codegen_fn_attrs(def_id); | |||
| if codegen_attrs.linkage.is_some() || | |||
| if codegen_attrs.contains_extern_indicator() || | |||
There was a problem hiding this comment.
This doesn't look correct, #[linkage] can be used to decrease visibilities as well, in this case we don't need to mark the item and everything it uses as reachable.
There was a problem hiding this comment.
Note that #[linkage] is an unstable attribute and has always been sort of wonky with how everything else in the compiler has evolved over time. This is just preserving the original behavior currently.
TBH the #[linkage] attribute is basically buggy enough to not be worth using any more, it needs a larger overhaul than changing the behavior at this location to be usable.
(for example there's no way to control visibility, and it's also not well defined whether the attribute makes it reachable).
Would you be ok leaving these sorts of questions to a later PR?
|
@craterbot run start=master#e7b5ba8661aa844a06c37f22d7af0afb1807d347 end=try#90cdb0b2660ed428dec2f4a3f9bf083fdc2247e5 mode=build-and-test |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
Centril
added
the
relnotes
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors: retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Oct 7, 2018
…elwoerister rustc: Allow `#[no_mangle]` anywhere in a crate This commit updates the compiler to allow the `#[no_mangle]` (and `#[export_name]` attributes) to be located anywhere within a crate. These attributes are unconditionally processed, causing the compiler to always generate an exported symbol with the appropriate name. After some discussion on #54135 it was found that not a great reason this hasn't been allowed already, and it seems to match the behavior that many expect! Previously the compiler would only export a `#[no_mangle]` symbol if it were *publicly reachable*, meaning that it itself is `pub` and it's otherwise publicly reachable from the root of the crate. This new definition is that `#[no_mangle]` *is always reachable*, no matter where it is in a crate or whether it has `pub` or not. This should make it much easier to declare an exported symbol with a known and unique name, even when it's an internal implementation detail of the crate itself. Note that these symbols will persist beyond LTO as well, always making their way to the linker. Along the way this commit removes the `private_no_mangle_functions` lint (also for statics) as there's no longer any need to lint these situations. Furthermore a good number of tests were updated now that symbol visibility has been changed. Closes #54135
|
☀️ Test successful - status-appveyor, status-travis |
japaric
added a commit
to rust-embedded/cortex-m-rt
that referenced
this pull request
Oct 26, 2018
Thanks to rust-lang/rust#54451, which will be available in 1.31, these attributes will work regardless of the visibility and reachability of the items.
bors bot
added a commit
to rust-embedded/cortex-m-rt
that referenced
this pull request
Oct 26, 2018
141: entry/exception/interrupt: reachability restriction is 1.30-only r=therealprof a=japaric Thanks to rust-lang/rust#54451, which will be available in 1.31, these attributes will work regardless of the visibility and reachability of the items. Co-authored-by: Jorge Aparicio <jorge@japaric.io>
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
Dec 11, 2018
mykmelez
pushed a commit
to mykmelez/gecko
that referenced
this pull request
Dec 12, 2018
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified
that referenced
this pull request
Oct 3, 2019
…st-lang/rust#54451. r=simonsapin UltraBlame original commit: 37f24bab080b559284ec7d9108f112f6a97db45c
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-comments-removed
that referenced
this pull request
Oct 3, 2019
…st-lang/rust#54451. r=simonsapin UltraBlame original commit: 37f24bab080b559284ec7d9108f112f6a97db45c
gecko-dev-updater
pushed a commit
to marco-c/gecko-dev-wordified-and-comments-removed
that referenced
this pull request
Oct 3, 2019
…st-lang/rust#54451. r=simonsapin UltraBlame original commit: 37f24bab080b559284ec7d9108f112f6a97db45c
adamgreig
pushed a commit
to rust-embedded/cortex-m
that referenced
this pull request
Jan 12, 2022
Thanks to rust-lang/rust#54451, which will be available in 1.31, these attributes will work regardless of the visibility and reachability of the items.
adamgreig
pushed a commit
to rust-embedded/cortex-m
that referenced
this pull request
Jan 12, 2022
141: entry/exception/interrupt: reachability restriction is 1.30-only r=therealprof a=japaric Thanks to rust-lang/rust#54451, which will be available in 1.31, these attributes will work regardless of the visibility and reachability of the items. Co-authored-by: Jorge Aparicio <jorge@japaric.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit updates the compiler to allow the
#[no_mangle](and#[export_name]attributes) to be located anywhere within a crate.These attributes are unconditionally processed, causing the compiler to
always generate an exported symbol with the appropriate name.
After some discussion on #54135 it was found that not a great reason
this hasn't been allowed already, and it seems to match the behavior
that many expect! Previously the compiler would only export a
#[no_mangle]symbol if it were publicly reachable, meaning that ititself is
puband it's otherwise publicly reachable from the root ofthe crate. This new definition is that
#[no_mangle]is alwaysreachable, no matter where it is in a crate or whether it has
pubornot.
This should make it much easier to declare an exported symbol with a
known and unique name, even when it's an internal implementation detail
of the crate itself. Note that these symbols will persist beyond LTO as
well, always making their way to the linker.
Along the way this commit removes the
private_no_mangle_functionslint(also for statics) as there's no longer any need to lint these
situations. Furthermore a good number of tests were updated now that
symbol visibility has been changed.
Closes #54135