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

Sun Feb 9 21:00:44 PST 2020

MaskRay added a comment.

In D72197#1866020 <https://reviews.llvm.org/D72197#1866020>, @xiangzhangllvm wrote:

> In D72197#1806225 <https://reviews.llvm.org/D72197#1806225>, @MaskRay 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?
> > >
> > > Also, this code is missing a case for STB_GLOBAL and visibility != STV_DEFAULT.  Actually, I have noticed before that this case is missing in various places in MC. As it stands this patch may cause a performance regression on platforms that use -fvisibility=hidden.
> >
> >
> > Visibility is irrelevant here. For a symbol defined in the same section as the instruction, the relocation record will reference a non-SECTION symbol instead of a STT_SECTION after this change. If the symbol is STV_HIDDEN, the linker will think the symbol is non-preemptible. So, no performance regression.
> >
> > There are many ways to make a symbol non-preemptible: binding (STB_LOCAL), visibility (STV_INTERNAL, STV_PROTECTED or STV_HIDDEN), --dynamic-list, VER_NDX_LOCAL, -Bsymbolic, -Bsymbolic-functions, etc. I have made enough refactorings to lld Symbol::isPreemptible and I am confident with these things :)
> >
> > GCC r212049 (first included in GCC 5) introduced -fsemantic-interposition. Yes, current Clang/LLVM behavior is like -fno-semantic-interposition. Our behavior is not ideal and @hfinkel had an RFC https://lists.llvm.org/pipermail/llvm-dev/2016-November/107625.html  . This change should help -fsemantic-interposition if we decide to move towards that goal.
>
>
> Hi Fangrui, for "protected" visibility, that references within the defining module (mostly same section) will bind to the local symbol. So I think here should except "protected":
>
>   -   if (SymA.getBinding() != ELF::STB_LOCAL
>   +   if (SymA.getBinding() != ELF::STB_LOCAL  && (SymA.getBinding() != ELF::STB_GLOBAL && SymA.getVisibility() != ELF::STV_PROTECTED)
>
>
> Now Clang also get a bug about the following test :
>
>   $cat t.c
>   __attribute__((visibility("protected"))) void * foo (void) { return (void *)foo; }
>   $clang -c  -O0 -g  -fpic t.c
>   $clang -o t.so  -O0 -g  -fpic t.o  -shared
>   /usr/bin/ld: t.o: relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object
>   /usr/bin/ld: final link failed: Bad value
>   clang-11: error: linker command failed with exit code 1 (use -v to see invocation)
>   $/usr/bin/ld -v
>   GNU ld version 2.30-49.el8
>

@xiangzhangllvm This is not related to this patch, though I'd like to make some explanations here.

  gcc -fpic a.c -shared -fuse-ld=bfd # relocation R_X86_64_PC32 against protected symbol `foo' can not be used when making a shared object

binutils 2.26 introduced a regression: R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

The original issue is "Copy relocation against protected symbol doesn't work".
I agree with Rich Felker (https://gcc.gnu.org/ml/gcc/2016-04/msg00168.html) and
Cary Coutant (https://gcc.gnu.org/ml/gcc/2016-04/msg00158.html https://gcc.gnu.org/ml/gcc/2016-04/msg00169.html) that we should
keep using direct access against protected symbols and disallow copy relocations against protected symbols.

I appreciate that Cary Coutant and Rafael Ávila de Espíndola added diagnostics to gold and lld, respectively:

- gold (https://sourceware.org/bugzilla/show_bug.cgi?id=19823)
- lld (https://bugs.llvm.org/show_bug.cgi?id=31476)

@xiangzhangllvm Perhaps you are in a good position to change the resolutions to the following issues:)

GCC 5 x86-64 introduced a regression (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65248)
i386 was flagged as a reproduce (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55012)

  __attribute__((visibility("protected"))) int a;
  int foo() { return a; } // GCC>=5 uses R_X86_64_GOTPCREL/R_X86_64_REX_GOTPCRELX instead of R_X86_64_PC32

binutils 2.26 introduced a regression R_X86_64_PC32 can no longer be used against a protected symbol https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=ca3fe95e469b9daec153caa2c90665f5daaec2b5

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72197