[2.6 patch] x86_64: re-add a newline to RESTORE_CONTEXT

[2.6 patch] x86_64: re-add a newline to RESTORE_CONTEXT

[Adrian Bunk]

RESTORE_CONTEXT lost a newline in 
commit 658fdbef66e5e9be79b457edc2cbbb3add840aa9:
http://www.mail-archive.com/kgdb-bugreport@lists.sourceforge.net/msg00559.html

Reported by Steven M. Christey.

Signed-off-by: Adrian Bunk <bunk@stusta.de>

--- a/include/asm-x86_64/system.h
+++ b/include/asm-x86_64/system.h
@@ -21,7 +21,7 @@
 
 /* frame pointer must be last for get_wchan */
 #define SAVE_CONTEXT    "pushf ; pushq %%rbp ; movq %%rsi,%%rbp\n\t"
-#define RESTORE_CONTEXT "movq %%rbp,%%rsi ; popq %%rbp ; popf\t"
+#define RESTORE_CONTEXT "movq %%rbp,%%rsi ; popq %%rbp ; popf\n\t"
 
 #define __EXTRA_CLOBBER  \
 	,"rcx","rbx","rdx","r8","r9","r10","r11","r12","r13","r14","r15"

Re: [2.6 patch] x86_64: re-add a newline to RESTORE_CONTEXT

[Andi Kleen]

On Tuesday 09 January 2007 03:55, Adrian Bunk wrote:
> RESTORE_CONTEXT lost a newline in 
> commit 658fdbef66e5e9be79b457edc2cbbb3add840aa9:
> http://www.mail-archive.com/kgdb-bugreport@lists.sourceforge.net/msg00559.html

I don't think we should add such changes for external patchkits.

In general kgdb shouldn't add any patches at all. If the existing 
hooks are not enough they should submit their changes needed so
that it can just work.


-Andi

Re: [2.6 patch] x86_64: re-add a newline to RESTORE_CONTEXT

[Andrew Morton]

On Tue, 9 Jan 2007 12:01:21 +0100
Andi Kleen <ak@suse.de> wrote:

> On Tuesday 09 January 2007 03:55, Adrian Bunk wrote:
> > RESTORE_CONTEXT lost a newline in 
> > commit 658fdbef66e5e9be79b457edc2cbbb3add840aa9:
> > http://www.mail-archive.com/kgdb-bugreport@lists.sourceforge.net/msg00559.html
> 
> I don't think we should add such changes for external patchkits.
> 
> In general kgdb shouldn't add any patches at all. If the existing 
> hooks are not enough they should submit their changes needed so
> that it can just work.
> 

But the patch is a bugfix.  Without it, you cannot do

	RESTORE_CONTEXT	\
	.globl ...	\

Was the addition of this restriction to RESTORE_CONTEXT deliberate, or
mistaken?

Re: [2.6 patch] x86_64: re-add a newline to RESTORE_CONTEXT

[Andi Kleen]

> 
> But the patch is a bugfix.  Without it, you cannot do
> 
> 	RESTORE_CONTEXT	\
> 	.globl ...	\
> 
> Was the addition of this restriction to RESTORE_CONTEXT deliberate, or
> mistaken?

RESTORE_CONTEXT is a private macro and I don't see why we should
support out of tree usages for that. As long as it works as it is 
in the tree it is fine.

-Andi

Re: [2.6 patch] x86_64: re-add a newline to RESTORE_CONTEXT

[Andrew Morton]

On Tue, 9 Jan 2007 23:43:00 +0100
Andi Kleen <ak@suse.de> wrote:

> 
> > 
> > But the patch is a bugfix.  Without it, you cannot do
> > 
> > 	RESTORE_CONTEXT	\
> > 	.globl ...	\
> > 
> > Was the addition of this restriction to RESTORE_CONTEXT deliberate, or
> > mistaken?
> 
> RESTORE_CONTEXT is a private macro and I don't see why we should
> support out of tree usages for that. As long as it works as it is 
> in the tree it is fine.
> 

In other words we'll leave it in its present buggy form so that it will
explode next time someone tries to use it for something new, rather than a)
fixing that potential problem and b) fixing a real problem with a popular
external GPLed product.

Unfathomable.

Re: [2.6 patch] x86_64: re-add a newline to RESTORE_CONTEXT

[Andi Kleen]

> 
> In other words we'll leave it in its present buggy form so that it will
> explode next time someone tries to use it for something new, rather than a)

It shouldn't be used for anything new. It's really a private macro
in the context switch code, nothing that any other code is supposed
to use.

> fixing that potential problem and b) fixing a real problem with a popular
> external GPLed product.

kgdb shouldn't need any patches to core kernel I had it some time ago running fine
just by hooking it into the die hooks (and minor changes to the serial layer,
with netpoll these shouldn't be even needed anymore) 

If the kgdb people need more changes they should submit them.

But I suspect if they change RESTORE_CONTEXT they're doing something wrong
anyways and they just need to fix their code properly.

-Andi

Re: [discuss] [2.6 patch] x86_64: re-add a newline to RESTORE_CONTEXT

[Jan Beulich]

>>> Andrew Morton <akpm@osdl.org> 09.01.07 23:04 >>>
>On Tue, 9 Jan 2007 12:01:21 +0100
>Andi Kleen <ak@suse.de> wrote:
>
>> On Tuesday 09 January 2007 03:55, Adrian Bunk wrote:
>> > RESTORE_CONTEXT lost a newline in 
>> > commit 658fdbef66e5e9be79b457edc2cbbb3add840aa9:
>> > http://www.mail-archive.com/kgdb-bugreport@lists.sourceforge.net/msg00559.html 
>> 
>> I don't think we should add such changes for external patchkits.
>> 
>> In general kgdb shouldn't add any patches at all. If the existing 
>> hooks are not enough they should submit their changes needed so
>> that it can just work.
>> 
>
>But the patch is a bugfix.  Without it, you cannot do
>
>	RESTORE_CONTEXT	\
>	.globl ...	\

Their use was broken in the first place - they shouldn't have made
assumptions about the contents of the macro, by writing this like

	RESTORE_CONTEXT "\n\t"	\
	".globl ..."	\

if they really need to make use of the macro. This is similar to
requirements of other (assembly) macros that normally also
don't have a line terminator and hence require the users to
add appropriate line termination after the macro name (and
eventual arguments).

I would even go as far as asking for removing the \n\t on SAVE_CONTEXT
and the left \t on RESTORE_CONTEXT.

Jan