x86-64: fix unwind info for incomplete frames
Subsystems (3)
PATCH 1/1 x86-64: fix unwind info for incomplete frames
Commit 76f5df43ca ('x86/asm/entry/64: Always allocate a complete "struct pt_regs" on the kernel stack') deleted PARTIAL_FRAME without considering that while a full frame is now being allocated, not all registers get always saved into it. Instead of restoring that macro, simply make DEFAULT_FRAME capable of expressing both.
--- arch/x86/kernel/entry_64.S | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
arch/x86/kernel/entry_64.S +10 -8
@@ -148,7 +148,7 @@ ENDPROC(native_usergs_sysret64) /* * frame that enables passing a complete pt_regs to a C function. */ - .macro DEFAULT_FRAME start=1 offset=0 + .macro DEFAULT_FRAME start=1 offset=0 extra=1 XCPT_FRAME \start, ORIG_RAX+\offset CFI_REL_OFFSET rdi, RDI+\offset CFI_REL_OFFSET rsi, RSI+\offset
@@ -159,12 +159,14 @@ ENDPROC(native_usergs_sysret64) CFI_REL_OFFSET r9, R9+\offset CFI_REL_OFFSET r10, R10+\offset CFI_REL_OFFSET r11, R11+\offset + .if \extra CFI_REL_OFFSET rbx, RBX+\offset CFI_REL_OFFSET rbp, RBP+\offset CFI_REL_OFFSET r12, R12+\offset CFI_REL_OFFSET r13, R13+\offset CFI_REL_OFFSET r14, R14+\offset CFI_REL_OFFSET r15, R15+\offset + .endif .endm /*
@@ -491,7 +493,7 @@ END(system_call) .macro FORK_LIKE func ENTRY(stub_\func) CFI_STARTPROC - DEFAULT_FRAME 0, 8 /* offset 8: return address */ + DEFAULT_FRAME 0, 8, 0 /* offset 8: return address */ SAVE_EXTRA_REGS 8 jmp sys_\func CFI_ENDPROC
@@ -504,7 +506,7 @@ END(stub_\func) ENTRY(stub_execve) CFI_STARTPROC - DEFAULT_FRAME 0, 8 + DEFAULT_FRAME 0, 8, 0 call sys_execve return_from_execve: testl %eax, %eax
@@ -527,7 +529,7 @@ END(stub_execve) .align 8 GLOBAL(stub_execveat) CFI_STARTPROC - DEFAULT_FRAME 0, 8 + DEFAULT_FRAME 0, 8, 0 call sys_execveat jmp return_from_execve CFI_ENDPROC
@@ -537,7 +539,7 @@ END(stub_execveat) .align 8 GLOBAL(stub_x32_execve) CFI_STARTPROC - DEFAULT_FRAME 0, 8 + DEFAULT_FRAME 0, 8, 0 call compat_sys_execve jmp return_from_execve CFI_ENDPROC
@@ -545,7 +547,7 @@ END(stub_x32_execve) .align 8 GLOBAL(stub_x32_execveat) CFI_STARTPROC - DEFAULT_FRAME 0, 8 + DEFAULT_FRAME 0, 8, 0 call compat_sys_execveat jmp return_from_execve CFI_ENDPROC
@@ -575,7 +577,7 @@ END(stub32_execveat) */ ENTRY(stub_rt_sigreturn) CFI_STARTPROC - DEFAULT_FRAME 0, 8 + DEFAULT_FRAME 0, 8, 0 /* * SAVE_EXTRA_REGS result is not normally needed: * sigreturn overwrites all pt_regs->GPREGS.
@@ -597,7 +599,7 @@ END(stub_rt_sigreturn) #ifdef CONFIG_X86_X32_ABI ENTRY(stub_x32_rt_sigreturn) CFI_STARTPROC - DEFAULT_FRAME 0, 8 + DEFAULT_FRAME 0, 8, 0 SAVE_EXTRA_REGS 8 call sys32_x32_rt_sigreturn jmp return_from_stub
Quoted 27 lines · Jan Beulich
Commit
76f5df43ca('x86/asm/entry/64: Always allocate a complete "structpt_regs" on the kernel stack') deletedPARTIAL_FRAMEwithout considering that while a full frame is now being allocated, not all registers get always saved into it. Instead of restoring that macro, simply makeDEFAULT_FRAMEcapable of expressing both.Signed-off-by Jan Beulich <[email protected]>Cc Denys Vlasenko <[email protected]>Cc Andy Lutomirski <[email protected]>--- arch/x86/kernel/entry_64.S | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)arch/x86/kernel/entry_64.S +1
-1@@ -148,7 +148,7 @@ ENDPROC(native_usergs_sysret64) /* * frame that enables passing a complete pt_regs to a C function. */ - .macro DEFAULT_FRAME start=1 offset=0 + .macro DEFAULT_FRAME start=1 offset=0 extra=1 XCPT_FRAME \start, ORIG_RAX+\offset CFI_REL_OFFSET rdi, RDI+\offset CFI_REL_OFFSET rsi, RSI+\offset@@ -159,12 +159,14 @@ ENDPROC(native_usergs_sysret64) CFI_REL_OFFSET r9, R9+\offset CFI_REL_OFFSET r10, R10+\offset
arch/x86/kernel/entry_64.S ENDPROC(native_usergs_sysret64) · line 170
Quoted 10 lines
arch/x86/kernel/entry_64.S +2
-0CFI_REL_OFFSET r11, R11+\offset + .if \extra CFI_REL_OFFSET rbx, RBX+\offset CFI_REL_OFFSET rbp, RBP+\offset CFI_REL_OFFSET r12, R12+\offset CFI_REL_OFFSET r13, R13+\offset CFI_REL_OFFSET r14, R14+\offset CFI_REL_OFFSET r15, R15+\offset + .endif .endm
I have a couple of code cleanliness complaints:
- So 'extra' isn't very expressive, I'd name it 'full' to signal a full frame, and full=0 denotes
- So I had to go into the source and double check various nested macros to see that DEFAULT_FRAME is only defining debug information, it's not emitting any actual code. This should have been glaringly obvious from the macro name!
- So I hate these 'default values' vararg-ish assembly macros:
arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 /* offset 8: return address */ arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0 arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0
because unlike C functions they make the actual arguments a guessing game:
you always have to double check the macro definition itself - while the 'savings' in terms of less code written are miniscule. So it actually obscures macros.
So these should be flattened, with clear, fixed length parameter signatures, to make them as similar to regular C code as syntactically possible.
Thanks,
Ingo
Quoted 29 lines
Quoted 1 line
Quoted 1 line · On 28.05.15 at 11:01, <[email protected]> wrote:
Quoted 14 lines · Jan Beulich
arch/x86/kernel/entry_64.S +1
-1@@ -148,7 +148,7 @@ ENDPROC(native_usergs_sysret64) /* * frame that enables passing a complete pt_regs to a C function. */ - .macro DEFAULT_FRAME start=1 offset=0 + .macro DEFAULT_FRAME start=1 offset=0 extra=1 XCPT_FRAME \start, ORIG_RAX+\offset CFI_REL_OFFSET rdi, RDI+\offset CFI_REL_OFFSET rsi, RSI+\offset@@ -159,12 +159,14 @@ ENDPROC(native_usergs_sysret64) CFI_REL_OFFSET r9, R9+\offset CFI_REL_OFFSET r10, R10+\offsetarch/x86/kernel/entry_64.S ENDPROC(native_usergs_sysret64) · line 170
Quoted 10 lines
arch/x86/kernel/entry_64.S +2
-0CFI_REL_OFFSET r11, R11+\offset + .if \extra CFI_REL_OFFSET rbx, RBX+\offset CFI_REL_OFFSET rbp, RBP+\offset CFI_REL_OFFSET r12, R12+\offset CFI_REL_OFFSET r13, R13+\offset CFI_REL_OFFSET r14, R14+\offset CFI_REL_OFFSET r15, R15+\offset + .endif .endmI have a couple of code cleanliness complaints:
Quoted 2 lines
- So 'extra' isn't very expressive, I'd name it 'full' to signal a full frame, and full=0 denotes
I can certainly do this; as easy as "sed s/extra/full/g" on the patch - perhaps you could even do this while committing?
Quoted 3 lines
- So I had to go into the source and double check various nested macros to see that
DEFAULT_FRAMEis only defining debug information, it's not emitting any actual code. This should have been glaringly obvious from the macro name!
CFI_DEFAULT_FRAME? Anyway - clearly not in this patch.
Quoted 17 lines
- So I hate these 'default values' vararg-ish assembly macros:
arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 /* offset 8: return address */ arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0 arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0
Quoted 7 lines
because unlike C functions they make the actual arguments a guessing game:
you always have to double check the macro definition itself - while the 'savings' in terms of less code written are miniscule. So it actually obscures macros.So these should be flattened, with clear, fixed length parameter signatures, to make them as similar to regular C code as syntactically possible.
Not sure why assembly code should look like C code. It's a matter of taste perhaps, and I can see your point, but I'm also not really eager to do changes just to match other people's taste. And just like above - certainly not something for this patch I would think.
Jan
PATCH 1/1 x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 22 lines · Jan Beulich
Quoted 22 lines
- So I hate these 'default values' vararg-ish assembly macros:
arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 /* offset 8: return address */ arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0, 8 arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0 arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0 arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME arch/x86/kernel/entry_64.S: DEFAULT_FRAME 0because unlike C functions they make the actual arguments a guessing game:
you always have to double check the macro definition itself - while the 'savings' in terms of less code written are miniscule. So it actually obscures macros.
Quoted 7 lines
Quoted 2 lines
So these should be flattened, with clear, fixed length parameter signatures, to make them as similar to regular C code as syntactically possible.
Not sure why assembly code should look like C code. It's a matter of taste perhaps, and I can see your point, but I'm also not really eager to do changes just to match other people's taste. And just like above - certainly not something for this patch I would think.
Yeah, no, so this isn't going to work that way.
On one hand you want dwarf annotations mostly for the out of tree dwarf-unwinding stack backtraces patch on SUSE kernels, while for the upstream kernel it's mostly just unreadable gunk in some of the most security sensitive code paths of the kernel, which only gets in the way of readability.
But on the other hand you are unwilling to (or don't have the time to) do a proper job of making this palatable for upstream.
That's unacceptable from the upstream kernel's POV, so instead of limping forward I'll do the attached patch: it gets rid of the unmaintainable dwarf mess from low level x86 assembly code. This isn't a new concern, a couple of years ago we almost did this.
Someone who has the willingness and time to do this properly can reintroduce dwarf debuginfo in x86 assembly code from first principles, with the following conditions:
- keep CFI annotations out of the primary .S files. Having them in .h files where we define helper macros to set up symbols is probably fine, but not in .S files. Assembly code is hard to read already, without any annotations.
- find a build time method to insert dwarf annotations automatically in the most common cases, for example for pop/push instructions that manipulate the stack pointer. This could be done for example via a preprocessing step that just looks for common patterns. We have hundreds of CFI annotations, so automating most of that makes sense.
- the new dwarf code should come with build tooling checks that ensure that CFI annotations are sensible and that we haven't missed any symbol. We've seen such build time sanity checking efforts from the framepointer side, and there's no reason it couldn't be done on the dwarf side.
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Thanks,
Ingo
=============================>
From: Ingo Molnar <[email protected]>
Date: Thu, 28 May 2015 12:21:47 +0200
Subject: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
So the dwarf2 annotations in low level assembly code have become an increasing hindrance: unreadable, messy macros mixed into some of the most security sensitive code paths of the Linux kernel.
These debug info annotations don't even buy the upstream kernel anything: dwarf driven stack unwinding has caused problems in the past so it's out of tree, and the upstream kernel only uses the much more robust framepointers based stack unwinding method.
In addition to that there's a steady, slow bitrot going on with these annotations, requiring frequent fixups. There's no tooling and no functionality upstream that keeps it correct.
So burn down the sick forest, allowing new, healthier growth:
27 files changed, 350 insertions(+), 1101 deletions(-)
Someone who has the willingness and time to do this properly can attempt to reintroduce dwarf debuginfo in x86 assembly code plus dwarf unwinding from first principles, with the following conditions:
- keep CFI annotations out of the primary .S files. Having them in .h files where we define helper macros to set up symbols is probably fine, but not in .S files. Assembly code is hard to read already, without any annotations.
- find a build time method to insert dwarf annotations automatically in the most common cases, for example for pop/push instructions that manipulate the stack pointer. This could be done for example via a preprocessing step that just looks for common patterns. We have hundreds of CFI annotations, so automating most of that makes sense.
- the new dwarf code should come with build tooling checks that ensure that CFI annotations are sensible and that we haven't missed any symbol. We've seen such build time sanity checking efforts from the framepointer side, and there's no reason it couldn't be done on the dwarf side.
--- arch/x86/Makefile | 10 +- arch/x86/ia32/ia32entry.S | 133 ++++----------- arch/x86/include/asm/calling.h | 94 +++++------ arch/x86/include/asm/dwarf2.h | 170 ------------------- arch/x86/include/asm/frame.h | 7 +- arch/x86/kernel/entry_32.S | 368 ++++++++++++----------------------------- arch/x86/kernel/entry_64.S | 288 ++++++-------------------------- arch/x86/lib/atomic64_386_32.S | 7 +- arch/x86/lib/atomic64_cx8_32.S | 61 +++---- arch/x86/lib/checksum_32.S | 52 +++--- arch/x86/lib/clear_page_64.S | 7 - arch/x86/lib/cmpxchg16b_emu.S | 12 +- arch/x86/lib/cmpxchg8b_emu.S | 11 +- arch/x86/lib/copy_page_64.S | 11 -- arch/x86/lib/copy_user_64.S | 15 -- arch/x86/lib/csum-copy_64.S | 17 -- arch/x86/lib/getuser.S | 13 -- arch/x86/lib/iomap_copy_64.S | 3 - arch/x86/lib/memcpy_64.S | 3 - arch/x86/lib/memmove_64.S | 3 - arch/x86/lib/memset_64.S | 5 - arch/x86/lib/msr-reg.S | 44 ++--- arch/x86/lib/putuser.S | 8 +- arch/x86/lib/rwsem.S | 49 +++--- arch/x86/lib/thunk_32.S | 15 +- arch/x86/lib/thunk_64.S | 44 +++-- arch/x86/net/bpf_jit.S | 1 - 27 files changed, 350 insertions(+), 1101 deletions(-)
arch/x86/Makefile +2 -8
@@ -149,12 +149,6 @@ endif sp-$(CONFIG_X86_32) := esp sp-$(CONFIG_X86_64) := rsp -# do binutils support CFI? -cfi := $(call as-instr,.cfi_startproc\n.cfi_rel_offset $(sp-y)$(comma)0\n.cfi_endproc,-DCONFIG_AS_CFI=1) -# is .cfi_signal_frame supported too? -cfi-sigframe := $(call as-instr,.cfi_startproc\n.cfi_signal_frame\n.cfi_endproc,-DCONFIG_AS_CFI_SIGNAL_FRAME=1) -cfi-sections := $(call as-instr,.cfi_sections .debug_frame,-DCONFIG_AS_CFI_SECTIONS=1) - # does binutils support specific instructions? asinstr := $(call as-instr,fxsaveq (%rax),-DCONFIG_AS_FXSAVEQ=1) asinstr += $(call as-instr,pshufb %xmm0$(comma)%xmm0,-DCONFIG_AS_SSSE3=1)
@@ -162,8 +156,8 @@ asinstr += $(call as-instr,crc32l %eax$(comma)%eax,-DCONFIG_AS_CRC32=1) avx_instr := $(call as-instr,vxorps %ymm0$(comma)%ymm1$(comma)%ymm2,-DCONFIG_AS_AVX=1) avx2_instr :=$(call as-instr,vpbroadcastb %xmm0$(comma)%ymm1,-DCONFIG_AS_AVX2=1) -KBUILD_AFLAGS += $(cfi) $(cfi-sigframe) $(cfi-sections) $(asinstr) $(avx_instr) $(avx2_instr) -KBUILD_CFLAGS += $(cfi) $(cfi-sigframe) $(cfi-sections) $(asinstr) $(avx_instr) $(avx2_instr) +KBUILD_AFLAGS += $(asinstr) $(avx_instr) $(avx2_instr) +KBUILD_CFLAGS += $(asinstr) $(avx_instr) $(avx2_instr) LDFLAGS := -m elf_$(UTS_MACHINE)
arch/x86/ia32/ia32entry.S +32 -101
283 lines not on this page · raw message
arch/x86/include/asm/calling.h +45 -49
158 lines not on this page · raw message
arch/x86/include/asm/dwarf2.h deleted +0 -170
170 lines not on this page · raw message
arch/x86/include/asm/frame.h +2 -5
@@ -1,20 +1,17 @@#ifdef __ASSEMBLY__#include <asm/asm.h> -#include <asm/dwarf2.h> /* The annotation hides the frame from the unwinder and makes it look like a ordinary ebp save/restore. This avoids some special cases for frame pointer later */ #ifdef CONFIG_FRAME_POINTER .macro FRAME - __ASM_SIZE(push,_cfi) %__ASM_REG(bp) - CFI_REL_OFFSET __ASM_REG(bp), 0 + __ASM_SIZE(push,) %__ASM_REG(bp) __ASM_SIZE(mov) %__ASM_REG(sp), %__ASM_REG(bp) .endm .macro ENDFRAME - __ASM_SIZE(pop,_cfi) %__ASM_REG(bp) - CFI_RESTORE __ASM_REG(bp) + __ASM_SIZE(pop,) %__ASM_REG(bp) .endm #else .macro FRAME
arch/x86/kernel/entry_32.S +108 -260
805 lines not on this page · raw message
arch/x86/kernel/entry_64.S +48 -240
778 lines not on this page · raw message
arch/x86/lib/atomic64_386_32.S +2 -5
@@ -11,26 +11,23 @@ #include <linux/linkage.h> #include <asm/alternative-asm.h> -#include <asm/dwarf2.h> /* if you want SMP support, implement these with real spinlocks */ .macro LOCK reg - pushfl_cfi + pushfl cli .endm .macro UNLOCK reg - popfl_cfi + popfl .endm #define BEGIN(op) \ .macro endp; \ - CFI_ENDPROC; \ ENDPROC(atomic64_##op##_386); \ .purgem endp; \ .endm; \ ENTRY(atomic64_##op##_386); \ - CFI_STARTPROC; \ LOCK v; #define ENDP endp
arch/x86/lib/atomic64_cx8_32.S +20 -41
154 lines not on this page · raw message
arch/x86/lib/checksum_32.S +21 -31
111 lines not on this page · raw message
arch/x86/lib/clear_page_64.S +0 -7
@@ -1,5 +1,4 @@ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/cpufeature.h> #include <asm/alternative-asm.h>
@@ -15,7 +14,6 @@ * %rdi - page */ ENTRY(clear_page) - CFI_STARTPROC ALTERNATIVE_2 "jmp clear_page_orig", "", X86_FEATURE_REP_GOOD, \ "jmp clear_page_c_e", X86_FEATURE_ERMS
@@ -24,11 +22,9 @@ ENTRY(clear_page) xorl %eax,%eax rep stosq ret - CFI_ENDPROC ENDPROC(clear_page) ENTRY(clear_page_orig) - CFI_STARTPROC xorl %eax,%eax movl $4096/64,%ecx
@@ -48,14 +44,11 @@ ENTRY(clear_page_orig) jnz .Lloop nop ret - CFI_ENDPROC ENDPROC(clear_page_orig) ENTRY(clear_page_c_e) - CFI_STARTPROC movl $4096,%ecx xorl %eax,%eax rep stosb ret - CFI_ENDPROC ENDPROC(clear_page_c_e)
arch/x86/lib/cmpxchg16b_emu.S +3 -9
@@ -6,7 +6,6 @@ * */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/percpu.h> .text
@@ -21,7 +20,6 @@ * %al : Operation successful */ ENTRY(this_cpu_cmpxchg16b_emu) -CFI_STARTPROC # # Emulate 'cmpxchg16b %gs:(%rsi)' except we return the result in %al not
@@ -32,7 +30,7 @@ CFI_STARTPROC # *atomic* on a single cpu (as provided by the this_cpu_xx class of # macros). # - pushfq_cfi + pushfq cli cmpq PER_CPU_VAR((%rsi)), %rax
@@ -43,17 +41,13 @@ CFI_STARTPROC movq %rbx, PER_CPU_VAR((%rsi)) movq %rcx, PER_CPU_VAR(8(%rsi)) - CFI_REMEMBER_STATE - popfq_cfi + popfq mov $1, %al ret - CFI_RESTORE_STATE .Lnot_same: - popfq_cfi + popfq xor %al,%al ret -CFI_ENDPROC - ENDPROC(this_cpu_cmpxchg16b_emu)
arch/x86/lib/cmpxchg8b_emu.S +3 -8
@@ -7,7 +7,6 @@ */ #include <linux/linkage.h> -#include <asm/dwarf2.h> .text
@@ -20,14 +19,13 @@ * %ecx : high 32 bits of new value */ ENTRY(cmpxchg8b_emu) -CFI_STARTPROC # # Emulate 'cmpxchg8b (%esi)' on UP except we don't # set the whole ZF thing (caller will just compare # eax:edx with the expected value) # - pushfl_cfi + pushfl cli cmpl (%esi), %eax
@@ -38,18 +36,15 @@ CFI_STARTPROC movl %ebx, (%esi) movl %ecx, 4(%esi) - CFI_REMEMBER_STATE - popfl_cfi + popfl ret - CFI_RESTORE_STATE .Lnot_same: movl (%esi), %eax .Lhalf_same: movl 4(%esi), %edx - popfl_cfi + popfl ret -CFI_ENDPROC ENDPROC(cmpxchg8b_emu)
arch/x86/lib/copy_page_64.S +0 -11
@@ -1,7 +1,6 @@ /* Written 2003 by Andi Kleen, based on a kernel by Evandro Menezes */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/cpufeature.h> #include <asm/alternative-asm.h>
@@ -13,22 +12,16 @@ */ ALIGN ENTRY(copy_page) - CFI_STARTPROC ALTERNATIVE "jmp copy_page_regs", "", X86_FEATURE_REP_GOOD movl $4096/8, %ecx rep movsq ret - CFI_ENDPROC ENDPROC(copy_page) ENTRY(copy_page_regs) - CFI_STARTPROC subq $2*8, %rsp - CFI_ADJUST_CFA_OFFSET 2*8 movq %rbx, (%rsp) - CFI_REL_OFFSET rbx, 0 movq %r12, 1*8(%rsp) - CFI_REL_OFFSET r12, 1*8 movl $(4096/64)-5, %ecx .p2align 4
@@ -87,11 +80,7 @@ ENTRY(copy_page_regs) jnz .Loop2 movq (%rsp), %rbx - CFI_RESTORE rbx movq 1*8(%rsp), %r12 - CFI_RESTORE r12 addq $2*8, %rsp - CFI_ADJUST_CFA_OFFSET -2*8 ret - CFI_ENDPROC ENDPROC(copy_page_regs)
arch/x86/lib/copy_user_64.S +0 -15
@@ -7,7 +7,6 @@ */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/current.h> #include <asm/asm-offsets.h> #include <asm/thread_info.h>
@@ -18,7 +17,6 @@ /* Standard copy_to_user with segment limit checking */ ENTRY(_copy_to_user) - CFI_STARTPROC GET_THREAD_INFO(%rax) movq %rdi,%rcx addq %rdx,%rcx
@@ -30,12 +28,10 @@ ENTRY(_copy_to_user) X86_FEATURE_REP_GOOD, \ "jmp copy_user_enhanced_fast_string", \ X86_FEATURE_ERMS - CFI_ENDPROC ENDPROC(_copy_to_user) /* Standard copy_from_user with segment limit checking */ ENTRY(_copy_from_user) - CFI_STARTPROC GET_THREAD_INFO(%rax) movq %rsi,%rcx addq %rdx,%rcx
@@ -47,14 +43,12 @@ ENTRY(_copy_from_user) X86_FEATURE_REP_GOOD, \ "jmp copy_user_enhanced_fast_string", \ X86_FEATURE_ERMS - CFI_ENDPROC ENDPROC(_copy_from_user) .section .fixup,"ax" /* must zero dest */ ENTRY(bad_from_user) bad_from_user: - CFI_STARTPROC movl %edx,%ecx xorl %eax,%eax rep
@@ -62,7 +56,6 @@ ENTRY(bad_from_user) bad_to_user: movl %edx,%eax ret - CFI_ENDPROC ENDPROC(bad_from_user) .previous
@@ -80,7 +73,6 @@ ENDPROC(bad_from_user) * eax uncopied bytes or 0 if successful. */ ENTRY(copy_user_generic_unrolled) - CFI_STARTPROC ASM_STAC cmpl $8,%edx jb 20f /* less then 8 bytes, go to byte copy loop */
@@ -162,7 +154,6 @@ ENTRY(copy_user_generic_unrolled) _ASM_EXTABLE(19b,40b) _ASM_EXTABLE(21b,50b) _ASM_EXTABLE(22b,50b) - CFI_ENDPROC ENDPROC(copy_user_generic_unrolled) /* Some CPUs run faster using the string copy instructions.
@@ -184,7 +175,6 @@ ENDPROC(copy_user_generic_unrolled) * eax uncopied bytes or 0 if successful. */ ENTRY(copy_user_generic_string) - CFI_STARTPROC ASM_STAC cmpl $8,%edx jb 2f /* less than 8 bytes, go to byte copy loop */
@@ -209,7 +199,6 @@ ENTRY(copy_user_generic_string) _ASM_EXTABLE(1b,11b) _ASM_EXTABLE(3b,12b) - CFI_ENDPROC ENDPROC(copy_user_generic_string) /*
@@ -225,7 +214,6 @@ ENDPROC(copy_user_generic_string) * eax uncopied bytes or 0 if successful. */ ENTRY(copy_user_enhanced_fast_string) - CFI_STARTPROC ASM_STAC movl %edx,%ecx 1: rep
@@ -240,7 +228,6 @@ ENTRY(copy_user_enhanced_fast_string) .previous _ASM_EXTABLE(1b,12b) - CFI_ENDPROC ENDPROC(copy_user_enhanced_fast_string) /*
@@ -248,7 +235,6 @@ ENDPROC(copy_user_enhanced_fast_string) * This will force destination/source out of cache for more performance. */ ENTRY(__copy_user_nocache) - CFI_STARTPROC ASM_STAC cmpl $8,%edx jb 20f /* less then 8 bytes, go to byte copy loop */
@@ -332,5 +318,4 @@ ENTRY(__copy_user_nocache) _ASM_EXTABLE(19b,40b) _ASM_EXTABLE(21b,50b) _ASM_EXTABLE(22b,50b) - CFI_ENDPROC ENDPROC(__copy_user_nocache)
arch/x86/lib/csum-copy_64.S +0 -17
@@ -6,7 +6,6 @@ * for more details. No warranty for anything given at all. */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/errno.h> #include <asm/asm.h>
@@ -47,23 +46,16 @@ ENTRY(csum_partial_copy_generic) - CFI_STARTPROC cmpl $3*64, %edx jle .Lignore .Lignore: subq $7*8, %rsp - CFI_ADJUST_CFA_OFFSET 7*8 movq %rbx, 2*8(%rsp) - CFI_REL_OFFSET rbx, 2*8 movq %r12, 3*8(%rsp) - CFI_REL_OFFSET r12, 3*8 movq %r14, 4*8(%rsp) - CFI_REL_OFFSET r14, 4*8 movq %r13, 5*8(%rsp) - CFI_REL_OFFSET r13, 5*8 movq %rbp, 6*8(%rsp) - CFI_REL_OFFSET rbp, 6*8 movq %r8, (%rsp) movq %r9, 1*8(%rsp)
@@ -206,22 +198,14 @@ ENTRY(csum_partial_copy_generic) addl %ebx, %eax adcl %r9d, %eax /* carry */ - CFI_REMEMBER_STATE .Lende: movq 2*8(%rsp), %rbx - CFI_RESTORE rbx movq 3*8(%rsp), %r12 - CFI_RESTORE r12 movq 4*8(%rsp), %r14 - CFI_RESTORE r14 movq 5*8(%rsp), %r13 - CFI_RESTORE r13 movq 6*8(%rsp), %rbp - CFI_RESTORE rbp addq $7*8, %rsp - CFI_ADJUST_CFA_OFFSET -7*8 ret - CFI_RESTORE_STATE /* Exception handlers. Very simple, zeroing is done in the wrappers */ .Lbad_source:
@@ -237,5 +221,4 @@ ENTRY(csum_partial_copy_generic) jz .Lende movl $-EFAULT, (%rax) jmp .Lende - CFI_ENDPROC ENDPROC(csum_partial_copy_generic)
arch/x86/lib/getuser.S +0 -13
@@ -26,7 +26,6 @@ */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/page_types.h> #include <asm/errno.h> #include <asm/asm-offsets.h>
@@ -36,7 +35,6 @@ .text ENTRY(__get_user_1) - CFI_STARTPROC GET_THREAD_INFO(%_ASM_DX) cmp TI_addr_limit(%_ASM_DX),%_ASM_AX jae bad_get_user
@@ -45,11 +43,9 @@ ENTRY(__get_user_1) xor %eax,%eax ASM_CLAC ret - CFI_ENDPROC ENDPROC(__get_user_1) ENTRY(__get_user_2) - CFI_STARTPROC add $1,%_ASM_AX jc bad_get_user GET_THREAD_INFO(%_ASM_DX)
@@ -60,11 +56,9 @@ ENTRY(__get_user_2) xor %eax,%eax ASM_CLAC ret - CFI_ENDPROC ENDPROC(__get_user_2) ENTRY(__get_user_4) - CFI_STARTPROC add $3,%_ASM_AX jc bad_get_user GET_THREAD_INFO(%_ASM_DX)
@@ -75,11 +69,9 @@ ENTRY(__get_user_4) xor %eax,%eax ASM_CLAC ret - CFI_ENDPROC ENDPROC(__get_user_4) ENTRY(__get_user_8) - CFI_STARTPROC #ifdef CONFIG_X86_64 add $7,%_ASM_AX jc bad_get_user
@@ -104,28 +96,23 @@ ENTRY(__get_user_8) ASM_CLAC ret #endif - CFI_ENDPROC ENDPROC(__get_user_8) bad_get_user: - CFI_STARTPROC xor %edx,%edx mov $(-EFAULT),%_ASM_AX ASM_CLAC ret - CFI_ENDPROC END(bad_get_user) #ifdef CONFIG_X86_32 bad_get_user_8: - CFI_STARTPROC xor %edx,%edx xor %ecx,%ecx mov $(-EFAULT),%_ASM_AX ASM_CLAC ret - CFI_ENDPROC END(bad_get_user_8) #endif
arch/x86/lib/iomap_copy_64.S +0 -3
@@ -16,15 +16,12 @@ */ #include <linux/linkage.h> -#include <asm/dwarf2.h> /* * override generic version in lib/iomap_copy.c */ ENTRY(__iowrite32_copy) - CFI_STARTPROC movl %edx,%ecx rep movsd ret - CFI_ENDPROC ENDPROC(__iowrite32_copy)
arch/x86/lib/memcpy_64.S +0 -3
@@ -2,7 +2,6 @@ #include <linux/linkage.h> #include <asm/cpufeature.h> -#include <asm/dwarf2.h> #include <asm/alternative-asm.h> /*
@@ -53,7 +52,6 @@ ENTRY(memcpy_erms) ENDPROC(memcpy_erms) ENTRY(memcpy_orig) - CFI_STARTPROC movq %rdi, %rax cmpq $0x20, %rdx
@@ -178,5 +176,4 @@ ENTRY(memcpy_orig) .Lend: retq - CFI_ENDPROC ENDPROC(memcpy_orig)
arch/x86/lib/memmove_64.S +0 -3
@@ -6,7 +6,6 @@ * - Copyright 2011 Fenghua Yu <[email protected]> */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/cpufeature.h> #include <asm/alternative-asm.h>
@@ -27,7 +26,6 @@ ENTRY(memmove) ENTRY(__memmove) - CFI_STARTPROC /* Handle more 32 bytes in loop */ mov %rdi, %rax
@@ -207,6 +205,5 @@ ENTRY(__memmove) movb %r11b, (%rdi) 13: retq - CFI_ENDPROC ENDPROC(__memmove) ENDPROC(memmove)
arch/x86/lib/memset_64.S +0 -5
@@ -1,7 +1,6 @@ /* Copyright 2002 Andi Kleen, SuSE Labs */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/cpufeature.h> #include <asm/alternative-asm.h>
@@ -66,7 +65,6 @@ ENTRY(memset_erms) ENDPROC(memset_erms) ENTRY(memset_orig) - CFI_STARTPROC movq %rdi,%r10 /* expand byte value */
@@ -78,7 +76,6 @@ ENTRY(memset_orig) movl %edi,%r9d andl $7,%r9d jnz .Lbad_alignment - CFI_REMEMBER_STATE .Lafter_bad_alignment: movq %rdx,%rcx
@@ -128,7 +125,6 @@ ENTRY(memset_orig) movq %r10,%rax
Lines of this file are not shown. View the raw message.
arch/x86/lib/msr-reg.S +17 -27
44 lines not on this page · raw message
arch/x86/lib/rwsem.S +20 -29
49 lines not on this page · raw message
arch/x86/lib/thunk_64.S +19 -25
44 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Part of the diff is not shown here. View the raw message.
PATCH v2 1/1 x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 2 lines · Ingo Molnar
I'll do the attached patch: it gets rid of the unmaintainable dwarf mess from low level x86 assembly code. [...]
The patch below fixes a bug in the removal and is also minimally build and boot tested.
Thanks,
Ingo
===================================>
From: Ingo Molnar <[email protected]>
Date: Thu, 28 May 2015 12:21:47 +0200
Subject: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
So the dwarf2 annotations in low level assembly code have become an increasing hindrance: unreadable, messy macros mixed into some of the most security sensitive code paths of the Linux kernel.
These debug info annotations don't even buy the upstream kernel anything: dwarf driven stack unwinding has caused problems in the past so it's out of tree, and the upstream kernel only uses the much more robust framepointers based stack unwinding method.
In addition to that there's a steady, slow bitrot going on with these annotations, requiring frequent fixups. There's no tooling and no functionality upstream that keeps it correct.
So burn down the sick forest, allowing new, healthier growth:
27 files changed, 350 insertions(+), 1101 deletions(-)
Someone who has the willingness and time to do this properly can attempt to reintroduce dwarf debuginfo in x86 assembly code plus dwarf unwinding from first principles, with the following conditions:
- it should be maximally readable, and maximally low-key to 'ordinary' code reading and maintenance.
- find a build time method to insert dwarf annotations automatically in the most common cases, for pop/push instructions that manipulate the stack pointer. This could be done for example via a preprocessing step that just looks for common patterns - plus special annotations for the few cases where we want to depart from the default. We have hundreds of CFI annotations, so automating most of that makes sense.
- it should come with build tooling checks that ensure that CFI annotations are sensible. We've seen such efforts from the framepointer side, and there's no reason it couldn't be done on the dwarf side.
--- arch/x86/Makefile | 10 +- arch/x86/ia32/ia32entry.S | 133 ++++----------- arch/x86/include/asm/calling.h | 94 +++++------ arch/x86/include/asm/dwarf2.h | 170 ------------------- arch/x86/include/asm/frame.h | 7 +- arch/x86/kernel/entry_32.S | 368 ++++++++++++----------------------------- arch/x86/kernel/entry_64.S | 288 ++++++-------------------------- arch/x86/lib/atomic64_386_32.S | 7 +- arch/x86/lib/atomic64_cx8_32.S | 61 +++---- arch/x86/lib/checksum_32.S | 52 +++--- arch/x86/lib/clear_page_64.S | 7 - arch/x86/lib/cmpxchg16b_emu.S | 12 +- arch/x86/lib/cmpxchg8b_emu.S | 11 +- arch/x86/lib/copy_page_64.S | 11 -- arch/x86/lib/copy_user_64.S | 15 -- arch/x86/lib/csum-copy_64.S | 17 -- arch/x86/lib/getuser.S | 13 -- arch/x86/lib/iomap_copy_64.S | 3 - arch/x86/lib/memcpy_64.S | 3 - arch/x86/lib/memmove_64.S | 3 - arch/x86/lib/memset_64.S | 5 - arch/x86/lib/msr-reg.S | 44 ++--- arch/x86/lib/putuser.S | 8 +- arch/x86/lib/rwsem.S | 49 +++--- arch/x86/lib/thunk_32.S | 15 +- arch/x86/lib/thunk_64.S | 44 +++-- arch/x86/net/bpf_jit.S | 1 - 27 files changed, 350 insertions(+), 1101 deletions(-)
arch/x86/Makefile +2 -8
@@ -149,12 +149,6 @@ endif sp-$(CONFIG_X86_32) := esp sp-$(CONFIG_X86_64) := rsp -# do binutils support CFI? -cfi := $(call as-instr,.cfi_startproc\n.cfi_rel_offset $(sp-y)$(comma)0\n.cfi_endproc,-DCONFIG_AS_CFI=1) -# is .cfi_signal_frame supported too? -cfi-sigframe := $(call as-instr,.cfi_startproc\n.cfi_signal_frame\n.cfi_endproc,-DCONFIG_AS_CFI_SIGNAL_FRAME=1) -cfi-sections := $(call as-instr,.cfi_sections .debug_frame,-DCONFIG_AS_CFI_SECTIONS=1) - # does binutils support specific instructions? asinstr := $(call as-instr,fxsaveq (%rax),-DCONFIG_AS_FXSAVEQ=1) asinstr += $(call as-instr,pshufb %xmm0$(comma)%xmm0,-DCONFIG_AS_SSSE3=1)
@@ -162,8 +156,8 @@ asinstr += $(call as-instr,crc32l %eax$(comma)%eax,-DCONFIG_AS_CRC32=1) avx_instr := $(call as-instr,vxorps %ymm0$(comma)%ymm1$(comma)%ymm2,-DCONFIG_AS_AVX=1) avx2_instr :=$(call as-instr,vpbroadcastb %xmm0$(comma)%ymm1,-DCONFIG_AS_AVX2=1) -KBUILD_AFLAGS += $(cfi) $(cfi-sigframe) $(cfi-sections) $(asinstr) $(avx_instr) $(avx2_instr) -KBUILD_CFLAGS += $(cfi) $(cfi-sigframe) $(cfi-sections) $(asinstr) $(avx_instr) $(avx2_instr) +KBUILD_AFLAGS += $(asinstr) $(avx_instr) $(avx2_instr) +KBUILD_CFLAGS += $(asinstr) $(avx_instr) $(avx2_instr) LDFLAGS := -m elf_$(UTS_MACHINE)
arch/x86/ia32/ia32entry.S +32 -101
283 lines not on this page · raw message
arch/x86/include/asm/calling.h +45 -49
158 lines not on this page · raw message
arch/x86/include/asm/dwarf2.h deleted +0 -170
170 lines not on this page · raw message
arch/x86/include/asm/frame.h +2 -5
@@ -1,20 +1,17 @@#ifdef __ASSEMBLY__#include <asm/asm.h> -#include <asm/dwarf2.h> /* The annotation hides the frame from the unwinder and makes it look like a ordinary ebp save/restore. This avoids some special cases for frame pointer later */ #ifdef CONFIG_FRAME_POINTER .macro FRAME - __ASM_SIZE(push,_cfi) %__ASM_REG(bp) - CFI_REL_OFFSET __ASM_REG(bp), 0 + __ASM_SIZE(push,) %__ASM_REG(bp) __ASM_SIZE(mov) %__ASM_REG(sp), %__ASM_REG(bp) .endm .macro ENDFRAME - __ASM_SIZE(pop,_cfi) %__ASM_REG(bp) - CFI_RESTORE __ASM_REG(bp) + __ASM_SIZE(pop,) %__ASM_REG(bp) .endm #else .macro FRAME
arch/x86/kernel/entry_32.S +108 -260
805 lines not on this page · raw message
arch/x86/kernel/entry_64.S +48 -240
778 lines not on this page · raw message
arch/x86/lib/atomic64_386_32.S +2 -5
@@ -11,26 +11,23 @@ #include <linux/linkage.h> #include <asm/alternative-asm.h> -#include <asm/dwarf2.h> /* if you want SMP support, implement these with real spinlocks */ .macro LOCK reg - pushfl_cfi + pushfl cli .endm .macro UNLOCK reg - popfl_cfi + popfl .endm #define BEGIN(op) \ .macro endp; \ - CFI_ENDPROC; \ ENDPROC(atomic64_##op##_386); \ .purgem endp; \ .endm; \ ENTRY(atomic64_##op##_386); \ - CFI_STARTPROC; \ LOCK v; #define ENDP endp
arch/x86/lib/atomic64_cx8_32.S +20 -41
154 lines not on this page · raw message
arch/x86/lib/checksum_32.S +21 -31
111 lines not on this page · raw message
arch/x86/lib/clear_page_64.S +0 -7
@@ -1,5 +1,4 @@ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/cpufeature.h> #include <asm/alternative-asm.h>
@@ -15,7 +14,6 @@ * %rdi - page */ ENTRY(clear_page) - CFI_STARTPROC ALTERNATIVE_2 "jmp clear_page_orig", "", X86_FEATURE_REP_GOOD, \ "jmp clear_page_c_e", X86_FEATURE_ERMS
@@ -24,11 +22,9 @@ ENTRY(clear_page) xorl %eax,%eax rep stosq ret - CFI_ENDPROC ENDPROC(clear_page) ENTRY(clear_page_orig) - CFI_STARTPROC xorl %eax,%eax movl $4096/64,%ecx
@@ -48,14 +44,11 @@ ENTRY(clear_page_orig) jnz .Lloop nop ret - CFI_ENDPROC ENDPROC(clear_page_orig) ENTRY(clear_page_c_e) - CFI_STARTPROC movl $4096,%ecx xorl %eax,%eax rep stosb ret - CFI_ENDPROC ENDPROC(clear_page_c_e)
arch/x86/lib/cmpxchg16b_emu.S +3 -9
@@ -6,7 +6,6 @@ * */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/percpu.h> .text
@@ -21,7 +20,6 @@ * %al : Operation successful */ ENTRY(this_cpu_cmpxchg16b_emu) -CFI_STARTPROC # # Emulate 'cmpxchg16b %gs:(%rsi)' except we return the result in %al not
@@ -32,7 +30,7 @@ CFI_STARTPROC # *atomic* on a single cpu (as provided by the this_cpu_xx class of # macros). # - pushfq_cfi + pushfq cli cmpq PER_CPU_VAR((%rsi)), %rax
@@ -43,17 +41,13 @@ CFI_STARTPROC movq %rbx, PER_CPU_VAR((%rsi)) movq %rcx, PER_CPU_VAR(8(%rsi)) - CFI_REMEMBER_STATE - popfq_cfi + popfq mov $1, %al ret - CFI_RESTORE_STATE .Lnot_same: - popfq_cfi + popfq xor %al,%al ret -CFI_ENDPROC - ENDPROC(this_cpu_cmpxchg16b_emu)
arch/x86/lib/cmpxchg8b_emu.S +3 -8
@@ -7,7 +7,6 @@ */ #include <linux/linkage.h> -#include <asm/dwarf2.h> .text
@@ -20,14 +19,13 @@ * %ecx : high 32 bits of new value */ ENTRY(cmpxchg8b_emu) -CFI_STARTPROC # # Emulate 'cmpxchg8b (%esi)' on UP except we don't # set the whole ZF thing (caller will just compare # eax:edx with the expected value) # - pushfl_cfi + pushfl cli cmpl (%esi), %eax
@@ -38,18 +36,15 @@ CFI_STARTPROC movl %ebx, (%esi) movl %ecx, 4(%esi) - CFI_REMEMBER_STATE - popfl_cfi + popfl ret - CFI_RESTORE_STATE .Lnot_same: movl (%esi), %eax .Lhalf_same: movl 4(%esi), %edx - popfl_cfi + popfl ret -CFI_ENDPROC ENDPROC(cmpxchg8b_emu)
arch/x86/lib/copy_page_64.S +0 -11
@@ -1,7 +1,6 @@ /* Written 2003 by Andi Kleen, based on a kernel by Evandro Menezes */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/cpufeature.h> #include <asm/alternative-asm.h>
@@ -13,22 +12,16 @@ */ ALIGN ENTRY(copy_page) - CFI_STARTPROC ALTERNATIVE "jmp copy_page_regs", "", X86_FEATURE_REP_GOOD movl $4096/8, %ecx rep movsq ret - CFI_ENDPROC ENDPROC(copy_page) ENTRY(copy_page_regs) - CFI_STARTPROC subq $2*8, %rsp - CFI_ADJUST_CFA_OFFSET 2*8 movq %rbx, (%rsp) - CFI_REL_OFFSET rbx, 0 movq %r12, 1*8(%rsp) - CFI_REL_OFFSET r12, 1*8 movl $(4096/64)-5, %ecx .p2align 4
@@ -87,11 +80,7 @@ ENTRY(copy_page_regs) jnz .Loop2 movq (%rsp), %rbx - CFI_RESTORE rbx movq 1*8(%rsp), %r12 - CFI_RESTORE r12 addq $2*8, %rsp - CFI_ADJUST_CFA_OFFSET -2*8 ret - CFI_ENDPROC ENDPROC(copy_page_regs)
arch/x86/lib/copy_user_64.S +0 -15
@@ -7,7 +7,6 @@ */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/current.h> #include <asm/asm-offsets.h> #include <asm/thread_info.h>
@@ -18,7 +17,6 @@ /* Standard copy_to_user with segment limit checking */ ENTRY(_copy_to_user) - CFI_STARTPROC GET_THREAD_INFO(%rax) movq %rdi,%rcx addq %rdx,%rcx
@@ -30,12 +28,10 @@ ENTRY(_copy_to_user) X86_FEATURE_REP_GOOD, \ "jmp copy_user_enhanced_fast_string", \ X86_FEATURE_ERMS - CFI_ENDPROC ENDPROC(_copy_to_user) /* Standard copy_from_user with segment limit checking */ ENTRY(_copy_from_user) - CFI_STARTPROC GET_THREAD_INFO(%rax) movq %rsi,%rcx addq %rdx,%rcx
@@ -47,14 +43,12 @@ ENTRY(_copy_from_user) X86_FEATURE_REP_GOOD, \ "jmp copy_user_enhanced_fast_string", \ X86_FEATURE_ERMS - CFI_ENDPROC ENDPROC(_copy_from_user) .section .fixup,"ax" /* must zero dest */ ENTRY(bad_from_user) bad_from_user: - CFI_STARTPROC movl %edx,%ecx xorl %eax,%eax rep
@@ -62,7 +56,6 @@ ENTRY(bad_from_user) bad_to_user: movl %edx,%eax ret - CFI_ENDPROC ENDPROC(bad_from_user) .previous
@@ -80,7 +73,6 @@ ENDPROC(bad_from_user) * eax uncopied bytes or 0 if successful. */ ENTRY(copy_user_generic_unrolled) - CFI_STARTPROC ASM_STAC cmpl $8,%edx jb 20f /* less then 8 bytes, go to byte copy loop */
@@ -162,7 +154,6 @@ ENTRY(copy_user_generic_unrolled) _ASM_EXTABLE(19b,40b) _ASM_EXTABLE(21b,50b) _ASM_EXTABLE(22b,50b) - CFI_ENDPROC ENDPROC(copy_user_generic_unrolled) /* Some CPUs run faster using the string copy instructions.
@@ -184,7 +175,6 @@ ENDPROC(copy_user_generic_unrolled) * eax uncopied bytes or 0 if successful. */ ENTRY(copy_user_generic_string) - CFI_STARTPROC ASM_STAC cmpl $8,%edx jb 2f /* less than 8 bytes, go to byte copy loop */
@@ -209,7 +199,6 @@ ENTRY(copy_user_generic_string) _ASM_EXTABLE(1b,11b) _ASM_EXTABLE(3b,12b) - CFI_ENDPROC ENDPROC(copy_user_generic_string) /*
@@ -225,7 +214,6 @@ ENDPROC(copy_user_generic_string) * eax uncopied bytes or 0 if successful. */ ENTRY(copy_user_enhanced_fast_string) - CFI_STARTPROC ASM_STAC movl %edx,%ecx 1: rep
@@ -240,7 +228,6 @@ ENTRY(copy_user_enhanced_fast_string) .previous _ASM_EXTABLE(1b,12b) - CFI_ENDPROC ENDPROC(copy_user_enhanced_fast_string) /*
@@ -248,7 +235,6 @@ ENDPROC(copy_user_enhanced_fast_string) * This will force destination/source out of cache for more performance. */ ENTRY(__copy_user_nocache) - CFI_STARTPROC ASM_STAC cmpl $8,%edx jb 20f /* less then 8 bytes, go to byte copy loop */
@@ -332,5 +318,4 @@ ENTRY(__copy_user_nocache) _ASM_EXTABLE(19b,40b) _ASM_EXTABLE(21b,50b) _ASM_EXTABLE(22b,50b) - CFI_ENDPROC ENDPROC(__copy_user_nocache)
arch/x86/lib/csum-copy_64.S +0 -17
@@ -6,7 +6,6 @@ * for more details. No warranty for anything given at all. */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/errno.h> #include <asm/asm.h>
@@ -47,23 +46,16 @@ ENTRY(csum_partial_copy_generic) - CFI_STARTPROC cmpl $3*64, %edx jle .Lignore .Lignore: subq $7*8, %rsp - CFI_ADJUST_CFA_OFFSET 7*8 movq %rbx, 2*8(%rsp) - CFI_REL_OFFSET rbx, 2*8 movq %r12, 3*8(%rsp) - CFI_REL_OFFSET r12, 3*8 movq %r14, 4*8(%rsp) - CFI_REL_OFFSET r14, 4*8 movq %r13, 5*8(%rsp) - CFI_REL_OFFSET r13, 5*8 movq %rbp, 6*8(%rsp) - CFI_REL_OFFSET rbp, 6*8 movq %r8, (%rsp) movq %r9, 1*8(%rsp)
@@ -206,22 +198,14 @@ ENTRY(csum_partial_copy_generic) addl %ebx, %eax adcl %r9d, %eax /* carry */ - CFI_REMEMBER_STATE .Lende: movq 2*8(%rsp), %rbx - CFI_RESTORE rbx movq 3*8(%rsp), %r12 - CFI_RESTORE r12 movq 4*8(%rsp), %r14 - CFI_RESTORE r14 movq 5*8(%rsp), %r13 - CFI_RESTORE r13 movq 6*8(%rsp), %rbp - CFI_RESTORE rbp addq $7*8, %rsp - CFI_ADJUST_CFA_OFFSET -7*8 ret - CFI_RESTORE_STATE /* Exception handlers. Very simple, zeroing is done in the wrappers */ .Lbad_source:
@@ -237,5 +221,4 @@ ENTRY(csum_partial_copy_generic) jz .Lende movl $-EFAULT, (%rax) jmp .Lende - CFI_ENDPROC ENDPROC(csum_partial_copy_generic)
arch/x86/lib/getuser.S +0 -13
@@ -26,7 +26,6 @@ */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/page_types.h> #include <asm/errno.h> #include <asm/asm-offsets.h>
@@ -36,7 +35,6 @@ .text ENTRY(__get_user_1) - CFI_STARTPROC GET_THREAD_INFO(%_ASM_DX) cmp TI_addr_limit(%_ASM_DX),%_ASM_AX jae bad_get_user
@@ -45,11 +43,9 @@ ENTRY(__get_user_1) xor %eax,%eax ASM_CLAC ret - CFI_ENDPROC ENDPROC(__get_user_1) ENTRY(__get_user_2) - CFI_STARTPROC add $1,%_ASM_AX jc bad_get_user GET_THREAD_INFO(%_ASM_DX)
@@ -60,11 +56,9 @@ ENTRY(__get_user_2) xor %eax,%eax ASM_CLAC ret - CFI_ENDPROC ENDPROC(__get_user_2) ENTRY(__get_user_4) - CFI_STARTPROC add $3,%_ASM_AX jc bad_get_user GET_THREAD_INFO(%_ASM_DX)
@@ -75,11 +69,9 @@ ENTRY(__get_user_4) xor %eax,%eax ASM_CLAC ret - CFI_ENDPROC ENDPROC(__get_user_4) ENTRY(__get_user_8) - CFI_STARTPROC #ifdef CONFIG_X86_64 add $7,%_ASM_AX jc bad_get_user
@@ -104,28 +96,23 @@ ENTRY(__get_user_8) ASM_CLAC ret #endif - CFI_ENDPROC ENDPROC(__get_user_8) bad_get_user: - CFI_STARTPROC xor %edx,%edx mov $(-EFAULT),%_ASM_AX ASM_CLAC ret - CFI_ENDPROC END(bad_get_user) #ifdef CONFIG_X86_32 bad_get_user_8: - CFI_STARTPROC xor %edx,%edx xor %ecx,%ecx mov $(-EFAULT),%_ASM_AX ASM_CLAC ret - CFI_ENDPROC END(bad_get_user_8) #endif
arch/x86/lib/iomap_copy_64.S +0 -3
@@ -16,15 +16,12 @@ */ #include <linux/linkage.h> -#include <asm/dwarf2.h> /* * override generic version in lib/iomap_copy.c */ ENTRY(__iowrite32_copy) - CFI_STARTPROC movl %edx,%ecx rep movsd ret - CFI_ENDPROC ENDPROC(__iowrite32_copy)
arch/x86/lib/memcpy_64.S +0 -3
@@ -2,7 +2,6 @@ #include <linux/linkage.h> #include <asm/cpufeature.h> -#include <asm/dwarf2.h> #include <asm/alternative-asm.h> /*
@@ -53,7 +52,6 @@ ENTRY(memcpy_erms) ENDPROC(memcpy_erms) ENTRY(memcpy_orig) - CFI_STARTPROC movq %rdi, %rax cmpq $0x20, %rdx
@@ -178,5 +176,4 @@ ENTRY(memcpy_orig) .Lend: retq - CFI_ENDPROC ENDPROC(memcpy_orig)
arch/x86/lib/memmove_64.S +0 -3
@@ -6,7 +6,6 @@ * - Copyright 2011 Fenghua Yu <[email protected]> */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/cpufeature.h> #include <asm/alternative-asm.h>
@@ -27,7 +26,6 @@ ENTRY(memmove) ENTRY(__memmove) - CFI_STARTPROC /* Handle more 32 bytes in loop */ mov %rdi, %rax
@@ -207,6 +205,5 @@ ENTRY(__memmove) movb %r11b, (%rdi) 13: retq - CFI_ENDPROC ENDPROC(__memmove) ENDPROC(memmove)
arch/x86/lib/memset_64.S +0 -5
@@ -1,7 +1,6 @@ /* Copyright 2002 Andi Kleen, SuSE Labs */ #include <linux/linkage.h> -#include <asm/dwarf2.h> #include <asm/cpufeature.h> #include <asm/alternative-asm.h>
@@ -66,7 +65,6 @@ ENTRY(memset_erms) ENDPROC(memset_erms) ENTRY(memset_orig) - CFI_STARTPROC movq %rdi,%r10 /* expand byte value */
@@ -78,7 +76,6 @@ ENTRY(memset_orig) movl %edi,%r9d andl $7,%r9d jnz .Lbad_alignment - CFI_REMEMBER_STATE .Lafter_bad_alignment: movq %rdx,%rcx
@@ -128,7 +125,6 @@ ENTRY(memset_orig) movq %r10,%rax
Lines of this file are not shown. View the raw message.
arch/x86/lib/msr-reg.S +17 -27
44 lines not on this page · raw message
arch/x86/lib/rwsem.S +20 -29
49 lines not on this page · raw message
arch/x86/lib/thunk_64.S +19 -25
44 lines not on this page · raw message
The rest of this diff is not shown. View the raw message.
Part of the diff is not shown here. View the raw message.
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
In reply to Ingo MolnarQuoted 22 lines
Quoted 1 line
Quoted 1 line · On 28.05.15 at 13:20, <[email protected]> wrote:
Quoted 2 lines · Jan Beulich
Not sure why assembly code should look like C code. It's a matter of taste perhaps, and I can see your point, but I'm also not really eager to do
changes
Quoted 2 lines
just to match other people's taste. And just like above - certainly not something for this patch I would think.
Yeah, no, so this isn't going to work that way.
On one hand you want dwarf annotations mostly for the out of tree dwarf-unwinding
stack backtraces patch on SUSE kernels, while for the upstream kernel it's mostly
just unreadable gunk in some of the most security sensitive code paths of the
kernel, which only gets in the way of readability.But on the other hand you are unwilling to (or don't have the time to) do a proper
job of making this palatable for upstream.
Quoted 7 lines
That's unacceptable from the upstream kernel's POV, so instead of limping forward
I'll do the attached patch: it gets rid of the unmaintainable dwarf mess from low
level x86 assembly code. This isn't a new concern, a couple of years ago we almost
did this.
I can understand your motivation, yet I still view it as rather sad that you move this way. Indeed I don't have the time to do major rework in this area, but I don't think you can blame me for not having tried to at least investigate and eliminate breakage when I found such (which, as you say, happens every now and then). Yet I do recall people indicating that the unwind data can be useful for other than the out-of-tree live stack unwinder. I.e. those will be broken along with that code which we're _forced_ to maintain out-of-tree.
Quoted 2 lines
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Funny suggestion - I don't think that's reasonable for us to do. Or if we were to, we could as well invest in doing the re-work you're asking for; I don't think anyone will have the time to do either.
Jan
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 6 lines · Jan Beulich
Quoted 2 lines
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Funny suggestion - I don't think that's reasonable for us to do. Or if we were to, we could as well invest in doing the re-work you're asking for; I don't think anyone will have the time to do either.
That's fair enough: if there's not enough resources to keep a feature maintainable upstream then it should not be upstream in that form.
This isn't just some driver we can let bit-rot in peace until it finds a maintainer (or not), without affecting anyone but users of that driver.
This is hundreds of usage sites of ugly code intermixed with critical pieces of assembly code that negatively affects the hackability of everything.
Also, with the feature missing completely, maybe someone finds a method to introduce it in a maintainable fashion, while with the feature included upstream there's very little pressure to do that. As a bonus we'd also win a workable dwarf unwinder.
Thanks,
Ingo
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Jan Beulich
Quoted 2 lines
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Funny suggestion - I don't think that's reasonable for us to do. Or if we were to, we could as well invest in doing the re-work you're asking for; I don't think anyone will have the time to do either.
That's fair enough: if there's not enough resources to keep a feature maintainable upstream then it should not be upstream in that form.
This isn't just some driver we can let bit-rot in peace until it finds a maintainer (or not), without affecting anyone but users of that driver.
Quoted 7 lines
This is hundreds of usage sites of ugly code intermixed with critical pieces of assembly code that negatively affects the hackability of everything.
Also, with the feature missing completely, maybe someone finds a method to introduce it in a maintainable fashion, while with the feature included upstream there's very little pressure to do that. As a bonus we'd also win a workable dwarf unwinder.
Before doing something drastic like this, I think we should get Josh's opinion, since I think he's working on a new (?) unwinder.
FWIW, musl is considering some kind of automatic annotation scheme:
http://www.openwall.com/lists/musl/2015/05/13/5
--Andy
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 18 lines · Andy Lutomirski
Quoted 17 lines · Ingo Molnar
Quoted 6 lines · Jan Beulich
Quoted 2 lines
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Funny suggestion - I don't think that's reasonable for us to do. Or if we were to, we could as well invest in doing the re-work you're asking for; I don't think anyone will have the time to do either.
That's fair enough: if there's not enough resources to keep a feature maintainable upstream then it should not be upstream in that form.
This isn't just some driver we can let bit-rot in peace until it finds a maintainer (or not), without affecting anyone but users of that driver.
This is hundreds of usage sites of ugly code intermixed with critical pieces of assembly code that negatively affects the hackability of everything.
Quoted 7 lines
Quoted 4 lines
Also, with the feature missing completely, maybe someone finds a method to introduce it in a maintainable fashion, while with the feature included upstream there's very little pressure to do that. As a bonus we'd also win a workable dwarf unwinder.
Before doing something drastic like this, I think we should get Josh's opinion, since I think he's working on a new (?) unwinder.
I'd definitely like to replace all the asm DWARF CFI annotations with something more automated and robust. So it doesn't really affect me whether they're ripped out now or replaced later.
But it might be a few months before I have the code. If they get ripped out now, it might affect those tools which rely on debuginfo like perf, kprobes, systemtap, gdb, crash, etc.
Then again, I'm not sure how useful or reliable the existing annotations are anyway, so maybe it doesn't matter much.
CCing some possible consumers of kernel x86 DWARF data to see if they have any opinions.
Signature · 1 line
Josh
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Hi -
Quoted 9 lines · Josh Poimboeuf
[...]
Quoted 7 lines
Quoted 4 lines
Also, with the feature missing completely, maybe someone finds a method to introduce it in a maintainable fashion, while with the feature included upstream there's very little pressure to do that. As a bonus we'd also win a workable dwarf unwinder.
Before doing something drastic like this, I think we should get Josh's opinion, since I think he's working on a new (?) unwinder.
Quoted 6 lines
I'd definitely like to replace all the asm DWARF CFI annotations with something more automated and robust. So it doesn't really affect me whether they're ripped out now or replaced later.
[...]
Then again, I'm not sure how useful or reliable the existing annotations are anyway, so maybe it doesn't matter much.
In our experience as consumers of this CFI information for years in systemtap, the annotations have been generally correct and reliable. Their presence allows reliable, correct, and efficient kernel->userspace backtracing as used in important systemtap scripts.
If the current complaint is primarily about testability, it would be easy to add simple stap-based tests to the kernel to exercise the code and confirm its operation. Perhaps we could extract a specialized self-contained test case (containing an unwinder).
I'm not in a position to judge the purported cost savings of removing this code, but there is definitely a negative benefit as a loss of useful functionality, esp. with no replacement in sight.
- FChE
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
In reply to Andy LutomirskiQuoted 25 lines · Andy Lutomirski
Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Jan Beulich
Quoted 2 lines
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Funny suggestion - I don't think that's reasonable for us to do. Or if we were to, we could as well invest in doing the re-work you're asking for; I don't think anyone will have the time to do either.
That's fair enough: if there's not enough resources to keep a feature maintainable upstream then it should not be upstream in that form.
This isn't just some driver we can let bit-rot in peace until it finds a maintainer (or not), without affecting anyone but users of that driver.
Quoted 7 lines
This is hundreds of usage sites of ugly code intermixed with critical pieces of assembly code that negatively affects the hackability of everything.
Also, with the feature missing completely, maybe someone finds a method to introduce it in a maintainable fashion, while with the feature included upstream there's very little pressure to do that. As a bonus we'd also win a workable dwarf unwinder.
Quoted 6 lines
Before doing something drastic like this, I think we should get Josh's opinion, since I think he's working on a new (?) unwinder.
FWIW, musl is considering some kind of automatic annotation scheme:
Thanks for the link! I found a newer version of it here:
http://www.openwall.com/lists/musl/2015/05/31/5
Overall I think that script is a really good solution.
Quoted 1 line
From what I can tell, it tracks the CFA (stack pointer) perfectly.
(Which is actually pretty straightfoward if you just hook into function entry/exit, push/pop, and add/sub to rsp).
It also does a nice job at making a best effort at tracking the caller's register values (which are less important than CFA but still nice to have).
Overall I'd bet that it would produce much more accurate results than our current manual annotations.
I'm not crazy about the fact that it relies on awk, but I can't think of a less hacky way to do that, without stuffing the CFI metadata directly into the binary.
We could pretty easily port something like that to the kernel. Before that though, I think we'll need my stack validation patch set which helps enforce asm function/non-function boundaries and proper ELF function annotations. Wrapping up a new version of that now.
Signature · 1 line
Josh
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 39 lines · Josh Poimboeuf
Quoted 25 lines · Andy Lutomirski
Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Jan Beulich
Quoted 2 lines
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Funny suggestion - I don't think that's reasonable for us to do. Or if we were to, we could as well invest in doing the re-work you're asking for; I don't think anyone will have the time to do either.
That's fair enough: if there's not enough resources to keep a feature maintainable upstream then it should not be upstream in that form.
This isn't just some driver we can let bit-rot in peace until it finds a maintainer (or not), without affecting anyone but users of that driver.
Quoted 7 lines
This is hundreds of usage sites of ugly code intermixed with critical pieces of assembly code that negatively affects the hackability of everything.
Also, with the feature missing completely, maybe someone finds a method to introduce it in a maintainable fashion, while with the feature included upstream there's very little pressure to do that. As a bonus we'd also win a workable dwarf unwinder.
Quoted 6 lines
Before doing something drastic like this, I think we should get Josh's opinion, since I think he's working on a new (?) unwinder.
FWIW, musl is considering some kind of automatic annotation scheme:
Thanks for the link! I found a newer version of it here:
http://www.openwall.com/lists/musl/2015/05/31/5
Overall I think that script is a really good solution.
Quoted 7 lines
From what I can tell, it tracks the CFA (stack pointer) perfectly. (Which is actually pretty straightfoward if you just hook into function entry/exit, push/pop, and add/sub to rsp).
It also does a nice job at making a best effort at tracking the caller's register values (which are less important than CFA but still nice to have).
It might be nice to be able to reliably unwind out from an exception / interrupt / syscall frame into userspace or into the kernel code that trapped, complete with registers.
In any event, we'll almost certainly have to manually annotate these weird types of entries. I wonder if we could manage to annotate just the entry parts and let a magic script do the rest.
--Andy
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 48 lines · Andy Lutomirski
Quoted 39 lines · Josh Poimboeuf
Quoted 25 lines · Andy Lutomirski
Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Jan Beulich
Quoted 2 lines
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Funny suggestion - I don't think that's reasonable for us to do. Or if we were to, we could as well invest in doing the re-work you're asking for; I don't think anyone will have the time to do either.
That's fair enough: if there's not enough resources to keep a feature maintainable upstream then it should not be upstream in that form.
This isn't just some driver we can let bit-rot in peace until it finds a maintainer (or not), without affecting anyone but users of that driver.
Quoted 7 lines
This is hundreds of usage sites of ugly code intermixed with critical pieces of assembly code that negatively affects the hackability of everything.
Also, with the feature missing completely, maybe someone finds a method to introduce it in a maintainable fashion, while with the feature included upstream there's very little pressure to do that. As a bonus we'd also win a workable dwarf unwinder.
Quoted 6 lines
Before doing something drastic like this, I think we should get Josh's opinion, since I think he's working on a new (?) unwinder.
FWIW, musl is considering some kind of automatic annotation scheme:
Thanks for the link! I found a newer version of it here:
http://www.openwall.com/lists/musl/2015/05/31/5
Overall I think that script is a really good solution.
Quoted 7 lines
From what I can tell, it tracks the CFA (stack pointer) perfectly. (Which is actually pretty straightfoward if you just hook into function entry/exit, push/pop, and add/sub to rsp).
It also does a nice job at making a best effort at tracking the caller's register values (which are less important than CFA but still nice to have).
Quoted 7 lines
It might be nice to be able to reliably unwind out from an exception / interrupt / syscall frame into userspace or into the kernel code that trapped, complete with registers.
In any event, we'll almost certainly have to manually annotate these weird types of entries. I wonder if we could manage to annotate just the entry parts and let a magic script do the rest.
Yeah. Any callable function can be annotated "magically". We may have to manually annotate the rest.
Signature · 1 line
Josh
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
In reply to Andy LutomirskiQuoted 48 lines · Andy Lutomirski
Quoted 39 lines · Josh Poimboeuf
Quoted 25 lines · Andy Lutomirski
Quoted 16 lines · Ingo Molnar
Quoted 6 lines · Jan Beulich
Quoted 2 lines
and meanwhile you can keep a revert of this patch ported to SUSE kernels in whatever fashion you prefer.
Funny suggestion - I don't think that's reasonable for us to do. Or if we were to, we could as well invest in doing the re-work you're asking for; I don't think anyone will have the time to do either.
That's fair enough: if there's not enough resources to keep a feature maintainable upstream then it should not be upstream in that form.
This isn't just some driver we can let bit-rot in peace until it finds a maintainer (or not), without affecting anyone but users of that driver.
Quoted 7 lines
This is hundreds of usage sites of ugly code intermixed with critical pieces of assembly code that negatively affects the hackability of everything.
Also, with the feature missing completely, maybe someone finds a method to introduce it in a maintainable fashion, while with the feature included upstream there's very little pressure to do that. As a bonus we'd also win a workable dwarf unwinder.
Quoted 6 lines
Before doing something drastic like this, I think we should get Josh's opinion, since I think he's working on a new (?) unwinder.
FWIW, musl is considering some kind of automatic annotation scheme:
Thanks for the link! I found a newer version of it here:
http://www.openwall.com/lists/musl/2015/05/31/5
Overall I think that script is a really good solution.
Quoted 7 lines
From what I can tell, it tracks the CFA (stack pointer) perfectly. (Which is actually pretty straightfoward if you just hook into function entry/exit, push/pop, and add/sub to rsp).
It also does a nice job at making a best effort at tracking the caller's register values (which are less important than CFA but still nice to have).
Quoted 7 lines
It might be nice to be able to reliably unwind out from an exception / interrupt / syscall frame into userspace or into the kernel code that trapped, complete with registers.
In any event, we'll almost certainly have to manually annotate these weird types of entries. I wonder if we could manage to annotate just the entry parts and let a magic script do the rest.
Even the entry parts we could help without uglifying the code:
- either by adding a 'RET' instruction after IRET/SYSRET/SYSEXIT/etc. that the tooling can recognize as 'return from function'. That's much nicer than ugly annotations.
- enhancing the tooling script to also recognize these instructions as function returns - because they _are_ function returns.
So I'll commit the removal patch - it's clear at this point that the old annotations won't be used and they are actively getting in the way of bug fixes and new work. This became glaringly obvious during and after the big asm code refresh we did in v4.1.
If anyone needs debuginfo badly (who?) then they should help Josh get it all upstream again ASAP, it doesn't seem much is missing to get all that done in a much cleaner way.
Thanks,
Ingo
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 29 lines · Ingo Molnar
Quoted 20 lines · Andy Lutomirski
Quoted 11 lines · Josh Poimboeuf
Quoted 3 lines · Andy Lutomirski
FWIW, musl is considering some kind of automatic annotation scheme:
Thanks for the link! I found a newer version of it here:
http://www.openwall.com/lists/musl/2015/05/31/5
Overall I think that script is a really good solution.
Quoted 7 lines
From what I can tell, it tracks the CFA (stack pointer) perfectly. (Which is actually pretty straightfoward if you just hook into function entry/exit, push/pop, and add/sub to rsp).
It also does a nice job at making a best effort at tracking the caller's register values (which are less important than CFA but still nice to have).
Quoted 7 lines
It might be nice to be able to reliably unwind out from an exception / interrupt / syscall frame into userspace or into the kernel code that trapped, complete with registers.
In any event, we'll almost certainly have to manually annotate these weird types of entries. I wonder if we could manage to annotate just the entry parts and let a magic script do the rest.
Quoted 8 lines
Even the entry parts we could help without uglifying the code:
- either by adding a 'RET' instruction after IRET/SYSRET/SYSEXIT/etc. that the tooling can recognize as 'return from function'. That's much nicer than ugly annotations.
- enhancing the tooling script to also recognize these instructions as function returns - because they _are_ function returns.
I think the problem with the entry code (and other non-function asm code) is that it's quite spaghetti-esque, with lots of jumps, returns, calls, etc to random places. There aren't enough constraints which would help the tooling make sense of where execution begins and ends, when registers are saved or trashed, etc.
Maybe over time we can figure out what constraints (and/or annotations) are needed there.
Signature · 1 line
Josh
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
Quoted 39 lines · Josh Poimboeuf
Quoted 29 lines · Ingo Molnar
Quoted 20 lines · Andy Lutomirski
Quoted 11 lines · Josh Poimboeuf
Quoted 3 lines · Andy Lutomirski
FWIW, musl is considering some kind of automatic annotation scheme:
Thanks for the link! I found a newer version of it here:
http://www.openwall.com/lists/musl/2015/05/31/5
Overall I think that script is a really good solution.
Quoted 7 lines
From what I can tell, it tracks the CFA (stack pointer) perfectly. (Which is actually pretty straightfoward if you just hook into function entry/exit, push/pop, and add/sub to rsp).
It also does a nice job at making a best effort at tracking the caller's register values (which are less important than CFA but still nice to have).
Quoted 7 lines
It might be nice to be able to reliably unwind out from an exception / interrupt / syscall frame into userspace or into the kernel code that trapped, complete with registers.
In any event, we'll almost certainly have to manually annotate these weird types of entries. I wonder if we could manage to annotate just the entry parts and let a magic script do the rest.
Quoted 8 lines
Even the entry parts we could help without uglifying the code:
- either by adding a 'RET' instruction after IRET/SYSRET/SYSEXIT/etc. that the tooling can recognize as 'return from function'. That's much nicer than ugly annotations.
- enhancing the tooling script to also recognize these instructions as function returns - because they _are_ function returns.
Quoted 9 lines
I think the problem with the entry code (and other non-function asm code) is that it's quite spaghetti-esque, with lots of jumps, returns, calls, etc to random places. There aren't enough constraints which would help the tooling make sense of where execution begins and ends, when registers are saved or trashed, etc.
Maybe over time we can figure out what constraints (and/or annotations) are needed there.
We can also try to reduce the spaghetti, which I'm working on. (It's even harder than I expected, and I doubt that any of my code will be ready for 4.2.)
--Andy
Re: [PATCH] x86/debug: Remove perpetually broken, unmaintainable dwarf annotations
In reply to Andy LutomirskiQuoted 3 lines · Andy Lutomirski
Before doing something drastic like this, I think we should get Josh's opinion, since I think he's working on a new (?) unwinder.
It's also needed by gdb. gdbstub is in tree.
So no Ingo. You cannot just remove it. Please put it back.
-Andi
Signature · 1 line
[email protected] -- Speaking for myself only