[PATCH, bfin] Clear relocs for removed entries in .eh_frame section

[PATCH, bfin] Clear relocs for removed entries in .eh_frame section

[Jie Zhang]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

When debugging a C++ exception unwinding issue for Blackfin FDPIC
format, I found a bug of Blackfin BFD FDPIC support. When linking,
duplicate CIE will be removed from output .eh_frame section. But the
relocations for them are not. When dynamic linker does these
relocations, some contents in .eh_frame will be wrongly modified. This
causes the exception unwinding failure. I attached a patch for it. But
there are several points I'm not sure:

1. I saw this in elflink.c:elf_link_input_bfd ()

		      /* This is a reloc for a deleted entry or somesuch.
			 Turn it into an R_*_NONE reloc, at the same
			 offset as the last reloc.  elf_eh_frame.c and
			 elf_bfd_discard_info rely on reloc offsets
			 being ordered.  */

In my patch I just set the offset to zero. It seems works. I don't
know if we need do the same.

Alan: The comment was from you. Can you help me understand it?

2. In bfinfdpic_relocate_section () there are several calls to
_bfd_elf_section_offset () before calling _bfinfdpic_add_dyn_reloc ().
Only in one place the return value of _bfd_elf_section_offset () is
checked in my patch. It seems works. But I'm not sure if we need add
such check for all these calls.

3. Is it possible to not emit the reloc instead of set its type to R_*_NONE?

Alex: I think FRV has the same issue. In the lasted release of
redhat's frv toolchain from redhat's ftp, the bug is hidden since all
dupilicated CIEs are not optimized away because there is CIE format
incompatibility between gcc (3.4) and binutils (2.14).

Thanks,
Jie

[-- Attachment #2: bfin-fdpic-bfd-cie.diff --]
[-- Type: text/x-patch, Size: 1772 bytes --]

Index: elf32-bfin.c
===================================================================
RCS file: /cvsroot/gcc3/binutils/binutils-2.15/bfd/elf32-bfin.c,v
retrieving revision 1.35
diff -u -p -r1.35 elf32-bfin.c
--- elf32-bfin.c	6 Apr 2006 05:48:02 -0000	1.35
+++ elf32-bfin.c	24 Apr 2006 10:46:44 -0000
@@ -2476,6 +2481,8 @@ bfinfdpic_relocate_section (bfd * output
 						 input_section->output_section)
 			  & (SEC_ALLOC | SEC_LOAD)) == (SEC_ALLOC | SEC_LOAD))
 		  {
+		    bfd_vma offset;
+
 		    if (_bfinfdpic_osec_readonly_p (output_bfd,
 						   input_section
 						   ->output_section))
@@ -2486,15 +2493,23 @@ bfinfdpic_relocate_section (bfd * output
 			   name, input_bfd, input_section, rel->r_offset);
 			return FALSE;
 		      }
-		    _bfinfdpic_add_dyn_reloc (output_bfd,
-					      bfinfdpic_gotrel_section (info),
-					      _bfd_elf_section_offset
-					      (output_bfd, info,
-					       input_section, rel->r_offset)
-					      + input_section
-					      ->output_section->vma
-					      + input_section->output_offset,
-					      r_type, dynindx, addend, picrel);
+		    offset = _bfd_elf_section_offset (output_bfd, info,
+						      input_section, rel->r_offset);
+		    /* Only output a reloc for a not deleted entry.  */
+		    if (offset >= (bfd_vma) -2)
+		      _bfinfdpic_add_dyn_reloc (output_bfd,
+						bfinfdpic_gotrel_section (info),
+						0,
+						R_unused0,
+						dynindx, addend, picrel);
+		    else
+		      _bfinfdpic_add_dyn_reloc (output_bfd,
+						bfinfdpic_gotrel_section (info),
+						offset + input_section
+						->output_section->vma
+						+ input_section->output_offset,
+						r_type,
+						dynindx, addend, picrel);
 		  }
 		else
 		  addend += bfinfdpic_got_section (info)->output_section->vma;

Re: [PATCH, bfin] Clear relocs for removed entries in .eh_frame section

[Alan Modra]

On Wed, Apr 26, 2006 at 11:45:19AM +0800, Jie Zhang wrote:
> 1. I saw this in elflink.c:elf_link_input_bfd ()
> 
> 		      /* This is a reloc for a deleted entry or somesuch.
> 			 Turn it into an R_*_NONE reloc, at the same
> 			 offset as the last reloc.  elf_eh_frame.c and
> 			 elf_bfd_discard_info rely on reloc offsets
> 			 being ordered.  */
> 
> In my patch I just set the offset to zero. It seems works. I don't
> know if we need do the same.
> 
> Alan: The comment was from you. Can you help me understand it?

bfd_elf_discard_info indirectly calls bfd_elf_reloc_symbol_deleted_p,
which scans relocs looking for a particular offset.

  for (; rcookie->rel < rcookie->relend; rcookie->rel++)
    {
      unsigned long r_symndx;

      if (! rcookie->bad_symtab)
	if (rcookie->rel->r_offset > offset)
	  return FALSE;
      if (rcookie->rel->r_offset != offset)
	continue;

There are some macros in elf-eh-frame.c, ENSURE_NO_RELOCS and
SKIP_RELOCS that similarly test r_offset.  The above code won't work in
general if relocs are unordered.  However, I see that the way the code
is written, setting r_offset to zero is OK.  You just need to ensure
r_offset is not set to some larger value than r_offset in following
relocs.

> 2. In bfinfdpic_relocate_section () there are several calls to
> _bfd_elf_section_offset () before calling _bfinfdpic_add_dyn_reloc ().
> Only in one place the return value of _bfd_elf_section_offset () is
> checked in my patch. It seems works. But I'm not sure if we need add
> such check for all these calls.

If _bfd_elf_section_offset can return -1 or -2 then you need to add the
check.  If it cannot, eg. because the particular reloc type is not used
in eh_frame or stabs section, then you don't need the check.  Of course,
if you don't need the check, then you also don't need to call
_bfd_elf_section_offset.  :-)

> 3. Is it possible to not emit the reloc instead of set its type to R_*_NONE?

It's too late.  You have already allocated space for the dynamic relocs.

-- 
Alan Modra
IBM OzLabs - Linux Technology Centre

Re: [PATCH, bfin] Clear relocs for removed entries in .eh_frame section

[Jie Zhang]

On 4/26/06, Alan Modra <amodra@bigpond.net.au> wrote:
> On Wed, Apr 26, 2006 at 11:45:19AM +0800, Jie Zhang wrote:
> > 1. I saw this in elflink.c:elf_link_input_bfd ()
> >
> >                     /* This is a reloc for a deleted entry or somesuch.
> >                        Turn it into an R_*_NONE reloc, at the same
> >                        offset as the last reloc.  elf_eh_frame.c and
> >                        elf_bfd_discard_info rely on reloc offsets
> >                        being ordered.  */
> >
> > In my patch I just set the offset to zero. It seems works. I don't
> > know if we need do the same.
> >
> > Alan: The comment was from you. Can you help me understand it?
>
> bfd_elf_discard_info indirectly calls bfd_elf_reloc_symbol_deleted_p,
> which scans relocs looking for a particular offset.
>
>   for (; rcookie->rel < rcookie->relend; rcookie->rel++)
>     {
>       unsigned long r_symndx;
>
>       if (! rcookie->bad_symtab)
>         if (rcookie->rel->r_offset > offset)
>           return FALSE;
>       if (rcookie->rel->r_offset != offset)
>         continue;
>
> There are some macros in elf-eh-frame.c, ENSURE_NO_RELOCS and
> SKIP_RELOCS that similarly test r_offset.  The above code won't work in
> general if relocs are unordered.  However, I see that the way the code
> is written, setting r_offset to zero is OK.  You just need to ensure
> r_offset is not set to some larger value than r_offset in following
> relocs.
>
Thanks for explain this. Now I feel safe to set the offset of deleted
reloc to 0.

> > 2. In bfinfdpic_relocate_section () there are several calls to
> > _bfd_elf_section_offset () before calling _bfinfdpic_add_dyn_reloc ().
> > Only in one place the return value of _bfd_elf_section_offset () is
> > checked in my patch. It seems works. But I'm not sure if we need add
> > such check for all these calls.
>
> If _bfd_elf_section_offset can return -1 or -2 then you need to add the
> check.  If it cannot, eg. because the particular reloc type is not used
> in eh_frame or stabs section, then you don't need the check.  Of course,
> if you don't need the check, then you also don't need to call
> _bfd_elf_section_offset.  :-)
>
I'll review the related code in Blackfin FDPIC to see if there are
more cases need check. I also would like to hear from Alex before
doing any checkin.

> > 3. Is it possible to not emit the reloc instead of set its type to R_*_NONE?
>
> It's too late.  You have already allocated space for the dynamic relocs.
>
OK.

Thanks,
Jie

Re: [PATCH, bfin] Clear relocs for removed entries in .eh_frame section

[Jie Zhang]

This patch has been in ADI's CVS repository <blackfin.uclinux.org> for
a while and seems OK. So I have installed this patch with the
following ChangeLog entry:

        * elf32-bfin.c (bfinfdpic_relocate_section): Clear reloc for
        deteted entries in .eh_frame section.

Jie