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

Make asm label blocks safe context #131544

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Make asm label blocks safe context #131544

merged 1 commit into from
Nov 21, 2024
+78 −2

Conversation

nbdd0121
Copy link
Contributor

Tracking issue: #119364

asm!() is forced to be wrapped inside unsafe. If there's no special treatment, the label blocks would also always be unsafe with no way of opting out. It was suggested that a simple fix is to make asm label blocks safe: #119364 (comment).

@rustbot labels: +A-inline-assembly +F-asm

@nbdd0121
Make asm label blocks safe context
809dc73 
`asm!()` is forced to be wrapped inside unsafe. If there's no special
treatment, the label blocks would also always be unsafe with no way
of opting out.
@rustbot
Copy link
Collaborator

rustbot commented Oct 11, 2024

r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) labels Oct 11, 2024
petrochenkov
petrochenkov reviewed Oct 19, 2024
View reviewed changes
compiler/rustc_mir_build/src/check_unsafety.rs
Out { expr: None, reg: _, late: _ }
| Const { value: _, span: _ }
| SymFn { value: _, span: _ }
| SymStatic { def_id: _ } => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

The default visitor uses visit_anon_const and visit_qpath in this place, don't you need them here?

To avoid potential mistakes, it probably makes sense to add a public fn walk_inline_asm_operand helper to intravisit.rs.

match op {
  Label { .. } => self.in_safety_context(SafetyContext::Safe, walk_inline_asm_operand(...)),
  _ => walk_inline_asm_operand(...),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the THIR visitor which doesn't have visit_anon_const and visit_qpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ping @compiler-errors

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, pinged wrong person. @petrochenkov

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I never noticed the update, it's better to set the S-waiting-on-review label if a PR is waiting on review.

If this part cannot be improved, then it's good to go.

@petrochenkov petrochenkov 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 Oct 19, 2024
@nikomatsakis nikomatsakis mentioned this pull request Oct 26, 2024
Open
26 tasks
@ojeda ojeda mentioned this pull request Nov 8, 2024
Open
91 tasks
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 21, 2024

📌 Commit 809dc73 has been approved by petrochenkov

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 Nov 21, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 21, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2024
@bors
Auto merge of rust-lang#133287 - matthiaskrgr:rollup-ab9j3pu, r=matth…
75703c1 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#130236 (unstable feature usage metrics)
 - rust-lang#131544 (Make asm label blocks safe context)
 - rust-lang#131586 (Support s390x z13 vector ABI)
 - rust-lang#132489 (Fix closure arg extraction in `extract_callable_info`, generalize it to async closures)
 - rust-lang#133078 (tests: ui/inline-consts: add issue number to a test, rename other tests)
 - rust-lang#133283 (Don't exclude relnotes from `needs-triage` label)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3956495 into rust-lang:master Nov 21, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2024
@rust-timer
Unrolled build for rust-lang#131544
4b51c44 
Rollup merge of rust-lang#131544 - nbdd0121:asm_goto_safe_block, r=petrochenkov

Make asm label blocks safe context

Tracking issue: rust-lang#119364

`asm!()` is forced to be wrapped inside unsafe. If there's no special treatment, the label blocks would also always be unsafe with no way of opting out. It was suggested that a simple fix is to make asm label blocks safe: rust-lang#119364 (comment).

`@rustbot` labels: +A-inline-assembly +F-asm
@nbdd0121 nbdd0121 mentioned this pull request Nov 21, 2024
Closed
@nbdd0121 nbdd0121 deleted the asm_goto_safe_block branch November 21, 2024 21:29
@nbdd0121 nbdd0121 mentioned this pull request Dec 4, 2024
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 17, 2025
@matthiaskrgr
Rollup merge of rust-lang#133870 - nbdd0121:asm, r=traviscross,nnethe…
3d3f817 
…rcote

Stabilize `asm_goto` feature gate

Stabilize `asm_goto` feature (tracked by rust-lang#119364). The issue will remain open and be updated to track `asm_goto_with_outputs`.

Reference PR: rust-lang/reference#1693

# Stabilization Report

This feature adds a `label <block>` operand type to `asm!`. `<block>` must be a block expression with type unit or never. The address of the block is substituted and the assembly may jump to the block. When block completes the `asm!` block returns and continues execution.

The block starts a new safety context and unsafe operations within must have additional `unsafe`s; the effect of `unsafe` that surrounds `asm!` block is cancelled. See rust-lang#119364 (comment) and rust-lang#131544.

It's currently forbidden to use `asm_goto` with output operands; that is still unstable under `asm_goto_with_outputs`.

Example:

```rust
unsafe {
    asm!(
        "jmp {}",
        label {
            println!("Jumped from asm!");
        }
    );
}
```

Tests:
- tests/ui/asm/x86_64/goto.rs
- tests/ui/asm/x86_64/goto-block-safe.stderr
- tests/ui/asm/x86_64/bad-options.rs
- tests/codegen/asm/goto.rs
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2025
@rust-timer
Unrolled build for rust-lang#133870
b4ca2de 
Rollup merge of rust-lang#133870 - nbdd0121:asm, r=traviscross,nnethercote

Stabilize `asm_goto` feature gate

Stabilize `asm_goto` feature (tracked by rust-lang#119364). The issue will remain open and be updated to track `asm_goto_with_outputs`.

Reference PR: rust-lang/reference#1693

# Stabilization Report

This feature adds a `label <block>` operand type to `asm!`. `<block>` must be a block expression with type unit or never. The address of the block is substituted and the assembly may jump to the block. When block completes the `asm!` block returns and continues execution.

The block starts a new safety context and unsafe operations within must have additional `unsafe`s; the effect of `unsafe` that surrounds `asm!` block is cancelled. See rust-lang#119364 (comment) and rust-lang#131544.

It's currently forbidden to use `asm_goto` with output operands; that is still unstable under `asm_goto_with_outputs`.

Example:

```rust
unsafe {
    asm!(
        "jmp {}",
        label {
            println!("Jumped from asm!");
        }
    );
}
```

Tests:
- tests/ui/asm/x86_64/goto.rs
- tests/ui/asm/x86_64/goto-block-safe.stderr
- tests/ui/asm/x86_64/bad-options.rs
- tests/codegen/asm/goto.rs
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 20, 2025
@matthiaskrgr
Rollup merge of #133870 - nbdd0121:asm, r=traviscross,nnethercote
f004139 
Stabilize `asm_goto` feature gate

Stabilize `asm_goto` feature (tracked by #119364). The issue will remain open and be updated to track `asm_goto_with_outputs`.

Reference PR: rust-lang/reference#1693

# Stabilization Report

This feature adds a `label <block>` operand type to `asm!`. `<block>` must be a block expression with type unit or never. The address of the block is substituted and the assembly may jump to the block. When block completes the `asm!` block returns and continues execution.

The block starts a new safety context and unsafe operations within must have additional `unsafe`s; the effect of `unsafe` that surrounds `asm!` block is cancelled. See rust-lang/rust#119364 (comment) and rust-lang/rust#131544.

It's currently forbidden to use `asm_goto` with output operands; that is still unstable under `asm_goto_with_outputs`.

Example:

```rust
unsafe {
    asm!(
        "jmp {}",
        label {
            println!("Jumped from asm!");
        }
    );
}
```

Tests:
- tests/ui/asm/x86_64/goto.rs
- tests/ui/asm/x86_64/goto-block-safe.stderr
- tests/ui/asm/x86_64/bad-options.rs
- tests/codegen/asm/goto.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov

Assignees

@petrochenkov petrochenkov

Labels
A-inline-assembly Area: Inline assembly (`asm!(…)`) F-asm `#![feature(asm)]` (not `llvm_asm`) 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.84.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nbdd0121 @rustbot @petrochenkov @bors