Disallow an empty string literal in an asm label

[aaron.ballman]

Summary

An empty string literal in an asm label does not make a whole lot of sense. GCC does not diagnose such a construct, but it also generates code that cannot be assembled by gas should two symbols have an empty asm label within the same TU. This does not affect an asm statement with an empty string literal, which is still a useful construct.

The default parameter was converted to a mandated parameter in ParseSimpleAsm() because all callers passed the argument anyway. The test code in CodeGen\asm-label.c was removed because it was no longer needed to satisfy the PR (it's covered by test code in Parser\asm.c).

[rjmccall]

LGTM

[aaron.ballman]

Accepting based on @rjmccall's LG so I can close.

[aaron.ballman]

Committed in 55a51e1c79a21080289ba88d5eac4bbe54ec4272

clang/include/clang/Basic/DiagnosticParseKinds.td

 def note_force_empty_selector_name : Note<
   "or insert whitespace before ':' to use %0 as parameter name "
   "and have an empty entry in the selector">;
 def err_label_end_of_compound_statement : Error<
   "label at end of compound statement: expected statement">;
 def err_address_of_label_outside_fn : Error<
   "use of address-of-label extension outside of a function body">;
 def err_asm_operand_wide_string_literal : Error<
-  "cannot use %select{unicode|wide}0 string literal in 'asm'">;
+  "cannot use %select{unicode|wide|an empty}0 string literal in 'asm'">;
 def err_expected_selector_for_method : Error<
   "expected selector for Objective-C method">;
 def err_expected_property_name : Error<"expected property name">;
 
 def err_unexpected_at : Error<"unexpected '@' in program">;
 def err_atimport : Error<
 "use of '@import' when modules are disabled">;
 

clang/include/clang/Parse/Parser.h

                                                 ParsingDeclSpec &DS,
                                                 AccessSpecifier AS);
 
   void SkipFunctionBody();
   Decl *ParseFunctionDefinition(ParsingDeclarator &D,
                  const ParsedTemplateInfo &TemplateInfo = ParsedTemplateInfo(),
                  LateParsedAttrList *LateParsedAttrs = nullptr);
   void ParseKNRParamDeclarations(Declarator &D);
-  // EndLoc, if non-NULL, is filled with the location of the last token of
-  // the simple-asm.
-  ExprResult ParseSimpleAsm(SourceLocation *EndLoc = nullptr);
-  ExprResult ParseAsmStringLiteral();
+  // EndLoc is filled with the location of the last token of the simple-asm.
+  ExprResult ParseSimpleAsm(bool ForAsmLabel, SourceLocation *EndLoc);
+  ExprResult ParseAsmStringLiteral(bool ForAsmLabel);
 
   // Objective-C External Declarations
   void MaybeSkipAttributes(tok::ObjCKeywordKind Kind);
   DeclGroupPtrTy ParseObjCAtDirectives(ParsedAttributesWithRange &Attrs);
   DeclGroupPtrTy ParseObjCAtClassDeclaration(SourceLocation atLoc);
   Decl *ParseObjCAtInterfaceDeclaration(SourceLocation AtLoc,
                                         ParsedAttributes &prefixAttrs);
   class ObjCTypeParamListScope;

clang/lib/Parse/ParseDecl.cpp

 }
 
 /// Parse an optional simple-asm-expr and attributes, and attach them to a
 /// declarator. Returns true on an error.
 bool Parser::ParseAsmAttributesAfterDeclarator(Declarator &D) {
   // If a simple-asm-expr is present, parse it.
   if (Tok.is(tok::kw_asm)) {
     SourceLocation Loc;
-    ExprResult AsmLabel(ParseSimpleAsm(&Loc));
+    ExprResult AsmLabel(ParseSimpleAsm(/*ForAsmLabel*/ true, &Loc));
     if (AsmLabel.isInvalid()) {
       SkipUntil(tok::semi, StopBeforeMatch);
       return true;
     }
 
     D.setAsmLabel(AsmLabel.get());
     D.SetRangeEnd(Loc);
   }

clang/lib/Parse/ParseDeclCXX.cpp

         DeclaratorInfo.getDeclSpec().getFriendSpecLoc());
     if (!VS.isUnset())
       MaybeParseAndDiagnoseDeclSpecAfterCXX11VirtSpecifierSeq(DeclaratorInfo, VS);
   }
 
   // If a simple-asm-expr is present, parse it.
   if (Tok.is(tok::kw_asm)) {
     SourceLocation Loc;
-    ExprResult AsmLabel(ParseSimpleAsm(&Loc));
+    ExprResult AsmLabel(ParseSimpleAsm(/*ForAsmLabel*/ true, &Loc));
     if (AsmLabel.isInvalid())
       SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
 
     DeclaratorInfo.setAsmLabel(AsmLabel.get());
     DeclaratorInfo.SetRangeEnd(Loc);
   }
 
   // If attributes exist after the declarator, but before an '{', parse them.

clang/lib/Parse/ParseExprCXX.cpp

 
   // declarator
   Declarator DeclaratorInfo(DS, DeclaratorContext::ConditionContext);
   ParseDeclarator(DeclaratorInfo);
 
   // simple-asm-expr[opt]
   if (Tok.is(tok::kw_asm)) {
     SourceLocation Loc;
-    ExprResult AsmLabel(ParseSimpleAsm(&Loc));
+    ExprResult AsmLabel(ParseSimpleAsm(/*ForAsmLabel*/ true, &Loc));
     if (AsmLabel.isInvalid()) {
       SkipUntil(tok::semi, StopAtSemi);
       return Sema::ConditionError();
     }
     DeclaratorInfo.setAsmLabel(AsmLabel.get());
     DeclaratorInfo.SetRangeEnd(Loc);
   }
 

clang/lib/Parse/ParseStmtAsm.cpp

StmtResult Parser::ParseAsmStatement(bool &msAsm) {
   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();
 
-  ExprResult AsmString(ParseAsmStringLiteral());
+  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);
   }
if (AteExtraColon)
       AteExtraColon = false;
     else {
       AteExtraColon = Tok.is(tok::coloncolon);
       ConsumeToken();
     }
     // Parse the asm-string list for clobbers if present.
     if (!AteExtraColon && isTokenStringLiteral()) {
       while (1) {
-        ExprResult Clobber(ParseAsmStringLiteral());
+        ExprResult Clobber(ParseAsmStringLiteral(/*ForAsmLabel*/ false));
 
         if (Clobber.isInvalid())
           break;
 
         Clobbers.push_back(Clobber.get());
 
         if (!TryConsumeToken(tok::comma))
           break;
if (Tok.is(tok::l_square)) {
       IdentifierInfo *II = Tok.getIdentifierInfo();
       ConsumeToken();
 
       Names.push_back(II);
       T.consumeClose();
     } else
       Names.push_back(nullptr);
 
-    ExprResult Constraint(ParseAsmStringLiteral());
+    ExprResult Constraint(ParseAsmStringLiteral(/*ForAsmLabel*/ false));
     if (Constraint.isInvalid()) {
       SkipUntil(tok::r_paren, StopAtSemi);
       return true;
     }
     Constraints.push_back(Constraint.get());
 
     if (Tok.isNot(tok::l_paren)) {
       Diag(Tok, diag::err_expected_lparen_after) << "asm operand";

clang/lib/Parse/Parser.cpp

case tok::kw___extension__: {
     return ParseExternalDeclaration(attrs);
   }
   case tok::kw_asm: {
     ProhibitAttributes(attrs);
 
     SourceLocation StartLoc = Tok.getLocation();
     SourceLocation EndLoc;
 
-    ExprResult Result(ParseSimpleAsm(&EndLoc));
+    ExprResult Result(ParseSimpleAsm(/*ForAsmLabel*/ false, &EndLoc));
 
     // Check if GNU-style InlineAsm is disabled.
     // Empty asm string is allowed because it will not introduce
     // any assembly code.
     if (!(getLangOpts().GNUAsm || Result.isInvalid())) {
       const auto *SL = cast<StringLiteral>(Result.get());
       if (!SL->getString().trim().empty())
         Diag(StartLoc, diag::err_gnu_inline_asm_disabled);
void Parser::ParseKNRParamDeclarations(Declarator &D) {
 
   // The actions module must verify that all arguments were declared.
   Actions.ActOnFinishKNRParamDeclarations(getCurScope(), D, Tok.getLocation());
 }
 
 
 /// ParseAsmStringLiteral - This is just a normal string-literal, but is not
 /// allowed to be a wide string, and is not subject to character translation.
+/// Unlike GCC, we also diagnose an empty string literal when parsing for an
+/// asm label as opposed to an asm statement, because such a construct does not
+/// behave well.
 ///
 /// [GNU] asm-string-literal:
 ///         string-literal
 ///
-ExprResult Parser::ParseAsmStringLiteral() {
+ExprResult Parser::ParseAsmStringLiteral(bool ForAsmLabel) {
   if (!isTokenStringLiteral()) {
     Diag(Tok, diag::err_expected_string_literal)
       << /*Source='in...'*/0 << "'asm'";
     return ExprError();
   }
 
   ExprResult AsmString(ParseStringLiteralExpression());
   if (!AsmString.isInvalid()) {
     const auto *SL = cast<StringLiteral>(AsmString.get());
     if (!SL->isAscii()) {
       Diag(Tok, diag::err_asm_operand_wide_string_literal)
         << SL->isWide()
         << SL->getSourceRange();
       return ExprError();
     }
+    if (ForAsmLabel && SL->getString().empty()) {
+      Diag(Tok, diag::err_asm_operand_wide_string_literal)
+          << 2 /* an empty */ << SL->getSourceRange();
+      return ExprError();
+    }
   }
   return AsmString;
 }
 
 /// ParseSimpleAsm
 ///
 /// [GNU] simple-asm-expr:
 ///         'asm' '(' asm-string-literal ')'
 ///
-ExprResult Parser::ParseSimpleAsm(SourceLocation *EndLoc) {
+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'.
     SourceRange RemovalRange(PP.getLocForEndOfToken(Loc),
                              PP.getLocForEndOfToken(Tok.getLocation()));
 
     Diag(Tok, diag::warn_file_asm_volatile)
       << 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());
+  ExprResult Result(ParseAsmStringLiteral(ForAsmLabel));
 
   if (!Result.isInvalid()) {
     // Close the paren and get the location of the end bracket
     T.consumeClose();
     if (EndLoc)
       *EndLoc = T.getCloseLocation();
   } else if (SkipUntil(tok::r_paren, StopAtSemi | StopBeforeMatch)) {
     if (EndLoc)

clang/test/AST/ast-print-attr.c

 // RUN: %clang_cc1 -ast-print -x objective-c++ -fms-extensions %s -o - | FileCheck %s
 
 // CHECK: using A = __kindof id (*)[1];
 using A = __kindof id (*)[1];
 
 // CHECK: using B = int ** __ptr32 *[3];
 using B = int ** __ptr32 *[3];
 
 // FIXME: This is the wrong spelling for the attribute.
 // FIXME: Too many parens here!
 // CHECK: using C = int ((*))() __attribute__((cdecl));
 using C = int (*)() [[gnu::cdecl]];
 
-// CHECK: int fun_asm() asm("");
-int fun_asm() asm("");
-// CHECK: int var_asm asm("");
-int var_asm asm("");
+// CHECK: int fun_asm() asm("test");
+int fun_asm() asm("test");
+// CHECK: int var_asm asm("test");
+int var_asm asm("test");

clang/test/CodeGen/asm-label.c

 
 // LINUX: @bar = internal global i32 0
 // LINUX: @foo = common global i32 0
 // LINUX: declare i8* @alias(i32)
 
 // DARWIN: @"\01bar" = internal global i32 0
 // DARWIN: @"\01foo" = common global i32 0
 // DARWIN: declare i8* @"\01alias"(i32)
-
-// PR7887
-int pr7887_1 asm("");
-extern int pr7887_2 asm("");
-int pr7887_3 () asm("");
-
-int pt7887_4 () {
-  static int y asm("");
-  y = pr7887_3();
-  pr7887_2 = 1;
-  return pr7887_1;
-}

clang/test/Parser/asm.c

void f2() {
 
   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
 __asm ; // expected-error {{expected '(' after 'asm'}}
 
 // <rdar://problem/10465079> - Don't crash on wide string literals in 'asm'.
 int foo asm (L"bar"); // expected-error {{cannot use wide string literal in 'asm'}}
 
 asm() // expected-error {{expected string literal in 'asm'}}