Re: [PATCH] x86-64: fix unwind info for incomplete frames

From: Ingo Molnar <mingo@kernel.org>
To: Jan Beulich <JBeulich@suse.com>
Cc: mingo@elte.hu, tglx@linutronix.de, hpa@zytor.com,
	Andy Lutomirski <luto@amacapital.net>,
	Denys Vlasenko <dvlasenk@redhat.com>,
	linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Brian Gerst <brgerst@gmail.com>
Subject: Re: [PATCH] x86-64: fix unwind info for incomplete frames
Date: Thu, 28 May 2015 11:01:34 +0200	[thread overview]
Message-ID: <20150528090133.GA469@gmail.com> (raw)
In-Reply-To: <5566EBE7020000780007E659@mail.emea.novell.com>

* 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