[PATCH] Add default parameter to MCSymbol accessors to control IsUsed

[Rafael Espíndola via llvm-commits]

With the second patch I get these failures:

    LLVM :: MC/ARM/thumb_set-diagnostics.s
    LLVM :: MC/AsmParser/variables-invalid.s

Please update tests as necessary.

Having said that, if I replace your second patch with the attached one
I get what I would expect is a no functionally cleanup. Unfortunately
we then assert on things like

.set cc,1
.if cc
nop
.endif
.set cc,0

Maybe we should just not mark cc used in the .if processing?


On 10 August 2015 at 12:41, Vedant Kumar <vsk at apple.com> wrote:
>> +    IsUsed = SetUsed;
>>
>> Should this be
>>
>> IsUsed |= SetUsed;
>
> Yes. I've fixed that in patch 1 now (attached).
>
>
>> Can you leave the old tests to show that now we don't error on them?
>
> Ok, I'll leave them in.
>
> The second patch needed additional adjustments.
>
> Previously, we only allowed re-definitions of unused variables:
>
>>> -    else if (Sym->isVariable() && !Sym->isUsed() && allow_redef)
>>>        ; // Allow redefinitions of variables that haven't yet been used.
>
> This happened because of an assertion in MCSymbol::setVariableValue(V). It forbade assignments to used symbols. To get around this, we used a suspicious trick in parseAssignmentExpression():
>
>>> -
>>> -    // Don't count these checks as uses.
>>> -    Sym->setUsed(false);
>
>
> This is bad because the checks in parseAssignmentExpression() no longer create uses. Unconditionally clearing the Used bit seems wrong. Since this looked hacky, I removed it. I also fixed the assertion in MCSymbol::setVariableValue() to allow assignments to used variables.
>
> The only two tests we error on are:
>
>     - /test/MC/ARM/thumb_set-diagnostics.s
>     - test/MC/AsmParser/variables-invalid.s
>
> This is because we no longer see "invalid reassignment to non-absolute variable". I still need help figuring out what this means, and how to write a proper negative test case for it :).
>
> Here's a convenient way to run all the check-llvm-mc* tests:
>
>>> $ ninja $(grep "build check-llvm-mc" build.ninja | cut -d' ' -f2 | sed 's/://g' | xargs echo)
>
>
> I've attached the patch 1 to MCSymbol.h (NFC) and patch 2 which fixes issues in MC/AsmParser.
>
> thanks
> vedant
>
>
>
>
>> On 6 August 2015 at 18:14, Vedant Kumar <vsk at apple.com> wrote:
>>> Hi,
>>>
>>> Currently, some accessors in MCSymbol set IsUsed when they shouldn't be. This can lead to surprising bugs.
>>>
>>> The first patch I've attached adds a default parameter, "SetUsed = true", to some MCSymbol accessors. This preserves the current behavior (NFC) without impacting existing code. It also buys us more flexibility when we don't want to set IsUsed.
>>>
>>> The second patch touches MC/AsmParser to fix an issue where we get "invalid reassignment of non-absolute variable" when the error doesn't apply.
>>>
>>> The second patch deserves closer scrutiny, because, try as I might, I couldn't craft a test case which gets this error to trigger. I need some help coming up for a test case for it. It could be the case that the error should never trigger -- if so, we can get rid of it.
>>>
>>> ninja check-all reports no issues.
>>>
>>> I don't have commit rights. I'd appreciate someone reviewing all of this and (hopefully) guiding it into trunk.
>>>
>>> thanks!
>>> vedant
>>>
>>>
>>>
>>>
>>>
>
>
-------------- next part --------------
diff --git a/lib/MC/MCParser/AsmParser.cpp b/lib/MC/MCParser/AsmParser.cpp
index 3f45b3d..26b952f 100644
--- a/lib/MC/MCParser/AsmParser.cpp
+++ b/lib/MC/MCParser/AsmParser.cpp
@@ -4805,21 +4805,18 @@ 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(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))
+    else if (!Sym->isUndefined(false) && (!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()))
+    else if (!isa<MCConstantExpr>(Sym->getVariableValue(false)))
       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");