[PATCH] D71649: [PPC32] Emit R_PPC_PLTREL24 for calls to dso_local ifunc

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D71649#1789767 <https://reviews.llvm.org/D71649#1789767>, @sfertile wrote:

> In terms of making a dso_local ifunc work this behavior seems logical to me: I'm skeptical though because I'm not sure we typically **know** when we are calling an ifunc.
>  From the gcc  docs on ifunc attribute <https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes>
>
> > The exported header file declaring the function the user calls would contain:
> >  `extern void *memcpy (void *, const void *, size_t);`
> >  allowing the user to call memcpy as a regular function, unaware of the actual implementation
>
> Maybe non-preemptable ifuncs are different in that they must be declared as ifuncs explicitly before use, but if not I think the compiler/linker/loader need to work in tandem to support calling an ifunc where the call site does not know the implementation is an `STT_IFUNC `as opposed to an `STT_FUNC`. If thats the case, doing something different at the callsite based on if the callee being an ifunc would be the wrong approach to making this work (even though it seems its the approach taken by gcc and ld.bfd).


I had wondered whether we should do the following instead, but I figure out that this may be a PPC32 specific problem.

  --- a/llvm/lib/Target/TargetMachine.cpp
  +++ b/llvm/lib/Target/TargetMachine.cpp
  @@ -97,3 +97,3 @@ bool TargetMachine::shouldAssumeDSOLocal(const Module &M,
     if (GV && GV->isDSOLocal())
  -    return true;
  +    return !isa<GlobalIFunc>(GV);

On PPC32, the problem is that for R_PPC_REL24 and R_PPC_PLTREL24(r_addend=0x8000), r30 is expected to point to different places (_GLOBAL_OFFSET_TABLE_ for R_PPC_REL24, .got2+addend for R_PPC_PLTREL24). If a function calls functions with different relocation types and both callees need a call stub, a single r30 value cannot make both call stubs happy.

In llvm MC and GNU as, STT_FUNC can be upgraded to STT_IFUNC. Ideally in C/C++ code users don't have to annotate a function as `__attribute__((ifunc(...)))` and they can annotate it in (inline) assembly with `.type ifunc, at gnu_indirect_function`.
It looks like GCC and ABI makers made a mistake. I think R_PPC_LOCAL24PC was a mistake. The jump instruction is relocated with either R_PPC_LOCAL24PC, R_PPC_REL24 or R_PPC_PLTREL24 depending on the -fno-pic/-fpic/-fPIC and -msecure-plt/-mbss-plt. I am glad that clang does not generate the useless R_PPC_LOCAL24PC for regular symbols (it generates it for `_GLOBAL_OFFSET_TABLE_`). (I should note that PPC64 does fix the problem. R_PPC_REL24, R_PPC_PLTREL24 and R_PPC_LOCAL24PC are unified. Choosing the relocation type according to the preemptivity property is wrong.)

I think non-preemptible ifuncs work correctly on EM_386, EM_X86_64, EM_ARM, and EM_AARCH64. They don't need the GlobalIFunc special case in TargetMachine::shouldAssumeDSOLocal or its call sites to work well. However, I may have missed something. I have also noticed that ifunc is not well tested. Applying the previous patch does not cause any test updates. If we do find other targets may need a similar GlobalIFunc special case, we can check whether it makes more sense to be placed in shouldAssumeDSOLocal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71649