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

Andrew Burgess
2021-06-30T15:19:00Z

* Simon Marchi <simon.marchi@polymtl.ca> [2021-06-29 13:53:20 -0400]:

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

It was unexpected to me :)

The frame number being reported here is the frame that wants the
register, not the frame that provides the register, so given a stack
like this (now with frame numbers):

#1:normal_frame -> #0:inline_frame -> #-1:sentinel_frame

And GDB says:

{ value_fetch_lazy (frame=1,regnum=.......

GDB is trying to say that frame #1 asked to read a register, and the
value returned as .... whatever ....

BUT, what we _actually_ end up saying is:

{ value_fetch_lazy (frame=0,regnum=.......

Which just isn't correct, frame 0 didn't cause the lazy register value
to be created, frame 1 did. A developer reading these logs needs to
understand that frame 0 is inline, and that the value in frame 1 is
the same as the value in frame 0.

You are correct that the patch you linked is indeed what introduced
this mess in the first place, but I think the reasoning behind that
original patch is still good.

If this makes more sense then I will update the commit message to
hopefully make things clearer - what do you think?

Thanks,
Andrew