[PATCH] D72197: [MC][ELF] Emit a relocation if target is defined in the same section and is non-local

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D72197#1825545 <https://reviews.llvm.org/D72197#1825545>, @bd1976llvm wrote:

> In D72197#1825494 <https://reviews.llvm.org/D72197#1825494>, @MaskRay wrote:
>
> > In D72197#1825390 <https://reviews.llvm.org/D72197#1825390>, @pcc wrote:
> >
> > > In D72197#1825247 <https://reviews.llvm.org/D72197#1825247>, @MaskRay wrote:
> > >
> > > > In D72197#1825012 <https://reviews.llvm.org/D72197#1825012>, @pcc wrote:
> > > >
> > > > > It looks like this change caused us to start rejecting:
> > > > >
> > > > >   .thumb
> > > > >   .thumb_func
> > > > >   .globl foo
> > > > >   .hidden foo
> > > > >   foo:
> > > > >   adr lr, foo + 1
> > > > >
> > > > >
> > > > > with:
> > > > >
> > > > >   test.s:6:1: error: unsupported relocation on symbol
> > > > >   adr lr, foo + 1
> > > > >   ^
> > > > >  
> > > > >
> > > > >
> > > > > This is exposed by this Android ART code: https://cs.android.com/android/platform/superproject/+/master:art/runtime/arch/arm/quick_entrypoints_arm.S;l=1826
> > > > >
> > > > > I see a couple of possible fixes for this:
> > > > >
> > > > > 1. We could go back to the previous behaviour for global hidden symbols.
> > > > > 2. We could teach MC and LLD about the `R_ARM_THM_ALU_PREL_11_0` relocation required to relocate this instruction. Arguably the ART code shouldn't be adding the 1 for Thumb here (because `R_ARM_THM_ALU_PREL_11_0` adds the 1 itself), so ART would then need to be fixed.
> > > >
> > > >
> > > > `foo` is a hidden definition in the current object file. Can the ART code be changed to use a local symbol (`adr lr, .Lfoo`) instead?
> > >
> > >
> > > It could, but I don't really see a good argument for local symbols having different behaviour to strong hidden symbols.
> >
> >
> > Not special casing hidden in the general case has the nice property that: the assembler behavior is not affected by the visibility. Only the binding can.
> >
> > We can thus postpone decisions to the linker, and all the logic can be implemented on the linker side. Both a hidden defined symbol and a local symbol is considered non-preemptive, thus they have the same behavior at the linker stage.
> >
> > The downside is that we increase object file sizes. Hidden symbols are not that common so the downside may be acceptable.
>
>
> Please add a code comment with this rationale.


We seem to be consistent with GNU as on most targets. ARM/Thumb2 ADR seems a bit special. I can send a separate patch to you.

>> 
>> 
>>>> GNU as resolves `R_ARM_THM_ALU_PREL_11_0` at assembly time, regardless of the visibility. It also rejects an undefined symbol. This looks likes a relocation-specific behavior.
>>> 
>>> I think it's more like it doesn't know about the relocation, so it tries to resolve it locally.
>>> 
>>> I wouldn't put too much stock into GNU as's behaviour here, there seem to be bugs. For example, at least the version I'm using will accept this:
>>> 
>>>   .thumb
>>>   .thumb_func
>>>   .syntax unified
>>>   .globl foo
>>>   .hidden foo
>>>   foo:
>>>   adr lr, bar
>>>   
>>>   .data
>>>   bar:
>>> 
>>> 
>>> and won't emit a relocation for the adr instruction at all.
>>> 
>>>> I think a local symbol can make the intention more explicit, just don't know whether this is a broader issue.
>>> 
>>> I'm not sure about that, to me it would seem more like a workaround for an assembler bug.
> 
> I was feeling ok about the design decision to always emit relocations for references to STB_GLOBAL symbols. However, an alternative approach might be to base the behaviour on a switch? I might suggest something like --emit-relocations-for-references=<always,always-for-globals,only-when-required>? We used to have a downstream patch-set to force the emission of relocations for references to enable linker optimizations so I think that this is useful behaviour.

Do you mind sharing a bit more information how enabling relocations everywhere can help linker optimizations?

>> I'll need to make more analysis.
> 
> It looks to me like LLD is simply missing support for that relocation?

The assembler does not produce `R_ARM_THM_ALU_PREL_11_0`. I created D72892 <https://reviews.llvm.org/D72892> for @pcc's Thumb issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72197