[PATCH] D72892: [MC][ARM] Resolve some pcrel fixups at assembly time

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D72892#1923502 <https://reviews.llvm.org/D72892#1923502>, @psmith wrote:

> In D72892#1918532 <https://reviews.llvm.org/D72892#1918532>, @efriedma wrote:
>
> > Oh, if binutils is assuming the symbol isn't preemptible, there isn't much point to generating the relocation and waiting for the linker to resolve it; we should just resolve it the same way in the assembler.
>
>
> My understanding is that fixing this at link time is falling out of favour. We still have some work to do here and I'd like to agree a plan on what to do next. Current state:
>
> - Arm state adr and ldr instructions are resolved at assembly time. Unfortunately it looks like we resolve even the cross section references which leads to incorrect code. No existing code will have this problem as it used to give an error message, but it is something we need to fix.
> - Thumb state adr and ldr instructions are resolved at link time via relocations
> - LLD Thumb state support for adr and ldr relocations have landed, depending on the assembler support for tests.
> - LLD Arm state support has not yet landed. It is more complex due to the adr using modified immediate addressing.
>
>   If we are to resolve the relocations at assembly time then we need to decide:
> - What do we do about adr and ldr to symbols outside the section? We can expose as relocations or make an error message. Currently we resolve them prematurely, most likely getting the wrong result. I have written the patches for LLD support and the binutils linkers have support so I'd prefer that.
> - What do we do about the LLD support for Thumb relocations? If we remove assembler support then we can either revert the patch adding them or I can use yaml2obj. I'd prefer the latter.
> - Is there any hope for the LLD support for Arm adr and ldr relocations? If there is none I'll abandon the patch. I'm happy to update tests to use yaml2obj if assembler support is removed.
>
>   My preference is:
> - Resolve fixup at assembly time where possible, emit relocation when not.


This is the same as GNU as. +1

> - Keep LLD support for Thumb and add support for Arm, the tests have been written to only use adr and ldr that cannot be resolved at assembly time. But I would say that as I don't want to waste all of the effort put into writing it. I'm happy to go with the consensus.

More precisely, keep code change in D75349 <https://reviews.llvm.org/D75349> but rewrite the tests with yaml2obj plus output section address. The lld code path will be very difficult to exercise.

> The problem with early resolution in the current implementation is:
> 
>   IsResolved = (FixupFlags & MCFixupKindInfo::FKF_Constant) ||
>                Writer->isSymbolRefDifferenceFullyResolvedImpl(
>                    *this, SA, *DF, false, true);
>    
> 
> We should add an additional check that the relocation and destination are in the same section for FKF_Constant. A quick hack that fixes this is:
> 
>    IsResolved = ((FixupFlags & MCFixupKindInfo::FKF_Constant) &&
>                   Writer->isSymbolRefDifferenceFullyResolvedImpl(
>                       *this, SA, *DF, false, false)) ||
>                  Writer->isSymbolRefDifferenceFullyResolvedImpl(
>                      *this, SA, *DF, false, true);
>   }
>    
> 
> This lies about being pc-relative to isSymbolRefDifferenceFullyResolvedImpl which will ultimately call MCObject::isSymbolRefDifferenceFullyResolvedImpl which will test that the fixup and destination are in the same section. There is likely a better way though.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72892