[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

747–756

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

32

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

clang/include/clang/Sema/DeclSpec.h

324

Asm Qualifiers -> asm qualifiers

378

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

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

736

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

688

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

704

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

704

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

722

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

704

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.)

722

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

747–756

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

19

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

1545 ↗

(On Diff #249246)

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

3215

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

3216

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

clang/lib/Parse/ParseStmtAsm.cpp

377–378

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

[nickdesaulniers]

• small refactorings

[aaron.ballman]

clang/include/clang/Basic/DiagnosticParseKinds.td

21

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

3207

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

3215

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

3216

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.

3217

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

3225

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

377

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

971

type vs param name and top-level const.

976

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

990

type vs param name and top-level const.

[nickdesaulniers]

• address latest review comments, more refactoring

clang/lib/Parse/ParseStmtAsm.cpp

976

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

702

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

976

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

 ---------------
 
 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/DiagnosticParseKinds.td

 //===----------------------------------------------------------------------===//
 
 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>;
 
 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

enum AttrRequirements {
     AR_VendorAttributesParsed = AR_GNUAttributesParsed |
                                 AR_DeclspecAttributesParsed
   };
 
   void ParseTypeQualifierListOpt(
       DeclSpec &DS, unsigned AttrReqs = AR_AllAttributesParsed,
       bool AtomicAllowed = true, bool IdentifierRequired = false,
       Optional<llvm::function_ref<void()>> CodeCompletionHandler = None);
+  void ParseGNUAsmQualifierListOpt(DeclSpec &DS);
   void ParseDirectDeclarator(Declarator &D);
   void ParseDecompositionDeclarator(Declarator &D);
   void ParseParenDeclarator(Declarator &D);
   void ParseFunctionDeclarator(Declarator &D,
                                ParsedAttributes &attrs,
                                BalancedDelimiterTracker &Tracker,
                                bool IsAmbiguous,
                                bool RequiresArg = false);
private:
   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;
 };
 
 }  // end namespace clang
 
 #endif

clang/include/clang/Sema/DeclSpec.h

enum TQ {   // NOTE: These flags must be kept in sync with Qualifiers::TQ.
     TQ_restrict    = 2,
     TQ_volatile    = 4,
     TQ_unaligned   = 8,
     // This has no corresponding Qualifiers::TQ value, because it's not treated
     // as a qualifier in our type system.
     TQ_atomic      = 16
   };
 
+  // GNU Asm Qualifiers
+  enum AQ {
+    AQ_unspecified = 0,
+    AQ_volatile    = 1,
+    AQ_inline      = 2,
+    AQ_goto        = 4,
+  };
+
   /// ParsedSpecifiers - Flags to query which specifiers were applied.  This is
   /// returned by getParsedSpecifiers.
   enum ParsedSpecifiers {
     PQ_None                  = 0,
     PQ_StorageClassSpecifier = 1,
     PQ_TypeSpecifier         = 2,
     PQ_TypeQualifier         = 4,
     PQ_FunctionSpecifier     = 8
private:
   unsigned FS_noreturn_specified : 1;
 
   // friend-specifier
   unsigned Friend_specified : 1;
 
   // constexpr-specifier
   unsigned ConstexprSpecifier : 2;
 
+  // GNU asm specifier
+  unsigned GNUAsmQualifiers : 4;
+
   union {
     UnionParsedType TypeRep;
     Decl *DeclRep;
     Expr *ExprRep;
     TemplateIdAnnotation *TemplateIdRep;
   };
 
   /// ExplicitSpecifier - Store information about explicit spicifer.
public:
   DeclSpec(AttributeFactory &attrFactory)
       : StorageClassSpec(SCS_unspecified),
         ThreadStorageClassSpec(TSCS_unspecified),
         SCS_extern_in_linkage_spec(false), TypeSpecWidth(TSW_unspecified),
         TypeSpecComplex(TSC_unspecified), TypeSpecSign(TSS_unspecified),
         TypeSpecType(TST_unspecified), TypeAltiVecVector(false),
         TypeAltiVecPixel(false), TypeAltiVecBool(false), TypeSpecOwned(false),
         TypeSpecPipe(false), TypeSpecSat(false), ConstrainedAuto(false),
         TypeQualifiers(TQ_unspecified),
         FS_inline_specified(false), FS_forceinline_specified(false),
         FS_virtual_specified(false), FS_noreturn_specified(false),
         Friend_specified(false), ConstexprSpecifier(CSK_unspecified),
-        FS_explicit_specifier(), Attrs(attrFactory), writtenBS(),
-        ObjCQualifiers(nullptr) {}
+        GNUAsmQualifiers(AQ_unspecified), FS_explicit_specifier(),
+        Attrs(attrFactory), writtenBS(), ObjCQualifiers(nullptr) {}
 
   // storage-class-specifier
   SCS getStorageClassSpec() const { return (SCS)StorageClassSpec; }
   TSCS getThreadStorageClassSpec() const {
     return (TSCS)ThreadStorageClassSpec;
   }
   bool isExternInLinkageSpec() const { return SCS_extern_in_linkage_spec; }
   void setExternInLinkageSpec(bool Value) {
static const char *getSpecifierName(DeclSpec::TST T,
                                       const PrintingPolicy &Policy);
   static const char *getSpecifierName(DeclSpec::TQ Q);
   static const char *getSpecifierName(DeclSpec::TSS S);
   static const char *getSpecifierName(DeclSpec::TSC C);
   static const char *getSpecifierName(DeclSpec::TSW W);
   static const char *getSpecifierName(DeclSpec::SCS S);
   static const char *getSpecifierName(DeclSpec::TSCS S);
   static const char *getSpecifierName(ConstexprSpecKind C);
+  static const char *getSpecifierName(DeclSpec::AQ A);
 
   // type-qualifiers
 
   /// getTypeQualifiers - Return a set of TQs.
   unsigned getTypeQualifiers() const { return TypeQualifiers; }
   SourceLocation getConstSpecLoc() const { return TQ_constLoc; }
   SourceLocation getRestrictSpecLoc() const { return TQ_restrictLoc; }
   SourceLocation getVolatileSpecLoc() const { return TQ_volatileLoc; }
   SourceLocation getAtomicSpecLoc() const { return TQ_atomicLoc; }
   SourceLocation getUnalignedSpecLoc() const { return TQ_unalignedLoc; }
   SourceLocation getPipeLoc() const { return TQ_pipeLoc; }
 
   /// Clear out all of the type qualifiers.
   void ClearTypeQualifiers() {
     TypeQualifiers = 0;
     TQ_constLoc = SourceLocation();
     TQ_restrictLoc = SourceLocation();
     TQ_volatileLoc = SourceLocation();
     TQ_atomicLoc = SourceLocation();
     TQ_unalignedLoc = SourceLocation();
     TQ_pipeLoc = SourceLocation();
   }
 
+  /// getGNUAsmQualifiers - Return a set of AQs.
+  unsigned GetGNUAsmQual() const { return GNUAsmQualifiers; }
+
   // function-specifier
   bool isInlineSpecified() const {
     return FS_inline_specified | FS_forceinline_specified;
   }
   SourceLocation getInlineSpecLoc() const {
     return FS_inline_specified ? FS_inlineLoc : FS_forceinlineLoc;
   }
 
void UpdateExprRep(Expr *Rep) {
     ExprRep = Rep;
   }
 
   bool SetTypeQual(TQ T, SourceLocation Loc);
 
   bool SetTypeQual(TQ T, SourceLocation Loc, const char *&PrevSpec,
                    unsigned &DiagID, const LangOptions &Lang);
 
+  bool SetGNUAsmQual(AQ A, SourceLocation Loc);
+
   bool setFunctionSpecInline(SourceLocation Loc, const char *&PrevSpec,
                              unsigned &DiagID);
   bool setFunctionSpecForceInline(SourceLocation Loc, const char *&PrevSpec,
                                   unsigned &DiagID);
   bool setFunctionSpecVirtual(SourceLocation Loc, const char *&PrevSpec,
                               unsigned &DiagID);
   bool setFunctionSpecExplicit(SourceLocation Loc, const char *&PrevSpec,
                                unsigned &DiagID, ExplicitSpecifier ExplicitSpec,

clang/lib/Parse/ParseStmtAsm.cpp

case tok::kw___write_only:
     return true;
   }
 }
 
 // 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);
 }
 
 /// 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
 ///
 /// [MS]  ms-asm-block:
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-qualifiers:
+///         volatile
+///         inline
+///         goto
+void Parser::ParseGNUAsmQualifierListOpt(DeclSpec &DS) {
+  SourceLocation EndLoc;
+  while (1) {
+    SourceLocation Loc = Tok.getLocation();
+    DeclSpec::AQ AQ = DeclSpec::AQ_unspecified;
+
+    if (Tok.getKind() == tok::kw_volatile)
+      AQ = DeclSpec::AQ_volatile;
+    else if (Tok.getKind() == tok::kw_inline)
+      AQ = DeclSpec::AQ_inline;
+    else if (Tok.getKind() == tok::kw_goto)
+      AQ = DeclSpec::AQ_goto;
+    else {
+      if (EndLoc.isValid())
+        DS.SetRangeEnd(EndLoc);
+      return;
+    }
+    if (DS.SetGNUAsmQual(AQ, Loc))
+      Diag(Loc, diag::err_asm_duplicate_qual)
+          << DeclSpec::getSpecifierName(AQ);
+    EndLoc = ConsumeToken();
+  }
+}
+
 /// 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[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);
+  ParseGNUAsmQualifierListOpt(DS);
   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;
+  bool isVolatile = DS.GetGNUAsmQual() & DeclSpec::AQ_volatile;
   // Remember if this was a goto asm.
-  bool isGotoAsm = false;
-
-  if (Tok.is(tok::kw_goto)) {
-    isGotoAsm = true;
-    ConsumeToken();
-  }
+  bool isGotoAsm = DS.GetGNUAsmQual() & DeclSpec::AQ_goto;
 
   if (Tok.isNot(tok::l_paren)) {
     Diag(Tok, diag::err_expected_lparen_after) << "asm";
     SkipUntil(tok::r_paren, StopAtSemi);
     return StmtError();
   }
   BalancedDelimiterTracker T(*this, tok::l_paren);
   T.consumeOpen();
if (Res.isInvalid()) {
       SkipUntil(tok::r_paren, StopAtSemi);
       return true;
     }
     Exprs.push_back(Res.get());
     // Eat the comma and continue parsing if it exists.
     if (!TryConsumeToken(tok::comma))
       return false;
   }
 }

clang/lib/Sema/DeclSpec.cpp

const char *DeclSpec::getSpecifierName(TQ T) {
   case DeclSpec::TQ_restrict:    return "restrict";
   case DeclSpec::TQ_volatile:    return "volatile";
   case DeclSpec::TQ_atomic:      return "_Atomic";
   case DeclSpec::TQ_unaligned:   return "__unaligned";
   }
   llvm_unreachable("Unknown typespec!");
 }
 
+const char *DeclSpec::getSpecifierName(AQ A) {
+  switch (A) {
+    case DeclSpec::AQ_unspecified: return "unspecified";
+    case DeclSpec::AQ_volatile: return "volatile";
+    case DeclSpec::AQ_inline: return "inline";
+    case DeclSpec::AQ_goto: return "goto";
+  }
+  llvm_unreachable("Unknown GNUAsmQualifier!");
+}
+
 bool DeclSpec::SetStorageClassSpec(Sema &S, SCS SC, SourceLocation Loc,
                                    const char *&PrevSpec,
                                    unsigned &DiagID,
                                    const PrintingPolicy &Policy) {
   // OpenCL v1.1 s6.8g: "The extern, static, auto and register storage-class
   // specifiers are not supported.
   // It seems sensible to prohibit private_extern too
   // The cl_clang_storage_class_specifiers extension enables support for
bool DeclSpec::SetTypeQual(TQ T, SourceLocation Loc) {
   case TQ_volatile: TQ_volatileLoc = Loc; return false;
   case TQ_unaligned: TQ_unalignedLoc = Loc; return false;
   case TQ_atomic:   TQ_atomicLoc = Loc; return false;
   }
 
   llvm_unreachable("Unknown type qualifier!");
 }
 
+bool DeclSpec::SetGNUAsmQual(AQ A, SourceLocation Loc) {
+  bool IsInvalid = GNUAsmQualifiers & A;
+  GNUAsmQualifiers |= A;
+  return IsInvalid;
+}
+
 bool DeclSpec::setFunctionSpecInline(SourceLocation Loc, const char *&PrevSpec,
                                      unsigned &DiagID) {
   // 'inline inline' is ok.  However, since this is likely not what the user
   // intended, we will always warn, similar to duplicates of type qualifiers.
   if (FS_inline_specified) {
     DiagID = diag::warn_duplicate_declspec;
     PrevSpec = "inline";
     return true;

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 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:;
+}