bug: GOLD handling of Sparc PLTREL

bug: GOLD handling of Sparc PLTREL

[David Miller]

Several test cases in GOLD fail on sparc because of how it lays out
the .rela.plt and .rela.dyn sections.

On Sparc, the .rela.dyn must include .rela.plt in it's range.  If this
is not followed, the dynamic linker will reference past the end of the
relocations.  This is true on 32-bit PowerPC and 32-bit S390 as well.

But GOLD isn't doing this, it makes the size of .rela.dyn only include
the .rela.dyn relocs, it doesn't include the  .rela.plt reloc size
too.

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 08 Feb 2010 16:42:45 -0800 (PST)

> But GOLD isn't doing this, it makes the size of .rela.dyn only include
> the .rela.dyn relocs, it doesn't include the  .rela.plt reloc size
> too.

This bug was by was introduced by:

2010-01-07  Ian Lance Taylor  <iant@google.com>

	* output.h (class Output_data): Add const version of
	output_section and do_output_section.
	(class Output_section_data): Add const version of
	do_output_section.
	(class Output_section): Likewise.
	* layout.cc (Layout::add_target_dynamic_tags): New function.
	* layout.h (class Layout): Update declarations.
	* arm.cc (Target_arm::do_finalize_sections): Use
	add_target_dynamic_tags.
	* i386.cc (Target_i386::do_finalize_sections): Likewise.
	* powerpc.cc (Target_powerpc::do_finalize_sections): Likewise.
	* sparc.cc (Target_sparc::do_finalize_sections): Likewise.
	* x86_64.cc (Target_x86_64::do_finalize_sections): Likewise.

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 08 Feb 2010 16:57:00 -0800 (PST)

> From: David Miller <davem@davemloft.net>
> Date: Mon, 08 Feb 2010 16:42:45 -0800 (PST)
> 
>> But GOLD isn't doing this, it makes the size of .rela.dyn only include
>> the .rela.dyn relocs, it doesn't include the  .rela.plt reloc size
>> too.
> 
> This bug was by was introduced by:
> 
> 2010-01-07  Ian Lance Taylor  <iant@google.com>

Actually, it seems more complicated than this.

The lack of overlapping .rela.dyn only causes visible problems breaks
once we start emitting the DT_RELACOUNT which gets added in a later
change.

And it seems GOLD has always not made the sections overlap.

Hmmm...

Re: bug: GOLD handling of Sparc PLTREL

[Ian Lance Taylor]

David Miller <davem@davemloft.net> writes:

> Several test cases in GOLD fail on sparc because of how it lays out
> the .rela.plt and .rela.dyn sections.
>
> On Sparc, the .rela.dyn must include .rela.plt in it's range.  If this
> is not followed, the dynamic linker will reference past the end of the
> relocations.  This is true on 32-bit PowerPC and 32-bit S390 as well.
>
> But GOLD isn't doing this, it makes the size of .rela.dyn only include
> the .rela.dyn relocs, it doesn't include the  .rela.plt reloc size
> too.

I don't see how this could be a problem, since the only thing the
dynamic linker sees are the segments and the dynamic tags.  The
dynamic linker doesn't see anything about the .rela.dyn or .rela.plt
sections.


> The lack of overlapping .rela.dyn only causes visible problems breaks
> once we start emitting the DT_RELACOUNT which gets added in a later
> change.

Hmmm, I'm not sure I see how that could be a problem either, unless
something weird is happening.  DT_RELACOUNT just counts the number of
relative relocs.

I see a few calls to add_local_relative and add_global_relative that
are not passing down elfcpp::R_SPARC_RELATIVE as the reloc type.  That
is rather suspicious and seems like it could cause this sort of
problem.  The add_local_relative and add_global_relative functions
should only be used to add RELATIVE relocs to the dynamic reloc
section, not to add any other sort of relocation.

Ian

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: Ian Lance Taylor <iant@google.com>
Date: Mon, 08 Feb 2010 21:53:50 -0800

> David Miller <davem@davemloft.net> writes:
> 
>> Several test cases in GOLD fail on sparc because of how it lays out
>> the .rela.plt and .rela.dyn sections.
>>
>> On Sparc, the .rela.dyn must include .rela.plt in it's range.  If this
>> is not followed, the dynamic linker will reference past the end of the
>> relocations.  This is true on 32-bit PowerPC and 32-bit S390 as well.
>>
>> But GOLD isn't doing this, it makes the size of .rela.dyn only include
>> the .rela.dyn relocs, it doesn't include the  .rela.plt reloc size
>> too.
> 
> I don't see how this could be a problem, since the only thing the
> dynamic linker sees are the segments and the dynamic tags.  The
> dynamic linker doesn't see anything about the .rela.dyn or .rela.plt
> sections.

It is a problem, GLIBC has code which does the following when the
macro ELF_MACHINE_PLTREL_OVERLAP is defined:

/* On some machines, notably SPARC, DT_REL* includes DT_JMPREL in its
   range.  Note that according to the ELF spec, this is completely legal!
   But conditionally define things so that on machines we know this will
   not happen we do something more optimal.  */

# ifdef ELF_MACHINE_PLTREL_OVERLAP
#  define _ELF_DYNAMIC_DO_RELOC(RELOC, reloc, map, do_lazy, test_rel) \
  do {									      \
    struct { ElfW(Addr) start, size; int lazy; } ranges[3];		      \
    int ranges_index;							      \
									      \
    ranges[0].lazy = ranges[2].lazy = 0;				      \
    ranges[1].lazy = 1;							      \
    ranges[0].size = ranges[1].size = ranges[2].size = 0;		      \
									      \
    if ((map)->l_info[DT_##RELOC])					      \
      {									      \
	ranges[0].start = D_PTR ((map), l_info[DT_##RELOC]);		      \
	ranges[0].size = (map)->l_info[DT_##RELOC##SZ]->d_un.d_val;	      \
      }									      \
									      \
    if ((do_lazy)							      \
	&& (map)->l_info[DT_PLTREL]					      \
	&& (!test_rel || (map)->l_info[DT_PLTREL]->d_un.d_val == DT_##RELOC)) \
      {									      \
	ranges[1].start = D_PTR ((map), l_info[DT_JMPREL]);		      \
	ranges[1].size = (map)->l_info[DT_PLTRELSZ]->d_un.d_val;	      \
	ranges[2].start = ranges[1].start + ranges[1].size;		      \
	ranges[2].size = ranges[0].start + ranges[0].size - ranges[2].start;  \
	ranges[0].size = ranges[1].start - ranges[0].start;		      \
      }									      \
									      \
    for (ranges_index = 0; ranges_index < 3; ++ranges_index)		      \
      elf_dynamic_do_##reloc ((map),					      \
			      ranges[ranges_index].start,		      \
			      ranges[ranges_index].size,		      \
			      ranges[ranges_index].lazy);		      \
  } while (0)

(##RELOC here is RELA)

Think about what happens to those ranges in the "do_lazy" case when
.rela.dyn does not overlap with .rela.plt

The values in ranges[2] will be garbage (size will be negative for one
thing), and elf_dynamic_do_*() will walk over the edge of the
relcation entries.

Because of how this is coded, GLIBC absolutely expects .rela.dyn
to overlap into .rela.plt when present.

Binutils has been ensuring this forever, so I think that GOLD has to
do it too.

> I see a few calls to add_local_relative and add_global_relative that
> are not passing down elfcpp::R_SPARC_RELATIVE as the reloc type.  That
> is rather suspicious and seems like it could cause this sort of
> problem.

It has nothing to do with these crashes.

I'm playng around with the following patch to fix this:

gold/

2010-02-08  David S. Miller  <davem@davemloft.net>

	* output.h (Output_data_dynamic::add_section_size): New method
	that takes two Output_data objects.
	(Output_data_dynamic::Dynamic_entry): Create storage for secondary
	entry param.  Handle it in initializers.
	* output.cc (Output_data_dynamic::Dynamic_entry::write): For
	DYNAMIC_SECTION_SIZE, add in section object size if non-NULL.
	* layout.h (Layout::add_target_dynamic_tags): Add pltrel_overlap arg.
	* layout.cc (Layout::add_target_dynamic_tags): If pltrel_overlap, and
	.rela.plt exists, set DT_REL{,A}SZ to sum of .rela.dyn and .rela.plt
	* arm.cc (Target_arm::do_finalize_sections): Update to pass false
	for pltrel_overlap.
	* i386.cc (Target_i386::do_finalize_sections): Likewise.
	* x86_64.cc (Target_x86_64::do_finalize_sections): Likewise.
	* sparc.cc (Target_sparc::make_plt_entry): Force .rela.dyn to be output
	before .rela.plt
	(Target_sparc::do_finalize_sections): Update to pass true for
	pltrel_overlap.
	* powerpc.cc (Target_powerpc::make_plt_entry): Force .rela.dyn to be
	output before .rela.plt
	(Target_powerpc::do_finalize_sections): Update to pass true for
	pltrel_overlap when 32-bit.

diff --git a/gold/arm.cc b/gold/arm.cc
index bec70db..51a660f 100644
--- a/gold/arm.cc
+++ b/gold/arm.cc
@@ -7066,7 +7066,7 @@ Target_arm<big_endian>::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rel_plt());
   layout->add_target_dynamic_tags(true, this->got_plt_, rel_plt,
-				  this->rel_dyn_, true);
+				  this->rel_dyn_, true, false);
 
   // Emit any relocs we saved in an attempt to avoid generating COPY
   // relocs.
diff --git a/gold/i386.cc b/gold/i386.cc
index 2eab3f8..f2a7b53 100644
--- a/gold/i386.cc
+++ b/gold/i386.cc
@@ -1609,7 +1609,7 @@ Target_i386::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rel_plt());
   layout->add_target_dynamic_tags(true, this->got_plt_, rel_plt,
-				  this->rel_dyn_, true);
+				  this->rel_dyn_, true, false);
 
   // Emit any relocs we saved in an attempt to avoid generating COPY
   // relocs.
diff --git a/gold/layout.cc b/gold/layout.cc
index 52989a5..bb74ac0 100644
--- a/gold/layout.cc
+++ b/gold/layout.cc
@@ -3240,7 +3240,7 @@ void
 Layout::add_target_dynamic_tags(bool use_rel, const Output_data* plt_got,
 				const Output_data* plt_rel,
 				const Output_data_reloc_generic* dyn_rel,
-				bool add_debug)
+				bool add_debug, bool pltrel_overlap)
 {
   Output_data_dynamic* odyn = this->dynamic_data_;
   if (odyn == NULL)
@@ -3261,8 +3261,12 @@ Layout::add_target_dynamic_tags(bool use_rel, const Output_data* plt_got,
     {
       odyn->add_section_address(use_rel ? elfcpp::DT_REL : elfcpp::DT_RELA,
 				dyn_rel);
-      odyn->add_section_size(use_rel ? elfcpp::DT_RELSZ : elfcpp::DT_RELASZ,
-			     dyn_rel);
+      if (plt_rel != NULL && pltrel_overlap)
+	odyn->add_section_size(use_rel ? elfcpp::DT_RELSZ : elfcpp::DT_RELASZ,
+			       dyn_rel, plt_rel);
+      else
+	odyn->add_section_size(use_rel ? elfcpp::DT_RELSZ : elfcpp::DT_RELASZ,
+			       dyn_rel);
       const int size = parameters->target().get_size();
       elfcpp::DT rel_tag;
       int rel_size;
diff --git a/gold/layout.h b/gold/layout.h
index 15e7548..ff375f8 100644
--- a/gold/layout.h
+++ b/gold/layout.h
@@ -563,7 +563,7 @@ class Layout
   add_target_dynamic_tags(bool use_rel, const Output_data* plt_got,
 			  const Output_data* plt_rel,
 			  const Output_data_reloc_generic* dyn_rel,
-			  bool add_debug);
+			  bool add_debug, bool pltrel_overlap);
 
   // Compute and write out the build ID if needed.
   void
diff --git a/gold/output.cc b/gold/output.cc
index a8f03e7..44922bf 100644
--- a/gold/output.cc
+++ b/gold/output.cc
@@ -1560,6 +1560,8 @@ Output_data_dynamic::Dynamic_entry::write(
 
     case DYNAMIC_SECTION_SIZE:
       val = this->u_.od->data_size();
+      if (this->u2_.od)
+	val += this->u2_.od->data_size();
       break;
 
     case DYNAMIC_SYMBOL:
diff --git a/gold/output.h b/gold/output.h
index 60d57de..b35a6f6 100644
--- a/gold/output.h
+++ b/gold/output.h
@@ -1997,6 +1997,12 @@ class Output_data_dynamic : public Output_section_data
   add_section_size(elfcpp::DT tag, const Output_data* od)
   { this->add_entry(Dynamic_entry(tag, od, true)); }
 
+  // Add a new dynamic entry with the total size of two output datas.
+  void
+  add_section_size(elfcpp::DT tag, const Output_data* od,
+		   const Output_data* od2)
+  { this->add_entry(Dynamic_entry(tag, od, od2)); }
+
   // Add a new dynamic entry with the address of a symbol.
   void
   add_symbol(elfcpp::DT tag, const Symbol* sym)
@@ -2045,7 +2051,13 @@ class Output_data_dynamic : public Output_section_data
 	offset_(section_size
 		? DYNAMIC_SECTION_SIZE
 		: DYNAMIC_SECTION_ADDRESS)
-    { this->u_.od = od; }
+    { this->u_.od = od; this->u2_.od = NULL; }
+
+    // Create an entry with the size of two sections.
+    Dynamic_entry(elfcpp::DT tag, const Output_data* od, const Output_data* od2)
+      : tag_(tag),
+	offset_(DYNAMIC_SECTION_SIZE)
+    { this->u_.od = od; this->u2_.od = od2; }
 
     // Create an entry with the address of a section plus a constant offset.
     Dynamic_entry(elfcpp::DT tag, const Output_data* od, unsigned int offset)
@@ -2100,7 +2112,7 @@ class Output_data_dynamic : public Output_section_data
       const Symbol* sym;
       // For DYNAMIC_STRING.
       const char* str;
-    } u_;
+    } u_, u2_;
     // The dynamic tag.
     elfcpp::DT tag_;
     // The type of entry (Classification) or offset within a section.
diff --git a/gold/powerpc.cc b/gold/powerpc.cc
index 83bb992..cc46782 100644
--- a/gold/powerpc.cc
+++ b/gold/powerpc.cc
@@ -948,6 +948,11 @@ Target_powerpc<size, big_endian>::make_plt_entry(Symbol_table* symtab,
       // Create the GOT section first.
       this->got_section(symtab, layout);
 
+      // Ensure that .rela.dyn always appears before .rela.plt  This is
+      // necessary due to how, on PowerPC and some other targets, .rela.dyn
+      // needs to include .rela.plt in it's range.
+      this->rela_dyn_section(layout);
+
       this->plt_ = new Output_data_plt_powerpc<size, big_endian>(layout);
       layout->add_output_section_data(".plt", elfcpp::SHT_PROGBITS,
 				      (elfcpp::SHF_ALLOC
@@ -1556,7 +1561,7 @@ Target_powerpc<size, big_endian>::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rel_plt());
   layout->add_target_dynamic_tags(false, this->plt_, rel_plt,
-				  this->rela_dyn_, true);
+				  this->rela_dyn_, true, size == 32);
 
   // Emit any relocs we saved in an attempt to avoid generating COPY
   // relocs.
diff --git a/gold/sparc.cc b/gold/sparc.cc
index 6075c44..6a98d91 100644
--- a/gold/sparc.cc
+++ b/gold/sparc.cc
@@ -1369,6 +1369,11 @@ Target_sparc<size, big_endian>::make_plt_entry(Symbol_table* symtab,
       // Create the GOT sections first.
       this->got_section(symtab, layout);
 
+      // Ensure that .rela.dyn always appears before .rela.plt  This is
+      // necessary due to how, on Sparc and some other targets, .rela.dyn
+      // needs to include .rela.plt in it's range.
+      this->rela_dyn_section(layout);
+
       this->plt_ = new Output_data_plt_sparc<size, big_endian>(layout);
       layout->add_output_section_data(".plt", elfcpp::SHT_PROGBITS,
 				      (elfcpp::SHF_ALLOC
@@ -2341,7 +2346,7 @@ Target_sparc<size, big_endian>::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rel_plt());
   layout->add_target_dynamic_tags(false, this->plt_, rel_plt,
-				  this->rela_dyn_, true);
+				  this->rela_dyn_, true, true);
 
   // Emit any relocs we saved in an attempt to avoid generating COPY
   // relocs.
diff --git a/gold/x86_64.cc b/gold/x86_64.cc
index 585a499..fea2ec9 100644
--- a/gold/x86_64.cc
+++ b/gold/x86_64.cc
@@ -1741,7 +1741,7 @@ Target_x86_64::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rela_plt());
   layout->add_target_dynamic_tags(false, this->got_plt_, rel_plt,
-				  this->rela_dyn_, true);
+				  this->rela_dyn_, true, false);
 				  
   // Fill in some more dynamic tags.
   Output_data_dynamic* const odyn = layout->dynamic_data();

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 08 Feb 2010 22:01:21 -0800 (PST)

> # ifdef ELF_MACHINE_PLTREL_OVERLAP
> #  define _ELF_DYNAMIC_DO_RELOC(RELOC, reloc, map, do_lazy, test_rel) \
 ...
> (##RELOC here is RELA)

And for reference here is the code that gets passed those ranges,
from elf/do-rel.h:

auto inline void __attribute__ ((always_inline))
elf_dynamic_do_rel (struct link_map *map,
		    ElfW(Addr) reladdr, ElfW(Addr) relsize,
		    int lazy)
{
  const ElfW(Rel) *r = (const void *) reladdr;
  const ElfW(Rel) *end = (const void *) (reladdr + relsize);
  ElfW(Addr) l_addr = map->l_addr;

#if (!defined DO_RELA || !defined ELF_MACHINE_PLT_REL) && !defined RTLD_BOOTSTRAP
  /* We never bind lazily during ld.so bootstrap.  Unfortunately gcc is
     not clever enough to see through all the function calls to realize
     that.  */
  if (lazy)
    {
      /* Doing lazy PLT relocations; they need very little info.  */
      for (; r < end; ++r)
	elf_machine_lazy_rel (map, l_addr, r);
    }
  else
#endif
    {
      const ElfW(Sym) *const symtab =
	(const void *) D_PTR (map, l_info[DT_SYMTAB]);
      ElfW(Word) nrelative = (map->l_info[RELCOUNT_IDX] == NULL
			      ? 0 : map->l_info[RELCOUNT_IDX]->d_un.d_val);
      const ElfW(Rel) *relative = r;
      r = r + MIN (nrelative, relsize / sizeof (ElfW(Rel)));

#ifndef RTLD_BOOTSTRAP
      /* This is defined in rtld.c, but nowhere in the static libc.a; make
	 the reference weak so static programs can still link.  This
	 declaration cannot be done when compiling rtld.c (i.e. #ifdef
	 RTLD_BOOTSTRAP) because rtld.c contains the common defn for
	 _dl_rtld_map, which is incompatible with a weak decl in the same
	 file.  */
# ifndef SHARED
      weak_extern (GL(dl_rtld_map));
# endif
      if (map != &GL(dl_rtld_map)) /* Already done in rtld itself.  */
# if !defined DO_RELA || defined ELF_MACHINE_REL_RELATIVE
	/* Rela platforms get the offset from r_addend and this must
	   be copied in the relocation address.  Therefore we can skip
	   the relative relocations only if this is for rel
	   relocations or rela relocations if they are computed as
	   memory_loc += l_addr...  */
	if (l_addr != 0)
# else
	/* ...or we know the object has been prelinked.  */
	if (l_addr != 0 || ! map->l_info[VALIDX(DT_GNU_PRELINKED)])
# endif
#endif
	  for (; relative < r; ++relative)
	    DO_ELF_MACHINE_REL_RELATIVE (map, l_addr, relative);

#ifdef RTLD_BOOTSTRAP
      /* The dynamic linker always uses versioning.  */
      assert (map->l_info[VERSYMIDX (DT_VERSYM)] != NULL);
#else
      if (map->l_info[VERSYMIDX (DT_VERSYM)])
#endif
	{
	  const ElfW(Half) *const version =
	    (const void *) D_PTR (map, l_info[VERSYMIDX (DT_VERSYM)]);

	  for (; r < end; ++r)
	    {
	      ElfW(Half) ndx = version[ELFW(R_SYM) (r->r_info)] & 0x7fff;
	      elf_machine_rel (map, r, &symtab[ELFW(R_SYM) (r->r_info)],
			       &map->l_versions[ndx],
			       (void *) (l_addr + r->r_offset));
	    }
	}
#ifndef RTLD_BOOTSTRAP
      else
	for (; r < end; ++r)
	  elf_machine_rel (map, r, &symtab[ELFW(R_SYM) (r->r_info)], NULL,
			   (void *) (l_addr + r->r_offset));
#endif
    }
}

Re: bug: GOLD handling of Sparc PLTREL

[Ian Lance Taylor]

David Miller <davem@davemloft.net> writes:

> From: Ian Lance Taylor <iant@google.com>
> Date: Mon, 08 Feb 2010 21:53:50 -0800
>
>> David Miller <davem@davemloft.net> writes:
>> 
>>> Several test cases in GOLD fail on sparc because of how it lays out
>>> the .rela.plt and .rela.dyn sections.
>>>
>>> On Sparc, the .rela.dyn must include .rela.plt in it's range.  If this
>>> is not followed, the dynamic linker will reference past the end of the
>>> relocations.  This is true on 32-bit PowerPC and 32-bit S390 as well.
>>>
>>> But GOLD isn't doing this, it makes the size of .rela.dyn only include
>>> the .rela.dyn relocs, it doesn't include the  .rela.plt reloc size
>>> too.
>> 
>> I don't see how this could be a problem, since the only thing the
>> dynamic linker sees are the segments and the dynamic tags.  The
>> dynamic linker doesn't see anything about the .rela.dyn or .rela.plt
>> sections.
>
> It is a problem, GLIBC has code which does the following when the
> macro ELF_MACHINE_PLTREL_OVERLAP is defined:

Yes, but to be pedantically clear, the problem there is not that
.rela.dyn and .rela.plt are separate.  The problem is that the DT_RELA
dynamic tag does not include the PLT relocations.


>> I see a few calls to add_local_relative and add_global_relative that
>> are not passing down elfcpp::R_SPARC_RELATIVE as the reloc type.  That
>> is rather suspicious and seems like it could cause this sort of
>> problem.
>
> It has nothing to do with these crashes.

It is a problem, though, because it will cause DT_RELACOUNT to be
incorrect.


> I'm playng around with the following patch to fix this:

Looks like a reasonable approach.  I prefer boolean variables to have
names which read correctly with if, so perhaps dynrel_includes_plt
instead of of pltrel_overlap.  Also no need to duplicate the entire
union in Output_data_dynamic.

Another approach would be to in fact put the PLT into the .rela.dyn
output section.  That could be done by just changing the call to
layout->add_output_section_data to use .rela.dyn in both SHT_RELA
calls in sparc.cc.

Ian

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: Ian Lance Taylor <iant@google.com>
Date: Tue, 09 Feb 2010 07:04:13 -0800

> David Miller <davem@davemloft.net> writes:
> 
>> It is a problem, GLIBC has code which does the following when the
>> macro ELF_MACHINE_PLTREL_OVERLAP is defined:
> 
> Yes, but to be pedantically clear, the problem there is not that
> .rela.dyn and .rela.plt are separate.  The problem is that the DT_RELA
> dynamic tag does not include the PLT relocations.

Correct.

>>> I see a few calls to add_local_relative and add_global_relative that
>>> are not passing down elfcpp::R_SPARC_RELATIVE as the reloc type.  That
>>> is rather suspicious and seems like it could cause this sort of
>>> problem.
>>
>> It has nothing to do with these crashes.
> 
> It is a problem, though, because it will cause DT_RELACOUNT to be
> incorrect.

Ok.  I do remember there was a reason I gave non-R_SPARC_RELATIVE
relocations to that function.  I needed the calculations it
performed, even for non-R_SPARC_RELATIVE relocs.

I'll look into this, it probably explains some of the failures
I still get (about 12) after fixing the pltrel-overlap issue.

>> I'm playing around with the following patch to fix this:
> 
> Looks like a reasonable approach.  I prefer boolean variables to have
> names which read correctly with if, so perhaps dynrel_includes_plt
> instead of of pltrel_overlap.  Also no need to duplicate the entire
> union in Output_data_dynamic.
> 
> Another approach would be to in fact put the PLT into the .rela.dyn
> output section.  That could be done by just changing the call to
> layout->add_output_section_data to use .rela.dyn in both SHT_RELA
> calls in sparc.cc.

Thanks for the suggestions, I'll work on finalizing this fix
and will post a new version.

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: Ian Lance Taylor <iant@google.com>
Date: Tue, 09 Feb 2010 07:04:13 -0800

> Another approach would be to in fact put the PLT into the .rela.dyn
> output section.  That could be done by just changing the call to
> layout->add_output_section_data to use .rela.dyn in both SHT_RELA
> calls in sparc.cc.

So I tried this, and it doesn't work, even if I force the .rela.dyn to
get created before the PLT relocation section.

The dynamic linker doesn't process the PLT relocations properly.

Re: bug: GOLD handling of Sparc PLTREL

[Ian Lance Taylor]

David Miller <davem@davemloft.net> writes:

> From: Ian Lance Taylor <iant@google.com>
> Date: Tue, 09 Feb 2010 07:04:13 -0800
>
>> Another approach would be to in fact put the PLT into the .rela.dyn
>> output section.  That could be done by just changing the call to
>> layout->add_output_section_data to use .rela.dyn in both SHT_RELA
>> calls in sparc.cc.
>
> So I tried this, and it doesn't work, even if I force the .rela.dyn to
> get created before the PLT relocation section.
>
> The dynamic linker doesn't process the PLT relocations properly.

You may need to change Layout::add_target_dynamic_tags to
dyn_rel->output_section().

Ian

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: Ian Lance Taylor <iant@google.com>
Date: Tue, 09 Feb 2010 10:01:14 -0800

> You may need to change Layout::add_target_dynamic_tags to
> dyn_rel->output_section().

I'll give that a try after what I'm doing right now, which
is fixing up those bogus ->*_relative() calls you noticed.

Thanks.

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: Ian Lance Taylor <iant@google.com>
Date: Tue, 09 Feb 2010 07:04:13 -0800

> David Miller <davem@davemloft.net> writes:
> 
>>> I see a few calls to add_local_relative and add_global_relative that
>>> are not passing down elfcpp::R_SPARC_RELATIVE as the reloc type.  That
>>> is rather suspicious and seems like it could cause this sort of
>>> problem.
>>
>> It has nothing to do with these crashes.
> 
> It is a problem, though, because it will cause DT_RELACOUNT to be
> incorrect.

How does this look?

gold/

2010-02-09  David S. Miller  <davem@davemloft.net>

	* sparc.cc (Target_sparc::Scan::local): Do not emit relocs other than
	R_SPARC_RELATIVE using ->add_local_relative().
	(Target_sparc::Scan::global): Likewise for ->add_global_relative().

diff --git a/gold/sparc.cc b/gold/sparc.cc
index d3c9435..234c5f5 100644
--- a/gold/sparc.cc
+++ b/gold/sparc.cc
@@ -1675,23 +1675,31 @@ Target_sparc<size, big_endian>::Scan::local(
       if (parameters->options().output_is_position_independent())
         {
           Reloc_section* rela_dyn = target->rela_dyn_section(layout);
+          unsigned int r_sym = elfcpp::elf_r_sym<size>(reloc.get_r_info());
 
 	  check_non_pic(object, r_type);
           if (lsym.get_st_type() != elfcpp::STT_SECTION)
             {
-              unsigned int r_sym = elfcpp::elf_r_sym<size>(reloc.get_r_info());
               rela_dyn->add_local(object, r_sym, orig_r_type, output_section,
 				  data_shndx, reloc.get_r_offset(),
 				  reloc.get_r_addend());
             }
           else
             {
-	      unsigned int r_sym = elfcpp::elf_r_sym<size>(reloc.get_r_info());
+	      unsigned int shndx = lsym.get_st_shndx();
+	      bool is_ordinary;
+
               gold_assert(lsym.get_st_value() == 0);
-              rela_dyn->add_local_relative(object, r_sym, orig_r_type,
-					   output_section, data_shndx,
-					   reloc.get_r_offset(),
-					   reloc.get_r_addend());
+	      shndx = object->adjust_sym_shndx(r_sym, shndx,
+					       &is_ordinary);
+	      if (!is_ordinary)
+		object->error(_("section symbol %u has bad shndx %u"),
+			      r_sym, shndx);
+	      else
+		rela_dyn->add_local_section(object, shndx,
+					    r_type, output_section,
+					    data_shndx, reloc.get_r_offset(),
+					    reloc.get_r_addend());
             }
         }
       break;
@@ -1839,15 +1847,13 @@ Target_sparc<size, big_endian>::Scan::local(
 		if (!object->local_has_got_offset(r_sym, GOT_TYPE_TLS_OFFSET))
 		  {
 		    Reloc_section* rela_dyn = target->rela_dyn_section(layout);
-		    unsigned int off = got->add_constant(0);
-
-		    object->set_local_got_offset(r_sym, GOT_TYPE_TLS_OFFSET,
-						 off);
-		    rela_dyn->add_local_relative(object, r_sym,
-						 (size == 64 ?
-						  elfcpp::R_SPARC_TLS_TPOFF64 :
-						  elfcpp::R_SPARC_TLS_TPOFF32),
-						 got, off, 0);
+
+		    got->add_local_with_rela(object, r_sym,
+					     GOT_TYPE_TLS_OFFSET,
+					     rela_dyn,
+					     (size == 64 ?
+					      elfcpp::R_SPARC_TLS_TPOFF64 :
+					      elfcpp::R_SPARC_TLS_TPOFF32));
 		  }
 	      }
 	    else if (optimized_type != tls::TLSOPT_TO_LE)
@@ -1863,9 +1869,9 @@ Target_sparc<size, big_endian>::Scan::local(
                 gold_assert(lsym.get_st_type() != elfcpp::STT_SECTION);
                 unsigned int r_sym = elfcpp::elf_r_sym<size>(reloc.get_r_info());
                 Reloc_section* rela_dyn = target->rela_dyn_section(layout);
-                rela_dyn->add_local_relative(object, r_sym, r_type,
-					     output_section, data_shndx,
-					     reloc.get_r_offset(), 0);
+                rela_dyn->add_local(object, r_sym, r_type,
+				    output_section, data_shndx,
+				    reloc.get_r_offset(), 0);
 	      }
 	    break;
 	  }
@@ -2060,19 +2066,10 @@ Target_sparc<size, big_endian>::Scan::global(
                 Reloc_section* rela_dyn = target->rela_dyn_section(layout);
 
 		check_non_pic(object, r_type);
-		if (gsym->is_from_dynobj()
-		    || gsym->is_undefined()
-		    || gsym->is_preemptible())
-		  rela_dyn->add_global(gsym, orig_r_type, output_section,
-				       object, data_shndx,
-				       reloc.get_r_offset(),
-				       reloc.get_r_addend());
-		else
-		  rela_dyn->add_global_relative(gsym, orig_r_type,
-						output_section, object,
-						data_shndx,
-						reloc.get_r_offset(),
-						reloc.get_r_addend());
+		rela_dyn->add_global(gsym, orig_r_type, output_section,
+				     object, data_shndx,
+				     reloc.get_r_offset(),
+				     reloc.get_r_addend());
               }
           }
       }
@@ -2204,10 +2201,10 @@ Target_sparc<size, big_endian>::Scan::global(
 	    if (parameters->options().shared())
 	      {
 		Reloc_section* rela_dyn = target->rela_dyn_section(layout);
-		rela_dyn->add_global_relative(gsym, orig_r_type,
-					      output_section, object,
-					      data_shndx, reloc.get_r_offset(),
-					      0);
+		rela_dyn->add_global(gsym, orig_r_type,
+				     output_section, object,
+				     data_shndx, reloc.get_r_offset(),
+				     0);
 	      }
 	    break;
 

Re: bug: GOLD handling of Sparc PLTREL

[Ian Lance Taylor]

David Miller <davem@davemloft.net> writes:

> 2010-02-09  David S. Miller  <davem@davemloft.net>
>
> 	* sparc.cc (Target_sparc::Scan::local): Do not emit relocs other than
> 	R_SPARC_RELATIVE using ->add_local_relative().
> 	(Target_sparc::Scan::global): Likewise for ->add_global_relative().

This is OK.

Thanks.

Ian

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: Ian Lance Taylor <iant@google.com>
Date: Tue, 09 Feb 2010 10:01:14 -0800

> David Miller <davem@davemloft.net> writes:
> 
>> From: Ian Lance Taylor <iant@google.com>
>> Date: Tue, 09 Feb 2010 07:04:13 -0800
>>
>>> Another approach would be to in fact put the PLT into the .rela.dyn
>>> output section.  That could be done by just changing the call to
>>> layout->add_output_section_data to use .rela.dyn in both SHT_RELA
>>> calls in sparc.cc.
>>
>> So I tried this, and it doesn't work, even if I force the .rela.dyn to
>> get created before the PLT relocation section.
>>
>> The dynamic linker doesn't process the PLT relocations properly.
> 
> You may need to change Layout::add_target_dynamic_tags to
> dyn_rel->output_section().

Still fails the same way, even if I do this.

Let's stick to the original game plan.

Ok to commit?

I'll look now at the remaining 11 sparc gold testsuite failures.

gold/

2010-02-08  David S. Miller  <davem@davemloft.net>

	* output.h (Output_data_dynamic::add_section_size): New method
	that takes two Output_data objects.
	(Output_data_dynamic::Dynamic_entry): Create storage for secondary
	entry param.  Handle it in initializers.
	* output.cc (Output_data_dynamic::Dynamic_entry::write): For
	DYNAMIC_SECTION_SIZE, add in second object size if non-NULL.
	* layout.h (Layout::add_target_dynamic_tags): Add dynrel_includes_plt
	arg.
	* layout.cc (Layout::add_target_dynamic_tags): If dynrel_includes_plt,
	and .rela.plt exists, set DT_REL{,A}SZ to sum of .rela.dyn and .rela.plt
	* arm.cc (Target_arm::do_finalize_sections): Update to pass false
	for dynrel_includes_plt.
	* i386.cc (Target_i386::do_finalize_sections): Likewise.
	* x86_64.cc (Target_x86_64::do_finalize_sections): Likewise.
	* sparc.cc (Target_sparc::make_plt_entry): Force .rela.dyn to be output
	before .rela.plt
	(Target_sparc::do_finalize_sections): Update to pass true for
	dynrel_includes_plt.
	* powerpc.cc (Target_powerpc::make_plt_entry): Force .rela.dyn to be
	output before .rela.plt
	(Target_powerpc::do_finalize_sections): Update to pass true for
	dynrel_includes_plt when 32-bit.
---
 gold/arm.cc     |    2 +-
 gold/i386.cc    |    2 +-
 gold/layout.cc  |   10 +++++++---
 gold/layout.h   |    2 +-
 gold/output.cc  |    2 ++
 gold/output.h   |   16 +++++++++++++++-
 gold/powerpc.cc |    7 ++++++-
 gold/sparc.cc   |    7 ++++++-
 gold/x86_64.cc  |    2 +-
 9 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/gold/arm.cc b/gold/arm.cc
index f121f93..902805c 100644
--- a/gold/arm.cc
+++ b/gold/arm.cc
@@ -7094,7 +7094,7 @@ Target_arm<big_endian>::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rel_plt());
   layout->add_target_dynamic_tags(true, this->got_plt_, rel_plt,
-				  this->rel_dyn_, true);
+				  this->rel_dyn_, true, false);
 
   // Emit any relocs we saved in an attempt to avoid generating COPY
   // relocs.
diff --git a/gold/i386.cc b/gold/i386.cc
index 2eab3f8..f2a7b53 100644
--- a/gold/i386.cc
+++ b/gold/i386.cc
@@ -1609,7 +1609,7 @@ Target_i386::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rel_plt());
   layout->add_target_dynamic_tags(true, this->got_plt_, rel_plt,
-				  this->rel_dyn_, true);
+				  this->rel_dyn_, true, false);
 
   // Emit any relocs we saved in an attempt to avoid generating COPY
   // relocs.
diff --git a/gold/layout.cc b/gold/layout.cc
index 52989a5..faa0c72 100644
--- a/gold/layout.cc
+++ b/gold/layout.cc
@@ -3240,7 +3240,7 @@ void
 Layout::add_target_dynamic_tags(bool use_rel, const Output_data* plt_got,
 				const Output_data* plt_rel,
 				const Output_data_reloc_generic* dyn_rel,
-				bool add_debug)
+				bool add_debug, bool dynrel_includes_plt)
 {
   Output_data_dynamic* odyn = this->dynamic_data_;
   if (odyn == NULL)
@@ -3261,8 +3261,12 @@ Layout::add_target_dynamic_tags(bool use_rel, const Output_data* plt_got,
     {
       odyn->add_section_address(use_rel ? elfcpp::DT_REL : elfcpp::DT_RELA,
 				dyn_rel);
-      odyn->add_section_size(use_rel ? elfcpp::DT_RELSZ : elfcpp::DT_RELASZ,
-			     dyn_rel);
+      if (plt_rel != NULL && dynrel_includes_plt)
+	odyn->add_section_size(use_rel ? elfcpp::DT_RELSZ : elfcpp::DT_RELASZ,
+			       dyn_rel, plt_rel);
+      else
+	odyn->add_section_size(use_rel ? elfcpp::DT_RELSZ : elfcpp::DT_RELASZ,
+			       dyn_rel);
       const int size = parameters->target().get_size();
       elfcpp::DT rel_tag;
       int rel_size;
diff --git a/gold/layout.h b/gold/layout.h
index 15e7548..cd15c98 100644
--- a/gold/layout.h
+++ b/gold/layout.h
@@ -563,7 +563,7 @@ class Layout
   add_target_dynamic_tags(bool use_rel, const Output_data* plt_got,
 			  const Output_data* plt_rel,
 			  const Output_data_reloc_generic* dyn_rel,
-			  bool add_debug);
+			  bool add_debug, bool dynrel_includes_plt);
 
   // Compute and write out the build ID if needed.
   void
diff --git a/gold/output.cc b/gold/output.cc
index a8f03e7..5c4cb22 100644
--- a/gold/output.cc
+++ b/gold/output.cc
@@ -1560,6 +1560,8 @@ Output_data_dynamic::Dynamic_entry::write(
 
     case DYNAMIC_SECTION_SIZE:
       val = this->u_.od->data_size();
+      if (this->od2)
+	val += this->od2->data_size();
       break;
 
     case DYNAMIC_SYMBOL:
diff --git a/gold/output.h b/gold/output.h
index 60d57de..6a9b1a2 100644
--- a/gold/output.h
+++ b/gold/output.h
@@ -1997,6 +1997,12 @@ class Output_data_dynamic : public Output_section_data
   add_section_size(elfcpp::DT tag, const Output_data* od)
   { this->add_entry(Dynamic_entry(tag, od, true)); }
 
+  // Add a new dynamic entry with the total size of two output datas.
+  void
+  add_section_size(elfcpp::DT tag, const Output_data* od,
+		   const Output_data* od2)
+  { this->add_entry(Dynamic_entry(tag, od, od2)); }
+
   // Add a new dynamic entry with the address of a symbol.
   void
   add_symbol(elfcpp::DT tag, const Symbol* sym)
@@ -2045,7 +2051,13 @@ class Output_data_dynamic : public Output_section_data
 	offset_(section_size
 		? DYNAMIC_SECTION_SIZE
 		: DYNAMIC_SECTION_ADDRESS)
-    { this->u_.od = od; }
+    { this->u_.od = od; this->od2 = NULL; }
+
+    // Create an entry with the size of two sections.
+    Dynamic_entry(elfcpp::DT tag, const Output_data* od, const Output_data* od2)
+      : tag_(tag),
+	offset_(DYNAMIC_SECTION_SIZE)
+    { this->u_.od = od; this->od2 = od2; }
 
     // Create an entry with the address of a section plus a constant offset.
     Dynamic_entry(elfcpp::DT tag, const Output_data* od, unsigned int offset)
@@ -2101,6 +2113,8 @@ class Output_data_dynamic : public Output_section_data
       // For DYNAMIC_STRING.
       const char* str;
     } u_;
+    // For DYNAMIC_SYMBOL with two sections.
+    const Output_data* od2;
     // The dynamic tag.
     elfcpp::DT tag_;
     // The type of entry (Classification) or offset within a section.
diff --git a/gold/powerpc.cc b/gold/powerpc.cc
index 83bb992..cc46782 100644
--- a/gold/powerpc.cc
+++ b/gold/powerpc.cc
@@ -948,6 +948,11 @@ Target_powerpc<size, big_endian>::make_plt_entry(Symbol_table* symtab,
       // Create the GOT section first.
       this->got_section(symtab, layout);
 
+      // Ensure that .rela.dyn always appears before .rela.plt  This is
+      // necessary due to how, on PowerPC and some other targets, .rela.dyn
+      // needs to include .rela.plt in it's range.
+      this->rela_dyn_section(layout);
+
       this->plt_ = new Output_data_plt_powerpc<size, big_endian>(layout);
       layout->add_output_section_data(".plt", elfcpp::SHT_PROGBITS,
 				      (elfcpp::SHF_ALLOC
@@ -1556,7 +1561,7 @@ Target_powerpc<size, big_endian>::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rel_plt());
   layout->add_target_dynamic_tags(false, this->plt_, rel_plt,
-				  this->rela_dyn_, true);
+				  this->rela_dyn_, true, size == 32);
 
   // Emit any relocs we saved in an attempt to avoid generating COPY
   // relocs.
diff --git a/gold/sparc.cc b/gold/sparc.cc
index 9bca176..d3c9435 100644
--- a/gold/sparc.cc
+++ b/gold/sparc.cc
@@ -1369,6 +1369,11 @@ Target_sparc<size, big_endian>::make_plt_entry(Symbol_table* symtab,
       // Create the GOT sections first.
       this->got_section(symtab, layout);
 
+      // Ensure that .rela.dyn always appears before .rela.plt  This is
+      // necessary due to how, on Sparc and some other targets, .rela.dyn
+      // needs to include .rela.plt in it's range.
+      this->rela_dyn_section(layout);
+
       this->plt_ = new Output_data_plt_sparc<size, big_endian>(layout);
       layout->add_output_section_data(".plt", elfcpp::SHT_PROGBITS,
 				      (elfcpp::SHF_ALLOC
@@ -2341,7 +2346,7 @@ Target_sparc<size, big_endian>::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rel_plt());
   layout->add_target_dynamic_tags(false, this->plt_, rel_plt,
-				  this->rela_dyn_, true);
+				  this->rela_dyn_, true, true);
 
   // Emit any relocs we saved in an attempt to avoid generating COPY
   // relocs.
diff --git a/gold/x86_64.cc b/gold/x86_64.cc
index 585a499..fea2ec9 100644
--- a/gold/x86_64.cc
+++ b/gold/x86_64.cc
@@ -1741,7 +1741,7 @@ Target_x86_64::do_finalize_sections(
 				  ? NULL
 				  : this->plt_->rela_plt());
   layout->add_target_dynamic_tags(false, this->got_plt_, rel_plt,
-				  this->rela_dyn_, true);
+				  this->rela_dyn_, true, false);
 				  
   // Fill in some more dynamic tags.
   Output_data_dynamic* const odyn = layout->dynamic_data();
-- 
1.6.6.1

Re: bug: GOLD handling of Sparc PLTREL

[Ian Lance Taylor]

David Miller <davem@davemloft.net> writes:

> 2010-02-08  David S. Miller  <davem@davemloft.net>
>
> 	* output.h (Output_data_dynamic::add_section_size): New method
> 	that takes two Output_data objects.
> 	(Output_data_dynamic::Dynamic_entry): Create storage for secondary
> 	entry param.  Handle it in initializers.
> 	* output.cc (Output_data_dynamic::Dynamic_entry::write): For
> 	DYNAMIC_SECTION_SIZE, add in second object size if non-NULL.
> 	* layout.h (Layout::add_target_dynamic_tags): Add dynrel_includes_plt
> 	arg.
> 	* layout.cc (Layout::add_target_dynamic_tags): If dynrel_includes_plt,
> 	and .rela.plt exists, set DT_REL{,A}SZ to sum of .rela.dyn and .rela.plt
> 	* arm.cc (Target_arm::do_finalize_sections): Update to pass false
> 	for dynrel_includes_plt.
> 	* i386.cc (Target_i386::do_finalize_sections): Likewise.
> 	* x86_64.cc (Target_x86_64::do_finalize_sections): Likewise.
> 	* sparc.cc (Target_sparc::make_plt_entry): Force .rela.dyn to be output
> 	before .rela.plt
> 	(Target_sparc::do_finalize_sections): Update to pass true for
> 	dynrel_includes_plt.
> 	* powerpc.cc (Target_powerpc::make_plt_entry): Force .rela.dyn to be
> 	output before .rela.plt
> 	(Target_powerpc::do_finalize_sections): Update to pass true for
> 	dynrel_includes_plt when 32-bit.


> +      if (this->od2)
> +	val += this->od2->data_size();

Write this->od2 != NULL.

> @@ -2045,7 +2051,13 @@ class Output_data_dynamic : public Output_section_data
>  	offset_(section_size
>  		? DYNAMIC_SECTION_SIZE
>  		: DYNAMIC_SECTION_ADDRESS)
> -    { this->u_.od = od; }
> +    { this->u_.od = od; this->od2 = NULL; }

Use two separate lines.

> +    Dynamic_entry(elfcpp::DT tag, const Output_data* od, const Output_data* od2)
> +      : tag_(tag),
> +	offset_(DYNAMIC_SECTION_SIZE)
> +    { this->u_.od = od; this->od2 = od2; }

Here too.


This is OK with those changes.

Thanks.

Ian

Re: bug: GOLD handling of Sparc PLTREL

[David Miller]

From: Ian Lance Taylor <iant@google.com>
Date: Tue, 09 Feb 2010 12:12:01 -0800

> David Miller <davem@davemloft.net> writes:
> 
>> +      if (this->od2)
>> +	val += this->od2->data_size();
> 
> Write this->od2 != NULL.
> 
>> @@ -2045,7 +2051,13 @@ class Output_data_dynamic : public Output_section_data
>>  	offset_(section_size
>>  		? DYNAMIC_SECTION_SIZE
>>  		: DYNAMIC_SECTION_ADDRESS)
>> -    { this->u_.od = od; }
>> +    { this->u_.od = od; this->od2 = NULL; }
> 
> Use two separate lines.
> 
>> +    Dynamic_entry(elfcpp::DT tag, const Output_data* od, const Output_data* od2)
>> +      : tag_(tag),
>> +	offset_(DYNAMIC_SECTION_SIZE)
>> +    { this->u_.od = od; this->od2 = od2; }
> 
> Here too.
> 
> 
> This is OK with those changes.

Will do, thanks for reviewing Ian.