Optimise away eh_frame advance_loc 0

Message ID 20191026095443.GD3433@bubble.grove.modra.org
State New
Headers show
Series
  • Optimise away eh_frame advance_loc 0
Related show

Commit Message

Alan Modra Oct. 26, 2019, 9:54 a.m.
These can be generated when multiple cfi directives are emitted for an
instruction and the insn frag is closed off between directives, as
happens when listings are enabled.  No doubt the advance_loc of zero
could be avoided by backtracking over frags in dw2gencfi.c before
calling cfi_add_advance_loc, but that seems like more work than
cleaning up afterwards as this patch does.

Noticed when looking at the testcase in PR25125.

	PR 25125
	* dw2gencfi.c (output_cfi_insn): Don't output DW_CFA_advance_loc+0.
	* ehopt.c (eh_frame_estimate_size_before_relax): Return -1 for
	an advance_loc of zero.
	(eh_frame_relax_frag): Translate fr_subtype of 7 to size -1.
	(eh_frame_convert_frag): Handle fr_subtype of 7.  Abort on
	unexpected fr_subtype.


-- 
Alan Modra
Australia Development Lab, IBM

Comments

Joseph Myers Oct. 28, 2019, 4:16 p.m. | #1
This breaks the build of glibc for sh4-linux-gnu (confirmed this is the 
revision that introduces the breakage) and mips64-linux-gnu (not tested 
the specific revisions, but the symptoms are the same so it looks like 
the same breakage).

https://sourceware.org/ml/libc-testresults/2019-q4/msg00129.html

I've attached malloc.s (gzipped) from an sh4-linux-gnu glibc build.  The 
symptom is:

$ sh4-glibc-linux-gnu-as -little -o malloc.o malloc.s
malloc.s: Assembler messages:
malloc.s: Fatal error: can't write -1 bytes to section .debug_frame of malloc.o: 'bad value'

(and binutils commit 6f69abb0498286297936a178ba81c7e445aa4437 fails while 
the previous commit passes).

-- 
Joseph S. Myers
joseph@codesourcery.com
Jeff Law Oct. 28, 2019, 4:21 p.m. | #2
On 10/28/19 10:16 AM, Joseph Myers wrote:
> This breaks the build of glibc for sh4-linux-gnu (confirmed this is the 

> revision that introduces the breakage) and mips64-linux-gnu (not tested 

> the specific revisions, but the symptoms are the same so it looks like 

> the same breakage).

> 

> https://sourceware.org/ml/libc-testresults/2019-q4/msg00129.html

> 

> I've attached malloc.s (gzipped) from an sh4-linux-gnu glibc build.  The 

> symptom is:

> 

> $ sh4-glibc-linux-gnu-as -little -o malloc.o malloc.s

> malloc.s: Assembler messages:

> malloc.s: Fatal error: can't write -1 bytes to section .debug_frame of malloc.o: 'bad value'

> 

> (and binutils commit 6f69abb0498286297936a178ba81c7e445aa4437 fails while 

> the previous commit passes).

> 

I'm seeing this across a variety of architectures in my tester as well.

jeff
Alan Modra Oct. 28, 2019, 11:25 p.m. | #3
On Mon, Oct 28, 2019 at 04:16:30PM +0000, Joseph Myers wrote:
> This breaks the build of glibc for sh4-linux-gnu (confirmed this is the 

> revision that introduces the breakage) and mips64-linux-gnu (not tested 

> the specific revisions, but the symptoms are the same so it looks like 

> the same breakage).

> 

> https://sourceware.org/ml/libc-testresults/2019-q4/msg00129.html

> 

> I've attached malloc.s (gzipped) from an sh4-linux-gnu glibc build.  The 


Thanks.

> symptom is:

> $ sh4-glibc-linux-gnu-as -little -o malloc.o malloc.s

> malloc.s: Assembler messages:

> malloc.s: Fatal error: can't write -1 bytes to section .debug_frame of malloc.o: 'bad value'

> 


Looks like I committed the sin that catches all new gas developers
(and even this old one) of assuming no frag breaks at critical points.
The following should fix it.  I'll commit this after running some tests.

diff --git a/gas/dw2gencfi.c b/gas/dw2gencfi.c
index b01e4c4a9e..6c0478a720 100644
--- a/gas/dw2gencfi.c
+++ b/gas/dw2gencfi.c
@@ -1630,7 +1630,12 @@ output_cfi_insn (struct cfi_insn_data *insn)
 	    /* The code in ehopt.c expects that one byte of the encoding
 	       is already allocated to the frag.  This comes from the way
 	       that it scans the .eh_frame section looking first for the
-	       .byte DW_CFA_advance_loc4.  */
+	       .byte DW_CFA_advance_loc4.  Call frag_grow with the sum of
+	       room needed by frag_more and frag_var to preallocate space
+	       ensuring that the DW_CFA_advance_loc4 is in the fixed part
+	       of the rs_cfa frag, so that the relax machinery can remove
+	       the advance_loc should it advance by zero.  */
+	    frag_grow (5);
 	    *frag_more (1) = DW_CFA_advance_loc4;
 
 	    frag_var (rs_cfa, 4, 0, DWARF2_LINE_MIN_INSN_LENGTH << 3,

-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/gas/dw2gencfi.c b/gas/dw2gencfi.c
index 388123fd24..b01e4c4a9e 100644
--- a/gas/dw2gencfi.c
+++ b/gas/dw2gencfi.c
@@ -1598,7 +1598,9 @@  output_cfi_insn (struct cfi_insn_data *insn)
 	    addressT delta = S_GET_VALUE (to) - S_GET_VALUE (from);
 	    addressT scaled = delta / DWARF2_LINE_MIN_INSN_LENGTH;
 
-	    if (scaled <= 0x3F)
+	    if (scaled == 0)
+	      ;
+	    else if (scaled <= 0x3F)
 	      out_one (DW_CFA_advance_loc + scaled);
 	    else if (scaled <= 0xFF)
 	      {
diff --git a/gas/ehopt.c b/gas/ehopt.c
index 207e799405..bf65602f53 100644
--- a/gas/ehopt.c
+++ b/gas/ehopt.c
@@ -482,7 +482,9 @@  eh_frame_estimate_size_before_relax (fragS *frag)
 
   gas_assert (ca > 0);
   diff /= ca;
-  if (diff < 0x40)
+  if (diff == 0)
+    ret = -1;
+  else if (diff < 0x40)
     ret = 0;
   else if (diff < 0x100)
     ret = 1;
@@ -491,7 +493,7 @@  eh_frame_estimate_size_before_relax (fragS *frag)
   else
     ret = 4;
 
-  frag->fr_subtype = (frag->fr_subtype & ~7) | ret;
+  frag->fr_subtype = (frag->fr_subtype & ~7) | (ret & 7);
 
   return ret;
 }
@@ -506,6 +508,8 @@  eh_frame_relax_frag (fragS *frag)
   int oldsize, newsize;
 
   oldsize = frag->fr_subtype & 7;
+  if (oldsize == 7)
+    oldsize = -1;
   newsize = eh_frame_estimate_size_before_relax (frag);
   return newsize - oldsize;
 }
@@ -548,9 +552,17 @@  eh_frame_convert_frag (fragS *frag)
       md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 2);
       break;
 
-    default:
+    case 4:
       md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 4);
       break;
+
+    case 7:
+      gas_assert (diff == 0);
+      frag->fr_fix -= 8;
+      break;
+
+    default:
+      abort ();
     }
 
   frag->fr_fix += frag->fr_subtype & 7;