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

No branch protection metadata unless enabled #93516

Merged
merged 1 commit into from
Feb 27, 2022
Merged

No branch protection metadata unless enabled #93516

merged 1 commit into from
Feb 27, 2022

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jan 31, 2022

Even if we emit metadata disabling branch protection, this metadata may
conflict with other modules (e.g. during LTO) that have different branch
protection metadata set.

This is an unstable flag and feature, so ideally the flag not being
specified should act as if the feature wasn't implemented in the first
place.

Additionally this PR also ensures we emit an error if
-Zbranch-protection is set on targets other than the supported
aarch64. For now the error is being output from codegen, but ideally it
should be moved to earlier in the pipeline before stabilization.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 31, 2022
@rust-highfive
Copy link
Collaborator

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 31, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot
Copy link
Contributor

cjgillot commented Feb 3, 2022

Could you also add a test that branch protection does not run when not explicitly enabled?
Isn't changing this default a breaking change compared to some accidental insta-stabilization?

@cjgillot cjgillot 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 Feb 11, 2022
@nagisa
Copy link
Member Author

nagisa commented Feb 16, 2022 •
edited
Loading

Could you also add a test that branch protection does not run when not explicitly enabled?

I think that should covered by the NONE revision of the branch-protection.rs test.

Isn't changing this default a breaking change compared to some accidental insta-stabilization?

What do you mean? branch-protection isn't stable there was never a way to enable it in a stable way. The module flags that were being generated without the flag were explicitly specifying to LLVM that the protection ought to be disabled.

@nagisa
Copy link
Member Author

nagisa commented Feb 16, 2022 •
edited
Loading

To add context, -Zbranch-protection was removed from beta in #93523, so today this functionality is nightly-only.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Feb 18, 2022

☔ The latest upstream changes (presumably #94121) made this pull request unmergeable. Please resolve the merge conflicts.

@nagisa nagisa added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 19, 2022
@nagisa
Copy link
Member Author

nagisa commented Feb 19, 2022

Nominating for beta since this missed the beta cut-off.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nagisa
No branch protection metadata unless enabled
b995dc9 
Even if we emit metadata disabling branch protection, this metadata may
conflict with other modules (e.g. during LTO) that have different branch
protection metadata set.

This is an unstable flag and feature, so ideally the flag not being
specified should act as if the feature wasn't implemented in the first
place.

Additionally this PR also ensures we emit an error if
`-Zbranch-protection` is set on targets other than the supported
aarch64. For now the error is being output from codegen, but ideally it
should be moved to earlier in the pipeline before stabilization.
@nagisa nagisa 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 Feb 19, 2022
@Mark-Simulacrum
Copy link
Member

(Note: I believe the beta being referenced here is 1.60, not 1.59 which is shipping later this week).

@apiraino
Copy link
Contributor

Beta backport accepted (pending landing on nightly) as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 24, 2022
@cjgillot
Copy link
Contributor

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Feb 26, 2022

📌 Commit b995dc9 has been approved by cjgillot

@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 Feb 26, 2022
@bors
Copy link
Contributor

bors commented Feb 26, 2022

⌛ Testing commit b995dc9 with merge 761e888...

@bors
Copy link
Contributor

bors commented Feb 27, 2022

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 761e888 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 27, 2022
@bors bors merged commit 761e888 into rust-lang:master Feb 27, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 27, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (761e888): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

ehuss pushed a commit to ehuss/rust that referenced this pull request Mar 3, 2022
@bors @ehuss
Auto merge of rust-lang#93516 - nagisa:branch-protection, r=cjgillot
7eda329 
No branch protection metadata unless enabled

Even if we emit metadata disabling branch protection, this metadata may
conflict with other modules (e.g. during LTO) that have different branch
protection metadata set.

This is an unstable flag and feature, so ideally the flag not being
specified should act as if the feature wasn't implemented in the first
place.

Additionally this PR also ensures we emit an error if
`-Zbranch-protection` is set on targets other than the supported
aarch64. For now the error is being output from codegen, but ideally it
should be moved to earlier in the pipeline before stabilization.
@ehuss ehuss mentioned this pull request Mar 3, 2022
Merged
@ehuss ehuss modified the milestones: 1.61.0, 1.60.0 Mar 3, 2022
@ehuss ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 3, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 3, 2022
@bors
Auto merge of rust-lang#94545 - ehuss:beta-backports, r=ehuss
c1d075a 
[beta] Beta backports

* No branch protection metadata unless enabled rust-lang#93516
* update auto trait lint for PhantomData rust-lang#94315
* Cargo:
    * [1.60] Fix term.verbose without quiet, and vice versa (rust-lang/cargo#10436)
    * [beta] Add common profile validation. (rust-lang/cargo#10413)
    * Avoid new deprecation warnings from clap 3.1.0 (rust-lang/cargo#10396)
@jacobbramley jacobbramley mentioned this pull request Dec 7, 2022
Merged
bors pushed a commit to rust-lang-ci/rust that referenced this pull request Dec 16, 2022
@jacobbramley
Check AArch64 branch-protection earlier in the pipeline.
49f3c0b 
As suggested in rust-lang#93516.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 17, 2022
@bors
Auto merge of rust-lang#105421 - jacobbramley:jb/branch-prot-check, r…
aef17b7 
…=nagisa

Check AArch64 branch-protection earlier in the pipeline.

As suggested in rust-lang#93516.

r? `@nagisa`
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#105421 - jacobbramley:jb/branch-prot-check, r…
0cdd816 
…=nagisa

Check AArch64 branch-protection earlier in the pipeline.

As suggested in rust-lang#93516.

r? `@nagisa`
@jacobbramley jacobbramley mentioned this pull request Jul 5, 2023
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobbramley jacobbramley jacobbramley left review comments

Assignees

@cjgillot cjgillot

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.