Make inline assembly volatile if it has no outputs. Fixes #46026

[Zoxc]

No description provided.

[kennytm]

🤔 Is it possible to write a test for this?

[kennytm]

r? @nrc

[Zoxc]

Added a test.

r? @eddyb

[eddyb]

r? @alexcrichton

[alexcrichton]

If LLVM has this option then presumably it's useful for someone at some point? In the sense of "you probably want this" but perhaps not always? For those users that may wish to turn this off, would they have any recourse?

[Zoxc]

You always want this behavior.

[alexcrichton]

@Zoxc as the reviewer of a PR it's sort of my job to understand why we would want such a change, what the change itself is doing, and if possible there's an alternative route to achieving the final goal. To understand this change there is no description on this PR, no comments indicating what the change is doing on the PR itself, an issue opened by yourself which advocates a different solution, a reference to another PR fixing a similar bug, and only internal comments which say what's happening rather than why.

I am not personally intimately familiar with either inline assembly or volatile inline assembly. When I ask a question like "For those users that may wish to turn this off, would they have any recourse?" I'm actually genuinely curious as to what's going on here and why this is being proposed. An answer like "You always want this behavior." basically doesn't help me at all and is very curt and seems to assume that I should obviously understand that this is the only possible change we could do.

I'd ask again, then, why doesn't LLVM do this already? Is there a use case for nonvolatile assembly with no outputs? I sort of realize that you don't seem to believe so, but have you investigated LLVM's source code for this? Test cases? Other users? Are there others we could ask?

I've found that 99% of the time if there's an option or a flag in LLVM it's there for good reason. We have been bitten many times in the past for not exposing or otherwise defaulting various options. I'd like to be sure that we should do this before approving so, and at this point I without doing what seems to be all of the investigation myself I don't have a lot to go on to make a decision on this PR.

[Zoxc]

Inline assembly expressions are pure by default, which means they can be removed if they are unused. You can tell LLVM that it has side effects by marking it "volatile"; for example disabling interrupts or changing some FPU flag. You can also say that the assembly changes memory by using the "memory" clobber. If either "volatile" or "memory" are present, LLVM can never remove the inline assembly expression.

If an inline assembly expression has no outputs and is pure, it can never be used and LLVM is free to remove it entirely. This is what happened in compiler-builtins. Such inline assembly expressions are useless and are pretty much guaranteed to be user errors, hence I created #46026.

Just adding "volatile" in these cases as suggested by @parched seemed like the better option. It avoids breaking crates and we want "volatile" (and "memory") to eventually be default anyway. We should probably add both "volatile" and "memory" if neither "volatile" or "memory" are present and the inline assembly has no output. Adding both is more conservative, although I suspect "memory" is a stronger constraint than "volatile".

If we do this we can write asm!("cli"::::"volatile") to get LLVM "volatile" inline assembly, asm!("cli":::"memory") to get LLVM "memory" inline assembly. asm!("cli") will get us LLVM "volatile" and "memory" inline assembly. If we want the current behavior for asm!("cli") we can just remove the inline assembly expression.

[petrochenkov]

Such inline assembly expressions are useless and are pretty much guaranteed to be user errors

Unless they are produced by some kind of macro or code generator, then LLVM removing them is exactly the desired outcome.
(I can't give a concrete example though.)

[hanna-kruppe]

What counts as "output" for the purposes of this code? Is it the = constraint (e.g., =r, =*m)? If so, I believe I agree with @Zoxc's reasoning that inline asm without outputs and "volatile" is likely a user error, pointing to this part of the language reference:

Inline asms with side effects not visible in the constraint list must be marked as having side effects. This is done through the use of the ‘sideeffect‘ keyword, like so:

However, it is far from obvious to me that silently adding the side-effects flag is the right option. I'd favor giving the user an error, for several reasons:

• as @sunfishcode said in Add an error for inline assembly which are non-volatile without any outputs #46026, "inline asm is uncommon and extraordinarily unsafe, conditions which generally favor explicitness over implicitness"

• Silently fixing the user's mistake passes up an opportunity to teach them about the need for "volatile" and/or "memory", which will come back to bite them if they later write some inline asm that does have outputs but also has side effects beyond what's declared in the output constraints.

• I am not sure I understand this statement by @Zoxc correctly:

  we want "volatile" (and "memory") to eventually be default anyway.

  ... but if it is taken to refer to all inline asm everywhere, then I disagree and don't see why we would want that.

To avoid breaking user code, it could be a deny-by-default lint (dependent crates would still compile thanks to --cap-lints, the crate itself is easily fixed).

[arielb1]

@Zoxc

I second @rkruppe . It feels more natural to have a lint against this rather than to implicitly "DWIM":

1. it would not give problems with macros
2. DWIM can be surprising: if someone adds an output that is ignored in some configurations, their code will randomly be optimized out.

[kennytm]

Hi @Zoxc, it seems the majority here prefer to emit a deny-by-fault lint instead of silently make the asm "volatile". Would you mind to change the PR to emit a lint, or is DWIM still a better choice?

[Amanieu]

See this line from the GCC docs:

asm statements that have no output operands, including asm goto statements, are implicitly volatile.

Matching the GCC/Clang behavior here is probably a good idea.

[hanna-kruppe]

I was working under the assumption that Clang differs from GCC here, but apparently it doesn't.

Not matching C compilers is unfortunate, but the arguments in favor of an error still stand. Since it's not a silent mismatch, I don't see it causing a lot of trouble (i.e., if someone ports inline asm without outputs from C, they'll get a deny-by-default lint rather than miscompilation).

[Zoxc]

I am not sure I understand this statement by @Zoxc correctly:

we want "volatile" (and "memory") to eventually be default anyway.
... but if it is taken to refer to all inline asm everywhere, then I disagree and don't see why we would want that.

What I want is for all inline assembly to be volatile by default. If you want purity you have to explicitly opt-in by marking the assembly pure. This is consistent with Rust approach of safety by default. It is also an ergonomic default since most inline assembly needs to be volatile.

[hanna-kruppe]

I can see the appeal of defaulting to volatile. Is that plan written down anywhere else? Does it have buy-in from others?

[Zoxc]

@rkruppe There is this thing: https://internals.rust-lang.org/t/inline-assembly-syntax/239

[hanna-kruppe]

Okay, interesting. However, that's a far bigger and more radical proposal than just making volatile the default. It's also quite old, and has received relatively little discussion compared to its size and radicalness (surely explained by the smaller size of the Rust community in 2014).

In any case, it doesn't seem like any bigger changes to inline asm are sure to happen soon, so this doesn't really change my position re: this narrower patch. Specifically,

• if "volatile by default" does happen (which I'd welcome!), this patch is obsolete
• if it doesn't happen, I stand by my reasoning that DWIM treatment of inline asm without outputs is strictly worse than a deny-by-default-lint

[Amanieu]

I am opposed to changing the current asm! macro and enabling volatile by default since this would be a silent change to the semantics of all the inline asm currently in use. However I wouldn't mind having that as part of a larger rework of inline assembly.

Regardless, I feel that such a change is outside the scope of this issue. I see only two options here:

• Follow GCC/Clang's inline asm semantics and automatically mark inline asm statements with no outputs as volatile.
• Emit a deny-by-default lint whenever an inline asm statement has no outputs and isn't marked as volatile.

I don't have any particular preference for either of these options.

[kennytm]

The test needs to be ignored on asm.js.

[pietroalbini]

Hi @Zoxc, seems like you need to update a test as per #46030 (comment). Could you do that so the PR can be merged?

[Zoxc]

@bors r=nikomatsakis

[bors]

📌 Commit a29d854 has been approved by nikomatsakis

[bors]

⌛ Testing commit a29d854 with merge 1696b8ebf319e7c4258e6bd47154a0b14bfb8d15...

[bors]

💔 Test failed - status-appveyor

[kennytm]

@bors retry #46903

[bors]

⌛ Testing commit a29d854 with merge b224fc8...

[bors]

💥 Test timed out

[kennytm]

@bors clean retry r-