Messages in this thread | | | Date | Thu, 28 May 2015 10:45:33 +0100 | From | "Jan Beulich" <> | Subject | Re: [PATCH] x86-64: fix unwind info for incomplete frames |
| |
>>> On 28.05.15 at 11:01, <mingo@kernel.org> wrote: > * Jan Beulich <JBeulich@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
| |