Re: [PATCH/RFA] SH TLS support (Revised)

[Hans-Peter Nilsson <hp at bitrange dot com>]

On Sat, 5 Oct 2002, kaz Kojima wrote:

> Ulrich Drepper pointed out to me that some SH TLS relocations can be
> removed. By this, only 8 TLS relocations are required and ld/gas
> changes can be simplified a little bit.

> Here is a revised patch for
> SH TLS support. Regtested on sh-unknown-linux-gnu and sh64-unknown-elf.

Thank you for doing this work.

I won't have much comments for the actual functionality (other
than that it looks correct at a glance), though I think I
understand Drepper's paper after reading it enough times. :-)

(Other SH maintainers are extremely welcome to chime in,
specifically about the TLS functionality.)

The changes I request are mostly simple, I hope, but may be
large in extent.

> Sorry for sending a relatively large patch twice.

No problem.  Regarding patches, please use -p in future patches,
so the function name for each context is visible.  (Whether
context or unified diff is best depends on the patch; unified
was fine here; -up would have helped, particularly with a
patch this large.)

> 	* elf32-sh.c (sh_elf_tls_transition, sh_elf_mkobject,
> 	sh_elf_object_p, dtpoff_base): New functions.
> 	(sh_elf_howto_table): Add TLS relocs.
> 	(sh_reloc_map): Likewise.
> 	(sh_elf_info_to_howto): Support TLS relocs.
> 	(elf_sh_link_hash_entry): Add tls_type and tls_tpoff32.
> 	(sh_elf_hash_entry, sh_elf_tdata, sh_elf_local_got_tls_type):
> 	New macros.
> 	(sh_elf_obj_tdata): New.
> 	(elf_sh_link_hash_table): Add tls_ldm_got.
> 	(sh_elf_link_hash_table_create): Clear refcount of tls_ldm_got.
> 	(allocate_dynrelocs): Support TLS relocs.
> 	(sh_elf_size_dynamic_sections): Likewise.
> 	(sh_elf_relocate_section): Likewise.

You should also mention that there are some cleanups in
sh_elf_relocate_section and the warning that is now an error.

> [gas/ChangeLog]
> 	* config/tc-sh.c (md_apply_fix3): Add TLS relocs.
> 	(sh_parse_name): Support @TLSGD, @TLSLDM, @GOTTPOFF, @TPOFF and
> 	@DTPOFF.

Don't you also need to handle these relocs in
sh_force_relocation and sh_fix_adjustable?

> [include/ChangeLog]
> 	* elf/sh.h: Add SH TLS relocs.

The proper ChangeLog is in include/elf.

> +++ LOCAL/src/bfd/elf32-sh-lin.c	Sat Oct  5 13:54:49 2002
> @@ -105,6 +105,6 @@
>  #define elf_backend_grok_prstatus	elf32_shlin_grok_prstatus
>  #define elf_backend_grok_psinfo		elf32_shlin_grok_psinfo
>
> -
> +#define USE_SH_TLS32

I think we do not need this (or any of the #ifdefs); I see no
reason to exclude this code.  If there is one, it should
be stated in a comment above.

Compare to INCLUDE_SHMEDIA, where code can be left out for those
wanting a non-sh64 toolchain.  Regarding TLS, I think all SH
toolchains supporting ELF shared libraries will eventually want
to use this code, and so #define it anyway, and targets without
it would rot.  Toolchains with no support for shared libraries
are usually cross tools with no need to keep down the code size
of the tools.  I don't see how this will interfere with 64-bit
TLS (sh64)  or 32-bit SHmedia TLS, since that will require some
new relocs anyway.

> +++ LOCAL/src/bfd/elf32-sh.c	Sat Oct  5 13:57:19 2002
> @@ -59,6 +59,14 @@
>  static void sh_elf_copy_indirect_symbol
>    PARAMS ((struct elf_backend_data *, struct elf_link_hash_entry *,
>  	   struct elf_link_hash_entry *));
> +#ifdef USE_SH_TLS32
> +static int sh_elf_tls_transition
> +  PARAMS ((struct bfd_link_info *, int, int));

I think a better name would be sh_elf_optimized_tls_reloc, since
the function doesn't actually perform a (state) transition, just
presents a better choice for the reloc type given known locality
and visibility.  (And that comment goes for the same-named
function in elf32-i386.c too. ;-)

> +static boolean
> +sh_elf_mkobject (abfd)

This function should have a preceding comment, like other
functions.  (It would e.g. point out that the generic function
needs to be overridden because sizeof sh_elf_obj_tdata is
needed, not just elf_obj_tdata.) I know there are functions
missing a header, but patches should improve that.

(Yes, elf32-i386.c misses one too.  I think it'd count as an
obvious fix to add one.)

> @@ -3943,6 +4138,9 @@
>      {
>        asection *s;
>        boolean dyn;
> +#ifdef USE_SH_TLS32
> +      int tls_type = sh_elf_hash_entry(h)->tls_type;

Minor spacing nit here and in many other similar "foo(h)" in the
patch; it should be: "sh_elf_hash_entry (h)->tls_type"

> @@ -4534,7 +4796,7 @@
>  		    (_("%s: warning: unresolvable relocation against symbol `%s' from %s section"),
>  		     bfd_archive_filename (input_bfd), h->root.root.string,
>  		     bfd_get_section_name (input_bfd, input_section));
> -		  relocation = 0;
> +		  return false;

Please adjust the message to not say "warning", since it's not
anymore (and shouldn't have been in the first place).  This
needs a changelog entry by itself.

> @@ -4872,11 +5131,14 @@
>
>  		  if (info->shared)
>  		    {
> -		      asection *srelgot;
>  		      Elf_Internal_Rela outrel;
>
> -		      srelgot = bfd_get_section_by_name (dynobj, ".rela.got");
> -		      BFD_ASSERT (srelgot != NULL);
> +		      if (srelgot == NULL)
> +			{
> +			  srelgot = bfd_get_section_by_name (dynobj,
> +							     ".rela.got");
> +			  BFD_ASSERT (srelgot != NULL);
> +			}
>
>  		      outrel.r_offset = (sgot->output_section->vma
>  					 + sgot->output_offset

This is an improvement that deserves a ChangeLog note that
doesn't just briefly say it's a TLS change.

> @@ -5010,6 +5272,452 @@
>  				   rel->r_offset, sec, start, end);
>  	    break;
>  	  }
> +
> +#ifdef USE_SH_TLS32
> +	case R_SH_TLS_GD_32:
> +	case R_SH_TLS_IE_32:
> +	  r_type = sh_elf_tls_transition (info, r_type, h == NULL);

I think you want

  h == NULL || ELF_ST_VISIBILITY (h->other) != STV_DEFAULT

as an optimization, but this isn't the only place.
But don't change this now; it's not needed for correctness or
patch approval.  I just suggest it as an improvement and as such
it's better done separately.  I see visibility isn't optimized
for in many places in elf32-sh.c.

>
> +#ifdef USE_SH_TLS32
> +static int
> +sh_elf_tls_transition (info, r_type, is_local)

Missing function header comment.  Note also the name change
suggestion.

> @@ -5530,15 +6354,61 @@
>  		     codelabel local GOT offsets.  */
>  		  size *= 2;
>  #endif
> +#ifdef USE_SH_TLS32
> +		  size += symtab_hdr->sh_info * sizeof (char);

Sizeof char is 1 by standard definition; no need for that
multiplication.

> +#ifdef USE_SH_TLS32
> +#ifdef 	INCLUDE_SHMEDIA
> +		  sh_elf_local_got_tls_type (abfd)
> +		    = (char *) (local_got_refcounts + 2 * symtab_hdr->sh_info);
> +#else
> +		  sh_elf_local_got_tls_type (abfd)
> +		    = (char *) (local_got_refcounts + symtab_hdr->sh_info);
> +#endif
> +#endif

Why do you multiply by two here ifdef INCLUDE_SHMEDIA?
(Tell it in a comment in the code.)

> @@ -5835,6 +6806,27 @@
>  }
>  #endif /* not sh_elf_merge_private_data */
>
> +#ifdef USE_SH_TLS32
> +static boolean
> +sh_elf_object_p (abfd)

Missing function header comment.

> +++ LOCAL/src/gas/testsuite/gas/sh/basic.exp	Sat Oct  5 13:54:49 2002
> @@ -136,4 +136,13 @@
>      if {[istarget sh*-*elf] || [istarget sh*-linux*]} then {
>  	run_dump_test "pic"
>      }
> +
> +    if {[istarget "sh*-linux*"] && ![istarget sh64*-linux*]} then {

No, please match any SH ELF target.  It seems gas options like
"#as: -little" to each test are necessary.  Perhaps a
command-line option to set SHcompact mode is needed too, then.

> +++ LOCAL/src/ld/testsuite/ld-sh/sh-tls.exp	Sat Oct  5 13:55:14 2002
> @@ -0,0 +1,41 @@

> +if {! [istarget sh*-*-linux*] || [istarget sh64*-*-linux*]} {
> +    # Currently only for 32-bit linux target.

No, please let the tests run on other targets, like the changes
requested for the gas tests.

> +set tlstests {
> +    {"TLS -fpic -shared transitions" "-shared -mshlelf_linux"
> +     "" {tlspic1.s tlspic2.s}
> +     {{readelf -Ssrl tlspic.rd} {objdump -drj.text tlspic.dd}
> +      {objdump -sj.got tlspic.sd} {objdump -sj.tdata tlspic.td}}
> +      "libtlspic.so"}
> +     {"Helper shared library" "-shared -mshlelf_linux"
> +    "" {tlslib.s} {} "libtlslib.so"}
> +    {"TLS -fpic and -fno-pic exec transitions"
> +    "-mshlelf_linux tmpdir/libtlslib.so" "" {tlsbinpic.s tlsbin.s}
> +    {{readelf -Ssrl tlsbin.rd} {objdump -drj.text tlsbin.dd}
> +    {objdump -sj.got tlsbin.sd} {objdump -sj.tdata tlsbin.td}}
> +    "tlsbin"}
> +}
> +
> +run_ld_link_tests $tlstests

I've come to dislike run_ld_link_tests: it has too much
redundancy.  I hope I haven't given it too many positive
comments before.

There's the table, then there's options in *both the tests and
the table*.  Which one is picked?  Also, for each new test, you
need to add something to the table and also the test files
themselves.  It's maintenance-wise a step backwards from
e.g. run_dump_test *.d.

I'd prefer to organize linker-tests that link against other
libraries like in ld-cris/cris.exp (of course), where some
run_dump_tests run before others, keyed on the filename.  (Each
of the .rd, .td, .sd and .dd test could be a separate
run_dump_test; the DSO needs to be created before, in a separate
test.)  Still, I leave the choice to you.

brgds, H-P