This is an archive of the discontinued LLVM Phabricator instance.

[clang] add Diag -Wasm-volatile for implied volatile asm stmts
Needs ReviewPublic

Authored by nickdesaulniers on Jan 26 2022, 4:52 PM.

Details

Summary

According to the GCC docs on inline asm
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile

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

We tend to see the volatile qualifier tacked on to asm statements "just
to be safe" when in reality it makes no difference in IR for asm goto
statements or output-less asm. The sideeffect flag is set on such IR
instructions.

Really the volatile asm-qualifier exists only to signal that an asm
statement should not be DCE'd (when it has outputs but they are unused),
CSE'd, or LICM'd. It is not a general compiler barrier.

Create a new warning, -Wasm-volatile and warn in either case.

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Jan 26 2022, 4:52 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 26 2022, 4:52 PM
Herald added subscribers: Restricted Project, cfe-commits, pcwang-thead, aheejin. · View Herald Transcript
nickdesaulniers planned changes to this revision.Jan 26 2022, 4:53 PM
nickdesaulniers added a subscriber: nathanchance.
clang/lib/Parse/ParseStmtAsm.cpp
794

Loc is not correct if volatile is not the first asm-qualifier.

clang/include/clang/Basic/DiagnosticParseKinds.td
31

should this go in clang/include/clang/Basic/DiagnosticSemaKinds.td instead?

nickdesaulniers edited the summary of this revision. (Show Details)
  • fix volatile location, more tests

I agree that's the expected semantics. I think those semantics are unfortunate, but they're not gonna change. IMO it would've been better if you had to opt-in to "no side effects" via __attribute__((const)) or so.

But I wonder why you think we should be discouraging people from writing "asm volatile" for things that are implicitly treated as having side-effects? It's quite non-obvious that adding an output argument to an existing asm statement changes whether it's treated as having side-effects, and I don't think it improves a codebase to make people lay out that trap more more frequently.

The volatile qualifier is implied but makes the intention explicit. I agree that the user should not be punished (-Wasm-volatile) by writing asm volatile("int3") instead of asm("int3").

The volatile qualifier is implied but makes the intention explicit. I agree that the user should not be punished (-Wasm-volatile) by writing asm volatile("int3") instead of asm("int3").

Not punished no. But it is useful education (like all the other "statement without effect"
etc. warnings).