Re: [PATCH] x86: clean up after: move entry_64.S register savingout of the macros

[Cyrill Gorcunov]

[Alexander van Heukelum - Thu, Nov 20, 2008 at 04:57:44PM +0100]
| On Thu, Nov 20, 2008 at 04:39:54PM +0100, Ingo Molnar wrote:
| > 
| > * Alexander van Heukelum <heukelum@xxxxxxxxxxxxx> wrote:
| > 
| > > On Thu, Nov 20, 2008 at 04:04:12PM +0100, Ingo Molnar wrote:
| > > > 
| > > > * Alexander van Heukelum <heukelum@xxxxxxxxxxxxx> wrote:
| > > > 
| > > > > This add-on patch to x86: move entry_64.S register saving out of the 
| > > > > macros visually cleans up the appearance of the code by introducing 
| > > > > some basic helper macro's. It also adds some cfi annotations which 
| > > > > were missing.
| > > > > 
| > > > > Signed-off-by: Alexander van Heukelum <heukelum@xxxxxxxxxxx>
| > > > > ---
| > > > >  arch/x86/kernel/entry_64.S |  220 ++++++++++++++++++++++----------------------
| > > > >  1 files changed, 112 insertions(+), 108 deletions(-)
| > > > > 
| > > > > Hello Ingo,
| > > > > 
| > > > > This patch improves the CFI-situation in entry_64.S, but restricted 
| > > > > mostly to the areas touched by "x86: move entry_64.S register saving 
| > > > > out of the macros". I'm sure there will be some small errors 
| > > > > somewhere, but it compiles and runs fine.
| > > > 
| > > > very nice cleanup! This is exactly what should be done. Applied to 
| > > > tip/x86/irq.
| > > > 
| > > > Note, i did a small rename:
| > > > 
| > > >     cfi_pushq  =>  pushq_cfi
| > > >     cfi_popq   =>  popq_cfi
| > > >     cfi_store  =>  movq_cfi
| 
| Does not work... But if you are attached to the underscores, I
| think we can force it to work by using CPP to convert it to
| something the assembler does parse right:
| 
| #define pushq_cfi pushq.cfi
| 
| etc?
| 
| Or is that just too ugly?
| 
| Alexander
| 
| > > > as the goal is to have the actual source code read mostly as regular 
| > > > assembly code. The fact that the macro is equivalent to a 
| > > > default-annotated pushq/popq/movq instruction is much more important 
| > > > than the fact that it also does CFI annotations.
| > > > 
| > > > Also, while cfi_store is correct as well, the usual x86 assembly term 
| > > > (and instruction used here) is movq.
| > > 
| > > Now I have a little problem with my next patch... I wanted to 
| > > introduce cfi_load. Guess what assembly instruction that maps to ;).
| > 
| > heh ;-)
| > 
| > the restore direction could be named movq_cfi_restore, and have the same 
| > order of arguments as the regular movq that it replaces. I.e.:
| > 
| >         movq 8(%rsp),%r11
| >         CFI_RESTORE r11
| > 
| > would map to:
| > 
| > 	movq_cfi_restore 8, r11
| > 
| > or so.
| > 
| > cfi_store has really a bad name: it's confusing whether it's the CFI 
| > info we are storing/registering (which we are), or a 'store' instruction 
| > (which this is too).
| > 
| > If then we should name it movq_cfi_store or movq_cfi_register - but 
| > that's too long.
| > 
| > movq_cfi for the frame construction direction and movq_cfi_restore for 
| > the frame deconstruction phase sounds like a good naming compromise, hm?
| > 
| > 	Ingo
| 
| -- 
| Alexander van Heukelum
| 

Hi,

I didn't check if it possible but maybe just put these CFI
annotations on right side like

	movq 8(%rsp),%r11		; CFI_RESTORE r11

Hmm? Yes I know -- it will not make 'auto' cfi annotations
but *anyway* if say I'm to write this kind of code I _have_
to know how CFI works (or at least read some specs about)
I guess.

		- Cyrill -
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/