RESTORE_CONTEXT lost a newline in
commit 658fdbef66e5e9be79b457edc2cbbb3add840aa9:
http://www.mail-archive.com/[email protected]/msg00559.html
Reported by Steven M. Christey.
Signed-off-by: Adrian Bunk <[email protected]>
--- 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"
On Tuesday 09 January 2007 03:55, Adrian Bunk wrote:
> RESTORE_CONTEXT lost a newline in
> commit 658fdbef66e5e9be79b457edc2cbbb3add840aa9:
> http://www.mail-archive.com/[email protected]/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
On Tue, 9 Jan 2007 12:01:21 +0100
Andi Kleen <[email protected]> wrote:
> On Tuesday 09 January 2007 03:55, Adrian Bunk wrote:
> > RESTORE_CONTEXT lost a newline in
> > commit 658fdbef66e5e9be79b457edc2cbbb3add840aa9:
> > http://www.mail-archive.com/[email protected]/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?
>
> 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
On Tue, 9 Jan 2007 23:43:00 +0100
Andi Kleen <[email protected]> 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.
>
> 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
>>> Andrew Morton <[email protected]> 09.01.07 23:04 >>>
>On Tue, 9 Jan 2007 12:01:21 +0100
>Andi Kleen <[email protected]> wrote:
>
>> On Tuesday 09 January 2007 03:55, Adrian Bunk wrote:
>> > RESTORE_CONTEXT lost a newline in
>> > commit 658fdbef66e5e9be79b457edc2cbbb3add840aa9:
>> > http://www.mail-archive.com/[email protected]/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