Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replace -Z default-hidden-visibility with -Z default-visibility #130005

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Replace -Z default-hidden-visibility with -Z default-visibility #130005

merged 1 commit into from
Oct 1, 2024

Conversation

davidlattimore
Copy link
Contributor

Issue #105518

@rustbot
Copy link
Collaborator

rustbot commented Sep 5, 2024

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

@rustbot
Copy link
Collaborator

rustbot commented Sep 5, 2024

These commits modify compiler targets.
(See the Target Tier Policy.)

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 5, 2024
@davidlattimore
Copy link
Contributor Author

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.

@Urgau
Copy link
Member

Urgau commented Sep 6, 2024 •
edited
Loading

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.

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

@rustbot rustbot added 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 Sep 6, 2024
@davidlattimore davidlattimore mentioned this pull request Sep 6, 2024
Closed
3 tasks
@mati865
Copy link
Contributor

mati865 commented Sep 6, 2024 •
edited
Loading

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.

@davidlattimore davidlattimore force-pushed the protected-vis-flag branch 2 times, most recently from 5060ce8 to be96620 Compare September 30, 2024 23:24
@davidlattimore davidlattimore changed the title Add experimental option -Z default-protected-visibility Replace -Z default-hidden-visibility with -Z default-visibility Sep 30, 2024
@davidlattimore
Copy link
Contributor Author

MCP 782 has been accepted. I've updated this PR to replace default-hidden-visibility with default-visibility.

@davidlattimore
Copy link
Contributor Author

@rustbot labels +S-waiting-on-review -S-waiting-on-MCP

@rustbot rustbot added S-waiting-on-review 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 Oct 1, 2024
Urgau
Urgau reviewed Oct 1, 2024
View reviewed changes
compiler/rustc_codegen_llvm/src/llvm/ffi.rs Outdated Show resolved Hide resolved
src/doc/unstable-book/src/compiler-flags/default-visibility.md Outdated Show resolved Hide resolved
tests/codegen/default-visibility.rs Outdated Show resolved Hide resolved
@davidlattimore davidlattimore mentioned this pull request Oct 1, 2024
Open
4 tasks
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@davidlattimore @bjorn3
Replace -Z default-hidden-visibility with -Z default-visibility
f48194e 
MCP: rust-lang/compiler-team#782

Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
@Urgau
Copy link
Member

Urgau commented Oct 1, 2024

LGTM.

r? @Urgau
@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2024

📌 Commit f48194e has been approved by Urgau

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors 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 Oct 1, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 1, 2024
Merged
lqd
lqd reviewed Oct 1, 2024
View reviewed changes
src/doc/unstable-book/src/compiler-flags/default-visibility.md

This option only affects building of shared objects and should have no effect on executables.

Visibility an be set to one of three options:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 Oct 1, 2024
@bors
Auto merge of rust-lang#131111 - matthiaskrgr:rollup-n6do187, r=matth…
06bb836 
…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
@bors bors merged commit 389a399 into rust-lang:master Oct 1, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 1, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2024
@rust-timer
Unrolled build for rust-lang#130005
b6a6fdc 
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
Closed
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 11, 2024
@matthiaskrgr
Rollup merge of rust-lang#131519 - davidlattimore:intrinsics-default-…
33b1264 
…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 Oct 11, 2024
@rust-timer
Unrolled build for rust-lang#131519
032ef50 
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
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqd lqd lqd left review comments

@Urgau Urgau Urgau approved these changes

Assignees

@Urgau Urgau

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
8 participants
@davidlattimore @rustbot @Urgau @mati865 @rust-log-analyzer @bors @lqd @pnkfelix