-
Notifications
You must be signed in to change notification settings - Fork 1.6k
RFC: #[export_visibility = ...]
attribute
#3834
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
9e36c42
to
a79823e
Compare
a79823e
to
ab37907
Compare
|
||
## Benefit: Smaller binaries | ||
|
||
One undesirable consequence of unnecessary public exports is binary size bloat. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should only be the case for libraries. For binaries all functions are already made not-exported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, though on (weird/stunt) occasions there might be reasons to have public symbols in a binary.
(That case is not common and not important, just mentioning it in case someone figured it was categorically impossible and never happened.)
(when the freeing allocator expects that the pointer it got was earlier | ||
allocated by the same allocator instance). | ||
|
||
This is what happened in https://crbug.com/418073233. In the smaller repro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected all of Chromium to use a single rust allocator rather than use a different one for each DSO. Why is that not the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected all of Chromium to use a single rust allocator rather than use a different one for each DSO. Why is that not the case?
Is that really a requirement if foo.so
doesn't export any functions that return pointers to Rust-related objects? I would expect in such a case that which Rust allocator / standard library / etc is used would be an internal implementation detail of foo.so
. IIUC this detail leaks out only because of an unintentional export of a cxx
-generated, internal symbol.
But to try to answer the question - the same allocator is statically linked into Chromium binaries. This means that an executable and an .so
may end up with a separate copy of the same global data structures of the allocator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that really a requirement if foo.so doesn't export any functions that return pointers to Rust-related objects?
It is not a requirement. I'm just surprised that Chromium copies the entire rust standard library between the dylib and executable rather than using the copy from the dylib in the executable to save space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that really a requirement if foo.so doesn't export any functions that return pointers to Rust-related objects?
It is not a requirement. I'm just surprised that Chromium copies the entire rust standard library between the dylib and executable rather than using the copy from the dylib in the executable to save space.
That is indeed a bit unfortunate. I think this is to some extent based on the following:
- Chromium requirement to use an external linker
- Assumption that only
rlib
s /static_lib
s can be linked by an external linker, and that an external linker wouldn't be able to handledylib
s
But thank you for bringing this up - maybe this should indeed be treated as an alternative fix for https://crbug.com/418073233. I am not sure what the next steps should be for this aspect:
- Maybe I should open a bug (either under https://github.com/rust-lang/rust/issues, or under https://crbug.com) to move this discussion elsewhere?
- Maybe before opening a bug I should first learn more about
dylib
s...
For most use cases rather than specifying the exact symbol visibility (which may not even be supported by the object file format, like interposable on pe/coff or (with the default two-level namespaces) mach-o) I think having just a way to force SymbolExportLevel::Rust rather than the default SymbolExportLevel::C would be a better idea. This causes it to still be exported from rust dylibs (as necessary to avoid linker errors depending on the exactly when rustc decides to codegen functions), but prevents it from being exported from cdylibs. It doesn't work for staticlibs currently, but for those if you want to limit symbol visibility you have to specify your own version script during linking anyway to prevent exporting all rust mangled symbols too. |
The fact that you are distinguishing between
That's not 100% correct - instead of using a version script, one may also use |
The Chromium case is effectively equivalent to using staticlibs, not to using rust dylibs/cdylibs.
That doesn't apply to the standard library unless you go out of your way using unstable features to recompile the standard library. |
Ack / agreed.
Thank you for bringing up this point. This probably should be explicitly addressed by the RFC (*), but I am not sure if I agree with your conclusions so far. This is because:
(*) I am not sure what the right process is here. Should I add commits to the RFC as we keep discussing here? Should I first give people an opportunity to review the first draft? |
I think this is a good opportunity to expand the design space (and documentation) of "various levels of exportendess" a bit, even if the resulting proposal for There are multiple attributes that targets this similar space ( What I'd like to see is a table of "levels of exportedness" combined with the kinds of end artifacts, and how we can users can express all those levels with the attributes listed above.
In particular, one of my requirements is that |
This is something only rustc must be allowed to do (other than for symbols defined in inline asm called from within the same inline asm block). Only rustc knows if all callers will end up in the same object file as the definition and it doesn't provide any guarantees around when this happens. So exposing this to the user is a stability hazard.
For regular functions and
For rlib this doesn't make sense. There is no way to make rlibs a symbol export boundary without introducing an expensive link/object file rewrite step for each individual rlib. For staticlib it would be nice to have a symbol export boundary, but unfortunately we don't have one right now even for
This makes sense to me. See the end of my comment.
This has to always be the case if it is visible outside of the object file. The very point of rust dylibs is that rust code in a separate DSO can call any public function, which thanks to cross-crate inlining can call effectively every function that rustc wouldn't make private to the current object file. And again, rustc doesn't provide any guarantees when this happens, so allowing you to not export symbols from a rust dylib is a stability hazard.
Yes.
No
Not really aside from the visibility information we already tell the linker (export from rust dylib, don't export from cdylib). Currently rustc internally works with three different symbol export levels:
It makes sense to me to allow |
I think this probably should be captured somehow as one of the alternatives in the RFC. Is there a specific syntax that you have in mind here? I guess one option would be to have a |
👍
I don't think this is a good name as it is still meant to be usable from C, just not outside of the linked DSO.
This would be an option, although ideally if we manage to stop exporting all symbols from staticlibs, I would like the same attribute to be usable to prevent export from both cdylib and staticlib, so it should probably not mention cdylib in the name. I don't have suggestions for a better name though. |
Frankly, if we have |
…ternal_symbol--take-1 New unresolved question section: Rust standard library
In Rust 1.87 and before on GNU/Linux, this code: #![feature(rustc_attrs)]
#[rustc_std_internal_symbol]
#[no_mangle]
pub extern "C" fn blah(i: u32) -> u32 {
i+1
} produced an object with the What this issue is not applicable to:
Omitting the In C++, mangling (controlled by The analog to what This errors now in 1.88, the only way i'm aware of being injecting an assembly declaration |
@bjorn3 - thank you for proposing a narrower fix, focusing on setting
From my perspective both
|
I think the current RFC does a good job of creating a conceptual framework for symbol visibility that can abstract over platform differences, and the remaining issues seem like they can be addressed. Perhaps the issue with dylibs can be solved by linting on calls to non-inlined, hidden-linkage functions from inlined or generic functions. My feeling is that having two export levels, "Rust" and "C", isn't quite the right abstraction – or if it is, we should be able to spell it as I love the idea @petrochenkov raised in #3834 (comment) of having a table of all the things you might want to accomplish and how you would spell them. I wouldn't put the onus on this RFC to completely fill out that table, but it would be helpful to know which gaps exist and which are getting filled. It's also important to me that the users who really know what they're doing when it comes to symbol visibility should be able to exercise direct control. While those users are a small percentage of Rust users, I do see this as a significant benefit to them and to unlocking Rust usage in more arcane contexts. That control can be mediated by a table that maps from "how it's spelled in C compilers and linker scripts" to "how to spell it in Rust"; it doesn't require us to adopt the existing models or terminology wholesale. There are places we might reasonably want to place limits on this (as brought up by @bjorn3 above, setting visibility to an individual codegen unit is almost certainly a bad idea). Otherwise I think we should have some path that favors flexibility and transparency, and avoid forcing outside experts who come to Rust to guess at complex logic the compiler might be doing and why. |
|
||
### Interposable visibility | ||
|
||
`#[export_visibility = "interposable"]` will cause symbols to be emitted with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advocate for naming this "default" if it makes sense to do so, to avoid the bikeshed. That implies it would be affected by the unstable -Zdefault-visibility
flag.
If naming it "default" doesn't fit your use case, I would prefer a name with prior art, or something that fits the part-of-speech of existing names (perhaps "unprotected"). I wouldn't block on the name "interposable" if other people like it, but it sounds a little bit out of place to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
I agree that this should be treated as an open issue and should be resolved before either 1) merging this RFC or 2) landing PRs associated with this RFC. I think this doesn't necessarily change the overall sentiment for the RFC, so think this is not an urgent issue.
I think it is quite desirable to have consistent naming across -Zdefault-visibility=...
and #[export_visibility = ...]
. Therefore I would propose to 1) first land the current RFC + implementation PR as-is ("interposable" is one of values accepted today by -Zdefault-visibility=...
), and 2) then in separate PRs consistently change "interposable" to "public" / "default" / "exported" / whatever-we-agree-on in all end-user-facing language aspects (i.e. single PR to change the name both in -Zdefault-visibility=...
and #[export_visibility = ...]
+ maybe separate PRs to change the RFC (?)).
I think there are multiple naming options to consider:
- "interposable"
- "default". I dislike potential confusion with "inherit" which means - inherit platform defaults or
-Zdefault-visibility=...
. Maybe one way out is to rename:- "interposable" => "linker_default"
- "inherit" => "platform_default" (and document that
-Zdefault-visibility=...
affects the platform default)
- "public"
- "exported"
- ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I guess "linker_default" doesn't make sense for platforms like WASM...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading through this list again, I no longer think it should be called "default".
In any case I agree with your plan: Let's leave the exact names as an unresolved question and resolve it via separate FCP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way rustc always compiles with -fno-semantic-interposition
just like clang does by default, so interposing functions is unsound even when allowed by the dynamic linker.
@rfcbot fcp merge |
#[export_visibility = ...]
attribute.#[export_visibility = ...]
attribute
Nominating for lang team feedback on the approach of this RFC. Assuming we want to control symbol visibilities, the main lang question is how to surface those visibilities. This RFC proposes
Explanations are in this RFC section. The main alternative proposed is to create an attribute that tweaks the current compiler behavior, like As I argue in #3834 (comment), I think it would be clearer to surface a more direct mapping to the symbol visibility concepts that appear in other languages like C/C++, in linker arguments, and in binary formats. That's what this RFC does. @rustbot label I-lang-nominated |
Is this new attribute unsafe or not? From the caveats describes in the RFC, it sounds like it has to be unsafe. |
* This scenario leads to memory unsafety when: | ||
- The call from test executable to `rust_lib::get_string` ends up calling | ||
`dso!rust_lib::get_string` rather than `exe!rust_lib::get_string`. | ||
- The call from test executable to `cxxbridge1$string$drop` ends up | ||
calling `exe!cxxbridge1$string$drop`. | ||
- This means that the `exe`'s allocator tries to free an allocation made | ||
by the allocator from the `dso`. In debug builds this is caught by an | ||
assertion. In release builds this would lead to memory unsafety. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It remains unclear why this would ever happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens because dso!rust_lib::get_string
is a public/default/interposable (rather than hidden) symbol. This results in a naming conflict between dso!rust_lib::get_string
and exe!rust_lib::get_string
. And such a naming conflict means Undefined Behavior. So after dynamic linker links exe
and rust_lib
it is undefined if calls to rust_lib::get_string
will go to dso!rust_lib::get_string
vs exe!rust_lib::get_string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be better to explain the UB, than to explain how that UB happens to play out on current compilers and linkers. We are all already agreed that UB is bad so it doesn't really matter what happens afterwards. :)
@RalfJung Why would it be unsafe? Visibility changes no behavior of the compiled program, it only has meaning to the linker. (Disregarding LTO) |
Apparently it can break dylibs as well. Also, changing which function the linker picks up for which call can change the behavior of the compiled program, can't it? |
It cannot make anything unsafe that was safe before. If a certain binary is a valid implementation of the default-visibility program, the same binary but with symbols artificially removed is a valid implementation of the hidden-visibility program. |
Modifying your binary (I assume that's what you mean by "artificially removed") is definitely an unsafe operation so I don't see how that's an argument for the attribute being safe. |
My point is that visibility only affects behavior of outside code which is beyond Rust's business. By your argument, #[no_mangle] alone should already be unsafe since linking with outside code is unsafe. |
|
Then it's no problem since Rust mangled symbols have hidden visibility anyway! |
It also affects rust code that calls the function if it is included in a rust dylib. |
The target audience for this attribute is building cdylibs, not rust dylibs. |
Do you want usage of the |
...?!? So first you implied that no_mangle should be safe but then when you learned that it isn't you take that as supporting your argument? That doesn't seem coherent. (Btw, the reason it is unsafe is that one can use it to overwrite well-known functions like
That's not true when the Rust compiler is doing the linking. So we have to ensure that all those cases still behave correctly.
How's that relevant for the safety of the attribute? If there's any way for this to cause UB in Rust code, it must be unsafe. It doesn't matter whether that is using the attribute as intended or not. The bar for a safe attribute/function in Rust generally is that it must be impossible for this to make the program go wrong, even when deliberately misusing the language. To be clear, I don't know whether it should be unsafe or not. I am asking you and the other domain experts here. |
My take is:
|
This I agree with. |
RFC author here. I think the new attribute does not need to marked
|
My $.02 is that dylibs could check, and simply error on calling anything accross the boundary with less than protected visibility. Though, inline and generics can make this fuzzy. |
Hmmm... after having sent the above, now I think it is a bit inaccurate. I guess |
I hope that the inliner can check if the inlined code makes cross- |
In general, it can't codegen an inline function that calls a hidden symbol, which is bad because |
During codegen it is not yet known if two crates will end up getting linked into the same dylib, so it would need to inhibit inlining for any cross-crate calls into hidden symbols, not just cross-dylib calls. |
/// gets inlined into another `dylib` then the call to the internal helper | ||
/// will cross `dylib` boundaries - this will **not** work if the internal | ||
/// helper is hidden from dynamic linking. | ||
#[inline] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we solve this with a check that public #[inline]
and generic functions do not call a function with #[export_visibility = "hidden"]
? I don't think we need any kind of transitive call-graph analysis, just to check which functions are directly called.
I think we should also prevent pub
fns from being marked as #[export_visibility = "hidden"]
.
cc @bjorn3 whose perspective I'm particularly interested in on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this comment: #3834 (comment)
During codegen it is not yet known if two crates will end up getting linked into the same dylib, so it would need to inhibit inlining for any cross-crate calls into hidden symbols, not just cross-dylib calls.
This makes sense. My expectation would be for us to explicitly error/lint if you try to do this with public #[inline]
or generics, and silently inhibit inlining of the function calling a hidden symbol otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fundamentally, trying to avoid these errors by restricting how source programs are written means committing to details of rustc’s codegen strategy that are currently unstable implementation details. To give just one concrete example not covered by the above comments, rustc currently has some logic to treat small functions as-if they were #[inline]
for codegen purposes even if they weren’t declared as such in the source code. I expect it’ll be really hard to figure out a set of rules that reliably avoids these errors and doesn’t restrict desirable current or future cleverness in the compiler. (Edit: … and isn’t too restrictive to be useful — #[no_mangle]
is basically a guarantee that it’ll get codegen’d but people probably want to factor out calls to visibility=hidden without adding more unmangled symbols to their library.)
## Cross-platform behavior | ||
[cross-platform-behavior]: #cross-platform-behavior | ||
|
||
We don't really know | ||
whether the `hidden` / `protected` / `interposable` visibilities | ||
make sense across different target platforms and/or map to distinct entities | ||
(see | ||
[a Zulip question here](https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/.60.23.5Bexport_visibility.20.3D.20.2E.2E.2E.5D.60.20attribute.20compiler-team.23881/near/522491140)). | ||
|
||
One weak argument is that these visibilities are supported by LLVM and Clang, so hopefully | ||
they would also make sense for Rust: | ||
|
||
* **LLVM**: Those visibilities are ultimately mapped from | ||
[`rustc_target`'s `SymbolVisibility`](https://github.com/rust-lang/rust/blob/81a964c23ea4fe9ab52b4449bb166bf280035797/compiler/rustc_target/src/spec/mod.rs#L839-L843), | ||
through | ||
[`rustc_middle`'s `Visibility`](https://github.com/rust-lang/rust/blob/81a964c23ea4fe9ab52b4449bb166bf280035797/compiler/rustc_middle/src/mir/mono.rs#L396-L407), | ||
and into | ||
[`rustc_codegen_llvm`'s `Visibility`](https://github.com/rust-lang/rust/blob/81a964c23ea4fe9ab52b4449bb166bf280035797/compiler/rustc_codegen_llvm/src/llvm/ffi.rs#L153-L160). | ||
So all the values make some sense at | ||
[the LLVM level](https://llvm.org/docs/LangRef.html#visibility-styles). | ||
* **Clang** and **GCC** support those 3 visibilities | ||
(see the "Parity with C++" subsection in the "Motivation" section above). | ||
|
||
OTOH, ideally we would somehow check what happens on some representative subset | ||
of target platforms (maybe: Posix, Windows, Wasm?): | ||
|
||
* TODO: what exactly do we want to verify on these target platforms? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can merge the RFC without any due diligence here. The RFC should demonstrate that the parameters it gives for each option can be implemented for at least our Tier 1 platforms, or specify where there are exceptions or particular unknowns.
|
||
* TODO: what exactly do we want to verify on these target platforms? | ||
|
||
## Rust standard library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this strikes me as a future possibility and not within the direct scope of the RFC. Do you agree? If so we should move it out of this section.
One problem here is that `llvm::Visibility::Default` is not sufficient to | ||
achieve actual interposability. https://crbug.com/418073233 has one example of | ||
undefined behavior, but even if DSO-local global data structures were not an | ||
issue, then LLVM-level assumptions could still lead to undefined behavior. | ||
This is because the LLVM optimization passes assume that a symbol with normal | ||
external linkage (not weak, odr, etc) the definition it can see is the | ||
definition that will be actually used. To avoid these LLVM assumptions `rustc` | ||
would have to enable | ||
[the SemanticInterposition feature](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-fsemantic-interposition). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no way to enable semantic interposition on a per-symbol basis? It seems like a stronger version of #[inline(never)]
would be close to what you want.
Otherwise it sounds like we can't implement "interposable" soundly without disabling a bunch of optimizations, so we should either remove it or mark the attribute unsafe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C compilers generally present it as a per-translation-unit flag but LLVM IR has a per-symbol distinction (dso_preemptable
vs dso_local
). That’s needed for LTO anyway.
I did another pass over the RFC and remembered a few things that need to be resolved before merging. I'm still positive on the RFC overall. @rfcbot concern specify how inlining interacts with hidden symbols
@rfcbot concern due diligence on tier 1 platforms
@rfcbot concern UB potential of interposable
|
This RFC proposes to add
#[export_visibility = …]
attribute, which seems like a reasonable way to address the following issues:#[no_mangle]
symbols are exported from acdylib
rust#98449This RFC complements the
-Zdefault-visibility=...
command-line flag, which is tracked in rust-lang/rust#131090This PR replaces the Major Change Proposal (MCP) at rust-lang/compiler-team#881
(/cc @bjorn3, @ChrisDenton, @chorman0773, @joshtriplett, @mati865, @workingjubilee, and @Urgau who have kindly provided feedback in the Zulip thread associated with that MCP)
/cc @tmandry from rust-lang/rust-project-goals#253, because one area where this RFC seems needed is FFI tooling
Rendered