[PATCH] D12347: [MC/AsmParser] Avoid setting MCSymbol.IsUsed

[Rafael Espíndola via llvm-commits]

On 27 August 2015 at 13:24, Vedant Kumar <vsk at apple.com> wrote:
> vsk added a comment.
>
> I tried the RAII helper object approach. It improves readability in some areas, but IMHO also leads to brittle code.
>
> For one, some spots remain more readable with the `/*SetUsed*/` annotations. E.g, we would need to put the first conditional in its own scope to use the helper object here:
>
>   else if (Sym->isUndefined(/*SetUsed*/ false) && !Sym->isUsed() && !Sym->isVariable()) { ... }
>   else if (Sym->isVariable() && !Sym->isUsed() && allow_redef) { ... }
>
> Secondly, you need to remember to create new helper objects every time a mutating accessor is invoked so that you don't accidentally grab an incorrect IsUsed state.
>
> I'd prefer to stick with this revision. As a followup, I can work on (1) identifying the call sites which //intend// to mutate IsUsed, (2) forcing those call sites to be explicit with Sym->setUsed(), and (3) making the MCSymbol accessors read-only w.r.t IsUsed.
>
> Let me know what you think!

While MC interprets redefinitions differently from gas, having IsUsed
set automatically is an important safety/sanity feature. With the
SetUsed option we can individually allow cases that are known to be
safe. So my suggestions are

* For this patch, keep the use a minimum (see attached).
* Drop support for clearing the used bit with setUsed. I.E.: setUsed
always sets IsUsed to true.
* Pass SetUsed as false in more cases, but each with a test showing what we do.
-------------- next part --------------
diff --git a/lib/MC/MCParser/AsmParser.cpp b/lib/MC/MCParser/AsmParser.cpp
index 3f45b3d..8fccbb5 100644
--- a/lib/MC/MCParser/AsmParser.cpp
+++ b/lib/MC/MCParser/AsmParser.cpp
@@ -867,11 +867,12 @@ bool AsmParser::parsePrimaryExpr(const MCExpr *&Res, SMLoc &EndLoc) {
 
     // If this is an absolute variable reference, substitute it now to preserve
     // semantics in the face of reassignment.
-    if (Sym->isVariable() && isa<MCConstantExpr>(Sym->getVariableValue())) {
+    if (Sym->isVariable() &&
+        isa<MCConstantExpr>(Sym->getVariableValue(/*SetUsed*/ false))) {
       if (Variant)
         return Error(EndLoc, "unexpected modifier on variable reference");
 
-      Res = Sym->getVariableValue();
+      Res = Sym->getVariableValue(/*SetUsed*/ false);
       return false;
     }
 
@@ -4805,7 +4806,8 @@ bool parseAssignmentExpression(StringRef Name, bool allow_redef,
     // FIXME: Diagnose assignment to protected identifier (e.g., register name).
     if (isSymbolUsedInExpression(Sym, Value))
       return Parser.Error(EqualLoc, "Recursive use of '" + Name + "'");
-    else if (Sym->isUndefined() && !Sym->isUsed() && !Sym->isVariable())
+    else if (Sym->isUndefined(/*SetUsed*/ false) && !Sym->isUsed() &&
+             !Sym->isVariable())
       ; // Allow redefinitions of undefined symbols only used in directives.
     else if (Sym->isVariable() && !Sym->isUsed() && allow_redef)
       ; // Allow redefinitions of variables that haven't yet been used.
@@ -4817,9 +4819,6 @@ bool parseAssignmentExpression(StringRef Name, bool allow_redef,
       return Parser.Error(EqualLoc,
                           "invalid reassignment of non-absolute variable '" +
                               Name + "'");
-
-    // Don't count these checks as uses.
-    Sym->setUsed(false);
   } else if (Name == ".") {
     if (Parser.getStreamer().EmitValueToOffset(Value, 0)) {
       Parser.Error(EqualLoc, "expected absolute expression");
diff --git a/test/MC/AsmParser/reassign.s b/test/MC/AsmParser/reassign.s
new file mode 100644
index 0000000..817836b
--- /dev/null
+++ b/test/MC/AsmParser/reassign.s
@@ -0,0 +1,12 @@
+// RUN: llvm-mc -triple i386-unknown-unknown %s | FileCheck %s
+
+	.text
+bar:
+
+	.data
+.globl foo
+.set   foo, bar
+.globl foo
+.set   foo, bar
+
+// CHECK-NOT: invalid reassignment of non-absolute variable