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

Reject unsupported naked functions #93153

Merged
merged 2 commits into from
Jan 22, 2022
Merged

Reject unsupported naked functions #93153

merged 2 commits into from
Jan 22, 2022

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jan 21, 2022

Transition unsupported naked functions future incompatibility lint into an error:

Closes #32490.
Closes #32489.

r? @Amanieu @npmccallum @joshtriplett

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 21, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2022
@tmiasko tmiasko added A-inline-assembly Area: Inline assembly (`asm!(…)`) A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS F-naked_functions `#![feature(naked_functions)]` labels Jan 21, 2022
@ds84182
Copy link

ds84182 commented Jan 21, 2022 •
edited
Loading

Is there a reason why naked functions are only constrained to a single inline assembly block? This limits the ability to stitch multiple pieces of inline assembly together.

Edit: To be a bit clearer/on topic, this affects older projects people may have sitting around. The current error messages give zero context around the new constraints for naked functions, other than the new requirement. In particular, the single block restriction is extremely vague and seems like it should work with multiple blocks.

@tmiasko tmiasko force-pushed the reject-unsupported-naked-functions branch from ee0ef5f to a6c2599 Compare January 21, 2022 11:31
@npmccallum
Copy link
Contributor

Is there a reason why naked functions are only constrained to a single inline assembly block?

It is harder to lint all the invariants across multiple blocks.

It is also less obvious to read and comprehend. And therefore it is a likely source of bugs.

A cleaner method if you need this is separate functions.

@tmiasko
Copy link
Contributor Author

tmiasko commented Jan 21, 2022

Is there a reason why naked functions are only constrained to a single inline assembly block? This limits the ability to stitch multiple pieces of inline assembly together.

It is possible to stitch multiple pieces of inline assembly together using macros, rust-lang/rfcs#2972 (comment) contains an example.

The current error messages give zero context around the new constraints for naked functions, other than the new requirement. In particular, the single block restriction is extremely vague and seems like it should work with multiple blocks.

Regarding the missing context in error messages, I introduced a new error code, so that rustc --explain E0787 can be used to obtain additional details about the restrictions. Though as far as the single block restriction is concerned, I don't think there is a fundamental reason for it.

@tmiasko
Reject unsupported naked functions
888332f 
Transition unsupported naked functions future incompatibility lint into
an error:

* Naked functions must contain a single inline assembly block.
  Introduced as future incompatibility lint in 1.50 rust-lang#79653.
  Change into an error fixes a soundness issue described in rust-lang#32489.

* Naked functions must not use any forms of inline attribute.
  Introduced as future incompatibility lint in 1.56 rust-lang#87652.
@tmiasko
Reject may_unwind option in naked functions
beeba4b
@tmiasko tmiasko force-pushed the reject-unsupported-naked-functions branch 3 times, most recently from eeab749 to beeba4b Compare January 21, 2022 16:54
@Amanieu
Copy link
Member

Amanieu commented Jan 21, 2022

What's up with the CI?

@Amanieu
Copy link
Member

Amanieu commented Jan 21, 2022

Seems to be a general GHA issue.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 21, 2022

📌 Commit beeba4b has been approved by Amanieu

@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 Jan 21, 2022
@ds84182
Copy link

ds84182 commented Jan 22, 2022

Regarding the missing context in error messages, I introduced a new error code, so that rustc --explain E0787 can be used to obtain additional details about the restrictions. Though as far as the single block restriction is concerned, I don't think there is a fundamental reason for it.

Ah, I missed the added explanation. I'll file a bug for allowing multiple blocks, since the behavior prior to this PR is well-defined and the current workaround is subpar.

@ds84182 ds84182 mentioned this pull request Jan 22, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
@matthiaskrgr
Rollup merge of rust-lang#93153 - tmiasko:reject-unsupported-naked-fu…
86b4a3e 
…nctions, r=Amanieu

Reject unsupported naked functions

Transition unsupported naked functions future incompatibility lint into an error:

* Naked functions must contain a single inline assembly block. Introduced as future incompatibility lint in 1.50 rust-lang#79653. Change into an error fixes a soundness issue described in rust-lang#32489.

* Naked functions must not use any forms of inline attribute. Introduced as future incompatibility lint in 1.56 rust-lang#87652.

Closes rust-lang#32490.
Closes rust-lang#32489.

r? `@Amanieu` `@npmccallum` `@joshtriplett`
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 22, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2022
@matthiaskrgr
Rollup merge of rust-lang#93153 - tmiasko:reject-unsupported-naked-fu…
7d624e1 
…nctions, r=Amanieu

Reject unsupported naked functions

Transition unsupported naked functions future incompatibility lint into an error:

* Naked functions must contain a single inline assembly block. Introduced as future incompatibility lint in 1.50 rust-lang#79653. Change into an error fixes a soundness issue described in rust-lang#32489.

* Naked functions must not use any forms of inline attribute. Introduced as future incompatibility lint in 1.56 rust-lang#87652.

Closes rust-lang#32490.
Closes rust-lang#32489.

r? ``@Amanieu`` ``@npmccallum`` ``@joshtriplett``
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 22, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 22, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2022
@bors
Auto merge of rust-lang#93202 - matthiaskrgr:rollup-rki39xg, r=matthi…
bfe1564 
…askrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#85967 (add support for the l4-bender linker on the x86_64-unknown-l4re-uclibc tier 3 target)
 - rust-lang#92828 (Print a helpful message if unwinding aborts when it reaches a nounwind function)
 - rust-lang#93012 (Update pulldown-cmark version to fix markdown list issue)
 - rust-lang#93116 (Simplify use of `map_or`)
 - rust-lang#93132 (Increase the format version of rustdoc-json-types)
 - rust-lang#93147 (Interner cleanups)
 - rust-lang#93153 (Reject unsupported naked functions)
 - rust-lang#93170 (Add missing GUI test explanations)
 - rust-lang#93172 (rustdoc: remove dashed underline under main heading)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a8f64c0 into rust-lang:master Jan 22, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 22, 2022
@tmiasko tmiasko deleted the reject-unsupported-naked-functions branch January 22, 2022 22:16
@bstrie bstrie mentioned this pull request Feb 2, 2022
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npmccallum npmccallum npmccallum left review comments

@Amanieu Amanieu Amanieu left review comments

@joshtriplett joshtriplett joshtriplett left review comments

Assignees

@Amanieu Amanieu

Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) A-naked Area: `#[naked]`, prologue and epilogue-free, functions, https://git.io/vAzzS F-naked_functions `#![feature(naked_functions)]` 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.

This naked function SIGSEGVs Naked functions do not require unsafety
8 participants
@tmiasko @ds84182 @npmccallum @Amanieu @bors @joshtriplett @rust-highfive @rustbot