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

[Vedant Kumar via llvm-commits]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: MCSymbol-patch1-NFC.patch
Type: application/octet-stream
Size: 2856 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150810/07a6fae0/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: MCSymbol-patch2.patch
Type: application/octet-stream
Size: 2351 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150810/07a6fae0/attachment-0001.obj>
-------------- next part --------------


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