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

[kaz Kojima <kkojima at rr dot iij4u dot or dot jp>]

Hans-Peter Nilsson <hp@bitrange.com> wrote:
>The changes I request are mostly simple, I hope, but may be
>large in extent.

I'd like to change almost of them as your suggestions.

>> [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?

The TLS relocs should be handled as non-adjustable in
sh_fix_adjustable. I'm not sure about sh_force_relocation.
It seems that there is no handling in i386_force_relocation,
right?

>> @@ -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.

Sure. The more optimizations based on visibility will be needed.
Can we add to the TODO list at this point?

>> +++ 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.

I've tried this, but have a problem. sh64 objects contain .crangs
section even with -isa=SHcompact, so the results of TLS tests
are pretty different. The separated result patterns seems better
for sh64, though it may be worse from the maintenance point of view.

BTW, the current pure sh*-elf can't generate  DSO, with the
message "Not enough room for program headers". The all TLS tests
fail with the same status. It isn't the problem of the test but
is the problem of the sh-elf linker itself.

>> +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.

OK. I prefer the scheme of the run_dump_tests but couldn't
find cris.exp :-)

Thanks a lot for your advises. I'll come back with the revised
patch again.

Regards,
	kaz