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

Validate use of parameters in naked functions #79411

Merged
merged 1 commit into from
Nov 25, 2020
Merged

Validate use of parameters in naked functions #79411

merged 1 commit into from
Nov 25, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Nov 25, 2020 •
edited
Loading

  • Reject use of parameters inside naked function body.
  • Reject use of patterns inside function parameters, to emphasize role
    of parameters a signature declaration (mirroring existing behaviour
    for function declarations) and avoid generating code introducing
    specified bindings.

Closes issues below by considering input to be ill-formed.

Closes #75922.
Closes #77848.
Closes #79350.

@tmiasko
Validate use of parameters in naked functions
22d3431 
* Reject use of parameters inside naked function body.
* Reject use of patterns inside function parameters, to emphasize role
  of parameters a signature declaration (mirroring existing behaviour
  for function declarations) and avoid generating code introducing
  specified bindings.
@rustbot rustbot added A-naked Area: #[naked], prologue and epilogue-free, functions, https://git.io/vAzzS T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 25, 2020
@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Nov 25, 2020
@lcnr
Copy link
Contributor

lcnr commented Nov 25, 2020

impl looks good, don't know enough about how #[naked] should be used though

maybe r? @bjorn3 as they get recommended by github?

@rust-highfive rust-highfive assigned bjorn3 and unassigned lcnr Nov 25, 2020
@tmiasko
Copy link
Contributor Author

tmiasko commented Nov 25, 2020

The overall direction of this changes is to constrain the use of naked functions towards cases that can be reliably supported. The next step would be to permit only assembly inside the naked functions.

cc @joshtriplett @Amanieu

@bjorn3
Copy link
Member

bjorn3 commented Nov 25, 2020

@lcnr

maybe r? @bjorn3 as they get recommended by github?

I probably got recommended as I recently changed rustc_interface. I don't think I am a good reviewer for this. r? @Amanieu maybe?

@tmiasko

Reject use of parameters inside naked function body.

Maybe it should be allowed for asm! though I am not sure.

Reject use of patterns inside function parameters, to emphasize role
of parameters a signature declaration (mirroring existing behaviour
for function declarations) and avoid generating code introducing
specified bindings.

Makes sense.

@bjorn3 bjorn3 assigned Amanieu and unassigned bjorn3 Nov 25, 2020
@Amanieu
Copy link
Member

Amanieu commented Nov 25, 2020

Maybe it should be allowed for asm! though I am not sure.

No, you're supposed to access parameters directly through registers or the stack based on the function ABI.

@Amanieu
Copy link
Member

Amanieu commented Nov 25, 2020

Should we consider adding additional checks to this pass, such as the requirement that a naked function only contain a single asm! block with only const and sym operands?

@tmiasko
Copy link
Contributor Author

tmiasko commented Nov 25, 2020

Should we consider adding additional checks to this pass, such as the requirement that a naked function only contain a single asm! block with only const and sym operands?

Yes, I think we should have additional checks along those lines. I didn't think through all the details to propose one yet. There is also a question of compatibility with llvm_asm! which is often used together with core::intrinsics::unreachable(), e.g., code in compiler-builtins.

@Amanieu
Copy link
Member

Amanieu commented Nov 25, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Nov 25, 2020

📌 Commit 22d3431 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 Nov 25, 2020
@bors
Copy link
Contributor

bors commented Nov 25, 2020

⌛ Testing commit 22d3431 with merge b48cafd...

@bors
Copy link
Contributor

bors commented Nov 25, 2020

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing b48cafd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 25, 2020
@bors bors merged commit b48cafd into rust-lang:master Nov 25, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 25, 2020
@tmiasko tmiasko deleted the naked-params branch November 26, 2020 10:06
@tmiasko tmiasko mentioned this pull request Dec 3, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 7, 2020
@bors
Auto merge of rust-lang#79653 - tmiasko:naked-functions, r=Amanieu
bda05cc 
Validate naked functions definitions

Validate that naked functions are defined in terms of a single inline assembly
block that uses only `const` and `sym` operands and has `noreturn` option.

Implemented as future incompatibility lint with intention to migrate it into
hard error. When it becomes a hard error it will ensure that naked functions are
either unsafe or contain an unsafe block around the inline assembly. It will
guarantee that naked functions do not reference functions parameters (obsoleting
part of existing checks from rust-lang#79411). It will limit the definitions of naked
functions to what can be reliably supported. It will also reject naked functions
implemented using legacy LLVM style assembly since it cannot satisfy those
conditions.

rust-lang/rfcs#2774
rust-lang/rfcs#2972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Amanieu Amanieu

Labels
A-naked Area: #[naked], prologue and epilogue-free, functions, https://git.io/vAzzS 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-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
7 participants
@tmiasko @rust-highfive @lcnr @bjorn3 @Amanieu @bors @rustbot