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

Expose default_hidden_visibility as a rustc command line option #656

Closed
1 of 3 tasks
bridiver opened this issue Jul 18, 2023 · 13 comments
Closed
1 of 3 tasks

Expose default_hidden_visibility as a rustc command line option #656

bridiver opened this issue Jul 18, 2023 · 13 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@bridiver
Copy link

bridiver commented Jul 18, 2023

Proposal

I would like to expose default-hidden-visibility https://github.com/rust-lang/rust/blob/d3515155216e98c23440ea92c3f49c6a0d7101fc/compiler/rustc_target/src/spec/mod.rs#L1877C9-L1877C34 as a rustc unstable command line option. The problem we're trying to solve is that we have a mixed cpp/rust framework for mac/ios that needs to export symbols for various apis, but we do not want to expose all the core rust apis which are default visible. I have already built rust locally with the required changes and verified that setting default_hidden_visibility works as expected and the only exported symbols are the cpp symbols that are intended to be exported (no rust symbols are exported). There does not appear to be any other way to the equivalent of -fvisibility=hidden.

Mentors or Reviewers

@bridiver

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@bridiver bridiver added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Jul 18, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 18, 2023

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jul 18, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jul 20, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jul 20, 2023

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jul 20, 2023
@apiraino
Copy link
Contributor

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Aug 10, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 10, 2023
@danakj
Copy link

danakj commented Sep 6, 2023

Was there a PR for this, I am having trouble finding anything?

@bridiver
Copy link
Author

bridiver commented Sep 6, 2023

@danakj not yet, I've been swamped with other things. I should be able to get to it soon.

@anforowicz
Copy link

Do we want to expose default_hidden_visibility as -Zdefault-hidden-visibility VS as -Cdefault_hidden_visibility=yes (or =no)? The top comment talks about an "unstable command line option" which seems to indicate the -Z... flavour. OTOH, it seems that other similar options are passed as codegen options (e.g. to control LTO or to prefer-dynamic).

@bjorn3
Copy link
Member

bjorn3 commented Nov 16, 2023

New options are introduced as -Z first and moved to -C once they get stabilized.

@bridiver
Copy link
Author

@anforowicz see discussion here https://rust-lang.zulipchat.com/#narrow/stream/233931-xxx/topic/Expose.20default_hidden_visibility.20as.20a.20rus.E2.80.A6.20compiler-team.23656 where the decision was made to make it unstable. I guess it isn't really clear whether it would be -Zdefault-hidden-visibility or -Zdefault-hidden-visibility=yes|no so I guess I would defer to other options and follow the same pattern, but I don't know if any platforms currently default to hidden so yes|no might not be applicable here

@danakj
Copy link

danakj commented Nov 17, 2023

I believe MSVC defaults to hidden but also not sure you would want to make everything exported there. I guess you might, if you wanted to on posix, for whatever similar reasons.

@bridiver
Copy link
Author

@danakj I meant that in rust I don't think anything currently defaults to hidden.

anforowicz added a commit to anforowicz/rust that referenced this issue Nov 17, 2023
The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
anforowicz added a commit to anforowicz/rust that referenced this issue Nov 28, 2023
The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
anforowicz added a commit to anforowicz/rust that referenced this issue Nov 28, 2023
The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
anforowicz added a commit to anforowicz/rust that referenced this issue Nov 28, 2023
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 issue 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 issue 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 issue 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 issue 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
anforowicz added a commit to anforowicz/rust that referenced this issue Dec 8, 2023
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 issue Dec 11, 2023
…, r=<try>

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 issue Dec 11, 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 issue Dec 11, 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
anforowicz added a commit to anforowicz/rust that referenced this issue Dec 11, 2023
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 issue Dec 12, 2023
…, 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
anforowicz added a commit to anforowicz/rust that referenced this issue Dec 13, 2023
The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
anforowicz added a commit to anforowicz/rust that referenced this issue Dec 13, 2023
The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
anforowicz added a commit to anforowicz/rust that referenced this issue Dec 13, 2023
The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
anforowicz added a commit to anforowicz/rust that referenced this issue Dec 13, 2023
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 issue Dec 14, 2023
…, 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
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 15, 2023
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 pushed a commit to GuillaumeGomez/rustc_codegen_gcc that referenced this issue Feb 15, 2024
The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
GuillaumeGomez pushed a commit to GuillaumeGomez/rustc_codegen_gcc that referenced this issue Feb 20, 2024
The new flag has been described in the Major Change Proposal at
rust-lang/compiler-team#656
@chbaker0
Copy link

chbaker0 commented Apr 9, 2024

The command line flag should work differently than the original target spec field; see rust-lang/rust#123427

To avoid confusion, I think the flag proposed here should be renamed. Since the proposal was already accepted, how do I propose a rename?

@bridiver
Copy link
Author

bridiver commented Apr 9, 2024

The command line flag should work differently than the original target spec field; see rust-lang/rust#123427

To avoid confusion, I think the flag proposed here should be renamed. Since the proposal was already accepted, how do I propose a rename?

Wouldn't you run into the same issues if you created a custom macos target that had default hidden visibility? I'm fine changing the command line option name and functionality so it doesn't cause this problem, but it seems like the actual bug here is in the implementation of default hidden visibility for macos in general. I suspect since this is closed we probably have to open a new MCP, but not sure.

@chbaker0
Copy link

The target field was added because it was needed for WASM. I don't understand WASM, so I can't say for sure whether the behavior was intentional.

BTW this is an issue on Linux (and I assume Windows too), not just Mac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

8 participants