[PATCH] D88625: [MC] Redirect fixup symbol to the aliasee (if undefined)

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

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



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88625