MC asm parser: allow @'s in symbol names in MS assembly

[hans]

Summary

This is another (final?) stab at making us able to parse our own asm output.

On Windows, it's not uncommon to have @'s and ?'s show up in symbol names. Our parser didn't like this. The ?'s were rejected, and the @'s were interpreted as trying to reference something like PLT or GOT.

My previous solution of quoting these names didn't work, as for some targets we use the gas assembler, which doesn't like quotes in certain places (most notably in .def directives).

Instead, I think we should try to parse these symbol names without quotes, as gas does.

This is probably not perfect, but should allow us to start being able to parse our asm output on windows.

[majnemer]

lib/MC/MCParser/AsmParser.cpp

2129

Should this be gated on doesAllowAtInName?

[hans]

lib/MC/MCParser/AsmParser.cpp

2129

I don't know, we already allow @'s in names during parsing. This just allows them as the initial character, and I don't think this introduces any ambiguity.

The doesAllowAtInName() is more like "is the @ part of the name for realz, or does it signify the separator between the name and PLT/GOT/etc".

If we gate it here, we'd have to gate it in the switch in parsePrimaryExpr() too.

[• rafael]

Do we already have a testcase showing that we reject foo@bar on ELF? If not, it would be a good idea to add one.

lib/MC/MCParser/AsmLexer.cpp

143

adding support for ? in a name can be an independent change, no?

btw, on linux gas rejects

.globl foo?bar

Should be reject too? I am tempted to just accept it everywhere as an extension, since it doesn't look ambiguous (unlike the @)

[hans]

Do we already have a testcase showing that we reject foo@bar on ELF? If not, it would be a good idea to add one.

We don't. I'll add one.

lib/MC/MCParser/AsmLexer.cpp

143

Right, this would be an extension, and like you say I think it would be harmless - I don't think the lexer actually accepts in anywhere else.

gas doesn't accept this, but these symbols should not be showing up in asm the we send to gas.

I could do this in an independent change, but I think it ties in nicely with the test I'm adding here. Happy to split it out if you think it's important though.

[hans]

Add another test.

[• rafael]

I could do this in an independent change, but I think it ties in nicely with the test I'm adding here. Happy to split it out if you think it's important though.

Nah, it is fine. Just add a test showing we accept in on ELF too with
a comment saying it is an extension.

Cheers,
Rafael

[hans]

Closed by commit rL193000 (authored by @hans).

include/llvm/MC/MCAsmInfo.h

protected:
     /// AllowNameToStartWithDigit - This is true if the assembler allows symbol
     /// names to start with a digit (e.g., "0x0021").  This defaults to false.
     bool AllowNameToStartWithDigit;
 
     /// AllowPeriodsInName - This is true if the assembler allows periods in
     /// symbol names.  This defaults to true.
     bool AllowPeriodsInName;
 
+    /// \brief This is true if the assembler allows @ characters in symbol
+    /// names. Defaults to false.
+    bool AllowAtInName;
+
     /// AllowUTF8 - This is true if the assembler accepts UTF-8 input.
     // FIXME: Make this a more general encoding setting?
     bool AllowUTF8;
 
     /// UseDataRegionDirectives - This is true if data region markers should
     /// be printed as ".data_region/.end_data_region" directives. If false,
     /// use "$d/$a" labels instead.
     bool UseDataRegionDirectives;
bool doesAllowQuotesInName() const {
       return AllowQuotesInName;
     }
     bool doesAllowNameToStartWithDigit() const {
       return AllowNameToStartWithDigit;
     }
     bool doesAllowPeriodsInName() const {
       return AllowPeriodsInName;
     }
+    bool doesAllowAtInName() const {
+      return AllowAtInName;
+    }
     bool doesAllowUTF8() const {
       return AllowUTF8;
     }
     bool doesSupportDataRegionDirectives() const {
       return UseDataRegionDirectives;
     }
     const char *getZeroDirective() const {
       return ZeroDirective;

lib/MC/MCAsmInfo.cpp

MCAsmInfo::MCAsmInfo() {
   InlineAsmEnd = "NO_APP";
   Code16Directive = ".code16";
   Code32Directive = ".code32";
   Code64Directive = ".code64";
   AssemblerDialect = 0;
   AllowQuotesInName = false;
   AllowNameToStartWithDigit = false;
   AllowPeriodsInName = true;
+  AllowAtInName = false;
   AllowUTF8 = true;
   UseDataRegionDirectives = false;
   ZeroDirective = "\t.zero\t";
   AsciiDirective = "\t.ascii\t";
   AscizDirective = "\t.asciz\t";
   Data8bitsDirective = "\t.byte\t";
   Data16bitsDirective = "\t.short\t";
   Data32bitsDirective = "\t.long\t";

lib/MC/MCParser/AsmLexer.cpp

AsmToken AsmLexer::LexHexFloatLiteral(bool NoIntDigits) {
 
   if (CurPtr == ExpStart)
     return ReturnError(TokStart, "invalid hexadecimal floating-point constant: "
                                  "expected at least one exponent digit");
 
   return AsmToken(AsmToken::Real, StringRef(TokStart, CurPtr - TokStart));
 }
 
-/// LexIdentifier: [a-zA-Z_.][a-zA-Z0-9_$.@]*
+/// LexIdentifier: [a-zA-Z_.][a-zA-Z0-9_$.@?]*
 static bool IsIdentifierChar(char c) {
-  return isalnum(c) || c == '_' || c == '$' || c == '.' || c == '@';
+  return isalnum(c) || c == '_' || c == '$' || c == '.' || c == '@' || c == '?';
 }
 AsmToken AsmLexer::LexIdentifier() {
   // Check for floating point literals.
   if (CurPtr[-1] == '.' && isdigit(*CurPtr)) {
     // Disambiguate a .1243foo identifier from a floating literal.
     while (isdigit(*CurPtr))
       ++CurPtr;
     if (*CurPtr == 'e' || *CurPtr == 'E' || !IsIdentifierChar(*CurPtr))

lib/MC/MCParser/AsmParser.cpp

case AsmToken::Error:
     return true;
   case AsmToken::Exclaim:
     Lex(); // Eat the operator.
     if (parsePrimaryExpr(Res, EndLoc))
       return true;
     Res = MCUnaryExpr::CreateLNot(Res, getContext());
     return false;
   case AsmToken::Dollar:
+  case AsmToken::At:
   case AsmToken::String:
   case AsmToken::Identifier: {
     StringRef Identifier;
     if (parseIdentifier(Identifier)) {
       if (FirstTokenKind == AsmToken::Dollar) {
         if (Lexer.getMAI().getDollarIsPC()) {
           // This is a '$' reference, which references the current PC.  Emit a
           // temporary label to the streamer and refer to it.
           MCSymbol *Sym = Ctx.CreateTempSymbol();
           Out.EmitLabel(Sym);
           Res = MCSymbolRefExpr::Create(Sym, MCSymbolRefExpr::VK_None,
                                         getContext());
           EndLoc = FirstTokenLoc;
           return false;
         } else
           return Error(FirstTokenLoc, "invalid token in expression");
         return true;
       }
     }
 
     EndLoc = SMLoc::getFromPointer(Identifier.end());
 
     // This is a symbol reference.
+    StringRef SymbolName = Identifier;
+    MCSymbolRefExpr::VariantKind Variant = MCSymbolRefExpr::VK_None;
     std::pair<StringRef, StringRef> Split = Identifier.split('@');
-    MCSymbol *Sym = getContext().GetOrCreateSymbol(Split.first);
 
     // Lookup the symbol variant if used.
-    MCSymbolRefExpr::VariantKind Variant = MCSymbolRefExpr::VK_None;
     if (Split.first.size() != Identifier.size()) {
       Variant = MCSymbolRefExpr::getVariantKindForName(Split.second);
-      if (Variant == MCSymbolRefExpr::VK_Invalid) {
+      if (Variant != MCSymbolRefExpr::VK_Invalid) {
+        SymbolName = Split.first;
+      } else if (MAI.doesAllowAtInName()) {
+        Variant = MCSymbolRefExpr::VK_None;
+      } else {
         Variant = MCSymbolRefExpr::VK_None;
         return TokError("invalid variant '" + Split.second + "'");
       }
     }
 
+    MCSymbol *Sym = getContext().GetOrCreateSymbol(SymbolName);
+
     // If this is an absolute variable reference, substitute it now to preserve
     // semantics in the face of reassignment.
     if (Sym->isVariable() && isa<MCConstantExpr>(Sym->getVariableValue())) {
       if (Variant)
         return Error(EndLoc, "unexpected modifier on variable reference");
 
       Res = Sym->getVariableValue();
       return false;
bool AsmParser::parseAssignment(StringRef Name, bool allow_redef,
   return false;
 }
 
 /// parseIdentifier:
 ///   ::= identifier
 ///   ::= string
 bool AsmParser::parseIdentifier(StringRef &Res) {
   // The assembler has relaxed rules for accepting identifiers, in particular we
-  // allow things like '.globl $foo', which would normally be separate
-  // tokens. At this level, we have already lexed so we cannot (currently)
+  // allow things like '.globl $foo' and '.def @feat.00', which would normally be
+  // separate tokens. At this level, we have already lexed so we cannot (currently)
   // handle this as a context dependent token, instead we detect adjacent tokens
   // and return the combined identifier.
-  if (Lexer.is(AsmToken::Dollar)) {
-    SMLoc DollarLoc = getLexer().getLoc();
+  if (Lexer.is(AsmToken::Dollar) || Lexer.is(AsmToken::At)) {
+    SMLoc PrefixLoc = getLexer().getLoc();
 
-    // Consume the dollar sign, and check for a following identifier.
+    // Consume the prefix character, and check for a following identifier.
     Lex();
     if (Lexer.isNot(AsmToken::Identifier))
       return true;
 
-    // We have a '$' followed by an identifier, make sure they are adjacent.
-    if (DollarLoc.getPointer() + 1 != getTok().getLoc().getPointer())
+    // We have a '$' or '@' followed by an identifier, make sure they are adjacent.
+    if (PrefixLoc.getPointer() + 1 != getTok().getLoc().getPointer())
       return true;
 
     // Construct the joined identifier and consume the token.
     Res =
-        StringRef(DollarLoc.getPointer(), getTok().getIdentifier().size() + 1);
+        StringRef(PrefixLoc.getPointer(), getTok().getIdentifier().size() + 1);
     Lex();
     return false;
   }
 
   if (Lexer.isNot(AsmToken::Identifier) && Lexer.isNot(AsmToken::String))
     return true;
 
   Res = getTok().getIdentifier();

lib/Target/X86/MCTargetDesc/X86MCAsmInfo.cpp

X86MCAsmInfoMicrosoft::X86MCAsmInfoMicrosoft(const Triple &Triple) {
   if (Triple.getArch() == Triple::x86_64) {
     GlobalPrefix = "";
     PrivateGlobalPrefix = ".L";
   }
 
   AssemblerDialect = AsmWriterFlavor;
 
   TextAlignFillValue = 0x90;
+
+  AllowAtInName = true;
 }
 
 void X86MCAsmInfoGNUCOFF::anchor() { }
 
 X86MCAsmInfoGNUCOFF::X86MCAsmInfoGNUCOFF(const Triple &Triple) {
   if (Triple.getArch() == Triple::x86_64) {
     GlobalPrefix = "";
     PrivateGlobalPrefix = ".L";

test/MC/COFF/tricky-names.ll

+; Check how tricky symbols are printed in the asm output.
+; RUN: llc -mtriple=i686-pc-win32 %s -o - | FileCheck %s --check-prefix=ASM
+
+; Check that we can roundtrip these names through our assembler.
+; RUN: llc -mtriple=i686-pc-win32 %s -o - | llvm-mc -triple i686-pc-win32 -filetype=obj | llvm-readobj -t | FileCheck %s --check-prefix=READOBJ
+
+
+@"\01??__E_Generic_object@?$_Error_objects@H@std@@YAXXZ" = global i32 0
+@"\01__ZL16ExceptionHandlerP19_EXCEPTION_POINTERS@4" = global i32 0
+@"\[e-mail]" = global i32 0
+
+define weak i32 @"\01??_B?$num_put@_WV?$back_insert_iterator@V?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@@std@@@std@@51"() section ".text" {
+  %a = load i32* @"\01??__E_Generic_object@?$_Error_objects@H@std@@YAXXZ"
+  %b = load i32* @"\01__ZL16ExceptionHandlerP19_EXCEPTION_POINTERS@4"
+  %c = load i32* @"\[e-mail]"
+  %x = add i32 %a, %b
+  %y = add i32 %x, %c
+  ret i32 %y
+}
+
+; Check that these symbols are not quoted. They occur in output that gets passed to GAS.
+; ASM: .globl __ZL16ExceptionHandlerP19_EXCEPTION_POINTERS@4
+; ASM-NOT: .globl "__ZL16ExceptionHandlerP19_EXCEPTION_POINTERS@4"
+; ASM: .globl @foo.bar
+; ASM-NOT: .globl "@foo.bar"
+
+; READOBJ: Symbol
+; READOBJ: Name: .text$??_B?$num_put@_WV?$back_insert_iterator@V?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@@std@@@std@@51
+; READOBJ: Section: .text$??_B?$num_put@_WV?$back_insert_iterator@V?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@@std@@@std@@51
+; READOBJ: Symbol
+; READOBJ: Name: ??_B?$num_put@_WV?$back_insert_iterator@V?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@@std@@@std@@51
+; READOBJ: Section: .text$??_B?$num_put@_WV?$back_insert_iterator@V?$basic_string@_WU?$char_traits@_W@std@@V?$allocator@_W@2@@std@@@std@@@std@@51
+; READOBJ: Symbol
+; READOBJ: Name: ??__E_Generic_object@?$_Error_objects@H@std@@YAXXZ
+; READOBJ: Symbol
+; READOBJ: Name: __ZL16ExceptionHandlerP19_EXCEPTION_POINTERS@4
+; READOBJ: Symbol
+; READOBJ: Name: @foo.bar

test/MC/ELF/bad-relocation.s

+// RUN: not llvm-mc -filetype=obj -triple i386-pc-linux-gnu %s -o /dev/null 2>&1 | FileCheck  %s
+
+// CHECK: error: invalid variant 'BADRELOC'
+
+        .text
+foo:
+  leal  .Lfoo@BADRELOC(%ebx), %eax