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.
Signed-off-by: Mark Wielaard <[email protected]>
---
arch/x86/kernel/entry_64.S | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 3fe8239..e00ef55 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -813,7 +813,7 @@ ret_from_intr:
/* Restore saved previous stack */
popq %rsi
- CFI_DEF_CFA_REGISTER rsi
+ CFI_DEF_CFA rsi,0 /* needed after def_cfa_expression */
leaq ARGOFFSET-RBP(%rsi), %rsp
CFI_DEF_CFA_REGISTER rsp
CFI_ADJUST_CFA_OFFSET RBP-ARGOFFSET
--
1.7.7.6
>>> On 21.02.12 at 15:06, Mark Wielaard <[email protected]> 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.
Jan
> Signed-off-by: Mark Wielaard <[email protected]>
> ---
> arch/x86/kernel/entry_64.S | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index 3fe8239..e00ef55 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -813,7 +813,7 @@ ret_from_intr:
>
> /* Restore saved previous stack */
> popq %rsi
> - CFI_DEF_CFA_REGISTER rsi
> + CFI_DEF_CFA rsi,0 /* needed after def_cfa_expression */
> leaq ARGOFFSET-RBP(%rsi), %rsp
> CFI_DEF_CFA_REGISTER rsp
> CFI_ADJUST_CFA_OFFSET RBP-ARGOFFSET
> --
> 1.7.7.6
On Tue, 2012-02-21 at 14:26 +0000, Jan Beulich wrote:
> >>> On 21.02.12 at 15:06, Mark Wielaard <[email protected]> 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.
Thanks,
Mark
>>> On 21.02.12 at 15:43, Mark Wielaard <[email protected]> wrote:
> On Tue, 2012-02-21 at 14:26 +0000, Jan Beulich wrote:
>> >>> On 21.02.12 at 15:06, Mark Wielaard <[email protected]> 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
On Tue, Feb 21, 2012 at 03:26:30PM +0000, Jan Beulich wrote:
> >>> On 21.02.12 at 15:43, Mark Wielaard <[email protected]> wrote:
> > 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).
I am afraid gas cannot help us here. Since like you pointed out in your
patch:
This requires the use of .cfi_escape (allowing arbitrary byte
streams to be emitted into .eh_frame), as there is no
.cfi_def_cfa_expression (which also cannot reasonably be
expected, as it would require a full expression parser).
So we are on our own here.
> 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.
Yeah, I dunno what I was thinking. The offset should be set to the offset
that was there before when rsi was pushed. The attached patch does that
by using the same value as was used at the start of common_interrupt.
Does that look OK?
Thanks,
Mark
>>> Mark Wielaard <[email protected]> 02/21/12 11:08 PM >>>
>On Tue, Feb 21, 2012 at 03:26:30PM +0000, Jan Beulich wrote:
>> >>> On 21.02.12 at 15:43, Mark Wielaard <[email protected]> wrote:
>> > 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).
>
>I am afraid gas cannot help us here. Since like you pointed out in your
>patch:
>
>This requires the use of .cfi_escape (allowing arbitrary byte
>streams to be emitted into .eh_frame), as there is no
>.cfi_def_cfa_expression (which also cannot reasonably be
>expected, as it would require a full expression parser).
>
>So we are on our own here.
Hmm, yes, probably it wouldn't be nice if gas reset all its state when
.cfi_escape is used.
>> 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.
>
>Yeah, I dunno what I was thinking. The offset should be set to the offset
>that was there before when rsi was pushed. The attached patch does that
>by using the same value as was used at the start of common_interrupt.
>Does that look OK?
I would have thought that it should be SS+8-RBP (as %rbp is at the top
of the stack at that point). I can't verify this immediately, though, as I'm
not in the office today.
Jan
>>> On 21.02.12 at 23:08, Mark Wielaard <[email protected]> wrote:
> On Tue, Feb 21, 2012 at 03:26:30PM +0000, Jan Beulich wrote:
>> 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.
>
> Yeah, I dunno what I was thinking. The offset should be set to the offset
> that was there before when rsi was pushed. The attached patch does that
> by using the same value as was used at the start of common_interrupt.
> Does that look OK?
As written before, it ought to be
CFI_DEF_CFA rsi,SS+8-RBP /* reg/off reset after def_cfa_expr */
With that, feel free to add
Acked-by: Jan Beulich <[email protected]>
when you re-submit.
Jan
From: Mark Wielaard <[email protected]>
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.
Signed-off-by: Mark Wielaard <[email protected]>
Acked-by: Jan Beulich <[email protected]>
---
arch/x86/kernel/entry_64.S | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 3fe8239..54be36b 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -813,7 +813,7 @@ ret_from_intr:
/* Restore saved previous stack */
popq %rsi
- CFI_DEF_CFA_REGISTER rsi
+ CFI_DEF_CFA rsi,SS+8-RBP /* reg/off reset after def_cfa_expr */
leaq ARGOFFSET-RBP(%rsi), %rsp
CFI_DEF_CFA_REGISTER rsp
CFI_ADJUST_CFA_OFFSET RBP-ARGOFFSET
--
1.7.7.6
Commit-ID: 928282e432ee584129a39da831ffa72c38e189b7
Gitweb: http://git.kernel.org/tip/928282e432ee584129a39da831ffa72c38e189b7
Author: Mark Wielaard <[email protected]>
AuthorDate: Fri, 24 Feb 2012 11:32:05 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Mon, 27 Feb 2012 10:46:14 +0100
x86-64: Fix CFI data for common_interrupt()
Commit eab9e6137f23 ("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.
Signed-off-by: Mark Wielaard <[email protected]>
Acked-by: Jan Beulich <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/entry_64.S | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 3fe8239..54be36b 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -813,7 +813,7 @@ ret_from_intr:
/* Restore saved previous stack */
popq %rsi
- CFI_DEF_CFA_REGISTER rsi
+ CFI_DEF_CFA rsi,SS+8-RBP /* reg/off reset after def_cfa_expr */
leaq ARGOFFSET-RBP(%rsi), %rsp
CFI_DEF_CFA_REGISTER rsp
CFI_ADJUST_CFA_OFFSET RBP-ARGOFFSET