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

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

Reviewers: aaron.ballman

Reviewed By: aaron.ballman

Subscribers: aheejin, jfb, nathanchance, cfe-commits, echristo, efriedma, rsmith, chandlerc, craig.topper, erichkeane, jyu2, void, srhines

Tags: #clang

Differential Revision: https://reviews.llvm.org/D75563

clang/docs/ReleaseNotes.rst

@@ -91,6 +91,13 @@ Modified Compiler Flags
   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
 --------------------
@@ -111,6 +118,9 @@ 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/include/clang/Basic/DiagnosticGroups.td

@@ -1090,9 +1090,8 @@ def ObjCSignedCharBool : DiagGroup<"objc-signed-char-bool",
 
 // Inline ASM warnings.
 def ASMOperandWidths : DiagGroup<"asm-operand-widths">;
-def ASMIgnoredQualifier : DiagGroup<"asm-ignored-qualifier">;
 def ASM : DiagGroup<"asm", [
-    ASMOperandWidths, ASMIgnoredQualifier
+    ASMOperandWidths
   ]>;
 
 // OpenMP warnings.

clang/include/clang/Basic/DiagnosticParseKinds.td

@@ -12,11 +12,10 @@
 
 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">;
@@ -29,6 +28,7 @@ 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 {

clang/include/clang/Parse/Parser.h

@@ -3201,6 +3201,27 @@ private:
                                  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

clang/lib/Parse/ParseStmtAsm.cpp

@@ -349,31 +349,13 @@ static bool buildMSAsmString(Preprocessor &PP, SourceLocation AsmLoc,
   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,
@@ -684,13 +666,41 @@ StmtResult Parser::ParseMicrosoftAsmStatement(SourceLocation AsmLoc) {
                                 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
@@ -712,34 +722,11 @@ StmtResult Parser::ParseAsmStatement(bool &msAsm) {
     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();
 
@@ -767,11 +754,10 @@ StmtResult Parser::ParseAsmStatement(bool &msAsm) {
   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.
@@ -829,7 +815,7 @@ StmtResult Parser::ParseAsmStatement(bool &msAsm) {
       }
     }
   }
-  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();
@@ -862,16 +848,16 @@ StmtResult Parser::ParseAsmStatement(bool &msAsm) {
       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,
-      T.getCloseLocation());
+  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
@@ -942,3 +928,28 @@ bool Parser::ParseAsmOperandsOpt(SmallVectorImpl<IdentifierInfo *> &Names,
       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

@@ -1529,13 +1529,13 @@ 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)
-      << FixItHint::CreateRemoval(RemovalRange);
+    Diag(Tok, diag::err_global_asm_qualifier_ignored)
+        << GNUAsmQualifiers::getQualifierName(getGNUAsmQualifier(Tok))
+        << FixItHint::CreateRemoval(RemovalRange);
     ConsumeToken();
   }
 

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

@@ -14,11 +14,6 @@ void f() {
   // 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
 

clang/test/Parser/asm-qualifiers.c

@@ -0,0 +1,59 @@
+// 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

@@ -12,12 +12,6 @@ void f1() {
 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'}}

clang/test/Sema/asm.c

@@ -91,9 +91,6 @@ int test7(unsigned long long b) {
   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.