[PATCH] x86-64: Fix CFI data for common_interrupt

[PATCH] x86-64: Fix CFI data for common_interrupt

[Mark Wielaard]

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 <mjw@redhat.com>
---
 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

Re: [PATCH] x86-64: Fix CFI data for common_interrupt

[Jan Beulich]

>>> 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.

Jan

> Signed-off-by: Mark Wielaard <mjw@redhat.com>
> ---
>  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

Re: [PATCH] x86-64: Fix CFI data for common_interrupt

[Mark Wielaard]

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.

Thanks,

Mark

Re: [PATCH] x86-64: Fix CFI data for common_interrupt

[Jan Beulich]

>>> 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

Re: [PATCH] x86-64: Fix CFI data for common_interrupt

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1371 bytes --]

On Tue, Feb 21, 2012 at 03:26:30PM +0000, Jan Beulich wrote:
> >>> On 21.02.12 at 15:43, Mark Wielaard <mjw@redhat.com> 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

[-- Attachment #2: 0001-x86-64-Fix-CFI-data-for-common_interrupt.patch --]
[-- Type: text/plain, Size: 1170 bytes --]

>From 5b48b64319463a4ccab560304ce3cf72c71d081d Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Tue, 21 Feb 2012 14:45:52 +0100
Subject: [PATCH] x86-64: Fix CFI data for common_interrupt

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 <mjw@redhat.com>
---
 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..308b604 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,16+SS-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

Re: [PATCH] x86-64: Fix CFI data for common_interrupt

[Jan Beulich]

>>> Mark Wielaard <mjw@redhat.com> 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 <mjw@redhat.com> 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

Re: [PATCH] x86-64: Fix CFI data for common_interrupt

[Jan Beulich]

>>> On 21.02.12 at 23:08, Mark Wielaard <mjw@redhat.com> 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 <jbeulich@suse.com>

when you re-submit.

Jan

[PATCH] x86-64: Fix CFI data for common_interrupt

[Mark J. Wielaard]

From: Mark Wielaard <mjw@redhat.com>

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 <mjw@redhat.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 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

[tip:x86/debug] x86-64: Fix CFI data for common_interrupt()

[tip-bot for Mark Wielaard]

Commit-ID:  928282e432ee584129a39da831ffa72c38e189b7
Gitweb:     http://git.kernel.org/tip/928282e432ee584129a39da831ffa72c38e189b7
Author:     Mark Wielaard <mjw@redhat.com>
AuthorDate: Fri, 24 Feb 2012 11:32:05 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
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 <mjw@redhat.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Link: http://lkml.kernel.org/r/1330079527-30711-1-git-send-email-mjw@redhat.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 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