[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

732–733

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

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

667

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

683

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

683

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

710

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

683

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

710

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

732–733

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

1545

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

355–356

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

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

355

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

941

type vs param name and top-level const.

946

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

960

type vs param name and top-level const.

[nickdesaulniers]

• address latest review comments, more refactoring

clang/lib/Parse/ParseStmtAsm.cpp

946

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

681

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

946

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

 The following options are deprecated and ignored. They will be removed in
 future versions of Clang.
 
 - ...
 
 Modified Compiler Flags
 -----------------------
 
+- -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).
+
 
 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

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);
+  bool 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 : 3; // Bitwise OR of AQ.
+
   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),
+        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), 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;
   }
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 getGNUAsmQualifiers() 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 setGNUAsmQualifier(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

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.
 static bool isGCCAsmStatement(const Token &TokAfterAsm) {
-  return TokAfterAsm.is(tok::l_paren) || TokAfterAsm.is(tok::kw_goto) ||
-         isTypeQualifier(TokAfterAsm);
+  return TokAfterAsm.is(tok::l_paren) || TokAfterAsm.is(tok::kw_volatile) ||
+         TokAfterAsm.is(tok::kw_inline) || TokAfterAsm.is(tok::kw_goto);
 }
 
 /// 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(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);
+      if (Tok.isNot(tok::l_paren)) {
+        SkipUntil(tok::r_paren, StopAtSemi);
+        Diag(Loc, diag::err_asm_qualifier_ignored);
+        return true;
+      }
+      return false;
+    }
+    if (DS.setGNUAsmQualifier(AQ, Loc))
+      Diag(Loc, diag::err_asm_duplicate_qual) << DeclSpec::getSpecifierName(AQ);
+    EndLoc = 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);
+  if (ParseGNUAsmQualifierListOpt(DS))
     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())) {
     const auto *SL = cast<StringLiteral>(AsmString.get());
     if (!SL->getString().trim().empty())
-      Diag(Loc, diag::err_gnu_inline_asm_disabled);
+      Diag(Tok.getLocation(), diag::err_gnu_inline_asm_disabled);
   }
 
   if (AsmString.isInvalid()) {
     // Consume up to and including the closing paren.
     T.skipToEnd();
     return StmtError();
   }
 
   SmallVector<IdentifierInfo *, 4> Names;
   ExprVector Constraints;
   ExprVector Exprs;
   ExprVector Clobbers;
 
+  bool isVolatile = DS.getGNUAsmQualifiers() & DeclSpec::AQ_volatile;
   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());
if (!AteExtraColon && isTokenStringLiteral()) {
 
         Clobbers.push_back(Clobber.get());
 
         if (!TryConsumeToken(tok::comma))
           break;
       }
     }
   }
+  bool isGotoAsm = DS.getGNUAsmQualifiers() & DeclSpec::AQ_goto;
   if (!isGotoAsm && (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 (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/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 (Tok.is(tok::kw_volatile) || Tok.is(tok::kw_goto) ||
+      Tok.is(tok::kw_inline)) {
+    // 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)
+    DeclSpec::AQ AQ = DeclSpec::AQ_unspecified;
+    if (Tok.is(tok::kw_volatile))
+      AQ = DeclSpec::AQ_volatile;
+    else if (Tok.is(tok::kw_goto))
+      AQ = DeclSpec::AQ_goto;
+    else if (Tok.is(tok::kw_inline))
+      AQ = DeclSpec::AQ_inline;
+    Diag(Tok, diag::err_global_asm_qualifier_ignored)
+        << DeclSpec::getSpecifierName(AQ)
         << FixItHint::CreateRemoval(RemovalRange);
     ConsumeToken();
   }
 
   BalancedDelimiterTracker T(*this, tok::l_paren);
   if (T.consumeOpen()) {
     Diag(Tok, diag::err_expected_lparen_after) << "asm";
     return ExprError();
   }

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::setGNUAsmQualifier(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/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) {