Re: [PATCHv2 2/2] gdb: remove VALUE_FRAME_ID

Simon Marchi via Gdb-patches
2021-06-29T17:53:49Z

On 2021-06-21 3:46 p.m., Andrew Burgess wrote:
> While working on the previous commit I happened to switch on 'set
> debug frame 1', and ran into a different assertion than the one I was
> trying to fix.
>
> The new problem I see is this assertion triggering:
>
> gdb/frame.c:622: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->this_id.p != frame_id_status::COMPUTING' failed.
>
> We attempt to get the frame_id for a frame while we are computing the
> frame_id for that same frame.
>
> What happens is we have a stack like this:
>
> normal_frame -> inline_frame -> sentinel_frame
>
> When we initially stop, GDB creates a frame for inline_frame but
> doesn't sniff its type, or try to fill in its frame_id (see code near
> the top of get_prev_frame_if_no_cycle).
>
> Later on during the stop, this happens:
>
> process_event_stop_test
> get_stack_frame_id
> skip_artificial_frames
> get_frame_type
>
> The call to get_frame_type causes inline_frame to sniff its type,
> establishing that it is an INLINE_FRAME, but does not cause the frame
> to build its frame_id.
>
> The same skip_artificial_frames call then calls get_prev_frame_always
> to unwind the stack, this will then try to get the frame previous to
> inline_frame, i.e. normal_frame.
>
> So, we create a new frame, but unlike frame #0, in
> get_prev_frame_if_no_cycle, we immediately try to compute the frame_id
> for this new frame #1.
>
> Computing the frame_id for frame #1 invokes the sniffer. If this
> sniffer tries to read a register then we will create a lazy register
> value by calling value_of_register_lazy. As the next frame (frame #0)
> is an INLINE_FRAME, we will skip this and instead create the lazy
> register value using the frame_id for frame #-1 (the sentinel frame).
>
> Now, when we try to resolve the lazy register value, in the value.c
> function value_fetch_lazy_register, we want to print which frame the
> lazy register value was for (in debug mode), so we call:
>
> frame = frame_find_by_id (VALUE_FRAME_ID (val));
>
> where:
>
> #define VALUE_FRAME_ID(val) (get_prev_frame_id_by_id (VALUE_NEXT_FRAME_ID (val)))
>
> In our case we call get_prev_frame_id_by_id with the frame_id of the
> sentinel_frame frame, we then call frame_find_by_id, followed by
> get_prev_frame (which gives us frame #0), followed by get_frame_id.
>
> Frame #0 has not yet computed its frame_id, so we start that process.
> The first thing we need when computing the frame_id of an inline frame
> is the frame_id of the previous frame, so that's what we ask for.
> Unfortunately, that's where we started this journey, we are already
> computing the frame_id for frame #1, and thus the assert fires.
>
> Solving the assertion failure is pretty easy, if we consider the code
> in value_fetch_lazy_register and get_prev_frame_id_by_id then what we
> do is:
>
> 1. Start with a frame_id taken from a value,
> 2. Lookup the corresponding frame,
> 3. Find the previous frame,
> 4. Get the frame_id for that frame, and
> 5. Lookup the corresponding frame
> 6. Print the frame's level
>
> Notice that steps 3 and 5 give us the exact same result, step 4 is
> just wasted effort. We could shorten this process such that we drop
> steps 4 and 5, thus:
>
> 1. Start with a frame_id taken from a value,
> 2. Lookup the corresponding frame,
> 3. Find the previous frame,
> 6. Print the frame's level
>
> This will give the exact same frame as a result, and this is what I
> have done in this patch by removing the use of VALUE_FRAME_ID from
> value_fetch_lazy_register.
>
> Out of curiosity I looked to see how widely VALUE_FRAME_ID was used,
> and saw it was only used in one other place in valops.c:value_assign,
> where, once again, we take the result of VALUE_FRAME_ID and pass it to
> frame_find_by_id, thus introducing a redundant frame_id lookup.
>
> I don't think the value_assign case risks triggering the assertion
> though, as we are unlikely to call value_assign while computing the
> frame_id for a frame, however, we could make value_assign slightly
> more efficient, with no real additional complexity, by removing the
> use of VALUE_FRAME_ID.
>
> So, in this commit, I completely remove VALUE_FRAME_ID, and replace it
> with a use of VALUE_NEXT_FRAME_ID, followed by a direct call to
> get_prev_frame_always, this should make no difference in either case,
> and resolves the assertion issue from value.c.
>
> One thing that is worth noticing here is that in these two situations
> we don't end up getting the frame we expected to, though this is not a
> result of my change, we were not getting the expected result before
> either.
>
> Consider the debug printing case, the stack is:
>
> normal_frame -> inline_frame -> sentinel_frame
>
> We read a register from normal_frame (frame #1), the value of which is
> fetched from sentinel_frame (frame #-1). The debug print is trying to
> say:
>
> frame=1,regnum=....
>
> However, as the lazy register value points at frame #-1, we will
> actually (incorrectly) print:
>
> frame=0,regnum=....
>
> Like I said, this bug existed before this commit, and so, if we didn't
> assert (i.e. the lazy register read occurred in some context other
> than during the frame sniffer), then the debug print would previous
> have, and will continue to, print the wrong frame level. Thankfully,
> this is only in debug output, so not something a normal user should
> see.

Are you sure this is not expected? An inline frame does not have
registers of its own, so it could be normal/expected, that to unwind
registers, we just skip over inline frames, so that a register value's
next frame id skips over inline frame.

See this commit (that you wrote :)):

https://gitlab.com/gnutools/gdb/-/commit/9fc501fdfe5dc82b5e5388cde4ac2ab70ed69d75

I might not understand correctly what you mean though.

The patch itself LGTM.

Simon