.reloc improvement

.reloc improvement

[Alan Modra]

This patch makes some improvements to relocations emitted by the
.reloc directive:
1) A symbol on these relocs is converted to section symbol plus offset
   if the symbol satisifes S_IS_LOCAL and some section related
   conditions.  For example, forward/backward labels like "1f", and
   ELF locals like .L123 will be converted.  This helps keep the
   symbol table tidy.
2) These relocations are merge sorted with the normal fixup relocs,
   instead of being emitted after the fixup relocs.  A merge sort
   ought to be sufficient;  If someone generates .reloc with offsets
   out of order then I'd say they obviously don't care about reloc
   sorting!
3) Time taken to find .reloc frags should be considerably better
   than it used to be.

	* write.c (resolve_reloc_expr_symbols): Convert local symbols
	on relocs to section+offset.
	(get_frag_for_reloc): New function.
	(write_relocs): Merge sort fixup relocs with those from .reloc
	directives.

Index: gas/write.c
===================================================================
RCS file: /cvs/src/src/gas/write.c,v
retrieving revision 1.144
diff -u -p -r1.144 write.c
--- gas/write.c	4 Aug 2011 20:53:58 -0000	1.144
+++ gas/write.c	18 Aug 2011 13:25:23 -0000
@@ -708,7 +708,20 @@ resolve_reloc_expr_symbols (void)
 	      sec = NULL;
 	    }
 	  else if (sym != NULL)
-	    symbol_mark_used_in_reloc (sym);
+	    {
+	      if (S_IS_LOCAL (sym) && !symbol_section_p (sym))
+		{
+		  asection *symsec = S_GET_SEGMENT (sym);
+		  if (!(((symsec->flags & SEC_MERGE) != 0
+			 && addend != 0)
+			|| (symsec->flags & SEC_THREAD_LOCAL) != 0))
+		    {
+		      addend += S_GET_VALUE (sym);
+		      sym = section_symbol (symsec);
+		    }
+		}
+	      symbol_mark_used_in_reloc (sym);
+	    }
 	}
       if (sym == NULL)
 	{
@@ -1146,15 +1159,37 @@ install_reloc (asection *sec, arelent *r
     }
 }
 
+static fragS *
+get_frag_for_reloc (fragS *last_frag,
+		    const segment_info_type *seginfo,
+		    const struct reloc_list *r)
+{
+  fragS *f;
+  
+  for (f = last_frag; f != NULL; f = f->fr_next)
+    if (f->fr_address <= r->u.b.r.address
+	&& r->u.b.r.address < f->fr_address + f->fr_fix)
+      return f;
+
+  for (f = seginfo->frchainP->frch_root; f != NULL; f = f->fr_next)
+    if (f->fr_address <= r->u.b.r.address
+	&& r->u.b.r.address < f->fr_address + f->fr_fix)
+      return f;
+
+  as_bad_where (r->file, r->line,
+		_("reloc not within (fixed part of) section"));
+  return NULL;
+}
+
 static void
 write_relocs (bfd *abfd, asection *sec, void *xxx ATTRIBUTE_UNUSED)
 {
   segment_info_type *seginfo = seg_info (sec);
-  unsigned int i;
   unsigned int n;
   struct reloc_list *my_reloc_list, **rp, *r;
   arelent **relocs;
   fixS *fixp;
+  fragS *last_frag;
 
   /* If seginfo is NULL, we did not create this section; don't do
      anything with it.  */
@@ -1188,12 +1223,19 @@ write_relocs (bfd *abfd, asection *sec, 
 
   relocs = (arelent **) xcalloc (n, sizeof (arelent *));
 
-  i = 0;
+  n = 0;
+  r = my_reloc_list;
+  last_frag = NULL;
   for (fixp = seginfo->fix_root; fixp != (fixS *) NULL; fixp = fixp->fx_next)
     {
-      int j;
       int fx_size, slack;
       offsetT loc;
+      arelent **reloc;
+#ifndef RELOC_EXPANSION_POSSIBLE
+      arelent *rel;
+
+      reloc = &rel;
+#endif
 
       if (fixp->fx_done)
 	continue;
@@ -1208,28 +1250,46 @@ write_relocs (bfd *abfd, asection *sec, 
 		      _("internal error: fixup not contained within frag"));
 
 #ifndef RELOC_EXPANSION_POSSIBLE
-      {
-	arelent *reloc = tc_gen_reloc (sec, fixp);
-
-	if (!reloc)
-	  continue;
-	relocs[i++] = reloc;
-	j = 1;
-      }
+      *reloc = tc_gen_reloc (sec, fixp);
 #else
-      {
-	arelent **reloc = tc_gen_reloc (sec, fixp);
+      reloc = tc_gen_reloc (sec, fixp);
+#endif
 
-	for (j = 0; reloc[j]; j++)
-	  relocs[i++] = reloc[j];
-      }
+      while (*reloc)
+	{
+	  while (r != NULL && r->u.b.r.address < (*reloc)->address)
+	    {
+	      fragS *f = get_frag_for_reloc (last_frag, seginfo, r);
+	      if (f != NULL)
+		{
+		  last_frag = f;
+		  relocs[n++] = &r->u.b.r;
+		  install_reloc (sec, &r->u.b.r, f, r->file, r->line);
+		}
+	      r = r->next;
+	    }
+	  relocs[n++] = *reloc;
+	  install_reloc (sec, *reloc, fixp->fx_frag,
+			 fixp->fx_file, fixp->fx_line);
+#ifndef RELOC_EXPANSION_POSSIBLE
+	  break;
+#else
+	  reloc++;
 #endif
+	}
+    }
 
-      for ( ; j != 0; --j)
-	install_reloc (sec, relocs[i - j], fixp->fx_frag,
-		       fixp->fx_file, fixp->fx_line);
+  while (r != NULL)
+    {
+      fragS *f = get_frag_for_reloc (last_frag, seginfo, r);
+      if (f != NULL)
+	{
+	  last_frag = f;
+	  relocs[n++] = &r->u.b.r;
+	  install_reloc (sec, &r->u.b.r, f, r->file, r->line);
+	}
+      r = r->next;
     }
-  n = i;
 
 #ifdef DEBUG4
   {
@@ -1249,23 +1309,6 @@ write_relocs (bfd *abfd, asection *sec, 
   }
 #endif
 
-  for (r = my_reloc_list; r != NULL; r = r->next)
-    {
-      fragS *f;
-      for (f = seginfo->frchainP->frch_root; f; f = f->fr_next)
-	if (f->fr_address <= r->u.b.r.address
-	    && r->u.b.r.address < f->fr_address + f->fr_fix)
-	  break;
-      if (f == NULL)
-	as_bad_where (r->file, r->line,
-		      _("reloc not within (fixed part of) section"));
-      else
-	{
-	  relocs[n++] = &r->u.b.r;
-	  install_reloc (sec, &r->u.b.r, f, r->file, r->line);
-	}
-    }
-
   if (n)
     {
       flagword flags = bfd_get_section_flags (abfd, sec);

-- 
Alan Modra
Australia Development Lab, IBM

Re: .reloc improvement

[Richard Sandiford]

Alan Modra <amodra@gmail.com> writes:
> This patch makes some improvements to relocations emitted by the
> .reloc directive:
> 1) A symbol on these relocs is converted to section symbol plus offset
>    if the symbol satisifes S_IS_LOCAL and some section related
>    conditions.  For example, forward/backward labels like "1f", and
>    ELF locals like .L123 will be converted.  This helps keep the
>    symbol table tidy.
> 2) These relocations are merge sorted with the normal fixup relocs,
>    instead of being emitted after the fixup relocs.  A merge sort
>    ought to be sufficient;  If someone generates .reloc with offsets
>    out of order then I'd say they obviously don't care about reloc
>    sorting!
> 3) Time taken to find .reloc frags should be considerably better
>    than it used to be.
>
> 	* write.c (resolve_reloc_expr_symbols): Convert local symbols
> 	on relocs to section+offset.
> 	(get_frag_for_reloc): New function.
> 	(write_relocs): Merge sort fixup relocs with those from .reloc
> 	directives.

Thanks for doing this.  The relaxation might need to be conditional
for some REL targets though.  E.g. one of the main .reloc uses for
MIPS is to add an R_MIPS_JALR hint to say where an indirect PIC call
actually goes.  There's no room in the instruction itself for an
in-place addend, so on REL targets, the addend needs to be zero.

It's very unlikely anyone would use R_MIPS_JALR with local labels,
but the same might apply to other relocs (especially PC-relative relocs)
for which the relocation field is smaller than an address.

Maybe one way would be to create a fake, temporary fixup and pass
that to tc_fix_adjustable.  Doesn't feel very clean though. :-)

Richard

Re: .reloc improvement

[Alan Modra]

On Sat, Aug 20, 2011 at 07:37:40PM +0100, Richard Sandiford wrote:
> Alan Modra <amodra@gmail.com> writes:
> > This patch makes some improvements to relocations emitted by the
> > .reloc directive:
> > 1) A symbol on these relocs is converted to section symbol plus offset
> >    if the symbol satisifes S_IS_LOCAL and some section related
> >    conditions.  For example, forward/backward labels like "1f", and
> >    ELF locals like .L123 will be converted.  This helps keep the
> >    symbol table tidy.
> 
> Thanks for doing this.  The relaxation might need to be conditional
> for some REL targets though.

Yes, it was this sort of consideration that led me to choose to only
substitute the section symbol for symbols that satisfy S_IS_LOCAL,
rather than the larger set handled by adjust_reloc_symbols.  I know it
potentially breaks existing code, but hopefully I won't be hit by too
many brickbats..

[snip]
> Maybe one way would be to create a fake, temporary fixup and pass
> that to tc_fix_adjustable.

This might not be such a bad idea.  If resolve_reloc_expr_symbols
found the frags and inserted a fake fixup in the section fixup chain,
then the rest of the fixup handling machinery might even work for
.reloc.

-- 
Alan Modra
Australia Development Lab, IBM

Re: .reloc improvement

[Maciej W. Rozycki]

On Sun, 21 Aug 2011, Alan Modra wrote:

> > > This patch makes some improvements to relocations emitted by the
> > > .reloc directive:
> > > 1) A symbol on these relocs is converted to section symbol plus offset
> > >    if the symbol satisifes S_IS_LOCAL and some section related
> > >    conditions.  For example, forward/backward labels like "1f", and
> > >    ELF locals like .L123 will be converted.  This helps keep the
> > >    symbol table tidy.
> > 
> > Thanks for doing this.  The relaxation might need to be conditional
> > for some REL targets though.
> 
> Yes, it was this sort of consideration that led me to choose to only
> substitute the section symbol for symbols that satisfy S_IS_LOCAL,
> rather than the larger set handled by adjust_reloc_symbols.  I know it
> potentially breaks existing code, but hopefully I won't be hit by too
> many brickbats..
> 
> [snip]
> > Maybe one way would be to create a fake, temporary fixup and pass
> > that to tc_fix_adjustable.
> 
> This might not be such a bad idea.  If resolve_reloc_expr_symbols
> found the frags and inserted a fake fixup in the section fixup chain,
> then the rest of the fixup handling machinery might even work for
> .reloc.

 Please note that on REL MIPS targets local symbols have to be retained in 
several cases where crucial information is lost if a relocation against 
such a symbol is converted to one against the containing section's symbol.  
This applies to all the PC-relative relocations, where the relocatable 
field is too narrow to hold arbitrary addends, and also, more recently, to 
microMIPS targets, where linker relaxation has to know local symbol 
(label) addresses to perform branch displacement recalculations and to 
make the LUI/ADDIU->ADDIUPC relaxation.  The latter is a linker's internal 
limitation and may possibly be lifted (possibly by using the RELA 
representation internally even on REL targets), but the former is an 
inherent problem of the object file format used.

 Just making sure these issues are not missed -- I can't have a look at 
your change at the moment and see what exact implications it has, sorry.

  Maciej

Re: .reloc improvement

[Maciej W. Rozycki]

On Sun, 21 Aug 2011, Alan Modra wrote:

> > > This patch makes some improvements to relocations emitted by the
> > > .reloc directive:
> > > 1) A symbol on these relocs is converted to section symbol plus offset
> > >    if the symbol satisifes S_IS_LOCAL and some section related
> > >    conditions.  For example, forward/backward labels like "1f", and
> > >    ELF locals like .L123 will be converted.  This helps keep the
> > >    symbol table tidy.
> > 
> > Thanks for doing this.  The relaxation might need to be conditional
> > for some REL targets though.
> 
> Yes, it was this sort of consideration that led me to choose to only
> substitute the section symbol for symbols that satisfy S_IS_LOCAL,
> rather than the larger set handled by adjust_reloc_symbols.  I know it
> potentially breaks existing code, but hopefully I won't be hit by too
> many brickbats..
> 
> [snip]
> > Maybe one way would be to create a fake, temporary fixup and pass
> > that to tc_fix_adjustable.
> 
> This might not be such a bad idea.  If resolve_reloc_expr_symbols
> found the frags and inserted a fake fixup in the section fixup chain,
> then the rest of the fixup handling machinery might even work for
> .reloc.

 Please note that on REL MIPS targets local symbols have to be retained in 
several cases where crucial information is lost if a relocation against 
such a symbol is converted to one against the containing section's symbol.  
This applies to all the PC-relative relocations, where the relocatable 
field is too narrow to hold arbitrary addends, and also, more recently, to 
microMIPS targets, where linker relaxation has to know local symbol 
(label) addresses to perform branch displacement recalculations and to 
make the LUI/ADDIU->ADDIUPC relaxation.  The latter is a linker's internal 
limitation and may possibly be lifted (possibly by using the RELA 
representation internally even on REL targets), but the former is an 
inherent problem of the object file format used.

 Just making sure these issues are not missed -- I can't have a look at 
your change at the moment and see what exact implications it has, sorry.

  Maciej

Re: .reloc improvement

[Maciej W. Rozycki]

On Sun, 21 Aug 2011, Alan Modra wrote:

> > > This patch makes some improvements to relocations emitted by the
> > > .reloc directive:
> > > 1) A symbol on these relocs is converted to section symbol plus offset
> > >    if the symbol satisifes S_IS_LOCAL and some section related
> > >    conditions.  For example, forward/backward labels like "1f", and
> > >    ELF locals like .L123 will be converted.  This helps keep the
> > >    symbol table tidy.
> > 
> > Thanks for doing this.  The relaxation might need to be conditional
> > for some REL targets though.
> 
> Yes, it was this sort of consideration that led me to choose to only
> substitute the section symbol for symbols that satisfy S_IS_LOCAL,
> rather than the larger set handled by adjust_reloc_symbols.  I know it
> potentially breaks existing code, but hopefully I won't be hit by too
> many brickbats..
> 
> [snip]
> > Maybe one way would be to create a fake, temporary fixup and pass
> > that to tc_fix_adjustable.
> 
> This might not be such a bad idea.  If resolve_reloc_expr_symbols
> found the frags and inserted a fake fixup in the section fixup chain,
> then the rest of the fixup handling machinery might even work for
> .reloc.

 Please note that on REL MIPS targets local symbols have to be retained in 
several cases where crucial information is lost if a relocation against 
such a symbol is converted to one against the containing section's symbol.  
This applies to all the PC-relative relocations, where the relocatable 
field is too narrow to hold arbitrary addends, and also, more recently, to 
microMIPS targets, where linker relaxation has to know local symbol 
(label) addresses to perform branch displacement recalculations and to 
make the LUI/ADDIU->ADDIUPC relaxation.  The latter is a linker's internal 
limitation and may possibly be lifted (possibly by using the RELA 
representation internally even on REL targets), but the former is an 
inherent problem of the object file format used.

 Just making sure these issues are not missed -- I can't have a look at 
your change at the moment and see what exact implications it has, sorry.

  Maciej

Re: .reloc improvement

[Alan Modra]

On Sun, Aug 21, 2011 at 06:07:32AM +0100, Maciej W. Rozycki wrote:
> On Sun, 21 Aug 2011, Alan Modra wrote:
> 
> > > > This patch makes some improvements to relocations emitted by the
> > > > .reloc directive:
> > > > 1) A symbol on these relocs is converted to section symbol plus offset
> > > >    if the symbol satisifes S_IS_LOCAL and some section related
> > > >    conditions.  For example, forward/backward labels like "1f", and
> > > >    ELF locals like .L123 will be converted.  This helps keep the
> > > >    symbol table tidy.
> > > 
> > > Thanks for doing this.  The relaxation might need to be conditional
> > > for some REL targets though.
> > 
> > Yes, it was this sort of consideration that led me to choose to only
> > substitute the section symbol for symbols that satisfy S_IS_LOCAL,
> > rather than the larger set handled by adjust_reloc_symbols.  I know it
> > potentially breaks existing code, but hopefully I won't be hit by too
> > many brickbats..
> > 
> > [snip]
> > > Maybe one way would be to create a fake, temporary fixup and pass
> > > that to tc_fix_adjustable.
> > 
> > This might not be such a bad idea.  If resolve_reloc_expr_symbols
> > found the frags and inserted a fake fixup in the section fixup chain,
> > then the rest of the fixup handling machinery might even work for
> > .reloc.

As I found when I started to implement this, the fly in the ointment
is that tc_fix_adjustable, md_apply_fix and so forth expect
fix->rx_r_type to be one of those BFD_RELOC_* enums, while .reloc
looks up reloc howtos.  howto->type is typically the external reloc
number, and we don't have a mapping from those to a BFD_RELOC_* enum.
Even if I did build such a mapping, not all of the extern reloc
numbers have a corresponding BFD_RELOC_* enum, and not all those that
do are handled by the various backend md_apply_fix functions.  :-(

>  Please note that on REL MIPS targets local symbols have to be retained in 
> several cases where crucial information is lost if a relocation against 
> such a symbol is converted to one against the containing section's symbol.  
> This applies to all the PC-relative relocations, where the relocatable 
> field is too narrow to hold arbitrary addends, and also, more recently, to 
> microMIPS targets, where linker relaxation has to know local symbol 
> (label) addresses to perform branch displacement recalculations and to 
> make the LUI/ADDIU->ADDIUPC relaxation.  The latter is a linker's internal 
> limitation and may possibly be lifted (possibly by using the RELA 
> representation internally even on REL targets), but the former is an 
> inherent problem of the object file format used.
> 
>  Just making sure these issues are not missed -- I can't have a look at 
> your change at the moment and see what exact implications it has, sorry.

The implication of my change is that the programmer will need to be
careful in choosing symbols used with .reloc.  While that isn't ideal,
.reloc is such a low-level interface that you need to know what you're
doing if you use it.  Alternatively, I'm quite willing to disable the
symbol to section symbol conversion for REL targets.

-- 
Alan Modra
Australia Development Lab, IBM

Re: .reloc improvement

[Maciej W. Rozycki]

On Mon, 22 Aug 2011, Alan Modra wrote:

> >  Please note that on REL MIPS targets local symbols have to be retained in 
> > several cases where crucial information is lost if a relocation against 
> > such a symbol is converted to one against the containing section's symbol.  
> > This applies to all the PC-relative relocations, where the relocatable 
> > field is too narrow to hold arbitrary addends, and also, more recently, to 
> > microMIPS targets, where linker relaxation has to know local symbol 
> > (label) addresses to perform branch displacement recalculations and to 
> > make the LUI/ADDIU->ADDIUPC relaxation.  The latter is a linker's internal 
> > limitation and may possibly be lifted (possibly by using the RELA 
> > representation internally even on REL targets), but the former is an 
> > inherent problem of the object file format used.
> > 
> >  Just making sure these issues are not missed -- I can't have a look at 
> > your change at the moment and see what exact implications it has, sorry.
> 
> The implication of my change is that the programmer will need to be
> careful in choosing symbols used with .reloc.  While that isn't ideal,
> .reloc is such a low-level interface that you need to know what you're
> doing if you use it.

 Problem is on REL targets some MIPS relocation types can never ever be 
safely converted to refer to a section symbol + addend instead of the 
intended symbol.  You'd therefore make .reloc useless for these types.

 Yes, REL shouldn't have been chosen for the MIPS ABI in the first place, 
with the complication observed here being the tip of an iceberg only, but 
there you go.  The choice was later fixed with the new ABIs made for 
64-bit MIPS processors, but the old ABI still remains for 32-bit ones.

> Alternatively, I'm quite willing to disable the symbol to section symbol 
> conversion for REL targets.

 Good idea, that sounds like a plan to me.  That could be made platform 
specific if that made anybody's life easier.

  Maciej

Re: .reloc improvement

[Maciej W. Rozycki]

Hi Alan,

On Tue, 6 Sep 2011, Maciej W. Rozycki wrote:

> On Mon, 22 Aug 2011, Alan Modra wrote:
> 
> > >  Please note that on REL MIPS targets local symbols have to be retained in 
> > > several cases where crucial information is lost if a relocation against 
> > > such a symbol is converted to one against the containing section's symbol.  
> > > This applies to all the PC-relative relocations, where the relocatable 
> > > field is too narrow to hold arbitrary addends, and also, more recently, to 
> > > microMIPS targets, where linker relaxation has to know local symbol 
> > > (label) addresses to perform branch displacement recalculations and to 
> > > make the LUI/ADDIU->ADDIUPC relaxation.  The latter is a linker's internal 
> > > limitation and may possibly be lifted (possibly by using the RELA 
> > > representation internally even on REL targets), but the former is an 
> > > inherent problem of the object file format used.
> > > 
> > >  Just making sure these issues are not missed -- I can't have a look at 
> > > your change at the moment and see what exact implications it has, sorry.
> > 
> > The implication of my change is that the programmer will need to be
> > careful in choosing symbols used with .reloc.  While that isn't ideal,
> > .reloc is such a low-level interface that you need to know what you're
> > doing if you use it.
> 
>  Problem is on REL targets some MIPS relocation types can never ever be 
> safely converted to refer to a section symbol + addend instead of the 
> intended symbol.  You'd therefore make .reloc useless for these types.
> 
>  Yes, REL shouldn't have been chosen for the MIPS ABI in the first place, 
> with the complication observed here being the tip of an iceberg only, but 
> there you go.  The choice was later fixed with the new ABIs made for 
> 64-bit MIPS processors, but the old ABI still remains for 32-bit ones.
> 
> > Alternatively, I'm quite willing to disable the symbol to section symbol 
> > conversion for REL targets.
> 
>  Good idea, that sounds like a plan to me.  That could be made platform 
> specific if that made anybody's life easier.

 Where are we with this problem?  Your change regressed this program (an 
excerpt from a test case I prepared a while ago and was about to integrate 
with our test suite):

$ cat reloc.s
	.text

	.fill	0x1000000

	.set	micromips
foo:
	nop32
	.reloc	., R_MICROMIPS_PC23_S2, 0f
	.fill	0
	addiu	$2, $pc, 8
	nop32
0:

from this:

$ mips-sde-elf-as -32 -o reloc.o reloc.s
$ mips-sde-elf-objdump -dr reloc.o

reloc.o:     file format elf32-tradbigmips


Disassembly of section .text:

00000000 <foo-0x1000000>:
	...

01000000 <foo>:
 1000000:	0000 0000 	nop
 1000004:	7900 0002 	addiu	v0,$pc,8
			1000004: R_MICROMIPS_PC23_S2	.L\x021
 1000008:	0000 0000 	nop

to this:

$ mips-sde-elf-as -o reloc.o reloc.s
reloc.s: Assembler messages:
reloc.s:8: Error: relocation overflow

  Maciej

Re: .reloc improvement

[Maciej W. Rozycki]

On Thu, 27 Oct 2011, Maciej W. Rozycki wrote:

> On Tue, 6 Sep 2011, Maciej W. Rozycki wrote:
> 
> > On Mon, 22 Aug 2011, Alan Modra wrote:
> > 
> > > >  Please note that on REL MIPS targets local symbols have to be retained in 
> > > > several cases where crucial information is lost if a relocation against 
> > > > such a symbol is converted to one against the containing section's symbol.  
> > > > This applies to all the PC-relative relocations, where the relocatable 
> > > > field is too narrow to hold arbitrary addends, and also, more recently, to 
> > > > microMIPS targets, where linker relaxation has to know local symbol 
> > > > (label) addresses to perform branch displacement recalculations and to 
> > > > make the LUI/ADDIU->ADDIUPC relaxation.  The latter is a linker's internal 
> > > > limitation and may possibly be lifted (possibly by using the RELA 
> > > > representation internally even on REL targets), but the former is an 
> > > > inherent problem of the object file format used.
> > > > 
> > > >  Just making sure these issues are not missed -- I can't have a look at 
> > > > your change at the moment and see what exact implications it has, sorry.
> > > 
> > > The implication of my change is that the programmer will need to be
> > > careful in choosing symbols used with .reloc.  While that isn't ideal,
> > > .reloc is such a low-level interface that you need to know what you're
> > > doing if you use it.
> > 
> >  Problem is on REL targets some MIPS relocation types can never ever be 
> > safely converted to refer to a section symbol + addend instead of the 
> > intended symbol.  You'd therefore make .reloc useless for these types.
> > 
> >  Yes, REL shouldn't have been chosen for the MIPS ABI in the first place, 
> > with the complication observed here being the tip of an iceberg only, but 
> > there you go.  The choice was later fixed with the new ABIs made for 
> > 64-bit MIPS processors, but the old ABI still remains for 32-bit ones.
> > 
> > > Alternatively, I'm quite willing to disable the symbol to section symbol 
> > > conversion for REL targets.
> > 
> >  Good idea, that sounds like a plan to me.  That could be made platform 
> > specific if that made anybody's life easier.
> 
>  Where are we with this problem?  Your change regressed this program (an 
> excerpt from a test case I prepared a while ago and was about to integrate 
> with our test suite):
> 
> $ cat reloc.s
> 	.text
> 
> 	.fill	0x1000000
> 
> 	.set	micromips
> foo:
> 	nop32
> 	.reloc	., R_MICROMIPS_PC23_S2, 0f
> 	.fill	0
> 	addiu	$2, $pc, 8
> 	nop32
> 0:
> 
> from this:
> 
> $ mips-sde-elf-as -32 -o reloc.o reloc.s
> $ mips-sde-elf-objdump -dr reloc.o
> 
> reloc.o:     file format elf32-tradbigmips
> 
> 
> Disassembly of section .text:
> 
> 00000000 <foo-0x1000000>:
> 	...
> 
> 01000000 <foo>:
>  1000000:	0000 0000 	nop
>  1000004:	7900 0002 	addiu	v0,$pc,8
> 			1000004: R_MICROMIPS_PC23_S2	.L\x021
>  1000008:	0000 0000 	nop
> 
> to this:
> 
> $ mips-sde-elf-as -o reloc.o reloc.s
> reloc.s: Assembler messages:
> reloc.s:8: Error: relocation overflow

 Long time, no response, so I went ahead and implemented this fix.  It 
unbreaks the test case for me and causes no regressions for mips-sde-elf 
(although I have no slightest idea how much coverage in the test suite 
.reloc has).

 OK to apply?  Since this is a regression OK for 2.22 too?

2011-11-14  Maciej W. Rozycki  <macro@codesourcery.com>

	gas/
	write.c (dump_section_relocs): Don't convert PC-relative relocs 
	that have an in-place addend narrower than the addresses used.

  Maciej

binutils-gas-reloc-pcrel-inplace.diff
Index: binutils-fsf-trunk-quilt/gas/write.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/gas/write.c	2011-11-14 13:50:51.000000000 +0000
+++ binutils-fsf-trunk-quilt/gas/write.c	2011-11-14 14:19:07.795860362 +0000
@@ -654,15 +654,21 @@ dump_section_relocs (bfd *abfd ATTRIBUTE
 static void
 resolve_reloc_expr_symbols (void)
 {
+  bfd_vma addr_mask = 1;
   struct reloc_list *r;
 
+  /* Avoid a shift by the width of type.  */
+  addr_mask <<= bfd_arch_bits_per_address (stdoutput) - 1;
+  addr_mask <<= 1;
+  addr_mask -= 1;
+
   for (r = reloc_list; r; r = r->next)
     {
+      reloc_howto_type *howto = r->u.a.howto;
       expressionS *symval;
       symbolS *sym;
       bfd_vma offset, addend;
       asection *sec;
-      reloc_howto_type *howto;
 
       resolve_symbol_value (r->u.a.offset_sym);
       symval = symbol_get_value_expression (r->u.a.offset_sym);
@@ -709,7 +715,16 @@ resolve_reloc_expr_symbols (void)
 	    }
 	  else if (sym != NULL)
 	    {
-	      if (S_IS_LOCAL (sym) && !symbol_section_p (sym))
+	      /* Convert relocs against local symbols to refer to the
+	         corresponding section symbol plus offset instead.  Keep
+	         PC-relative relocs of the REL variety intact though to
+		 prevent the offset from overflowing the relocated field,
+	         unless it has enough bits to cover the whole address
+	         space.  */
+	      if (S_IS_LOCAL (sym) && !symbol_section_p (sym)
+		  && !(howto->partial_inplace
+		       && howto->pc_relative
+		       && howto->src_mask != addr_mask))
 		{
 		  asection *symsec = S_GET_SEGMENT (sym);
 		  if (!(((symsec->flags & SEC_MERGE) != 0
@@ -730,8 +745,6 @@ resolve_reloc_expr_symbols (void)
 	  sym = abs_section_sym;
 	}
 
-      howto = r->u.a.howto;
-
       r->u.b.sec = sec;
       r->u.b.s = symbol_get_bfdsym (sym);
       r->u.b.r.sym_ptr_ptr = &r->u.b.s;

Re: .reloc improvement

[Tristan Gingold]

On Nov 14, 2011, at 3:22 PM, Maciej W. Rozycki wrote:

> On Thu, 27 Oct 2011, Maciej W. Rozycki wrote:
> 
>> On Tue, 6 Sep 2011, Maciej W. Rozycki wrote:
>> 
>>> On Mon, 22 Aug 2011, Alan Modra wrote:
>>> 
>>>>> Please note that on REL MIPS targets local symbols have to be retained in 
>>>>> several cases where crucial information is lost if a relocation against 
>>>>> such a symbol is converted to one against the containing section's symbol.  
>>>>> This applies to all the PC-relative relocations, where the relocatable 
>>>>> field is too narrow to hold arbitrary addends, and also, more recently, to 
>>>>> microMIPS targets, where linker relaxation has to know local symbol 
>>>>> (label) addresses to perform branch displacement recalculations and to 
>>>>> make the LUI/ADDIU->ADDIUPC relaxation.  The latter is a linker's internal 
>>>>> limitation and may possibly be lifted (possibly by using the RELA 
>>>>> representation internally even on REL targets), but the former is an 
>>>>> inherent problem of the object file format used.
>>>>> 
>>>>> Just making sure these issues are not missed -- I can't have a look at 
>>>>> your change at the moment and see what exact implications it has, sorry.
>>>> 
>>>> The implication of my change is that the programmer will need to be
>>>> careful in choosing symbols used with .reloc.  While that isn't ideal,
>>>> .reloc is such a low-level interface that you need to know what you're
>>>> doing if you use it.
>>> 
>>> Problem is on REL targets some MIPS relocation types can never ever be 
>>> safely converted to refer to a section symbol + addend instead of the 
>>> intended symbol.  You'd therefore make .reloc useless for these types.
>>> 
>>> Yes, REL shouldn't have been chosen for the MIPS ABI in the first place, 
>>> with the complication observed here being the tip of an iceberg only, but 
>>> there you go.  The choice was later fixed with the new ABIs made for 
>>> 64-bit MIPS processors, but the old ABI still remains for 32-bit ones.
>>> 
>>>> Alternatively, I'm quite willing to disable the symbol to section symbol 
>>>> conversion for REL targets.
>>> 
>>> Good idea, that sounds like a plan to me.  That could be made platform 
>>> specific if that made anybody's life easier.
>> 
>> Where are we with this problem?  Your change regressed this program (an 
>> excerpt from a test case I prepared a while ago and was about to integrate 
>> with our test suite):
>> 
>> $ cat reloc.s
>> 	.text
>> 
>> 	.fill	0x1000000
>> 
>> 	.set	micromips
>> foo:
>> 	nop32
>> 	.reloc	., R_MICROMIPS_PC23_S2, 0f
>> 	.fill	0
>> 	addiu	$2, $pc, 8
>> 	nop32
>> 0:
>> 
>> from this:
>> 
>> $ mips-sde-elf-as -32 -o reloc.o reloc.s
>> $ mips-sde-elf-objdump -dr reloc.o
>> 
>> reloc.o:     file format elf32-tradbigmips
>> 
>> 
>> Disassembly of section .text:
>> 
>> 00000000 <foo-0x1000000>:
>> 	...
>> 
>> 01000000 <foo>:
>> 1000000:	0000 0000 	nop
>> 1000004:	7900 0002 	addiu	v0,$pc,8
>> 			1000004: R_MICROMIPS_PC23_S2	.L\x021
>> 1000008:	0000 0000 	nop
>> 
>> to this:
>> 
>> $ mips-sde-elf-as -o reloc.o reloc.s
>> reloc.s: Assembler messages:
>> reloc.s:8: Error: relocation overflow
> 
> Long time, no response, so I went ahead and implemented this fix.  It 
> unbreaks the test case for me and causes no regressions for mips-sde-elf 
> (although I have no slightest idea how much coverage in the test suite 
> .reloc has).
> 
> OK to apply?  Since this is a regression OK for 2.22 too?

It potentially affects all targets.

Ok for the branch if you have the OK for mainline.

Tristan.

> 
> 2011-11-14  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gas/
> 	write.c (dump_section_relocs): Don't convert PC-relative relocs 
> 	that have an in-place addend narrower than the addresses used.
> 
>  Maciej
> 
> binutils-gas-reloc-pcrel-inplace.diff
> Index: binutils-fsf-trunk-quilt/gas/write.c
> ===================================================================
> --- binutils-fsf-trunk-quilt.orig/gas/write.c	2011-11-14 13:50:51.000000000 +0000
> +++ binutils-fsf-trunk-quilt/gas/write.c	2011-11-14 14:19:07.795860362 +0000
> @@ -654,15 +654,21 @@ dump_section_relocs (bfd *abfd ATTRIBUTE
> static void
> resolve_reloc_expr_symbols (void)
> {
> +  bfd_vma addr_mask = 1;
>   struct reloc_list *r;
> 
> +  /* Avoid a shift by the width of type.  */
> +  addr_mask <<= bfd_arch_bits_per_address (stdoutput) - 1;
> +  addr_mask <<= 1;
> +  addr_mask -= 1;
> +
>   for (r = reloc_list; r; r = r->next)
>     {
> +      reloc_howto_type *howto = r->u.a.howto;
>       expressionS *symval;
>       symbolS *sym;
>       bfd_vma offset, addend;
>       asection *sec;
> -      reloc_howto_type *howto;
> 
>       resolve_symbol_value (r->u.a.offset_sym);
>       symval = symbol_get_value_expression (r->u.a.offset_sym);
> @@ -709,7 +715,16 @@ resolve_reloc_expr_symbols (void)
> 	    }
> 	  else if (sym != NULL)
> 	    {
> -	      if (S_IS_LOCAL (sym) && !symbol_section_p (sym))
> +	      /* Convert relocs against local symbols to refer to the
> +	         corresponding section symbol plus offset instead.  Keep
> +	         PC-relative relocs of the REL variety intact though to
> +		 prevent the offset from overflowing the relocated field,
> +	         unless it has enough bits to cover the whole address
> +	         space.  */
> +	      if (S_IS_LOCAL (sym) && !symbol_section_p (sym)
> +		  && !(howto->partial_inplace
> +		       && howto->pc_relative
> +		       && howto->src_mask != addr_mask))
> 		{
> 		  asection *symsec = S_GET_SEGMENT (sym);
> 		  if (!(((symsec->flags & SEC_MERGE) != 0
> @@ -730,8 +745,6 @@ resolve_reloc_expr_symbols (void)
> 	  sym = abs_section_sym;
> 	}
> 
> -      howto = r->u.a.howto;
> -
>       r->u.b.sec = sec;
>       r->u.b.s = symbol_get_bfdsym (sym);
>       r->u.b.r.sym_ptr_ptr = &r->u.b.s;

Re: .reloc improvement

[Alan Modra]

On Mon, Nov 14, 2011 at 02:22:54PM +0000, Maciej W. Rozycki wrote:
> 2011-11-14  Maciej W. Rozycki  <macro@codesourcery.com>
> 
> 	gas/
> 	write.c (dump_section_relocs): Don't convert PC-relative relocs 
> 	that have an in-place addend narrower than the addresses used.

OK.  This looks better than simply disabling this code for REL
targets.

-- 
Alan Modra
Australia Development Lab, IBM

Re: .reloc improvement

[Maciej W. Rozycki]

On Mon, 14 Nov 2011, Alan Modra wrote:

> > 	write.c (dump_section_relocs): Don't convert PC-relative relocs 
> > 	that have an in-place addend narrower than the addresses used.
> 
> OK.  This looks better than simply disabling this code for REL
> targets.

 Thanks for the review, committed now to trunk and 2.22.

  Maciej