Re: [PATCHv5] gdb: prevent an assertion when computing the frame_id for an inline frame

Andrew Burgess
2021-09-21T13:53:09Z

* Pedro Alves <pedro@palves.net> [2021-09-20 13:24:50 +0100]:

> Hi Andrew,
>
> On 2021-08-09 4:41 p.m., Andrew Burgess wrote:
> > I've not heard anything from Pedro, but I'd like to move this patch
> > forward.
> >
> > My assumption is that Pedro doesn't like using exceptions to pass
> > around information for a case that maybe isn't that exceptional. As
> > Pedro was happy with v2, this new patch removes the use of exceptions.
> >
> > Simon's concerns with v1/v2 were, I think, summarised as:
> >
> > 1. What get_prev_frame_if_no_cycle does no longer matches the
> > function name, and
> >
> > 2. get_prev_frame_if_no_cycle was trying to figure out the callers
> > intention in order to change its behaviour.
> >
> > To try and address these two things I have:
> >
> > 1. Renamed get_prev_frame_if_no_cycle to hopefully make it clearer
> > that its result is not so clear cut, and
> >
> > 2. Changed the behaviour of get_prev_frame_if_no_cycle based on a
> > passed in parameter instead of "peeking" at various bits of state.
> >
> > My hope is that this might be more acceptable to everyone, but I'd
> > love to hear any thoughts,
> >
>
> To be honest, to me, the new "cycle_detection_p" argument seems pointless/redundant,
> since inside the function, you have the INLINE_FRAME assertion checks anyhow. I.e.,
> putting:
>
> bool cycle_detection_p = get_frame_type (this_frame) != INLINE_FRAME;
>
> inside get_prev_frame_with_optional_cycle_detection yields the exact same, IIUC.
>
> So adding the argument is just adding scope for someone passing the wrong
> cycle_detection_p argument at some point, it seems to me.
>
> frame.c already has to know special things about inline frames, I mean,
> even get_prev_frame_always_1 already must know to bypass a bunch of tests
> for inline frames, like:
>
> /* 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);
>
>
> I'd remove the cycle_detection_p argument, replacing it said local:
>
> bool cycle_detection_p = get_frame_type (this_frame) != INLINE_FRAME;
>
> and rename get_prev_frame_if_no_cycle to, say, get_prev_frame_maybe_check_cycle.
>
> Like, e.g.:
>
> ~~~~
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 6433e9db788..2c9a324c93f 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -2051,16 +2051,14 @@ frame_register_unwind_location (struct frame_info *this_frame, int regnum,
> the previous frame is a duplicate and we return nullptr then we will be
> unable to calculate the frame_id of the inline frame, this in turn
> causes inline_frame_this_id() to fail. So for inline frames (and only
> - for inline frames) it is acceptable to pass CYCLE_DETECTION_P as false,
> - in that case the previous frame will always be returned, even when it
> + for inline frames), the previous frame will always be returned, even when it
> has a duplicate frame_id. We're not worried about cycles in the frame
> chain as, if the previous frame returned here has a duplicate frame_id,
> then the frame_id of the inline frame, calculated based off the frame_id
> of the previous frame, should also be a duplicate. */
>
> static struct frame_info *
> -get_prev_frame_with_optional_cycle_detection (struct frame_info *this_frame,
> - bool cycle_detection_p)
> +get_prev_frame_maybe_check_cycle (struct frame_info *this_frame)
> {
> struct frame_info *prev_frame;
>
> @@ -2083,6 +2081,9 @@ get_prev_frame_with_optional_cycle_detection (struct frame_info *this_frame,
> are simpler. For these frames we immediately compute the frame_id,
> and so, for those frames, we will always reenter this function with
> the frame_id status of COMPUTING. */
> +
> + bool cycle_detection_p = get_frame_type (this_frame) != INLINE_FRAME;
> +
> gdb_assert (cycle_detection_p
> || (get_frame_type (this_frame) == INLINE_FRAME
> && ((this_frame->level > 0
> @@ -2193,7 +2194,7 @@ get_prev_frame_always_1 (struct frame_info *this_frame)
> 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_with_optional_cycle_detection (this_frame, false);
> + return get_prev_frame_maybe_check_cycle (this_frame);
>
> /* If this_frame is the current frame, then compute and stash its
> frame id prior to fetching and computing the frame id of the
> @@ -2294,7 +2295,7 @@ get_prev_frame_always_1 (struct frame_info *this_frame)
> }
> }
>
> - return get_prev_frame_with_optional_cycle_detection (this_frame, true);
> + return get_prev_frame_maybe_check_cycle (this_frame);
> }
>
> /* Return a "struct frame_info" corresponding to the frame that called
> ~~~~

Thanks, I think that's a great suggestion. I'll use this in the next
version.

>
>
>
> > --- a/gdb/inline-frame.c
> > +++ b/gdb/inline-frame.c
> > @@ -163,7 +163,10 @@ inline_frame_this_id (struct frame_info *this_frame,
> > function, there must be previous frames, so this is safe - as
> > long as we're careful not to create any cycles. See related
> > comments in get_prev_frame_always_1. */
> > - *this_id = get_frame_id (get_prev_frame_always (this_frame));
> > + frame_info *prev_frame = get_prev_frame_always (this_frame);
> > + if (prev_frame == nullptr)
> > + error ("failed to find previous frame when computing inline frame id");
>
> Missing _(). But, is this a "just in case" check/error? As in, a softer
> gdb_assert? With the bug fixed, is this error call ever expected to
> trigger (modulo other gdb bugs)?

No, I don't think this is a softer assert. get_prev_frame_always can
return nullptr for other reasons, e.g if a MEMORY_ERROR is thrown
while figuring out the previous frame.

If that should happen then we'll be back in the original situation,
*this_id will be null_frame_id, and the assert that I originally saw
will trigger again.

I don't see any reason why get_prev_frame_always _shouldn't_ return
nullptr here, but if it does I we can't get the inline frame's id.

I'll post a new patch with the changes made.

Thanks,
Andrew