>>> On 28.05.15 at 11:01, <mi...@kernel.org> wrote: > * Jan Beulich <jbeul...@suse.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
