| Message ID | 20170727195014.GA10240@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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);