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

>> 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'll need to make more analysis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72197