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

[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]

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

2

I couldn't get lit to just detect the warning, for some reason. I'm open to suggestions!

[thakis]

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

1

Maybe you need 2>&1 in front of the | so that lit sees stderr?

1

Wait you meant the other test. One moment…

[thakis]

I tried patching this in

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

2

Hm, works for me:

; 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

% out/gn/bin/llvm-lit -s -vv llvm/test/tools/llvm-ml/command_line_defines_errors.asm


Testing Time: 0.13s
  Passed: 1

[epastor]

Directly check for the warning, rather than converting it to an error

[epastor]

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

2

... darn, I would've sworn I tried that, but apparently I kept putting it in the other file - forgetting that CHECK always requires lines to occur in order, and that the warning is emitted before any of the output lines I'm checking against.

Thanks!

[thakis]

LG with tweak below

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

2

You don't need the implicit check not for error: if there's an error the command will exit non-0 and lit will fail the test

[epastor]

Dropping unnecessary implicit-check-not guard

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:
     if (afterNextTok.is(AsmToken::Identifier) &&
         afterNextTok.getString().equals_lower("ptr")) {
       // Size directive; part of an instruction.
       break;
     }
     LLVM_FALLTHROUGH;
   case DK_SBYTE:

 }
 
 /// parseDirectiveEquate:
 ///  ::= name "=" expression
 ///    | name "equ" expression    (not redefinable)
 ///    | 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;
   }
 
   SMLoc StartLoc = Lexer.getLoc();
   if (DirKind == DK_EQU || DirKind == DK_TEXTEQU) {
     // "equ" and "textequ" both allow text expressions.
     std::string Value;
     std::string TextItem;
     if (!parseTextItem(TextItem)) {
       Value += TextItem;
 
       // Accept a text-list, not just one text-item.
       auto parseItem = [&]() -> bool {
         if (parseTextItem(TextItem))
           return TokError("expected text item");
         Value += TextItem;
         return false;
       };
       if (parseOptionalToken(AsmToken::Comma) && parseMany(parseItem))
         return addErrorSuffix(" in '" + Twine(IDVal) + "' directive");
 
-      if (!Var.Redefinable && (!Var.IsText || Var.TextValue != Value))
+      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;
     }
   }
   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");
 
   int64_t Value;
   if (!Expr->evaluateAsAbsolute(Value, getStreamer().getAssemblerPtr())) {
     // 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))
+    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))
+    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);
 
   return false;
 }
 
 bool MasmParser::parseEscapedString(std::string &Data) {
   if (check(getTok().isNot(AsmToken::String), "expected string"))
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_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

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

+; 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