Replace -Z default-hidden-visibility with -Z default-visibility by davidlattimore · Pull Request #130005 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits1 Checks6 Files changed
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
r? @pnkfelix
rustbot has assigned @pnkfelix.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
I contemplated if rather than two separate flags, we should merge -Z default-hidden-visibility and -Z default-protected-visibility and just have -Z default-visibility, which could then be set to one of hidden, protected or default. There were two reasons I didn't do that. (1) I didn't want to mess with an existing flag and risk breaking stuff and (2) I feel that long-term the ideal default for such a flag would be protected, however when one of the options is default, having that not be the default would be weird. It's unfortunate that when they designed visibility, they didn't call "default" by a better name, like "interposable" or "overridable".
This PR is partially based on a previous attempt by @bjorn3. The main thing it adds is making it a flag rather than always using protected visibility. It would have been nice if we could have skipped the flag and just always used protected, but unfortunately that's not an option while we still support GNU ld < 2.40, which presumably will be for quite a few years.
I wrote up my findings in this area in a blog post: https://davidlattimore.github.io/posts/2024/08/27/rust-dylib-rabbit-holes.html
My hope is that we can (in a separate PR) enable this new flag when LLD is being used as the linker.
I contemplated if rather than two separate flags, we should merge
-Z default-hidden-visibilityand-Z default-protected-visibilityand just have-Z default-visibility, which could then be set to one ofhidden,protectedordefault.
IMO we should do this. Having two non-independent flags instead of one risk having discrepancy in the handling, a enum on the other end would force handling at every usage. (For example your current PR doesn't error-out when setting both flags at the same time, while with one flag it wouldn't an issue).
Having one option the being default while not being the default is unfortunate, but not a blocker IMO, with good documentation it can be mitigated.
(btw, very exited to see experimentation is this area)
In anyway, given that this PR adds a new (unstable) flag it should first go through our MCP / Major Change Proposols. It's our is a lightweight form of RFC (it's very lightweight compared to RFC!). See the mini-guide on how to create one.
@rustbot labels -S-waiting-on-review +S-waiting-on-MCP
Status: PR has a compiler MCP and is waiting for the compiler MCP to complete.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
This might be a bit unexpected, so I'm posting it for your information.
This flag will also affect windows-gnu* targets because of llvm/llvm-project@c5b3de6.
So, it's not an ELF only flag.
Add experimental option -Z default-protected-visibility Replace -Z default-hidden-visibility with -Z default-visibility
MCP 782 has been accepted. I've updated this PR to replace default-hidden-visibility with default-visibility.
@rustbot labels +S-waiting-on-review -S-waiting-on-MCP
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-MCP
Status: PR has a compiler MCP and is waiting for the compiler MCP to complete.
labels
This comment has been minimized.
📌 Commit f48194e has been approved by Urgau
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
| This option only affects building of shared objects and should have no effect on executables. |
| Visibility an be set to one of three options: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Visibility an be set to one of three options: |
|---|
| Visibility can be set to one of three options: |
bors added a commit to rust-lang-ci/rust that referenced this pull request
…iaskrgr
Rollup of 4 pull requests
Successful merges:
- rust-lang#130005 (Replace -Z default-hidden-visibility with -Z default-visibility)
- rust-lang#130229 (ptr::add/sub: do not claim equivalence with
offset(c as isize)) - rust-lang#130773 (Update Unicode escapes in
/library/core/src/char/methods.rs) - rust-lang#130933 (rustdoc: lists items that contain multiple paragraphs are more clear)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#130005 - davidlattimore:protected-vis-flag, r=Urgau
Replace -Z default-hidden-visibility with -Z default-visibility
Issue rust-lang#105518
This was referenced
Oct 8, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…vis, r=Urgau
Use Default visibility for rustc-generated C symbol declarations
Non-default visibilities should only be used for definitions, not declarations, otherwise linking can fail.
This is based on rust-lang#123994.
Issue rust-lang#123427
When I changed default-hidden-visibility to default-visibility in rust-lang#130005, I updated all places in the code that used default-hidden-visibility, replicating the hidden-visibility bug to also happen for protected visibility.
Without this change, trying to build rustc with -Z default-visibility=protected fails with a link error.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#131519 - davidlattimore:intrinsics-default-vis, r=Urgau
Use Default visibility for rustc-generated C symbol declarations
Non-default visibilities should only be used for definitions, not declarations, otherwise linking can fail.
This is based on rust-lang#123994.
Issue rust-lang#123427
When I changed default-hidden-visibility to default-visibility in rust-lang#130005, I updated all places in the code that used default-hidden-visibility, replicating the hidden-visibility bug to also happen for protected visibility.
Without this change, trying to build rustc with -Z default-visibility=protected fails with a link error.
This was referenced
Oct 14, 2024
antoyo pushed a commit to antoyo/rust that referenced this pull request
…iaskrgr
Rollup of 4 pull requests
Successful merges:
- rust-lang#130005 (Replace -Z default-hidden-visibility with -Z default-visibility)
- rust-lang#130229 (ptr::add/sub: do not claim equivalence with
offset(c as isize)) - rust-lang#130773 (Update Unicode escapes in
/library/core/src/char/methods.rs) - rust-lang#130933 (rustdoc: lists items that contain multiple paragraphs are more clear)
r? @ghost
@rustbot modify labels: rollup
adalessandro added a commit to adalessandro/meta-browser that referenced this pull request
adalessandro added a commit to adalessandro/meta-browser that referenced this pull request
adalessandro added a commit to adalessandro/meta-browser that referenced this pull request
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.