[PATCH] D88625: MCExpr::evaluateAsRelocatableImpl : allow evaluation of non-VK_None MCSymbolRefExpr when MCAsmLayout is available

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Changed to patch MCExpr::evaluateAsRelocatableImpl instead

In D88625#2369263 <https://reviews.llvm.org/D88625#2369263>, @jyknight wrote:

> In D88625#2328351 <https://reviews.llvm.org/D88625#2328351>, @MaskRay wrote:
>
>> In D88625#2328335 <https://reviews.llvm.org/D88625#2328335>, @jyknight wrote:
>>
>>>> I have tried adding the expression folding stuff to`MCExpr::evaluateAsRelocatableImpl` but I abandoned that idea because evaluateAsRelocatableImpl does not like a modifier different from VK_None (@PLT). (What does a at plt + 1 do? Relaxing the condition (== VK_None) may work... but the semantics are not very clear.
>>>
>>> `a at plt + 1` is perfectly fine -- that turns into a R_X86_64_PLT32 relocation with symbol "a" and addend 1.
>>>
>>> The interesting question is what this should do:
>>>
>>>   b=a+1 # no @plt!
>>>   ... b at plt
>>>
>>> It's certainly impossible to create a PLT referring to a function at address "a+1", in current object formats. So, probably this should be invalid. However, GAS seems to consider the modifier as more of an overall-modifier, resulting in the same thing as `a at plt + 1`. I agree that's probably not something we ought to support, since it seems wrong.
>>>
>>> But, it does still seem reasonable to put the !VK_None support into evaluateAsRelocatableImpl -- as long as the resolved MCValue has only a single symbol, no offset and no kind.
>>
>> The current way the patch implements the "if a fixup symbol is equated to an expression with an undefined symbol" logic is consistent with the step GNU as implements the check.
>> GNU as implements the logic when it is about to emit a relocation, not when folding expressions.
>
> All of this is pretty confusing. We //do// need to expand variable references, including those to undefined symbols, during expression evaluation in certain contexts, such as `.if`. We have some code in there to do this now (conditioning expansion based on "InSet"), but e.g.
>
>   x = a - 1
>   .if x + 1 == a
>   nop
>   .endif
>
> doesn't work with llvm, today, while it does in gas.



> OTOH, for something like the following, we definitely need to preserve the reference to b, and NOT try to expand it to a+1 too early (because we need to not emit the PLT relocation to global symbol a). Binutils' as compiles this as a PC32 relocation off of .text.a, while we generate a PLT32 relocation off of b; both of those seem probably fine.
>
>   b = a+1
>   
>   .section .text.foo
>   foo:
>   jmp b at PLT
>   
>   
>   .section .text.a
>   .globl a
>   a:
>   nop
>   nop

`b` is local (a symbol assignment does not copy the binding), so GNU as emits a relocation referencing the section symbol.
This patch does not change the behavior.

>> One major problem by moving the condition to MCExpr::evaluateAsRelocatableImpl is that we may do `isUndefined()` check too early (this special case does not kick in when the symbol is defined (`memcpy = __GI_memcpy; call memcpy ... __GI_memcpy: ...` should emit a relocation referencing memcpy, instead of __GI_memcpy)).
>
> I think we can ensure that we only do it at the right point, by checking whether we have the optional Layout object passed in. evaluateFixup calls evaluateAsRelocatable with a Layout object, but other uses that occur pre-layout will not.

It seems that the parse-time MCExpr::evaluateAsRelocatableImpl only takes actions if the value is absolute (not `isInSection()`),,,,, so MCExpr::evaluateAsRelocatableImpl actually works.
The whole infrastructure does give me an impression that it can have subtle behaviors in a few places....


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88625/new/

https://reviews.llvm.org/D88625