[ms] [llvm-ml] Warn on redefinition of a command-line definition

[epastor]

Summary

If a macro is defined on the command line and then overridden in the source code, this is likely to be an error in the user's build system. We should warn on this.

[epastor]

The test here doesn't currently warn - because test5 is already defined, and as such the line under t5: is lexed as:

def textequ <redef>

I need to check how ML.EXE handles this!

[epastor]

Rebase on parent

[epastor]

Rebase on parent

[thakis]

(this didn't have me as a reviewer, so I only saw it by coincidence)

We should also add the /WX switch :)

If I read it right, this isn't ready to go yet, right?

[epastor]

In D92505#2694696, @thakis wrote:

(this didn't have me as a reviewer, so I only saw it by coincidence)

We should also add the /WX switch :)

That was added in https://reviews.llvm.org/D92504.

If I read it right, this isn't ready to go yet, right?

Correct; I need to investigate more corner cases in ML.EXE. It's unclear how ML.EXE handles redefinitions... when I read the spec as suggesting that (e.g.) test5 should be replaced by def before the line is even parsed!

[epastor]

This required a few too many changes; I'll re-upload a working variant on top of a prerequisite (D103993).

llvm/lib/MC/MCParser/MasmParser.cpp

private:
 
   /// maps directive names to handler methods in parser
   /// extensions. Extensions register themselves in this map by calling
   /// addDirectiveHandler.
   StringMap<ExtensionDirectiveHandler> ExtensionDirectiveMap;
 
   /// maps assembly-time variable names to variables.
   struct Variable {
+    enum RedefinableKind { NOT_REDEFINABLE, WARN_ON_REDEFINITION, REDEFINABLE };
+
     StringRef Name;
-    bool Redefinable = true;
+    RedefinableKind Redefinable = REDEFINABLE;
     bool IsText = false;
     int64_t NumericValue = 0;
     std::string TextValue;
   };
   StringMap<Variable> Variables;
 
   /// Stack of active struct definitions.
   SmallVector<StructInfo, 1> StructInProgress;
private:
   bool parseDirectiveStructValue(const StructInfo &Structure,
                                  StringRef Directive, SMLoc DirLoc);
   bool parseDirectiveNamedStructValue(const StructInfo &Structure,
                                       StringRef Directive, SMLoc DirLoc,
                                       StringRef Name);
 
   // "=", "equ", "textequ"
   bool parseDirectiveEquate(StringRef IDVal, StringRef Name,
-                            DirectiveKind DirKind);
+                            DirectiveKind DirKind, SMLoc NameLoc);
 
   bool parseDirectiveOrg(); // ".org"
   bool parseDirectiveAlign();  // "align"
 
   // ".file", ".line", ".loc", ".stabs"
   bool parseDirectiveFile(SMLoc DirectiveLoc);
   bool parseDirectiveLine();
   bool parseDirectiveLoc();
DirKind = (DirKindIt == DirectiveKindMap.end())
                 : DirKindIt->getValue();
   switch (DirKind) {
   default:
     break;
   case DK_ASSIGN:
   case DK_EQU:
   case DK_TEXTEQU:
     Lex();
-    return parseDirectiveEquate(nextVal, IDVal, DirKind);
+    return parseDirectiveEquate(nextVal, IDVal, DirKind, IDLoc);
   case DK_BYTE:
   case DK_SBYTE:
   case DK_DB:
     Lex();
     return parseDirectiveNamedValue(nextVal, 1, IDVal, IDLoc);
   case DK_WORD:
   case DK_SWORD:
   case DK_DW:

 }
 
 /// 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) {
+                                      DirectiveKind DirKind, SMLoc NameLoc) {
   Variable &Var = Variables[Name.lower()];
   if (Var.Name.empty()) {
     Var.Name = Name;
-  } else if (!Var.Redefinable) {
+  } else if (Var.Redefinable == Variable::NOT_REDEFINABLE) {
     return TokError("invalid variable redefinition");
+  } else if (Var.Redefinable == Variable::WARN_ON_REDEFINITION &&
+             Warning(NameLoc, "redefining '" + Name +
+                                  "', already defined on the command line")) {
+    return true;
   }
-  Var.Redefinable = (DirKind != DK_EQU);
+
+  if (DirKind == DK_EQU)
+    Var.Redefinable = Variable::NOT_REDEFINABLE;
 
   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;
if (DirKind == DK_TEXTEQU)
     return TokError("expected <text> in '" + Twine(IDVal) + "' directive");
 
   // Parse as expression assignment.
   const MCExpr *Expr;
   SMLoc EndLoc;
   if (parseExpression(Expr, EndLoc))
     return addErrorSuffix(" in '" + Twine(IDVal) + "' directive");
   MCSymbol *Sym = getContext().getOrCreateSymbol(Var.Name);
-  Sym->setRedefinable(Var.Redefinable);
+  Sym->setRedefinable(Var.Redefinable != Variable::NOT_REDEFINABLE);
   Sym->setVariableValue(Expr);
   Sym->setExternal(false);
 
   if (Expr->evaluateAsAbsolute(Var.NumericValue,
                                getStreamer().getAssemblerPtr()))
     return false;
 
   // Not an absolute expression; define as a text replacement.
if (AsmRewritePrecedence[AsmRewriteA->Kind] <
     return 1;
   llvm_unreachable("Unstable rewrite sort.");
 }
 
 bool MasmParser::defineMacro(StringRef Name, StringRef Value) {
   Variable &Var = Variables[Name.lower()];
   if (Var.Name.empty()) {
     Var.Name = Name;
-  } else if (!Var.Redefinable) {
-    return TokError("invalid variable redefinition");
+  } else if (Var.Redefinable == Variable::NOT_REDEFINABLE) {
+    return Error(SMLoc(), "invalid variable redefinition");
+  } else if (Var.Redefinable == Variable::WARN_ON_REDEFINITION &&
+             Warning(SMLoc(), "redefining '" + Name +
+                                  "', already defined on the command line")) {
+    return true;
   }
-  Var.Redefinable = true;
+  Var.Redefinable = Variable::WARN_ON_REDEFINITION;
   Var.IsText = true;
   Var.TextValue = Value.str();
   return false;
 }
 
 bool MasmParser::lookUpField(StringRef Name, AsmFieldInfo &Info) const {
   const std::pair<StringRef, StringRef> BaseMember = Name.split('.');
   const StringRef Base = BaseMember.first, Member = BaseMember.second;

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

-; RUN: llvm-ml -filetype=s %s /Fo - /DT1=test1 /D T2=test2 | FileCheck %s
+; RUN: llvm-ml -filetype=s %s /Fo - /DT1=test1 /D T2=test2 /Dtest5=def | FileCheck %s
 
 .code
 
 t1:
   ret
 ; CHECK-NOT: t1:
 ; CHECK-LABEL: test1:
 ; CHECK-NOT: t1:
elseifdef t2
   xor ebx, ebx
 endif
   ret
 ; CHECK-LABEL: t4:
 ; CHECK-NOT: xor eax, eax
 ; CHECK: xor ebx, ebx
 ; CHECK: ret
 
+t5:
+test5 textequ <redef>
+
 end