ARM-software / abi-aa

Application Binary Interface for the Arm® Architecture
Other
878 stars 173 forks source link

[aaelf64] Fix equation for R_AARCH64_GOTPCREL32 #247

Open PiJoules opened 5 months ago

PiJoules commented 5 months ago

The current wording for this reloc is G(GDAT(S+A))-P which means the addend is applied to the symbol we're taking the GOT entry for. What we actually want (and what's implemented in lld) is something similar to R_X86_64_GOTPCREL where the addend is applied to the location of the reloc rather than the symbol.

smithp35 commented 4 months ago

As mentioned in https://github.com/ARM-software/abi-aa/pull/223 there is a generic ABI issue with how GDAT(S+A) is written vs how it is implemented https://github.com/ARM-software/abi-aa/issues/217 . No linker implements it as written, and the implementations are not consistent. The practical conclusion is that the only useful addend value for most of these relocations is 0 as that is the one value that all agree on.

For this specific relocation, it is only implemented by LLD so we have some scope to make the proposed change to match an implementation.

As an open question, can you foresee any occasion when the compiler would use a non zero addend using this relocation? If the answer is yes then we definitely need to use this change. If the answer is no, then we may be better off forbidding non zero addends as proposed as a resolution for https://github.com/ARM-software/abi-aa/issues/217

MaskRay commented 4 months ago

I agree that GDAT(S+A) is certainly not ideal. Other architectures don't do this, and typical linker implementations associate a single GOT entry to a symbol. Support multiple GOT entries for a symbol or supporting a GOT entry for an address requires more code and typically not good for performance.

As an open question, can you foresee any occasion when the compiler would use a non zero addend using this relocation? If the answer is yes then we definitely need to use this change. If the answer is no, then we may be better off forbidding non zero addends as proposed as a resolution for https://github.com/ARM-software/abi-aa/issues/217

There is a curious R_X86_64_GOTPCREL use with a non-zero addend: https://sourceware.org/bugzilla/show_bug.cgi?id=26939

   void x();
   int foo() { return (long)x >> 32; }

If ABI arbiters' stance is that this does not justify allowing a non-zero addend, I think it will be fairly reasonable to disallow addend!=0.

If the answer is no, then we may be better off forbidding non zero addends as proposed as a resolution for https://github.com/ARM-software/abi-aa/issues/217

I support this stance.

smithp35 commented 2 weeks ago

I've submitted https://github.com/ARM-software/abi-aa/pull/272 which removes A from the GDAT(S + A) relocation operation.