Re: RFC: fix PR backtrace/15558

[Pedro Alves <palves at redhat dot com>]

Hi Tom,

On 06/06/2013 08:35 PM, Tom Tromey wrote:
> PR backtrace/15558 concerns an assertion failure in gdb that can be
> triggered by setting "backtrace limit" to a value that causes gdb to
> stop unwinding in the middle of a series of inlined frames.  In this
> case, an assertion in inline_frame_this_id will trigger.
> 
> The bug is essentially that get_prev_frame checks backtrace_limit.
> However, this is not needed or desirable in most cases.  E.g., the
> Python API to unwinding should probably not be limited by this setting.
> 
> This patch removes the check from get_prev_frame and introduces a new
> checking function that is used when printing a stack trace.
> 
> Pedro had suggested removing all the other checks from get_prev_frame.

Yeah, I was suggesting that "internal" / non-user-facing code should
not be using get_prev_frame, but get_prev_frame_1 instead, bypassing
all the checks.  (Or rather wondering why that isn't so).  Strongly more
so in an unwinder's innards.  get_prev_frame uses need to be investigated
on a case-by-case manner manner to decide the best course of action, IMO.

Focusing on inline_frame_this_id first, the case in question for
the bug:

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're building the frame id for the inline frame.  I say the CLI
"set backtrace limit/past-entry/past-main" settings should really have
no bearing on this.  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 it 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 stack frame,
then we wouldn't have an inline frame either, by design.  It's just
logically impossible.  That's what the assertion catches:

  /* 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));

A note on that NOTE.  It's stale; we have outer_frame_id to distinguish
from null_frame_id now.  (though that should really die.)

Hard to imagine the main function being inlined (at least on c-like
languages), but if any of the NULL returns in get_prev_frame hit here,
you'll trip on this assertion again.  It seems like the zero PC case
at the bottom of get_prev_frame can trigger.  I can't say I understand what
exactly that is supposed to catch.

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_raw (this_frame);

And note how the somewhat 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;
}

So conceptually, in this case, I think what makes most sense it to skip
_all_ the checks in get_prev_frame* that might return NULL, as there should
always be a prev frame for an inline frame.  IOW, in this case, I believe
we should be making inline_frame_this_id call get_prev_frame_1, or whatever
it gets renamed to, or equivalent.

-- 
Pedro Alves