forked from rust-lang/rfcs
-
Notifications
You must be signed in to change notification settings - Fork 0
New unresolved question section: Rust standard library #1
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
Open
anforowicz
wants to merge
2
commits into
export-visibility
Choose a base branch
from
export-visibility--rustc_std_internal_symbol--take-1
base: export-visibility
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -531,6 +531,105 @@ of target platforms (maybe: Posix, Windows, Wasm?): | |
|
||
* TODO: what exactly do we want to verify on these target platforms? | ||
|
||
## Rust standard library | ||
|
||
### Problem description | ||
|
||
The scope of this RFC is currently limited to just introducing the | ||
`#[export_visibility = ...]` attribute. This should help realize the | ||
benefits described by this RFC wherever the new attribute is used | ||
(even if there remain places where the new attribute is not used). | ||
OTOH this means that this RFC treats | ||
visibility of Rust standard library symbols | ||
as out of scope. | ||
|
||
Currently Rust standard library may end up exporting two kinds of symbols. | ||
One kind is symbols using `#[rustc_std_internal_symbol]` attribute | ||
(similar to `#[no_mangle]` so in theory `#[export_visibility = ...]` | ||
attribute could be applied to such symbols). | ||
An example can be found below: | ||
|
||
``` | ||
$ git clone [email protected]:guidance-ai/llguidance.git | ||
$ cd llguidance/parser | ||
$ cargo rustc -- --crate-type=staticlib | ||
... | ||
$ nm --demangle --defined-only ../target/debug/libllguidance.a 2>/dev/null | grep __rustc:: | ||
0000000000000000 T __rustc::__rust_alloc | ||
0000000000000000 T __rustc::__rust_dealloc | ||
0000000000000000 T __rustc::__rust_realloc | ||
0000000000000000 T __rustc::__rust_alloc_zeroed | ||
0000000000000000 T __rustc::__rust_alloc_error_handler | ||
0000000000000000 B __rustc::__rust_alloc_error_handler_should_panic | ||
00000000 T __rustc::__rust_probestack | ||
``` | ||
|
||
But non-`#[rustc_std_internal_symbol]` symbols (e.g. | ||
[`String::new`](https://github.com/rust-lang/rust/blob/9c4ff566babe632af5e30281a822d1ae9972873b/library/alloc/src/string.rs#L439-L446)) | ||
can also end up publicly exported: | ||
|
||
``` | ||
$ nm --demangle --defined-only ../target/debug/libllguidance.a 2>/dev/null \ | ||
| grep alloc::string::String::new | ||
0000000000000000 T alloc::string::String::new | ||
0000000000000000 T alloc::string::String::new | ||
0000000000000000 T alloc::string::String::new | ||
0000000000000000 t alloc::string::String::new | ||
0000000000000000 T alloc::string::String::new | ||
0000000000000000 T alloc::string::String::new | ||
0000000000000000 T alloc::string::String::new | ||
``` | ||
|
||
> **Disclaimer**: The example above could be illustrated with other crates. | ||
> It uses `llguidance` because: | ||
> | ||
> 1. it exposes C API | ||
> (and therefore it is potentially useful to build it as a `staticlib`) | ||
> 2. it happens to be used by Chromium so the RFC author is somewhat familiar | ||
> with the crate | ||
> 3. the RFC author had trouble building `rustc-demangle-capi` in this way | ||
> (hitting `#[panic_handler]`-related errors). | ||
|
||
### Potential answers | ||
|
||
The following options have been identified so far as a potential way for | ||
hiding symbols coming from Rust standard library: | ||
|
||
* Do nothing. | ||
- Hiding symbols would require rebuilding Rust standard library with | ||
`-Zdefault-visibility=hidden`. | ||
- Note that there are other valid reasons | ||
for rebuilding the standard library when building a given project. | ||
For example this is a way to use globally consistent `-C` options | ||
like `-Cpanic=abort`, | ||
[`-Clto=no`](https://source.chromium.org/chromium/chromium/src/+/main:build/config/compiler/BUILD.gn;l=1115-1118;drc=26d51346374a0d16b0ba2243ef83c015a944d975), | ||
etc. | ||
- Rebuilding the standard library is possible, | ||
although it is currently supported as an **unstable** | ||
[`-Zbuild-std`](https://doc.rust-lang.org/cargo/reference/unstable.html#build-std) | ||
command-line flag of `cargo`. | ||
FWIW Chromium currently does rebuild the standard library | ||
(using automated | ||
[tooling](https://source.chromium.org/chromium/chromium/src/+/main:tools/rust/gnrt_stdlib.py;drc=628c608971bc01c96193055bb0848149cccde645) | ||
to translate standard library's `Cargo.toml` files into | ||
[equivalent `BUILD.gn` rules](https://source.chromium.org/chromium/chromium/src/+/main:build/rust/std/rules/BUILD.gn;drc=35fb76c686b55acc25b53f7e5c9b58e56dca7f4a)), | ||
which is one reason why this RFC is a viable UB fix for | ||
https://crbug.com/418073233. | ||
* Alternative: support in `rustc` emitting a `staticlib` with symbol visibility | ||
that matches `cdylib` behavior. | ||
- Initially this behavior would be gated behind a new unstable `-Z` flag. | ||
- Either a new flag: `-Zhide-rust-level-exports` | ||
- Or by ensuring that `-Zdefault-visibility=hidden` also applies | ||
to Rust-level exports transitively linked into a `staticlib` | ||
- Open question: should this behavior become the default in the future? | ||
- Implementation-wise, this behavior can be seen as partial linking or | ||
as object file rewriting. | ||
- It seems that this behavior makes most sense for `staticlib`s, because | ||
`rlib`s don't include transitive dependencies. So when linking `rlib`s | ||
(using an external, non-`rustc` linker) hiding standard library symbols | ||
would still require rebuilding it with `-Zdefault-visibility=hidden` | ||
as described in the "do nothing" alternative above. | ||
|
||
## Windows and `__declspec(dllexport)` | ||
[interposable-vs-dllexport]: #windows-and-__declspecdllexport | ||
|
||
|
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.
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.
The other option is to use partial linking or object file rewriting when building a staticlib to make the symbol visibility match cdylib.
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.
Thank you for thinking about and pointing out this alternative. I've tried to cover it in the text of the RFC. Please shout if I misunderstood or misrepresented anything. (In particular, I am not sure if I have a good grasp on how/if this alternative would affect handling of
rlib
s.)