RFC: fix PR backtrace/15558

  On 04/14/2014 07:52 PM, Tom Tromey wrote:
> Tom> I never got back to fixing it according to Pedro's review.
> 
> Tom> As I recall it isn't entirely trivial because for a good fix one would
> Tom> need to expose some new Python methods for use by the frame filter code.
> 
> Pedro> Hmm.  Not sure what you're thinking of.
> 
> There's a good chance I'm confusing it with some other approach I'd
> considered.
> 
> Pedro> We discussed auditing all
> Pedro> get_prev_frame uses and see if they are better replaced by
> Pedro> get_prev_frame_1, but I don't think that should hold back
> Pedro> fixing the inline frame unwinder.
> 
> Ok.

See patch below.  I've simplified your test a bit, and added a test
that doesn't depend on Python, to the existing gdb.opt/inline-bt.exp.

> I think what I was thinking is that if one starts this change, then the
> question arises: which should the Python unwinder call?  Calling the
> user-facing one is plainly incorrect.

Not sure.  I guess Python code might want to be implementing some
new frame-related CLI command, where the limits make sense.  Maybe
Python should really have access to both variants, somehow?

> But, calling the internal one is
> also incorrect, as some termination conditions may be skipped.
>
> Perhaps this only applies if we move all the checks into get_prev_frame_1.
> I don't recall whether that was part of the plan or not.

Well, the plan didn't go as far as prescribe what to do exactly, only
auditing/thinking through this.  :-)  The only termination condition in
get_prev_frame that is a not user setting is the "zero PC" one, and
it's arguable whether that one should be there or in the internal
one.

-------------
[PATCH] Fix PR backtrace/15558

This PR is about an assertion failure in GDB that can be triggered by
setting "backtrace limit" to a value that causes GDB to stop unwinding
after an inline frame.  In this case, an assertion in
inline_frame_this_id will trigger:

  /* We need a valid frame ID, so we need to be based on a valid
     frame.  FSF submission NOTE: this would be a good assertion to
     apply to all frames, all the time.  That would fix the ambiguity
     of null_frame_id (between "no/any frame" and "the outermost
     frame").  This will take work.  */
  gdb_assert (frame_id_p (*this_id));

Looking at the function:

static void
inline_frame_this_id (struct frame_info *this_frame,
		      void **this_cache,
		      struct frame_id *this_id)
{
  struct symbol *func;

  /* In order to have a stable frame ID for a given inline function,
     we must get the stack / special addresses from the underlying
     real frame's this_id method.  So we must call get_prev_frame.
     Because we are inlined into some function, there must be previous
     frames, so this is safe - as long as we're careful not to
     create any cycles.  */
  *this_id = get_frame_id (get_prev_frame (this_frame));

we see we're computing the frame id for the inline frame.  If this is
an inline frame, which is a virtual frame constructed based on debug
info, on top of a real stack frame, we should _always_ be able to find
where the frame was inlined into, as that ultimately just means
peeling off the virtual frames on top of the real stack frame.  If
there ultimately was no prev (real) stack frame, then we wouldn't have
been able to construct the inline frame either, by design.  That's
what the assertion catches.

So we have an inline frame, we should _always_ be able to compute its
ID, even if that means bypassing the user backtrace limits to get at
the real stack frame's info.  The problem is that inline_frame_id
calls get_prev_frame, and that takes user backtrace limits into
account.  Code that wants to bypass the limits calls get_prev_frame_1
instead.

(A note on that NOTE above.  It's stale: we have outer_frame_id to
distinguish from null_frame_id now.)

Note how get_prev_frame_1 already skips all checks for inline frames:

  /* If we are unwinding from an inline frame, all of the below tests
     were already performed when we unwound from the next non-inline
     frame.  We must skip them, since we can not get THIS_FRAME's ID
     until we have unwound all the way down to the previous non-inline
     frame.  */
  if (get_frame_type (this_frame) == INLINE_FRAME)
    return get_prev_frame_if_no_cycle (this_frame);

And note how the related frame_unwind_caller_id function also uses
get_prev_frame_1:

struct frame_id
frame_unwind_caller_id (struct frame_info *next_frame)
{
  struct frame_info *this_frame;

  /* Use get_prev_frame_1, and not get_prev_frame.  The latter will truncate
     the frame chain, leading to this function unintentionally
     returning a null_frame_id (e.g., when a caller requests the frame
     ID of "main()"s caller.  */

  next_frame = skip_artificial_frames (next_frame);
  this_frame = get_prev_frame_1 (next_frame);
  if (this_frame)
    return get_frame_id (skip_artificial_frames (this_frame));
  else
    return null_frame_id;
}

get_prev_frame_1 is currently static in frame.c.  As a _1 suffix is
not a good name for an extern function, I've renamed it.

Tested on x86-64 Fedora 17.

gdb/
2014-04-14  Pedro alves  <palves@redhat.com>
	    Tom Tromey  <tromey@redhat.com>

	PR backtrace/15558
	* frame.c (get_prev_frame_1): Rename to ...
	(get_prev_frame_always): ... this, and make extern.  Adjust.
	(skip_artificial_frames): Use get_prev_frame_always.
	(frame_unwind_caller_id, frame_pop, get_prev_frame)
	(get_frame_unwind_stop_reason): Adjust to rename.
	* frame.h (get_prev_frame_always): Declare.
	* inline-frame.c: Include frame.h.
	(inline_frame_this_id): Use get_prev_frame_always.

gdb/testsuite/
2014-04-14  Tom Tromey  <palves@redhat.com>
	    Pedro alves  <tromey@redhat.com>

	PR backtrace/15558
	* gdb.opt/inline-bt.exp: Test backtracing from an inline function
	with a backtrace limit.
	* py-frame-inline.exp: Test running to an inline function with a
	bactrace limit, and printing the newest frame.
---
 gdb/frame.c                                  | 31 ++++++++++++++++------------
 gdb/frame.h                                  |  7 +++++++
 gdb/inline-frame.c                           | 16 +++++++-------
 gdb/testsuite/gdb.opt/inline-bt.exp          | 16 ++++++++++++++
 gdb/testsuite/gdb.python/py-frame-inline.c   |  4 +++-
 gdb/testsuite/gdb.python/py-frame-inline.exp | 14 +++++++++++++
 6 files changed, 65 insertions(+), 23 deletions(-)

RFC: fix PR backtrace/15558

Commit Message

Comments

Patch