[MC/AsmParser] Avoid setting MCSymbol.IsUsed

[vsk]

Summary

Avoid marking MCSymbols as used in MC/AsmParser.cpp when no uses exist. This fixes a bug in parseAssignmentExpression() where we accidentally set IsUsed, thereby triggering:

"invalid re-assignment of non-absolute variable"

on otherwise valid code. No other functionality change is intended.

[compnerd]

The various false parameters slightly muddle the code IMO, since they are for value accessors.

test/MC/AsmParser/reassign.s

1 ↗

(On Diff #33154)

Can you add a CHECK, or in your case, a CHECK-NOT here please?

[vsk]

In D12347#232844, @compnerd wrote:

The various false parameters slightly muddle the code IMO, since they are for value accessors.

We could save the IsUsed state and restore it before the MCSymbol leaves scope. E.g;

bool IsUsed = Sym->isUsed();
if (expect_defined)
  TheCondState.CondMet = (Sym && !Sym->isUndefined());
else
  TheCondState.CondMet = (!Sym || Sym->isUndefined());
Sym->setUsed(IsUsed);

Alternatively we could make a separate set of read-only accessors: isUndefinedRO, getVariableValueRO, ... (with a better name).

Is there an approach you'd prefer?

[• rafael]

What we normally do is:

foo(/*SetUsed*/ false);

[vsk]

• Added /*SetUsed*/ annotations
• Added a CHECK-NOT line, made sure we don't pass the test on master

[compnerd]

It is unfortunate that these accessors mutate the symbol. Personally, I think I prefer the alternate approach of a RAII helper that would restore the state. However, if you are planning some follow up work and this blocks forward progress, then committing this as is to make forward progress would be acceptable (thanks to the annotations).

Thanks for fixing up the test.

[vsk]

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!

[compnerd]

The follow up approach is definitely better. Lets go with that.

[• rafael]

• t.diff2 KBDownload

llvm/trunk/include/llvm/MC/MCSymbol.h

public:
   /// \name Accessors
   /// @{
 
   /// isTemporary - Check if this is an assembler temporary symbol.
   bool isTemporary() const { return IsTemporary; }
 
   /// isUsed - Check if this is used.
   bool isUsed() const { return IsUsed; }
-  void setUsed(bool Value) const { IsUsed = Value; }
+  void setUsed(bool Value) const { IsUsed |= Value; }
 
   /// \brief Check if this symbol is redefinable.
   bool isRedefinable() const { return IsRedefinable; }
   /// \brief Mark this symbol as redefinable.
   void setRedefinable(bool Value) { IsRedefinable = Value; }
   /// \brief Prepare this symbol to be redefined.
   void redefineIfPossible() {
     if (IsRedefinable) {

llvm/trunk/lib/MC/MCParser/AsmParser.cpp

for (MCContext::SymbolTable::const_iterator i = Symbols.begin(),
       MCSymbol *Sym = i->getValue();
       // Variable symbols may not be marked as defined, so check those
       // explicitly. If we know it's a variable, we have a definition for
       // the purposes of this check.
       if (Sym->isTemporary() && !Sym->isVariable() && !Sym->isDefined())
         // FIXME: We would really like to refer back to where the symbol was
         // first referenced for a source location. We need to add something
         // to track that. Currently, we just point to the end of the file.
-        printMessage(
-            getLexer().getLoc(), SourceMgr::DK_Error,
-            "assembler local symbol '" + Sym->getName() + "' not defined");
+        printMessage(getLexer().getLoc(), SourceMgr::DK_Error,
+                     "assembler local symbol '" + Sym->getName() +
+                         "' not defined");
     }
   }
 
   // Finalize the output stream if there are no errors and if the client wants
   // us to.
   if (!HadError && !NoFinalize)
     Out.Finish();
 
if (Split.second.size()) {
                      "invalid variant '" + Split.second + "'");
       }
     }
 
     MCSymbol *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() && 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;
     }
 
     // Otherwise create a symbol ref.
     Res = MCSymbolRefExpr::create(Sym, Variant, getContext());
     return false;
   }
   case AsmToken::BigNum:
bool parseAssignmentExpression(StringRef Name, bool allow_redef,
   Sym = Parser.getContext().lookupSymbol(Name);
   if (Sym) {
     // Diagnose assignment to a label.
     //
     // FIXME: Diagnostics. Note the location of the definition as a label.
     // 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.
     else if (!Sym->isUndefined() && (!Sym->isVariable() || !allow_redef))
       return Parser.Error(EqualLoc, "redefinition of '" + Name + "'");
     else if (!Sym->isVariable())
       return Parser.Error(EqualLoc, "invalid assignment to '" + Name + "'");
     else if (!isa<MCConstantExpr>(Sym->getVariableValue()))
       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");
       Parser.eatToEndOfStatement();
       return true;
     }
     return false;
   } else

llvm/trunk/test/MC/AsmParser/reassign.s

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