[PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[Jakub Jelinek]

Hi!

When .cfi_* directives need to skip from one fragment to another one and
that skip in the end turns to be >= 64KB, the generated .eh_frame section
is incorrect.  The problem is that we add the needed 1 byte fragment
before rs_cfa fragment, but don't initialize it, so it often remains
at DW_CFA_nop (or could be any other opcode).  Later on when
eh_frame_convert_frag is called, if the skip is small, the previous
fragment's byte is changed into DW_CFA_advance_loc | distance,
DW_CFA_advance_loc1 or DW_CFA_advance_loc2.  But if it is >= 65536,
ehopt.c assumes (IMHO correctly) that the byte before it is
DW_CFA_advance_loc4 and doesn't change it, so we end up with a random opcode
(usually DW_CFA_nop) followed by the 4 byte skip value (which is
interpreted as random, often invalid, opcodes).

Ok for trunk?

2009-06-09  Jakub Jelinek  <jakub@redhat.com>

	PR gas/10255
	* dw2gencfi.c (output_cfi_insn): Initialize fragment before rs_cfa
	to DW_CFA_advance_loc4.

	* gas/cfi/cfi-common-7.d: New test.
	* gas/cfi/cfi-common-7.s: New.
	* gas/cfi/cfi.exp: Add cfi-common-7 test.

--- gas/dw2gencfi.c.jj	2009-01-27 17:50:43.000000000 +0100
+++ gas/dw2gencfi.c	2009-06-09 13:51:19.000000000 +0200
@@ -1,5 +1,6 @@
 /* dw2gencfi.c - Support for generating Dwarf2 CFI information.
-   Copyright 2003, 2004, 2005, 2006, 2007, 2008 Free Software Foundation, Inc.
+   Copyright 2003, 2004, 2005, 2006, 2007, 2008, 2009
+   Free Software Foundation, Inc.
    Contributed by Michal Ludvig <mludvig@suse.cz>
 
    This file is part of GAS, the GNU Assembler.
@@ -1001,7 +1002,7 @@ output_cfi_insn (struct cfi_insn_data *i
 	       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.  */
-	    frag_more (1);
+	    *frag_more (1) = DW_CFA_advance_loc4;
 
 	    frag_var (rs_cfa, 4, 0, DWARF2_LINE_MIN_INSN_LENGTH << 3,
 		      make_expr_symbol (&exp), frag_now_fix () - 1,
--- gas/testsuite/gas/cfi/cfi.exp.jj	2008-09-25 20:39:42.000000000 +0200
+++ gas/testsuite/gas/cfi/cfi.exp	2009-06-09 14:07:08.000000000 +0200
@@ -89,6 +89,7 @@ if { ![istarget "hppa64*-*"] } then {
   run_dump_test "cfi-common-3"
   run_dump_test "cfi-common-4"
   run_dump_test "cfi-common-5"
+  run_dump_test "cfi-common-7"
 }
 
 # MIPS doesn't support PC relative cfi directives.
--- gas/testsuite/gas/cfi/cfi-common-7.d.jj	2009-06-09 13:58:25.000000000 +0200
+++ gas/testsuite/gas/cfi/cfi-common-7.d	2009-06-09 14:04:33.000000000 +0200
@@ -0,0 +1,22 @@
+#readelf: -wf
+#name: CFI common 7
+Contents of the .eh_frame section:
+
+00000000 00000010 00000000 CIE
+  Version:               1
+  Augmentation:          "zR"
+  Code alignment factor: .*
+  Data alignment factor: .*
+  Return address column: .*
+  Augmentation data:     [01]b
+
+  DW_CFA_nop
+  DW_CFA_nop
+  DW_CFA_nop
+
+00000014 000000(1c|20) 00000018 FDE cie=00000000 pc=.*
+  DW_CFA_advance_loc: 16 to .*
+  DW_CFA_def_cfa: r0( \([er]ax\)|) ofs 16
+  DW_CFA_advance_loc4: 75031 to .*
+  DW_CFA_def_cfa: r0( \([er]ax\)|) ofs 64
+#...
--- gas/testsuite/gas/cfi/cfi-common-7.s.jj	2009-06-09 13:57:19.000000000 +0200
+++ gas/testsuite/gas/cfi/cfi-common-7.s	2009-06-09 13:59:34.000000000 +0200
@@ -0,0 +1,6 @@
+	.cfi_startproc simple
+	.skip 16
+	.cfi_def_cfa 0, 16
+	.skip 75031
+	.cfi_def_cfa 0, 64
+	.cfi_endproc

	Jakub

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[H.J. Lu]

On Tue, Jun 9, 2009 at 5:28 AM, Jakub Jelinek<jakub@redhat.com> wrote:
> Hi!
>
> When .cfi_* directives need to skip from one fragment to another one and
> that skip in the end turns to be >= 64KB, the generated .eh_frame section
> is incorrect.  The problem is that we add the needed 1 byte fragment
> before rs_cfa fragment, but don't initialize it, so it often remains
> at DW_CFA_nop (or could be any other opcode).  Later on when
> eh_frame_convert_frag is called, if the skip is small, the previous
> fragment's byte is changed into DW_CFA_advance_loc | distance,
> DW_CFA_advance_loc1 or DW_CFA_advance_loc2.  But if it is >= 65536,
> ehopt.c assumes (IMHO correctly) that the byte before it is
> DW_CFA_advance_loc4 and doesn't change it, so we end up with a random opcode
> (usually DW_CFA_nop) followed by the 4 byte skip value (which is
> interpreted as random, often invalid, opcodes).
>
> Ok for trunk?
>

Will this patch:

Index: ehopt.c
===================================================================
--- ehopt.c     (revision 6057)
+++ ehopt.c     (working copy)
@@ -543,6 +543,7 @@ eh_frame_convert_frag (fragS *frag)
       break;

     default:
+      loc4_frag->fr_literal[loc4_fix] = DW_CFA_advance_loc4;
       md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 4);
       break;
     }

work better?


-- 
H.J.

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[Jakub Jelinek]

On Tue, Jun 09, 2009 at 05:46:03AM -0700, H.J. Lu wrote:
> Will this patch:
> 
> Index: ehopt.c
> ===================================================================
> --- ehopt.c     (revision 6057)
> +++ ehopt.c     (working copy)
> @@ -543,6 +543,7 @@ eh_frame_convert_frag (fragS *frag)
>        break;
> 
>      default:
> +      loc4_frag->fr_literal[loc4_fix] = DW_CFA_advance_loc4;
>        md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 4);
>        break;
>      }
> 
> work better?

It will work too, but not sure why would it be considered better.

	Jakub

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[H.J. Lu]

On Tue, Jun 9, 2009 at 5:58 AM, Jakub Jelinek<jakub@redhat.com> wrote:
> On Tue, Jun 09, 2009 at 05:46:03AM -0700, H.J. Lu wrote:
>> Will this patch:
>>
>> Index: ehopt.c
>> ===================================================================
>> --- ehopt.c     (revision 6057)
>> +++ ehopt.c     (working copy)
>> @@ -543,6 +543,7 @@ eh_frame_convert_frag (fragS *frag)
>>        break;
>>
>>      default:
>> +      loc4_frag->fr_literal[loc4_fix] = DW_CFA_advance_loc4;
>>        md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 4);
>>        break;
>>      }
>>
>> work better?
>
> It will work too, but not sure why would it be considered better.
>

We can avoid a memory write when DW_CFA_advance_loc4
isn't used.

-- 
H.J.

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[Jakub Jelinek]

On Tue, Jun 09, 2009 at 06:08:25AM -0700, H.J. Lu wrote:
> On Tue, Jun 9, 2009 at 5:58 AM, Jakub Jelinek<jakub@redhat.com> wrote:
> > On Tue, Jun 09, 2009 at 05:46:03AM -0700, H.J. Lu wrote:
> >> Will this patch:
> >>
> >> Index: ehopt.c
> >> ===================================================================
> >> --- ehopt.c     (revision 6057)
> >> +++ ehopt.c     (working copy)
> >> @@ -543,6 +543,7 @@ eh_frame_convert_frag (fragS *frag)
> >>        break;
> >>
> >>      default:
> >> +      loc4_frag->fr_literal[loc4_fix] = DW_CFA_advance_loc4;
> >>        md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 4);
> >>        break;
> >>      }
> >>
> >> work better?
> >
> > It will work too, but not sure why would it be considered better.
> >
> 
> We can avoid a memory write when DW_CFA_advance_loc4
> isn't used.

I don't think a memory write is something we should try hard to avoid, what
matters how it is maintanable.  IMHO leaving the fragment uninitialized
is a bad idea, but doing what you wrote above in eh_frame_convert_frag
doesn't hurt as well.

Anyway, I don't care about this too much, as long as the bug is fixed.

	Jakub

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[H.J. Lu]

On Tue, Jun 9, 2009 at 6:16 AM, Jakub Jelinek<jakub@redhat.com> wrote:
> On Tue, Jun 09, 2009 at 06:08:25AM -0700, H.J. Lu wrote:
>> On Tue, Jun 9, 2009 at 5:58 AM, Jakub Jelinek<jakub@redhat.com> wrote:
>> > On Tue, Jun 09, 2009 at 05:46:03AM -0700, H.J. Lu wrote:
>> >> Will this patch:
>> >>
>> >> Index: ehopt.c
>> >> ===================================================================
>> >> --- ehopt.c     (revision 6057)
>> >> +++ ehopt.c     (working copy)
>> >> @@ -543,6 +543,7 @@ eh_frame_convert_frag (fragS *frag)
>> >>        break;
>> >>
>> >>      default:
>> >> +      loc4_frag->fr_literal[loc4_fix] = DW_CFA_advance_loc4;
>> >>        md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 4);
>> >>        break;
>> >>      }
>> >>
>> >> work better?
>> >
>> > It will work too, but not sure why would it be considered better.
>> >
>>
>> We can avoid a memory write when DW_CFA_advance_loc4
>> isn't used.
>
> I don't think a memory write is something we should try hard to avoid, what
> matters how it is maintanable.  IMHO leaving the fragment uninitialized
> is a bad idea, but doing what you wrote above in eh_frame_convert_frag

It is uninitialized because we don't know what we will put in there.
If we want to initialize it, I think it should be initialized to something
invalid and verify it is valid when we output it. Otherwise, initialize
it may still generate bad output.


-- 
H.J.

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[Jakub Jelinek]

On Tue, Jun 09, 2009 at 06:27:37AM -0700, H.J. Lu wrote:
> > I don't think a memory write is something we should try hard to avoid, what
> > matters how it is maintanable.  IMHO leaving the fragment uninitialized
> > is a bad idea, but doing what you wrote above in eh_frame_convert_frag
> 
> It is uninitialized because we don't know what we will put in there.
> If we want to initialize it, I think it should be initialized to something
> invalid and verify it is valid when we output it. Otherwise, initialize
> it may still generate bad output.

I don't understand it.  We generate DW_CFA_advance_loc4 by default, and if
possible optimize it to something shorter.  So DW_CFA_advance_loc4 is the
right default...

	Jakub

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[H.J. Lu]

On Tue, Jun 9, 2009 at 6:32 AM, Jakub Jelinek<jakub@redhat.com> wrote:
> On Tue, Jun 09, 2009 at 06:27:37AM -0700, H.J. Lu wrote:
>> > I don't think a memory write is something we should try hard to avoid, what
>> > matters how it is maintanable.  IMHO leaving the fragment uninitialized
>> > is a bad idea, but doing what you wrote above in eh_frame_convert_frag
>>
>> It is uninitialized because we don't know what we will put in there.
>> If we want to initialize it, I think it should be initialized to something
>> invalid and verify it is valid when we output it. Otherwise, initialize
>> it may still generate bad output.
>
> I don't understand it.  We generate DW_CFA_advance_loc4 by default, and if
> possible optimize it to something shorter.  So DW_CFA_advance_loc4 is the
> right default...
>

That is true only because of the logic in  eh_frame_convert_frag.


-- 
H.J.

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[Jakub Jelinek]

On Tue, Jun 09, 2009 at 06:41:36AM -0700, H.J. Lu wrote:
> On Tue, Jun 9, 2009 at 6:32 AM, Jakub Jelinek<jakub@redhat.com> wrote:
> > On Tue, Jun 09, 2009 at 06:27:37AM -0700, H.J. Lu wrote:
> >> > I don't think a memory write is something we should try hard to avoid, what
> >> > matters how it is maintanable.  IMHO leaving the fragment uninitialized
> >> > is a bad idea, but doing what you wrote above in eh_frame_convert_frag
> >>
> >> It is uninitialized because we don't know what we will put in there.
> >> If we want to initialize it, I think it should be initialized to something
> >> invalid and verify it is valid when we output it. Otherwise, initialize
> >> it may still generate bad output.
> >
> > I don't understand it.  We generate DW_CFA_advance_loc4 by default, and if
> > possible optimize it to something shorter.  So DW_CFA_advance_loc4 is the
> > right default...
> >
> 
> That is true only because of the logic in  eh_frame_convert_frag.

That logic comes from the original rs_cfa use, to optimize
DW_CFA_advance_loc4 in compiler written .eh_frame and for that case
it is very reasonable to assume that the byte before it is
DW_CFA_advance_loc4, because the optimization doesn't happen for other
opcodes.  When dw2gencfi.c started using it, it should IMHO just do what is
needed to set it up the same as hand writte .eh_frame does.

	Jakub

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[Nick Clifton]

Hi Jakub,

> Ok for trunk?
> 
> 2009-06-09  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR gas/10255
> 	* dw2gencfi.c (output_cfi_insn): Initialize fragment before rs_cfa
> 	to DW_CFA_advance_loc4.
> 
> 	* gas/cfi/cfi-common-7.d: New test.
> 	* gas/cfi/cfi-common-7.s: New.
> 	* gas/cfi/cfi.exp: Add cfi-common-7 test.


Approved (or H.J.'s variation if you prefer), please apply.

Cheers
   Nick

Re: [PATCH] Fix .cfi_* directive skip over >= 64KB (PR gas/10255)

[Alan Modra]

	* gas/cfi/cfi-common-7.s: Skip a multiple of four.
	* gas/cfi/cfi-common-7.d: Adjust.

Index: gas/testsuite/gas/cfi/cfi-common-7.d
===================================================================
RCS file: /cvs/src/src/gas/testsuite/gas/cfi/cfi-common-7.d,v
retrieving revision 1.1
diff -u -p -r1.1 cfi-common-7.d
--- gas/testsuite/gas/cfi/cfi-common-7.d	9 Jun 2009 15:12:45 -0000	1.1
+++ gas/testsuite/gas/cfi/cfi-common-7.d	18 Jun 2009 13:59:44 -0000
@@ -14,9 +14,9 @@ Contents of the .eh_frame section:
   DW_CFA_nop
   DW_CFA_nop
 
-00000014 000000(1c|20) 00000018 FDE cie=00000000 pc=.*
+00000014 000000(18|1c|20) 00000018 FDE cie=00000000 pc=.*
   DW_CFA_advance_loc: 16 to .*
   DW_CFA_def_cfa: r0( \([er]ax\)|) ofs 16
-  DW_CFA_advance_loc4: 75031 to .*
+  DW_CFA_advance_loc[24]: 75040 to .*
   DW_CFA_def_cfa: r0( \([er]ax\)|) ofs 64
 #...
Index: gas/testsuite/gas/cfi/cfi-common-7.s
===================================================================
RCS file: /cvs/src/src/gas/testsuite/gas/cfi/cfi-common-7.s,v
retrieving revision 1.1
diff -u -p -r1.1 cfi-common-7.s
--- gas/testsuite/gas/cfi/cfi-common-7.s	9 Jun 2009 15:12:45 -0000	1.1
+++ gas/testsuite/gas/cfi/cfi-common-7.s	18 Jun 2009 13:59:44 -0000
@@ -1,6 +1,6 @@
 	.cfi_startproc simple
 	.skip 16
 	.cfi_def_cfa 0, 16
-	.skip 75031
+	.skip 75040
 	.cfi_def_cfa 0, 64
 	.cfi_endproc

-- 
Alan Modra
Australia Development Lab, IBM