[clang][Parse] properly parse asm-qualifiers, asm inline

[nickdesaulniers]

Summary

The parsing of GNU C extended asm statements was a little brittle and
had a few issues:

• It was using Parse::ParseTypeQualifierListOpt to parse the volatile qualifier. That parser is really meant for TypeQualifiers; an asm statement doesn't really have a type qualifier. This is still maybe nice to have, but not necessary. We now can check for the volatile token by properly expanding the grammer, rather than abusing Parse::ParseTypeQualifierListOpt.
• The parsing of goto was position dependent, so asm goto volatile wouldn't parse. The qualifiers should be position independent to one another. Now they are.
• We would warn on duplicate volatile, but the parse error for duplicate goto was a generic parse error and wasn't clear.
• We need to add support for the recent GNU C extension asm inline. Adding support to the parser with the above issues highlighted the need for this refactoring.

Link: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

[nickdesaulniers]

I still plan to add CodeGen support for asm inline before the next release, which should be as simple as emitting an additional inlinehint, but I'd like to save that for a follow up patch on top of this.

[nickdesaulniers]

• remove impossible case in ParseGNUAsmQualifierListOpt
• simplify SetGNUAsmQual
• fix typo in comment

[nickdesaulniers]

• reupload patch

[nickdesaulniers]

• make sure to initialize GNUAsmQualifiers

[nathanchance]

clang/test/Parser/asm-qualifiers.c

21

I'm probably being dense but what is intended to be tested differently between combinations and permutations? I assume the order of the qualifiers? Wouldn't it just be better to merge combinations into permutations or was there some deeper reasoning for the compartmentalization?

[nickdesaulniers]

clang/test/Parser/asm-qualifiers.c

21

combinations tests a combination of different asm-qualifiers together. permutations are just permutations of the combinations that have not been tested above. I may not even have my nomenclature correct. Shall I combine them?

[nathanchance]

clang/test/Parser/asm-qualifiers.c

21

I assume that you want permutations since you want to make sure that the ordering does not matter, right? If you just care about combinations then

asm inline goto volatile("" ::::foo);
asm inline volatile goto("" ::::foo);

asm goto inline volatile("" ::::foo);
asm goto volatile inline("" ::::foo);

asm volatile goto inline("" ::::foo); // note, this one should probably be added in permutations
asm volatile inline goto("" ::::foo);

could just be distilled down to one of those since they are the same combination of qualifiers (combinations do not care about order). I would say that moving combinations into permutations would be wise since permutations tests the same thing that combinations does and more.

[nickdesaulniers]

• combine combinations into permutations test

[nickdesaulniers]

clang/test/Parser/asm-qualifiers.c

21

Great suggestion, done. Thanks!

[nickdesaulniers]

• git-clang-format HEAD~

[nickdesaulniers]

clang/lib/Parse/ParseStmtAsm.cpp

725–726

I think in a follow up commit, I'll just delete this (the call to ParseTypeQualifierListOpt and subsequent warn_asm_qualifier_ignored emission); making it a parse error to put type qualifiers after the keyword asm, which would match the behavior of GCC, and not waste time looking for and parsing type qualifiers. It maybe made sense when ParseTypeQualifierListOpt was being abused to parse volatile, but this patch fixes that.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticParseKinds.td

31

duplicate asm qualifier '%0' (with the quotes around the %0).

clang/include/clang/Sema/DeclSpec.h

324 ↗

(On Diff #248226)

Asm Qualifiers -> asm qualifiers

378 ↗

(On Diff #248226)

I think we only need three bits to store this. Also, the comment should be clear that this is a bitwise OR of AQ, similar to the comment for TypeQualifiers.

582 ↗

(On Diff #248226)

The comment and the code don't match. I prefer the spelling in the comment.

736 ↗

(On Diff #248226)

Given that we're already inconsistent in this area regarding naming, I'd prefer to follow the LLVM naming convention and use setGNUAsmQualifier()

clang/lib/Parse/ParseStmtAsm.cpp

670

The comment doesn't match the code -- I think the comment should be:

/// asm-qualifier:
///   volatile
///   inline
///   goto
///
/// asm-qualifier-list:
///   asm-qualifier
///   asm-qualifier-list asm-qualifier

686

I would expect a diagnostic if the unknown token is anything other than a left paren.

clang/test/Parser/asm-qualifiers.c

4

There should also be tests where the qualifiers are incorrect. e.g.,

asm noodle("");
asm goto noodle("");
asm volatile noodle inline("");

[nickdesaulniers]

• address review comments, add tests for global asm statements.

clang/lib/Parse/ParseStmtAsm.cpp

686

That currently is is handled by the caller, see if (Tok.isNot(tok::l_paren)) { in Parser::ParseAsmStatement (line 762 of clang/lib/Parse/ParseStmtAsm.cpp).

[nickdesaulniers]

clang/lib/Parse/ParseStmtAsm.cpp

703

I guess this should be asm-qualifier-list[opt]?

clang/test/Parser/asm-qualifiers.c

55

I'll probably also drop parsing for this here, too, in a follow up; GCC treats this case as a parse error.

[aaron.ballman]

clang/lib/Parse/ParseStmtAsm.cpp

686

My suggestion is to move it out of the caller and into the helper. This function is expected to parse the asm qualifiers, so it stands to reason if it gets something other than an asm qualifier, it should diagnose. (We probably won't need to re-use this parsing logic in other contexts, but that's why it's better to have the diagnostic here rather than assume the caller will do it.)

703

Yup, good catch!

clang/test/Parser/asm-qualifiers.c

13–15

I think these diagnostics should be improved as none of them suggest the underlying problem. As they read now, it sounds like you can write asm(volatile noodle inline("")) to appease the diagnostic.

[nickdesaulniers]

• completely drop use of Parse::ParseTypeQualifierListOpt
• move paren parsing into helper
• fix up test cases for global asm statements
• delete duplicate test cases covered by the added test file

[nickdesaulniers]

clang/lib/Parse/ParseStmtAsm.cpp

725–726

Chatting with @aaron.ballman , I think it makes sense to tackle this in this CL, which will simplify the requested changes to parsing (above).

[nickdesaulniers]

clang/include/clang/Basic/DiagnosticParseKinds.td

15–18

oh, I meant to rename this; and I think flipping the order of this and following def is a little clearer for code review.

[nickdesaulniers]

• use better error name
• reorder new errors
• git-clang-format HEAD~

[nickdesaulniers]

• add release note about change in behavior of -Wduplicate-decl-specifier

[aaron.ballman]

Thank you for working on this, this LGTM! If you wanted a follow-up patch beyond adding semantic support for the inline keyword, I think it might make sense to investigate divorcing the qualifier parsing from the DeclSpec interface. These are ASM statements, not declarations, so the fact that we're using a DeclSpec to smuggle the qualifiers around is a bit unexpected. However, I don't think that work needs to hold up this patch.

[nickdesaulniers]

In D75563#1911375, @aaron.ballman wrote:

Thank you for working on this, this LGTM! If you wanted a follow-up patch beyond adding semantic support for the inline keyword, I think it might make sense to investigate divorcing the qualifier parsing from the DeclSpec interface. These are ASM statements, not declarations, so the fact that we're using a DeclSpec to smuggle the qualifiers around is a bit unexpected. However, I don't think that work needs to hold up this patch.

I agree and I think that recommendation is in good taste. I'd rather do things the right way now and hopefully never need to revisit. Also, there's no rush on this. I'll declare a new class for the qualifiers, since we just need to know which qualifiers were used (what is essentially 3 booleans) and strings for each.

[nickdesaulniers]

• rebase, divorce from DeclSpec

[nickdesaulniers]

clang/lib/Parse/Parser.cpp

1544

This pattern is repeated twice, let me see if I can DRY that up.

[nickdesaulniers]

• DRY up getting the Qualifier, and checking Tok.is

[nickdesaulniers]

Sorry, got a little trigger happy with the refactoring. Having a weekend to think about this more definitely helps. It's nice to avoid instantiating a whole instance of SemaDecl and instead use a single unsigned bitfield! I have a few more refactorings I'd like to do, but I'm working remote and my workstation is suddenly unavailable over SSH. Hopefully I can reboot it tomorrow.

clang/include/clang/Parse/Parser.h

3214

Might be nice to have an implicit constructor that takes a GNUAsmQualifier operand that defaults to AQ_unspecified.

3215

Now that we're not using DeclSpec, maybe we could drop this in preference of operator<<.

clang/lib/Parse/ParseStmtAsm.cpp

358–359

Can be implemented in terms of Parser::GNUAsmQualifiers::getQualifierForTokenKind.

[nickdesaulniers]

• small refactorings

[aaron.ballman]

clang/include/clang/Basic/DiagnosticParseKinds.td

17

If we know of any compilers that support asm statements with qualifiers other than what Clang supports, it may make sense for these diagnostics to be warnings that are treated as an error by default so that users can disable the warning when compiling with Clang. I don't know of any such compilers off the top of my head, but perhaps you know of some.

clang/include/clang/Parse/Parser.h

3206

There's really no benefit to using a bit-field here, so I'd just store an unsigned.

3214

I think you should use an in-class initializer for Qualifiers and then can drop the constructor entirely.

3215

I prefer the named function to a streaming operator given how little this will be used -- it's easier to call the named function from within a debugger than to use a streaming operator.

Please don't use a type name as a parameter identifier. Also, drop the top-level const.

3216

Please don't use a type name as a parameter identifier. Also, drop the top-level const.

3224

This should be taking a reference rather than a pointer. It should also be named parseGNUAsmQualifierListOpt() with a lowercase p.

clang/lib/Parse/ParseStmtAsm.cpp

358

This is too clever for my tastes -- I'd prefer an explicit test against AQ_unspecified.

932

type vs param name and top-level const.

937

This looks wrong to me -- it flows through to an unreachable despite being reachable. I think this should return "unspecified";.

951

type vs param name and top-level const.

[nickdesaulniers]

• address latest review comments, more refactoring

clang/lib/Parse/ParseStmtAsm.cpp

937

This method is only called for printing; it seems weird to return "unspecified" when it's kind of an invariant that that should never happen. Maybe an assert here would be better?

I've updated the implementation slightly, but leaving this comment thread open for more feedback.

[nickdesaulniers]

• remove isUnspecified(), was partially committed from refactoring I changed my mind on

[nickdesaulniers]

• drop auto as type is no longer implicit

[aaron.ballman]

clang/lib/Parse/ParseStmtAsm.cpp

684

I think this will point to the right paren because of the preceding SkipUntil when it should point at the original token location.

937

An assert seems wrong to me -- the enumeration value is valid and the user has asked to print it. There's not much to assert on there. It's the caller of the method that should know whether it's violating an invariant to have that value in the first place. Also, people print from debuggers with relative frequency, so I'd personally avoid an assert in this case.

[nickdesaulniers]

• restore order between Diag and SkipUntil

[aaron.ballman]

LGTM!

clang/docs/ReleaseNotes.rst

 
 - -fno-common has been enabled as the default for all targets.  Therefore, C
   code that uses tentative definitions as definitions of a variable in multiple
   translation units will trigger multiple-definition linker errors.  Generally,
   this occurs when the use of the ``extern`` keyword is neglected in the declaration
   of a variable in a header file. In some cases, no specific translation unit
   provides a definition of the variable. The previous behavior can be restored by
   specifying ``-fcommon``.
+- -Wasm-ignored-qualifier (ex. `asm const ("")`) has been removed and replaced
+  with an error (this matches a recent change in GCC-9).
+- -Wasm-file-asm-volatile (ex. `asm volatile ("")` at global scope) has been
+  removed and replaced with an error (this matches GCC's behavior).
+- Duplicate qualifiers on asm statements (ex. `asm volatile volatile ("")`) no
+  longer produces a warning via -Wduplicate-decl-specifier, but now an error
+  (this matches GCC's behavior).
 
 New Pragmas in Clang
 --------------------
 
 - ...
 
 Attribute Changes in Clang
 --------------------------
 
 - ...
 
 Windows Support
 ---------------
 
 C Language Changes in Clang
 ---------------------------
 
 - The default C language standard used when `-std=` is not specified has been
   upgraded from gnu11 to gnu17.
 
+- Clang now supports the GNU C extension `asm inline`; it won't do anything
+  *yet*, but it will be parsed.
+
 - ...
 
 C++ Language Changes in Clang
 -----------------------------
 
 - Clang now implements a restriction on giving non-C-compatible anonymous
   structs a typedef name for linkage purposes, as described in C++ committee
   paper `P1766R1 <http://wg21.link/p1766r1>`. This paper was adopted by the

clang/include/clang/Basic/DiagnosticGroups.td

 def ObjCSignedCharBool : DiagGroup<"objc-signed-char-bool",
   [ObjCSignedCharBoolImplicitIntConversion,
    ObjCSignedCharBoolImplicitFloatConversion,
    ObjCBoolConstantConversion,
    TautologicalObjCBoolCompare]>;
 
 // Inline ASM warnings.
 def ASMOperandWidths : DiagGroup<"asm-operand-widths">;
-def ASMIgnoredQualifier : DiagGroup<"asm-ignored-qualifier">;
 def ASM : DiagGroup<"asm", [
-    ASMOperandWidths, ASMIgnoredQualifier
+    ASMOperandWidths
   ]>;
 
 // OpenMP warnings.
 def SourceUsesOpenMP : DiagGroup<"source-uses-openmp">;
 def OpenMPClauses : DiagGroup<"openmp-clauses">;
 def OpenMPLoopForm : DiagGroup<"openmp-loop-form">;
 def OpenMPMapping : DiagGroup<"openmp-mapping">;
 def OpenMPTarget : DiagGroup<"openmp-target", [OpenMPMapping]>;

clang/include/clang/Basic/DiagnosticParseKinds.td

 //==--- DiagnosticParseKinds.td - libparse diagnostics --------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 //===----------------------------------------------------------------------===//
 // Parser Diagnostics
 //===----------------------------------------------------------------------===//
 
 let Component = "Parse" in {
 
-def warn_asm_qualifier_ignored : Warning<
-  "ignored %0 qualifier on asm">, CatInlineAsm, InGroup<ASMIgnoredQualifier>;
-def warn_file_asm_volatile : Warning<
-  "meaningless 'volatile' on asm outside function">, CatInlineAsm,
-  InGroup<ASMIgnoredQualifier>;
+def err_asm_qualifier_ignored : Error<
+  "expected 'volatile', 'inline', 'goto', or '('">, CatInlineAsm;
+def err_global_asm_qualifier_ignored : Error<
+  "meaningless '%0' on asm outside function">, CatInlineAsm;
 
 let CategoryName = "Inline Assembly Issue" in {
 def err_asm_empty : Error<"__asm used with no assembly instructions">;
 def err_inline_ms_asm_parsing : Error<"%0">;
 def err_msasm_unsupported_arch : Error<
   "Unsupported architecture '%0' for MS-style inline assembly">;
 def err_msasm_unable_to_create_target : Error<
   "MS-style inline assembly is not available: %0">;
 def err_gnu_inline_asm_disabled : Error<
   "GNU-style inline assembly is disabled">;
 def err_asm_goto_cannot_have_output : Error<
   "'asm goto' cannot have output constraints">;
+def err_asm_duplicate_qual : Error<"duplicate asm qualifier '%0'">;
 }
 
 let CategoryName = "Parse Issue" in {
 
 def ext_empty_translation_unit : Extension<
   "ISO C requires a translation unit to contain at least one declaration">,
   InGroup<DiagGroup<"empty-translation-unit">>;
 def warn_cxx98_compat_top_level_semi : Warning<

clang/include/clang/Parse/Parser.h

private:
   void CodeCompleteDirective(bool InConditional) override;
   void CodeCompleteInConditionalExclusion() override;
   void CodeCompleteMacroName(bool IsDefinition) override;
   void CodeCompletePreprocessorExpression() override;
   void CodeCompleteMacroArgument(IdentifierInfo *Macro, MacroInfo *MacroInfo,
                                  unsigned ArgumentIndex) override;
   void CodeCompleteIncludedFile(llvm::StringRef Dir, bool IsAngled) override;
   void CodeCompleteNaturalLanguage() override;
+
+  class GNUAsmQualifiers {
+    unsigned Qualifiers = AQ_unspecified;
+
+  public:
+    enum AQ {
+      AQ_unspecified = 0,
+      AQ_volatile    = 1,
+      AQ_inline      = 2,
+      AQ_goto        = 4,
+    };
+    static const char *getQualifierName(AQ Qualifier);
+    bool setAsmQualifier(AQ Qualifier);
+    inline bool isVolatile() const { return Qualifiers & AQ_volatile; };
+    inline bool isInline() const { return Qualifiers & AQ_inline; };
+    inline bool isGoto() const { return Qualifiers & AQ_goto; }
+  };
+  bool isGCCAsmStatement(const Token &TokAfterAsm) const;
+  bool isGNUAsmQualifier(const Token &TokAfterAsm) const;
+  GNUAsmQualifiers::AQ getGNUAsmQualifier(const Token &Tok) const;
+  bool parseGNUAsmQualifierListOpt(GNUAsmQualifiers &AQ);
 };
 
 }  // end namespace clang
 
 #endif

clang/lib/Parse/ParseStmtAsm.cpp

static bool buildMSAsmString(Preprocessor &PP, SourceLocation AsmLoc,
   // Ensure that the buffer is null-terminated.
   Asm.push_back('\0');
   Asm.pop_back();
 
   assert(TokOffsets.size() == AsmToks.size());
   return false;
 }
 
-/// isTypeQualifier - Return true if the current token could be the
-/// start of a type-qualifier-list.
-static bool isTypeQualifier(const Token &Tok) {
-  switch (Tok.getKind()) {
-  default: return false;
-  // type-qualifier
-  case tok::kw_const:
-  case tok::kw_volatile:
-  case tok::kw_restrict:
-  case tok::kw___private:
-  case tok::kw___local:
-  case tok::kw___global:
-  case tok::kw___constant:
-  case tok::kw___generic:
-  case tok::kw___read_only:
-  case tok::kw___read_write:
-  case tok::kw___write_only:
-    return true;
-  }
+// Determine if this is a GCC-style asm statement.
+bool Parser::isGCCAsmStatement(const Token &TokAfterAsm) const {
+  return TokAfterAsm.is(tok::l_paren) || isGNUAsmQualifier(TokAfterAsm);
 }
 
-// Determine if this is a GCC-style asm statement.
-static bool isGCCAsmStatement(const Token &TokAfterAsm) {
-  return TokAfterAsm.is(tok::l_paren) || TokAfterAsm.is(tok::kw_goto) ||
-         isTypeQualifier(TokAfterAsm);
+bool Parser::isGNUAsmQualifier(const Token &TokAfterAsm) const {
+  return getGNUAsmQualifier(TokAfterAsm) != GNUAsmQualifiers::AQ_unspecified;
 }
 
 /// ParseMicrosoftAsmStatement. When -fms-extensions/-fasm-blocks is enabled,
 /// this routine is called to collect the tokens for an MS asm statement.
 ///
 /// [MS]  ms-asm-statement:
 ///         ms-asm-block
 ///         ms-asm-block ms-asm-statement
 ///
StmtResult Parser::ParseMicrosoftAsmStatement(SourceLocation AsmLoc) {
   }
 
   // FIXME: We should be passing source locations for better diagnostics.
   return Actions.ActOnMSAsmStmt(AsmLoc, LBraceLocs[0], AsmToks, AsmStringIR,
                                 NumOutputs, NumInputs, ConstraintRefs,
                                 ClobberRefs, Exprs, EndLoc);
 }
 
+/// parseGNUAsmQualifierListOpt - Parse a GNU extended asm qualifier list.
+///       asm-qualifier:
+///         volatile
+///         inline
+///         goto
+///
+///       asm-qualifier-list:
+///         asm-qualifier
+///         asm-qualifier-list asm-qualifier
+bool Parser::parseGNUAsmQualifierListOpt(GNUAsmQualifiers &AQ) {
+  while (1) {
+    const GNUAsmQualifiers::AQ A = getGNUAsmQualifier(Tok);
+    if (A == GNUAsmQualifiers::AQ_unspecified) {
+      if (Tok.isNot(tok::l_paren)) {
+        Diag(Tok.getLocation(), diag::err_asm_qualifier_ignored);
+        SkipUntil(tok::r_paren, StopAtSemi);
+        return true;
+      }
+      return false;
+    }
+    if (AQ.setAsmQualifier(A))
+      Diag(Tok.getLocation(), diag::err_asm_duplicate_qual)
+          << GNUAsmQualifiers::getQualifierName(A);
+    ConsumeToken();
+  }
+  return false;
+}
+
 /// ParseAsmStatement - Parse a GNU extended asm statement.
 ///       asm-statement:
 ///         gnu-asm-statement
 ///         ms-asm-statement
 ///
 /// [GNU] gnu-asm-statement:
-///         'asm' type-qualifier[opt] '(' asm-argument ')' ';'
+///         'asm' asm-qualifier-list[opt] '(' asm-argument ')' ';'
 ///
 /// [GNU] asm-argument:
 ///         asm-string-literal
 ///         asm-string-literal ':' asm-operands[opt]
 ///         asm-string-literal ':' asm-operands[opt] ':' asm-operands[opt]
 ///         asm-string-literal ':' asm-operands[opt] ':' asm-operands[opt]
 ///                 ':' asm-clobbers
 ///
 /// [GNU] asm-clobbers:
 ///         asm-string-literal
 ///         asm-clobbers ',' asm-string-literal
 ///
 StmtResult Parser::ParseAsmStatement(bool &msAsm) {
   assert(Tok.is(tok::kw_asm) && "Not an asm stmt");
   SourceLocation AsmLoc = ConsumeToken();
 
   if (getLangOpts().AsmBlocks && !isGCCAsmStatement(Tok)) {
     msAsm = true;
     return ParseMicrosoftAsmStatement(AsmLoc);
   }
 
-  DeclSpec DS(AttrFactory);
   SourceLocation Loc = Tok.getLocation();
-  ParseTypeQualifierListOpt(DS, AR_VendorAttributesParsed);
-
-  // GNU asms accept, but warn, about type-qualifiers other than volatile.
-  if (DS.getTypeQualifiers() & DeclSpec::TQ_const)
-    Diag(Loc, diag::warn_asm_qualifier_ignored) << "const";
-  if (DS.getTypeQualifiers() & DeclSpec::TQ_restrict)
-    Diag(Loc, diag::warn_asm_qualifier_ignored) << "restrict";
-  // FIXME: Once GCC supports _Atomic, check whether it permits it here.
-  if (DS.getTypeQualifiers() & DeclSpec::TQ_atomic)
-    Diag(Loc, diag::warn_asm_qualifier_ignored) << "_Atomic";
-
-  // Remember if this was a volatile asm.
-  bool isVolatile = DS.getTypeQualifiers() & DeclSpec::TQ_volatile;
-  // Remember if this was a goto asm.
-  bool isGotoAsm = false;
-
-  if (Tok.is(tok::kw_goto)) {
-    isGotoAsm = true;
-    ConsumeToken();
-  }
-
-  if (Tok.isNot(tok::l_paren)) {
-    Diag(Tok, diag::err_expected_lparen_after) << "asm";
-    SkipUntil(tok::r_paren, StopAtSemi);
+  GNUAsmQualifiers GAQ;
+  if (parseGNUAsmQualifierListOpt(GAQ))
     return StmtError();
-  }
+
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
 
   ExprResult AsmString(ParseAsmStringLiteral(/*ForAsmLabel*/ false));
 
   // Check if GNU-style InlineAsm is disabled.
   // Error on anything other than empty string.
   if (!(getLangOpts().GNUAsm || AsmString.isInvalid())) {
StmtResult Parser::ParseAsmStatement(bool &msAsm) {
   SmallVector<IdentifierInfo *, 4> Names;
   ExprVector Constraints;
   ExprVector Exprs;
   ExprVector Clobbers;
 
   if (Tok.is(tok::r_paren)) {
     // We have a simple asm expression like 'asm("foo")'.
     T.consumeClose();
-    return Actions.ActOnGCCAsmStmt(AsmLoc, /*isSimple*/ true, isVolatile,
-                                   /*NumOutputs*/ 0, /*NumInputs*/ 0, nullptr,
-                                   Constraints, Exprs, AsmString.get(),
-                                   Clobbers, /*NumLabels*/ 0,
-                                   T.getCloseLocation());
+    return Actions.ActOnGCCAsmStmt(
+        AsmLoc, /*isSimple*/ true, GAQ.isVolatile(),
+        /*NumOutputs*/ 0, /*NumInputs*/ 0, nullptr, Constraints, Exprs,
+        AsmString.get(), Clobbers, /*NumLabels*/ 0, T.getCloseLocation());
   }
 
   // Parse Outputs, if present.
   bool AteExtraColon = false;
   if (Tok.is(tok::colon) || Tok.is(tok::coloncolon)) {
     // In C++ mode, parse "::" like ": :".
     AteExtraColon = Tok.is(tok::coloncolon);
     ConsumeToken();
if (!AteExtraColon && isTokenStringLiteral()) {
 
         Clobbers.push_back(Clobber.get());
 
         if (!TryConsumeToken(tok::comma))
           break;
       }
     }
   }
-  if (!isGotoAsm && (Tok.isNot(tok::r_paren) || AteExtraColon)) {
+  if (!GAQ.isGoto() && (Tok.isNot(tok::r_paren) || AteExtraColon)) {
     Diag(Tok, diag::err_expected) << tok::r_paren;
     SkipUntil(tok::r_paren, StopAtSemi);
     return StmtError();
   }
 
   // Parse the goto label, if present.
   unsigned NumLabels = 0;
   if (AteExtraColon || Tok.is(tok::colon)) {
while (true) {
       ExprResult Res =
           Actions.ActOnAddrLabel(Tok.getLocation(), Tok.getLocation(), LD);
       Exprs.push_back(Res.get());
       NumLabels++;
       ConsumeToken();
       if (!TryConsumeToken(tok::comma))
         break;
     }
-  } else if (isGotoAsm) {
+  } else if (GAQ.isGoto()) {
     Diag(Tok, diag::err_expected) << tok::colon;
     SkipUntil(tok::r_paren, StopAtSemi);
     return StmtError();
   }
   T.consumeClose();
-  return Actions.ActOnGCCAsmStmt(
-      AsmLoc, false, isVolatile, NumOutputs, NumInputs, Names.data(),
-      Constraints, Exprs, AsmString.get(), Clobbers, NumLabels,
+  return Actions.ActOnGCCAsmStmt(AsmLoc, false, GAQ.isVolatile(), NumOutputs,
+                                 NumInputs, Names.data(), Constraints, Exprs,
+                                 AsmString.get(), Clobbers, NumLabels,
                                  T.getCloseLocation());
 }
 
 /// ParseAsmOperands - Parse the asm-operands production as used by
 /// asm-statement, assuming the leading ':' token was eaten.
 ///
 /// [GNU] asm-operands:
 ///         asm-operand
 ///         asm-operands ',' asm-operand
if (Res.isInvalid()) {
       return true;
     }
     Exprs.push_back(Res.get());
     // Eat the comma and continue parsing if it exists.
     if (!TryConsumeToken(tok::comma))
       return false;
   }
 }
+
+const char *Parser::GNUAsmQualifiers::getQualifierName(AQ Qualifier) {
+  switch (Qualifier) {
+    case AQ_volatile: return "volatile";
+    case AQ_inline: return "inline";
+    case AQ_goto: return "goto";
+    case AQ_unspecified: return "unspecified";
+  }
+  llvm_unreachable("Unkown GNUAsmQualifier");
+}
+
+Parser::GNUAsmQualifiers::AQ
+Parser::getGNUAsmQualifier(const Token &Tok) const {
+  switch (Tok.getKind()) {
+    case tok::kw_volatile: return GNUAsmQualifiers::AQ_volatile;
+    case tok::kw_inline: return GNUAsmQualifiers::AQ_inline;
+    case tok::kw_goto: return GNUAsmQualifiers::AQ_goto;
+    default: return GNUAsmQualifiers::AQ_unspecified;
+  }
+}
+bool Parser::GNUAsmQualifiers::setAsmQualifier(AQ Qualifier) {
+  bool IsDuplicate = Qualifiers & Qualifier;
+  Qualifiers |= Qualifier;
+  return IsDuplicate;
+}

clang/lib/Parse/Parser.cpp

 ///
 /// [GNU] simple-asm-expr:
 ///         'asm' '(' asm-string-literal ')'
 ///
 ExprResult Parser::ParseSimpleAsm(bool ForAsmLabel, SourceLocation *EndLoc) {
   assert(Tok.is(tok::kw_asm) && "Not an asm!");
   SourceLocation Loc = ConsumeToken();
 
-  if (Tok.is(tok::kw_volatile)) {
-    // Remove from the end of 'asm' to the end of 'volatile'.
+  if (isGNUAsmQualifier(Tok)) {
+    // Remove from the end of 'asm' to the end of the asm qualifier.
     SourceRange RemovalRange(PP.getLocForEndOfToken(Loc),
                              PP.getLocForEndOfToken(Tok.getLocation()));
-
-    Diag(Tok, diag::warn_file_asm_volatile)
+    Diag(Tok, diag::err_global_asm_qualifier_ignored)
+        << GNUAsmQualifiers::getQualifierName(getGNUAsmQualifier(Tok))
         << FixItHint::CreateRemoval(RemovalRange);
     ConsumeToken();
   }
 
   BalancedDelimiterTracker T(*this, tok::l_paren);
   if (T.consumeOpen()) {
     Diag(Tok, diag::err_expected_lparen_after) << "asm";
     return ExprError();
   }
 
   ExprResult Result(ParseAsmStringLiteral(ForAsmLabel));
 
   if (!Result.isInvalid()) {
     // Close the paren and get the location of the end bracket
     T.consumeClose();

clang/test/CodeGen/inline-asm-mixed-style.c

 // RUN: %clang_cc1 -triple i386-unknown-unknown -fasm-blocks -O0 -emit-llvm -S %s -o - | FileCheck %s
 // REQUIRES: x86-registered-target
 
 void f() {
   __asm mov eax, ebx
   __asm mov ebx, ecx
   __asm__("movl %ecx, %edx");
   // CHECK: movl    %ebx, %eax
   // CHECK: movl    %ecx, %ebx
   // CHECK: movl    %ecx, %edx
 
   __asm mov eax, ebx
   __asm volatile ("movl %ecx, %edx");
   // CHECK: movl    %ebx, %eax
   // CHECK: movl    %ecx, %edx
 
-  __asm mov eax, ebx
-  __asm const ("movl %ecx, %edx"); // expected-warning {{ignored const qualifier on asm}} 
-  // CHECK: movl    %ebx, %eax
-  // CHECK: movl    %ecx, %edx
-
   __asm volatile goto ("movl %ecx, %edx");
   // CHECK: movl    %ecx, %edx
 
   __asm mov eax, ebx
   __asm goto ("movl %ecx, %edx");
   // CHECK: movl    %ebx, %eax
   // CHECK: movl    %ecx, %edx
 }

clang/test/Parser/asm-qualifiers.c

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify %s
+
+void qualifiers(void) {
+  asm("");
+  asm volatile("");
+  asm inline("");
+  asm goto("" ::::foo);
+foo:;
+}
+
+void unknown_qualifiers(void) {
+  asm noodle(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm goto noodle("" ::::foo); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+  asm volatile noodle inline(""); // expected-error {{expected 'volatile', 'inline', 'goto', or '('}}
+foo:;
+}
+
+void underscores(void) {
+  __asm__("");
+  __asm__ __volatile__("");
+  __asm__ __inline__("");
+  // Note: goto is not supported with underscore prefix+suffix.
+  __asm__ goto("" ::::foo);
+foo:;
+}
+
+void permutations(void) {
+  asm goto inline volatile("" ::::foo);
+  asm goto inline("");
+  asm goto volatile inline("" ::::foo);
+  asm goto volatile("");
+  asm inline goto volatile("" ::::foo);
+  asm inline goto("" ::::foo);
+  asm inline volatile goto("" ::::foo);
+  asm inline volatile("");
+  asm volatile goto("" ::::foo);
+  asm volatile inline goto("" ::::foo);
+  asm volatile inline("");
+foo:;
+}
+
+void duplicates(void) {
+  asm volatile volatile("");             // expected-error {{duplicate asm qualifier 'volatile'}}
+  __asm__ __volatile__ __volatile__(""); // expected-error {{duplicate asm qualifier 'volatile'}}
+  asm inline inline("");                 // expected-error {{duplicate asm qualifier 'inline'}}
+  __asm__ __inline__ __inline__("");     // expected-error {{duplicate asm qualifier 'inline'}}
+  asm goto goto("" ::::foo);             // expected-error {{duplicate asm qualifier 'goto'}}
+  __asm__ goto goto("" ::::foo);         // expected-error {{duplicate asm qualifier 'goto'}}
+foo:;
+}
+
+// globals
+asm ("");
+// <rdar://problem/7574870>
+asm volatile (""); // expected-error {{meaningless 'volatile' on asm outside function}}
+asm inline (""); // expected-error {{meaningless 'inline' on asm outside function}}
+asm goto (""::::noodle); // expected-error {{meaningless 'goto' on asm outside function}}
+// expected-error@-1 {{expected ')'}}
+// expected-note@-2 {{to match this '('}}

clang/test/Parser/asm.c

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 
 #if !__has_extension(gnu_asm)
 #error Extension 'gnu_asm' should be available by default
 #endif
 
 void f1() {
   // PR7673: Some versions of GCC support an empty clobbers section.
   asm ("ret" : : :);
 }
 
 void f2() {
   asm("foo" : "=r" (a)); // expected-error {{use of undeclared identifier 'a'}}
   asm("foo" : : "r" (b)); // expected-error {{use of undeclared identifier 'b'}} 
-
-  asm const (""); // expected-warning {{ignored const qualifier on asm}}
-  asm volatile ("");
-  asm restrict (""); // expected-warning {{ignored restrict qualifier on asm}}
-  // FIXME: Once GCC supports _Atomic, check whether it allows this.
-  asm _Atomic (""); // expected-warning {{ignored _Atomic qualifier on asm}}
 }
 
 void a() __asm__(""); // expected-error {{cannot use an empty string literal in 'asm'}}
 void a() {
   __asm__(""); // ok
 }
 
 // rdar://5952468

clang/test/Sema/asm.c

 
 // PR4077
 int test7(unsigned long long b) {
   int a;
   asm volatile("foo %0 %1" : "=a" (a) :"0" (b)); // expected-error {{input with type 'unsigned long long' matching output with type 'int'}}
   return a;
 }
 
-// <rdar://problem/7574870>
-asm volatile (""); // expected-warning {{meaningless 'volatile' on asm outside function}}
-
 // PR3904
 void test8(int i) {
   // A number in an input constraint can't point to a read-write constraint.
   asm("" : "+r" (i), "=r"(i) :  "0" (i)); // expected-error{{invalid input constraint '0' in asm}}
 }
 
 // PR3905
 void test9(int i) {