[PATCH] Fix DW_CFA_set_loc handling

[PATCH] Fix DW_CFA_set_loc handling

[Jakub Jelinek]

Hi!

Recent http://gcc.gnu.org/PR22313 change (including gcc 4.1 branch) started
(unnecessarily) using DW_CFA_set_loc at the start of all FDEs, but
unfortunately linker .eh_frame optimizations didn't handle that well.
If FDE encoding is pc relative or linker is changing it from absptr
to pc relative, we need to adjust the DW_CFA_set_loc operand accordingly.

Tested with make check on x86_64-linux, visual inspection of x86_64
readelf -wf libc.so.6 (this was seriously broken before) and visual
inspection of a hacked up test for absptr -> pcrel conversion.

Can anyone test this on e.g. mips (which I believe heavily converts from
absolute to relative)?

2006-09-26  Jakub Jelinek  <jakub@redhat.com>

	* elf-bfd.h (struct eh_cie_fde): Add set_loc pointer.
	* elf-eh-frame.c (skip_cfa_op): Fix handling of DW_CFA_advance_loc.
	Handle DW_CFA_{remember,restore}_state, DW_CFA_GNU_window_save,
	DW_CFA_val_{offset{,_sf},expression}.
	(skip_non_nops): Record number of DW_CFA_set_loc ops.
	(_bfd_elf_discard_section_eh_frame): Require skip_non_nops recognizes
	all ops.  If there are any DW_CFA_set_loc ops and they are pcrel
	or going to be pcrel, compute set_loc array.
	(_bfd_elf_eh_frame_section_offset): If make_relative, kill relocations
	against DW_CFA_set_loc operands.
	(_bfd_elf_write_section_eh_frame): Handle DW_CFA_set_loc adjusting.

--- bfd/elf-eh-frame.c.jj	2006-06-29 14:25:14.000000000 +0200
+++ bfd/elf-eh-frame.c	2006-09-26 14:12:57.000000000 +0200
@@ -273,11 +273,14 @@ skip_cfa_op (bfd_byte **iter, bfd_byte *
   if (!read_byte (iter, end, &op))
     return FALSE;
 
-  switch (op & 0x80 ? op & 0xc0 : op)
+  switch (op & 0xc0 ? op & 0xc0 : op)
     {
     case DW_CFA_nop:
     case DW_CFA_advance_loc:
     case DW_CFA_restore:
+    case DW_CFA_remember_state:
+    case DW_CFA_restore_state:
+    case DW_CFA_GNU_window_save:
       /* No arguments.  */
       return TRUE;
 
@@ -292,6 +295,8 @@ skip_cfa_op (bfd_byte **iter, bfd_byte *
       /* One leb128 argument.  */
       return skip_leb128 (iter, end);
 
+    case DW_CFA_val_offset:
+    case DW_CFA_val_offset_sf:
     case DW_CFA_offset_extended:
     case DW_CFA_register:
     case DW_CFA_def_cfa:
@@ -308,6 +313,7 @@ skip_cfa_op (bfd_byte **iter, bfd_byte *
 	      && skip_bytes (iter, end, length));
 
     case DW_CFA_expression:
+    case DW_CFA_val_expression:
       /* A leb128 followed by a variable-length argument.  */
       return (skip_leb128 (iter, end)
 	      && read_uleb128 (iter, end, &length)
@@ -339,7 +345,8 @@ skip_cfa_op (bfd_byte **iter, bfd_byte *
    ENCODED_PTR_WIDTH is as for skip_cfa_op.  */
 
 static bfd_byte *
-skip_non_nops (bfd_byte *buf, bfd_byte *end, unsigned int encoded_ptr_width)
+skip_non_nops (bfd_byte *buf, bfd_byte *end, unsigned int encoded_ptr_width,
+	       unsigned int *set_loc_count)
 {
   bfd_byte *last;
 
@@ -349,6 +356,8 @@ skip_non_nops (bfd_byte *buf, bfd_byte *
       buf++;
     else
       {
+	if (*buf == DW_CFA_set_loc)
+	  ++*set_loc_count;
 	if (!skip_cfa_op (&buf, end, encoded_ptr_width))
 	  return 0;
 	last = buf;
@@ -453,8 +462,9 @@ _bfd_elf_discard_section_eh_frame
   for (;;)
     {
       char *aug;
-      bfd_byte *start, *end, *insns;
+      bfd_byte *start, *end, *insns, *insns_end;
       bfd_size_type length;
+      unsigned int set_loc_count;
 
       if (sec_info->count == sec_info->alloced)
 	{
@@ -558,6 +568,7 @@ _bfd_elf_discard_section_eh_frame
 	  cie_usage_count = 0;
 	  memset (&cie, 0, sizeof (cie));
 	  cie.hdr = hdr;
+	  start = buf;
 	  REQUIRE (read_byte (&buf, end, &cie.version));
 
 	  /* Cannot handle unknown versions.  */
@@ -775,11 +786,38 @@ _bfd_elf_discard_section_eh_frame
 
       /* Try to interpret the CFA instructions and find the first
 	 padding nop.  Shrink this_inf's size so that it doesn't
-	 including the padding.  */
+	 include the padding.  */
       length = get_DW_EH_PE_width (cie.fde_encoding, ptr_size);
-      insns = skip_non_nops (insns, end, length);
-      if (insns != 0)
-	this_inf->size -= end - insns;
+      set_loc_count = 0;
+      insns_end = skip_non_nops (insns, end, length, &set_loc_count);
+      /* If we don't understand the CFA instructions, we can't know
+	 what needs to be adjusted there.  */
+      if (insns_end == NULL
+	  /* For the time being we don't support DW_CFA_set_loc in
+	     CIE instructions.  */
+	  || (set_loc_count && this_inf->cie))
+	goto free_no_table;
+      this_inf->size -= end - insns_end;
+      if (set_loc_count
+	  && ((cie.fde_encoding & 0xf0) == DW_EH_PE_pcrel
+	      || cie.make_relative))
+	{
+	  unsigned int cnt;
+	  bfd_byte *p;
+
+	  this_inf->set_loc = bfd_malloc ((set_loc_count + 1)
+					  * sizeof (unsigned int));
+	  REQUIRE (this_inf->set_loc);
+	  this_inf->set_loc[0] = set_loc_count;
+	  p = insns;
+	  cnt = 0;
+	  while (p < end)
+	    {
+	      if (*p == DW_CFA_set_loc)
+		this_inf->set_loc[++cnt] = p + 1 - start;
+	      REQUIRE (skip_cfa_op (&p, end, length));
+	    }
+	}
 
       this_inf->fde_encoding = cie.fde_encoding;
       this_inf->lsda_encoding = cie.lsda_encoding;
@@ -965,6 +1003,23 @@ _bfd_elf_eh_frame_section_offset (bfd *o
       return (bfd_vma) -2;
     }
 
+  /* If converting to DW_EH_PE_pcrel, there will be no need for run-time
+     relocation against DW_CFA_set_loc's arguments.  */
+  if (sec_info->entry[mid].set_loc
+      && (sec_info->entry[mid].cie
+	  ? sec_info->entry[mid].make_relative
+	  : sec_info->entry[mid].cie_inf->make_relative)
+      && (offset >= sec_info->entry[mid].offset + 8
+		    + sec_info->entry[mid].set_loc[1]))
+    {
+      unsigned int cnt;
+
+      for (cnt = 1; cnt <= sec_info->entry[mid].set_loc[0]; cnt++)
+	if (offset == sec_info->entry[mid].offset + 8
+		      + sec_info->entry[mid].set_loc[cnt])
+	  return (bfd_vma) -2;
+    }
+
   if (hdr_info->offsets_adjusted)
     offset -= sec->output_offset;
   /* Any new augmentation bytes go before the first relocation.  */
@@ -1189,6 +1244,7 @@ _bfd_elf_write_section_eh_frame (bfd *ab
 	  /* FDE */
 	  bfd_vma value, address;
 	  unsigned int width;
+	  bfd_byte *start;
 
 	  /* Skip length.  */
 	  buf += 4;
@@ -1225,6 +1281,8 @@ _bfd_elf_write_section_eh_frame (bfd *ab
 	      write_value (abfd, buf, value, width);
 	    }
 
+	  start = buf;
+
 	  if (hdr_info)
 	    {
 	      hdr_info->array[hdr_info->array_count].initial_loc = address;
@@ -1257,6 +1315,36 @@ _bfd_elf_write_section_eh_frame (bfd *ab
 	      memmove (buf + 1, buf, end - buf);
 	      *buf = 0;
 	    }
+
+	  if (ent->set_loc)
+	    {
+	      /* Adjust DW_CFA_set_loc.  */
+	      unsigned int cnt, width;
+	      bfd_vma new_offset;
+
+	      width = get_DW_EH_PE_width (ent->fde_encoding, ptr_size);
+	      new_offset = ent->new_offset + 8
+			   + extra_augmentation_string_bytes (ent)
+			   + extra_augmentation_data_bytes (ent);
+
+	      for (cnt = 1; cnt <= ent->set_loc[0]; cnt++)
+		{
+		  bfd_vma value;
+		  buf = start + ent->set_loc[cnt];
+
+		  value = read_value (abfd, buf, width,
+				      get_DW_EH_PE_signed (ent->fde_encoding));
+		  if (!value)
+		    continue;
+
+		  if ((ent->fde_encoding & 0xf0) == DW_EH_PE_pcrel)
+		    value += ent->offset + 8 - new_offset;
+		  if (ent->cie_inf->make_relative)
+		    value -= sec->output_section->vma + new_offset
+			     + ent->set_loc[cnt];
+		  write_value (abfd, buf, value, width);
+		}
+	    }
 	}
     }
 
--- bfd/elf-bfd.h.jj	2006-09-26 07:42:56.000000000 +0200
+++ bfd/elf-bfd.h	2006-09-26 13:38:45.000000000 +0200
@@ -304,6 +304,7 @@ struct eh_cie_fde
   unsigned int make_lsda_relative : 1;
   unsigned int need_lsda_relative : 1;
   unsigned int per_encoding_relative : 1;
+  unsigned int *set_loc;
 };
 
 struct eh_frame_sec_info


	Jakub

Re: [PATCH] Fix DW_CFA_set_loc handling

[Eric Botcazou]

Jakub,

> Recent http://gcc.gnu.org/PR22313 change (including gcc 4.1 branch) started
> (unnecessarily) using DW_CFA_set_loc at the start of all FDEs, but
> unfortunately linker .eh_frame optimizations didn't handle that well.
> If FDE encoding is pc relative or linker is changing it from absptr
> to pc relative, we need to adjust the DW_CFA_set_loc operand accordingly.
>
> Tested with make check on x86_64-linux, visual inspection of x86_64
> readelf -wf libc.so.6 (this was seriously broken before) and visual
> inspection of a hacked up test for absptr -> pcrel conversion.

This sounds a bit frightening, not everyone can afford to use binutils 
mainline with the 4.1.x compiler.  We already found a nasty fallout
  http://gcc.gnu.org/ml/gcc-patches/2006-09/msg00285.html
so the change seems to be really destabilizing at this point.

Would you be OK to jointly ask Roger to back it out on the 4.1 branch?

-- 
Eric Botcazou

Re: [PATCH] Fix DW_CFA_set_loc handling

[H. J. Lu]

On Tue, Sep 26, 2006 at 02:38:40PM +0200, Jakub Jelinek wrote:
> Hi!
> 
> Recent http://gcc.gnu.org/PR22313 change (including gcc 4.1 branch) started
> (unnecessarily) using DW_CFA_set_loc at the start of all FDEs, but
> unfortunately linker .eh_frame optimizations didn't handle that well.
> If FDE encoding is pc relative or linker is changing it from absptr
> to pc relative, we need to adjust the DW_CFA_set_loc operand accordingly.
> 
> Tested with make check on x86_64-linux, visual inspection of x86_64
> readelf -wf libc.so.6 (this was seriously broken before) and visual
> inspection of a hacked up test for absptr -> pcrel conversion.
> 

Is that possible to include a testcase, even just for one arch/format?



H.J.

Re: [PATCH] Fix DW_CFA_set_loc handling

[Jakub Jelinek]

On Tue, Sep 26, 2006 at 06:57:42AM -0700, H. J. Lu wrote:
> On Tue, Sep 26, 2006 at 02:38:40PM +0200, Jakub Jelinek wrote:
> > Hi!
> > 
> > Recent http://gcc.gnu.org/PR22313 change (including gcc 4.1 branch) started
> > (unnecessarily) using DW_CFA_set_loc at the start of all FDEs, but
> > unfortunately linker .eh_frame optimizations didn't handle that well.
> > If FDE encoding is pc relative or linker is changing it from absptr
> > to pc relative, we need to adjust the DW_CFA_set_loc operand accordingly.
> > 
> > Tested with make check on x86_64-linux, visual inspection of x86_64
> > readelf -wf libc.so.6 (this was seriously broken before) and visual
> > inspection of a hacked up test for absptr -> pcrel conversion.
> > 
> 
> Is that possible to include a testcase, even just for one arch/format?

Sure:

2006-09-26  Jakub Jelinek  <jakub@redhat.com>

	* ld-elf/eh4.d: New test.
	* ld-elf/eh4.s: New file.
	* ld-elf/eh4a.s: New file.

--- ld/testsuite/ld-elf/eh4.d.jj	2006-09-26 16:37:48.000000000 +0200
+++ ld/testsuite/ld-elf/eh4.d	2006-09-26 16:38:31.000000000 +0200
@@ -0,0 +1,32 @@
+#source: eh4.s
+#source: eh4a.s
+#ld: -shared
+#readelf: -wf
+#target: x86_64-*-*
+
+The section .eh_frame contains:
+
+00000000 00000014 00000000 CIE
+  Version:               1
+  Augmentation:          "zR"
+  Code alignment factor: 1
+  Data alignment factor: -8
+  Return address column: 16
+  Augmentation data:     1b
+
+  DW_CFA_def_cfa: r7 ofs 8
+  DW_CFA_offset: r16 at cfa-8
+  DW_CFA_nop
+  DW_CFA_nop
+
+00000018 00000014 0000001c FDE cie=00000000 pc=00000400..00000413
+  DW_CFA_set_loc: 00000404
+  DW_CFA_def_cfa_offset: 80
+
+00000030 00000014 00000034 FDE cie=00000000 pc=00000413..00000426
+  DW_CFA_set_loc: 00000417
+  DW_CFA_def_cfa_offset: 80
+
+00000048 ZERO terminator
+#pass
+
--- ld/testsuite/ld-elf/eh4.s.jj	2006-09-26 16:12:13.000000000 +0200
+++ ld/testsuite/ld-elf/eh4.s	2006-09-26 16:30:39.000000000 +0200
@@ -0,0 +1,92 @@
+	.text
+	.align	512
+	.globl foo
+	.type	foo, @function
+foo:
+.LFB1:
+	subq	$72, %rsp
+.LCFI1:
+	xorl	%eax, %eax
+	movq	%rsp, %rdi
+	call	bar@PLT
+	addq	$72, %rsp
+	ret
+.LFE1:
+	.size	foo, .-foo
+	.globl bar
+	.type	bar, @function
+bar:
+.LFB2:
+	subq	$72, %rsp
+.LCFI2:
+	xorl	%eax, %eax
+	movq	%rsp, %rdi
+	call	bar@PLT
+	addq	$72, %rsp
+	ret
+.LFE2:
+	.size	bar, .-bar
+	.section	.eh_frame,"a",@progbits
+.Lframe1:
+	.long	.LECIE1-.LSCIE1	# Length of Common Information Entry
+.LSCIE1:
+	.long	0x0	# CIE Identifier Tag
+	.byte	0x1	# CIE Version
+	.ascii "zR\0"	# CIE Augmentation
+	.uleb128 0x1	# CIE Code Alignment Factor
+	.sleb128 -8	# CIE Data Alignment Factor
+	.byte	0x10	# CIE RA Column
+	.uleb128 0x1	# Augmentation size
+	.byte	0x1b	# FDE Encoding (pcrel sdata4)
+	.byte	0xc	# DW_CFA_def_cfa
+	.uleb128 0x7
+	.uleb128 0x8
+	.byte	0x90	# DW_CFA_offset, column 0x10
+	.uleb128 0x1
+	.align 8
+.LECIE1:
+.LSFDE1:
+	.long	.LEFDE1-.LASFDE1	# FDE Length
+.LASFDE1:
+	.long	.LASFDE1-.Lframe1	# FDE CIE offset
+	.long	.LFB1-.	# FDE initial location
+	.long	.LFE1-.LFB1	# FDE address range
+	.uleb128 0x0	# Augmentation size
+	.byte	0x1	# DW_CFA_set_loc
+	.long	.LCFI1-.
+	.byte	0xe	# DW_CFA_def_cfa_offset
+	.uleb128 0x50
+	.align 8
+.LEFDE1:
+.Lframe2:
+	.long	.LECIE2-.LSCIE2	# Length of Common Information Entry
+.LSCIE2:
+	.long	0x0	# CIE Identifier Tag
+	.byte	0x1	# CIE Version
+	.ascii "zR\0"	# CIE Augmentation
+	.uleb128 0x1	# CIE Code Alignment Factor
+	.sleb128 -8	# CIE Data Alignment Factor
+	.byte	0x10	# CIE RA Column
+	.uleb128 0x1	# Augmentation size
+	.byte	0x1b	# FDE Encoding (pcrel sdata4)
+	.byte	0xc	# DW_CFA_def_cfa
+	.uleb128 0x7
+	.uleb128 0x8
+	.byte	0x90	# DW_CFA_offset, column 0x10
+	.uleb128 0x1
+	.align 8
+.LECIE2:
+.LSFDE2:
+	.long	.LEFDE2-.LASFDE2	# FDE Length
+.LASFDE2:
+	.long	.LASFDE2-.Lframe2	# FDE CIE offset
+	.long	.LFB2-.	# FDE initial location
+	.long	.LFE2-.LFB2	# FDE address range
+	.uleb128 0x0	# Augmentation size
+	.byte	0x1	# DW_CFA_set_loc
+	.long	.LCFI2-.
+	.byte	0xe	# DW_CFA_def_cfa_offset
+	.uleb128 0x50
+	.align 8
+.LEFDE2:
+	.section	.note.GNU-stack,"",@progbits
--- ld/testsuite/ld-elf/eh4a.s.jj	2006-09-26 16:39:07.000000000 +0200
+++ ld/testsuite/ld-elf/eh4a.s	2006-05-24 04:39:16.000000000 +0200
@@ -0,0 +1,3 @@
+	.section	.eh_frame,"a",%progbits
+	.align	8
+	.zero	8

	Jakub

Re: [PATCH] Fix DW_CFA_set_loc handling

[David Daney]

Eric Botcazou wrote:
> Jakub,
> 
> 
>>Recent http://gcc.gnu.org/PR22313 change (including gcc 4.1 branch) started
>>(unnecessarily) using DW_CFA_set_loc at the start of all FDEs, but
>>unfortunately linker .eh_frame optimizations didn't handle that well.
>>If FDE encoding is pc relative or linker is changing it from absptr
>>to pc relative, we need to adjust the DW_CFA_set_loc operand accordingly.
>>
>>Tested with make check on x86_64-linux, visual inspection of x86_64
>>readelf -wf libc.so.6 (this was seriously broken before) and visual
>>inspection of a hacked up test for absptr -> pcrel conversion.
> 
> 
> This sounds a bit frightening, not everyone can afford to use binutils 
> mainline with the 4.1.x compiler.  We already found a nasty fallout
>   http://gcc.gnu.org/ml/gcc-patches/2006-09/msg00285.html
> so the change seems to be really destabilizing at this point.
> 
> Would you be OK to jointly ask Roger to back it out on the 4.1 branch?
> 

This has already been 'fixed'.

See: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29132

David Daney.

Re: [PATCH] Fix DW_CFA_set_loc handling

[Eric Botcazou]

> This has already been 'fixed'.
>
> See: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29132

What is 'this' exactly?  Why did Jakub need to write a patch?

-- 
Eric Botcazou

Re: [PATCH] Fix DW_CFA_set_loc handling

[David Daney]

Eric Botcazou wrote:
>>This has already been 'fixed'.
>>
>>See: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29132
> 
> 
> What is 'this' exactly?  Why did Jakub need to write a patch?
> 

GCC (4.1 and 4.2) was fixed so that it no longer generates (as many) 
DW_CFA_set_loc.  My understanding is that DW_CFA_set_loc can still be 
generated if GCC's hot-cold section partitioning is used and perhaps in 
other cases as well.

This does not change the fact that the .eh_frame optimizations were 
broken in ld.  So I think Jakub's patch is still needed.

I will try to test the ld patch on my MIPS system (with the GCC PR 29132 
patch reverted),  but that I will not be able to do that for several days.

David Daney

Re: [PATCH] Fix DW_CFA_set_loc handling

[Nick Clifton]

Hi Jakub,

> 2006-09-26  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* elf-bfd.h (struct eh_cie_fde): Add set_loc pointer.
> 	* elf-eh-frame.c (skip_cfa_op): Fix handling of DW_CFA_advance_loc.
> 	Handle DW_CFA_{remember,restore}_state, DW_CFA_GNU_window_save,
> 	DW_CFA_val_{offset{,_sf},expression}.
> 	(skip_non_nops): Record number of DW_CFA_set_loc ops.
> 	(_bfd_elf_discard_section_eh_frame): Require skip_non_nops recognizes
> 	all ops.  If there are any DW_CFA_set_loc ops and they are pcrel
> 	or going to be pcrel, compute set_loc array.
> 	(_bfd_elf_eh_frame_section_offset): If make_relative, kill relocations
> 	against DW_CFA_set_loc operands.
> 	(_bfd_elf_write_section_eh_frame): Handle DW_CFA_set_loc adjusting.

Please go ahead and apply this patch together with your test case:

>    2006-09-26  Jakub Jelinek  <jakub@redhat.com>
 >
 >	* ld-elf/eh4.d: New test.
 >	* ld-elf/eh4.s: New file.
 >	* ld-elf/eh4a.s: New file.

If problems turn up for non-x86 architectures then we will fix them as 
they arise.

Cheers
   Nick