Re: [PATCH 1/4] Distinguish sentinel frame from null frame

[Pedro Alves <palves at redhat dot com>]

Hi Kevin,

Finally looking at this.

On 09/28/2016 09:49 AM, Kevin Buettner wrote:

> ---
>  gdb/frame.c | 101 +++++++++++++++++++++++++++++++++++++++++-------------------
>  gdb/frame.h |  12 +++++++-
>  2 files changed, 81 insertions(+), 32 deletions(-)
> 
> diff --git a/gdb/frame.c b/gdb/frame.c
> index d3f3056..3ca8ab2 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -43,6 +43,17 @@
>  #include "hashtab.h"
>  #include "valprint.h"
>  
> +/* The sentinel frame is the innermost frame.  It generally does not
> +   have any stack associated with it.  Register values may be directly
> +   determined by inspection with no unwinding necessary.

I think this comment is confusing.  If you look at comments in
sentinel-frame.h|c, you'll find it calling the sentinel 
frame the one past the inner-most instead of being _the_ innermost:

[...]
/* Implement the sentinel frame.  The sentinel frame terminates the
   inner most end of the frame chain.  If unwound, it returns the
   information need to construct an inner-most frame.  */
[...]
/* Pump prime the sentinel frame's cache.  Since this needs the
   REGCACHE provide that here.  */

  /* The sentinel frame is used as a starting point for creating the
     previous (inner most) frame.  That frame's THIS_ID method will be
     called to determine the inner most frame's ID.  Not this one.  */
[...]

> It generally does not have any stack associated with it. 

Right, it should not ever be possible to ask the sentinel
frame for its stack pointer, because we'd have to unwind that
from frame level -2.  :-)

> Register values may be directly determined by inspection with
> no unwinding necessary.

This is true of the current frame.  The reason we have a sentinel
frame is that the unwind machinery always gets the THIS_FRAME's
registers by unwinding them from the next frame.  In order to
keep that working without a special case for frame #0, we stuff
the sentinel frame before (at frame level -1).  That's why the
only useful thing sentinel-frame.c does is implement prev_register.


> +
> +   The sentinel frame is constructed so that the next frame is a pointer
> +   to the sentinel frame itself.
> +
> +   The current frame can be found at sentinel_frame->prev.  */
> +
> +static struct frame_info *sentinel_frame;


> @@ -323,6 +334,8 @@ fprint_frame_id (struct ui_file *file, struct frame_id id)
>      fprintf_unfiltered (file, "!stack");
>    else if (id.stack_status == FID_STACK_UNAVAILABLE)
>      fprintf_unfiltered (file, "stack=<unavailable>");
> +  else if (id.stack_status == FID_STACK_SENTINEL)
> +    fprintf_unfiltered (file, "stack=<sentinel>");
>    else
>      fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
>    fprintf_unfiltered (file, ",");
> @@ -511,6 +524,9 @@ get_frame_id (struct frame_info *fi)
>    if (fi == NULL)
>      return null_frame_id;
>  
> +  if (fi == sentinel_frame)
> +    return sentinel_frame_id;
> +

Why do you need this?  When the sentinel frame is created, it's
given a frame id already:

  /* The sentinel frame has a special ID.  */
  frame->this_id.p = 1;
  frame->this_id.value = sentinel_frame_id;

>    if (!fi->this_id.p)
>      {
>        int stashed;


> -/* Info about the innermost stack frame (contents of FP register).  */
> -
> -static struct frame_info *current_frame;
> -
>  /* Cache for frame addresses already read by gdb.  Valid only while
>     inferior is stopped.  Control variables for the frame cache should
>     be local to this module.  */
> @@ -1511,6 +1527,9 @@ static struct frame_info *get_prev_frame_always_1 (struct frame_info *this_frame
>  struct frame_info *
>  get_current_frame (void)
>  {
> +  struct frame_info *current_frame;
> +  int new_sentinel_p = 0;
> +  /* Set the current frame before computing the frame id, to avoid
> +     recursion inside compute_frame_id, in case the frame's
> +     unwinder decides to do a symbol lookup (which depends on the
> +     selected frame's block).
> +
> +     This call must always succeed.  In particular, nothing inside
> +     get_prev_frame_always_1 should try to unwind from the
> +     sentinel frame, because that could fail/throw, and we always
> +     want to leave with the current frame created and linked in --
> +     we should never end up with the sentinel frame as outermost
> +     frame.  */
> +  current_frame = get_prev_frame_always_1 (sentinel_frame);
> +  gdb_assert (current_frame != NULL);
> +
> +  /* The call to get_frame_id, below, is not necessary when the stack is
> +     well behaved.  But when it's not well behaved, we want to ensure
> +     that the frame id for the current frame is known (stashed) prior to
> +     finding frame id values for any outer frames.
> +
> +     The test gdb.dwarf2/dw2-dup-frame.exp shows an internal error
> +     in the "backtrace from stop_frame" test when we don't do this here.  */
> +  if (new_sentinel_p)
> +    get_frame_id (current_frame);

I don't understand this.  I applied patch #1 locally with this bit
removed, and I don't see said internal error.

Oh, this only happens when the following patches are applied.

IMO, it's better to move this hunk to the patch that actually
creates the requirement for it, so that we have the whole
logical change in one patch.  Can you elaborate more on
why the test causes the internal "backtrace from stop_frame"
error without this here?

> +
>    return current_frame;
>  }
>  
> @@ -1729,6 +1755,19 @@ get_next_frame (struct frame_info *this_frame)
>      return NULL;
>  }
>  
> +/* Return the frame that THIS_FRAME calls.  If THIS_FRAME is the innermost
> +   frame, we return the sentinel frame.  Thus, NULL will never be returned.  */

This comment is a ambiguous too -- is innermost here talking about the sentinel
frame or the real innermost frame (current frame) ?

> +
> +struct frame_info *
> +get_next_frame_sentinel_okay (struct frame_info *this_frame)
> +{
> +  gdb_assert (this_frame != NULL);
> +
> +  /* This always works, since the sentinel frame refers to itself via the
> +     ``next'' field.  */
> +  return this_frame->next;
> +}


Thanks,
Pedro Alves