Re: [PATCH 4/6] gdb: revert frame_unwind::this_id and callees to use `frame_info *`

[Simon Marchi <simon.marchi@polymtl.ca>]

On 12/5/22 16:41, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> So, this patch started out as changing the prototype of
> Simon> frame_unwind::this_id and adjusting callees recursively until the code
> Simon> compiled again.
> 
> Simon> The question is: doesn't this defeat the purpose of frame_info_ptr?  I
> Simon> think it depends on what we decide the purpose of frame_info_ptr is.
> 
> Simon>  1. catch accesses to stale frame_info ptr (and assert if so)
> Simon>  2. reinflate frame_info objects automatically.
> 
> Simon> Right now, it does #1.  My goal is to make it to #2.  I think that is
> Simon> more in line with the original "reinflatable frames" idea, and in
> Simon> general more useful.
> 
> To be clear, my goal in doing this was to try to eliminate the bug where
> a frame_info pointer is held across a frame cache reinit.  From my
> perspective, both of these approaches satisfy that goal, so the idea of
> this change seems completely fine.

Ok, thanks.

Like I said, it's not *impossible* for such a bug to creep up again in a
function that is converted back to use `frame_info *`, just not likely.

> Simon> And since those frames whose id is being computed
> Simon> are by definition not reinflatable (you need to id to reinflate), it's
> Simon> not really useful to have frame_info_ptr on code paths that deal with
> Simon> frames whose id is currently being computed.  Reinflation only really
> Simon> makes sense for the "users" of the frame API, those who receive a frame
> Simon> with a computed id.  And by experience, this is where the "using stale
> Simon> frame_info" bugs happen, a user holding a frame_info across a target
> Simon> resumption.
> 
> Basically I looked into this and it seemed difficult, so I gave up.
> And, a simpler approach didn't work.  But, you managed it :-) So kudos!
> 
> I think a good long run goal would be if frame_id and frame_info_ptr
> were generally merged.  That is, the "using" code should only ever need
> to touch a frame_info_ptr.  I don't know if this is really feasible, but
> it would be another step toward making the API simpler to use and less
> buggy.

Interesting idea.  I think that a lot of places that save a frame_id
(and sometimes frame level) for later use could indeed be converted to
use frame_info_ptr.  These places don't save a `frame_info *`, because
they know the frame_info object will get deleted along the way.  But
with a frame_info_ptr, it will just get reinflated transparently.  After
all, frame_info_ptr is exactly that, a frame_id and a frame level.
Plus a `frame_info *` that "caches" the frame_info object.  So, a tiny
bit of overhead compared to just frame_id + frame level, but much less
change of being mis-used.

> 
> Simon> Now, we could have two separate class frame_info wrapper classes, one
> Simon> that is today's frame_info_ptr, whose goal is just to catch uses of
> Simon> stale frame_info objects, and one that is the one at the end of this
> Simon> series, whose goal is to do automatic reinflation.
> 
> I would say the default choice should be a single class, unless for some
> reason it's too expensive.

I initially thought about that, a single class that could be in
different states - frame id know and frame id unknown, but dropped it
becuase of the extra complexity.  A frame_info_ptr created for a
`frame_info *` before the id is known will not save a frame_id.  What
happens once the frame_id of the frame_info becomes known?  If you copy
construct a frame_info_ptr from that original frame_info_ptr that
doesn't know the frame_id, should the new frame_info_ptr fetch the
frame_id from the frame_info?  I didn't want to solve these issues.

But in reality, that transition really happens around the this_id call
inside compute_frame_id, so we can have some special handling there.
The frame_info_ptr coming it could have no know frame id, and it could
have one when going out.

The only downside I see with this approach is that in a given random
function using frame_info_ptr, you can't easily know statically if the
frame_info_ptr is guaranteed to be reinflatable or not.  In other words,
if you are on the "frame id computation" path.  Maybe we can just hit an
assert when trying to reinflate a non-reinflatable frame_info_ptr (one
that doesn't know the frame id).  If that would happen, it means we had
a frame cache reinit while computing a frame id, which is the case I
told should never happen.  And so that might help us catch it, if it was
to happen.  I think you've convinced me to go back to use frame_info_ptr
and have these two states :).

Simon