[PATCH] PR debug/81570: dwarf2cfi.c: Update cfa.offset in create_pseudo_cfg

[PATCH] PR debug/81570: dwarf2cfi.c: Update cfa.offset in create_pseudo_cfg

[H.J. Lu]

execute_dwarf2_frame is called for each funtion.  But create_cie_data
is called only once to initialize cie_cfi_row for all functions.  Since
INCOMING_FRAME_SP_OFFSET may be different for each function, we can't
use the same INCOMING_FRAME_SP_OFFSET in cie_cfi_row for all functions.
This patch sets cie_cfi_row->cfa.offset to INCOMING_FRAME_SP_OFFSET in
create_pseudo_cfg which is called for each function.

Tested on x86-64.  OK for trunk?

Thanks.


H.J.
	PR debug/81570
	* dwarf2cfi.c (create_pseudo_cfg): Set cie_cfi_row->cfa.offset
	to INCOMING_FRAME_SP_OFFSET.
---
 gcc/dwarf2cfi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c
index a5f9832fc4a..c40f31d2f20 100644
--- a/gcc/dwarf2cfi.c
+++ b/gcc/dwarf2cfi.c
@@ -2831,6 +2831,9 @@ create_pseudo_cfg (void)
   memset (&ti, 0, sizeof (ti));
   ti.head = get_insns ();
   ti.beg_row = cie_cfi_row;
+  /* Set cfa.offset to INCOMING_FRAME_SP_OFFSET here since it may be
+     different for each function.  */
+  cie_cfi_row->cfa.offset = INCOMING_FRAME_SP_OFFSET;
   ti.cfa_store = cie_cfi_row->cfa;
   ti.cfa_temp.reg = INVALID_REGNUM;
   trace_info.quick_push (ti);
-- 
2.13.3

Re: [PATCH] PR debug/81570: dwarf2cfi.c: Update cfa.offset in create_pseudo_cfg

[Jason Merrill]

On Thu, Jul 27, 2017 at 3:50 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> execute_dwarf2_frame is called for each funtion.  But create_cie_data
> is called only once to initialize cie_cfi_row for all functions.  Since
> INCOMING_FRAME_SP_OFFSET may be different for each function, we can't
> use the same INCOMING_FRAME_SP_OFFSET in cie_cfi_row for all functions.
> This patch sets cie_cfi_row->cfa.offset to INCOMING_FRAME_SP_OFFSET in
> create_pseudo_cfg which is called for each function.
>
> Tested on x86-64.  OK for trunk?

This looks wrong.  cie_cfi_row is the state produced by the
instructions in the CIE, which don't vary between functions.  If
INCOMING_FRAME_SP_OFFSET varies, we need to add actual FDE
instructions to reflect that, not just clobber our current model of
what the CIE means.

Jason

Re: [PATCH] PR debug/81570: dwarf2cfi.c: Update cfa.offset in create_pseudo_cfg

[H.J. Lu]

On Tue, Oct 24, 2017 at 8:26 PM, Jason Merrill <jason@redhat.com> wrote:
> On Thu, Jul 27, 2017 at 3:50 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> execute_dwarf2_frame is called for each funtion.  But create_cie_data
>> is called only once to initialize cie_cfi_row for all functions.  Since
>> INCOMING_FRAME_SP_OFFSET may be different for each function, we can't
>> use the same INCOMING_FRAME_SP_OFFSET in cie_cfi_row for all functions.
>> This patch sets cie_cfi_row->cfa.offset to INCOMING_FRAME_SP_OFFSET in
>> create_pseudo_cfg which is called for each function.
>>
>> Tested on x86-64.  OK for trunk?
>
> This looks wrong.  cie_cfi_row is the state produced by the
> instructions in the CIE, which don't vary between functions.  If

/* The state of the first row of the FDE table, which includes the
   state provided by the CIE.  */
static GTY(()) dw_cfi_row *cie_cfi_row;

cie_cfi_row is created by

  cie_cfi_row = cur_row = new_cfi_row ();

  /* On entry, the Canonical Frame Address is at SP.  */
  memset (&loc, 0, sizeof (loc));
  loc.reg = dw_stack_pointer_regnum;
  loc.offset = INCOMING_FRAME_SP_OFFSET;
  def_cfa_1 (&loc);

and used by create_pseudo_cfg

  ti.beg_row = cie_cfi_row;
  ti.cfa_store = cie_cfi_row->cfa;

The problem is that the offset field in cie_cfi_row->cfa may not be the same for
all functions.  cie_cfi_row does change in this case.  My patch simply corrects
the offset in CFA of the first row of the FDE table.

> INCOMING_FRAME_SP_OFFSET varies, we need to add actual FDE
> instructions to reflect that, not just clobber our current model of
> what the CIE means.
>
> Jason



-- 
H.J.

Re: [PATCH] PR debug/81570: dwarf2cfi.c: Update cfa.offset in create_pseudo_cfg

[Jason Merrill]

On Tue, Oct 31, 2017 at 1:57 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Oct 24, 2017 at 8:26 PM, Jason Merrill <jason@redhat.com> wrote:
>> On Thu, Jul 27, 2017 at 3:50 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>> execute_dwarf2_frame is called for each funtion.  But create_cie_data
>>> is called only once to initialize cie_cfi_row for all functions.  Since
>>> INCOMING_FRAME_SP_OFFSET may be different for each function, we can't
>>> use the same INCOMING_FRAME_SP_OFFSET in cie_cfi_row for all functions.
>>> This patch sets cie_cfi_row->cfa.offset to INCOMING_FRAME_SP_OFFSET in
>>> create_pseudo_cfg which is called for each function.
>>>
>>> Tested on x86-64.  OK for trunk?
>>
>> This looks wrong.  cie_cfi_row is the state produced by the
>> instructions in the CIE, which don't vary between functions.  If
>
> /* The state of the first row of the FDE table, which includes the
>    state provided by the CIE.  */
> static GTY(()) dw_cfi_row *cie_cfi_row;
>
> cie_cfi_row is created by
>
>   cie_cfi_row = cur_row = new_cfi_row ();
>
>   /* On entry, the Canonical Frame Address is at SP.  */
>   memset (&loc, 0, sizeof (loc));
>   loc.reg = dw_stack_pointer_regnum;
>   loc.offset = INCOMING_FRAME_SP_OFFSET;
>   def_cfa_1 (&loc);
>
> and used by create_pseudo_cfg
>
>   ti.beg_row = cie_cfi_row;
>   ti.cfa_store = cie_cfi_row->cfa;
>
> The problem is that the offset field in cie_cfi_row->cfa may not be the same for
> all functions.

Sure, the desired value of the offset field may not be the same.  But
cie_cfi_row->cfa reflects what the actual DWARF instructions emitted
in the CIE tell the consumer.  If what those instructions tell the
consumer is wrong for some functions, then we need to add instructions
to the FDE for such functions in order to correct the information.
Pretending that the CIE means different things to different functions
will just mean that unwinding fails.

> cie_cfi_row does change in this case.  My patch simply corrects
> the offset in CFA of the first row of the FDE table.
>
>> INCOMING_FRAME_SP_OFFSET varies, we need to add actual FDE
>> instructions to reflect that, not just clobber our current model of
>> what the CIE means.
>>
>> Jason
>
>
>
> --
> H.J.