[RFC PATCH] Avoid emitting TEXTREL marker for R_ARM_NONE relocs.

[RFC PATCH] Avoid emitting TEXTREL marker for R_ARM_NONE relocs.

[Roland McGrath]

I have a case where ld -shared emitted the DT_TEXTREL marker but there were
no actual text relocs.  It turns out that what it thought were going to be
text relocs were reloc slots that got turned into R_ARM_NONE in the final
output.

I have a dim memory of someone explaining that emitting R_ARM_NONE dynamic
relocs was intractable to avoid in some cases.  But I don't understand what
situations cause that to happen.  Nor could I figure out today where in the
code it actually happens.  So I am at something of a loss for coming up
with a concise test case for the bug that I encountered.  Also for giving
an adequate explanation of the scenario in the comment I'm adding below.

It looks to me like this early setting of DF_TEXTREL was always redundant
with the scan done with elf32_arm_readonly_dynrelocs at the end.  So I'm
fairly confident the change is correct, if possibly suboptimal.  But I'd
like to understand enough to write a good test case and explain the fix
better in the comments.


Thanks,
Roland


bfd/
2014-03-12  Roland McGrath  <mcgrathr@google.com>

	* elf32-arm.c (elf32_arm_size_dynamic_sections): Don't set
	DF_TEXTREL while setting up space for local dynamic relocs.

--- a/bfd/elf32-arm.c
+++ b/bfd/elf32-arm.c
@@ -13766,8 +13766,13 @@ elf32_arm_size_dynamic_sections (bfd *
output_bfd ATTRIBUTE_UNUSED,
 		{
 		  srel = elf_section_data (p->sec)->sreloc;
 		  elf32_arm_allocate_dynrelocs (info, srel, p->count);
-		  if ((p->sec->output_section->flags & SEC_READONLY) != 0)
-		    info->flags |= DF_TEXTREL;
+                  /* It would appear that we could check for the output
+                     section being SEC_READONLY and set DF_TEXTREL here.
+                     But these relocs might be morphed into R_ARM_NONE
+                     later, making the test here false-positive.
+                     Instead, we'll just let the scan at the end of the
+                     function (using elf32_arm_readonly_dynrelocs) catch
+                     any that survive.  */
 		}
 	    }
 	}

Re: [RFC PATCH] Avoid emitting TEXTREL marker for R_ARM_NONE relocs.

[Alan Modra]

On Wed, Mar 12, 2014 at 10:17:32AM -0700, Roland McGrath wrote:
> I have a dim memory of someone explaining that emitting R_ARM_NONE dynamic
> relocs was intractable to avoid in some cases.  But I don't understand what
> situations cause that to happen.  Nor could I figure out today where in the
> code it actually happens.

One case that springs to mind is .eh_frame editing, which happens
after dynamic relocations have been allocated.  If you require dynamic
relocations in .eh_frame CIEs (say due to using absolute encoding for
some field) and CIEs are merged, then you can end up needing fewer
dynamic relocations than allocated.

Do you know where the R_ARM_NONE dynamic reloc came from?  Tip:
-z nocombreloc helps in answering that question.

> @@ -13766,8 +13766,13 @@ elf32_arm_size_dynamic_sections (bfd *
> output_bfd ATTRIBUTE_UNUSED,
>  		{
>  		  srel = elf_section_data (p->sec)->sreloc;
>  		  elf32_arm_allocate_dynrelocs (info, srel, p->count);
> -		  if ((p->sec->output_section->flags & SEC_READONLY) != 0)
> -		    info->flags |= DF_TEXTREL;
> +                  /* It would appear that we could check for the output
> +                     section being SEC_READONLY and set DF_TEXTREL here.
> +                     But these relocs might be morphed into R_ARM_NONE
> +                     later, making the test here false-positive.
> +                     Instead, we'll just let the scan at the end of the
> +                     function (using elf32_arm_readonly_dynrelocs) catch
> +                     any that survive.  */

No, this is wrong.  The code you're touching here deals with 
relocations for local symbols, elf32_arm_readonly_dynrelocs scans
global symbols.

-- 
Alan Modra
Australia Development Lab, IBM

Re: [RFC PATCH] Avoid emitting TEXTREL marker for R_ARM_NONE relocs.

[Roland McGrath]

On Wed, Mar 12, 2014 at 3:52 PM, Alan Modra <amodra@gmail.com> wrote:
> One case that springs to mind is .eh_frame editing, which happens
> after dynamic relocations have been allocated.  If you require dynamic
> relocations in .eh_frame CIEs (say due to using absolute encoding for
> some field) and CIEs are merged, then you can end up needing fewer
> dynamic relocations than allocated.

I was going to say this couldn't be my case because I don't have .eh_frame,
being ARM.  But in fact I do have some .eh_frame content (as well as
.ARM.exidx), and I'll have to figure out where that came from.

> Do you know where the R_ARM_NONE dynamic reloc came from?  Tip:
> -z nocombreloc helps in answering that question.

Thanks for the tip.  That just told me they (there are two) are in
.rel.text, and they're the only relocs that end up there.  So still
not telling me very much.  I'll keep investigating.

Probably I'll find these and make them go away and so the DT_TEXTREL mark
for me was just fallout of a different problem (and I'm glad that it made
me notice the underlying problem).

But I maintain it's a bug that DT_TEXTREL can ever be set when there are no
actual dynamic relocs in read-only segments in the final image.  (I still
have a hard time accepting it's not a bug to ever emit R_ARM_NONE instead
of just making the reloc section shorter, even if that means leaving some
useless padding after it because of layout order.  But I've long since
decided to let that one go.)

> No, this is wrong.  The code you're touching here deals with
> relocations for local symbols, elf32_arm_readonly_dynrelocs scans
> global symbols.

I see.


Thanks,
Roland

Re: [RFC PATCH] Avoid emitting TEXTREL marker for R_ARM_NONE relocs.

[Roland McGrath]

Alan,

It was indeed .eh_frame that led to it.  Here's a trivial test case:

	$ head eh[123].s
	==> eh1.s <==
	.cfi_sections .eh_frame
	foo:
		.cfi_startproc
		.cfi_personality 0, pers
		.cfi_endproc

	==> eh2.s <==
	.cfi_sections .eh_frame
	bar:
		.cfi_startproc
		.cfi_personality 0, pers
		.cfi_endproc

	==> eh3.s <==
	.globl pers
	pers:
	nop
	$ ./ld/ld-new --eh-frame-hdr -shared -o eh.so eh[123].o
	$ readelf -dr eh.so

	Dynamic section at offset 0x260 contains 10 entries:
	  Tag        Type                         Name/Value
	 0x00000004 (HASH)                       0x100000d4
	 0x00000005 (STRTAB)                     0x100001b0
	 0x00000006 (SYMTAB)                     0x10000110
	 0x0000000a (STRSZ)                      64 (bytes)
	 0x0000000b (SYMENT)                     16 (bytes)
	 0x00000011 (REL)                        0x100001f0
	 0x00000012 (RELSZ)                      16 (bytes)
	 0x00000013 (RELENT)                     8 (bytes)
WRONG=>	 0x00000016 (TEXTREL)                    0x0
	 0x00000000 (NULL)                       0x0

	Relocation section '.rel.dyn' at offset 0x1f0 contains 2 entries:
	 Offset     Info    Type            Sym.Value  Sym. Name
UGLY=>	00000000  00000000 R_ARM_NONE
	1000022e  00000202 R_ARM_ABS32       00000000   pers
	$


It's no longer an urgent issue for me, but I think it deserves to be fixed.


Thanks,
Roland

Re: [RFC PATCH] Avoid emitting TEXTREL marker for R_ARM_NONE relocs.

[Alan Modra]

On Thu, Mar 13, 2014 at 10:32:18AM -0700, Roland McGrath wrote:
> 		.cfi_startproc
> 		.cfi_personality 0, pers
> 		.cfi_endproc

Yes, the classic case of using an absolute encoding for a personality
function.

> 	.globl pers
> 	pers:
> 	nop

And for two R_ARM_NONE, make pers hidden.  ;)

> It's no longer an urgent issue for me, but I think it deserves to be fixed.

To do that, we need to run the eh_frame parts of bfd_elf_discard_info
before bfd_elf_size_dynamic_sections.  One of the keys to being able
to do that is not using a vma anywhere in the eh_frame editing code,
since at that stage we won't have laid out sections.

-- 
Alan Modra
Australia Development Lab, IBM

Don't use vma to identify eh_frame personality function

[Alan Modra]

On Fri, Mar 14, 2014 at 11:58:42PM +1030, Alan Modra wrote:
> On Thu, Mar 13, 2014 at 10:32:18AM -0700, Roland McGrath wrote:
> > 		.cfi_startproc
> > 		.cfi_personality 0, pers
> > 		.cfi_endproc
> 
> Yes, the classic case of using an absolute encoding for a personality
> function.
> 
> > 	.globl pers
> > 	pers:
> > 	nop
> 
> And for two R_ARM_NONE, make pers hidden.  ;)
> 
> > It's no longer an urgent issue for me, but I think it deserves to be fixed.
> 
> To do that, we need to run the eh_frame parts of bfd_elf_discard_info
> before bfd_elf_size_dynamic_sections.  One of the keys to being able
> to do that is not using a vma anywhere in the eh_frame editing code,
> since at that stage we won't have laid out sections.

This is all we should need to be able to run the eh_frame parts of
bfd_elf_discard_info before bfd_elf_size_dynamic_sections.  Other
places in elf-eh-frame that use VMAs run after final section
placement.

I was intending to also make the discard_info changes but haven't
found time in the last month, so figure I ought to flush this patch
I've had sitting in my local tree for 6 weeks.

	* elf-eh-frame.c (struct cie.personality): Replace val with sym.
	(find_merged_cie): Identify personality functions by (bfd_id,index)
	pair when a local sym is used.

diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c
index 8c3712f..0f0a563 100644
--- a/bfd/elf-eh-frame.c
+++ b/bfd/elf-eh-frame.c
@@ -40,7 +40,10 @@ struct cie
   bfd_vma augmentation_size;
   union {
     struct elf_link_hash_entry *h;
-    bfd_vma val;
+    struct {
+      unsigned int bfd_id;
+      unsigned int index;
+    } sym;
     unsigned int reloc_index;
   } personality;
   asection *output_sec;
@@ -1030,8 +1033,12 @@ find_merged_cie (bfd *abfd, struct bfd_link_info *info, asection *sec,
     {
       bfd_boolean per_binds_local;
 
-      /* Work out the address of personality routine, either as an absolute
-	 value or as a symbol.  */
+      /* Work out the address of personality routine, or at least
+	 enough info that we could calculate the address had we made a
+	 final section layout.  The symbol on the reloc is enough,
+	 either the hash for a global, or (bfd id, index) pair for a
+	 local.  The assumption here is that no one uses addends on
+	 the reloc.  */
       rel = cookie->rels + cie->personality.reloc_index;
       memset (&cie->personality, 0, sizeof (cie->personality));
 #ifdef BFD64
@@ -1071,9 +1078,8 @@ find_merged_cie (bfd *abfd, struct bfd_link_info *info, asection *sec,
 	    return cie_inf;
 
 	  cie->local_personality = 1;
-	  cie->personality.val = (sym->st_value
-				  + sym_sec->output_offset
-				  + sym_sec->output_section->vma);
+	  cie->personality.sym.bfd_id = abfd->id;
+	  cie->personality.sym.index = r_symndx;
 	  per_binds_local = TRUE;
 	}
 
-- 
Alan Modra
Australia Development Lab, IBM