[PATCH 1/1] gold: Avoid duplicate PLT stub symbols on PowerPC

[PATCH 1/1] gold: Avoid duplicate PLT stub symbols on PowerPC

[James Clarke]

If two objects are compiled with -fPIC or -fPIE and call the same
function, two different PLT entries are created, one for each object,
but the same stub symbol name is used for both. Therefore let's make
the names unique by incorporating the object's uniq_ value where
necessary.

gold/
	* (Stub_table::define_stub_syms): Include object's uniq_ value
	when set for non-local symbols as well.
---
 gold/powerpc.cc | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/gold/powerpc.cc b/gold/powerpc.cc
index 1f2bc9ede7..a448efbc94 100644
--- a/gold/powerpc.cc
+++ b/gold/powerpc.cc
@@ -4656,19 +4656,25 @@ Stub_table<size, big_endian>::define_stub_syms(Symbol_table* symtab)
 	  add[0] = 0;
 	  if (cs->first.addend_ != 0)
 	    sprintf(add, "+%x", static_cast<uint32_t>(cs->first.addend_));
-	  char localname[18];
-	  const char *symname;
-	  if (cs->first.sym_ == NULL)
+	  char obj[10];
+	  obj[0] = 0;
+	  if (cs->first.object_)
 	    {
 	      const Powerpc_relobj<size, big_endian>* ppcobj = static_cast
 		<const Powerpc_relobj<size, big_endian>*>(cs->first.object_);
-	      sprintf(localname, "%x:%x", ppcobj->uniq(), cs->first.locsym_);
+	      sprintf(obj, "%x:", ppcobj->uniq());
+	    }
+	  char localname[9];
+	  const char *symname;
+	  if (cs->first.sym_ == NULL)
+	    {
+	      sprintf(localname, "%x", cs->first.locsym_);
 	      symname = localname;
 	    }
 	  else
 	    symname = cs->first.sym_->name();
-	  char* name = new char[8 + 10 + strlen(symname) + strlen(add) + 1];
-	  sprintf(name, "%08x.plt_call.%s%s", this->uniq_, symname, add);
+	  char* name = new char[8 + 10 + strlen(obj) + strlen(symname) + strlen(add) + 1];
+	  sprintf(name, "%08x.plt_call.%s%s%s", this->uniq_, obj, symname, add);
 	  Address value = this->stub_address() - this->address() + cs->second;
 	  unsigned int stub_size = this->plt_call_size(cs);
 	  this->targ_->define_local(symtab, name, this, value, stub_size);
-- 
2.13.1

[PATCH 0/1] gold: Avoid duplicate PLT stub symbols on PowerPC

[James Clarke]

If two objects are compiled with -fPIC or -fPIE and call the same
function, two different PLT stubs are created, one for each object, but
the same stub symbol name is used for both. For example:

> (sid_powerpc-dchroot)jrtc27@redpanda:~$ cat test.c
> #include <stdio.h>
>
> extern void f(void);
>
> int main(int argc, char **argv) {
>         printf("Main\n");
>         f();
>         return 0;
> }
> (sid_powerpc-dchroot)jrtc27@redpanda:~$ cat f.c
> #include <stdio.h>
>
> void f(void) {
>         printf("f\n");
> }
> (sid_powerpc-dchroot)jrtc27@redpanda:~$ gcc -pie -fPIE -fuse-ld=gold -o test test.c f.c
> /usr/bin/ld.gold: error: invalid STB_LOCAL symbol in external symbols
> /usr/bin/ld.gold: error: linker defined: multiple definition of '00000001.plt_call.puts+8000'
> /usr/bin/ld.gold: command line: previous definition here
> collect2: error: ld returned 1 exit status

It seems BFD just ignores the issue, and will only add the symbol the
first time. I first wrote a patch to do that, but it struck me that this
might be nicer, as from what I can tell there is no guarantee that the
PLT stubs for the same symbol are next to each other so you cannot
easily tell the symbol for any of the subsequent PLT stubs. If you would
rather stick to mirroring BFD in this respect (even though the names
themselves are already different), however, I can submit that instead.

James Clarke (1):
  gold: Avoid duplicate PLT stub symbols on PowerPC

 gold/powerpc.cc | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

--
2.13.1

Re: [PATCH 1/1] gold: Avoid duplicate PLT stub symbols on PowerPC

[Alan Modra]

On Sun, Jun 18, 2017 at 10:58:03PM +0100, James Clarke wrote:
> If two objects are compiled with -fPIC or -fPIE and call the same
> function, two different PLT entries are created, one for each object,

I'd be surprised if that was so when using your simple testcase, and
testing here verifies it.  There is a situation where this can occur:
In a very large binary it may be necessary to create multiple stubs to
call a given function, because calls are so far apart that a single
stub cannot be reached by a "bl" instruction.  However, that case
ought to be covered by this->uniq_ being different for each
stub_table.  So I'm not sure how you are getting the failure.

> but the same stub symbol name is used for both. Therefore let's make
> the names unique by incorporating the object's uniq_ value where
> necessary.
> 
> gold/
> 	* (Stub_table::define_stub_syms): Include object's uniq_ value
> 	when set for non-local symbols as well.

This won't work.  object_ isn't set for ppc64 globals.  See the
Plt_stub_ent constructors.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 1/1] gold: Avoid duplicate PLT stub symbols on PowerPC

[Alan Modra]

On Mon, Jun 19, 2017 at 12:34:42PM +0930, Alan Modra wrote:
> On Sun, Jun 18, 2017 at 10:58:03PM +0100, James Clarke wrote:
> > If two objects are compiled with -fPIC or -fPIE and call the same
> > function, two different PLT entries are created, one for each object,
> 
> I'd be surprised if that was so when using your simple testcase, and
> testing here verifies it.  There is a situation where this can occur:
> In a very large binary it may be necessary to create multiple stubs to
> call a given function, because calls are so far apart that a single
> stub cannot be reached by a "bl" instruction.  However, that case
> ought to be covered by this->uniq_ being different for each
> stub_table.  So I'm not sure how you are getting the failure.

Incidentally, you can provoke multiple stubs with the small testcase
by using a ridiculously small --stub-group-size=64 (and ignore all the
warnings about section "exceeds group size".  If I do that, I see

nm printf | grep puts
0000000000000840 t 00000004.plt_call.puts
00000000000008a0 t 00000005.plt_call.puts
                 U puts

objdump also shows the two plt call stubs for puts.

> > but the same stub symbol name is used for both. Therefore let's make
> > the names unique by incorporating the object's uniq_ value where
> > necessary.
> > 
> > gold/
> > 	* (Stub_table::define_stub_syms): Include object's uniq_ value
> > 	when set for non-local symbols as well.
> 
> This won't work.  object_ isn't set for ppc64 globals.  See the
> Plt_stub_ent constructors.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 1/1] gold: Avoid duplicate PLT stub symbols on PowerPC

[James Clarke]

On 19 Jun 2017, at 04:13, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Jun 19, 2017 at 12:34:42PM +0930, Alan Modra wrote:
>> On Sun, Jun 18, 2017 at 10:58:03PM +0100, James Clarke wrote:
>>> If two objects are compiled with -fPIC or -fPIE and call the same
>>> function, two different PLT entries are created, one for each object,
>> 
>> I'd be surprised if that was so when using your simple testcase, and
>> testing here verifies it.  There is a situation where this can occur:
>> In a very large binary it may be necessary to create multiple stubs to
>> call a given function, because calls are so far apart that a single
>> stub cannot be reached by a "bl" instruction.  However, that case
>> ought to be covered by this->uniq_ being different for each
>> stub_table.  So I'm not sure how you are getting the failure.
> 
> Incidentally, you can provoke multiple stubs with the small testcase
> by using a ridiculously small --stub-group-size=64 (and ignore all the
> warnings about section "exceeds group size".  If I do that, I see
> 
> nm printf | grep puts
> 0000000000000840 t 00000004.plt_call.puts
> 00000000000008a0 t 00000005.plt_call.puts
>                 U puts
> 
> objdump also shows the two plt call stubs for puts.
> 
>>> but the same stub symbol name is used for both. Therefore let's make
>>> the names unique by incorporating the object's uniq_ value where
>>> necessary.
>>> 
>>> gold/
>>> 	* (Stub_table::define_stub_syms): Include object's uniq_ value
>>> 	when set for non-local symbols as well.
>> 
>> This won't work.  object_ isn't set for ppc64 globals.  See the
>> Plt_stub_ent constructors.

It's because, with -fPIC/-fPIE, one GOT is used per object file (.got2), so
each object file needs its own PLT stubs referring to the right GOT, which is
indicated by the addend being the magic 0x8000. You can see in one of the
Plt_stub_ent constructors that the object_ field is non-NULL for globals only
in this case. This means the same stub table ends up with multiple stub entries
for a given global function. This is only an issue on 32-bit PowerPC as 64-bit
PowerPC doesn't have this weirdness, and your multiple stub tables case *is*
covered correctly as you saw yourself.

Regards,
James

Re: [PATCH 1/1] gold: Avoid duplicate PLT stub symbols on PowerPC

[Alan Modra]

On Mon, Jun 19, 2017 at 12:08:24PM +0100, James Clarke wrote:
> This is only an issue on 32-bit PowerPC as 64-bit
> PowerPC doesn't have this weirdness, and your multiple stub tables case *is*
> covered correctly as you saw yourself.

Oh..  I was thinking you'd said powerpc64, but the problem is on
ppc32.  The patch is good.  Please apply.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [PATCH 1/1] gold: Avoid duplicate PLT stub symbols on PowerPC

[James Clarke]

On 20 Jun 2017, at 01:28, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Jun 19, 2017 at 12:08:24PM +0100, James Clarke wrote:
>> This is only an issue on 32-bit PowerPC as 64-bit
>> PowerPC doesn't have this weirdness, and your multiple stub tables case *is*
>> covered correctly as you saw yourself.
> 
> Oh..  I was thinking you'd said powerpc64, but the problem is on
> ppc32.  The patch is good.  Please apply.

Ah, I never explicitly mentioned 32-bit in my original message, and given that
the symbol stubs were added in the context of ppc64 I can see how this
confusion arose. I don't have commit access; please apply on my behalf
(including to the 2.28 branch).

Thanks,
James