lkml.org 
[lkml]   [2015]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86-64: fix unwind info for incomplete frames

* Jan Beulich <JBeulich@suse.com> wrote:

> 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.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> ---
> arch/x86/kernel/entry_64.S | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> --- 4.1-rc5/arch/x86/kernel/entry_64.S
> +++ 4.1-rc5-x86_64-unwind-info/arch/x86/kernel/entry_64.S
> @@ -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

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


\
 
 \ /
  Last update: 2015-05-28 11:41    [W:0.099 / U:1.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site