Messages in this thread | | | Date | Tue, 21 Feb 2012 15:26:30 +0000 | From | "Jan Beulich" <> | Subject | Re: [PATCH] x86-64: Fix CFI data for common_interrupt |
| |
>>> On 21.02.12 at 15:43, Mark Wielaard <mjw@redhat.com> wrote: > On Tue, 2012-02-21 at 14:26 +0000, Jan Beulich wrote: >> >>> On 21.02.12 at 15:06, Mark Wielaard <mjw@redhat.com> wrote: >> > Commit eab9e6 "x86-64: Fix CFI data for interrupt frames" introduced >> > a DW_CFA_def_cfa_expression in the SAVE_ARGS_IRQ macro. To later define >> > the CFA using a simple register+offset rule both register and offset >> > need to be supplied. Just using CFI_DEF_CFA_REGISTER leaves the offset >> > undefined. So use CFI_DEF_CFA with reg+off explicitly at the end of >> > common_interrupt. >> >> NAK, unless you can prove a path via which the offset will remain >> unset until hitting a CFI_DEF_CFA_REGISTER. And if you indeed >> found such a path, the entry point of the path is where the problem >> ought to be fixed. >> >> Are you perhaps thinking that .cfi_def_cfa_register invalidates >> the offset in any way? That, to my knowledge, isn't the case, it >> just replaces the CFA register with the one specified, leaving the >> offset unchanged. > > DW_CFA_def_cfa_expression invalidates the offset (and register). Used > through the interrupt macro for do_IRQ which uses the SAVE_ARGS_IRQ to > define common_interrupt. So after using DW_CFA_def_cfa_expression we get > a CFI_DEF_REGISTER and the CFI for common_interrupt looks like: > > [ 6e30] FDE length=148 cie=[ 6e18] > CIE_pointer: 28184 > initial_location: 0xffffffff815e8d00 <common_interrupt> > address_range: 0x1ba > > Program: > [...] > advance_loc 1 to 0x69 > def_cfa_expression 6 > [ 0] breg7 0 > [ 2] deref > [ 3] const1u 136 > [ 5] plus > advance_loc 22 to 0x7f > def_cfa_register r4 (rsi) > [...] > > For DW_CFA_def_register DWARF4 explicitly says so: "This operation is > valid only if the current CFA rule is defined to use a register and > offset." So one needs to use CFI_DEF_CFA with both a register and an > offset here after the def_cfa_expression.
Hmm, that's in contrast to the gas implementation (but I'd certainly give the specification preference if it explicitly states so, so gas should at least emit a warning here rather than considering this valid).
But provided the specification mandates this, I'm okay with the change in principle. Just that specifying an offset of 0 doesn't look right then.
Jan
| |