Re: [PATCH] Allow direct access relocations referencing a protected function symbol

["H.J. Lu" <hjl.tools@gmail.com>]

On Wed, Jun 16, 2021 at 9:24 PM Fangrui Song <i@maskray.me> wrote:
>
> On 2021-06-17, Alan Modra wrote:
> >On Wed, Jun 16, 2021 at 01:11:43AM -0700, Fangrui Song wrote:
> >> While (I think Cary/Michael) and I prefer that STV_PROTECTED STT_FUNC symbols don't have unneeded dynamic relocations in -shared mode,
> >
> >I strongly prefer that too.  The only thing I object to is incorrect
> >fixes..
>
> Thanks...
>
> >> I don't mind if GNU ld keeps producing them.
> >> Anyhow the important thing is the following spurious error:
> >>
> >>   % gcc -fpic -shared -fuse-ld=bfd a.s
> >>   /usr/bin/ld.bfd: /tmp/ccWPJCLw.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
> >>   __attribute__((visibility("protected"))) void *foo () {
> >>     return (void *)foo;
> >>   }
> >
> >Agreed, that is an x86 linker bug.
>
> Sent https://sourceware.org/pipermail/binutils/2021-June/116985.html for
> x86-64.
>
> Hope some aarch64 folks in the CC list can fix aarch64...
>
> >Here is the result of your testcase on powerpc64le, power10 to get
> >pc-relative cpu support, and linked without startup files so just the
> >relocs in this code are seen.
> >
> >powerpc64le-linux-gcc -c -O2 -fPIC -mcpu=power10 prot2.c
> >powerpc64le-linux-ld -shared -o prot2 prot2.o
> >readelf -r prot2
> >There are no relocations in this file.
>
> So x86 and aarch64 have major issues. arm has a minor issue.
> other ports are probably good.
>
> x86-64: broken direct access relocation, unneeded GLOB_DAT
> aarch64: broken direct access relocation, unneeded GLOB_DAT
> arm: unneeded GLOB_DAT
> ppc64le: good
> mips64el: good
> riscv64: good
>
> >>
> >> If you decide to take over, fixing the error will be really great and will be an important step fixing protected symbols.
> >
> >It would be inappropriate for me to take over.  This is a target
> >issue, caused by a deliberate choice on the part of HJ to optimise in
> >the executable at the expense of shared library optimisation (*).  The
> >fix needs to be in the x86 target code, and it's a very long time
> >since I was an x86 binutils maintainer.
>
> nvm, I sent  https://sourceware.org/pipermail/binutils/2021-June/116985.html
>
> >*) I'll note that at the time, when non-pic executables were more
> >common, the trick HJ used made quite a lot of sense from an
> >optimisation viewpoint, but frankly I'm amazed that he managed to get
> >it all working.  In fact, the optimisation makes even more sense on
> >powerpc prior to power10 but I was unwilling to go against the ELF
> >gABI (when narrowly considering the resulting shared libraries in
> >isolation) just to shave off cycles in micro-benchmarks.
>
> Hmm, I think those changes don't improve -fno-pic or -fpie performance.
> ELF -fno-pic has always been using absolute relocations for
> data/functions. For -fpie, HAVE_LD_PIE_COPYRELOC has no benefit but a
> tiny bit of size decrease.  ppc64 ELFv2 toc-indirection to toc-relative
> optimization renders the GOT optimization mostly useless, as well.
> HAVE_LD_PIE_COPYRELOC, if implemented, may have a tiny bit of effects on
> other arches, but I am of the opinion that toolchain developers do not
> necessarily show mercy to applications where global variable access is a
> bottleneck.  The applications should fix their global variable usage:
> hidden/protected/local alias. Either can improve global variable access
> if they indeed decide to care. Unfortunately protected data accesses
> have unneeded GOT indirection for all GCC arches in -fpic mode.

On x86-64, function pointers in executable for external funtions may be
resolved to their PLT entries in executable.  If it happens, function
pointers of protected funtions in shared libraries must be resolved to
the PLT entries in executable, not addresses of protected funtions in
shared libraries.

I made a proposal to remove copy relocation and add canonical function
address:

https://gitlab.com/x86-psABIs/x86-64-ABI/-/issues/8

It should address this issue.  But this is an ABI change.  The question
is if we should do it silently which can result in run-time failure.

-- 
H.J.