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

Pedro Alves
2021-09-20T12:25:06Z

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
~~~~

> --- 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)?