[ms] [llvm-ml] Warn on command-line redefinition

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.

Reviewed By: thakis

Differential Revision: https://reviews.llvm.org/D104008

diff --git a/llvm/lib/MC/MCParser/MasmParser.cpp b/llvm/lib/MC/MCParser/MasmParser.cpp
index 9d5f33e..77d61cd 100644 (file)
--- a/llvm/lib/MC/MCParser/MasmParser.cpp
+++ b/llvm/lib/MC/MCParser/MasmParser.cpp
@@ -383,8 +383,10 @@ private:
 
   /// 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;
@@ -867,7 +869,7 @@ private:
 
   // "=", "equ", "textequ"
   bool parseDirectiveEquate(StringRef IDVal, StringRef Name,
-                            DirectiveKind DirKind);
+                            DirectiveKind DirKind, SMLoc NameLoc);
 
   bool parseDirectiveOrg(); // ".org"
   bool parseDirectiveAlign();  // "align"
@@ -2522,7 +2524,7 @@ bool MasmParser::parseStatement(ParseStatementInfo &Info,
   case DK_EQU:
   case DK_TEXTEQU:
     Lex();
-    return parseDirectiveEquate(nextVal, IDVal, DirKind);
+    return parseDirectiveEquate(nextVal, IDVal, DirKind, IDLoc);
   case DK_BYTE:
     if (afterNextTok.is(AsmToken::Identifier) &&
         afterNextTok.getString().equals_lower("ptr")) {
@@ -3343,7 +3345,7 @@ bool MasmParser::parseIdentifier(StringRef &Res) {
 ///    | name "equ" text-list
 ///    | name "textequ" text-list (redefinability unspecified)
 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;
@@ -3367,11 +3369,23 @@ bool MasmParser::parseDirectiveEquate(StringRef IDVal, StringRef Name,
       if (parseOptionalToken(AsmToken::Comma) && parseMany(parseItem))
         return addErrorSuffix(" in '" + Twine(IDVal) + "' directive");
 
-      if (!Var.Redefinable && (!Var.IsText || Var.TextValue != Value))
-        return Error(getTok().getLoc(), "invalid variable redefinition");
+      if (!Var.IsText || Var.TextValue != Value) {
+        switch (Var.Redefinable) {
+        case Variable::NOT_REDEFINABLE:
+          return Error(getTok().getLoc(), "invalid variable redefinition");
+        case Variable::WARN_ON_REDEFINITION:
+          if (Warning(NameLoc, "redefining '" + Name +
+                                   "', already defined on the command line")) {
+            return true;
+          }
+          break;
+        default:
+          break;
+        }
+      }
       Var.IsText = true;
       Var.TextValue = Value;
-      Var.Redefinable = true;
+      Var.Redefinable = Variable::REDEFINABLE;
 
       return false;
     }
@@ -3390,19 +3404,44 @@ bool MasmParser::parseDirectiveEquate(StringRef IDVal, StringRef Name,
     // Not an absolute expression; define as a text replacement.
     StringRef ExprAsString = StringRef(
         StartLoc.getPointer(), EndLoc.getPointer() - StartLoc.getPointer());
-    if (!Var.Redefinable && (!Var.IsText && Var.TextValue != ExprAsString))
-      return Error(getTok().getLoc(), "invalid variable redefinition");
+    if (!Var.IsText || Var.TextValue != ExprAsString) {
+      switch (Var.Redefinable) {
+      case Variable::NOT_REDEFINABLE:
+        return Error(getTok().getLoc(), "invalid variable redefinition");
+      case Variable::WARN_ON_REDEFINITION:
+        if (Warning(NameLoc, "redefining '" + Name +
+                                 "', already defined on the command line")) {
+          return true;
+        }
+        break;
+      default:
+        break;
+      }
+    }
     Var.IsText = true;
     Var.TextValue = ExprAsString.str();
   } else {
-    if (!Var.Redefinable && (Var.IsText || Var.NumericValue != Value))
-      return Error(getTok().getLoc(), "invalid variable redefinition");
+    if (Var.IsText || Var.NumericValue != Value) {
+      switch (Var.Redefinable) {
+      case Variable::NOT_REDEFINABLE:
+        return Error(getTok().getLoc(), "invalid variable redefinition");
+      case Variable::WARN_ON_REDEFINITION:
+        if (Warning(NameLoc, "redefining '" + Name +
+                                 "', already defined on the command line")) {
+          return true;
+        }
+        break;
+      default:
+        break;
+      }
+    }
     Var.NumericValue = Value;
   }
-  Var.Redefinable = (DirKind == DK_ASSIGN);
+  Var.Redefinable = (DirKind == DK_ASSIGN) ? Variable::REDEFINABLE
+                                           : Variable::NOT_REDEFINABLE;
 
   MCSymbol *Sym = getContext().getOrCreateSymbol(Var.Name);
-  Sym->setRedefinable(Var.Redefinable);
+  Sym->setRedefinable(Var.Redefinable != Variable::NOT_REDEFINABLE);
   Sym->setVariableValue(Expr);
   Sym->setExternal(false);
 
@@ -7008,10 +7047,14 @@ 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;

diff --git a/llvm/test/tools/llvm-ml/command_line_defines.asm b/llvm/test/tools/llvm-ml/command_line_defines.asm
index 51b02e6..cce8388 100644 (file)
--- a/llvm/test/tools/llvm-ml/command_line_defines.asm
+++ b/llvm/test/tools/llvm-ml/command_line_defines.asm
@@ -1,4 +1,4 @@
-; 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
 
@@ -35,4 +35,20 @@ endif
 ; CHECK: xor ebx, ebx
 ; CHECK: ret
 
+% t5_original BYTE "&test5"
+; CHECK-LABEL: t5_original:
+; CHECK-NEXT: .byte 100
+; CHECK-NEXT: .byte 101
+; CHECK-NEXT: .byte 102
+
+test5 textequ <redef>
+
+% 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
+
 end

diff --git a/llvm/test/tools/llvm-ml/command_line_defines_errors.asm b/llvm/test/tools/llvm-ml/command_line_defines_errors.asm
new file mode 100644 (file)
index 0000000..a671e18
--- /dev/null
+++ b/llvm/test/tools/llvm-ml/command_line_defines_errors.asm
@@ -0,0 +1,8 @@
+; RUN: llvm-ml -filetype=s %s /Fo - /Dtest1=def 2>&1 | FileCheck %s --implicit-check-not=warning:
+
+.code
+
+; CHECK: :[[# @LINE + 1]]:1: warning: redefining 'test1', already defined on the command line
+test1 textequ <redef>
+
+end