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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D72197#1806478 <https://reviews.llvm.org/D72197#1806478>, @MaskRay wrote:

> In D72197#1805964 <https://reviews.llvm.org/D72197#1805964>, @sfertile wrote:
>
> > In D72197#1805438 <https://reviews.llvm.org/D72197#1805438>, @bd1976llvm wrote:
> >
> > > I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?
> >
> >
> > I had a talk about this once I believe with @hfinkel (or maybe @dblaikie), and this behavior was intentional. Essentially LLVM may perform IPO between the 2 functions, in which case its no longer appropriate  to allow the callee to be preempted at runtime. The back-ends aren't consistent on this behaviour either. For example:
> >
> >   __attribute__((noinline))
> >   int bar(int i) {
> >       return i;
> >   }
> >  
> >  
> >   int caller(int i) {
> >       if (bar(i) == i)
> >         return 0;
> >  
> >       return 1;
> >   }
> >
> >
> > Compiling this for PowerPC and AArch64 Linux and -fPIC I get no relocation for the call. Compiling for X86_64 Linux and -fPIC I do get a relocation for the call. So it seems to be pick your poison: Emit a relocation and expose a potential runtime problem, or don't emit a relocation and break ELF interposition semantics.  I'm also not sure if the optimizer actually skips all IPO when the functions are in different sections, so we might end up with both broken shared object interposition and potentially unsafe codegen 😦
>
>
> Behavior without this patch.
>
> - `clang -target {aarch64,ppc64le} -fPIC -c` => no relocation referencing `bar`
> - `clang -target {aarch64,ppc64le} -fPIC -c -ffunction-sections` => a branch relocation referencing `bar`
>
>   Look, we are already inconsistent. With this patch, we will consistently emit a relocation. I thinks this patch makes the situation better.


I wouldn't call that inconsistent - it's necessary if the two functions are in separate sections, and isn't (if you aren't supporting symbol interposition) if they aren't, right?

I think the question I have: What is the value (I don't mean to assume there is no value, but I want to understand the specific use case) of supporting this if the full generality of symbol interposition is not supported? (is there a well defined use case that is usable with this patch and doesn't need the full generality? Or is the use case going to be subtly broken if it changes even slighlty and happens to trip over another part of LLVM that doesn't support symbol interposition? (in which case this is a great patch, but only as part of an effort to fully support the general case))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72197