All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86-64: fix unwind annotations in entry_64.S
@ 2009-03-12 10:32 Jan Beulich
  2009-03-12 10:48 ` Ingo Molnar
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2009-03-12 10:32 UTC (permalink / raw)
  To: mingo, tglx, hpa; +Cc: linux-kernel

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

Those were completely screwed up with the recent rework.

(This patch applies to plain 2.6.29-rc7, a version that applies to tip
is attached.)

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 arch/x86/kernel/entry_64.S |  193 +++++++++++++++++++++++----------------------
 1 file changed, 102 insertions(+), 91 deletions(-)

--- linux-2.6.29-rc7/arch/x86/kernel/entry_64.S	2009-03-11 17:52:10.000000000 +0100
+++ 2.6.29-rc7-x86_64-unwind/arch/x86/kernel/entry_64.S	2009-03-10 16:41:53.000000000 +0100
@@ -38,6 +38,7 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/stringify.h>
 #include <asm/segment.h>
 #include <asm/cache.h>
 #include <asm/errno.h>
@@ -255,21 +256,21 @@ ENTRY(native_usergs_sysret64)
 /*
  * initial frame state for interrupts (and exceptions without error code)
  */
-	.macro EMPTY_FRAME start=1 offset=0
-	.if \start
+	.macro EMPTY_FRAME offset=0
 	CFI_STARTPROC simple
 	CFI_SIGNAL_FRAME
-	CFI_DEF_CFA rsp,8+\offset
-	.else
-	CFI_DEF_CFA_OFFSET 8+\offset
-	.endif
+	CFI_DEF_CFA rsp,\offset
 	.endm
 
 /*
  * initial frame state for interrupts (and exceptions without error code)
  */
 	.macro INTR_FRAME start=1 offset=0
-	EMPTY_FRAME \start, SS+8+\offset-RIP
+	.if \start
+	EMPTY_FRAME __stringify(SS+8+\offset-RIP)
+	.else
+	CFI_DEF_CFA_OFFSET SS+8+\offset-RIP
+	.endif
 	/*CFI_REL_OFFSET ss, SS+\offset-RIP*/
 	CFI_REL_OFFSET rsp, RSP+\offset-RIP
 	/*CFI_REL_OFFSET rflags, EFLAGS+\offset-RIP*/
@@ -282,15 +283,16 @@ ENTRY(native_usergs_sysret64)
  * with vector already pushed)
  */
 	.macro XCPT_FRAME start=1 offset=0
-	INTR_FRAME \start, RIP+\offset-ORIG_RAX
-	/*CFI_REL_OFFSET orig_rax, ORIG_RAX-ORIG_RAX*/
+	INTR_FRAME \start, __stringify(RIP+\offset-ORIG_RAX)
 	.endm
 
 /*
  * frame that enables calling into C.
  */
 	.macro PARTIAL_FRAME start=1 offset=0
-	XCPT_FRAME \start, ORIG_RAX+\offset-ARGOFFSET
+	.if \start >= 0
+	XCPT_FRAME \start, __stringify(ORIG_RAX+\offset-ARGOFFSET)
+	.endif
 	CFI_REL_OFFSET rdi, RDI+\offset-ARGOFFSET
 	CFI_REL_OFFSET rsi, RSI+\offset-ARGOFFSET
 	CFI_REL_OFFSET rdx, RDX+\offset-ARGOFFSET
@@ -306,7 +308,9 @@ ENTRY(native_usergs_sysret64)
  * frame that enables passing a complete pt_regs to a C function.
  */
 	.macro DEFAULT_FRAME start=1 offset=0
-	PARTIAL_FRAME \start, R11+\offset-R15
+	.if \start >= -1
+	PARTIAL_FRAME \start, __stringify(R11+\offset-R15)
+	.endif
 	CFI_REL_OFFSET rbx, RBX+\offset
 	CFI_REL_OFFSET rbp, RBP+\offset
 	CFI_REL_OFFSET r12, R12+\offset
@@ -317,21 +321,23 @@ ENTRY(native_usergs_sysret64)
 
 /* save partial stack frame */
 ENTRY(save_args)
-	XCPT_FRAME
+	XCPT_FRAME offset=__stringify(ORIG_RAX-ARGOFFSET+16)
 	cld
-	movq_cfi rdi, RDI+16-ARGOFFSET
-	movq_cfi rsi, RSI+16-ARGOFFSET
-	movq_cfi rdx, RDX+16-ARGOFFSET
-	movq_cfi rcx, RCX+16-ARGOFFSET
-	movq_cfi rax, RAX+16-ARGOFFSET
-	movq_cfi  r8,  R8+16-ARGOFFSET
-	movq_cfi  r9,  R9+16-ARGOFFSET
-	movq_cfi r10, R10+16-ARGOFFSET
-	movq_cfi r11, R11+16-ARGOFFSET
+	movq %rdi, RDI+16-ARGOFFSET(%rsp)
+	movq %rsi, RSI+16-ARGOFFSET(%rsp)
+	movq %rdx, RDX+16-ARGOFFSET(%rsp)
+	movq %rcx, RCX+16-ARGOFFSET(%rsp)
+	movq_cfi rax, __stringify(RAX+16-ARGOFFSET)
+	movq  %r8,  R8+16-ARGOFFSET(%rsp)
+	movq  %r9,  R9+16-ARGOFFSET(%rsp)
+	movq %r10, R10+16-ARGOFFSET(%rsp)
+	movq_cfi r11, __stringify(R11+16-ARGOFFSET)
 
 	leaq -ARGOFFSET+16(%rsp),%rdi	/* arg1 for handler */
 	movq_cfi rbp, 8		/* push %rbp */
 	leaq 8(%rsp), %rbp		/* mov %rsp, %ebp */
+	CFI_DEF_CFA_REGISTER rbp
+	CFI_ADJUST_CFA_OFFSET -8
 	testl $3, CS(%rdi)
 	je 1f
 	SWAPGS
@@ -343,11 +349,10 @@ ENTRY(save_args)
 	 */
 1:	incl %gs:pda_irqcount
 	jne 2f
-	popq_cfi %rax			/* move return address... */
+	popq %rax			/* move return address... */
 	mov %gs:pda_irqstackptr,%rsp
-	EMPTY_FRAME 0
-	pushq_cfi %rbp			/* backlink for unwinder */
-	pushq_cfi %rax			/* ... to the new stack */
+	pushq %rbp			/* backlink for unwinder */
+	pushq %rax			/* ... to the new stack */
 	/*
 	 * We entered an interrupt context - irqs are off:
 	 */
@@ -357,14 +362,14 @@ ENTRY(save_args)
 END(save_args)
 
 ENTRY(save_rest)
-	PARTIAL_FRAME 1 REST_SKIP+8
+	CFI_STARTPROC
 	movq 5*8+16(%rsp), %r11	/* save return address */
-	movq_cfi rbx, RBX+16
-	movq_cfi rbp, RBP+16
-	movq_cfi r12, R12+16
-	movq_cfi r13, R13+16
-	movq_cfi r14, R14+16
-	movq_cfi r15, R15+16
+	movq %rbx, RBX+16(%rsp)
+	movq %rbp, RBP+16(%rsp)
+	movq %r12, R12+16(%rsp)
+	movq %r13, R13+16(%rsp)
+	movq %r14, R14+16(%rsp)
+	movq %r15, R15+16(%rsp)
 	movq %r11, 8(%rsp)	/* return address */
 	FIXUP_TOP_OF_STACK %r11, 16
 	ret
@@ -373,23 +378,23 @@ END(save_rest)
 
 /* save complete stack frame */
 ENTRY(save_paranoid)
-	XCPT_FRAME 1 RDI+8
+	XCPT_FRAME offset=__stringify(ORIG_RAX-R15+8)
 	cld
-	movq_cfi rdi, RDI+8
-	movq_cfi rsi, RSI+8
-	movq_cfi rdx, RDX+8
-	movq_cfi rcx, RCX+8
-	movq_cfi rax, RAX+8
-	movq_cfi r8, R8+8
-	movq_cfi r9, R9+8
-	movq_cfi r10, R10+8
-	movq_cfi r11, R11+8
-	movq_cfi rbx, RBX+8
-	movq_cfi rbp, RBP+8
-	movq_cfi r12, R12+8
-	movq_cfi r13, R13+8
-	movq_cfi r14, R14+8
-	movq_cfi r15, R15+8
+	movq %rdi, RDI+8(%rsp)
+	movq %rsi, RSI+8(%rsp)
+	movq_cfi rdx, __stringify(RDX+8)
+	movq_cfi rcx, __stringify(RCX+8)
+	movq_cfi rax, __stringify(RAX+8)
+	movq %r8, R8+8(%rsp)
+	movq %r9, R9+8(%rsp)
+	movq %r10, R10+8(%rsp)
+	movq %r11, R11+8(%rsp)
+	movq_cfi rbx, __stringify(RBX+8)
+	movq %rbp, RBP+8(%rsp)
+	movq %r12, R12+8(%rsp)
+	movq %r13, R13+8(%rsp)
+	movq %r14, R14+8(%rsp)
+	movq %r15, R15+8(%rsp)
 	movl $1,%ebx
 	movl $MSR_GS_BASE,%ecx
 	rdmsr
@@ -706,7 +711,7 @@ ENTRY(\label)
 	subq $REST_SKIP, %rsp
 	CFI_ADJUST_CFA_OFFSET REST_SKIP
 	call save_rest
-	DEFAULT_FRAME 0 8		/* offset 8: return address */
+	DEFAULT_FRAME -2 8		/* offset 8: return address */
 	leaq 8(%rsp), \arg	/* pt_regs pointer */
 	call \func
 	jmp ptregscall_common
@@ -723,12 +728,12 @@ END(\label)
 ENTRY(ptregscall_common)
 	DEFAULT_FRAME 1 8	/* offset 8: return address */
 	RESTORE_TOP_OF_STACK %r11, 8
-	movq_cfi_restore R15+8, r15
-	movq_cfi_restore R14+8, r14
-	movq_cfi_restore R13+8, r13
-	movq_cfi_restore R12+8, r12
-	movq_cfi_restore RBP+8, rbp
-	movq_cfi_restore RBX+8, rbx
+	movq_cfi_restore __stringify(R15+8), r15
+	movq_cfi_restore __stringify(R14+8), r14
+	movq_cfi_restore __stringify(R13+8), r13
+	movq_cfi_restore __stringify(R12+8), r12
+	movq_cfi_restore __stringify(RBP+8), rbp
+	movq_cfi_restore __stringify(RBX+8), rbx
 	ret $REST_SKIP		/* pop extended registers */
 	CFI_ENDPROC
 END(ptregscall_common)
@@ -817,10 +822,12 @@ END(interrupt)
 
 /* 0(%rsp): ~(interrupt number) */
 	.macro interrupt func
-	subq $10*8, %rsp
-	CFI_ADJUST_CFA_OFFSET 10*8
+	subq $ORIG_RAX-ARGOFFSET+8, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-ARGOFFSET+8
 	call save_args
-	PARTIAL_FRAME 0
+	PARTIAL_FRAME -1 8
+	CFI_REL_OFFSET rbp, 0
+	CFI_DEF_CFA_REGISTER rbp
 	call \func
 	.endm
 
@@ -1033,10 +1040,10 @@ ENTRY(\sym)
 	INTR_FRAME
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 	pushq_cfi $-1		/* ORIG_RAX: no syscall to restart */
-	subq $15*8,%rsp
-	CFI_ADJUST_CFA_OFFSET 15*8
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call error_entry
-	DEFAULT_FRAME 0
+	DEFAULT_FRAME -1
 	movq %rsp,%rdi		/* pt_regs pointer */
 	xorl %esi,%esi		/* no error code */
 	call \do_sym
@@ -1051,8 +1058,10 @@ ENTRY(\sym)
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 	pushq $-1		/* ORIG_RAX: no syscall to restart */
 	CFI_ADJUST_CFA_OFFSET 8
-	subq $15*8, %rsp
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
+	DEFAULT_FRAME -1
 	TRACE_IRQS_OFF
 	movq %rsp,%rdi		/* pt_regs pointer */
 	xorl %esi,%esi		/* no error code */
@@ -1068,8 +1077,10 @@ ENTRY(\sym)
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 	pushq $-1		/* ORIG_RAX: no syscall to restart */
 	CFI_ADJUST_CFA_OFFSET 8
-	subq $15*8, %rsp
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
+	DEFAULT_FRAME -1
 	TRACE_IRQS_OFF
 	movq %rsp,%rdi		/* pt_regs pointer */
 	xorl %esi,%esi		/* no error code */
@@ -1086,10 +1097,10 @@ END(\sym)
 ENTRY(\sym)
 	XCPT_FRAME
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
-	subq $15*8,%rsp
-	CFI_ADJUST_CFA_OFFSET 15*8
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call error_entry
-	DEFAULT_FRAME 0
+	DEFAULT_FRAME -1
 	movq %rsp,%rdi			/* pt_regs pointer */
 	movq ORIG_RAX(%rsp),%rsi	/* get error code */
 	movq $-1,ORIG_RAX(%rsp)		/* no syscall to restart */
@@ -1104,10 +1115,10 @@ END(\sym)
 ENTRY(\sym)
 	XCPT_FRAME
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
-	subq $15*8,%rsp
-	CFI_ADJUST_CFA_OFFSET 15*8
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
-	DEFAULT_FRAME 0
+	DEFAULT_FRAME -1
 	TRACE_IRQS_OFF
 	movq %rsp,%rdi			/* pt_regs pointer */
 	movq ORIG_RAX(%rsp),%rsi	/* get error code */
@@ -1400,7 +1411,7 @@ paranoidzeroentry machine_check do_machi
 
 	/* ebx:	no swapgs flag */
 ENTRY(paranoid_exit)
-	INTR_FRAME
+	DEFAULT_FRAME
 	DISABLE_INTERRUPTS(CLBR_NONE)
 	TRACE_IRQS_OFF
 	testl %ebx,%ebx				/* swapgs needed? */
@@ -1447,25 +1458,24 @@ END(paranoid_exit)
  * returns in "no swapgs flag" in %ebx.
  */
 ENTRY(error_entry)
-	XCPT_FRAME
-	CFI_ADJUST_CFA_OFFSET 15*8
+	XCPT_FRAME offset=__stringify(ORIG_RAX-R15+8)
 	/* oldrax contains error code */
 	cld
-	movq_cfi rdi, RDI+8
-	movq_cfi rsi, RSI+8
-	movq_cfi rdx, RDX+8
-	movq_cfi rcx, RCX+8
-	movq_cfi rax, RAX+8
-	movq_cfi  r8,  R8+8
-	movq_cfi  r9,  R9+8
-	movq_cfi r10, R10+8
-	movq_cfi r11, R11+8
-	movq_cfi rbx, RBX+8
-	movq_cfi rbp, RBP+8
-	movq_cfi r12, R12+8
-	movq_cfi r13, R13+8
-	movq_cfi r14, R14+8
-	movq_cfi r15, R15+8
+	movq %rdi, RDI+8(%rsp)
+	movq %rsi, RSI+8(%rsp)
+	movq %rdx, RDX+8(%rsp)
+	movq %rcx, RCX+8(%rsp)
+	movq %rax, RAX+8(%rsp)
+	movq  %r8,  R8+8(%rsp)
+	movq  %r9,  R9+8(%rsp)
+	movq %r10, R10+8(%rsp)
+	movq %r11, R11+8(%rsp)
+	movq_cfi rbx, __stringify(RBX+8)
+	movq %rbp, RBP+8(%rsp)
+	movq %r12, R12+8(%rsp)
+	movq %r13, R13+8(%rsp)
+	movq %r14, R14+8(%rsp)
+	movq %r15, R15+8(%rsp)
 	xorl %ebx,%ebx
 	testl $3,CS+8(%rsp)
 	je error_kernelspace
@@ -1474,7 +1484,6 @@ error_swapgs:
 error_sti:
 	TRACE_IRQS_OFF
 	ret
-	CFI_ENDPROC
 
 /*
  * There are two places in the kernel that can potentially fault with
@@ -1484,6 +1493,7 @@ error_sti:
  * compat mode. Check for these here too.
  */
 error_kernelspace:
+	CFI_REL_OFFSET rcx, RCX+8
 	incl %ebx
 	leaq irq_return(%rip),%rcx
 	cmpq %rcx,RIP+8(%rsp)
@@ -1494,6 +1504,7 @@ error_kernelspace:
 	cmpq $gs_change,RIP+8(%rsp)
 	je error_swapgs
 	jmp error_sti
+	CFI_ENDPROC
 END(error_entry)
 
 
@@ -1522,10 +1533,10 @@ ENTRY(nmi)
 	INTR_FRAME
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 	pushq_cfi $-1
-	subq $15*8, %rsp
-	CFI_ADJUST_CFA_OFFSET 15*8
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
-	DEFAULT_FRAME 0
+	DEFAULT_FRAME -1
 	/* paranoidentry do_nmi, 0; without TRACE_IRQS_OFF */
 	movq %rsp,%rdi
 	movq $-1,%rsi



[-- Attachment #2: linux-2.6.29-rc7-x86_64-unwind.patch --]
[-- Type: text/plain, Size: 11098 bytes --]

Those were completely screwed up with the recent rework.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 arch/x86/kernel/entry_64.S |  193 +++++++++++++++++++++++----------------------
 1 file changed, 102 insertions(+), 91 deletions(-)

--- linux-2.6.29-rc7/arch/x86/kernel/entry_64.S	2009-03-11 17:52:10.000000000 +0100
+++ 2.6.29-rc7-x86_64-unwind/arch/x86/kernel/entry_64.S	2009-03-10 16:41:53.000000000 +0100
@@ -38,6 +38,7 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/stringify.h>
 #include <asm/segment.h>
 #include <asm/cache.h>
 #include <asm/errno.h>
@@ -255,21 +256,21 @@ ENTRY(native_usergs_sysret64)
 /*
  * initial frame state for interrupts (and exceptions without error code)
  */
-	.macro EMPTY_FRAME start=1 offset=0
-	.if \start
+	.macro EMPTY_FRAME offset=0
 	CFI_STARTPROC simple
 	CFI_SIGNAL_FRAME
-	CFI_DEF_CFA rsp,8+\offset
-	.else
-	CFI_DEF_CFA_OFFSET 8+\offset
-	.endif
+	CFI_DEF_CFA rsp,\offset
 	.endm
 
 /*
  * initial frame state for interrupts (and exceptions without error code)
  */
 	.macro INTR_FRAME start=1 offset=0
-	EMPTY_FRAME \start, SS+8+\offset-RIP
+	.if \start
+	EMPTY_FRAME __stringify(SS+8+\offset-RIP)
+	.else
+	CFI_DEF_CFA_OFFSET SS+8+\offset-RIP
+	.endif
 	/*CFI_REL_OFFSET ss, SS+\offset-RIP*/
 	CFI_REL_OFFSET rsp, RSP+\offset-RIP
 	/*CFI_REL_OFFSET rflags, EFLAGS+\offset-RIP*/
@@ -282,15 +283,16 @@ ENTRY(native_usergs_sysret64)
  * with vector already pushed)
  */
 	.macro XCPT_FRAME start=1 offset=0
-	INTR_FRAME \start, RIP+\offset-ORIG_RAX
-	/*CFI_REL_OFFSET orig_rax, ORIG_RAX-ORIG_RAX*/
+	INTR_FRAME \start, __stringify(RIP+\offset-ORIG_RAX)
 	.endm
 
 /*
  * frame that enables calling into C.
  */
 	.macro PARTIAL_FRAME start=1 offset=0
-	XCPT_FRAME \start, ORIG_RAX+\offset-ARGOFFSET
+	.if \start >= 0
+	XCPT_FRAME \start, __stringify(ORIG_RAX+\offset-ARGOFFSET)
+	.endif
 	CFI_REL_OFFSET rdi, RDI+\offset-ARGOFFSET
 	CFI_REL_OFFSET rsi, RSI+\offset-ARGOFFSET
 	CFI_REL_OFFSET rdx, RDX+\offset-ARGOFFSET
@@ -306,7 +308,9 @@ ENTRY(native_usergs_sysret64)
  * frame that enables passing a complete pt_regs to a C function.
  */
 	.macro DEFAULT_FRAME start=1 offset=0
-	PARTIAL_FRAME \start, R11+\offset-R15
+	.if \start >= -1
+	PARTIAL_FRAME \start, __stringify(R11+\offset-R15)
+	.endif
 	CFI_REL_OFFSET rbx, RBX+\offset
 	CFI_REL_OFFSET rbp, RBP+\offset
 	CFI_REL_OFFSET r12, R12+\offset
@@ -317,21 +321,23 @@ ENTRY(native_usergs_sysret64)
 
 /* save partial stack frame */
 ENTRY(save_args)
-	XCPT_FRAME
+	XCPT_FRAME offset=__stringify(ORIG_RAX-ARGOFFSET+16)
 	cld
-	movq_cfi rdi, RDI+16-ARGOFFSET
-	movq_cfi rsi, RSI+16-ARGOFFSET
-	movq_cfi rdx, RDX+16-ARGOFFSET
-	movq_cfi rcx, RCX+16-ARGOFFSET
-	movq_cfi rax, RAX+16-ARGOFFSET
-	movq_cfi  r8,  R8+16-ARGOFFSET
-	movq_cfi  r9,  R9+16-ARGOFFSET
-	movq_cfi r10, R10+16-ARGOFFSET
-	movq_cfi r11, R11+16-ARGOFFSET
+	movq %rdi, RDI+16-ARGOFFSET(%rsp)
+	movq %rsi, RSI+16-ARGOFFSET(%rsp)
+	movq %rdx, RDX+16-ARGOFFSET(%rsp)
+	movq %rcx, RCX+16-ARGOFFSET(%rsp)
+	movq_cfi rax, __stringify(RAX+16-ARGOFFSET)
+	movq  %r8,  R8+16-ARGOFFSET(%rsp)
+	movq  %r9,  R9+16-ARGOFFSET(%rsp)
+	movq %r10, R10+16-ARGOFFSET(%rsp)
+	movq_cfi r11, __stringify(R11+16-ARGOFFSET)
 
 	leaq -ARGOFFSET+16(%rsp),%rdi	/* arg1 for handler */
 	movq_cfi rbp, 8		/* push %rbp */
 	leaq 8(%rsp), %rbp		/* mov %rsp, %ebp */
+	CFI_DEF_CFA_REGISTER rbp
+	CFI_ADJUST_CFA_OFFSET -8
 	testl $3, CS(%rdi)
 	je 1f
 	SWAPGS
@@ -343,11 +349,10 @@ ENTRY(save_args)
 	 */
 1:	incl PER_CPU_VAR(irq_count)
 	jne 2f
-	popq_cfi %rax			/* move return address... */
+	popq %rax			/* move return address... */
 	mov PER_CPU_VAR(irq_stack_ptr),%rsp
-	EMPTY_FRAME 0
-	pushq_cfi %rbp			/* backlink for unwinder */
-	pushq_cfi %rax			/* ... to the new stack */
+	pushq %rbp			/* backlink for unwinder */
+	pushq %rax			/* ... to the new stack */
 	/*
 	 * We entered an interrupt context - irqs are off:
 	 */
@@ -357,14 +362,14 @@ ENTRY(save_args)
 END(save_args)
 
 ENTRY(save_rest)
-	PARTIAL_FRAME 1 REST_SKIP+8
+	CFI_STARTPROC
 	movq 5*8+16(%rsp), %r11	/* save return address */
-	movq_cfi rbx, RBX+16
-	movq_cfi rbp, RBP+16
-	movq_cfi r12, R12+16
-	movq_cfi r13, R13+16
-	movq_cfi r14, R14+16
-	movq_cfi r15, R15+16
+	movq %rbx, RBX+16(%rsp)
+	movq %rbp, RBP+16(%rsp)
+	movq %r12, R12+16(%rsp)
+	movq %r13, R13+16(%rsp)
+	movq %r14, R14+16(%rsp)
+	movq %r15, R15+16(%rsp)
 	movq %r11, 8(%rsp)	/* return address */
 	FIXUP_TOP_OF_STACK %r11, 16
 	ret
@@ -373,23 +378,23 @@ END(save_rest)
 
 /* save complete stack frame */
 ENTRY(save_paranoid)
-	XCPT_FRAME 1 RDI+8
+	XCPT_FRAME offset=__stringify(ORIG_RAX-R15+8)
 	cld
-	movq_cfi rdi, RDI+8
-	movq_cfi rsi, RSI+8
-	movq_cfi rdx, RDX+8
-	movq_cfi rcx, RCX+8
-	movq_cfi rax, RAX+8
-	movq_cfi r8, R8+8
-	movq_cfi r9, R9+8
-	movq_cfi r10, R10+8
-	movq_cfi r11, R11+8
-	movq_cfi rbx, RBX+8
-	movq_cfi rbp, RBP+8
-	movq_cfi r12, R12+8
-	movq_cfi r13, R13+8
-	movq_cfi r14, R14+8
-	movq_cfi r15, R15+8
+	movq %rdi, RDI+8(%rsp)
+	movq %rsi, RSI+8(%rsp)
+	movq_cfi rdx, __stringify(RDX+8)
+	movq_cfi rcx, __stringify(RCX+8)
+	movq_cfi rax, __stringify(RAX+8)
+	movq %r8, R8+8(%rsp)
+	movq %r9, R9+8(%rsp)
+	movq %r10, R10+8(%rsp)
+	movq %r11, R11+8(%rsp)
+	movq_cfi rbx, __stringify(RBX+8)
+	movq %rbp, RBP+8(%rsp)
+	movq %r12, R12+8(%rsp)
+	movq %r13, R13+8(%rsp)
+	movq %r14, R14+8(%rsp)
+	movq %r15, R15+8(%rsp)
 	movl $1,%ebx
 	movl $MSR_GS_BASE,%ecx
 	rdmsr
@@ -706,7 +711,7 @@ ENTRY(\label)
 	subq $REST_SKIP, %rsp
 	CFI_ADJUST_CFA_OFFSET REST_SKIP
 	call save_rest
-	DEFAULT_FRAME 0 8		/* offset 8: return address */
+	DEFAULT_FRAME -2 8		/* offset 8: return address */
 	leaq 8(%rsp), \arg	/* pt_regs pointer */
 	call \func
 	jmp ptregscall_common
@@ -723,12 +728,12 @@ END(\label)
 ENTRY(ptregscall_common)
 	DEFAULT_FRAME 1 8	/* offset 8: return address */
 	RESTORE_TOP_OF_STACK %r11, 8
-	movq_cfi_restore R15+8, r15
-	movq_cfi_restore R14+8, r14
-	movq_cfi_restore R13+8, r13
-	movq_cfi_restore R12+8, r12
-	movq_cfi_restore RBP+8, rbp
-	movq_cfi_restore RBX+8, rbx
+	movq_cfi_restore __stringify(R15+8), r15
+	movq_cfi_restore __stringify(R14+8), r14
+	movq_cfi_restore __stringify(R13+8), r13
+	movq_cfi_restore __stringify(R12+8), r12
+	movq_cfi_restore __stringify(RBP+8), rbp
+	movq_cfi_restore __stringify(RBX+8), rbx
 	ret $REST_SKIP		/* pop extended registers */
 	CFI_ENDPROC
 END(ptregscall_common)
@@ -817,10 +822,12 @@ END(interrupt)
 
 /* 0(%rsp): ~(interrupt number) */
 	.macro interrupt func
-	subq $10*8, %rsp
-	CFI_ADJUST_CFA_OFFSET 10*8
+	subq $ORIG_RAX-ARGOFFSET+8, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-ARGOFFSET+8
 	call save_args
-	PARTIAL_FRAME 0
+	PARTIAL_FRAME -1 8
+	CFI_REL_OFFSET rbp, 0
+	CFI_DEF_CFA_REGISTER rbp
 	call \func
 	.endm
 
@@ -1033,10 +1040,10 @@ ENTRY(\sym)
 	INTR_FRAME
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 	pushq_cfi $-1		/* ORIG_RAX: no syscall to restart */
-	subq $15*8,%rsp
-	CFI_ADJUST_CFA_OFFSET 15*8
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call error_entry
-	DEFAULT_FRAME 0
+	DEFAULT_FRAME -1
 	movq %rsp,%rdi		/* pt_regs pointer */
 	xorl %esi,%esi		/* no error code */
 	call \do_sym
@@ -1051,8 +1058,10 @@ ENTRY(\sym)
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 	pushq $-1		/* ORIG_RAX: no syscall to restart */
 	CFI_ADJUST_CFA_OFFSET 8
-	subq $15*8, %rsp
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
+	DEFAULT_FRAME -1
 	TRACE_IRQS_OFF
 	movq %rsp,%rdi		/* pt_regs pointer */
 	xorl %esi,%esi		/* no error code */
@@ -1068,8 +1077,10 @@ ENTRY(\sym)
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 	pushq $-1		/* ORIG_RAX: no syscall to restart */
 	CFI_ADJUST_CFA_OFFSET 8
-	subq $15*8, %rsp
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
+	DEFAULT_FRAME -1
 	TRACE_IRQS_OFF
 	movq %rsp,%rdi		/* pt_regs pointer */
 	xorl %esi,%esi		/* no error code */
@@ -1086,10 +1097,10 @@ END(\sym)
 ENTRY(\sym)
 	XCPT_FRAME
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
-	subq $15*8,%rsp
-	CFI_ADJUST_CFA_OFFSET 15*8
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call error_entry
-	DEFAULT_FRAME 0
+	DEFAULT_FRAME -1
 	movq %rsp,%rdi			/* pt_regs pointer */
 	movq ORIG_RAX(%rsp),%rsi	/* get error code */
 	movq $-1,ORIG_RAX(%rsp)		/* no syscall to restart */
@@ -1104,10 +1115,10 @@ END(\sym)
 ENTRY(\sym)
 	XCPT_FRAME
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
-	subq $15*8,%rsp
-	CFI_ADJUST_CFA_OFFSET 15*8
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
-	DEFAULT_FRAME 0
+	DEFAULT_FRAME -1
 	TRACE_IRQS_OFF
 	movq %rsp,%rdi			/* pt_regs pointer */
 	movq ORIG_RAX(%rsp),%rsi	/* get error code */
@@ -1400,7 +1411,7 @@ paranoidzeroentry machine_check do_machi
 
 	/* ebx:	no swapgs flag */
 ENTRY(paranoid_exit)
-	INTR_FRAME
+	DEFAULT_FRAME
 	DISABLE_INTERRUPTS(CLBR_NONE)
 	TRACE_IRQS_OFF
 	testl %ebx,%ebx				/* swapgs needed? */
@@ -1447,25 +1458,24 @@ END(paranoid_exit)
  * returns in "no swapgs flag" in %ebx.
  */
 ENTRY(error_entry)
-	XCPT_FRAME
-	CFI_ADJUST_CFA_OFFSET 15*8
+	XCPT_FRAME offset=__stringify(ORIG_RAX-R15+8)
 	/* oldrax contains error code */
 	cld
-	movq_cfi rdi, RDI+8
-	movq_cfi rsi, RSI+8
-	movq_cfi rdx, RDX+8
-	movq_cfi rcx, RCX+8
-	movq_cfi rax, RAX+8
-	movq_cfi  r8,  R8+8
-	movq_cfi  r9,  R9+8
-	movq_cfi r10, R10+8
-	movq_cfi r11, R11+8
-	movq_cfi rbx, RBX+8
-	movq_cfi rbp, RBP+8
-	movq_cfi r12, R12+8
-	movq_cfi r13, R13+8
-	movq_cfi r14, R14+8
-	movq_cfi r15, R15+8
+	movq %rdi, RDI+8(%rsp)
+	movq %rsi, RSI+8(%rsp)
+	movq %rdx, RDX+8(%rsp)
+	movq %rcx, RCX+8(%rsp)
+	movq %rax, RAX+8(%rsp)
+	movq  %r8,  R8+8(%rsp)
+	movq  %r9,  R9+8(%rsp)
+	movq %r10, R10+8(%rsp)
+	movq %r11, R11+8(%rsp)
+	movq_cfi rbx, __stringify(RBX+8)
+	movq %rbp, RBP+8(%rsp)
+	movq %r12, R12+8(%rsp)
+	movq %r13, R13+8(%rsp)
+	movq %r14, R14+8(%rsp)
+	movq %r15, R15+8(%rsp)
 	xorl %ebx,%ebx
 	testl $3,CS+8(%rsp)
 	je error_kernelspace
@@ -1474,7 +1484,6 @@ error_swapgs:
 error_sti:
 	TRACE_IRQS_OFF
 	ret
-	CFI_ENDPROC
 
 /*
  * There are two places in the kernel that can potentially fault with
@@ -1484,6 +1493,7 @@ error_sti:
  * compat mode. Check for these here too.
  */
 error_kernelspace:
+	CFI_REL_OFFSET rcx, RCX+8
 	incl %ebx
 	leaq irq_return(%rip),%rcx
 	cmpq %rcx,RIP+8(%rsp)
@@ -1494,6 +1504,7 @@ error_kernelspace:
 	cmpq $gs_change,RIP+8(%rsp)
 	je error_swapgs
 	jmp error_sti
+	CFI_ENDPROC
 END(error_entry)
 
 
@@ -1522,10 +1533,10 @@ ENTRY(nmi)
 	INTR_FRAME
 	PARAVIRT_ADJUST_EXCEPTION_FRAME
 	pushq_cfi $-1
-	subq $15*8, %rsp
-	CFI_ADJUST_CFA_OFFSET 15*8
+	subq $ORIG_RAX-R15, %rsp
+	CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
 	call save_paranoid
-	DEFAULT_FRAME 0
+	DEFAULT_FRAME -1
 	/* paranoidentry do_nmi, 0; without TRACE_IRQS_OFF */
 	movq %rsp,%rdi
 	movq $-1,%rsi

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] x86-64: fix unwind annotations in entry_64.S
  2009-03-12 10:32 [PATCH] x86-64: fix unwind annotations in entry_64.S Jan Beulich
@ 2009-03-12 10:48 ` Ingo Molnar
  2009-03-12 11:21   ` Jan Beulich
  0 siblings, 1 reply; 4+ messages in thread
From: Ingo Molnar @ 2009-03-12 10:48 UTC (permalink / raw)
  To: Jan Beulich, Cyrill Gorcunov, Alexander van Heukelum
  Cc: tglx, hpa, linux-kernel


* Jan Beulich <jbeulich@novell.com> wrote:

>  	.macro XCPT_FRAME start=1 offset=0
> -	INTR_FRAME \start, RIP+\offset-ORIG_RAX
> -	/*CFI_REL_OFFSET orig_rax, ORIG_RAX-ORIG_RAX*/
> +	INTR_FRAME \start, __stringify(RIP+\offset-ORIG_RAX)
>  	.endm
>  
>  /*
>   * frame that enables calling into C.
>   */
>  	.macro PARTIAL_FRAME start=1 offset=0
> -	XCPT_FRAME \start, ORIG_RAX+\offset-ARGOFFSET
> +	.if \start >= 0
> +	XCPT_FRAME \start, __stringify(ORIG_RAX+\offset-ARGOFFSET)
> +	.endif

So we pass in a stringified parameter from PARTIAL_FRAME to 
XCPT_FRAME and then we stringify it again?

> -	movq_cfi rdi, RDI+16-ARGOFFSET
> -	movq_cfi rsi, RSI+16-ARGOFFSET
> -	movq_cfi rdx, RDX+16-ARGOFFSET
> -	movq_cfi rcx, RCX+16-ARGOFFSET
> -	movq_cfi rax, RAX+16-ARGOFFSET
> -	movq_cfi  r8,  R8+16-ARGOFFSET
> -	movq_cfi  r9,  R9+16-ARGOFFSET
> -	movq_cfi r10, R10+16-ARGOFFSET
> -	movq_cfi r11, R11+16-ARGOFFSET
> +	movq %rdi, RDI+16-ARGOFFSET(%rsp)
> +	movq %rsi, RSI+16-ARGOFFSET(%rsp)
> +	movq %rdx, RDX+16-ARGOFFSET(%rsp)
> +	movq %rcx, RCX+16-ARGOFFSET(%rsp)
> +	movq_cfi rax, __stringify(RAX+16-ARGOFFSET)
> +	movq  %r8,  R8+16-ARGOFFSET(%rsp)
> +	movq  %r9,  R9+16-ARGOFFSET(%rsp)
> +	movq %r10, R10+16-ARGOFFSET(%rsp)
> +	movq_cfi r11, __stringify(R11+16-ARGOFFSET)

Hm, we used to have annotations for those arguments too, now 
they are gone.

>  	leaq 8(%rsp), %rbp		/* mov %rsp, %ebp */
> +	CFI_DEF_CFA_REGISTER rbp
> +	CFI_ADJUST_CFA_OFFSET -8

This open-codes CFI annotations again - please introduce a 
helper instead.

> -	popq_cfi %rax			/* move return address... */
> +	popq %rax			/* move return address... */

No annotation needed?

>  	mov %gs:pda_irqstackptr,%rsp
> -	EMPTY_FRAME 0
> -	pushq_cfi %rbp			/* backlink for unwinder */
> -	pushq_cfi %rax			/* ... to the new stack */
> +	pushq %rbp			/* backlink for unwinder */
> +	pushq %rax			/* ... to the new stack */

Ditto?

>  ENTRY(save_rest)
> -	PARTIAL_FRAME 1 REST_SKIP+8
> +	CFI_STARTPROC
>  	movq 5*8+16(%rsp), %r11	/* save return address */
> -	movq_cfi rbx, RBX+16
> -	movq_cfi rbp, RBP+16
> -	movq_cfi r12, R12+16
> -	movq_cfi r13, R13+16
> -	movq_cfi r14, R14+16
> -	movq_cfi r15, R15+16
> +	movq %rbx, RBX+16(%rsp)
> +	movq %rbp, RBP+16(%rsp)
> +	movq %r12, R12+16(%rsp)
> +	movq %r13, R13+16(%rsp)
> +	movq %r14, R14+16(%rsp)
> +	movq %r15, R15+16(%rsp)

Same here?

>  	CFI_ADJUST_CFA_OFFSET REST_SKIP
>  	call save_rest
> -	DEFAULT_FRAME 0 8		/* offset 8: return address */
> +	DEFAULT_FRAME -2 8		/* offset 8: return address */
>  	leaq 8(%rsp), \arg	/* pt_regs pointer */

Open-coded CFI annotation again.

I havent checked the rest of the patch but it shows similar 
patters.

The thing is, this patch is completely unacceptable - it just 
reintroduces the same ugly crufty CFI code we used to have 
there.

The current annotations might be completely broken but at least 
they _look_ structured and attempt to bring some order into the 
CFI annotations chaos.

There's really just two options here:

- You either submit clean, gradual patches that fix the problems 
  and explain what is done and why, and documents the annotation
  rules and makes CFI annotations maintainable,

- Or we'll remove all CFI annotations from all x86 assembly
  files and wait for future, clean patches.

	Ingo

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] x86-64: fix unwind annotations in entry_64.S
  2009-03-12 10:48 ` Ingo Molnar
@ 2009-03-12 11:21   ` Jan Beulich
  2009-03-12 11:55     ` Ingo Molnar
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2009-03-12 11:21 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Alexander van Heukelum, Cyrill Gorcunov, tglx, linux-kernel, hpa

>>> Ingo Molnar <mingo@elte.hu> 12.03.09 11:48 >>>
>* Jan Beulich <jbeulich@novell.com> wrote:
>>  	.macro XCPT_FRAME start=1 offset=0
>> -	INTR_FRAME \start, RIP+\offset-ORIG_RAX
>> -	/*CFI_REL_OFFSET orig_rax, ORIG_RAX-ORIG_RAX*/
>> +	INTR_FRAME \start, __stringify(RIP+\offset-ORIG_RAX)
>>  	.endm
>>  
>>  /*
>>   * frame that enables calling into C.
>>   */
>>  	.macro PARTIAL_FRAME start=1 offset=0
>> -	XCPT_FRAME \start, ORIG_RAX+\offset-ARGOFFSET
>> +	.if \start >= 0
>> +	XCPT_FRAME \start, __stringify(ORIG_RAX+\offset-ARGOFFSET)
>> +	.endif
>
>So we pass in a stringified parameter from PARTIAL_FRAME to 
>XCPT_FRAME and then we stringify it again?

Yes and no. You have to realize that __stringify() is a C preprocessor
operation, while the rest is assembler macro expansion (which strips the
quotes again, as they're just used to identify how much of the argument
string applies to the respective macro parameter. I've seen rather ugly
and difficult to diagnose problems with the assembler without doing this
(in the code before this patch they happened to work out of pure luck,
and dependent on undocumented gas internals).

>> -	movq_cfi rdi, RDI+16-ARGOFFSET
>> -	movq_cfi rsi, RSI+16-ARGOFFSET
>> -	movq_cfi rdx, RDX+16-ARGOFFSET
>> -	movq_cfi rcx, RCX+16-ARGOFFSET
>> -	movq_cfi rax, RAX+16-ARGOFFSET
>> -	movq_cfi  r8,  R8+16-ARGOFFSET
>> -	movq_cfi  r9,  R9+16-ARGOFFSET
>> -	movq_cfi r10, R10+16-ARGOFFSET
>> -	movq_cfi r11, R11+16-ARGOFFSET
>> +	movq %rdi, RDI+16-ARGOFFSET(%rsp)
>> +	movq %rsi, RSI+16-ARGOFFSET(%rsp)
>> +	movq %rdx, RDX+16-ARGOFFSET(%rsp)
>> +	movq %rcx, RCX+16-ARGOFFSET(%rsp)
>> +	movq_cfi rax, __stringify(RAX+16-ARGOFFSET)
>> +	movq  %r8,  R8+16-ARGOFFSET(%rsp)
>> +	movq  %r9,  R9+16-ARGOFFSET(%rsp)
>> +	movq %r10, R10+16-ARGOFFSET(%rsp)
>> +	movq_cfi r11, __stringify(R11+16-ARGOFFSET)
>
>Hm, we used to have annotations for those arguments too, now 
>they are gone.

It is pointless to annotate those, as the function doesn't modify them.

>>  	leaq 8(%rsp), %rbp		/* mov %rsp, %ebp */
>> +	CFI_DEF_CFA_REGISTER rbp
>> +	CFI_ADJUST_CFA_OFFSET -8
>
>This open-codes CFI annotations again - please introduce a 
>helper instead.

That seems pretty pointless, as what is being done here is very unlikely to
be done anywhere else again.

>> -	popq_cfi %rax			/* move return address... */
>> +	popq %rax			/* move return address... */
>
>No annotation needed?

No. The annotation was actually wrong (because rsp is no longer the frame
pointer here). This is one of the aspects of unwind annotations which make
me think that trying to abstract out too much is actually hurting.

>>  	mov %gs:pda_irqstackptr,%rsp
>> -	EMPTY_FRAME 0
>> -	pushq_cfi %rbp			/* backlink for unwinder */
>> -	pushq_cfi %rax			/* ... to the new stack */
>> +	pushq %rbp			/* backlink for unwinder */
>> +	pushq %rax			/* ... to the new stack */
>
>Ditto?

Exactly.

>>  ENTRY(save_rest)
>> -	PARTIAL_FRAME 1 REST_SKIP+8
>> +	CFI_STARTPROC
>>  	movq 5*8+16(%rsp), %r11	/* save return address */
>> -	movq_cfi rbx, RBX+16
>> -	movq_cfi rbp, RBP+16
>> -	movq_cfi r12, R12+16
>> -	movq_cfi r13, R13+16
>> -	movq_cfi r14, R14+16
>> -	movq_cfi r15, R15+16
>> +	movq %rbx, RBX+16(%rsp)
>> +	movq %rbp, RBP+16(%rsp)
>> +	movq %r12, R12+16(%rsp)
>> +	movq %r13, R13+16(%rsp)
>> +	movq %r14, R14+16(%rsp)
>> +	movq %r15, R15+16(%rsp)
>
>Same here?

No. Here it is again that the function doesn't modify these registers, and
hence there's no reason to annotate the spills.

>>  	CFI_ADJUST_CFA_OFFSET REST_SKIP
>>  	call save_rest
>> -	DEFAULT_FRAME 0 8		/* offset 8: return address */
>> +	DEFAULT_FRAME -2 8		/* offset 8: return address */
>>  	leaq 8(%rsp), \arg	/* pt_regs pointer */
>
>Open-coded CFI annotation again.

Where is there anything open-coded here. It just changes the first macro
argument.

>I havent checked the rest of the patch but it shows similar 
>patters.
>
>The thing is, this patch is completely unacceptable - it just 
>reintroduces the same ugly crufty CFI code we used to have 
>there.

Hiding things behind macros where it makes sense is fine, but there are
limits to this imo. Not the least because this macro-ization prevents actual
consolidation (in many places, rather than annotating individual instructions,
the annotations as written out to the object file could actually be
compacted by placing the annotations after e.g. a group of registers had
been spilled - reducing size and parsing overhead from whoever reads the
.eh_frame section).

>The current annotations might be completely broken but at least 
>they _look_ structured and attempt to bring some order into the 
>CFI annotations chaos.

There was no chaos. Everything was working fine (and was understandable
to someone familiar with CFI annotations). The chaos got introduced with
the improperly annotated code. To me (and to our kernel, which makes
actual use of the annotations) this is a clear regression that shouldn't even
haven been allowed in. I'm just trying to pick up the pieces others put on
the ground.

>There's really just two options here:
>
>- You either submit clean, gradual patches that fix the problems 
>  and explain what is done and why, and documents the annotation
>  rules and makes CFI annotations maintainable,

I think the patch is as clean as it can be. It's debatable whether splitting
it up would be possible - perhaps very few hunks could be broken out
and submitted individually, but the bulk of it has to stay together as it's
dependent stuff.

>- Or we'll remove all CFI annotations from all x86 assembly
>  files and wait for future, clean patches.

Oh yes, thanks for offering this as an option. Wasn't it you who always
wanted reliable stack traces? This would get us even further away from
that goal. Besides being forced to maintain the unwinder out-of-tree,
we'd then also need to maintain the annotations in this manner. This very
much reminds me of Linus' attitude of considering Open Source more
open to some people than to others.

Jan

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] x86-64: fix unwind annotations in entry_64.S
  2009-03-12 11:21   ` Jan Beulich
@ 2009-03-12 11:55     ` Ingo Molnar
  0 siblings, 0 replies; 4+ messages in thread
From: Ingo Molnar @ 2009-03-12 11:55 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Alexander van Heukelum, Cyrill Gorcunov, tglx, linux-kernel, hpa,
	Linus Torvalds


* Jan Beulich <jbeulich@novell.com> wrote:

> > The current annotations might be completely broken but at 
> > least they _look_ structured and attempt to bring some order 
> > into the CFI annotations chaos.
> 
> There was no chaos. Everything was working fine (and was 
> understandable to someone familiar with CFI annotations). 
> [...]

Well, in the patch you remove a ton of incorrect annotations, 
which all came from the old code. So the old code was all but 
fine - it was ugly and kept breaking all the time.

The thing is, we were this -->.<-- close to removing _all_ CFI 
annotations from x86 assembly files about a year ago. I actually 
wrote all the patches for that and committed them back then.

The reason: they made the assembly code even harder to read and 
distracted from the primary, functional purpose of the code - 
which is functionality. Writing assembly code is hard enough, we 
dont need hundreds of additional CFI annotations to obscure them 
as well.

So when you add back CFI annotations this should be your main 
driving principle: make them as unintrusive as possible. Merge 
them to existing frame layout macros wherever possible. Dont add 
raw CFI annotations. You dont seem to understand and respect any 
of these principles.

You seem to regard them as on par with code but they are not. 
They are at most an afterthought and we will not tolerate them 
if they look ugly - and heck does your patch look ugly.

So we want to hide the CFI details as much as possible 
technically, and make it all as self-maintaining as possible.

Document each and every type of annotation and perhaps also 
write up a small blurb in one of the include files about the 
rules and practices needed for good CFI annotations. That way 
you teach others how to keep it all working fine. Perhaps think 
about writing a KGDB drivn self-test that finds CFI annotation 
bugs - _anything_ that brings this code out of obscurity.

I dont mind quirky features that external entities like your 
dwarf2 unwinder rely on - as long as they benefit the code and 
as long as you participate in the process and as long a you are 
willing to structure it all in a clean way. Coming out of the 
corporate woodwork every 6 months and complaining that "it is 
broken currently" and then going back into hiding without 
changing the dynamics of the code is not enough.

	Ingo

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2009-03-12 11:56 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-12 10:32 [PATCH] x86-64: fix unwind annotations in entry_64.S Jan Beulich
2009-03-12 10:48 ` Ingo Molnar
2009-03-12 11:21   ` Jan Beulich
2009-03-12 11:55     ` Ingo Molnar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.