[SH] TLS IE -> LE optimization issue

[SH] TLS IE -> LE optimization issue

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 3546 bytes --]

Hi!

I saw the GDB testcase gdb.threads/tls.exp failing to build due to the
following assertion at the end of sh_elf_finish_dynamic_sections:

    7395      if (htab->srelgot)
    7396        BFD_ASSERT (htab->srelgot->reloc_count * sizeof (Elf32_External_Rela)
    7397                    == htab->srelgot->size);

    (gdb) print htab->srelgot->reloc_count
    $2 = 0
    (gdb) print htab->srelgot->size
    $3 = 12

I distilled this C++/pthread test case into the following minimal
arrangement:

    $ cat < tls.c
    int
    _start (void)
    {
      extern __thread int x;
      return x;
    }
    $ cat < tls2.c
    __thread int x;
    $ $ROOT/install/bin/*-gcc -O2 -c tls.c tls2.c
    $ $ROOT/install/bin/*-gcc -O2 -o null.o -c -x c /dev/null
    $ $ROOT/install/bin/*-ld -o tls3.so -shared null.o
    $ $ROOT/install/bin/*-ld -o tls tls.o tls2.o tls3.so
    [...]/sh-linux-gnu-ld: BFD (GNU Binutils) 2.22.52.20120315 assertion fail [...]/bfd/elf32-sh.c:7397

That is, we need a __thread variable that is referenced from another
compilation unit; these two are linked into an executable; and we need a
(dummy) SO in order to trigger creation of the .dynamic section, etc.
What happens is the following.  GCC will emit x as TLS IE.  Called from
sh_elf_size_dynamic_sections, in allocate_dynrelocs the the linker will
therefore make a reservation in line 3111:

      3098        dyn = htab->root.dynamic_sections_created;
      3099        if (!dyn)
      3100          {
      3101            /* No dynamic relocations required.  */
      3102            if (htab->fdpic_p && !info->shared
      3103                && h->root.type != bfd_link_hash_undefweak
      3104                && (got_type == GOT_NORMAL || got_type == GOT_FUNCDESC))
      3105              htab->srofixup->size += 4;
      3106          }
      3107        /* R_SH_TLS_IE_32 needs one dynamic relocation if dynamic,
      3108           R_SH_TLS_GD needs one if local symbol and two if global.  */
      3109        else if ((got_type == GOT_TLS_GD && h->dynindx == -1)
      3110                 || got_type == GOT_TLS_IE)
      3111          htab->srelgot->size += sizeof (Elf32_External_Rela);

(This is also where the (dummy) SO comes into play: otherwise »dyn ==
NULL«.)  Back in sh_elf_size_dynamic_sections, »srelgot->size != 0«
(»sizeof (Elf32_External_Rela) == 12«), thus memory for this section will
be allocated (using bfd_zalloc).  Later on, in sh_elf_relocate_section,
the linker recognizes that TLS IE can here be optimized into TLS LE, and
does so; the relocation slot is now not needed anymore (so
srelgot->reloc_count is not incremented), but it is not reclaimed/the
size reservation remains (and due to using zalloc, it's a R_SH_NONE), and
thus the assertion is triggered.

Expectedly, weakening the assertion into using <= instead of == makes the
problem go away, but the empty slot in .rela.dyn remains (12 bytes
wasted).

Or, instead, should the srelgot->size reservations be un-done as the TLS
optimizations are done (there may be other such cases, I didn't check)?
Can this actually be done at this stage?

Or, should there be another cleanup pass after the TLS optimizations have
been done?  Or, should one of the existing passes in fact be catching
this case, too?

Due to my lack of experience with BFD'S innards combined with its rather
monstrous appearance, I can't really tell which is preferable.  But I'm
willing to learn.


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [SH] TLS IE -> LE optimization issue

[David Miller]

From: Thomas Schwinge <thomas@codesourcery.com>
Date: Fri, 13 Apr 2012 17:48:06 +0200

> (This is also where the (dummy) SO comes into play: otherwise »dyn ==
> NULL«.)  Back in sh_elf_size_dynamic_sections, »srelgot->size != 0«
> (»sizeof (Elf32_External_Rela) == 12«), thus memory for this section will
> be allocated (using bfd_zalloc).  Later on, in sh_elf_relocate_section,
> the linker recognizes that TLS IE can here be optimized into TLS LE, and
> does so; the relocation slot is now not needed anymore (so
> srelgot->reloc_count is not incremented), but it is not reclaimed/the
> size reservation remains (and due to using zalloc, it's a R_SH_NONE), and
> thus the assertion is triggered.
> 
> Expectedly, weakening the assertion into using <= instead of == makes the
> problem go away, but the empty slot in .rela.dyn remains (12 bytes
> wasted).
> 
> Or, instead, should the srelgot->size reservations be un-done as the TLS
> optimizations are done (there may be other such cases, I didn't check)?
> Can this actually be done at this stage?
> 
> Or, should there be another cleanup pass after the TLS optimizations have
> been done?  Or, should one of the existing passes in fact be catching
> this case, too?

Sparc, which makes similar transformations, lacks the assertion you
mention altogether.

In fact SH is one of the few backends I see making this size check.

Re: [SH] TLS IE -> LE optimization issue

[Kaz Kojima]

Thomas Schwinge <thomas@codesourcery.com> wrote:
> (This is also where the (dummy) SO comes into play: otherwise ^[,A;^[(Bdyn ==
> NULL^[,A+^[(B.)  Back in sh_elf_size_dynamic_sections, ^[,A;^[(Bsrelgot->size != 0^[,A+^[(B
> (^[,A;^[(Bsizeof (Elf32_External_Rela) == 12^[,A+^[(B), thus memory for this section will
> be allocated (using bfd_zalloc).  Later on, in sh_elf_relocate_section,
> the linker recognizes that TLS IE can here be optimized into TLS LE, and
> does so; the relocation slot is now not needed anymore (so
> srelgot->reloc_count is not incremented), but it is not reclaimed/the
> size reservation remains (and due to using zalloc, it's a R_SH_NONE), and
> thus the assertion is triggered.
> 
> Expectedly, weakening the assertion into using <= instead of == makes the
> problem go away, but the empty slot in .rela.dyn remains (12 bytes
> wasted).

A weaken assertion would be enough.  I'd like to pre-approve
such a change.  The empty slots for this relatively rare case
would be not a big deal, I guess.

Regards,
	kaz

Re: [SH] TLS IE -> LE optimization issue

[Alan Modra]

On Fri, Apr 13, 2012 at 05:48:06PM +0200, Thomas Schwinge wrote:
> That is, we need a __thread variable that is referenced from another
> compilation unit; these two are linked into an executable; and we need a
> (dummy) SO in order to trigger creation of the .dynamic section, etc.

A dummy .so should make no difference to dynamic relocations, so
you've found a bug in the SH allocate_dynrelocs.

elf32-ppc.c allocate_dynrelocs handles this situation by testing
whether the tls symbol is defined in a dynamic library.  If not, no
dynamic relocation is allocated and a special GOT entry for local
dynamic TLS syms is used, tlsld_got.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [SH] TLS IE -> LE optimization issue

[Kaz Kojima]

Alan Modra <amodra@gmail.com> wrote:
> On Fri, Apr 13, 2012 at 05:48:06PM +0200, Thomas Schwinge wrote:
>> That is, we need a __thread variable that is referenced from another
>> compilation unit; these two are linked into an executable; and we need a
>> (dummy) SO in order to trigger creation of the .dynamic section, etc.
> 
> A dummy .so should make no difference to dynamic relocations, so
> you've found a bug in the SH allocate_dynrelocs.
> 
> elf32-ppc.c allocate_dynrelocs handles this situation by testing
> whether the tls symbol is defined in a dynamic library.  If not, no
> dynamic relocation is allocated and a special GOT entry for local
> dynamic TLS syms is used, tlsld_got.

Ah, then a minimal fix would be a patch like below, though I'm
not sure if it's enough.  Also there might be similar problems
in SH allocate_dynrelocs for the other TLS relocations.

Regards,
	kaz
--
--- ORIG/src/bfd/elf32-sh.c	2012-03-04 10:20:50.000000000 +0900
+++ src/bfd/elf32-sh.c	2012-04-15 07:52:20.000000000 +0900
@@ -3111,6 +3111,9 @@ allocate_dynrelocs (struct elf_link_hash
 	      && (got_type == GOT_NORMAL || got_type == GOT_FUNCDESC))
 	    htab->srofixup->size += 4;
 	}
+      /* No dynamic relocations required when IE->LE conversion happens.  */
+      else if (got_type == GOT_TLS_IE && !h->def_dynamic && !info->shared)
+	;
       /* R_SH_TLS_IE_32 needs one dynamic relocation if dynamic,
 	 R_SH_TLS_GD needs one if local symbol and two if global.  */
       else if ((got_type == GOT_TLS_GD && h->dynindx == -1)

Re: [SH] TLS IE -> LE optimization issue

[Thomas Schwinge]

[-- Attachment #1: Type: text/plain, Size: 3062 bytes --]

Hi!

On Mon, 16 Apr 2012 07:18:11 +0900, Kaz Kojima <kkojima@rr.iij4u.or.jp> wrote:
> Alan Modra <amodra@gmail.com> wrote:
> > On Fri, Apr 13, 2012 at 05:48:06PM +0200, Thomas Schwinge wrote:
> >> That is, we need a __thread variable that is referenced from another
> >> compilation unit; these two are linked into an executable; and we need a
> >> (dummy) SO in order to trigger creation of the .dynamic section, etc.
> > 
> > A dummy .so should make no difference to dynamic relocations, so
> > you've found a bug in the SH allocate_dynrelocs.

> --- ORIG/src/bfd/elf32-sh.c	2012-03-04 10:20:50.000000000 +0900
> +++ src/bfd/elf32-sh.c	2012-04-15 07:52:20.000000000 +0900
> @@ -3111,6 +3111,9 @@ allocate_dynrelocs (struct elf_link_hash
>  	      && (got_type == GOT_NORMAL || got_type == GOT_FUNCDESC))
>  	    htab->srofixup->size += 4;
>  	}
> +      /* No dynamic relocations required when IE->LE conversion happens.  */
> +      else if (got_type == GOT_TLS_IE && !h->def_dynamic && !info->shared)
> +	;
>        /* R_SH_TLS_IE_32 needs one dynamic relocation if dynamic,
>  	 R_SH_TLS_GD needs one if local symbol and two if global.  */
>        else if ((got_type == GOT_TLS_GD && h->dynindx == -1)

This does work fine, thanks!  (No regressions all over a sh-linux-gnu
toolchain (SH4).)


And, for GDB's benefit, I'd like to put in this mini patch, OK?

bfd/
	* elf32-sh.c (elf_sh_link_hash_entry): Specify an enum identifier for
	got_type.
	(allocate_dynrelocs, sh_elf_relocate_section, sh_elf_check_relocs): Use
	it.

Index: bfd/elf32-sh.c
===================================================================
RCS file: /cvs/src/src/bfd/elf32-sh.c,v
retrieving revision 1.175
diff -u -p -r1.175 elf32-sh.c
--- bfd/elf32-sh.c	13 Mar 2012 06:04:35 -0000	1.175
+++ bfd/elf32-sh.c	17 Apr 2012 11:57:31 -0000
@@ -2406,7 +2406,7 @@ struct elf_sh_link_hash_entry
      and thus require fixups or relocations.  */
   bfd_signed_vma abs_funcdesc_refcount;
 
-  enum {
+  enum got_type {
     GOT_UNKNOWN = 0, GOT_NORMAL, GOT_TLS_GD, GOT_TLS_IE, GOT_FUNCDESC
   } got_type;
 };
@@ -3078,7 +3078,7 @@ allocate_dynrelocs (struct elf_link_hash
     {
       asection *s;
       bfd_boolean dyn;
-      int got_type = sh_elf_hash_entry (h)->got_type;
+      enum got_type got_type = sh_elf_hash_entry (h)->got_type;
 
       /* Make sure this symbol is output as a dynamic symbol.
 	 Undefined weak syms won't yet be marked as dynamic.  */
@@ -3977,7 +3980,7 @@ sh_elf_relocate_section (bfd *output_bfd
       bfd_reloc_status_type r;
       int seen_stt_datalabel = 0;
       bfd_vma off;
-      int got_type;
+      enum got_type got_type;
       const char *symname = NULL;
 
       r_symndx = ELF32_R_SYM (rel->r_info);
@@ -6090,7 +6093,7 @@ sh_elf_check_relocs (bfd *abfd, struct b
   const Elf_Internal_Rela *rel_end;
   asection *sreloc;
   unsigned int r_type;
-  int got_type, old_got_type;
+  enum got_type got_type, old_got_type;
 
   sreloc = NULL;
 


Grüße,
 Thomas

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

Re: [SH] TLS IE -> LE optimization issue

[Kaz Kojima]

Thomas Schwinge <thomas@codesourcery.com> wrote:
> This does work fine, thanks!  (No regressions all over a sh-linux-gnu
> toolchain (SH4).)

Thanks for testing.  I'll apply it as a quick fix.

> And, for GDB's benefit, I'd like to put in this mini patch, OK?

OK.

Regards,
	kaz

> bfd/
> 	* elf32-sh.c (elf_sh_link_hash_entry): Specify an enum identifier for
> 	got_type.
> 	(allocate_dynrelocs, sh_elf_relocate_section, sh_elf_check_relocs): Use
> 	it.

Re: [SH] TLS IE -> LE optimization issue

[Kaz Kojima]

> Thanks for testing.  I'll apply it as a quick fix.

For the record, I've committed it with the entry below:

2012-04-17  Kaz Kojima  <kkojima@rr.iij4u.or.jp>

	* elf32-sh.c (allocate_dynrelocs): Don't allocate dynamic
	relocations when LE conversion happens on the IE tls symbol.

Regards,
	kaz