[Inline asm] Correctly parse GCC-style asm line following MS-style asm line

[• d.zobnin.bugzilla]

Summary

Clang fails to compile the following code:

void f() {

__asm mov ebx, ecx
__asm__("movl %ecx, %edx");

}

reporting unexpected token "asm" at start of statement.
Quit parsing MS-style assembly if the following statement has GCC style to handle such code.

[rsmith]

lib/Parse/ParseStmtAsm.cpp

423 ↗

(On Diff #52222)

The next token in GCC-style inline asm could also be volatile (or const or goto -- though we don't support the latter).

Please make this GCC / MS dialect selection the same way that ParseAsmStatement does (and factor out a static isGCCAsmStatement(Token &TokAfterAsm) function or similar).

[• d.zobnin.bugzilla]

Richard, thank you for the review!

I've created a static function to check if this is a GCC asm statement as you recommended to do, and also made "isTypeQualifier" function static.
I've added a check for "goto" after "asm" to treat such statements as GCC-style assembly.
Also, do you think it will be better to diagnose unsupported "asm goto" statements rather than fall into parsing them and emit "expected identifier" error or something like that?

Thank you,
Denis Zobnin

[• d.zobnin.bugzilla]

Richard, Eric,

Please take a look at the patch.

Thank you,
Denis Zobnin

[rsmith]

LGTM, thank you!

cfe/trunk/include/clang/Basic/DiagnosticParseKinds.td

 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_not_supported_yet : Error<
+  "'asm goto' constructs are not supported yet">;
 }
 
 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<

cfe/trunk/include/clang/Parse/Parser.h

void ParseStructUnionBody(SourceLocation StartLoc, unsigned TagType,
                             Decl *TagDecl);
 
   void ParseStructDeclaration(
       ParsingDeclSpec &DS,
       llvm::function_ref<void(ParsingFieldDeclarator &)> FieldsCallback);
 
   bool isDeclarationSpecifier(bool DisambiguatingWithExpression = false);
   bool isTypeSpecifierQualifier();
-  bool isTypeQualifier() const;
 
   /// isKnownToBeTypeSpecifier - Return true if we know that the specified token
   /// is definitely a type-specifier.  Return false if it isn't part of a type
   /// specifier or if we're not sure.
   bool isKnownToBeTypeSpecifier(const Token &Tok) const;
 
   /// \brief Return true if we know that we are definitely looking at a
   /// decl-specifier, and isn't part of an expression such as a function-style

cfe/trunk/lib/Parse/ParseDecl.cpp

if (!isValidAfterTypeSpecifier(CanBeBitfield)) {
     // Push this token back into the preprocessor and change our current token
     // to ';' so that the rest of the code recovers as though there were an
     // ';' after the definition.
     PP.EnterToken(Tok);
     Tok.setKind(tok::semi);
   }
 }
 
-/// isTypeSpecifierQualifier - Return true if the current token could be the
-/// start of a type-qualifier-list.
-bool Parser::isTypeQualifier() const {
-  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;
-  }
-}
-
 /// isKnownToBeTypeSpecifier - Return true if we know that the specified token
 /// is definitely a type-specifier.  Return false if it isn't part of a type
 /// specifier or if we're not sure.
 bool Parser::isKnownToBeTypeSpecifier(const Token &Tok) const {
   switch (Tok.getKind()) {
   default: return false;
     // type-specifiers
   case tok::kw_short:

cfe/trunk/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);
+}
+
 /// 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:
do {
     } else if (SingleLineMode || InAsmComment) {
       // If end-of-line is significant, check whether this token is on a
       // new line.
       std::pair<FileID, unsigned> ExpLoc =
           SrcMgr.getDecomposedExpansionLoc(TokLoc);
       if (ExpLoc.first != FID ||
           SrcMgr.getLineNumber(ExpLoc.first, ExpLoc.second) != LineNo) {
         // If this is a single-line __asm, we're done, except if the next
-        // line begins with an __asm too, in which case we finish a comment
+        // line is MS-style asm too, in which case we finish a comment
         // if needed and then keep processing the next line as a single
         // line __asm.
         bool isAsm = Tok.is(tok::kw_asm);
-        if (SingleLineMode && !isAsm)
+        if (SingleLineMode && (!isAsm || isGCCAsmStatement(NextToken())))
           break;
         // We're no longer in a comment.
         InAsmComment = false;
         if (isAsm) {
           // If this is a new __asm {} block we want to process it seperately
           // from the single-line __asm statements
           if (PP.LookAhead(0).is(tok::l_brace))
             break;

 /// [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 && Tok.isNot(tok::l_paren) &&
-      !isTypeQualifier()) {
+  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::w_asm_qualifier_ignored) << "const";
   if (DS.getTypeQualifiers() & DeclSpec::TQ_restrict)
     Diag(Loc, diag::w_asm_qualifier_ignored) << "restrict";
   // FIXME: Once GCC supports _Atomic, check whether it permits it here.
   if (DS.getTypeQualifiers() & DeclSpec::TQ_atomic)
     Diag(Loc, diag::w_asm_qualifier_ignored) << "_Atomic";
 
   // Remember if this was a volatile asm.
   bool isVolatile = DS.getTypeQualifiers() & DeclSpec::TQ_volatile;
+
+  // TODO: support "asm goto" constructs (PR#9295).
+  if (Tok.is(tok::kw_goto)) {
+    Diag(Tok, diag::err_asm_goto_not_supported_yet);
+    SkipUntil(tok::r_paren, StopAtSemi);
+    return StmtError();
+  }
+
   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();
 

cfe/trunk/test/CodeGen/inline-asm-mixed-style.c

+// RUN: %clang_cc1 -triple i386-unknown-unknown -fasm-blocks -fsyntax-only -verify %s -DCHECK_ASM_GOTO
+// RUN: %clang_cc1 -triple i386-unknown-unknown -fasm-blocks -O0 -emit-llvm -S %s -o - | FileCheck %s
+
+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
+
+#ifdef CHECK_ASM_GOTO
+  __asm volatile goto ("movl %ecx, %edx"); // expected-error {{'asm goto' constructs are not supported yet}}
+
+  __asm mov eax, ebx
+  __asm goto ("movl %ecx, %edx"); // expected-error {{'asm goto' constructs are not supported yet}}
+#endif
+}