Stabilize asm_cfg

[folkertdev]

tracking issue: #140364
closes #140364

Reference PR:

• document cfg conditions on inline assembly templates and operands reference#2063

Request for Stabilization

Summary

The cfg_asm feature allows #[cfg(...)] and #[cfg_attr(...)] on the arguments of the assembly macros, for instance:

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

Semantics

Templates, operands, options and clobber_abi in the assembly macros (asm!, naked_asm! and global_asm!) can be annotated with #[cfg(...)] and #[cfg_attr(...)]. When the condition evaluates to true, the annotated argument has no effect, and is completely ignored when expanding the assembly macro.

Documentation

reference PR: rust-lang/reference#2063

Tests

• tests/ui/asm/cfg.rs checks that cfg'd arguments where the condition evaluates to false have no effect
• tests/ui/asm/cfg-parse-error.rs checks the parsing rules (parsing effectively assumes that the cfg conditions are all true)

History

• Lang proposal: Allow #[cfg(...)] within asm! #140279
• add asm_cfg: #[cfg(...)] within asm! #140367

Resolved questions

how are other attributes handled

Other attributes are parsed, but explicitly rejected.

unresolved questions

operand before template

The current implementation expects at least one template string before any operands. In the example below, if the cfg condition evaluates to true, the assembly block is ill-formed. But even when it evaluates to false this block is rejected, because the parser still expects just a template (a template is parsed as an expression and then validated to ensure that it is or expands to a string literal).

Changing how this works is difficult.

// This is rejected because `a = out(reg) x` does not parse as an expresion.
asm!(
	#[cfg(false)]
	a = out(reg) x, //~ ERROR expected token: `,`
	"",
);

lint on positional arguments?

Adding a lint to warn on the definition or use of positional arguments being cfg'd out was discussed in #140279 (comment) and subsequent comments. Such a lint is not currently implemented, but that may not be a blocker based on the comments there.

r? @traviscross (I'm assuming you'll reassign as needed)

[rustbot]

compiler-builtins is developed in its own repository. If possible, consider making this change to rust-lang/compiler-builtins instead.

cc @tgross35

[joshtriplett]

@rfcbot merge

[rust-rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

@rfcbot reviewed

[joshtriplett]

I agree that the lint on positional arguments isn't a blocker, though I think it's highly desirable.

[joshtriplett]

operand before template

I don't think this is a problem, since it's not likely that people will expect it to work.

[traviscross]

@folkertdev, does this just allow it for cfg or does it allow it for all attributes? E.g., would an attribute macro that expanded to cfg work here? If not, are there any other places currently in the language with a similar restriction?

[folkertdev]

Macros are not currently expanded, e.g.

#![feature(asm_cfg)]

macro_rules! annotated_line {
    () => {
        #[cfg(true)]
        "ret"
    };
}

#[unsafe(no_mangle)]
pub fn square() {
    unsafe {
        std::arch::asm!(
            #[cfg(true)]
            "",
            annotated_line!(),
            #[macros::expand_to_cfg(true)]
            "",

        )
    }
}

fn main() {}

emits

error: this attribute is not supported on assembly
  --> src/main.rs:17:13
   |
17 |             #[macros::expand_to_cfg(true)]
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0658]: attributes on expressions are experimental
  --> src/main.rs:5:9
   |
 5 |         #[cfg(true)]
   |         ^^^^^^^^^^^^
...
16 |             annotated_line!(),
   |             ----------------- in this macro invocation
   |

I don't think we currently allow attributes on arguments to builtin macros. E.g. cfg is not allowed on the template parameter of println! etc.

I'll look at what we can do here, but I suspect it'll be tricky.

[traviscross]

I don't think we currently allow attributes on arguments to builtin macros. E.g. cfg is not allowed on the template parameter of println! etc.

It seems we do allow attributes here.

println!(#[allow(unused)] ""); //~ OK
println!(#[cfg(true)] ""); //~ OK

We don't allow removing expressions here though.

println!(#[cfg(false)] ""); //~ ERROR

(Edit: When I had tested this, I was in a file where I had enabled stmt_expr_attributes for an unrelated reason and didn't notice.)

reference PR: forthcoming (though maybe none is needed?)

The relevant parts of the Reference are attributes.allowed-positions, cfg.attr.allowed-positions, and cfg.cfg_attr.allowed-positions.

The latter two say:

The cfg attribute may be used anywhere attributes are allowed.

The cfg_attr attribute may be used anywhere attributes are allowed.

What would be a bit less than elegant would be to have to say, "the cfg/cfg_attr attributes may be used anywhere attributes are allowed and on arguments to the following macros...". It'd be more elegant if we could add arguments to these macros to the list of places that attributes are allowed and then keep the allowed-positions rules on cfg/cfg_attr untouched.

[petrochenkov]

@folkertdev, does this just allow it for cfg or does it allow it for all attributes? E.g., would an attribute macro that expanded to cfg work here? If not, are there any other places currently in the language with a similar restriction?

cfg in asm are a part of asm's DSL mimicking attributes, and are entirely unrelated to cfg attributes from the language proper.
Not sure whether it's a good style or not.
Technically they could use some other syntax, and still work in the same way as now.

So anything that these "attributes" can potentially do (eager expansion or something else) needs to be implemented manually by the asm macro and is not guaranteed to match behavior of real attributes in language-supported expansion points.

[folkertdev]

It seems we do allow attributes here.

println!(#[allow(unused)] ""); //~ OK
println!(#[cfg(true)] ""); //~ OK

This is really an implementation detail. Actually println!(#[cfg(true)] "") does emit

error[E0658]: attributes on expressions are experimental
 --> src/main.rs:3:9
  |
3 |         #[cfg(true)]
  |         ^^^^^^^^^^^^
  |
  = note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information

The allow attribute is just already allowed on expressions (so that error message is not complete).

---

We do actually parse arbitrary attributes on the assembly arguments, but anything besides cfg and cfg_attr is rejected with:

error: this attribute is not supported on assembly
 --> src/main.rs:3:9
  |
3 |         #[bar]
  |         ^^^^^^

[traviscross]

This is really an implementation detail. Actually println!(#[cfg(true)] "") does emit

Oops... when I tested this, I was in a file where I'd enabled stmt_expr_attributes for an unrelated reason (testing closure capture behavior with rustc_capture_analysis) and didn't notice.

[traviscross]

@rustbot labels -S-waiting-on-documentation

The Reference PR has been approved; this is now good to go as far as lang-docs is concerned, and the lang FCP has completed, so this is good to go as far as lang is concerned.

[folkertdev]

Will you perform the code review as well (the changes here are very straightforward), or should we assign someone else?

[traviscross]

Looks good to me.

r? compiler

View changes since this review

[lcnr]

@rustbot reroll

[jieyouxu]

@rustbot reroll

[jdonszelmann]

@bors r+ rollup

[bors]

📌 Commit 261d7eb has been approved by jdonszelmann

It is now in the queue for this repository.