split `asm!` parsing and validation #140490

folkertdev · 2025-04-29T23:21:31Z

This PR splits asm! parsing and validation into two separate steps.

The parser constructs a Vec<RawAsmArg>, with each element corresponding to an argument to one of the asm! macros.
The validation then checks things like ordering of arguments or that options are not provided twice.

The motivation is #140279, which wants to add #[cfg(...)] support to these arguments. This support can now be added in a straightforward way by adding an attributes: ast::AttrVec field to RawAsmArg.

An extra reason for this split is that rustfmt probably wants to format the assembly at some point (currently that appears to be stubbed out, and the formatting is unstable rust-lang/style-team#152).

r? @ghost (just want to look at CI for now)

cc @ytmimi we discussed asm formatting a little while ago in rust-lang/rustfmt#6526. Am I correct in assuming that AsmArgs does not give enough information for formatting, but that RawAsmArgs would (it e.g. does not join information from multiple lines). This must have been an issue before?

folkertdev

r? @Amanieu (I suspect you're most familiar with this code).

I've left some inline comments to hopefully make reviewing this easier.

folkertdev · 2025-04-30T09:35:18Z

compiler/rustc_builtin_macros/src/asm.rs

+/// An argument to one of the `asm!` macros. The argument is syntactically valid, but is otherwise
+/// not validated at all.
+pub struct RawAsmArg {
+    pub kind: RawAsmArgKind,
+    pub span: Span,
+}
+
+pub enum RawAsmArgKind {
+    Template(P<ast::Expr>),
+    Operand(Option<Symbol>, ast::InlineAsmOperand),
+    Options(Vec<(Symbol, ast::InlineAsmOptions, Span, Span)>),
+    ClobberAbi(Vec<(Symbol, Span)>),
+}


The main idea of this PR is to parse into these data structures first, and then validate them in to the pre-existing AsmArgs.

folkertdev · 2025-04-30T09:35:54Z

compiler/rustc_builtin_macros/src/asm.rs

+    Ok(Some(if eat_operand_keyword(p, exp!(In), asm_macro)? {
+        let reg = parse_reg(p)?;
+        if p.eat_keyword(exp!(Underscore)) {
+            let err = dcx.create_err(errors::AsmUnderscoreInput { span: p.token.span });
+            return Err(err);
+        }


a messy diff, but this is just the old operand parsing code extracted into its own function. No changes were made here.

folkertdev · 2025-04-30T09:46:16Z

compiler/rustc_builtin_macros/src/asm.rs

+                    // NOTE: should this be handled during validation instead?
+                    if !asm_macro.is_supported_option(option) {
+                        // Tool-only output
+                        p.dcx().emit_err(errors::AsmUnsupportedOption {
+                            span,
+                            symbol: exp.kw,
+                            full_span,
+                            macro_name: asm_macro.macro_name(),
+                        });
+                    }


I'd like to emit this error later, but that changes some of the error messages. For example currently global_asm emits a (non-fatal) warning here when it encounters an unsupported option (e.g. noreturn) even if the remainder of its argument list causes a parse error. By emitting the unsupported option error later, no error is emitted if the argument causes a parse error.

I can add that, but not changing the error output at all in this PR seemed like a good idea to me.

It'd at least be worth adding a FIXME(..) then about this.

I just made the change in a separate commit. I think this is the right call, and the error message change is acceptable: the user is now no longer hitting a "this option is not supported for this flavor of asm" error when there is a parse error in the list of options. When the parse error is resolved, of course the unsupported error is emitted.

folkertdev · 2025-04-30T09:47:49Z

src/tools/rustfmt/src/parse/macros/asm.rs

 #[allow(dead_code)]
-pub(crate) fn parse_asm(context: &RewriteContext<'_>, mac: &ast::MacCall) -> Option<AsmArgs> {
+pub(crate) fn parse_asm(
+    context: &RewriteContext<'_>,
+    mac: &ast::MacCall,
+) -> Option<Vec<RawAsmArg>> {
    let ts = mac.args.tokens.clone();
    let mut parser = super::build_parser(context, ts);
-    parse_asm_args(&mut parser, mac.span(), ast::AsmMacro::Asm).ok()
+    parse_raw_asm_args(&mut parser, mac.span(), ast::AsmMacro::Asm).ok()
 }


this function is defined, but currently not used by rustfmt. I'm assuming that it will be if/when we start formatting the asm! operands, and that RawAsmArg can actually be used for that, while AsmArgs does not have enough information to faithfully reproduce the user's syntax.

rust-lang/rustfmt#5191, an old PR for asm! formatting, does a bunch of its own parsing that I think would not be needed with RawAsmArg.

Speaking with my t-style hat, we do not yet have any established formatting rules for asm!, just a general desire to do so with some fairly concrete thoughts on what those rules would look like.

Speaking with my t-rustfmt hat, this split does look like it'll make our lives much easier to handle asm! formatting whenever t-style does finalize the style rules for it, as it does seem to me this would allow us to avoid needing the custom parsing within rustfmt (though Yacin please correct me if I'm missing something)

rustbot · 2025-04-30T10:43:23Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

folkertdev · 2025-05-02T13:48:25Z

#140367, specifically the last commit (currently feddf14) shows how #[cfg(...)] can be built on top of this change.

traviscross

Some nits; seems OK overall to me. Along with the nits addressed and @Amanieu's review, r=me.

compiler/rustc_builtin_macros/src/asm.rs

traviscross · 2025-05-03T06:44:00Z

compiler/rustc_builtin_macros/src/asm.rs

+                    // NOTE: should this be handled during validation instead?
+                    if !asm_macro.is_supported_option(option) {
+                        // Tool-only output
+                        p.dcx().emit_err(errors::AsmUnsupportedOption {
+                            span,
+                            symbol: exp.kw,
+                            full_span,
+                            macro_name: asm_macro.macro_name(),
+                        });
+                    }


It'd at least be worth adding a FIXME(..) then about this.

compiler/rustc_builtin_macros/src/asm.rs

Amanieu · 2025-05-15T13:49:55Z

compiler/rustc_builtin_macros/src/asm.rs

-pub struct AsmArgs {
+/// An argument to one of the `asm!` macros. The argument is syntactically valid, but is otherwise
+/// not validated at all.
+pub struct RawAsmArg {


Could this be renamed to AsmArgs (and in other places too).

Amanieu · 2025-05-15T13:55:16Z

compiler/rustc_builtin_macros/src/asm.rs

+pub enum RawAsmArgKind {
+    Template(P<ast::Expr>),
+    Operand(Option<Symbol>, ast::InlineAsmOperand),
+    Options(Vec<(Symbol, ast::InlineAsmOptions, Span, Span)>),


Can this 4-tuple be replaced with a struct.

apparently it doesn't really use the asm parsing at present, so this may work?

Co-authored-by: Travis Cross <[email protected]>

Amanieu · 2025-05-17T22:15:39Z

r=me once CI passes

traviscross · 2025-05-18T01:12:12Z

@bors r=Amanieu,traviscross rollup

bors · 2025-05-18T01:12:15Z

📌 Commit d91e3d3 has been approved by Amanieu,traviscross

It is now in the queue for this repository.

…manieu,traviscross split `asm!` parsing and validation This PR splits `asm!` parsing and validation into two separate steps. The parser constructs a `Vec<RawAsmArg>`, with each element corresponding to an argument to one of the `asm!` macros. The validation then checks things like ordering of arguments or that options are not provided twice. The motivation is rust-lang#140279, which wants to add `#[cfg(...)]` support to these arguments. This support can now be added in a straightforward way by adding an `attributes: ast::AttrVec` field to `RawAsmArg`. An extra reason for this split is that `rustfmt` probably wants to format the assembly at some point (currently that appears to be stubbed out, and the formatting is unstable rust-lang/style-team#152). r? `@ghost` (just want to look at CI for now) cc `@ytmimi` we discussed asm formatting a little while ago in rust-lang/rustfmt#6526. Am I correct in assuming that `AsmArgs` does not give enough information for formatting, but that `RawAsmArgs` would (it e.g. does not join information from multiple lines). This must have been an issue before?

…manieu,traviscross split `asm!` parsing and validation This PR splits `asm!` parsing and validation into two separate steps. The parser constructs a `Vec<RawAsmArg>`, with each element corresponding to an argument to one of the `asm!` macros. The validation then checks things like ordering of arguments or that options are not provided twice. The motivation is rust-lang#140279, which wants to add `#[cfg(...)]` support to these arguments. This support can now be added in a straightforward way by adding an `attributes: ast::AttrVec` field to `RawAsmArg`. An extra reason for this split is that `rustfmt` probably wants to format the assembly at some point (currently that appears to be stubbed out, and the formatting is unstable rust-lang/style-team#152). r? ``@ghost`` (just want to look at CI for now) cc ``@ytmimi`` we discussed asm formatting a little while ago in rust-lang/rustfmt#6526. Am I correct in assuming that `AsmArgs` does not give enough information for formatting, but that `RawAsmArgs` would (it e.g. does not join information from multiple lines). This must have been an issue before?

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 Apr 29, 2025

This comment has been minimized.

Sign in to view

folkertdev force-pushed the asm-parser-changes branch 2 times, most recently from c53bbd4 to d30124a Compare April 30, 2025 09:33

folkertdev commented Apr 30, 2025

View reviewed changes

rustbot assigned Amanieu Apr 30, 2025

folkertdev marked this pull request as ready for review April 30, 2025 10:43

folkertdev mentioned this pull request Apr 30, 2025

add asm_cfg: #[cfg(...)] within asm! #140367

Open

traviscross approved these changes May 3, 2025

View reviewed changes

folkertdev force-pushed the asm-parser-changes branch 3 times, most recently from 6c50ded to 16880b2 Compare May 3, 2025 10:22

Amanieu reviewed May 15, 2025

View reviewed changes

folkertdev and others added 11 commits May 17, 2025 23:06

determine later whether an explicit reg was used

cd22c1b

extract operand parser

5af9652

a new parser generating the exact same error messages

614c51a

cleanup parse_clobber_abi

9191373

cleanup parse_options

d85ea45

cleanup unused code

3ca1a16

more cleanup

b6bc513

leave a note about maybe delaying an error message

f457e4d

attempt to have rustfmt use the new logic

d7eb15d

apparently it doesn't really use the asm parsing at present, so this may work?

Apply suggestions from code review

6e1bddc

Co-authored-by: Travis Cross <[email protected]>

delay error for unsupported options

1305fcc

folkertdev force-pushed the asm-parser-changes branch from 16880b2 to 2e788d1 Compare May 17, 2025 21:53

This comment has been minimized.

Sign in to view

rename to get rid of the 'raw' concept

8e86a0e

add AsmOptions with some named fields

d91e3d3

folkertdev force-pushed the asm-parser-changes branch from 2e788d1 to d91e3d3 Compare May 17, 2025 22:09

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 May 18, 2025

fmease mentioned this pull request May 18, 2025

Rollup of 8 pull requests #141207

Closed

fmease mentioned this pull request May 18, 2025

Rollup of 7 pull requests #141209

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split `asm!` parsing and validation #140490

split `asm!` parsing and validation #140490

folkertdev commented Apr 29, 2025

This comment has been minimized.

folkertdev left a comment

folkertdev Apr 30, 2025

folkertdev Apr 30, 2025

folkertdev Apr 30, 2025

traviscross May 3, 2025

folkertdev May 3, 2025

folkertdev Apr 30, 2025

folkertdev Apr 30, 2025

calebcartwright May 1, 2025

rustbot commented Apr 30, 2025

folkertdev commented May 2, 2025

traviscross left a comment •

edited

Loading

traviscross May 3, 2025

Amanieu May 15, 2025

Amanieu May 15, 2025

This comment has been minimized.

Amanieu commented May 17, 2025

traviscross commented May 18, 2025

bors commented May 18, 2025

split asm! parsing and validation #140490

Are you sure you want to change the base?

split asm! parsing and validation #140490

Conversation

folkertdev commented Apr 29, 2025

This comment has been minimized.

folkertdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustbot commented Apr 30, 2025

folkertdev commented May 2, 2025

traviscross left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Amanieu commented May 17, 2025

traviscross commented May 18, 2025

bors commented May 18, 2025

split `asm!` parsing and validation #140490

split `asm!` parsing and validation #140490

traviscross left a comment •

edited

Loading