Fix a crash when stepping and unwinding fails

Fix a crash when stepping and unwinding fails

[Daniel Jacobowitz]

This patch stops GDB from segfaulting when we step into a function,
but can not unwind back out of the function.  We would previously
call get_prev_frame, which would return NULL, and then try to
get_frame_pc (NULL).

Now we'll issue this error instead, and stop stepping:

Could not step out of the function at 0x80144400 - unwinding failed

It's still not great, but at least it's an improvement over crashing.
It is reasonably likely that we've just stepped over a standard
function call, and that consequentially the function return
address is in the standard place for the architecture; in fact,
GDB used to have a hook for this, before the frame overhaul:
SAVED_PC_AFTER_CALL.  But it's gone now and there's no easy analogue,
and it was never 100% reliable anyway.  So unfortunately, if we
single-step out to an address that we can't find a way to unwind from,
we'll stop instead of stepping out.

Hmm.  Alternatively, we could stop stepping without an error.  Would
that be better?  Seems likely.  I'll wait for comments before I try
implementing that, though.  Should we warn when we do that, in addition
to stopping, or is the warning just noise?

Shaun, I believe this is the crash you reported on gdb@ several times.
I can't think of any easy way to write a test for this.

-- 
Daniel Jacobowitz
CodeSourcery

2006-02-20  Daniel Jacobowitz  <dan@codesourcery.com>

	* infrun.c (insert_step_resume_breakpoint_at_frame): Add
	USE_PREVIOUS argument.  Issue an error if we can not unwind
	to the previous frame.
	(handle_inferior_event): Update calls.

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2006-01-04 14:34:58.000000000 -0500
+++ src/gdb/infrun.c	2006-02-20 15:59:18.000000000 -0500
@@ -942,7 +942,7 @@ void init_execution_control_state (struc
 void handle_inferior_event (struct execution_control_state *ecs);
 
 static void step_into_function (struct execution_control_state *ecs);
-static void insert_step_resume_breakpoint_at_frame (struct frame_info *step_frame);
+static void insert_step_resume_breakpoint_at_frame (struct frame_info *, int);
 static void insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
 						  struct frame_id sr_id);
 static void stop_stepping (struct execution_control_state *ecs);
@@ -1965,7 +1965,7 @@ process_event_stop_test:
 	     code paths as single-step - set a breakpoint at the
 	     signal return address and then, once hit, step off that
 	     breakpoint.  */
-	  insert_step_resume_breakpoint_at_frame (get_current_frame ());
+	  insert_step_resume_breakpoint_at_frame (get_current_frame (), 0);
 	  ecs->step_after_step_resume_breakpoint = 1;
 	  keep_going (ecs);
 	  return;
@@ -1987,7 +1987,7 @@ process_event_stop_test:
 	     Note that this is only needed for a signal delivered
 	     while in the single-step range.  Nested signals aren't a
 	     problem as they eventually all return.  */
-	  insert_step_resume_breakpoint_at_frame (get_current_frame ());
+	  insert_step_resume_breakpoint_at_frame (get_current_frame (), 0);
 	  keep_going (ecs);
 	  return;
 	}
@@ -2396,7 +2396,7 @@ process_event_stop_test:
 	  /* We're doing a "next", set a breakpoint at callee's return
 	     address (the address at which the caller will
 	     resume).  */
-	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+	  insert_step_resume_breakpoint_at_frame (get_current_frame (), 1);
 	  keep_going (ecs);
 	  return;
 	}
@@ -2459,7 +2459,7 @@ process_event_stop_test:
 
       /* Set a breakpoint at callee's return address (the address at
          which the caller will resume).  */
-      insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+      insert_step_resume_breakpoint_at_frame (get_current_frame (), 1);
       keep_going (ecs);
       return;
     }
@@ -2528,7 +2528,7 @@ process_event_stop_test:
 	{
 	  /* Set a breakpoint at callee's return address (the address
 	     at which the caller will resume).  */
-	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+	  insert_step_resume_breakpoint_at_frame (get_current_frame (), 1);
 	  keep_going (ecs);
 	  return;
 	}
@@ -2741,22 +2741,33 @@ insert_step_resume_breakpoint_at_sal (st
    that the function/signal handler being skipped eventually returns
    to the breakpoint inserted at RETURN_FRAME.pc.
 
-   For the skip-function case, the function may have been reached by
-   either single stepping a call / return / signal-return instruction,
-   or by hitting a breakpoint.  In all cases, the RETURN_FRAME belongs
-   to the skip-function's caller.
+   If USE_PREVIOUS is zero, RETURN_FRAME belongs to the function being
+   skipped.  The function may have been reached by either single
+   stepping a call / return / signal-return instruction, or by hitting
+   a breakpoint.
 
    For the signals case, this is called with the interrupted
    function's frame.  The signal handler, when it returns, will resume
    the interrupted function at RETURN_FRAME.pc.  */
 
 static void
-insert_step_resume_breakpoint_at_frame (struct frame_info *return_frame)
+insert_step_resume_breakpoint_at_frame (struct frame_info *return_frame,
+					int use_previous)
 {
   struct symtab_and_line sr_sal;
 
   init_sal (&sr_sal);		/* initialize to zeros */
 
+  if (use_previous)
+    {
+      struct frame_info *caller_frame;
+      caller_frame = get_prev_frame (return_frame);
+      if (caller_frame == NULL)
+	error (_("Could not step out of the function at 0x%x - unwinding failed"),
+	       get_frame_pc (return_frame));
+      return_frame = caller_frame;
+    }
+
   sr_sal.pc = ADDR_BITS_REMOVE (get_frame_pc (return_frame));
   sr_sal.section = find_pc_overlay (sr_sal.pc);
 

Re: Fix a crash when stepping and unwinding fails

[Eli Zaretskii]

> Date: Mon, 20 Feb 2006 17:03:31 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> Now we'll issue this error instead, and stop stepping:
> 
> Could not step out of the function at 0x80144400 - unwinding failed
> [...]
> Hmm.  Alternatively, we could stop stepping without an error.  Would
> that be better?  Seems likely.  I'll wait for comments before I try
> implementing that, though.  Should we warn when we do that, in addition
> to stopping, or is the warning just noise?

How about making it a warning that depends on "set verbose"?

Btw, I think we should document this message in the manual anyway.

Re: Fix a crash when stepping and unwinding fails

[Mark Kettenis]

> Date: Mon, 20 Feb 2006 17:03:31 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> This patch stops GDB from segfaulting when we step into a function,
> but can not unwind back out of the function.  We would previously
> call get_prev_frame, which would return NULL, and then try to
> get_frame_pc (NULL).
> 
> Now we'll issue this error instead, and stop stepping:
> 
> Could not step out of the function at 0x80144400 - unwinding failed
> 
> It's still not great, but at least it's an improvement over crashing.
> It is reasonably likely that we've just stepped over a standard
> function call, and that consequentially the function return
> address is in the standard place for the architecture; in fact,
> GDB used to have a hook for this, before the frame overhaul:
> SAVED_PC_AFTER_CALL.  But it's gone now and there's no easy analogue,
> and it was never 100% reliable anyway.  So unfortunately, if we
> single-step out to an address that we can't find a way to unwind from,
> we'll stop instead of stepping out.

How can this happen?  Both affected calls to
insert_step_resume_breakpoint_at_frame() are in the same

  if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))
    {

block.  Assuming that step_frame_id isn't equal to null_frame_id, this
means that we *can* unwind.  Seems like the problem is that the code
uses get_prev_frame(), which can return NULL, even if we could unwind,
for example when we try to unwind past main.  Looks to me the real bug
here is that we're using get_prev_frame().  The right solution is
probably to use frame_pc_unwind(), and insert a the step resume
breakpoint there.  That should never fail.

This would probably demand us to introduce
insert_step_resume_breakpoint_at_pc(), and we could probably eliminate
insert_step_resume_breakpoint_at_frame() altogether.  An alternative
would be to export get_prev_name_1() from frame.c (giving it a more
useful name).

I must say, I don't really liked the way you changed the
insert_step_resume_breakpoint_at_frame() interface.  That extra
USE_PREVIOUS argument is really awkward, and made the function name
rather non-sensible.

Mark

Re: Fix a crash when stepping and unwinding fails

[Daniel Jacobowitz]

On Tue, Feb 21, 2006 at 09:15:16PM +0100, Mark Kettenis wrote:
> > It's still not great, but at least it's an improvement over crashing.
> > It is reasonably likely that we've just stepped over a standard
> > function call, and that consequentially the function return
> > address is in the standard place for the architecture; in fact,
> > GDB used to have a hook for this, before the frame overhaul:
> > SAVED_PC_AFTER_CALL.  But it's gone now and there's no easy analogue,
> > and it was never 100% reliable anyway.  So unfortunately, if we
> > single-step out to an address that we can't find a way to unwind from,
> > we'll stop instead of stepping out.
> 
> How can this happen?  Both affected calls to
> insert_step_resume_breakpoint_at_frame() are in the same
> 
>   if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))
>     {
> 
> block.  Assuming that step_frame_id isn't equal to null_frame_id, this
> means that we *can* unwind.

There's your problem: you're assuming that step_frame_id isn't equal to
null_frame_id.  But in fact it is.  If we can't unwind past the current
frame, then that means the last frame sniffer (generally the prologue
analyzer), which is required to accept any frame given to it, could not
make heads or tails of it.  Which in turn means it doesn't know what
the frame's ID is, so it gets left as invalid.  Which means the current
frame will have an ID of null_frame_id.

That's what's happening to me, although I seem to recall something
similar could be produced by stepping across main without debug info.

> Seems like the problem is that the code
> uses get_prev_frame(), which can return NULL, even if we could unwind,
> for example when we try to unwind past main.  Looks to me the real bug
> here is that we're using get_prev_frame().  The right solution is
> probably to use frame_pc_unwind(), and insert a the step resume
> breakpoint there.  That should never fail.
> 
> This would probably demand us to introduce
> insert_step_resume_breakpoint_at_pc(), and we could probably eliminate
> insert_step_resume_breakpoint_at_frame() altogether.  An alternative
> would be to export get_prev_name_1() from frame.c (giving it a more
> useful name).

That seems like a good change indeed, but probably wouldn't fix this
problem.

Hmm, what does frame_pc_unwind do when we've hit the last frame?  I'm
not sure it's meaningful.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix a crash when stepping and unwinding fails

[Mark Kettenis]

> Date: Tue, 21 Feb 2006 15:28:33 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Tue, Feb 21, 2006 at 09:15:16PM +0100, Mark Kettenis wrote:
> > > It's still not great, but at least it's an improvement over crashing.
> > > It is reasonably likely that we've just stepped over a standard
> > > function call, and that consequentially the function return
> > > address is in the standard place for the architecture; in fact,
> > > GDB used to have a hook for this, before the frame overhaul:
> > > SAVED_PC_AFTER_CALL.  But it's gone now and there's no easy analogue,
> > > and it was never 100% reliable anyway.  So unfortunately, if we
> > > single-step out to an address that we can't find a way to unwind from,
> > > we'll stop instead of stepping out.
> > 
> > How can this happen?  Both affected calls to
> > insert_step_resume_breakpoint_at_frame() are in the same
> > 
> >   if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))
> >     {
> > 
> > block.  Assuming that step_frame_id isn't equal to null_frame_id, this
> > means that we *can* unwind.
> 
> There's your problem: you're assuming that step_frame_id isn't equal to
> null_frame_id.  But in fact it is.

But if step_frame_id is equal to null_frame_id, we shouldn't be trying
to insert step-resume-breakpoints.  It means that step_frame_id is
still uninitialized, since step_frame_id is initialized by:

  step_frame_id = get_frame_id (get_current_frame ());

(or equivalent code), and unwinding from sentinel frame shoud always
yield a frame ID that's different from null_frame_id.

> If we can't unwind past the current frame, then that means the last
> frame sniffer (generally the prologue analyzer), which is required
> to accept any frame given to it, could not make heads or tails of
> it.  Which in turn means it doesn't know what the frame's ID is, so
> it gets left as invalid.  Which means the current frame will have an
> ID of null_frame_id.
> 
> That's what's happening to me, although I seem to recall something
> similar could be produced by stepping across main without debug info.

I think it can happen if you're trying to step "over" main from within
the C runtime code.  But in that case the frame ID won't be
null_frame_id.

> That seems like a good change indeed, but probably wouldn't fix this
> problem.
> 
> Hmm, what does frame_pc_unwind do when we've hit the last frame?  I'm
> not sure it's meaningful.

How can we hit the last frame?  If we're hitting the last frame, where
did we come from?

It may very well be that there are GDB bugs that make step_frame_id
equal to null_frame_id.  If we can't trace those bugs right now, we
should probably sprinkle a few gdb_assert()'s around and try to solve
the issues when we hit those.

Mark

Re: Fix a crash when stepping and unwinding fails

[Daniel Jacobowitz]

On Tue, Feb 21, 2006 at 09:50:58PM +0100, Mark Kettenis wrote:
> But if step_frame_id is equal to null_frame_id, we shouldn't be trying
> to insert step-resume-breakpoints.  It means that step_frame_id is
> still uninitialized, since step_frame_id is initialized by:
> 
>   step_frame_id = get_frame_id (get_current_frame ());
> 
> (or equivalent code), and unwinding from sentinel frame shoud always
> yield a frame ID that's different from null_frame_id.

It's this assumption I don't think is right.  I have plenty of
anecdotal evidence from yesterday that it's not right, in fact.
If the prologue analyzer can't handle the code at $pc, then
what do you expect it to put into the frame ID?  Or if it thinks we
are in the outermost frame?

> > That seems like a good change indeed, but probably wouldn't fix this
> > problem.
> > 
> > Hmm, what does frame_pc_unwind do when we've hit the last frame?  I'm
> > not sure it's meaningful.
> 
> How can we hit the last frame?  If we're hitting the last frame, where
> did we come from?
> 
> It may very well be that there are GDB bugs that make step_frame_id
> equal to null_frame_id.  If we can't trace those bugs right now, we
> should probably sprinkle a few gdb_assert()'s around and try to solve
> the issues when we hit those.

We use the null frame ID to represent the outermost frame.  If we can't
find another frame outer to this one, then we assume this one is the
outermost.

Just to sketch out my example a bit more: the embedded OS I'm debugging
lives in ROM.  The application I've supplied to GDB lives in RAM.  In
some later stage of the project, hopefully, I will have GDB magically
load some other ELF files (that I don't have yet) to cover the ROM
code; but right now I can't do that and there's no guarantee I'll have
debug info covering all of it anyway.  So we're executing code way
out in the boondocks.  GDB doesn't have any way on this platform
(ARM Thumb) to guess where the start of a function is if it doesn't
have a symbol table; so it can't be sure that we've really reached the
first instruction of a function, so it has no idea whether $lr is valid
or not.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix a crash when stepping and unwinding fails

[Mark Kettenis]

> Date: Tue, 21 Feb 2006 15:57:48 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> On Tue, Feb 21, 2006 at 09:50:58PM +0100, Mark Kettenis wrote:
> > But if step_frame_id is equal to null_frame_id, we shouldn't be trying
> > to insert step-resume-breakpoints.  It means that step_frame_id is
> > still uninitialized, since step_frame_id is initialized by:
> > 
> >   step_frame_id = get_frame_id (get_current_frame ());
> > 
> > (or equivalent code), and unwinding from sentinel frame shoud always
> > yield a frame ID that's different from null_frame_id.
> 
> It's this assumption I don't think is right.  I have plenty of
> anecdotal evidence from yesterday that it's not right, in fact.
> If the prologue analyzer can't handle the code at $pc, then
> what do you expect it to put into the frame ID?  Or if it thinks we
> are in the outermost frame?

But get_current_frame() should be the innermost frame when we execute
this code.  So the prologue analyzer can't be involved here.  However,
yes, it seems that step_frame_idd can end up as null_frame_id, if
get_current_frame() is also the outermost frame at the same time.

> > > That seems like a good change indeed, but probably wouldn't fix this
> > > problem.
> > > 
> > > Hmm, what does frame_pc_unwind do when we've hit the last frame?  I'm
> > > not sure it's meaningful.
> > 
> > How can we hit the last frame?  If we're hitting the last frame, where
> > did we come from?
> > 
> > It may very well be that there are GDB bugs that make step_frame_id
> > equal to null_frame_id.  If we can't trace those bugs right now, we
> > should probably sprinkle a few gdb_assert()'s around and try to solve
> > the issues when we hit those.
> 
> We use the null frame ID to represent the outermost frame.  If we can't
> find another frame outer to this one, then we assume this one is the
> outermost.

Yes, it seems there are issues here.  The frame ID is supposed to be
unique for a particular frame, yet there's a possibility that two
distinct frames both end up with the null frame ID.

> Just to sketch out my example a bit more: the embedded OS I'm debugging
> lives in ROM.  The application I've supplied to GDB lives in RAM.  In
> some later stage of the project, hopefully, I will have GDB magically
> load some other ELF files (that I don't have yet) to cover the ROM
> code; but right now I can't do that and there's no guarantee I'll have
> debug info covering all of it anyway.  So we're executing code way
> out in the boondocks.  GDB doesn't have any way on this platform
> (ARM Thumb) to guess where the start of a function is if it doesn't
> have a symbol table; so it can't be sure that we've really reached the
> first instruction of a function, so it has no idea whether $lr is valid
> or not.

But that really means that we shouldn't be messing with step-resume
breakpoints here.  The whole notion of functions that can be stepped
into isn't there.

Mark

Re: Fix a crash when stepping and unwinding fails

[Daniel Jacobowitz]

On Tue, Feb 21, 2006 at 10:34:03PM +0100, Mark Kettenis wrote:
> > It's this assumption I don't think is right.  I have plenty of
> > anecdotal evidence from yesterday that it's not right, in fact.
> > If the prologue analyzer can't handle the code at $pc, then
> > what do you expect it to put into the frame ID?  Or if it thinks we
> > are in the outermost frame?
> 
> But get_current_frame() should be the innermost frame when we execute
> this code.  So the prologue analyzer can't be involved here.  However,

Remember the off-by-one distinction between prev_register and this_id?

The sentinel frame and its unwinder provide the current frame's
registers via their prev_register method.  But the current frame's
unwinder is needed to compute the current frame's ID, and that's the
prologue analyzer in this case...

> yes, it seems that step_frame_idd can end up as null_frame_id, if
> get_current_frame() is also the outermost frame at the same time.

... so the outermost frame will generally have null_frame_id.  Not sure
if that's true when we stop because we encounter main, but it's
definitely true when we run out of unwindable frames.

> > > How can we hit the last frame?  If we're hitting the last frame, where
> > > did we come from?
> > > 
> > > It may very well be that there are GDB bugs that make step_frame_id
> > > equal to null_frame_id.  If we can't trace those bugs right now, we
> > > should probably sprinkle a few gdb_assert()'s around and try to solve
> > > the issues when we hit those.
> > 
> > We use the null frame ID to represent the outermost frame.  If we can't
> > find another frame outer to this one, then we assume this one is the
> > outermost.
> 
> Yes, it seems there are issues here.  The frame ID is supposed to be
> unique for a particular frame, yet there's a possibility that two
> distinct frames both end up with the null frame ID.

Are there?  I think there's only one - the outermost.  We've only got
that and the sentinel frame, and the sentinel frame I think doesn't
have an ID.

Perhaps part of the problem here is that step_frame_id is set to
null_frame_id when it is invalid; maybe we should keep that separate.
I don't think it would help me though.  Perhaps the real problem is the
use of null_frame_id for both the outermost frame and completely
unknown frames.  It would be nice if we could tell here:

  if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))

that frame_unwind_id has returned something completely invalid instead
of the outermost frame.

One way to do that in our current representation would be to check that
the frame ID for the current frame is not null_ptid.

> > Just to sketch out my example a bit more: the embedded OS I'm debugging
> > lives in ROM.  The application I've supplied to GDB lives in RAM.  In
> > some later stage of the project, hopefully, I will have GDB magically
> > load some other ELF files (that I don't have yet) to cover the ROM
> > code; but right now I can't do that and there's no guarantee I'll have
> > debug info covering all of it anyway.  So we're executing code way
> > out in the boondocks.  GDB doesn't have any way on this platform
> > (ARM Thumb) to guess where the start of a function is if it doesn't
> > have a symbol table; so it can't be sure that we've really reached the
> > first instruction of a function, so it has no idea whether $lr is valid
> > or not.
> 
> But that really means that we shouldn't be messing with step-resume
> breakpoints here.  The whole notion of functions that can be stepped
> into isn't there.

Yes, it is.  I've executed "step" in a place where I do have symbol
information (and working unwinders).  It's taken me into a place where
I don't (a DLL in ROM).  Since I don't have debug information any more
GDB would like to step back out to the call site, except it fails
because we've moved out of its known area.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix a crash when stepping and unwinding fails

[Jim Blandy]

On 2/21/06, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> But get_current_frame() should be the innermost frame when we execute
> this code.  So the prologue analyzer can't be involved here.  However,
> yes, it seems that step_frame_idd can end up as null_frame_id, if
> get_current_frame() is also the outermost frame at the same time.

An alternative explanation to Daniel's: the base address in the ID of
frame #0 needs to be the base of the stack frame, so that it won't
change as the function executes and does whatever pushes and pops it
has in mind.  The only way to find the base of the stack frame is to
do prologue analysis or consult CFI.

Re: Fix a crash when stepping and unwinding fails

[Mark Kettenis]

> Date: Tue, 21 Feb 2006 16:52:16 -0500
> From: Daniel Jacobowitz <drow@false.org>
> 
> > yes, it seems that step_frame_idd can end up as null_frame_id, if
> > get_current_frame() is also the outermost frame at the same time.
> 
> ... so the outermost frame will generally have null_frame_id.  Not sure
> if that's true when we stop because we encounter main

If we can unwind from main, its frame ID will not be null_frame_id.

> > > > How can we hit the last frame?  If we're hitting the last frame, where
> > > > did we come from?
> > > > 
> > > > It may very well be that there are GDB bugs that make step_frame_id
> > > > equal to null_frame_id.  If we can't trace those bugs right now, we
> > > > should probably sprinkle a few gdb_assert()'s around and try to solve
> > > > the issues when we hit those.
> > > 
> > > We use the null frame ID to represent the outermost frame.  If we can't
> > > find another frame outer to this one, then we assume this one is the
> > > outermost.
> > 
> > Yes, it seems there are issues here.  The frame ID is supposed to be
> > unique for a particular frame, yet there's a possibility that two
> > distinct frames both end up with the null frame ID.
> 
> Are there?  I think there's only one - the outermost.  We've only got
> that and the sentinel frame, and the sentinel frame I think doesn't
> have an ID.

If we can't unwind from a frame for some reason, then its frame ID
will also be null_frame_id.  And in multi-threaded programs there can
be multiple outermost frames.

> Perhaps part of the problem here is that step_frame_id is set to
> null_frame_id when it is invalid; maybe we should keep that separate.

Not sure; it might not be necessary if we generate a proper frame ID
for the outermost frame.

> I don't think it would help me though.  Perhaps the real problem is the
> use of null_frame_id for both the outermost frame and completely
> unknown frames.  It would be nice if we could tell here:
> 
>   if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))
> 
> that frame_unwind_id has returned something completely invalid instead
> of the outermost frame

Indeed.

> One way to do that in our current representation would be to check that
> the frame ID for the current frame is not null_ptid.

Yes, I think it makes sense to punt trying to insert a step-resume
breakpoint earlier than you do in your patch.

> > > Just to sketch out my example a bit more: the embedded OS I'm debugging
> > > lives in ROM.  The application I've supplied to GDB lives in RAM.  In
> > > some later stage of the project, hopefully, I will have GDB magically
> > > load some other ELF files (that I don't have yet) to cover the ROM
> > > code; but right now I can't do that and there's no guarantee I'll have
> > > debug info covering all of it anyway.  So we're executing code way
> > > out in the boondocks.  GDB doesn't have any way on this platform
> > > (ARM Thumb) to guess where the start of a function is if it doesn't
> > > have a symbol table; so it can't be sure that we've really reached the
> > > first instruction of a function, so it has no idea whether $lr is valid
> > > or not.
> > 
> > But that really means that we shouldn't be messing with step-resume
> > breakpoints here.  The whole notion of functions that can be stepped
> > into isn't there.
> 
> Yes, it is.  I've executed "step" in a place where I do have symbol
> information (and working unwinders).  It's taken me into a place where
> I don't (a DLL in ROM).  Since I don't have debug information any more
> GDB would like to step back out to the call site, except it fails
> because we've moved out of its known area.

Ah, ok.  But that means that step_frame_id really shouldn't be equal
to null_frame_id.  It will certainly not be null_frame_id if it isn't
the outermost frame.  And whether the place where you issued "step" is
the outermost frame or not should not influence GDB's behaviour here.

Mark

Re: Fix a crash when stepping and unwinding fails

[Daniel Jacobowitz]

On Wed, Feb 22, 2006 at 10:54:24PM +0100, Mark Kettenis wrote:
> > Are there?  I think there's only one - the outermost.  We've only got
> > that and the sentinel frame, and the sentinel frame I think doesn't
> > have an ID.
> 
> If we can't unwind from a frame for some reason, then its frame ID
> will also be null_frame_id.  And in multi-threaded programs there can
> be multiple outermost frames.

I suppose, but we should never ever ever be comparing frame IDs that
belong to different threads, anyway.  Admittedly they'll all normally
be different, so equality tests would "normally" behave, but e.g.
inner/outer would have no meaning.

> > Perhaps part of the problem here is that step_frame_id is set to
> > null_frame_id when it is invalid; maybe we should keep that separate.
> 
> Not sure; it might not be necessary if we generate a proper frame ID
> for the outermost frame.

I don't think there is any way to do this.  If we use the stack
pointer, the frame ID will change as we single-step; and we can't
use the frame pointer (because if we could, we could unwind).

> > I don't think it would help me though.  Perhaps the real problem is the
> > use of null_frame_id for both the outermost frame and completely
> > unknown frames.  It would be nice if we could tell here:
> > 
> >   if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))
> > 
> > that frame_unwind_id has returned something completely invalid instead
> > of the outermost frame
> 
> Indeed.
> 
> > One way to do that in our current representation would be to check that
> > the frame ID for the current frame is not null_ptid.
> 
> Yes, I think it makes sense to punt trying to insert a step-resume
> breakpoint earlier than you do in your patch.

OK, that makes sense to me too.  I'll give it a try!

> > > > Just to sketch out my example a bit more: the embedded OS I'm debugging
> > > > lives in ROM.  The application I've supplied to GDB lives in RAM.  In
> > > > some later stage of the project, hopefully, I will have GDB magically
> > > > load some other ELF files (that I don't have yet) to cover the ROM
> > > > code; but right now I can't do that and there's no guarantee I'll have
> > > > debug info covering all of it anyway.  So we're executing code way
> > > > out in the boondocks.  GDB doesn't have any way on this platform
> > > > (ARM Thumb) to guess where the start of a function is if it doesn't
> > > > have a symbol table; so it can't be sure that we've really reached the
> > > > first instruction of a function, so it has no idea whether $lr is valid
> > > > or not.
> > > 
> > > But that really means that we shouldn't be messing with step-resume
> > > breakpoints here.  The whole notion of functions that can be stepped
> > > into isn't there.
> > 
> > Yes, it is.  I've executed "step" in a place where I do have symbol
> > information (and working unwinders).  It's taken me into a place where
> > I don't (a DLL in ROM).  Since I don't have debug information any more
> > GDB would like to step back out to the call site, except it fails
> > because we've moved out of its known area.
> 
> Ah, ok.  But that means that step_frame_id really shouldn't be equal
> to null_frame_id.  It will certainly not be null_frame_id if it isn't
> the outermost frame.  And whether the place where you issued "step" is
> the outermost frame or not should not influence GDB's behaviour here.

At the moment I believe I stepped from the outermost frame to a
non-unwindable frame.  So they're going to appear to have the same ID
:-(

This bit I'm going to have to think about some more.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix a crash when stepping and unwinding fails

[NZG]

I don't understand a lot of this tecnical discussion, but I think I have a 
useful comment so I'm going to throw it out there. 
If it's stupid, please ignore.

I believe I recently battled this exact problem on the Coldfire architecture 
with gdb-6.2.
It didn't seem to have a way to know it had reached the end of it's frame list 
without consulting the dwarf symbol table, which didn't help before the code 
had actually entered into main.

My quick and dirty solution was to enable the use of 
deprecated_inside_entry_file 
which the comments warn against, but does seem to work at least in this 
situation.

Somehow this function succeeds in detecting the state is in the entry file and 
stops unwinding, perhaps it bears more investigation.

NZG

diff -Naur gdb-6.1/gdb/frame.c gdb-6.1-5282/gdb/frame.c
--- gdb-6.1/gdb/frame.c	2004-02-16 15:49:21.000000000 -0600
+++ gdb-6.1-5282/gdb/frame.c	2006-01-30 10:29:16.000000000 -0600
@@ -1880,7 +1880,10 @@
      normaly and following frames are treated as being inside the
      enttry file then.  This way, only the #0 frame is printed in the
      backtrace output.  */
-  if (0
+     /**NZG enabled for expermentation**/
+     
+     
+  if (1
       && this_frame->type != DUMMY_FRAME && this_frame->level >= 0
       && deprecated_inside_entry_file (get_frame_pc (this_frame)))
     {





On Wednesday 22 February 2006 4:00 pm, Daniel Jacobowitz wrote:
> On Wed, Feb 22, 2006 at 10:54:24PM +0100, Mark Kettenis wrote:
> > > Are there?  I think there's only one - the outermost.  We've only got
> > > that and the sentinel frame, and the sentinel frame I think doesn't
> > > have an ID.
> >
> > If we can't unwind from a frame for some reason, then its frame ID
> > will also be null_frame_id.  And in multi-threaded programs there can
> > be multiple outermost frames.
>
> I suppose, but we should never ever ever be comparing frame IDs that
> belong to different threads, anyway.  Admittedly they'll all normally
> be different, so equality tests would "normally" behave, but e.g.
> inner/outer would have no meaning.
>
> > > Perhaps part of the problem here is that step_frame_id is set to
> > > null_frame_id when it is invalid; maybe we should keep that separate.
> >
> > Not sure; it might not be necessary if we generate a proper frame ID
> > for the outermost frame.
>
> I don't think there is any way to do this.  If we use the stack
> pointer, the frame ID will change as we single-step; and we can't
> use the frame pointer (because if we could, we could unwind).
>
> > > I don't think it would help me though.  Perhaps the real problem is the
> > > use of null_frame_id for both the outermost frame and completely
> > > unknown frames.  It would be nice if we could tell here:
> > >
> > >   if (frame_id_eq (frame_unwind_id (get_current_frame ()),
> > > step_frame_id))
> > >
> > > that frame_unwind_id has returned something completely invalid instead
> > > of the outermost frame
> >
> > Indeed.
> >
> > > One way to do that in our current representation would be to check that
> > > the frame ID for the current frame is not null_ptid.
> >
> > Yes, I think it makes sense to punt trying to insert a step-resume
> > breakpoint earlier than you do in your patch.
>
> OK, that makes sense to me too.  I'll give it a try!
>
> > > > > Just to sketch out my example a bit more: the embedded OS I'm
> > > > > debugging lives in ROM.  The application I've supplied to GDB lives
> > > > > in RAM.  In some later stage of the project, hopefully, I will have
> > > > > GDB magically load some other ELF files (that I don't have yet) to
> > > > > cover the ROM code; but right now I can't do that and there's no
> > > > > guarantee I'll have debug info covering all of it anyway.  So we're
> > > > > executing code way out in the boondocks.  GDB doesn't have any way
> > > > > on this platform (ARM Thumb) to guess where the start of a function
> > > > > is if it doesn't have a symbol table; so it can't be sure that
> > > > > we've really reached the first instruction of a function, so it has
> > > > > no idea whether $lr is valid or not.
> > > >
> > > > But that really means that we shouldn't be messing with step-resume
> > > > breakpoints here.  The whole notion of functions that can be stepped
> > > > into isn't there.
> > >
> > > Yes, it is.  I've executed "step" in a place where I do have symbol
> > > information (and working unwinders).  It's taken me into a place where
> > > I don't (a DLL in ROM).  Since I don't have debug information any more
> > > GDB would like to step back out to the call site, except it fails
> > > because we've moved out of its known area.
> >
> > Ah, ok.  But that means that step_frame_id really shouldn't be equal
> > to null_frame_id.  It will certainly not be null_frame_id if it isn't
> > the outermost frame.  And whether the place where you issued "step" is
> > the outermost frame or not should not influence GDB's behaviour here.
>
> At the moment I believe I stepped from the outermost frame to a
> non-unwindable frame.  So they're going to appear to have the same ID
>
> :-(
>
> This bit I'm going to have to think about some more.

Re: Fix a crash when stepping and unwinding fails

[Daniel Jacobowitz]

[GDB 6.5 note: I have seen this bug reported roughly five times now.
I think we need to fix it before the release.]

On Wed, Feb 22, 2006 at 10:54:24PM +0100, Mark Kettenis wrote:
> > I don't think it would help me though.  Perhaps the real problem is the
> > use of null_frame_id for both the outermost frame and completely
> > unknown frames.  It would be nice if we could tell here:
> > 
> >   if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))
> > 
> > that frame_unwind_id has returned something completely invalid instead
> > of the outermost frame
> 
> Indeed.
> 
> > One way to do that in our current representation would be to check that
> > the frame ID for the current frame is not null_ptid.
> 
> Yes, I think it makes sense to punt trying to insert a step-resume
> breakpoint earlier than you do in your patch.

First, an apology for confusing the issue.  The frame_id_eq line posted
there is actually OK for this specific problem: frame_id_eq where
either argument is null_frame_id will always return false.  It's
actually the _next_ call to insert_step_resume_breakpoint_at_frame that
blows up.

Of course this means stepping in the next-to-outermost frame doesn't
work quite right.  That will be harder to fix, and see my post to gdb@
[no responses yet...] about the underlying issue of the outermost frame
ID representation.  This is a smaller problem than the segfault.

A related issue I noticed while working on this; there is an unlikely
potential segfault if the undebuggable function being stepped over is
named "main".  frame_unwind_id, called in the line quoted above, will
successfully unwind past main; but get_prev_frame will refuse to.  Thus
we will pass NULL to insert_step_resume_breakpoint_at_frame.

Accordingly, I think the best fix for now is a little code duplication.
I've split the "set a breakpoint at the current frame" operation, used
for signals, from the "set a breakpoint at the return address"
operation, used for stepping over functions.  I've also clarified and
updated some comments.

What do you think of this version?

If you'd like to reproduce this, here's a recipe that works on AMD64;
you'll need to adjust the 0x10 to the return column if you want to try
i386, IIRC 0x8.  I don't want to put this in the testsuite because of
the nasty hack with .rodata1, which serves to make sure stop_func_name
is NULL.  Otherwise, GDB will incorrectly associate it with some
earlier symbol.  This will become easier to test if we teach GDB about
symbol sizes and to not "extend" previous symbols :-)

a.s
===
        .globl main
main:
        xorl %ebp, %ebp
        call foo
        ret

foo:
        .cfi_startproc
        .cfi_escape 0x7, 0x10
        xorl %ebp, %ebp
        call bar
        ret
        .cfi_endproc

b.s
===
        .section ".rodata1", "ax"
        .globl bar
bar:
        .cfi_startproc
        .cfi_escape 0x7, 0x10
        ret
        .cfi_endproc


drow@caradoc:~% gcc -c b.s                              
b.s: Assembler messages:
b.s:1: Warning: setting incorrect section attributes for .rodata1
drow@caradoc:~% gcc -g -o a a.s b.o
drow@caradoc:~% strip -N bar a
drow@caradoc:~% gdb ./a
GNU gdb 6.4.50.20060511-debian
Copyright (C) 2006 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and
you are
welcome to change it and/or distribute copies of it under certain
conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for
details.
This GDB was configured as "x86_64-linux-gnu"...Using host libthread_db
library "/lib/libthread_db.so.1".

(gdb) b foo
Breakpoint 1 at 0x400430: file a.s, line 10.
(gdb) r
Starting program: /home/drow/a 

Breakpoint 1, foo () at a.s:10
10              xorl %ebp, %ebp
Current language:  auto; currently asm
(gdb) s
foo () at a.s:11
11              call bar
(gdb) 
zsh: segmentation fault (core dumped)  gdb ./a


-- 
Daniel Jacobowitz
CodeSourcery

2006-05-17  Daniel Jacobowitz  <dan@codesourcery.com>

	* infrun.c (insert_step_resume_breakpoint_at_caller): New function,
	based on insert_step_resume_breakpoint_at_frame.
	(handle_inferior_event): Update comments.  Use
	insert_step_resume_breakpoint_at_caller.
	(insert_step_resume_breakpoint_at_frame): Revise comments.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.210
diff -u -p -r1.210 infrun.c
--- infrun.c	30 Mar 2006 16:37:13 -0000	1.210
+++ infrun.c	17 May 2006 15:57:54 -0000
@@ -943,6 +943,7 @@ void handle_inferior_event (struct execu
 
 static void step_into_function (struct execution_control_state *ecs);
 static void insert_step_resume_breakpoint_at_frame (struct frame_info *step_frame);
+static void insert_step_resume_breakpoint_at_caller (struct frame_info *);
 static void insert_step_resume_breakpoint_at_sal (struct symtab_and_line sr_sal,
 						  struct frame_id sr_id);
 static void stop_stepping (struct execution_control_state *ecs);
@@ -2367,9 +2368,13 @@ process_event_stop_test:
       return;
     }
 
+  /* Check for subroutine calls.
+
+     NOTE: frame_id_eq will never report two invalid frame IDs as
+     being equal, so to get into this block, both the current and
+     previous frame must have valid frame IDs.  */
   if (frame_id_eq (frame_unwind_id (get_current_frame ()), step_frame_id))
     {
-      /* It's a subroutine call.  */
       CORE_ADDR real_stop_pc;
 
       if (debug_infrun)
@@ -2396,7 +2401,7 @@ process_event_stop_test:
 	  /* We're doing a "next", set a breakpoint at callee's return
 	     address (the address at which the caller will
 	     resume).  */
-	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+	  insert_step_resume_breakpoint_at_caller (get_current_frame ());
 	  keep_going (ecs);
 	  return;
 	}
@@ -2459,7 +2464,7 @@ process_event_stop_test:
 
       /* Set a breakpoint at callee's return address (the address at
          which the caller will resume).  */
-      insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+      insert_step_resume_breakpoint_at_caller (get_current_frame ());
       keep_going (ecs);
       return;
     }
@@ -2513,8 +2518,11 @@ process_event_stop_test:
          and no line number corresponding to the address where the
          inferior stopped).  Since we want to skip this kind of code,
          we keep going until the inferior returns from this
-         function.  */
-      if (step_stop_if_no_debug)
+         function - unless the user has asked us not to (via
+         set step-mode) or we no longer know how to get back
+         to the call site.  */
+      if (step_stop_if_no_debug
+	  || !frame_id_p (frame_unwind_id (get_current_frame ())))
 	{
 	  /* If we have no line number and the step-stop-if-no-debug
 	     is set, we stop the step so that the user has a chance to
@@ -2528,7 +2536,7 @@ process_event_stop_test:
 	{
 	  /* Set a breakpoint at callee's return address (the address
 	     at which the caller will resume).  */
-	  insert_step_resume_breakpoint_at_frame (get_prev_frame (get_current_frame ()));
+	  insert_step_resume_breakpoint_at_caller (get_current_frame ());
 	  keep_going (ecs);
 	  return;
 	}
@@ -2735,20 +2743,13 @@ insert_step_resume_breakpoint_at_sal (st
   if (breakpoints_inserted)
     insert_breakpoints ();
 }
-				      
+
 /* Insert a "step resume breakpoint" at RETURN_FRAME.pc.  This is used
-   to skip a function (next, skip-no-debug) or signal.  It's assumed
-   that the function/signal handler being skipped eventually returns
-   to the breakpoint inserted at RETURN_FRAME.pc.
-
-   For the skip-function case, the function may have been reached by
-   either single stepping a call / return / signal-return instruction,
-   or by hitting a breakpoint.  In all cases, the RETURN_FRAME belongs
-   to the skip-function's caller.
-
-   For the signals case, this is called with the interrupted
-   function's frame.  The signal handler, when it returns, will resume
-   the interrupted function at RETURN_FRAME.pc.  */
+   to skip a potential signal handler.
+
+   This is called with the interrupted function's frame.  The signal
+   handler, when it returns, will resume the interrupted function at
+   RETURN_FRAME.pc.  */
 
 static void
 insert_step_resume_breakpoint_at_frame (struct frame_info *return_frame)
@@ -2763,6 +2764,38 @@ insert_step_resume_breakpoint_at_frame (
   insert_step_resume_breakpoint_at_sal (sr_sal, get_frame_id (return_frame));
 }
 
+/* Similar to insert_step_resume_breakpoint_at_frame, except
+   but a breakpoint at the previous frame's PC.  This is used to
+   skip a function after stepping into it (for "next" or if the called
+   function has no debugging information).
+
+   The current function has almost always been reached by single
+   stepping a call or return instruction.  NEXT_FRAME belongs to the
+   current function, and the breakpoint will be set at the caller's
+   resume address.
+
+   This is a separate function rather than reusing
+   insert_step_resume_breakpoint_at_frame in order to avoid
+   get_prev_frame, which may stop prematurely (see the implementation
+   of frame_unwind_id for an example).  */
+
+static void
+insert_step_resume_breakpoint_at_caller (struct frame_info *next_frame)
+{
+  struct symtab_and_line sr_sal;
+
+  /* We shouldn't have gotten here if we don't know where the call site
+     is.  */
+  gdb_assert (frame_id_p (frame_unwind_id (next_frame)));
+
+  init_sal (&sr_sal);		/* initialize to zeros */
+
+  sr_sal.pc = ADDR_BITS_REMOVE (frame_pc_unwind (next_frame));
+  sr_sal.section = find_pc_overlay (sr_sal.pc);
+
+  insert_step_resume_breakpoint_at_sal (sr_sal, frame_unwind_id (next_frame));
+}
+
 static void
 stop_stepping (struct execution_control_state *ecs)
 {

Re: Fix a crash when stepping and unwinding fails

[Daniel Jacobowitz]

On Wed, May 17, 2006 at 12:02:20PM -0400, Daniel Jacobowitz wrote:
> [GDB 6.5 note: I have seen this bug reported roughly five times now.
> I think we need to fix it before the release.]

I didn't get any feedback from this patch, which is posted here:

  http://sourceware.org/ml/gdb-patches/2006-05/msg00379.html

Of course, I was mostly hoping for a response from Mark Kettenis, and I
believe he went on vacation a couple days later.  But I'd really like
to have this issue fixed for GDB 6.5.  Does anyone object to my
checking it in - and then, if Mark would like me to change it later,
I'll be sure to take care of it promptly.


-- 
Daniel Jacobowitz
CodeSourcery

Re: Fix a crash when stepping and unwinding fails

[Joel Brobecker]

> Of course, I was mostly hoping for a response from Mark Kettenis, and I
> believe he went on vacation a couple days later.  But I'd really like
> to have this issue fixed for GDB 6.5.  Does anyone object to my
> checking it in - and then, if Mark would like me to change it later,
> I'll be sure to take care of it promptly.

I was actually going to ask about this. No objection from me.

On a related note, there is also a change that you posted that
also fixed a crash for MinGW-hosted debugger, right? Stupidly,
I lost that message. Anyway, that might also be another interesting
fix for the release.

-- 
Joel

Re: Fix a crash when stepping and unwinding fails

[Daniel Jacobowitz]

On Wed, Jun 14, 2006 at 05:32:59AM -0700, Joel Brobecker wrote:
> > Of course, I was mostly hoping for a response from Mark Kettenis, and I
> > believe he went on vacation a couple days later.  But I'd really like
> > to have this issue fixed for GDB 6.5.  Does anyone object to my
> > checking it in - and then, if Mark would like me to change it later,
> > I'll be sure to take care of it promptly.
> 
> I was actually going to ask about this. No objection from me.

Done, branch and HEAD.

> On a related note, there is also a change that you posted that
> also fixed a crash for MinGW-hosted debugger, right? Stupidly,
> I lost that message. Anyway, that might also be another interesting
> fix for the release.

That's a good idea.  I've applied that one too.

-- 
Daniel Jacobowitz
CodeSourcery