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

Forbid the use of #[target_feature] on main #108651

Merged
merged 3 commits into from
Mar 12, 2023
Merged

Forbid the use of #[target_feature] on main #108651

merged 3 commits into from
Mar 12, 2023

Conversation

LeSeulArtichaut
Copy link
Contributor

Fixes #108645.

@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2023

r? @jackh726

(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 Mar 2, 2023
Noratrieb
Noratrieb approved these changes Mar 2, 2023
View reviewed changes
Copy link
Member

@Noratrieb Noratrieb left a comment

Choose a reason for hiding this comment

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

r? Nilstrieb
@bors delegate
r=me when CI is green

@rustbot rustbot assigned Noratrieb and unassigned jackh726 Mar 2, 2023
@Noratrieb
Copy link
Member

Uh, I don't think target_feature should be an only_local attribute.... that's unsound, isn't it? When it always pretends it isn't there across crates? Sounds like we need to encode it in the metadata (which is as simple as removing the @only_local: true)

@WaffleLapkin
Copy link
Member

WaffleLapkin commented Mar 2, 2023 •
edited
Loading

waffle rustc log analyzer

There are two failures:

  1. tests/ui/asm/x86_64/issue-89875.rs uses #[target_feature] on main (moving main's inside and #[target_feature] to a separate fn should fix the test)
  2. tests/ui/entry-point/imported_main_from_extern_crate.rs ICEs because the main function there is imported from another crate and the #[target_feature] attribute is local-only (probably an error?...)

@WaffleLapkin
Copy link
Member

Hm, target_feature was made local_only in #98450 as an optimization...

Ig it is fine, actually? Maybe? If the codegen only happens inside the crate, then target_feature being local is totally fine. At least we don't have any tests that tried to access target_feature in non-local crate...

@Noratrieb
Copy link
Member

This isn't unsound, the safeaty check probably goes through target feature queries instead of the attr.
So I guess you should try figuring out how that works and whether you can use the same way - although encoding target_feature is also a possible solution.

@LeSeulArtichaut
Copy link
Contributor Author

the safeaty check probably goes through target feature queries instead of the attr

Yes, see

rust/compiler/rustc_mir_transform/src/check_unsafety.rs

Lines 363 to 365 in 18caf88

let callee_features = &self.tcx.codegen_fn_attrs(func_did).target_features;
// The body might be a constant, so it doesn't have codegen attributes.
let self_features = &self.tcx.body_codegen_attrs(self.body_did.to_def_id()).target_features;

@LeSeulArtichaut
Copy link
Contributor Author

Also we need to forbid #[target_feature] on start, no?

@Noratrieb
Copy link
Member

Now it looks good and green 😊
@bors r+

@bors
Copy link
Contributor

bors commented Mar 2, 2023

📌 Commit dc6e0d9 has been approved by Nilstrieb

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 Mar 2, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Mar 2, 2023
@compiler-errors
Rollup merge of rust-lang#108651 - LeSeulArtichaut:108645-target-feat…
b5aec1c 
…ure-on-main, r=Nilstrieb

Forbid the use of `#[target_feature]` on `main`

Fixes rust-lang#108645.
@compiler-errors compiler-errors mentioned this pull request Mar 2, 2023
Closed
@Noratrieb
Copy link
Member

@bors r- this will fail after the revert, needs to add the unstable feature to the test

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 2, 2023
@LeSeulArtichaut
Copy link
Contributor Author

(I'll need to add the feature gates back if #108654 lands first)

@matthiaskrgr
Copy link
Member

failed in a rollup

https://github.com/rust-lang-ci/rust/actions/runs/4316566308/jobs/7532513148#step:26:17181

@compiler-errors
Copy link
Member

@bors r-

Fair chance this will land after #108654, so this needs blessing and fixing.

@LeSeulArtichaut LeSeulArtichaut force-pushed the 108645-target-feature-on-main branch 3 times, most recently from 1072bc8 to faebedf Compare March 3, 2023 07:05
@LeSeulArtichaut
Copy link
Contributor Author

Since #[target_feature] is allowed on safe functions (on stable), I guess we should allow #[target_feature] on main/start? It's sound and it would otherwise be a breaking change.

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@LeSeulArtichaut
Copy link
Contributor Author

(Committed a submodule change by accident, sorry for the noise)

@Noratrieb
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 11, 2023

📌 Commit 28dca77acf1d7280b51372abf3dd50c14616b628 has been approved by Nilstrieb

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 12, 2023
Closed
@bors
Copy link
Contributor

bors commented Mar 12, 2023

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

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 12, 2023
@Noratrieb
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 12, 2023

📌 Commit 29b1789 has been approved by Nilstrieb

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Mar 12, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2023
@bors
Auto merge of rust-lang#109056 - matthiaskrgr:rollup-9trny1z, r=matth…
7b4f489 
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#108651 (Forbid the use of `#[target_feature]` on `main`)
 - rust-lang#109009 (rustdoc: use restricted Damerau-Levenshtein distance for search)
 - rust-lang#109026 (Introduce `Rc::into_inner`, as a parallel to `Arc::into_inner`)
 - rust-lang#109029 (Gate usages of `dyn*` and const closures in macros)
 - rust-lang#109031 (Rename `config.toml.example` to `config.example.toml`)
 - rust-lang#109032 (Use `TyCtxt::trait_solver_next` in some places)
 - rust-lang#109047 (typo)
 - rust-lang#109052 (Add eslint check for rustdoc-gui tester)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f41796e into rust-lang:master Mar 12, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 12, 2023
@LeSeulArtichaut LeSeulArtichaut deleted the 108645-target-feature-on-main branch March 13, 2023 06:46
@ehuss ehuss mentioned this pull request Mar 14, 2023
Open
@est31 est31 mentioned this pull request Mar 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Noratrieb Noratrieb Noratrieb approved these changes

Assignees

@Noratrieb Noratrieb

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.70.0
Development

Successfully merging this pull request may close these issues.

#[target_feature] is allowed on main
8 participants
@LeSeulArtichaut @rustbot @Noratrieb @WaffleLapkin @bors @matthiaskrgr @compiler-errors @jackh726