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

[Vedant Kumar via llvm-commits]

Comments inline --

> With the second patch I get these failures:
> 
>    LLVM :: MC/ARM/thumb_set-diagnostics.s
>    LLVM :: MC/AsmParser/variables-invalid.s

As mentioned in my last email, I think these tests are suspicious.

Sorry, I should have been clearer about this. I left them in so it would be easier to see the failure and discuss how to write better tests for this.


> Having said that, if I replace your second patch with the attached one
> I get what I would expect is a no functionally cleanup.

I don't think we can have a NFC patch to AsmParser.cpp. The current behavior ("error: invalid reassignment to non-absolute variable") is caused by a bug. At least one person has run into it and switched to GAS [1].


> Unfortunately we then assert on things like
> 
> .set cc,1
> .if cc
> nop
> .endif
> .set cc,0

I explained why changing the assert in getVariableValue() is necessary in my last email. It lets us assign to used variables without clearing their IsUsed bit.


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

Hm, maybe. But it's not just ".if" blocks that cause the issue. Just ".set" has the same issue [1].

Could someone clarify what exactly "invalid reassignment to non-absolute variable" means? It's starting to look like this error should never pop up.

thanks
vedant

[1] http://stackoverflow.com/questions/26319034/reassigning-non-absolute-variables-in-osxs-assembler

> 
> 
> 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
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>> 
>> 
> <t.diff>