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.

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

Add unstable -Zdefault-hidden-visibility cmdline flag for rustc. #118417

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

anforowicz
Copy link
Contributor

@anforowicz anforowicz commented Nov 28, 2023

The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656

@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2023

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

@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 Nov 28, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 28, 2023

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

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

@anforowicz
Copy link
Contributor Author

/cc @bridiver, @danakj

@rust-log-analyzer

This comment has been minimized.

@anforowicz
Copy link
Contributor Author

@rustbot label +S-waiting-on-author -S-waiting-on-review

I need to fix the test failures reported above. AFAICT they didn't show up when I've run ./x.py test, but I can indeed repro them with ./x.py test compiler/rustc_interface.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2023
@bridiver
Copy link

Thanks @anforowicz ! Not sure when I was ever going to finally get around to this if you didn't pick it up.

@anforowicz
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 28, 2023
@bridiver
Copy link

@TaKO8Ki do you have some time to look at this? It's going to solve some important issues for us (Brave/Chromium)

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Dec 7, 2023

@bridiver Sorry. I missed it. I'll take a look.

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Dec 8, 2023

Sorry for the late review! @bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 8, 2023

📌 Commit 1a0829c has been approved by TaKO8Ki

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 Dec 8, 2023
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 8, 2023
…ty, r=TaKO8Ki

Add unstable `-Zdefault-hidden-visibility` cmdline flag for `rustc`.

The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 8, 2023
…ty, r=TaKO8Ki

Add unstable `-Zdefault-hidden-visibility` cmdline flag for `rustc`.

The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 8, 2023
…ty, r=TaKO8Ki

Add unstable `-Zdefault-hidden-visibility` cmdline flag for `rustc`.

The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Dec 8, 2023
…ty, r=TaKO8Ki

Add unstable `-Zdefault-hidden-visibility` cmdline flag for `rustc`.

The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2023
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#118150 (Add new targets {x86_64,i686}-win7-windows-msvc)
 - rust-lang#118417 (Add unstable `-Zdefault-hidden-visibility` cmdline flag for `rustc`.)
 - rust-lang#118692 (remove redundant imports)
 - rust-lang#118694 (Add instance evaluation and methods to read an allocation in StableMIR)
 - rust-lang#118737 (Extend tidy alphabetical checking to `tests/`.)

r? `@ghost`
`@rustbot` modify labels: rollup
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Dec 13, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 13, 2023

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs and change-id in config.example.toml.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@anforowicz
Copy link
Contributor Author

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs and change-id in config.example.toml.

That was accidental and should be fixed now.

@anforowicz
Copy link
Contributor Author

@TaKO8Ki, could you PTAL again?

The only change is in tests/codegen/default-hidden-visibility.rs - in the latest revision the test only applies to only-linux and only-x86_64. This is sufficient to verify that the new command-line flag works and has the desirable effect. I think it's okay that the verification is only done on a single target, because the implementation is target-agnostic (and because test expectations differ on other targets - the outcome on those targets is still acceptable, but difficult to cover with a test).

@rustbot label -S-waiting-on-author +S-waiting-on-review


Let me also discuss some alternative testing approaches that I have considered for this PR.

If there is a desire to cover additional target triples (now or (preferably) in a separate follow-up PR), then:

  • One way to proceed is to duplicate the test file into additional only-X flavours, with different expectations (e.g. expecting internal constant instead of hidden constant - both are okay). If needed I can ask teammates with a Mac to generate test expectations for their target triple. Additional triples can also be covered, although I would have to lean on others to generate the test expectations.
  • Another way to proceed may be to mimic tests/codegen/align-byval.rs which selects target triples via [PREFIX] compile-flags: --target …. I have low confidence in my ability to follow this approach, because it fails on my machine for some triples - the test complains about missing matching standard library, but trying to build the library with ./x.py build library --target i686-unknown-linux-gnu fails saying: “/usr/bin/ld: skipping incompatible /usr/lib/gcc/x86_64-linux-gnu/13/libgcc.a when searching for -lgcc”.

Instead of per-triple test expectations, we could also tweak and relax the patterns being matched against (e.g. saying {{hidden|internal}} in the test expectations. This alternative feels less robust: the relaxed pattern may match accidentally and I have low confidence in my ability to construct a pattern that will work on all triples (given the previous failed attempts in this PR + given that IIUC tier3 targets may not immediately exercise the new test).

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 13, 2023
@anforowicz
Copy link
Contributor Author

Actually, I think it would be safest if I specified the full triple, because i686-unknown-linux-gnu passes for me locally, but we've already seen that i686, linux, musl combination has different outcome.

The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
@anforowicz
Copy link
Contributor Author

Actually, I think it would be safest if I specified the full triple, because i686-unknown-linux-gnu passes for me locally, but we've already seen that i686, linux, musl combination has different outcome.

Ok - switched to only-x86_64-unknown-linux-gnu...

@TaKO8Ki
Copy link
Member

TaKO8Ki commented Dec 14, 2023

@anforowicz

TaKO8Ki, could you PTAL again?

Sure. @bors r+

@bors
Copy link
Contributor

bors commented Dec 14, 2023

📌 Commit 981c4e3 has been approved by TaKO8Ki

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 Dec 14, 2023
@bors
Copy link
Contributor

bors commented Dec 14, 2023

⌛ Testing commit 981c4e3 with merge 9d49eb7...

@bors
Copy link
Contributor

bors commented Dec 14, 2023

☀️ Test successful - checks-actions
Approved by: TaKO8Ki
Pushing 9d49eb7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 14, 2023
@bors bors merged commit 9d49eb7 into rust-lang:master Dec 14, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 14, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9d49eb7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.7% [0.7%, 0.7%] 1
Regressions ❌
(secondary)
0.7% [0.7%, 0.7%] 3
Improvements ✅
(primary)
-0.7% [-0.8%, -0.6%] 2
Improvements ✅
(secondary)
-2.4% [-2.4%, -2.4%] 1
All ❌✅ (primary) -0.3% [-0.8%, 0.7%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.9% [-0.9%, -0.9%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.9% [-0.9%, -0.9%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 670.867s -> 670.776s (-0.01%)
Artifact size: 312.41 MiB -> 312.39 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants