ppc64le: expected localentry:0 `pthread_condattr_destroy'

ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Josh Stone]

Hi folks,

I mentioned this regression to fweimer, and he suggested I should bring
it up to libc-alpha and binutils.  I was just trying to rebuild rustc on
Fedora Rawhide, and ppc64le alone failed with this localentry error.
Florian suspected that a binutils update in particular now enabled use
of PPC64_OPT_LOCALENTRY.

I reproduced the regression from old to new binutils, and uploaded those
artifacts here if anyone else wants to inspect them:

https://jistone.fedorapeople.org/rust-1.19.0-stage1-ppc64le-binutils-2.28.tar.xz
https://jistone.fedorapeople.org/rust-1.19.0-stage1-ppc64le-binutils-2.29.tar.xz

rust-1.19.0-stage1-ppc64le-binutils-2.28:
# LD_LIBRARY_PATH=./lib ./bin/rustc -V
rustc 1.19.0

rust-1.19.0-stage1-ppc64le-binutils-2.29:
# LD_LIBRARY_PATH=./lib ./bin/rustc -V
./bin/rustc: error while loading shared libraries:
./lib/libstd-c3a1748e15265da7.so: expected localentry:0
`pthread_condattr_destroy'

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Carlos O'Donell]

On 07/25/2017 09:11 PM, Josh Stone wrote:
> Hi folks,
> 
> I mentioned this regression to fweimer, and he suggested I should bring
> it up to libc-alpha and binutils.  I was just trying to rebuild rustc on
> Fedora Rawhide, and ppc64le alone failed with this localentry error.
> Florian suspected that a binutils update in particular now enabled use
> of PPC64_OPT_LOCALENTRY.
> 
> I reproduced the regression from old to new binutils, and uploaded those
> artifacts here if anyone else wants to inspect them:
> 
> https://jistone.fedorapeople.org/rust-1.19.0-stage1-ppc64le-binutils-2.28.tar.xz
> https://jistone.fedorapeople.org/rust-1.19.0-stage1-ppc64le-binutils-2.29.tar.xz
> 
> rust-1.19.0-stage1-ppc64le-binutils-2.28:
> # LD_LIBRARY_PATH=./lib ./bin/rustc -V
> rustc 1.19.0
> 
> rust-1.19.0-stage1-ppc64le-binutils-2.29:
> # LD_LIBRARY_PATH=./lib ./bin/rustc -V
> ./bin/rustc: error while loading shared libraries:
> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
> `pthread_condattr_destroy'
 
Right, this looks like a ppc64le-specific (ELFv2) regression in the unreleased
binutils 2.29. I've not seen this with the release 2.28.

-- 
Cheers,
Carlos.

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Josh Stone]

On 07/26/2017 12:13 PM, Carlos O'Donell wrote:
> On 07/25/2017 09:11 PM, Josh Stone wrote:
>> Hi folks,
>>
>> I mentioned this regression to fweimer, and he suggested I should bring
>> it up to libc-alpha and binutils.  I was just trying to rebuild rustc on
>> Fedora Rawhide, and ppc64le alone failed with this localentry error.
>> Florian suspected that a binutils update in particular now enabled use
>> of PPC64_OPT_LOCALENTRY.
>>
>> I reproduced the regression from old to new binutils, and uploaded those
>> artifacts here if anyone else wants to inspect them:
>>
>> https://jistone.fedorapeople.org/rust-1.19.0-stage1-ppc64le-binutils-2.28.tar.xz
>> https://jistone.fedorapeople.org/rust-1.19.0-stage1-ppc64le-binutils-2.29.tar.xz
>>
>> rust-1.19.0-stage1-ppc64le-binutils-2.28:
>> # LD_LIBRARY_PATH=./lib ./bin/rustc -V
>> rustc 1.19.0
>>
>> rust-1.19.0-stage1-ppc64le-binutils-2.29:
>> # LD_LIBRARY_PATH=./lib ./bin/rustc -V
>> ./bin/rustc: error while loading shared libraries:
>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
>> `pthread_condattr_destroy'
>  
> Right, this looks like a ppc64le-specific (ELFv2) regression in the unreleased
> binutils 2.29. I've not seen this with the release 2.28.

It does appear to be released -- binutils-2_29 was tagged on Monday.

But anyway, are you seeing this in other contexts yourself?

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Carlos O'Donell]

On 07/26/2017 06:34 PM, Josh Stone wrote:
> On 07/26/2017 12:13 PM, Carlos O'Donell wrote:
>> On 07/25/2017 09:11 PM, Josh Stone wrote:
>>> Hi folks,
>>>
>>> I mentioned this regression to fweimer, and he suggested I should bring
>>> it up to libc-alpha and binutils.  I was just trying to rebuild rustc on
>>> Fedora Rawhide, and ppc64le alone failed with this localentry error.
>>> Florian suspected that a binutils update in particular now enabled use
>>> of PPC64_OPT_LOCALENTRY.
>>>
>>> I reproduced the regression from old to new binutils, and uploaded those
>>> artifacts here if anyone else wants to inspect them:
>>>
>>> https://jistone.fedorapeople.org/rust-1.19.0-stage1-ppc64le-binutils-2.28.tar.xz
>>> https://jistone.fedorapeople.org/rust-1.19.0-stage1-ppc64le-binutils-2.29.tar.xz
>>>
>>> rust-1.19.0-stage1-ppc64le-binutils-2.28:
>>> # LD_LIBRARY_PATH=./lib ./bin/rustc -V
>>> rustc 1.19.0
>>>
>>> rust-1.19.0-stage1-ppc64le-binutils-2.29:
>>> # LD_LIBRARY_PATH=./lib ./bin/rustc -V
>>> ./bin/rustc: error while loading shared libraries:
>>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
>>> `pthread_condattr_destroy'
>>  
>> Right, this looks like a ppc64le-specific (ELFv2) regression in the unreleased
>> binutils 2.29. I've not seen this with the release 2.28.
> 
> It does appear to be released -- binutils-2_29 was tagged on Monday.
> 
> But anyway, are you seeing this in other contexts yourself?
 
No, you are the first person I've seen run into this issue.

I suggest you file a binutils bug right away to get this fixed,
or find out what is going wrong with the rust linkage of that
shared object. It could be something wrong with your invocation
of `ld' if you're doing that manually. When invoking ld without
using gcc as the driver you must be acutely aware of all the
required options to produce a functioning link.

-- 
Cheers,
Carlos.

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Alan Modra]

> >> On 07/25/2017 09:11 PM, Josh Stone wrote:
> >>> ./bin/rustc: error while loading shared libraries:
> >>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
> >>> `pthread_condattr_destroy'

You will get this error if the link-time version of a function symbol
is seen as localentry:0 (ie. not needing a global entry point due to
not needing a valid r2 toc pointer), but the run-time version does.

The most likely thing is that your library was linked against a stub
version of pthread_condattr_destroy.  Making the stub weak will
disable the generation of the optimized localentry:0 plt call code.
So will linking with -Wl,--no-plt-localentry

-- 
Alan Modra
Australia Development Lab, IBM

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Josh Stone]

On 07/26/2017 06:36 PM, Carlos O'Donell wrote:
> No, you are the first person I've seen run into this issue.

Someone else reported a similar error on fedora-devel, unrelated to
Rust, so it's not just me. :)

> I suggest you file a binutils bug right away to get this fixed,

Filed: https://bugzilla.redhat.com/show_bug.cgi?id=1475636

> or find out what is going wrong with the rust linkage of that
> shared object. It could be something wrong with your invocation
> of `ld' if you're doing that manually. When invoking ld without
> using gcc as the driver you must be acutely aware of all the
> required options to produce a functioning link.

As far as all that goes, rustc does use cc to link.

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Josh Stone]

On 07/26/2017 09:04 PM, Alan Modra wrote:
>>>> On 07/25/2017 09:11 PM, Josh Stone wrote:
>>>>> ./bin/rustc: error while loading shared libraries:
>>>>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
>>>>> `pthread_condattr_destroy'
> 
> You will get this error if the link-time version of a function symbol
> is seen as localentry:0 (ie. not needing a global entry point due to
> not needing a valid r2 toc pointer), but the run-time version does.
> 
> The most likely thing is that your library was linked against a stub
> version of pthread_condattr_destroy.  Making the stub weak will
> disable the generation of the optimized localentry:0 plt call code.
> So will linking with -Wl,--no-plt-localentry

Well, the only place the rust std library uses this function is in it's
own Condvar wrapper, and it's called through an FFI declaration without
any stub.  I can write a trivial program that creates a Condvar, and it
links and executes just fine.  I even verified that the call is really
made with ltrace.  So I can at least say the way rust is linking this is
not *totally* broken.

I also found that "ldd -r ./lib/libstd-c3a1748e15265da7.so" is just
fine, but "ldd -r ./bin/rustc" gives the error in libstd again.  That
rustc binary is basically just a shim that calls librustc_driver, but
"ldd -r" on that library also has no error.

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Florian Weimer]

On 07/27/2017 06:04 AM, Alan Modra wrote:
>>>> On 07/25/2017 09:11 PM, Josh Stone wrote:
>>>>> ./bin/rustc: error while loading shared libraries:
>>>>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
>>>>> `pthread_condattr_destroy'
> 
> You will get this error if the link-time version of a function symbol
> is seen as localentry:0 (ie. not needing a global entry point due to
> not needing a valid r2 toc pointer), but the run-time version does.
> 
> The most likely thing is that your library was linked against a stub
> version of pthread_condattr_destroy.  Making the stub weak will
> disable the generation of the optimized localentry:0 plt call code.
> So will linking with -Wl,--no-plt-localentry

The common theme seems to be that this only affects symbols which live
both in libpthread and libc.  Do you suggest that we have to make the
implementation in libc a weak symbol?

Why isn't this a plain regression break ELF symbol interposition?

I filed a glibc bug for this:

  https://sourceware.org/bugzilla/show_bug.cgi?id=21847

I think we need to sort this out before the release.  That does not mean
we have to fix it, we just need a better understanding what is going on.

Thanks,
Florian

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Florian Weimer]

On 07/27/2017 11:55 AM, Florian Weimer wrote:
> On 07/27/2017 06:04 AM, Alan Modra wrote:
>>>>> On 07/25/2017 09:11 PM, Josh Stone wrote:
>>>>>> ./bin/rustc: error while loading shared libraries:
>>>>>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
>>>>>> `pthread_condattr_destroy'
>>
>> You will get this error if the link-time version of a function symbol
>> is seen as localentry:0 (ie. not needing a global entry point due to
>> not needing a valid r2 toc pointer), but the run-time version does.
>>
>> The most likely thing is that your library was linked against a stub
>> version of pthread_condattr_destroy.  Making the stub weak will
>> disable the generation of the optimized localentry:0 plt call code.
>> So will linking with -Wl,--no-plt-localentry
> 
> The common theme seems to be that this only affects symbols which live
> both in libpthread and libc.  Do you suggest that we have to make the
> implementation in libc a weak symbol?
> 
> Why isn't this a plain regression break ELF symbol interposition?
> 
> I filed a glibc bug for this:
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=21847
> 
> I think we need to sort this out before the release.  That does not mean
> we have to fix it, we just need a better understanding what is going on.

I injected -Wl,--no-plt-localentry into the build, using this patch:

--- a/sysdeps/powerpc/powerpc64le/Makefile
+++ b/sysdeps/powerpc/powerpc64le/Makefile
@@ -6,6 +6,10 @@
 # linked executables, forcing to link the loader after libgcc link.
 f128-loader-link = $(as-needed) $(elf-objpfx)ld.so $(no-as-needed)

+sysdep-LDFLAGS += -Wl,--no-plt-localentry
+# Linking libc_pic.os ignores sysdep-LDFLAGS.
+LDFLAGS-c_pic.os += -Wl,--no-plt-localentry
+
 ifeq ($(subdir),math)
 # sqrtf128 requires emulation before POWER9.
 CPPFLAGS += -I../soft-fp

But the issue persists afterwards.  I'm afraid we'll need a real fix.

Thanks,
Florian

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Carlos O'Donell]

On 07/27/2017 12:04 AM, Alan Modra wrote:
>>>> On 07/25/2017 09:11 PM, Josh Stone wrote:
>>>>> ./bin/rustc: error while loading shared libraries:
>>>>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
>>>>> `pthread_condattr_destroy'
> 
> You will get this error if the link-time version of a function symbol
> is seen as localentry:0 (ie. not needing a global entry point due to
> not needing a valid r2 toc pointer), but the run-time version does.
> 
> The most likely thing is that your library was linked against a stub
> version of pthread_condattr_destroy.  Making the stub weak will
> disable the generation of the optimized localentry:0 plt call code.
> So will linking with -Wl,--no-plt-localentry
 
The new binutils 2.29 also appears to break glibc's tst-tlsopt-ppc test.

Which fails with:
tls_index not optimized, binutils too old?

I don't know if this is a test invariant being broken by the new binutils
in which case the test needs updating.

Tulio, Are you seeing this?

-- 
Cheers,
Carlos.

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Alan Modra]

On Thu, Jul 27, 2017 at 01:40:11PM -0400, Carlos O'Donell wrote:
> On 07/27/2017 12:04 AM, Alan Modra wrote:
> >>>> On 07/25/2017 09:11 PM, Josh Stone wrote:
> >>>>> ./bin/rustc: error while loading shared libraries:
> >>>>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
> >>>>> `pthread_condattr_destroy'
> > 
> > You will get this error if the link-time version of a function symbol
> > is seen as localentry:0 (ie. not needing a global entry point due to
> > not needing a valid r2 toc pointer), but the run-time version does.
> > 
> > The most likely thing is that your library was linked against a stub
> > version of pthread_condattr_destroy.  Making the stub weak will
> > disable the generation of the optimized localentry:0 plt call code.
> > So will linking with -Wl,--no-plt-localentry
>  
> The new binutils 2.29 also appears to break glibc's tst-tlsopt-ppc test.
> 
> Which fails with:
> tls_index not optimized, binutils too old?
> 
> I don't know if this is a test invariant being broken by the new binutils
> in which case the test needs updating.
> 
> Tulio, Are you seeing this?

That is a different issue, exposed by binutils commit 676ee2b5f

	* elf64-ppc.c (ppc64_elf_relocate_section): Don't optimize
	__tls_index GOT entries when using __tls_get_addr_opt stub.
	* elf32-ppc.c (ppc_elf_relocate_section): Likewise.

Before that, optimized __tls_index entries were generated by ld,
wrongly so for a shared library.  Now, the __tls_index entry needs to
be set up by a call to __tls_get_addr_opt.  I think it is exposing a
problem with CHECK_STATIC_TLS / TRY_STATIC_TLS in glibc but I have not
properly debugged it yet.

-- 
Alan Modra
Australia Development Lab, IBM

tst-tlsopt-powerpc fail

[Alan Modra]

On Fri, Jul 28, 2017 at 09:15:29AM +0930, Alan Modra wrote:
> On Thu, Jul 27, 2017 at 01:40:11PM -0400, Carlos O'Donell wrote:
> > The new binutils 2.29 also appears to break glibc's tst-tlsopt-ppc test.
> > 
> > Which fails with:
> > tls_index not optimized, binutils too old?
> > 
> > I don't know if this is a test invariant being broken by the new binutils
> > in which case the test needs updating.
> > 
> > Tulio, Are you seeing this?
> 
> That is a different issue, exposed by binutils commit 676ee2b5f
> 
> 	* elf64-ppc.c (ppc64_elf_relocate_section): Don't optimize
> 	__tls_index GOT entries when using __tls_get_addr_opt stub.
> 	* elf32-ppc.c (ppc_elf_relocate_section): Likewise.
> 
> Before that, optimized __tls_index entries were generated by ld,
> wrongly so for a shared library.  Now, the __tls_index entry needs to
> be set up by a call to __tls_get_addr_opt.  I think it is exposing a
> problem with CHECK_STATIC_TLS / TRY_STATIC_TLS in glibc but I have not
> properly debugged it yet.

OK, so now I have looked at it properly.  Ignore my comment about
__tls_get_addr_opt needing to set up the __tls_index entry.  That was
just plain wrong.  I'd forgotten how things are supposed to work.

The __tls_index entry is set up by ld.so processing DTPMOD64 and
DTPREL64 dynamic relocs, but in the case of tst-tslopt-powerpc we
don't have any such relocs..  It's really just a testsuite issue.
tst-tlsopt-powerpc.c was bogus from the start, and probably only
worked because ld emitted dynamic relocs unnecessarily.

Since tst-tlsopt-powerpc is supposed to test glibc dynamic relocation
processing, the body of the test ought to be put in a shared library
(*).  I cobbled together such a test, and TRY_STATIC_TLS works fine.
So, not a powerpc64 glibc bug.

*) Or at the very least, the thread variable put in a shared library,
but the whole thing shared is probably better.

-- 
Alan Modra
Australia Development Lab, IBM

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Alan Modra]

On Thu, Jul 27, 2017 at 06:34:33PM +0200, Florian Weimer wrote:
> On 07/27/2017 11:55 AM, Florian Weimer wrote:
> > On 07/27/2017 06:04 AM, Alan Modra wrote:
> >>>>> On 07/25/2017 09:11 PM, Josh Stone wrote:
> >>>>>> ./bin/rustc: error while loading shared libraries:
> >>>>>> ./lib/libstd-c3a1748e15265da7.so: expected localentry:0
> >>>>>> `pthread_condattr_destroy'
> >>
> >> You will get this error if the link-time version of a function symbol
> >> is seen as localentry:0 (ie. not needing a global entry point due to
> >> not needing a valid r2 toc pointer), but the run-time version does.
> >>
> >> The most likely thing is that your library was linked against a stub
> >> version of pthread_condattr_destroy.  Making the stub weak will
> >> disable the generation of the optimized localentry:0 plt call code.
> >> So will linking with -Wl,--no-plt-localentry
> > 
> > The common theme seems to be that this only affects symbols which live
> > both in libpthread and libc.  Do you suggest that we have to make the
> > implementation in libc a weak symbol?

Right, the stub symbols in libpthread.so should be make weak.
See my comment in pr21847.  We're getting a mismatch between ld symbol
resolution and ld.so resolution.

> > Why isn't this a plain regression break ELF symbol interposition?
> > 
> > I filed a glibc bug for this:
> > 
> >   https://sourceware.org/bugzilla/show_bug.cgi?id=21847
> > 
> > I think we need to sort this out before the release.  That does not mean
> > we have to fix it, we just need a better understanding what is going on.
> 
> I injected -Wl,--no-plt-localentry into the build, using this patch:
> 
> --- a/sysdeps/powerpc/powerpc64le/Makefile
> +++ b/sysdeps/powerpc/powerpc64le/Makefile
> @@ -6,6 +6,10 @@
>  # linked executables, forcing to link the loader after libgcc link.
>  f128-loader-link = $(as-needed) $(elf-objpfx)ld.so $(no-as-needed)
> 
> +sysdep-LDFLAGS += -Wl,--no-plt-localentry
> +# Linking libc_pic.os ignores sysdep-LDFLAGS.
> +LDFLAGS-c_pic.os += -Wl,--no-plt-localentry
> +
>  ifeq ($(subdir),math)
>  # sqrtf128 requires emulation before POWER9.
>  CPPFLAGS += -I../soft-fp
> 
> But the issue persists afterwards.  I'm afraid we'll need a real fix.

No, you misunderstood my comment about --no-plt-localentry.  The
*user* library would need to be built with that option, which isn't a
good solution.

-- 
Alan Modra
Australia Development Lab, IBM

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Nick Clifton]

Hi Alan,

> So will linking with -Wl,--no-plt-localentry

Naive question - how does this optimization get enabled ?

My reading of the code is that ppc64elf.em initialises the plt_localentry0
field in the params structure to -1, and then elf64-ppc.c:ppc64_elf_tls_setup()
sets the field to either 1 or 0 depending upon whether the GLIB_2.26 symbol
exists.  But - Fedora rawhide is using glibc 2.25, so the symbol should not
exist, and so the optimization should not be enabled... right ?

Cheers
  Nick

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Florian Weimer]

On 07/28/2017 12:41 PM, Nick Clifton wrote:
> My reading of the code is that ppc64elf.em initialises the plt_localentry0
> field in the params structure to -1, and then elf64-ppc.c:ppc64_elf_tls_setup()
> sets the field to either 1 or 0 depending upon whether the GLIB_2.26 symbol
> exists.  But - Fedora rawhide is using glibc 2.25, so the symbol should not
> exist, and so the optimization should not be enabled... right ?

Fedora rawhide uses glibc 2.26 (encoded as 2.25.90).

Thanks,
Florian

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Alan Modra]

On Fri, Jul 28, 2017 at 11:41:49AM +0100, Nick Clifton wrote:
> Hi Alan,
> 
> > So will linking with -Wl,--no-plt-localentry
> 
> Naive question - how does this optimization get enabled ?
> 
> My reading of the code is that ppc64elf.em initialises the plt_localentry0
> field in the params structure to -1, and then elf64-ppc.c:ppc64_elf_tls_setup()
> sets the field to either 1 or 0 depending upon whether the GLIB_2.26 symbol
> exists.

Right.

>  But - Fedora rawhide is using glibc 2.25, so the symbol should not
> exist,

But apparently it does..

> and so the optimization should not be enabled... right ?
> 
> Cheers
>   Nick
> 
> 

-- 
Alan Modra
Australia Development Lab, IBM

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Florian Weimer]

On 07/28/2017 12:07 PM, Alan Modra wrote:
> No, you misunderstood my comment about --no-plt-localentry.  The
> *user* library would need to be built with that option, which isn't a
> good solution.

Could you clarify how this optimization works?  Is the following
description accurate?

At static link time, when processing a relocation to a function, the
static link editor looks at the implementation of the called function
(which can live in a different DSO).  If the current implementation does
not use the TOC register, the caller is adjusted to call an optimized
PLT stub which does not set the TOC.  Otherwise, a full PLT stub which
sets the TOC is used.  If the link edit chose to apply the optimization,
there is no way to go back, and future implementations of the called
function must never require a TOC pointer.  The dynamic linker cannot
recover and thus bails out with the localentry error message.

Thanks,
Florian

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Alan Modra]

On Fri, Jul 28, 2017 at 02:39:13PM +0200, Florian Weimer wrote:
> On 07/28/2017 12:07 PM, Alan Modra wrote:
> > No, you misunderstood my comment about --no-plt-localentry.  The
> > *user* library would need to be built with that option, which isn't a
> > good solution.
> 
> Could you clarify how this optimization works?  Is the following
> description accurate?
> 
> At static link time, when processing a relocation to a function, the
> static link editor looks at the implementation of the called function
> (which can live in a different DSO).  If the current implementation does
> not use the TOC register, the caller is adjusted to call an optimized
> PLT stub which does not set the TOC.  Otherwise, a full PLT stub which
> sets the TOC is used.  If the link edit chose to apply the optimization,
> there is no way to go back, and future implementations of the called
> function must never require a TOC pointer.  The dynamic linker cannot
> recover and thus bails out with the localentry error message.

Yes, that's correct.  If the linker doesn't see a definition, or if
the definition is weak, then the optimization does not apply.

-- 
Alan Modra
Australia Development Lab, IBM

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Andreas Schwab]

On Jul 28 2017, Alan Modra <amodra@gmail.com> wrote:

> On Fri, Jul 28, 2017 at 02:39:13PM +0200, Florian Weimer wrote:
>> At static link time, when processing a relocation to a function, the
>> static link editor looks at the implementation of the called function
>> (which can live in a different DSO).  If the current implementation does
>> not use the TOC register, the caller is adjusted to call an optimized
>> PLT stub which does not set the TOC.  Otherwise, a full PLT stub which
>> sets the TOC is used.  If the link edit chose to apply the optimization,
>> there is no way to go back, and future implementations of the called
>> function must never require a TOC pointer.  The dynamic linker cannot
>> recover and thus bails out with the localentry error message.
>
> Yes, that's correct.  If the linker doesn't see a definition, or if
> the definition is weak, then the optimization does not apply.

That looks like a design bug.  The link-time object will almost never
exactly match the run-time object.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: tst-tlsopt-powerpc fail

[Adam Conrad]

On Fri, Jul 28, 2017 at 06:32:46PM +0930, Alan Modra wrote:
> 
> Since tst-tlsopt-powerpc is supposed to test glibc dynamic relocation
> processing, the body of the test ought to be put in a shared library
> (*).  I cobbled together such a test, and TRY_STATIC_TLS works fine.
> So, not a powerpc64 glibc bug.

Excellent.  Should I expect said cobbled test to replace tst-tlsopt-powerpc
in glibc trunk soonish (obviously, I'll just XFAIL it for now locally).

... Adam

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Florian Weimer]

On 07/28/2017 02:52 PM, Alan Modra wrote:
> On Fri, Jul 28, 2017 at 02:39:13PM +0200, Florian Weimer wrote:
>> On 07/28/2017 12:07 PM, Alan Modra wrote:
>>> No, you misunderstood my comment about --no-plt-localentry.  The
>>> *user* library would need to be built with that option, which isn't a
>>> good solution.
>>
>> Could you clarify how this optimization works?  Is the following
>> description accurate?
>>
>> At static link time, when processing a relocation to a function, the
>> static link editor looks at the implementation of the called function
>> (which can live in a different DSO).  If the current implementation does
>> not use the TOC register, the caller is adjusted to call an optimized
>> PLT stub which does not set the TOC.  Otherwise, a full PLT stub which
>> sets the TOC is used.  If the link edit chose to apply the optimization,
>> there is no way to go back, and future implementations of the called
>> function must never require a TOC pointer.  The dynamic linker cannot
>> recover and thus bails out with the localentry error message.
> 
> Yes, that's correct.  If the linker doesn't see a definition, or if
> the definition is weak, then the optimization does not apply.

Ugh.

This absolutely has to be disabled by default.  No one is prepared for
an ABI change because a global variable access has been added to a
function (or the compiler has decided to inline a function with a global
variable access).

In fact, I'm *extremely* reluctant to ship a distribution which as
support for this in the link editor and dynamic linker, even as an
optional feature, because developers will see this option and use it,
without realizing the full consequences (that they lose ABI forward
compatibility).

The optimization is only possible if it is source-directed, that is, the
compiler can take measures to preserve ABI in the presence of
implementation changes.  This would have to be encoded in the DSO (as a
promise “this function will never use the TOC pointer”), and static link
editor could pick up that flag and skip setting that TOC.

But we can't do that automatically just because the current
implementation has some convenient property.  Nobody knows if future
library upgrades will preserve this property.

Thanks,
Florian

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Alan Modra]

On Fri, Jul 28, 2017 at 03:17:45PM +0200, Florian Weimer wrote:
> This absolutely has to be disabled by default.

I'd already come to that conclusion.  I'll commit the following to
binutils master and 2.29 after some testing.

Subject: [PATCH] PR 21847, Don't default PowerPC64 to --plt-localentry

The big comment in ppc64_elf_tls_setup says why.  I've also added some
code to the bfd linker that catches the -lpthread -lc symbol
differences and disable generation of optimized call stubs even when
--plt-localentry is activated.  Gold doesn't yet have that.

	PR 21847
bfd/
	* elf64-ppc.c (struct ppc_link_hash_entry): Add non_zero_localentry.
	(ppc64_elf_merge_symbol): Set non_zero_localentry.
	(is_elfv2_localentry0): Test non_zero_localentry.
	(ppc64_elf_tls_setup): Default to --no-plt-localentry.
gold/
	* powerpc.cc (Target_powerpc::scan_relocs): Default to
	--no-plt-localentry.
ld/
	* ld.texinfo (plt-localentry): Document.

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 0f71358..41c935d 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,11 @@
+2017-07-29  Alan Modra  <amodra@gmail.com>
+
+	PR 21847
+	* elf64-ppc.c (struct ppc_link_hash_entry): Add non_zero_localentry.
+	(ppc64_elf_merge_symbol): Set non_zero_localentry.
+	(is_elfv2_localentry0): Test non_zero_localentry.
+	(ppc64_elf_tls_setup): Default to --no-plt-localentry.
+
 2017-07-28  Andreas Krebbel  <krebbel@linux.vnet.ibm.com>
 
         * elf32-s390.c (elf_s390_finish_dynamic_sections): Add NULL
diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c
index cc0e8ee..5f3c79f 100644
--- a/bfd/elf64-ppc.c
+++ b/bfd/elf64-ppc.c
@@ -4013,6 +4013,10 @@ struct ppc_link_hash_entry
      with non-standard calling convention.  */
   unsigned int save_res:1;
 
+  /* Set if a duplicate symbol with non-zero localentry is detected,
+     even when the duplicate symbol does not provide a definition.  */
+  unsigned int non_zero_localentry:1;
+
   /* Contexts in which symbol is used in the GOT (or TOC).
      TLS_GD .. TLS_EXPLICIT bits are or'd into the mask as the
      corresponding relocs are encountered during check_relocs.
@@ -5021,7 +5025,7 @@ ppc64_elf_merge_symbol_attribute (struct elf_link_hash_entry *h,
 
 static bfd_boolean
 ppc64_elf_merge_symbol (struct elf_link_hash_entry *h,
-			const Elf_Internal_Sym *isym ATTRIBUTE_UNUSED,
+			const Elf_Internal_Sym *isym,
 			asection **psec ATTRIBUTE_UNUSED,
 			bfd_boolean newdef ATTRIBUTE_UNUSED,
 			bfd_boolean olddef ATTRIBUTE_UNUSED,
@@ -5029,6 +5033,8 @@ ppc64_elf_merge_symbol (struct elf_link_hash_entry *h,
 			const asection *oldsec ATTRIBUTE_UNUSED)
 {
   ((struct ppc_link_hash_entry *) h)->fake = 0;
+  if ((STO_PPC64_LOCAL_MASK & isym->st_other) != 0)
+    ((struct ppc_link_hash_entry *) h)->non_zero_localentry = 1;
   return TRUE;
 }
 
@@ -6335,6 +6341,7 @@ is_elfv2_localentry0 (struct elf_link_hash_entry *h)
 	  && h->type == STT_FUNC
 	  && h->root.type == bfd_link_hash_defined
 	  && (STO_PPC64_LOCAL_MASK & h->other) == 0
+	  && !((struct ppc_link_hash_entry *) h)->non_zero_localentry
 	  && is_ppc64_elf (h->root.u.def.section->owner)
 	  && abiversion (h->root.u.def.section->owner) >= 2);
 }
@@ -8349,10 +8356,22 @@ ppc64_elf_tls_setup (struct bfd_link_info *info)
   else if (!htab->do_multi_toc)
     htab->params->no_multi_toc = 1;
 
+  /* Default to --no-plt-localentry, as this option can cause problems
+     with symbol interposition.  For example, glibc libpthread.so and
+     libc.so duplicate many pthread symbols, with a fallback
+     implementation in libc.so.  In some cases the fallback does more
+     work than the pthread implementation.  __pthread_condattr_destroy
+     is one such symbol: the libpthread.so implementation is
+     localentry:0 while the libc.so implementation is localentry:8.
+     An app that "cleverly" uses dlopen to only load necessary
+     libraries at runtime may omit loading libpthread.so when not
+     running multi-threaded, which then results in the libc.so
+     fallback symbols being used and ld.so complaining.  Now there
+     are workarounds in ld (see non_zero_localentry) to detect the
+     pthread situation, but that may not be the only case where
+     --plt-localentry can cause trouble.  */
   if (htab->params->plt_localentry0 < 0)
-    htab->params->plt_localentry0
-      = elf_link_hash_lookup (&htab->elf, "GLIBC_2.26",
-			      FALSE, FALSE, FALSE) != NULL;
+    htab->params->plt_localentry0 = 0;
 
   htab->tls_get_addr = ((struct ppc_link_hash_entry *)
 			elf_link_hash_lookup (&htab->elf, ".__tls_get_addr",
diff --git a/gold/ChangeLog b/gold/ChangeLog
index fdac931..a019393 100644
--- a/gold/ChangeLog
+++ b/gold/ChangeLog
@@ -1,3 +1,9 @@
+2017-07-29  Alan Modra  <amodra@gmail.com>
+
+	PR 21847
+	* powerpc.cc (Target_powerpc::scan_relocs): Default to
+	--no-plt-localentry.
+
 2017-07-28  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR gold/21857
diff --git a/gold/powerpc.cc b/gold/powerpc.cc
index a4966b8..e322d6f 100644
--- a/gold/powerpc.cc
+++ b/gold/powerpc.cc
@@ -7660,8 +7660,6 @@ Target_powerpc<size, big_endian>::scan_relocs(
 	{
 	  if (parameters->options().user_set_plt_localentry())
 	    plt_localentry0 = parameters->options().plt_localentry();
-	  else
-	    plt_localentry0 = symtab->lookup("GLIBC_2.26", NULL) != NULL;
 	}
       this->plt_localentry0_ = plt_localentry0;
       this->plt_localentry0_init_ = true;
diff --git a/ld/ChangeLog b/ld/ChangeLog
index 9345785..2a371b9 100644
--- a/ld/ChangeLog
+++ b/ld/ChangeLog
@@ -1,3 +1,7 @@
+2017-07-29  Alan Modra  <amodra@gmail.com>
+
+	* ld.texinfo (plt-localentry): Document.
+
 2017-07-28  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* ldgram.y (ldgram_had_keep): Make static.
diff --git a/ld/ld.texinfo b/ld/ld.texinfo
index 987816f..172c1dd 100644
--- a/ld/ld.texinfo
+++ b/ld/ld.texinfo
@@ -7600,6 +7600,21 @@ barrier in the call stub, or use LD_BIND_NOW=1.  By default, @code{ld}
 looks for calls to commonly used functions that create threads, and if
 seen, adds the necessary barriers.  Use these options to change the
 default behaviour.
+
+@cindex PowerPC64 ELFv2 PLT localentry optimization
+@kindex --plt-localentry
+@kindex --no-plt-localentry
+@item --plt-localentry
+@itemx --no-localentry
+ELFv2 functions with localentry:0 are those with a single entry point,
+ie. global entry == local entry, and that have no requirement on r2
+(the TOC/GOT pointer) or r12, and guarantee r2 is unchanged on return.
+Such an external function can be called via the PLT without saving r2
+or restoring it on return, avoiding a common load-hit-store for small
+functions.   The optimization is attractive, with up to 40% reduction
+in execution time for a small function, but can result in symbol
+interposition failures.  Use with care.  @option{--no-plt-localentry}
+is the default.
 @end table
 
 @ifclear GENERIC


-- 
Alan Modra
Australia Development Lab, IBM

[PATCH] tst-tlsopt-powerpc as a shared lib

[Alan Modra]

On Fri, Jul 28, 2017 at 01:07:44PM +0000, Adam Conrad wrote:
> On Fri, Jul 28, 2017 at 06:32:46PM +0930, Alan Modra wrote:
> > 
> > Since tst-tlsopt-powerpc is supposed to test glibc dynamic relocation
> > processing, the body of the test ought to be put in a shared library
> > (*).  I cobbled together such a test, and TRY_STATIC_TLS works fine.
> > So, not a powerpc64 glibc bug.
> 
> Excellent.  Should I expect said cobbled test to replace tst-tlsopt-powerpc
> in glibc trunk soonish (obviously, I'll just XFAIL it for now locally).

This makes the __tls_get_addr_opt test run as a shared library, and so
actually test that DTPMOD64/DTPREL64 pairs are processed by ld.so to
support the __tls_get_adfr_opt call stub fast return.  After a
2017-01-24 patch (binutils f0158f4416) ld.bfd no longer emitted
unnecessary dynamic relocations against local thread variables,
instead setting up the __tls_index GOT entries for the call stub fast
return.  This meant tst-tlsopt-powerpc passed but did not check ld.so
relocation support.  After a 2017-07-16 patch (binutils 676ee2b5fa)
ld.bfd no longer set up the __tls_index GOT entries for the call stub
fast return, and tst-tlsopt-powerpc failed.

Compiling mod-tlsopt-powerpc.c with -DSHARED exposed a bug in
powerpc64/tls-macros.h, which defines a __TLS_GET_ADDR macro that
clashes with one defined in dl-tls.h.  The tls-macros.h version is
only used in that file, so delete it and expand.

Regression tested powerpc64le-linux.  Please verify the Makefile
changes.  The test passes with "make -j64 check", but I may well have
missed something there.

	* sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from
	tst-tlsopt-powerpc.c with function name change and no test harness.
	* sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test.
	Call tls_get_addr_opt_test.
	* sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define.
	(modules-names): Add mod-tlsopt-powerpc.
	(mod-tlsopt-powerpc.so-no-z-defs): Define.
	(tst-tlsopt-powerpc): Depend on .so.
	* sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't
	define.  Expand use in TLS_GD and TLS_LD.

diff --git a/sysdeps/powerpc/Makefile b/sysdeps/powerpc/Makefile
index 0d9206b..6aa683b 100644
--- a/sysdeps/powerpc/Makefile
+++ b/sysdeps/powerpc/Makefile
@@ -8,9 +8,11 @@ sysdep-dl-routines += dl-machine hwcapinfo
 sysdep_routines += dl-machine hwcapinfo
 # extra shared linker files to link only into dl-allobjs.so
 sysdep-rtld-routines += dl-machine hwcapinfo
-# Don't optimize GD tls sequence to LE.
-LDFLAGS-tst-tlsopt-powerpc += -Wl,--no-tls-optimize
+
+modules-names += mod-tlsopt-powerpc
+mod-tlsopt-powerpc.so-no-z-defs = yes
 tests += tst-tlsopt-powerpc
+$(objpfx)tst-tlsopt-powerpc: $(objpfx)mod-tlsopt-powerpc.so
 
 ifneq (no,$(multi-arch))
 tests-static += tst-tlsifunc-static
diff --git a/sysdeps/powerpc/mod-tlsopt-powerpc.c b/sysdeps/powerpc/mod-tlsopt-powerpc.c
new file mode 100644
index 0000000..ee0db12
--- /dev/null
+++ b/sysdeps/powerpc/mod-tlsopt-powerpc.c
@@ -0,0 +1,49 @@
+/* shared library to test for __tls_get_addr optimization.  */
+#include <stdio.h>
+
+#include "../../elf/tls-macros.h"
+#include "dl-tls.h"
+
+/* common 'int' variable in TLS.  */
+COMMON_INT_DEF(foo);
+
+
+int
+tls_get_addr_opt_test (void)
+{
+  int result = 0;
+
+  /* Get variable using general dynamic model.  */
+  int *ap = TLS_GD (foo);
+  if (*ap != 0)
+    {
+      printf ("foo = %d\n", *ap);
+      result = 1;
+    }
+
+  tls_index *tls_arg;
+#ifdef __powerpc64__
+  register unsigned long thread_pointer __asm__ ("r13");
+  asm ("addi %0,2,foo@got@tlsgd" : "=r" (tls_arg));
+#else
+  register unsigned long thread_pointer __asm__ ("r2");
+  asm ("bcl 20,31,1f\n1:\t"
+       "mflr %0\n\t"
+       "addis %0,%0,_GLOBAL_OFFSET_TABLE_-1b@ha\n\t"
+       "addi %0,%0,_GLOBAL_OFFSET_TABLE_-1b@l\n\t"
+       "addi %0,%0,foo@got@tlsgd" : "=b" (tls_arg));
+#endif
+
+  if (tls_arg->ti_module != 0)
+    {
+      printf ("tls_index not optimized, binutils too old?\n");
+      result = 1;
+    }
+  else if (tls_arg->ti_offset + thread_pointer != (unsigned long) ap)
+    {
+      printf ("tls_index->ti_offset wrong value\n");
+      result = 1;
+    }
+
+  return result;
+}
diff --git a/sysdeps/powerpc/powerpc64/tls-macros.h b/sysdeps/powerpc/powerpc64/tls-macros.h
index 42a95ec..79a0b25 100644
--- a/sysdeps/powerpc/powerpc64/tls-macros.h
+++ b/sysdeps/powerpc/powerpc64/tls-macros.h
@@ -18,13 +18,11 @@
      __result;								      \
   })
 
-#define __TLS_GET_ADDR "__tls_get_addr"
-
 /* PowerPC64 Local Dynamic TLS access.  */
 #define TLS_LD(x)							      \
   ({ int * __result;							      \
      asm ("addi  3,2," #x "@got@tlsld\n\t"				      \
-	  "bl    " __TLS_GET_ADDR "\n\t"				      \
+	  "bl    __tls_get_addr\n\t"					      \
 	  "nop   \n\t"							      \
 	  "addis %0,3," #x "@dtprel@ha\n\t"				      \
 	  "addi  %0,%0," #x "@dtprel@l"					      \
@@ -36,7 +34,7 @@
 #define TLS_GD(x)							      \
   ({ register int *__result __asm__ ("r3");				      \
      asm ("addi  3,2," #x "@got@tlsgd\n\t"				      \
-	  "bl    " __TLS_GET_ADDR "\n\t"				      \
+	  "bl    __tls_get_addr\n\t"					      \
 	  "nop   "							      \
 	  : "=r" (__result) :						      \
 	  : __TLS_CALL_CLOBBERS);					      \
diff --git a/sysdeps/powerpc/tst-tlsopt-powerpc.c b/sysdeps/powerpc/tst-tlsopt-powerpc.c
index 8ae928a..cc682b2 100644
--- a/sysdeps/powerpc/tst-tlsopt-powerpc.c
+++ b/sysdeps/powerpc/tst-tlsopt-powerpc.c
@@ -1,51 +1,11 @@
 /* glibc test for __tls_get_addr optimization.  */
-#include <stdio.h>
-
-#include "../../elf/tls-macros.h"
-#include "dl-tls.h"
-
-/* common 'int' variable in TLS.  */
-COMMON_INT_DEF(foo);
-
 
 static int
 do_test (void)
 {
-  int result = 0;
-
-  /* Get variable using general dynamic model.  */
-  int *ap = TLS_GD (foo);
-  if (*ap != 0)
-    {
-      printf ("foo = %d\n", *ap);
-      result = 1;
-    }
-
-  tls_index *tls_arg;
-#ifdef __powerpc64__
-  register unsigned long thread_pointer __asm__ ("r13");
-  asm ("addi %0,2,foo@got@tlsgd" : "=r" (tls_arg));
-#else
-  register unsigned long thread_pointer __asm__ ("r2");
-  asm ("bcl 20,31,1f\n1:\t"
-       "mflr %0\n\t"
-       "addis %0,%0,_GLOBAL_OFFSET_TABLE_-1b@ha\n\t"
-       "addi %0,%0,_GLOBAL_OFFSET_TABLE_-1b@l\n\t"
-       "addi %0,%0,foo@got@tlsgd" : "=b" (tls_arg));
-#endif
-
-  if (tls_arg->ti_module != 0)
-    {
-      printf ("tls_index not optimized, binutils too old?\n");
-      result = 1;
-    }
-  else if (tls_arg->ti_offset + thread_pointer != (unsigned long) ap)
-    {
-      printf ("tls_index->ti_offset wrong value\n");
-      result = 1;
-    }
+  extern int tls_get_addr_opt_test (void);
 
-  return result;
+  return tls_get_addr_opt_test ();
 }
 
 #include <support/test-driver.c>


-- 
Alan Modra
Australia Development Lab, IBM

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Florian Weimer]

On 07/29/2017 06:44 AM, Alan Modra wrote:
> +@cindex PowerPC64 ELFv2 PLT localentry optimization
> +@kindex --plt-localentry
> +@kindex --no-plt-localentry
> +@item --plt-localentry
> +@itemx --no-localentry
> +ELFv2 functions with localentry:0 are those with a single entry point,
> +ie. global entry == local entry, and that have no requirement on r2
> +(the TOC/GOT pointer) or r12, and guarantee r2 is unchanged on return.
> +Such an external function can be called via the PLT without saving r2
> +or restoring it on return, avoiding a common load-hit-store for small
> +functions.   The optimization is attractive, with up to 40% reduction
> +in execution time for a small function, but can result in symbol
> +interposition failures.  Use with care.  @option{--no-plt-localentry}
> +is the default.

I think this is still very misleading.

The documentation should make the following things clear:

* If objects are linked with --plt-localentry, it is likely that at run
time, the exact same version of the object will be required by the
dynamic linker.  Even supposedly ABI-compatible library updates may not
work.

* Mere recompilation of shared object dependencies can prevent loading
of objects linked with --plt-localentry even if the sources are
unchanged.  Compiler flag changes (such as optimization levels) and
compiler version changes affect compatibility.

* There is no promise  that applications which link to the core GNU
toolchain libraries (libgcc_s, glibc, libstdc++) will continue to load
after any change to these libraries, even though these libraries
generally maintain ABI backward compatibility.

The point about symbol interposition is valid, but it is really not the
core issue here.

The source code comment should be adjusted in a similar way.

Thanks,
Florian

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Andreas Schwab]

On Jul 30 2017, Florian Weimer <fweimer@redhat.com> wrote:

> * If objects are linked with --plt-localentry, it is likely that at run
> time, the exact same version of the object will be required by the
> dynamic linker.  Even supposedly ABI-compatible library updates may not
> work.

Or, in other words, --plt-localentry changes the ABI.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Florian Weimer]

On 07/30/2017 12:22 PM, Andreas Schwab wrote:
> On Jul 30 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> * If objects are linked with --plt-localentry, it is likely that at run
>> time, the exact same version of the object will be required by the
>> dynamic linker.  Even supposedly ABI-compatible library updates may not
>> work.
> 
> Or, in other words, --plt-localentry changes the ABI.

It's worse than that: It changes what is part of the ABI.  It makes ABI
dependencies more strict, i.e., changes which were ABI-compatible before
no longer are.  And what I think it is important to convey is that none
of the GNU toolchain libraries (and other system libraries, presumably)
are prepared to preserve ABI compatibility according to this new
definition of what is part of the ABI of a shared object.

Thanks,
Florian

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Alan Modra]

This makes ld warn about --plt-localentry if a version of glibc
without the necessary ld.so checks is detected, and revises the
documentation.

bfd/
	* elf64-ppc.c (ppc64_elf_tls_setup): Warn on --plt-localentry
	without ld.so checks.
gold/
	* powerpc.cc (Target_powerpc::scan_relocs): Warn on --plt-localentry
	without ld.so checks.
ld/
	* ld.texinfo (plt-localentry): Revise.

diff --git a/bfd/elf64-ppc.c b/bfd/elf64-ppc.c
index 5f3c79f..7f4f7b6 100644
--- a/bfd/elf64-ppc.c
+++ b/bfd/elf64-ppc.c
@@ -8372,6 +8372,12 @@ ppc64_elf_tls_setup (struct bfd_link_info *info)
      --plt-localentry can cause trouble.  */
   if (htab->params->plt_localentry0 < 0)
     htab->params->plt_localentry0 = 0;
+  if (htab->params->plt_localentry0
+      && elf_link_hash_lookup (&htab->elf, "GLIBC_2.26",
+			       FALSE, FALSE, FALSE) == NULL)
+    info->callbacks->einfo
+      (_("%P: warning: --plt-localentry is especially dangerous without "
+	 "ld.so support to detect ABI violations.\n"));
 
   htab->tls_get_addr = ((struct ppc_link_hash_entry *)
 			elf_link_hash_lookup (&htab->elf, ".__tls_get_addr",
diff --git a/gold/powerpc.cc b/gold/powerpc.cc
index e322d6f..14e56d8 100644
--- a/gold/powerpc.cc
+++ b/gold/powerpc.cc
@@ -7660,6 +7660,10 @@ Target_powerpc<size, big_endian>::scan_relocs(
 	{
 	  if (parameters->options().user_set_plt_localentry())
 	    plt_localentry0 = parameters->options().plt_localentry();
+	  if (plt_localentry0
+	      && symtab->lookup("GLIBC_2.26", NULL) == NULL)
+	    gold_warning(_("--plt-localentry is especially dangerous without "
+			   "ld.so support to detect ABI violations"));
 	}
       this->plt_localentry0_ = plt_localentry0;
       this->plt_localentry0_init_ = true;
diff --git a/ld/ld.texinfo b/ld/ld.texinfo
index 172c1dd..ebe7e7b 100644
--- a/ld/ld.texinfo
+++ b/ld/ld.texinfo
@@ -7613,8 +7613,11 @@ Such an external function can be called via the PLT without saving r2
 or restoring it on return, avoiding a common load-hit-store for small
 functions.   The optimization is attractive, with up to 40% reduction
 in execution time for a small function, but can result in symbol
-interposition failures.  Use with care.  @option{--no-plt-localentry}
-is the default.
+interposition failures.  Also, minor changes in a shared library,
+including system libraries, can cause a function that was localentry:0
+to become localentry:8.  This will result in a dynamic loader
+complaint and failure to run.  The option is experimental, use with
+care.  @option{--no-plt-localentry} is the default.
 @end table
 
 @ifclear GENERIC

-- 
Alan Modra
Australia Development Lab, IBM

Re: ppc64le: expected localentry:0 `pthread_condattr_destroy'

[Florian Weimer]

On 07/31/2017 05:50 AM, Alan Modra wrote:
> diff --git a/ld/ld.texinfo b/ld/ld.texinfo
> index 172c1dd..ebe7e7b 100644
> --- a/ld/ld.texinfo
> +++ b/ld/ld.texinfo
> @@ -7613,8 +7613,11 @@ Such an external function can be called via the PLT without saving r2
>  or restoring it on return, avoiding a common load-hit-store for small
>  functions.   The optimization is attractive, with up to 40% reduction
>  in execution time for a small function, but can result in symbol
> -interposition failures.  Use with care.  @option{--no-plt-localentry}
> -is the default.
> +interposition failures.  Also, minor changes in a shared library,
> +including system libraries, can cause a function that was localentry:0
> +to become localentry:8.  This will result in a dynamic loader
> +complaint and failure to run.  The option is experimental, use with
> +care.  @option{--no-plt-localentry} is the default.
>  @end table

The wording looks okay to me.

Thanks,
Florian

Re: [PATCH] tst-tlsopt-powerpc as a shared lib

[Tulio Magno Quites Machado Filho]

Alan Modra <amodra@gmail.com> writes:

> On Fri, Jul 28, 2017 at 01:07:44PM +0000, Adam Conrad wrote:
>> On Fri, Jul 28, 2017 at 06:32:46PM +0930, Alan Modra wrote:
>> > 
>> > Since tst-tlsopt-powerpc is supposed to test glibc dynamic relocation
>> > processing, the body of the test ought to be put in a shared library
>> > (*).  I cobbled together such a test, and TRY_STATIC_TLS works fine.
>> > So, not a powerpc64 glibc bug.
>> 
>> Excellent.  Should I expect said cobbled test to replace tst-tlsopt-powerpc
>> in glibc trunk soonish (obviously, I'll just XFAIL it for now locally).
>
> This makes the __tls_get_addr_opt test run as a shared library, and so
> actually test that DTPMOD64/DTPREL64 pairs are processed by ld.so to
> support the __tls_get_adfr_opt call stub fast return.  After a
> 2017-01-24 patch (binutils f0158f4416) ld.bfd no longer emitted
> unnecessary dynamic relocations against local thread variables,
> instead setting up the __tls_index GOT entries for the call stub fast
> return.  This meant tst-tlsopt-powerpc passed but did not check ld.so
> relocation support.  After a 2017-07-16 patch (binutils 676ee2b5fa)
> ld.bfd no longer set up the __tls_index GOT entries for the call stub
> fast return, and tst-tlsopt-powerpc failed.
>
> Compiling mod-tlsopt-powerpc.c with -DSHARED exposed a bug in
> powerpc64/tls-macros.h, which defines a __TLS_GET_ADDR macro that
> clashes with one defined in dl-tls.h.  The tls-macros.h version is
> only used in that file, so delete it and expand.
>
> Regression tested powerpc64le-linux.  Please verify the Makefile
> changes.  The test passes with "make -j64 check", but I may well have
> missed something there.

Tested on powerpc-linux and powerpc64-linux too.

> 	* sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from
> 	tst-tlsopt-powerpc.c with function name change and no test harness.
> 	* sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test.
> 	Call tls_get_addr_opt_test.
> 	* sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define.
> 	(modules-names): Add mod-tlsopt-powerpc.
> 	(mod-tlsopt-powerpc.so-no-z-defs): Define.
> 	(tst-tlsopt-powerpc): Depend on .so.
> 	* sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't
> 	define.  Expand use in TLS_GD and TLS_LD.

Even if this patch doesn't fix bug 21847 [1], I think it makes sense to refer
it here and close the bug, which was reported to glibc.

Looks good to me.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=21847

-- 
Tulio Magno

Re: [PATCH] tst-tlsopt-powerpc as a shared lib

[Alan Modra]

[cc trimmed]
On Mon, Jul 31, 2017 at 01:40:48PM -0300, Tulio Magno Quites Machado Filho wrote:
> Alan Modra <amodra@gmail.com> writes:
> 
> > On Fri, Jul 28, 2017 at 01:07:44PM +0000, Adam Conrad wrote:
> >> On Fri, Jul 28, 2017 at 06:32:46PM +0930, Alan Modra wrote:
> >> > 
> >> > Since tst-tlsopt-powerpc is supposed to test glibc dynamic relocation
> >> > processing, the body of the test ought to be put in a shared library
> >> > (*).  I cobbled together such a test, and TRY_STATIC_TLS works fine.
> >> > So, not a powerpc64 glibc bug.
> >> 
> >> Excellent.  Should I expect said cobbled test to replace tst-tlsopt-powerpc
> >> in glibc trunk soonish (obviously, I'll just XFAIL it for now locally).
> >
> > This makes the __tls_get_addr_opt test run as a shared library, and so
> > actually test that DTPMOD64/DTPREL64 pairs are processed by ld.so to
> > support the __tls_get_adfr_opt call stub fast return.  After a
> > 2017-01-24 patch (binutils f0158f4416) ld.bfd no longer emitted
> > unnecessary dynamic relocations against local thread variables,
> > instead setting up the __tls_index GOT entries for the call stub fast
> > return.  This meant tst-tlsopt-powerpc passed but did not check ld.so
> > relocation support.  After a 2017-07-16 patch (binutils 676ee2b5fa)
> > ld.bfd no longer set up the __tls_index GOT entries for the call stub
> > fast return, and tst-tlsopt-powerpc failed.
> >
> > Compiling mod-tlsopt-powerpc.c with -DSHARED exposed a bug in
> > powerpc64/tls-macros.h, which defines a __TLS_GET_ADDR macro that
> > clashes with one defined in dl-tls.h.  The tls-macros.h version is
> > only used in that file, so delete it and expand.
> >
> > Regression tested powerpc64le-linux.  Please verify the Makefile
> > changes.  The test passes with "make -j64 check", but I may well have
> > missed something there.
> 
> Tested on powerpc-linux and powerpc64-linux too.

Thanks!  I'll commit this after the 2.26 freeze is over.

> > 	* sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from
> > 	tst-tlsopt-powerpc.c with function name change and no test harness.
> > 	* sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test.
> > 	Call tls_get_addr_opt_test.
> > 	* sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define.
> > 	(modules-names): Add mod-tlsopt-powerpc.
> > 	(mod-tlsopt-powerpc.so-no-z-defs): Define.
> > 	(tst-tlsopt-powerpc): Depend on .so.
> > 	* sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't
> > 	define.  Expand use in TLS_GD and TLS_LD.
> 
> Even if this patch doesn't fix bug 21847 [1], I think it makes sense to refer
> it here and close the bug, which was reported to glibc.
> 
> Looks good to me.
> 
> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=21847

No, 21847 is an entirely different issue.  I've moved it from
glibc/dynamic-linker to binutils/ld and mentioned the commits on the
binutils side that have already fixed the bug.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH] tst-tlsopt-powerpc as a shared lib

[Adam Conrad]

On Mon, Jul 31, 2017 at 01:40:48PM -0300, Tulio Magno Quites Machado Filho wrote:
> 
> Even if this patch doesn't fix bug 21847 [1], I think it makes sense to refer
> it here and close the bug, which was reported to glibc.

That's a complete unrelated bug, which is addressed here:

https://sourceware.org/ml/binutils/2017-07/msg00342.html

... Adam

Re: [PATCH] tst-tlsopt-powerpc as a shared lib

[Florian Weimer]

On 07/30/2017 02:04 AM, Alan Modra wrote:
> 	* sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from
> 	tst-tlsopt-powerpc.c with function name change and no test harness.
> 	* sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test.
> 	Call tls_get_addr_opt_test.
> 	* sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define.
> 	(modules-names): Add mod-tlsopt-powerpc.
> 	(mod-tlsopt-powerpc.so-no-z-defs): Define.
> 	(tst-tlsopt-powerpc): Depend on .so.
> 	* sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't
> 	define.  Expand use in TLS_GD and TLS_LD.

Should we backport this to the active release branches?

Thanks,
Florian

Re: [PATCH] tst-tlsopt-powerpc as a shared lib

[Carlos O'Donell]

On 09/04/2017 04:39 PM, Florian Weimer wrote:
> On 07/30/2017 02:04 AM, Alan Modra wrote:
>> 	* sysdeps/powerpc/mod-tlsopt-powerpc.c: Extract from
>> 	tst-tlsopt-powerpc.c with function name change and no test harness.
>> 	* sysdeps/powerpc/tst-tlsopt-powerpc.c: Remove body of test.
>> 	Call tls_get_addr_opt_test.
>> 	* sysdeps/powerpc/Makefile (LDFLAGS-tst-tlsopt-powerpc): Don't define.
>> 	(modules-names): Add mod-tlsopt-powerpc.
>> 	(mod-tlsopt-powerpc.so-no-z-defs): Define.
>> 	(tst-tlsopt-powerpc): Depend on .so.
>> 	* sysdeps/powerpc/powerpc64/tls-macros.h (__TLS_GET_ADDR): Don't
>> 	define.  Expand use in TLS_GD and TLS_LD.
> 
> Should we backport this to the active release branches?

Yes. Test fixes that make the active branches easier to maintain for the
distributions are always a good idea.

-- 
Cheers,
Carlos.