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

folkertdev · 2025-04-27T14:11:54Z

tracking issue: #140364
blocked on: #140490

This feature was discussed in #140279. It allows configuring templates and operands in the assembly macros, for example:

asm!( // or global_asm! or naked_asm!
    "nop",
    #[cfg(target_feature = "sse2")]
    "nop",
    // ...
    #[cfg(target_feature = "sse2")]
    a = const 123, // only used on sse2
);

r? @tgross35

cc @traviscross @Amanieu

Now builds on #140490, which should be merged first.

folkertdev · 2025-04-27T14:14:12Z

compiler/rustc_builtin_macros/src/asm.rs

+        let is_configured_out =
+            ecx.ecfg.features.asm_cfg() && strip_unconfigured.configure(attributes).is_none();
+
+        let template = p.parse_expr()?;


The fact that we use parse_expr here mean that this does not work, and does not produce a great error message. This complains about an unexpected , after the 6, but the real problem is that this cannot possibly be or expand to a string literal.

asm!( #[cfg(false)] a = const 6, "nop", );

Maybe we should specialize the parser to specifically only parse string literals and item macros?

folkertdev · 2025-04-27T14:16:26Z

compiler/rustc_builtin_macros/src/asm.rs

    if p.token == token::Eof {
        return Err(dcx.create_err(errors::AsmRequiresTemplate { span: sp }));
    }

-    let first_template = p.parse_expr()?;
+    let first_template = loop {
+        let attributes = AsmAttrVec::parse(ecx, p)?;


attributes are now parsed before the parse_expr below, and never attached to the expression. In theory that could interact with #[feature(stmt_expr_attributes)], but:

that feature is unstable

any attributes it parsed would have no effect

folkertdev · 2025-04-27T14:17:44Z

compiler/rustc_builtin_macros/src/asm.rs

+                _ => {
+                    ecx.dcx().emit_err(errors::AsmAttributeNotSupported { span: attr.span() });
+                }


non-cfg attributes are now accepted by the parser, but emit this error. Previously attributes were only parsed (and later rejected, unless #[feature(stmt_expr_attributes)] was enabled) on expressions. We now also always parse them on operands.

folkertdev · 2025-04-27T14:47:18Z

Hmm, rust analyzer also parses the assembly, but from what I can see it has no way of evaluating the cfgs while parsing. Maybe that can change, but if not we'll have to fundamentally change the parser to just parse, and then later have a validation pass. I tried that before and it's possible but messy, and you need to store a lot more information in AsmArgs (that rust-analyzer then would also have to process).

@rust-lang/rust-analyzer how can we make this work?

ChayimFriedman2 · 2025-04-27T17:26:44Z

(I haven't looked at the PR, but) parsing does not evaluated cfgs. Hir lowering does.

folkertdev · 2025-04-27T17:57:04Z

Well in any case the parser doesn't. I'm not exactly sure where to draw the line for when HIR lowering starts, but certainly macro expansion of builtin macros is able to do it, e.g. here for cfg!:

rust/compiler/rustc_builtin_macros/src/cfg.rs

Lines 24 to 30 in 267cae5

    
           let matches_cfg = attr::cfg_matches( 
        
               &cfg, 
        
               &cx.sess, 
        
               cx.current_expansion.lint_node_id, 
        
               Some(cx.ecfg.features), 
        
           ); 
        
           MacEager::expr(cx.expr_bool(sp, matches_cfg))

Anyway, are you saying that rust-analyzer just cannot evaluate cfg's while parsing the asm! macro? We can delay it, but I think that just means that rust-analyzer would have to duplicate the logic.

Veykril · 2025-04-28T04:19:26Z

rust-analyzer has to duplicate these changes either way. We don't use any of the touched rustc crates here.

folkertdev · 2025-04-30T10:45:09Z

I got confused between rust-analyzer and rustfmt here, sorry about that.

I've also opened #140490, which will make adding the cfg support a lot easier ( have that working on a branch). So for now:

@rustbot author

rustbot · 2025-04-30T10:45:13Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

tgross35 · 2025-05-02T04:51:55Z

I'm still take a look through this, but I don't know enough about this area of the compiler to give a good review. Picking somebody who shows up in the blame a lot:

r? @nnethercote

rustbot · 2025-05-02T13:44:09Z

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

traviscross

@rustbot author

compiler/rustc_builtin_macros/src/asm.rs

tests/ui/asm/cfg-parse-error.rs

tests/ui/asm/cfg.rs

tests/ui/asm/cfg-parse-error.rs

folkertdev · 2025-05-15T18:07:06Z

Cc @jdonszelmann

bors · 2025-05-18T20:40:01Z

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

folkertdev

At rustweek we discussed moving the asm parser to rustc_parse. I've now made that change and think it's the right call here:

we don't need to expose attribute parsing logic or hand-roll our own attribute parser
rustfmt was able to drop the dependency on rustc_builtin_macros

@rustbot ready

tests/ui/asm/cfg-parse-error.rs

nnethercote · 2025-05-27T01:01:40Z

tests/ui/asm/cfg.rs

+}
+
+fn options() {
+    // Without the cfg, this throws an error that the `noreturn` option is provided twice.


Comment is out of date? I don't see noreturn in the code.

nnethercote · 2025-05-27T01:07:42Z

Looks good, r=me. One nit in a test comment.

About the moving of the code to rustc_parse: I see that it removed rustfmt's dependency on rustc_builtin_macros, which is nice. Were there any other benefits? E.g. did anything pub become private? (It's hard to tell from the diff.)

@bors delegate=folkertdev

bors · 2025-05-27T01:07:45Z

✌️ @folkertdev, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

nnethercote · 2025-05-27T04:21:42Z

About the moving of the code to rustc_parse: I see that it removed rustfmt's dependency on rustc_builtin_macros, which is nice. Were there any other benefits? E.g. did anything pub become private? (It's hard to tell from the diff.)

Ah, I see you wrote "we don't need to expose attribute parsing logic or hand-roll our own attribute parser" above. 👍

folkertdev · 2025-05-27T09:35:41Z

Thanks everyone!

@bors r=@nnethercote

bors · 2025-05-27T09:35:43Z

📌 Commit c7c0194 has been approved by nnethercote

It is now in the queue for this repository.

rustbot assigned tgross35 Apr 27, 2025

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 27, 2025

folkertdev commented Apr 27, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot 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 Apr 30, 2025

rustbot assigned nnethercote and unassigned tgross35 May 2, 2025

folkertdev force-pushed the asm-cfg branch from 5cfd71a to feddf14 Compare May 2, 2025 13:44

folkertdev mentioned this pull request May 2, 2025

split asm! parsing and validation #140490

Merged

This comment has been minimized.

Sign in to view

folkertdev force-pushed the asm-cfg branch from feddf14 to 4c15900 Compare May 2, 2025 14:18

This comment has been minimized.

Sign in to view

folkertdev force-pushed the asm-cfg branch from 4c15900 to 6cc60a6 Compare May 2, 2025 14:47

traviscross reviewed May 3, 2025

View reviewed changes

compiler/rustc_builtin_macros/src/asm.rs Outdated Show resolved Hide resolved

tests/ui/asm/cfg-parse-error.rs Outdated Show resolved Hide resolved

tests/ui/asm/cfg.rs Outdated Show resolved Hide resolved

traviscross mentioned this pull request May 6, 2025

Tracking issue for cfg_match #115585

Open

3 tasks

Amanieu reviewed May 15, 2025

View reviewed changes

tests/ui/asm/cfg-parse-error.rs Show resolved Hide resolved

folkertdev force-pushed the asm-cfg branch 2 times, most recently from 5f856d4 to 0e567bd Compare May 18, 2025 20:47

This comment has been minimized.

Sign in to view

folkertdev force-pushed the asm-cfg branch from 0e567bd to c4ea711 Compare May 18, 2025 21:24

folkertdev commented May 18, 2025

View reviewed changes

tests/ui/asm/cfg-parse-error.rs Show resolved Hide resolved

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2025

nnethercote reviewed May 27, 2025

View reviewed changes

nnethercote 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 May 27, 2025

folkertdev added 2 commits May 27, 2025 09:44

support #[cfg(...)] on arguments to the asm! macros

e3bbbee

move asm parsing code into rustc_parse

c7c0194

folkertdev force-pushed the asm-cfg branch from c4ea711 to c7c0194 Compare May 27, 2025 07:49

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

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

Are you sure you want to change the base?

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

Conversation

folkertdev commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

folkertdev Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

folkertdev Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

folkertdev Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

folkertdev commented Apr 27, 2025

Uh oh!

ChayimFriedman2 commented Apr 27, 2025

Uh oh!

folkertdev commented Apr 27, 2025

Uh oh!

Veykril commented Apr 28, 2025

Uh oh!

folkertdev commented Apr 30, 2025

Uh oh!

rustbot commented Apr 30, 2025

Uh oh!

tgross35 commented May 2, 2025

Uh oh!

rustbot commented May 2, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

traviscross left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

folkertdev commented May 15, 2025

Uh oh!

bors commented May 18, 2025

Uh oh!

This comment has been minimized.

folkertdev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nnethercote May 27, 2025

Choose a reason for hiding this comment

Uh oh!

nnethercote commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented May 27, 2025

Uh oh!

nnethercote commented May 27, 2025

Uh oh!

folkertdev commented May 27, 2025

Uh oh!

bors commented May 27, 2025

Uh oh!

Uh oh!

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

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

folkertdev commented Apr 27, 2025 •

edited

Loading

nnethercote commented May 27, 2025 •

edited

Loading