RE: [PATCH] Add TLS support for SH architecture.

[Filippo ARCIDIACONO <filippo.arcidiacono@st.com>]

> -----Original Message-----
> From: prelink-owner@sourceware.org
> [mailto:prelink-owner@sourceware.org] On Behalf Of Jakub Jelinek
> Sent: Wednesday, July 01, 2009 10:55 AM
> To: Filippo ARCIDIACONO
> Cc: prelink@sourceware.org
> Subject: Re: [PATCH] Add TLS support for SH architecture.
> 
> On Wed, Jul 01, 2009 at 03:03:09AM -0400, Filippo ARCIDIACONO wrote:
> > This patch extend the prelink tool also for sh architecture
> adding the
> > tls support, and fixes some architecture specific issue.
> > The fix in the sh_undo_prelink_rela function is needed to
> restore all
> > the GOT entries to the original value.
> > In the prelink stage GOT[1] has been set to the first plt
> entry just
> > after the plt header (This is the necessary code to jump at PLT[0]).
> > To retrieve the other ones we need to add (28 * n) byte to
> the first
> > one, where n is the n-th entry of the PLTn (n = 0,1,2,...).
> > The fix in src/dwarf2.c avoid assertion fail at line 808 for 
> > layout[12], cxx[12] and undosyslibs tests.
> > 
> > --- a/src/dwarf2.c
> > +++ b/src/dwarf2.c
> > @@ -789,6 +789,13 @@ adjust_dwarf2_aranges (DSO *dso,
> GElf_Addr start, GElf_Addr adjust)
> >  	}
> >  
> >        ptr += 6;
> > +#ifdef __SH4__
> > +/*
> > + * This fix skips padding to avoid assertion fail at line 808 for
> > + * layout[12], cxx[12] and undosyslibs tests.
> > + */
> > +      ptr += 8;
> > +#endif
> >        while (ptr < endcu)
> >  	{
> >  	  addr = read_ptr (ptr);
> 

Hi Jakub,
firstly thanks for reviewing this patch.

> This change is not acceptable.  Sometimes prelink is used as a cross 
> tool, so host macros shouldn't make a difference on target behavior.

You're right, indeed I did not think to prelink to be used as cross Tool; that would be an interesting thing anyway.

> Also, if you really need to skip another 8 bytes, it doesn't seem SH 
> is using valid DWARF2/3 .debug_aranges, there really should be just 6 
> bytes skipped here (one byte address_size, one byte segment_size, 4 
> bytes padding to 16 byte boundary (the tuples are supposed to be 
> aligned to twice the pointer
> size) and the aranges entries start right after that.  I don't see 
> anything in GCC that would suggest SH is treated differently.
> 
I looked again at the test failure, indeed it seems the problem is in our libstdc++, While all other DSOs have a valid dwarf2 .debug_aranges.
Our libstdc++ .debug_aranges seems to be not valid.

readelf --debug-dump=aranges ./libstdc++.so.6

The section .debug_aranges contains:

  Length:                   1028 
  Version:                  2    
  Offset into .debug_info:  0x0  
  Pointer Size:             4    
  Segment Size:             0    

    Address    Length
00000000 00000000    <----- Spurious 8 bytes I skipped
319c7b00 00000020
319c7b20 00000020
319c7b40 00000054
319c7b94 00000040
319c7bd4 00000012
:
:

I'll have a look at binutils. So I agree with you to discard this part of the patch.

> So, which compiler are you testing with, if it is not GCC, are you 
> sure you aren't breaking prelink support for GCC on this arch?  And in 
> any case assembly output from the compiler with -dA for the 
> .debug_aranges section to show what those 8 bytes are.
> 
> Also, I'd like to understand those other non-TLS non-testsuite 
> changes, are you saying prelink as is never really worked on any SH, 
> or just the SH variant you are testing on behaves differently from the 
> one it has been used on before (my memory is weak, it has been many 
> years, so I don't remember well who tested prelink on which of the 
> embedded arches I didn't have any access to).

Which other changes (other than dwarf2.c) are you referring to?

> 
> > --- a/testsuite/reloc8.sh
> > +++ b/testsuite/reloc8.sh
> > @@ -8,6 +8,9 @@ NOCOPYRELOC=-Wl,-z,nocopyreloc  case "`uname -m`" in
> >    x86_64|s390*) if file reloc1lib1.so | grep -q 64-bit; then 
> > NOCOPYRELOC=; fi;;  esac
> > +case "`uname -m`" in
> > +  sh4) NOCOPYRELOC=;
> > +esac
> 
> This certainly should go into the same case, no need to call uname -m 
> the second time.
> 
> > --- a/testsuite/reloc9.sh
> > +++ b/testsuite/reloc9.sh
> > @@ -8,6 +8,9 @@ NOCOPYRELOC=-Wl,-z,nocopyreloc  case "`uname -m`" in
> >    x86_64|s390*) if file reloc1lib1.so | grep -q 64-bit; then 
> > NOCOPYRELOC=; fi;;  esac
> > +case "`uname -m`" in
> > +  sh4) NOCOPYRELOC=;
> > +esac

I'll review both.
> 
> Likewise.
> 
> 	Jakub
> 

Regards,
Filippo.