[PATCH] D157020: [lld/ELF] Don't relax R_X86_64_(REX_)GOTPCRELX when offset is too far

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/test/ELF/x86-64-gotpc-err.s:10
-
-# CHECK:      error: {{.*}}:(.text+0x2): relocation R_X86_64_GOTPCRELX out of range: 2147483655 is not in [-2147483648, 2147483647]; references '__stop_data'
-# CHECK-NEXT: error: {{.*}}:(.text+0x9): relocation R_X86_64_REX_GOTPCRELX out of range: 2147483648 is not in [-2147483648, 2147483647]; references '__stop_data'
----------------
aeubanks wrote:
> MaskRay wrote:
> > aeubanks wrote:
> > > aeubanks wrote:
> > > > MaskRay wrote:
> > > > > You move this into `x86-64-gotpc-relax-too-far.s` but lose this error.
> > > > > 
> > > > > We need another linker script to place `.got` far away from `.text` to retain coverage for this error.
> > > > > 
> > > > > 
> > > > Correct me if I'm wrong, but I don't think this was testing a far away `.got` and that's not what the original patch was doing: https://reviews.llvm.org/D93259.
> > > > 
> > > > I can add test coverage for that if you'd like though.
> > > https://github.com/llvm/llvm-project/blob/e6b2525dafbab55e5e7748f3f302bb6a45899511/lld/ELF/Arch/X86_64.cpp#L768 is the check for a far away `.got` and removing that check at clean ToT doesn't make check-lld fail so I don't think we have test coverage for it
> > I am not sure what you mean but `checkInt(loc, val, 32, rel);` in `relaxGot` is how we get an `relocation R_X86_64_REX_GOTPCRELX out of range` error for `x86-64-gotpc-err.s`.
> > 
> > With the current patch, we lose test for `relocation R_X86_64_REX_GOTPCRELX out of range`. We should do:
> > 
> > > We need another linker script to place .got far away from .text to retain coverage for this error.
> > 
> > 
> with this patch, the `checkInt` in `relaxGot` should never trigger (I should make it an assert instead). that check isn't the distance to the got, it's the distance to the actual symbol since we only go to `relaxGot` if we've relaxed the GOT load to a direct reference to the symbol
> 
> the check [here](https://github.com/llvm/llvm-project/blob/e6b2525dafbab55e5e7748f3f302bb6a45899511/lld/ELF/Arch/X86_64.cpp#L768) is what you're referring to the `.got` is too far away, and we'll only hit that with `-Wl,--no-relax`. and we don't have any existing test coverage for that `checkInt`
If checkInt in relaxGot should never trigger, that code should be changed.

We still need a check when `.got` is more than 2GiB away from the relocated location. It's ok if the message has changed, but we need to test the behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157020