[patch] allow switching stacks

[patch] allow switching stacks

[Jim Houston]

Hi Everyone,

This patch against gdb-6.0 adds an option to disable an error check
which reports a non-contiguous stack as corrupted. I need this
option to get a reliable stack trace using kgdb on the x86-64 Linux
kernel because it uses a separate per-processor interrupt stack.

This option is enabled with:

     set backtrace switch-stacks on

Jim Houston - Concurrent Computer Corp.

-

diff -urN old/gdb-6.0/gdb/frame.c new/gdb-6.0/gdb/frame.c
--- old/gdb-6.0/gdb/frame.c	2003-12-31 15:27:45.866840920 -0500
+++ new/gdb-6.0/gdb/frame.c	2003-12-31 15:27:58.310949128 -0500
@@ -138,6 +138,7 @@
 /* Flag to indicate whether backtraces should stop at main et.al.  */
 
 static int backtrace_past_main;
+static int backtrace_switch_stacks;
 static unsigned int backtrace_limit = UINT_MAX;
 
 
@@ -1971,7 +1972,7 @@
      the next frame.  This happens when a frame unwind goes backwards.
      Since the sentinel frame doesn't really exist, don't compare the
      inner-most against that sentinel.  */
-  if (this_frame->level > 0
+  if (!backtrace_switch_stacks && this_frame->level > 0
       && frame_id_inner (get_frame_id (this_frame),
 			 get_frame_id (this_frame->next)))
     error ("Previous frame inner to this frame (corrupt stack?)");
@@ -2461,6 +2462,19 @@
 			   NULL, NULL, &set_backtrace_cmdlist,
 			   &show_backtrace_cmdlist);
 
+  add_setshow_boolean_cmd ("switch-stacks", class_obscure,
+			   &backtrace_switch_stacks, "\
+Set if thread may use multiple stacks.  This flag disables checks in\n\
+the stack trace which expect that the stack grew in a consistent direction.\n\
+This option is needed for kernel debug when the kernel has separate\n\
+process and interrupt stacks.", "\
+Show if thread may use multiple stacks.  This flag disables checks in\n\
+the stack trace which expect that the stack grew in a consistent direction.\n\
+This option is needed for kernel debug when the kernel has separate\n\
+process and interrupt stacks.",
+			   NULL, NULL, &set_backtrace_cmdlist,
+			   &show_backtrace_cmdlist);
+
   add_setshow_uinteger_cmd ("limit", class_obscure,
 			    &backtrace_limit, "\
 Set an upper bound on the number of backtrace levels.\n\

Re: [patch] allow switching stacks

[Michael Snyder]

Jim Houston wrote:
> Hi Everyone,
> 
> This patch against gdb-6.0 adds an option to disable an error check
> which reports a non-contiguous stack as corrupted. I need this
> option to get a reliable stack trace using kgdb on the x86-64 Linux
> kernel because it uses a separate per-processor interrupt stack.
> 
> This option is enabled with:
> 
>      set backtrace switch-stacks on

Good idea, and nice implementation.  My only suggestions are in
the user interface realm.  I'm thinking something like
"non-contiguous stacks" would be more meaningful than "multiple
stacks" (there's really still only one stack, it's just broken
into two segments).  Same goes for switch-stacks; it's a bit
non-intuitive to me.  segmented-stack?  discontiguous-stack?

> 
> diff -urN old/gdb-6.0/gdb/frame.c new/gdb-6.0/gdb/frame.c
> --- old/gdb-6.0/gdb/frame.c	2003-12-31 15:27:45.866840920 -0500
> +++ new/gdb-6.0/gdb/frame.c	2003-12-31 15:27:58.310949128 -0500
> @@ -138,6 +138,7 @@
>  /* Flag to indicate whether backtraces should stop at main et.al.  */
>  
>  static int backtrace_past_main;
> +static int backtrace_switch_stacks;
>  static unsigned int backtrace_limit = UINT_MAX;
>  
>  
> @@ -1971,7 +1972,7 @@
>       the next frame.  This happens when a frame unwind goes backwards.
>       Since the sentinel frame doesn't really exist, don't compare the
>       inner-most against that sentinel.  */
> -  if (this_frame->level > 0
> +  if (!backtrace_switch_stacks && this_frame->level > 0
>        && frame_id_inner (get_frame_id (this_frame),
>  			 get_frame_id (this_frame->next)))
>      error ("Previous frame inner to this frame (corrupt stack?)");
> @@ -2461,6 +2462,19 @@
>  			   NULL, NULL, &set_backtrace_cmdlist,
>  			   &show_backtrace_cmdlist);
>  
> +  add_setshow_boolean_cmd ("switch-stacks", class_obscure,
> +			   &backtrace_switch_stacks, "\
> +Set if thread may use multiple stacks.  This flag disables checks in\n\
> +the stack trace which expect that the stack grew in a consistent direction.\n\
> +This option is needed for kernel debug when the kernel has separate\n\
> +process and interrupt stacks.", "\
> +Show if thread may use multiple stacks.  This flag disables checks in\n\
> +the stack trace which expect that the stack grew in a consistent direction.\n\
> +This option is needed for kernel debug when the kernel has separate\n\
> +process and interrupt stacks.",
> +			   NULL, NULL, &set_backtrace_cmdlist,
> +			   &show_backtrace_cmdlist);
> +
>    add_setshow_uinteger_cmd ("limit", class_obscure,
>  			    &backtrace_limit, "\
>  Set an upper bound on the number of backtrace levels.\n\
> 

Re: [patch] allow switching stacks

[Michael Snyder]

Jim Houston wrote:
> Hi Everyone,
> 
> This patch against gdb-6.0 adds an option to disable an error check
> which reports a non-contiguous stack as corrupted. I need this
> option to get a reliable stack trace using kgdb on the x86-64 Linux
> kernel because it uses a separate per-processor interrupt stack.
> 
> This option is enabled with:
> 
>      set backtrace switch-stacks on

Good idea, and nice implementation.  My only suggestions are in
the user interface realm.  I'm thinking something like
"non-contiguous stacks" would be more meaningful than "multiple
stacks" (there's really still only one stack, it's just broken
into two segments).  Same goes for switch-stacks; it's a bit
non-intuitive to me.  segmented-stack?  discontiguous-stack?

> 
> diff -urN old/gdb-6.0/gdb/frame.c new/gdb-6.0/gdb/frame.c
> --- old/gdb-6.0/gdb/frame.c	2003-12-31 15:27:45.866840920 -0500
> +++ new/gdb-6.0/gdb/frame.c	2003-12-31 15:27:58.310949128 -0500
> @@ -138,6 +138,7 @@
>  /* Flag to indicate whether backtraces should stop at main et.al.  */
>  
>  static int backtrace_past_main;
> +static int backtrace_switch_stacks;
>  static unsigned int backtrace_limit = UINT_MAX;
>  
>  
> @@ -1971,7 +1972,7 @@
>       the next frame.  This happens when a frame unwind goes backwards.
>       Since the sentinel frame doesn't really exist, don't compare the
>       inner-most against that sentinel.  */
> -  if (this_frame->level > 0
> +  if (!backtrace_switch_stacks && this_frame->level > 0
>        && frame_id_inner (get_frame_id (this_frame),
>  			 get_frame_id (this_frame->next)))
>      error ("Previous frame inner to this frame (corrupt stack?)");
> @@ -2461,6 +2462,19 @@
>  			   NULL, NULL, &set_backtrace_cmdlist,
>  			   &show_backtrace_cmdlist);
>  
> +  add_setshow_boolean_cmd ("switch-stacks", class_obscure,
> +			   &backtrace_switch_stacks, "\
> +Set if thread may use multiple stacks.  This flag disables checks in\n\
> +the stack trace which expect that the stack grew in a consistent direction.\n\
> +This option is needed for kernel debug when the kernel has separate\n\
> +process and interrupt stacks.", "\
> +Show if thread may use multiple stacks.  This flag disables checks in\n\
> +the stack trace which expect that the stack grew in a consistent direction.\n\
> +This option is needed for kernel debug when the kernel has separate\n\
> +process and interrupt stacks.",
> +			   NULL, NULL, &set_backtrace_cmdlist,
> +			   &show_backtrace_cmdlist);
> +
>    add_setshow_uinteger_cmd ("limit", class_obscure,
>  			    &backtrace_limit, "\
>  Set an upper bound on the number of backtrace levels.\n\
> 

Re: [patch] allow switching stacks

[Eli Zaretskii]

> Date: Mon, 08 Mar 2004 22:58:29 +0000
> From: Michael Snyder <msnyder@redhat.com>
> 
> Good idea, and nice implementation.  My only suggestions are in
> the user interface realm.  I'm thinking something like
> "non-contiguous stacks" would be more meaningful than "multiple
> stacks" (there's really still only one stack, it's just broken
> into two segments).  Same goes for switch-stacks; it's a bit
> non-intuitive to me.  segmented-stack?  discontiguous-stack?

I share Muchael's concerns (I'd use "non-contiguous stack" in both
instances).  On top of that, if this patch is approved, it needs a
corresponding patch for the manual, since we'd be introducing a new
command.

Re: [patch] allow switching stacks

[Eli Zaretskii]

> Date: Mon, 08 Mar 2004 22:58:29 +0000
> From: Michael Snyder <msnyder@redhat.com>
> 
> Good idea, and nice implementation.  My only suggestions are in
> the user interface realm.  I'm thinking something like
> "non-contiguous stacks" would be more meaningful than "multiple
> stacks" (there's really still only one stack, it's just broken
> into two segments).  Same goes for switch-stacks; it's a bit
> non-intuitive to me.  segmented-stack?  discontiguous-stack?

I share Muchael's concerns (I'd use "non-contiguous stack" in both
instances).  On top of that, if this patch is approved, it needs a
corresponding patch for the manual, since we'd be introducing a new
command.

Re: [patch] allow switching stacks

[Andrew Cagney]

> Hi Everyone,
> 
> This patch against gdb-6.0 adds an option to disable an error check
> which reports a non-contiguous stack as corrupted. I need this
> option to get a reliable stack trace using kgdb on the x86-64 Linux
> kernel because it uses a separate per-processor interrupt stack.
> 
> This option is enabled with:
> 
>      set backtrace switch-stacks on
> 
> Jim Houston - Concurrent Computer Corp.

Can you provide more details of the problem -- exactly what do things 
look like at the stack switch?

Your problem sounds very similar to the combination of a signal altstack 
and a signal trampoline - for such a situtation the stack direction 
check isn't applicable (pointing to the real bug).

Andrew

Re: [patch] allow switching stacks

[Andrew Cagney]

> Hi Everyone,
> 
> This patch against gdb-6.0 adds an option to disable an error check
> which reports a non-contiguous stack as corrupted. I need this
> option to get a reliable stack trace using kgdb on the x86-64 Linux
> kernel because it uses a separate per-processor interrupt stack.
> 
> This option is enabled with:
> 
>      set backtrace switch-stacks on
> 
> Jim Houston - Concurrent Computer Corp.

Can you provide more details of the problem -- exactly what do things 
look like at the stack switch?

Your problem sounds very similar to the combination of a signal altstack 
and a signal trampoline - for such a situtation the stack direction 
check isn't applicable (pointing to the real bug).

Andrew

Re: [patch] allow switching stacks

[Andrew Cagney]

> Your problem sounds very similar to the combination of a signal altstack and a signal trampoline - for such a situtation the stack direction check isn't applicable (pointing to the real bug).

I've just committed a patch to fix a bug where GDB wouldn't backtrace 
through sigaltstack signal handlers.

Andrew

Re: [patch] allow switching stacks

[Daniel Jacobowitz]

On Tue, Mar 09, 2004 at 03:21:25PM -0500, Andrew Cagney wrote:
> >Hi Everyone,
> >
> >This patch against gdb-6.0 adds an option to disable an error check
> >which reports a non-contiguous stack as corrupted. I need this
> >option to get a reliable stack trace using kgdb on the x86-64 Linux
> >kernel because it uses a separate per-processor interrupt stack.
> >
> >This option is enabled with:
> >
> >     set backtrace switch-stacks on
> >
> >Jim Houston - Concurrent Computer Corp.
> 
> Can you provide more details of the problem -- exactly what do things 
> look like at the stack switch?
> 
> Your problem sounds very similar to the combination of a signal altstack 
> and a signal trampoline - for such a situtation the stack direction 
> check isn't applicable (pointing to the real bug).

They're going to look like normal frames unless GDB has magic to
recognize the interrupt handler, which doesn't seem like a good idea to
me - we can just use CFI to unwind through these things, otherwise.

There's no way in unwind information to annotate a non-call frame,
which is the common relevant property for signal frames, dummy frames,
and interrupt handler frames.   Should there be?

The other question is whether the order check is really safe at all.  I
can write (un-portable, using assembly, sure) code to call a function
on an alternate stack.  It would be nice to be able to backtrace
through something like that, if practical.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch] allow switching stacks

[Jim Blandy]

Daniel Jacobowitz <drow@false.org> writes:
> There's no way in unwind information to annotate a non-call frame,
> which is the common relevant property for signal frames, dummy frames,
> and interrupt handler frames.   Should there be?

There isn't?  For signal frames, for example, the registers are all
saved in the ucontext_t, on the signal handler's stack.  Can't we
hand-build Dwarf 2 CFI that points into the ucontext_t?

On IA-32 machines, the processor saves %eflags, %eip, and %cs, and
possibly %esp and %ss, and it's up to the handler code to save
whatever else needs preserving.  We could hand-build Dwarf 2 CFI for
that, too.

Re: [patch] allow switching stacks

[Daniel Jacobowitz]

On Mon, Apr 12, 2004 at 04:02:37PM -0500, Jim Blandy wrote:
> 
> Daniel Jacobowitz <drow@false.org> writes:
> > There's no way in unwind information to annotate a non-call frame,
> > which is the common relevant property for signal frames, dummy frames,
> > and interrupt handler frames.   Should there be?
> 
> There isn't?  For signal frames, for example, the registers are all
> saved in the ucontext_t, on the signal handler's stack.  Can't we
> hand-build Dwarf 2 CFI that points into the ucontext_t?
> 
> On IA-32 machines, the processor saves %eflags, %eip, and %cs, and
> possibly %esp and %ss, and it's up to the handler code to save
> whatever else needs preserving.  We could hand-build Dwarf 2 CFI for
> that, too.

That's not what I meant.  In fact, if my memory serves me right, the
vsyscall DSO support that Roland is still submitting may let us see
CFI for the signal trampoline, courtesy of the kernel.

But there's no flag in the CFI that says 'this is a non-call frame'. 
So anything that assumes it is dealing with call frames will break if
we let the dwarf2 unwinder take precedence over the sigframe unwinder:
  - we won't get <signal frame> messages, which is mostly cosmetic,
    but not entirely - think unwindonsignal
  - frame_address_in_block will try to subtract from the saved PC
  - the inner-than check in this thread will be confused
  - et cetera.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

[patch] allow switching stacks

[Jim Houston]

Hi Everyone,

This patch against gdb-6.0 adds an option to disable an error check
which reports a non-contiguous stack as corrupted. I need this
option to get a reliable stack trace using kgdb on the x86-64 Linux
kernel because it uses a separate per-processor interrupt stack.

This option is enabled with:

     set backtrace switch-stacks on

Jim Houston - Concurrent Computer Corp.

-

diff -urN old/gdb-6.0/gdb/frame.c new/gdb-6.0/gdb/frame.c
--- old/gdb-6.0/gdb/frame.c	2003-12-31 15:27:45.866840920 -0500
+++ new/gdb-6.0/gdb/frame.c	2003-12-31 15:27:58.310949128 -0500
@@ -138,6 +138,7 @@
 /* Flag to indicate whether backtraces should stop at main et.al.  */
 
 static int backtrace_past_main;
+static int backtrace_switch_stacks;
 static unsigned int backtrace_limit = UINT_MAX;
 
 
@@ -1971,7 +1972,7 @@
      the next frame.  This happens when a frame unwind goes backwards.
      Since the sentinel frame doesn't really exist, don't compare the
      inner-most against that sentinel.  */
-  if (this_frame->level > 0
+  if (!backtrace_switch_stacks && this_frame->level > 0
       && frame_id_inner (get_frame_id (this_frame),
 			 get_frame_id (this_frame->next)))
     error ("Previous frame inner to this frame (corrupt stack?)");
@@ -2461,6 +2462,19 @@
 			   NULL, NULL, &set_backtrace_cmdlist,
 			   &show_backtrace_cmdlist);
 
+  add_setshow_boolean_cmd ("switch-stacks", class_obscure,
+			   &backtrace_switch_stacks, "\
+Set if thread may use multiple stacks.  This flag disables checks in\n\
+the stack trace which expect that the stack grew in a consistent direction.\n\
+This option is needed for kernel debug when the kernel has separate\n\
+process and interrupt stacks.", "\
+Show if thread may use multiple stacks.  This flag disables checks in\n\
+the stack trace which expect that the stack grew in a consistent direction.\n\
+This option is needed for kernel debug when the kernel has separate\n\
+process and interrupt stacks.",
+			   NULL, NULL, &set_backtrace_cmdlist,
+			   &show_backtrace_cmdlist);
+
   add_setshow_uinteger_cmd ("limit", class_obscure,
 			    &backtrace_limit, "\
 Set an upper bound on the number of backtrace levels.\n\