[ms] [llvm-ml] Standardize blocking of lexical substitution

[epastor]

Summary

In MASM, the ifdef family of directives treats its argument literally, without expanding it as a text macro. Add support for this, and also replace the special handling that was previously used for echo.

[thakis]

Nice! lg with clang-tidy addressed.

llvm/lib/MC/MCParser/MasmParser.cpp

495–497

nit: Consider instead:

enum ExpandKind { ExpandMacros, DoNotExpandMacros };
const AsmToken &Lex(ExpandKind);

Then the call sites are much easier to read. (Also something similar for the other method below taking a bool below)

1127

Why is this a pointer? You never set it to anything else and it can't be null -- make it a reference?

1143

no else after return…oh, clang-tidy already said that. Do what clang-tidy says :) (also above)

[epastor]

Address feedback (including clang-tidy)

llvm/lib/MC/MCParser/MasmParser.cpp

public:
   void setAssemblerDialect(unsigned i) override {
     AssemblerDialect = i;
   }
 
   void Note(SMLoc L, const Twine &Msg, SMRange Range = None) override;
   bool Warning(SMLoc L, const Twine &Msg, SMRange Range = None) override;
   bool printError(SMLoc L, const Twine &Msg, SMRange Range = None) override;
 
-  const AsmToken &Lex() override;
+  enum ExpandKind { ExpandMacros, DoNotExpandMacros };
+  const AsmToken &Lex(ExpandKind ExpandNextToken);
+  const AsmToken &Lex() override { return Lex(ExpandMacros); }
 
   void setParsingMSInlineAsm(bool V) override {
     ParsingMSInlineAsm = V;
     // When parsing MS inline asm, we must lex 0b1101 and 0ABCH as binary and
     // hex integer literals.
     Lexer.setLexMasmIntegers(V);
   }
   bool isParsingMSInlineAsm() override { return ParsingMSInlineAsm; }
public:
   bool parseAbsoluteExpression(int64_t &Res) override;
 
   /// Parse a floating point expression using the float \p Semantics
   /// and set \p Res to the value.
   bool parseRealValue(const fltSemantics &Semantics, APInt &Res);
 
   /// Parse an identifier or string (as a quoted identifier)
   /// and set \p Res to the identifier contents.
-  bool parseIdentifier(StringRef &Res) override;
+  enum IdentifierPositionKind { StandardPosition, StartOfStatement };
+  bool parseIdentifier(StringRef &Res, IdentifierPositionKind Position);
+  bool parseIdentifier(StringRef &Res) override {
+    return parseIdentifier(Res, StandardPosition);
+  }
   void eatToEndOfStatement() override;
 
   bool checkForValidSection() override;
 
   /// }
 
 private:
+  bool expandMacros();
   const AsmToken peekTok(bool ShouldSkipSpace = true);
 
   bool parseStatement(ParseStatementInfo &Info,
                       MCAsmParserSemaCallback *SI);
   bool parseCurlyBlockScope(SmallVectorImpl<AsmRewrite>& AsmStrRewrites);
   bool parseCppHashLineFilenameComment(SMLoc L);
 
   bool expandMacro(raw_svector_ostream &OS, StringRef Body,
bool parseDirectiveErrorIfidn(SMLoc DirectiveLoc, bool ExpectEqual,
                                 bool CaseInsensitive);
   // ".erre" or ".errnz", depending on ExpectZero.
   bool parseDirectiveErrorIfe(SMLoc DirectiveLoc, bool ExpectZero);
 
   // ".radix"
   bool parseDirectiveRadix(SMLoc DirectiveLoc);
 
   // "echo"
-  bool parseDirectiveEcho();
+  bool parseDirectiveEcho(SMLoc DirectiveLoc);
 
   void initializeDirectiveKindMap();
   void initializeCVDefRangeTypeMap();
 };
 
 } // end anonymous namespace
 
 namespace llvm {

 
 void MasmParser::jumpToLoc(SMLoc Loc, unsigned InBuffer,
                            bool EndStatementAtEOF) {
   CurBuffer = InBuffer ? InBuffer : SrcMgr.FindBufferContainingLoc(Loc);
   Lexer.setBuffer(SrcMgr.getMemoryBuffer(CurBuffer)->getBuffer(),
                   Loc.getPointer(), EndStatementAtEOF);
 }
 
-const AsmToken &MasmParser::Lex() {
+bool MasmParser::expandMacros() {
+  const AsmToken &Tok = getTok();
+
+  auto VarIt = Variables.find(Tok.getIdentifier().lower());
+  if (VarIt != Variables.end() && VarIt->second.IsText) {
+    std::unique_ptr<MemoryBuffer> Instantiation =
+        MemoryBuffer::getMemBufferCopy(VarIt->second.TextValue,
+                                       "<instantiation>");
+
+    // Jump to the macro instantiation and prime the lexer.
+    CurBuffer =
+        SrcMgr.AddNewSourceBuffer(std::move(Instantiation), Tok.getEndLoc());
+    Lexer.setBuffer(SrcMgr.getMemoryBuffer(CurBuffer)->getBuffer(), nullptr,
+                    /*EndStatementAtEOF=*/false);
+    EndStatementAtEOFStack.push_back(false);
+    Lexer.Lex();
+    return false;
+  }
+
+  const llvm::MCAsmMacro *M =
+      getContext().lookupMacro(Tok.getIdentifier().lower());
+  if (M && M->IsFunction && peekTok().is(AsmToken::LParen)) {
+    // This is a macro function invocation; expand it in place.
+    const SMLoc MacroLoc = Tok.getLoc();
+    const StringRef MacroId = Tok.getIdentifier();
+    Lexer.Lex();
+    if (handleMacroInvocation(M, MacroLoc)) {
+      Lexer.UnLex(AsmToken(AsmToken::Error, MacroId));
+      Lexer.Lex();
+    }
+    return false;
+  }
+
+  return true;
+}
+
+const AsmToken &MasmParser::Lex(ExpandKind ExpandNextToken) {
   if (Lexer.getTok().is(AsmToken::Error))
     Error(Lexer.getErrLoc(), Lexer.getErr());
 
   // if it's a end of statement with a comment in it
   if (getTok().is(AsmToken::EndOfStatement)) {
     // if this is a line comment output it.
     if (!getTok().getString().empty() && getTok().getString().front() != '\n' &&
         getTok().getString().front() != '\r' && MAI.preserveAsmComments())
       Out.addExplicitComment(Twine(getTok().getString()));
   }
 
   const AsmToken *tok = &Lexer.Lex();
   bool StartOfStatement = Lexer.isAtStartOfStatement();
 
-  while (tok->is(AsmToken::Identifier)) {
+  while (ExpandNextToken == ExpandMacros && tok->is(AsmToken::Identifier)) {
     if (StartOfStatement) {
       AsmToken NextTok;
-
       MutableArrayRef<AsmToken> Buf(NextTok);
       size_t ReadCount = Lexer.peekTokens(Buf);
       if (ReadCount && NextTok.is(AsmToken::Identifier) &&
           (NextTok.getString().equals_insensitive("equ") ||
            NextTok.getString().equals_insensitive("textequ"))) {
         // This looks like an EQU or TEXTEQU directive; don't expand the
         // identifier, allowing for redefinitions.
         break;
       }
     }
-    auto it = Variables.find(tok->getIdentifier().lower());
-    const llvm::MCAsmMacro *M =
-        getContext().lookupMacro(tok->getIdentifier().lower());
-    if (it != Variables.end() && it->second.IsText) {
-      // This is a textmacro; expand it in place.
-      std::unique_ptr<MemoryBuffer> Instantiation =
-          MemoryBuffer::getMemBufferCopy(it->second.TextValue,
-                                         "<instantiation>");
-
-      // Jump to the macro instantiation and prime the lexer.
-      CurBuffer = SrcMgr.AddNewSourceBuffer(std::move(Instantiation),
-                                            getTok().getEndLoc());
-      Lexer.setBuffer(SrcMgr.getMemoryBuffer(CurBuffer)->getBuffer(), nullptr,
-                      /*EndStatementAtEOF=*/false);
-      EndStatementAtEOFStack.push_back(false);
-      tok = &Lexer.Lex();
-    } else if (M && M->IsFunction && peekTok().is(AsmToken::LParen)) {
-      // This is a macro function invocation; expand it in place.
-      const AsmToken MacroTok = *tok;
-      tok = &Lexer.Lex();
-      if (handleMacroInvocation(M, MacroTok.getLoc())) {
-        Lexer.UnLex(AsmToken(AsmToken::Error, MacroTok.getIdentifier()));
-        tok = &Lexer.Lex();
-      }
-      continue;
-    } else {
+    if (expandMacros())
       break;
-    }
-  }
 
   // Parse comments here to be deferred until end of next statement.
   while (tok->is(AsmToken::Comment)) {
     if (MAI.preserveAsmComments())
       Out.addExplicitComment(Twine(tok->getString()));
     tok = &Lexer.Lex();
   }
 
if (Lexer.is(AsmToken::Integer)) {
     Lex();
     IDVal = "*";
   } else if (Lexer.is(AsmToken::Real)) {
     // Treat ".<number>" as a valid identifier in this context.
     IDVal = getTok().getString();
     Lex(); // always eat a token
     if (!IDVal.startswith("."))
       return Error(IDLoc, "unexpected token at start of statement");
-  } else if (Lexer.is(AsmToken::Identifier) &&
-             getTok().getString().equals_insensitive("echo")) {
-    // Intercept echo early to avoid lexical substitution in its message, and
-    // delegate all handling to the appropriate function.
-    return parseDirectiveEcho();
-  } else if (parseIdentifier(IDVal)) {
+  } else if (parseIdentifier(IDVal, StartOfStatement)) {
     if (!TheCondState.Ignore) {
       Lex(); // always eat a token
       return Error(IDLoc, "unexpected token at start of statement");
     }
     IDVal = "";
   }
 
   // Handle conditional assembly here before checking for skipping.  We
case DK_ERRIDNI:
       return parseDirectiveErrorIfidn(IDLoc, /*ExpectEqual=*/true,
                                       /*CaseInsensitive=*/true);
     case DK_ERRE:
       return parseDirectiveErrorIfe(IDLoc, true);
     case DK_ERRNZ:
       return parseDirectiveErrorIfe(IDLoc, false);
     case DK_RADIX:
       return parseDirectiveRadix(IDLoc);
+    case DK_ECHO:
+      return parseDirectiveEcho(IDLoc);
     }
 
     return Error(IDLoc, "unknown directive");
   }
 
   // We also check if this is allocating memory with user-defined type.
   auto IDIt = Structs.find(IDVal.lower());
   if (IDIt != Structs.end())
bool MasmParser::handleMacroInvocation(const MCAsmMacro *M, SMLoc NameLoc) {
   Lex();
 
   return false;
 }
 
 /// parseIdentifier:
 ///   ::= identifier
 ///   ::= string
-bool MasmParser::parseIdentifier(StringRef &Res) {
+bool MasmParser::parseIdentifier(StringRef &Res,
+                                 IdentifierPositionKind Position) {
   // The assembler has relaxed rules for accepting identifiers, in particular we
   // 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) || Lexer.is(AsmToken::At)) {
     SMLoc PrefixLoc = getLexer().getLoc();
 
if (Lexer.is(AsmToken::Dollar) || Lexer.is(AsmToken::At)) {
     return false;
   }
 
   if (Lexer.isNot(AsmToken::Identifier) && Lexer.isNot(AsmToken::String))
     return true;
 
   Res = getTok().getIdentifier();
 
-  Lex(); // Consume the identifier token.
+  // Consume the identifier token - but if parsing certain directives, avoid
+  // lexical expansion of the next token.
+  ExpandKind ExpandNextToken = ExpandMacros;
+  if (Position == StartOfStatement &&
+      StringSwitch<bool>(Res)
+          .CaseLower("echo", true)
+          .CasesLower("ifdef", "ifndef", "elseifdef", "elseifndef", true)
+          .Default(false)) {
+    ExpandNextToken = DoNotExpandMacros;
+  }
+  Lex(ExpandNextToken);
 
   return false;
 }
 
 /// parseDirectiveEquate:
 ///  ::= name "=" expression
 ///    | name "equ" expression    (not redefinable)
 ///    | name "equ" text-list
if (Radix < 2 || Radix > 16)
     return Error(Loc, "radix must be in the range 2 to 16; was " +
                           std::to_string(Radix));
   getLexer().setMasmDefaultRadix(Radix);
   return false;
 }
 
 /// parseDirectiveEcho
 ///   ::= "echo" message
-bool MasmParser::parseDirectiveEcho() {
-  // We're called before the directive is parsed, to avoid triggering lexical
-  // substitutions in the message. Assert that the next token is the directive,
-  // then eat it without using the Parser's Lex method.
-  assert(getTok().is(AsmToken::Identifier) &&
-         getTok().getString().equals_insensitive("echo"));
-  Lexer.Lex();
-
+bool MasmParser::parseDirectiveEcho(SMLoc DirectiveLoc) {
   std::string Message = parseStringTo(AsmToken::EndOfStatement);
   llvm::outs() << Message;
   if (!StringRef(Message).endswith("\n"))
     llvm::outs() << '\n';
   return false;
 }
 
 // We are comparing pointers, but the pointers are relative to a single string.

llvm/test/tools/llvm-ml/command_line_defines.asm

-; RUN: llvm-ml -filetype=s %s /Fo - /DT1=test1 /D T2=test2 /Dtest5=def | FileCheck %s
+; RUN: llvm-ml -filetype=s %s /Fo - /DT1=test1 /D T2=test2 /Dtest5=def /Dtest6 | FileCheck %s
 
 .code
 
 t1:
   ret
 ; CHECK-NOT: t1:
 ; CHECK-LABEL: test1:
 ; CHECK-NOT: t1:

 % t5_changed BYTE "&test5"
 ; CHECK-LABEL: t5_changed:
 ; CHECK-NEXT: .byte 114
 ; CHECK-NEXT: .byte 101
 ; CHECK-NEXT: .byte 100
 ; CHECK-NEXT: .byte 101
 ; CHECK-NEXT: .byte 102
 
+t6:
+ifdef test6
+  xor eax, eax
+endif
+  ret
+; CHECK-LABEL: t6:
+; CHECK: xor eax, eax
+; CHECK: ret
+
 end