mn10300: relaxing vs section merging

mn10300: relaxing vs section merging

[DJ Delorie]

There are problems with the mn10300 when you turn on both relaxing and
section merging.  What happens is, the relaxing code uses the
pre-merged symbol offsets.  I finally figured out why the obvious
solution didn't work, and now have a patch.  However, before I
consider applying it, I have a question: why don't we bother adjusting
relocs in gas?  You have to use section-relative relocs with section
merging, but I couldn't find any explanation of why the call to do so
was commented out.

2004-05-05  DJ Delorie  <dj@redhat.com>

	* config/tc-mn10300.h (tc_fix_adjustable): Enable so we get
	section-relative symbols.

	* elf-m10300.c (mn10300_elf_relax_section): Preserve reloc
	addend so it doesn't get adjusted twice, but do use merged
	value when calculating symbol value.

Index: gas/config/tc-mn10300.h
===================================================================
RCS file: /cvs/uberbaum/./gas/config/tc-mn10300.h,v
retrieving revision 1.14
diff -p -C2 -r1.14 tc-mn10300.h
*** gas/config/tc-mn10300.h	10 Jul 2003 04:44:56 -0000	1.14
--- gas/config/tc-mn10300.h	5 May 2004 18:12:29 -0000
*************** void mn10300_cons_fix_new PARAMS ((fragS
*** 103,108 ****
  
  /* Don't bother to adjust relocs.  */
! #define tc_fix_adjustable(FIX) 0
! /* #define tc_fix_adjustable(FIX) mn10300_fix_adjustable (FIX) */
  extern bfd_boolean mn10300_fix_adjustable PARAMS ((struct fix *));
  
--- 103,108 ----
  
  /* Don't bother to adjust relocs.  */
! /* #define tc_fix_adjustable(FIX) 0 */
! #define tc_fix_adjustable(FIX) mn10300_fix_adjustable (FIX)
  extern bfd_boolean mn10300_fix_adjustable PARAMS ((struct fix *));
  
Index: bfd/elf-m10300.c
===================================================================
RCS file: /cvs/uberbaum/./bfd/elf-m10300.c,v
retrieving revision 1.49
diff -p -C2 -r1.49 elf-m10300.c
*** bfd/elf-m10300.c	27 Mar 2004 10:58:05 -0000	1.49
--- bfd/elf-m10300.c	5 May 2004 18:12:31 -0000
*************** mn10300_elf_relax_section (abfd, sec, li
*** 2440,2443 ****
--- 2440,2444 ----
  	  const char *sym_name;
  	  char *new_name;
+ 	  bfd_vma saved_addend;
  
  	  /* A local symbol.  */
*************** mn10300_elf_relax_section (abfd, sec, li
*** 2459,2462 ****
--- 2460,2467 ----
  						      isym->st_name);
  
+ 	  saved_addend = irel->r_addend;
+ 	  symval = _bfd_elf_rela_local_sym (abfd, isym, &sym_sec, irel);
+ 	  symval += irel->r_addend;
+ 	  irel->r_addend = saved_addend;
  	  /* Tack on an ID so we can uniquely identify this
  	     local symbol in the global hash table.  */

Re: mn10300: relaxing vs section merging

[Alexandre Oliva]

On May  5, 2004, DJ Delorie <dj@redhat.com> wrote:

> However, before I
> consider applying it, I have a question: why don't we bother adjusting
> relocs in gas?

I don't recall the exact details, but what I recall had to do with
losing information that was essential for (some definition of) correct
relaxation.

Consider, for example:

.Lfoo:
        ;; code and data taking up 64 bytes
.Lbar:


Consider references to .Lfoo+32 and .Lbar-32.  They should be the
same, right?

What if the code between .Lfoo and .Lfoo+32 is relaxed?  What if
there's something relaxed between .Lbar-32 and .Lbar?  Should they be
adjusted differently?  Or should the constant remain unchanged?

Now on to the most difficult bit: in a PIC jump table, we compute
differences between two symbols in the text segment.  Entries are
differences between a code label and the jump table base address,
that's in the same section, e.g.:


.Ljumptable:
;; ...
        .long .Lcode-.Ljumptable
;; ...
.Lcode:
        

We can't compute the offset in the assembler, because we might do
linker relaxation.  But all the linker gets is a PC-relative
relocation at a certain point, and it has to somehow figure out the
actual code label we get to jump to.

The relocation is originally emitted as:

  PCREL  .Lcode + . - .Ljumptable

since we can't emit such a complex relocation, and the range between
.Ljumptable and . is not relaxable, we can compute that offset and use
that as the addend.

Should we reduce .Lcode to section+offset (+ addend), we'd lose track
of the exact code point we're pointing to, so linker relaxation might
end up adjusting it by the wrong amount.

The only way to get it to work right is to retain the reference symbol
as an anchor.


I do remember having run into this one problem years ago.  I probably
thought the same anchoring issue could very well show up in other
relocation types, and just disabled adjusting for them all.  Perhaps
it's safe to enable adjusting, at least as long as the addend is zero.

> You have to use section-relative relocs with section merging

Why?

Anyhow...  I don't see how this could actually work in the general
case, since mn10300_fix_adjustable() actually refuses to adjust
relocations that reference symbols in code sections.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: mn10300: relaxing vs section merging

[DJ Delorie]

> I don't recall the exact details, but what I recall had to do with
> losing information that was essential for (some definition of) correct
> relaxation.
> I do remember having run into this one problem years ago.  I probably
> thought the same anchoring issue could very well show up in other
> relocation types, and just disabled adjusting for them all.  Perhaps
> it's safe to enable adjusting, at least as long as the addend is zero.
> 
> > You have to use section-relative relocs with section merging
> 
> Why?

Because _bfd_elf_rela_local_sym() only adjusts STT_SECTION symbols.
If you have other types of symbols, it doesn't seem to adjust the
values properly.

> Anyhow...  I don't see how this could actually work in the general
> case, since mn10300_fix_adjustable() actually refuses to adjust
> relocations that reference symbols in code sections.

The case that's failing is thusly, where the symbol referenced is in
the merged rodata section and gets moved too far away through merging:

	.text
	mov	.L1,d1

	.section	.rodata.str1.1,"aMS",@progbits,1
.L1:
	.string	"\n"

Re: mn10300: relaxing vs section merging

[Alexandre Oliva]

On May  6, 2004, DJ Delorie <dj@redhat.com> wrote:

>> I don't recall the exact details, but what I recall had to do with
>> losing information that was essential for (some definition of) correct
>> relaxation.
>> I do remember having run into this one problem years ago.  I probably
>> thought the same anchoring issue could very well show up in other
>> relocation types, and just disabled adjusting for them all.  Perhaps
>> it's safe to enable adjusting, at least as long as the addend is zero.

>> > You have to use section-relative relocs with section merging

>> Why?

> Because _bfd_elf_rela_local_sym() only adjusts STT_SECTION symbols.
> If you have other types of symbols, it doesn't seem to adjust the
> values properly.

Hmm...  Maybe it should be improved/fixed, then?  I suppose whenever
we get called with a symbol, we should figure out in which section it
is, find the relocation for the section, and then apply the symbol
offset into the section again.

>> Anyhow...  I don't see how this could actually work in the general
>> case, since mn10300_fix_adjustable() actually refuses to adjust
>> relocations that reference symbols in code sections.

> The case that's failing is thusly, where the symbol referenced is in
> the merged rodata section and gets moved too far away through merging:

> 	.text
> 	mov	.L1,d1

> 	.section	.rodata.str1.1,"aMS",@progbits,1
> .L1:
> 	.string	"\n"

This case could definitely be turned into a section symbol, even
though it's relaxable.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: mn10300: relaxing vs section merging

[Alan Modra]

On Tue, May 11, 2004 at 07:35:59PM -0300, Alexandre Oliva wrote:
> On May  6, 2004, DJ Delorie <dj@redhat.com> wrote:
> >> > You have to use section-relative relocs with section merging
> 
> >> Why?
> 
> > Because _bfd_elf_rela_local_sym() only adjusts STT_SECTION symbols.
> > If you have other types of symbols, it doesn't seem to adjust the
> > values properly.
> 
> Hmm...  Maybe it should be improved/fixed, then?  I suppose whenever
> we get called with a symbol, we should figure out in which section it
> is, find the relocation for the section, and then apply the symbol
> offset into the section again.

Local symbols in SEC_MERGE sections are adjusted, but too late for
you when doing relaxing.  See elf_link_input_bfd.  You can't do the
adjustment until you know final section layout, because some symbols
might end up pointing outside their section, or even outside their
bfd.  Of course, you can't make the final section layout until
relaxing is complete..

I suppose it would be possible to move all the local sym adjustments
into _bfd_elf_rel{,a}_local_sym.  Note that _bfd_elf_rel_local_sym
looks like it handles any local sym.  Beware!  _bfd_elf_rel_local_sym
should be (and is currently) only called on section syms.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: mn10300: relaxing vs section merging

[DJ Delorie]

> > >> > You have to use section-relative relocs with section merging
> > 
> > >> Why?
> > 
> > > Because _bfd_elf_rela_local_sym() only adjusts STT_SECTION symbols.
> > > If you have other types of symbols, it doesn't seem to adjust the
> > > values properly.
> > 
> > Hmm...  Maybe it should be improved/fixed, then?  I suppose whenever
> > we get called with a symbol, we should figure out in which section it
> > is, find the relocation for the section, and then apply the symbol
> > offset into the section again.
> 
> Local symbols in SEC_MERGE sections are adjusted, but too late for
> you when doing relaxing.  See elf_link_input_bfd.  You can't do the
> adjustment until you know final section layout, because some symbols
> might end up pointing outside their section, or even outside their
> bfd.  Of course, you can't make the final section layout until
> relaxing is complete..
> 
> I suppose it would be possible to move all the local sym adjustments
> into _bfd_elf_rel{,a}_local_sym.  Note that _bfd_elf_rel_local_sym
> looks like it handles any local sym.  Beware!  _bfd_elf_rel_local_sym
> should be (and is currently) only called on section syms.

Ok, so can I commit my patch then?  It sounds like I'm doing the right
thing...

Re: mn10300: relaxing vs section merging

[Alan Modra]

On Tue, May 25, 2004 at 04:47:39PM -0400, DJ Delorie wrote:
> Ok, so can I commit my patch then?  It sounds like I'm doing the right
> thing...

It doesn't look right to me.  You are adding irel->r_addend to symval
where you previously did not.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: mn10300: relaxing vs section merging

[DJ Delorie]

> It doesn't look right to me.  You are adding irel->r_addend to symval
> where you previously did not.

That's because _bfd_elf_rela_local_sym adjusts the addend according to
where the symbol has been (so far) relocated to.  That's the value we
need for checking relax limits, so we need to add it in.

I don't think "previously" counts here.  It didn't work previously.

Re: mn10300: relaxing vs section merging

[Alan Modra]

On Tue, May 25, 2004 at 07:14:34PM -0400, DJ Delorie wrote:
> 
> > It doesn't look right to me.  You are adding irel->r_addend to symval
> > where you previously did not.
> 
> That's because _bfd_elf_rela_local_sym adjusts the addend according to
> where the symbol has been (so far) relocated to.  That's the value we
> need for checking relax limits, so we need to add it in.
> 
> I don't think "previously" counts here.  It didn't work previously.

I guessed that's why you were adding irel->r_addend, but doing so will
be wrong for other symbols and relocations.  Consider non SEC_MERGE
symbols, and note later code

	  value += irel->r_addend;

so now the addend is summed twice.  That can't be correct.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: mn10300: relaxing vs section merging

[DJ Delorie]

Hmmm... it doesn't work without that, I suppose we could conditionally
add it in for cases where _bfd_elf_rela_local_sym twiddles the value.
Does that sound right?  All the other code in that region is adding in
the vma and output offset, we need to do so also when we move a merged
symbol.

Re: mn10300: relaxing vs section merging

[Alan Modra]

On Tue, May 25, 2004 at 07:57:16PM -0400, DJ Delorie wrote:
> 
> Hmmm... it doesn't work without that, I suppose we could conditionally
> add it in for cases where _bfd_elf_rela_local_sym twiddles the value.
> Does that sound right?  All the other code in that region is adding in
> the vma and output offset, we need to do so also when we move a merged
> symbol.

Adding the difference between the old addend and the adjusted one, or
something like that, would probably be the right thing to do.  Also,
you have some dead code assigning symval before the rela_local_sym call.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: mn10300: relaxing vs section merging

[DJ Delorie]

Ok, here is my latest patch.  This one I tested with gcc's C
testsuite, with and without -mrelax, with no failures with that
weren't also without:

Index: bfd/elf-m10300.c
===================================================================
RCS file: /cvs/uberbaum/./bfd/elf-m10300.c,v
retrieving revision 1.51
diff -p -C2 -r1.51  bfd/elf-m10300.c
*** bfd/elf-m10300.c	27 May 2004 06:22:54 -0000	1.51
--- bfd/elf-m10300.c	27 May 2004 23:12:07 -0000
*************** mn10300_elf_relax_section (abfd, sec, li
*** 2463,2466 ****
--- 2463,2467 ----
  	  const char *sym_name;
  	  char *new_name;
+ 	  bfd_vma saved_addend;
  
  	  /* A local symbol.  */
*************** mn10300_elf_relax_section (abfd, sec, li
*** 2475,2485 ****
  	    sym_sec = bfd_section_from_elf_index (abfd, isym->st_shndx);
  
- 	  symval = (isym->st_value
- 		    + sym_sec->output_section->vma
- 		    + sym_sec->output_offset);
  	  sym_name = bfd_elf_string_from_elf_section (abfd,
  						      symtab_hdr->sh_link,
  						      isym->st_name);
  
  	  /* Tack on an ID so we can uniquely identify this
  	     local symbol in the global hash table.  */
--- 2476,2498 ----
  	    sym_sec = bfd_section_from_elf_index (abfd, isym->st_shndx);
  
  	  sym_name = bfd_elf_string_from_elf_section (abfd,
  						      symtab_hdr->sh_link,
  						      isym->st_name);
  
+ 	  if ((sym_sec->flags & SEC_MERGE)
+ 	      && ELF_ST_TYPE (isym->st_info) == STT_SECTION
+ 	      && sym_sec->sec_info_type == ELF_INFO_TYPE_MERGE)
+ 	    {
+ 	      saved_addend = irel->r_addend;
+ 	      symval = _bfd_elf_rela_local_sym (abfd, isym, &sym_sec, irel);
+ 	      symval += irel->r_addend;
+ 	      irel->r_addend = saved_addend;
+ 	    }
+ 	  else
+ 	    {
+ 	      symval = (isym->st_value
+ 			+ sym_sec->output_section->vma
+ 			+ sym_sec->output_offset);
+ 	    }
  	  /* Tack on an ID so we can uniquely identify this
  	     local symbol in the global hash table.  */
Index: gas/config/tc-mn10300.c
===================================================================
RCS file: /cvs/uberbaum/./gas/config/tc-mn10300.c,v
retrieving revision 1.46
diff -p -C2 -r1.46  gas/config/tc-mn10300.c
*** gas/config/tc-mn10300.c	6 May 2004 11:01:48 -0000	1.46
--- gas/config/tc-mn10300.c	27 May 2004 23:12:08 -0000
*************** mn10300_fix_adjustable (fixp)
*** 2543,2546 ****
--- 2543,2553 ----
      return 0;
  
+   /* Likewise, do not adjust symbols that won't be merged, or debug
+      symbols, because they too break relaxation.  We do want to adjust  */
+   if (! (S_GET_SEGMENT(fixp->fx_addsy)->flags & SEC_MERGE))
+     return 0;
+   if (strncmp (S_GET_SEGMENT (fixp->fx_addsy)->name, ".debug", 6) == 0)
+     return 0;
+ 
    return 1;
  }
Index: gas/config/tc-mn10300.h
===================================================================
RCS file: /cvs/uberbaum/./gas/config/tc-mn10300.h,v
retrieving revision 1.14
diff -p -C2 -r1.14  gas/config/tc-mn10300.h
*** gas/config/tc-mn10300.h	10 Jul 2003 04:44:56 -0000	1.14
--- gas/config/tc-mn10300.h	27 May 2004 23:12:08 -0000
*************** void mn10300_cons_fix_new PARAMS ((fragS
*** 103,108 ****
  
  /* Don't bother to adjust relocs.  */
! #define tc_fix_adjustable(FIX) 0
! /* #define tc_fix_adjustable(FIX) mn10300_fix_adjustable (FIX) */
  extern bfd_boolean mn10300_fix_adjustable PARAMS ((struct fix *));
  
--- 103,108 ----
  
  /* Don't bother to adjust relocs.  */
! /* #define tc_fix_adjustable(FIX) 0 */
! #define tc_fix_adjustable(FIX) mn10300_fix_adjustable (FIX)
  extern bfd_boolean mn10300_fix_adjustable PARAMS ((struct fix *));
  

Re: mn10300: relaxing vs section merging

[Alexandre Oliva]

On May 27, 2004, DJ Delorie <dj@redhat.com> wrote:

> Ok, here is my latest patch.  This one I tested with gcc's C
> testsuite, with and without -mrelax, with no failures with that
> weren't also without:

I take it that you didn't test C++?  I tried your previous patch with
that, and it still had a few additional failures that weren't present
without -mrelax.  I don't see that it would behave any different from
what you last posted.  I'll look into it.

+   /* Likewise, do not adjust symbols that won't be merged, or debug
+      symbols, because they too break relaxation.  We do want to adjust  */

This comment is incomplete.

Other than that, the patch looks good.  Please complete the comment,
post the final version and check it in.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: mn10300: relaxing vs section merging

[Alexandre Oliva]

On May 27, 2004, Alexandre Oliva <aoliva@redhat.com> wrote:

> I don't see that it would behave any different from what you last
> posted.

Of course, the answer was right after the incomplete comment that
distracted me :-)

> +   /* Likewise, do not adjust symbols that won't be merged, or debug
> +      symbols, because they too break relaxation.  We do want to adjust  */

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: mn10300: relaxing vs section merging

[DJ Delorie]

> Of course, the answer was right after the incomplete comment that
> distracted me :-)

Right.  Just forgot to do a final save before diffing that comment.

The problem I've run into is that we want to fixup relocations that
happen in SEC_CODE segments, regardless of where they point, but NOT
fixups that happen in non-code segments.  Unfortunately, fixP's don't
have a reference to where they're coming from, just where they're
going to.

I'll run C++ tests.

Re: mn10300: relaxing vs section merging

[Alexandre Oliva]

On May 27, 2004, DJ Delorie <dj@redhat.com> wrote:
D
>> Of course, the answer was right after the incomplete comment that
>> distracted me :-)

> I'll run C++ tests.

Don't bother.  Just did.  The problems are fixed.  Thanks!

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}

Re: mn10300: relaxing vs section merging

[DJ Delorie]

> > I'll run C++ tests.
> 
> Don't bother.  Just did.  The problems are fixed.  Thanks!

Mine passed too :-)

Ok, I'll check it in shortly.

Re: mn10300: relaxing vs section merging

[Alexandre Oliva]

On May 27, 2004, DJ Delorie <dj@redhat.com> wrote:

> The problem I've run into is that we want to fixup relocations that
> happen in SEC_CODE segments, regardless of where they point

?!?

I don't think so.  Unless I failed to describe the PIC jump table
problem properly, or I misremember where such jump tables are located
(I think it's in the text segment), this won't do you much good.

> but NOT fixups that happen in non-code segments.  Unfortunately,
> fixP's don't have a reference to where they're coming from, just
> where they're going to.

Yeah, I vaguely remember this dilemma myself.  More than once I
considered adding a new variant of tc_fix_adjustable that took an
additional argument, but never got around to implementing it.

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}