From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753626AbbE1Jpq (ORCPT ); Thu, 28 May 2015 05:45:46 -0400 Received: from mail.emea.novell.com ([130.57.118.101]:55337 "EHLO mail.emea.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751548AbbE1Jph convert rfc822-to-8bit (ORCPT ); Thu, 28 May 2015 05:45:37 -0400 Message-Id: <5566FFDD020000780007E711@mail.emea.novell.com> X-Mailer: Novell GroupWise Internet Agent 14.0.1 Date: Thu, 28 May 2015 10:45:33 +0100 From: "Jan Beulich" To: "Ingo Molnar" Cc: "Andy Lutomirski" , , "Brian Gerst" , , "Linus Torvalds" , "Denys Vlasenko" , , Subject: Re: [PATCH] x86-64: fix unwind info for incomplete frames References: <5566EBE7020000780007E659@mail.emea.novell.com> <20150528090133.GA469@gmail.com> In-Reply-To: <20150528090133.GA469@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 8BIT Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >>> On 28.05.15 at 11:01, wrote: > * Jan Beulich wrote: >> --- 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 I can certainly do this; as easy as "sed s/extra/full/g" on the patch - perhaps you could even do this while committing? > - 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! CFI_DEFAULT_FRAME? Anyway - clearly not in this patch. > - 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. 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