[ms] [llvm-ml] Fix case-sensitivity for variables and textmacros

[epastor]

Summary

Make variables and text-macro references case-insensitive, to match ml.exe.

Also improve error handling for text-macro expansion.

[thakis]

llvm/lib/MC/MCParser/MasmParser.cpp

1549

This looks up a label, and if that fails, tries to look up a var with the same (lowered) name. Is that intentional? If so, could you mention that in the patch summary? As far as I understand, this change is unrelated to what's currently in the description.

Same with the Unlex changes below which I'm guessing are related to this change here.

I understand all the .lower() changes in the diff, I think I understand the test, but I don't get why the rest is changing yet.

[thakis]

(marking "request changes" just so it disappears form my dashboard and can reappear once you've answered -- doesn't necessarily need changes)

[epastor]

Improve comments to clarify the change

llvm/lib/MC/MCParser/MasmParser.cpp

1549

I've added some comments in a new version of the change. Quick summary: this location doesn't change how things are handled - we already tried looking up a Symbol, now we just canonicalize the casing if it happens to be a Variable name.

Also, the UnLex changes below fix error handling for textmacro expansion, and were required to get the test working cleanly. I've updated the patch summary to mention those.

[epastor]

Rebase on HEAD

[epastor]

Rebase to HEAD

llvm/lib/MC/MCParser/MasmParser.cpp

if (Split.second.empty()) {
       } else if (Structs.count(SymbolName.lower())) {
         // This is actually a reference to a field offset.
         Res = MCConstantExpr::create(Info.Offset, getContext());
         return false;
       }
     }
 
     MCSymbol *Sym = getContext().getInlineAsmLabel(SymbolName);
-    if (!Sym)
+    if (!Sym) {
+      // Variables use case-insensitive symbol names; if this is a variable, we
+      // find the symbol using its canonical name.
+      auto VarIt = Variables.find(SymbolName.lower());
+      if (VarIt != Variables.end())
+        SymbolName = VarIt->second.Name;
       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()) {
       auto V = Sym->getVariableValue(/*SetUsed*/ false);
       bool DoInline = isa<MCConstantExpr>(V) && !Variant;
       if (auto TV = dyn_cast<MCTargetExpr>(V))
         DoInline = TV->inlineAssignedExpr();

 
 /// parseDirectiveEquate:
 ///  ::= name "=" expression
 ///    | name "equ" expression    (not redefinable)
 ///    | name "equ" text-list
 ///    | name "textequ" text-list
 bool MasmParser::parseDirectiveEquate(StringRef IDVal, StringRef Name,
                                       DirectiveKind DirKind) {
-  Variable &Var = Variables[Name];
+  Variable &Var = Variables[Name.lower()];
   if (Var.Name.empty()) {
     Var.Name = Name;
   } else if (!Var.Redefinable) {
     return TokError("invalid variable redefinition");
   }
   Var.Redefinable = (DirKind != DK_EQU);
 
+  SMLoc StartLoc = Lexer.getLoc();
   if (DirKind == DK_EQU || DirKind == DK_TEXTEQU) {
     // "equ" and "textequ" both allow text expressions.
     std::string Value;
     if (!parseTextItem(Value)) {
       Var.IsText = true;
       Var.TextValue = Value;
 
       // Accept a text-list, not just one text-item.
if (!parseTextItem(Value)) {
       return false;
     }
   }
   if (DirKind == DK_TEXTEQU)
     return TokError("expected <text> in '" + Twine(IDVal) + "' directive");
 
   // Parse as expression assignment.
   const MCExpr *Expr;
-  SMLoc EndLoc, StartLoc = Lexer.getLoc();
+  SMLoc EndLoc;
   if (parseExpression(Expr, EndLoc))
     return addErrorSuffix(" in '" + Twine(IDVal) + "' directive");
   MCSymbol *Sym = getContext().getOrCreateSymbol(Var.Name);
   Sym->setRedefinable(Var.Redefinable);
   Sym->setVariableValue(Expr);
   Sym->setExternal(false);
 
   if (Expr->evaluateAsAbsolute(Var.NumericValue,
case AsmToken::Percent: {
     return false;
   }
   case AsmToken::Less:
   case AsmToken::LessEqual:
   case AsmToken::LessLess:
   case AsmToken::LessGreater:
     return parseAngleBracketString(Data);
   case AsmToken::Identifier: {
+    // This must be a text macro; we need to expand it accordingly.
     StringRef ID;
     if (parseIdentifier(ID))
       return true;
     Data = ID.str();
 
-    auto it = Variables.find(ID);
-    if (it == Variables.end())
+    auto it = Variables.find(ID.lower());
+    if (it == Variables.end()) {
+      // Not a variable; since we haven't used the token, put it back for better
+      // error recovery.
+      getLexer().UnLex(AsmToken(AsmToken::Identifier, ID));
       return true;
+    }
 
     while (it != Variables.end()) {
       const Variable &Var = it->second;
-      if (!Var.IsText)
+      if (!Var.IsText) {
+        // Not a text macro; not usable in TextItem context. Since we haven't
+        // used the token, put it back for better error recovery.
+        getLexer().UnLex(AsmToken(AsmToken::Identifier, ID));
         return true;
+      }
       Data = Var.TextValue;
-      it = Variables.find(Data);
+      it = Variables.find(StringRef(Data).lower());
     }
     return false;
   }
   }
   llvm_unreachable("unhandled token kind");
 }
 
 /// parseDirectiveAscii:
if (TheCondState.Ignore) {
     is_defined = (getTargetParser().tryParseRegister(
                       RegNo, StartLoc, EndLoc) == MatchOperand_Success);
     if (!is_defined) {
       StringRef Name;
       if (check(parseIdentifier(Name), "expected identifier after 'ifdef'") ||
           parseToken(AsmToken::EndOfStatement, "unexpected token in 'ifdef'"))
         return true;
 
-      if (Variables.find(Name) != Variables.end()) {
+      if (Variables.find(Name.lower()) != Variables.end()) {
         is_defined = true;
       } else {
-        MCSymbol *Sym = getContext().lookupSymbol(Name);
+        MCSymbol *Sym = getContext().lookupSymbol(Name.lower());
         is_defined = (Sym && !Sym->isUndefined(false));
       }
     }
 
     TheCondState.CondMet = (is_defined == expect_defined);
     TheCondState.Ignore = !TheCondState.CondMet;
   }
 
if (LastIgnoreState || TheCondState.CondMet) {
     if (!is_defined) {
       StringRef Name;
       if (check(parseIdentifier(Name),
                 "expected identifier after 'elseifdef'") ||
           parseToken(AsmToken::EndOfStatement,
                      "unexpected token in 'elseifdef'"))
         return true;
 
-      if (Variables.find(Name) != Variables.end()) {
+      if (Variables.find(Name.lower()) != Variables.end()) {
         is_defined = true;
       } else {
         MCSymbol *Sym = getContext().lookupSymbol(Name);
         is_defined = (Sym && !Sym->isUndefined(false));
       }
     }
 
     TheCondState.CondMet = (is_defined == expect_defined);
bool MasmParser::parseDirectiveErrorIfdef(SMLoc DirectiveLoc,
   SMLoc StartLoc, EndLoc;
   IsDefined = (getTargetParser().tryParseRegister(RegNo, StartLoc, EndLoc) ==
                MatchOperand_Success);
   if (!IsDefined) {
     StringRef Name;
     if (check(parseIdentifier(Name), "expected identifier after '.errdef'"))
       return true;
 
-    if (Variables.find(Name) != Variables.end()) {
+    if (Variables.find(Name.lower()) != Variables.end()) {
       IsDefined = true;
     } else {
       MCSymbol *Sym = getContext().lookupSymbol(Name);
       IsDefined = (Sym && !Sym->isUndefined(false));
     }
   }
 
   std::string Message = ".errdef directive invoked in source file";

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

 # RUN: llvm-ml -filetype=s %s /Fo - | FileCheck %s
 
 .data
 t1_value equ 1 or 2
 
-t1 BYTE t1_value DUP (0)
-; CHECK: t1:
+t1 BYTE t1_VALUE DUP (0)
+; CHECK-LABEL: t1:
 ; CHECK-NEXT: .byte 0
 ; CHECK-NEXT: .byte 0
 ; CHECK-NEXT: .byte 0
 ; CHECK-NOT: .byte 0
 
+t2_value equ 4 or t1_value
+t2 BYTE t2_VALUE
+; CHECK-LABEL: t2:
+; CHECK-NEXT: .byte 7
+
+t3_value equ t1_VALUE or 8
+t3 BYTE t3_VALUE
+; CHECK-LABEL: t3:
+; CHECK-NEXT: .byte 11
+
 END