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

[Tom Tromey <tom@tromey.com>]

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

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.

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.

Tom