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

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

On 12/12/22 08:17, Bruno Larsen wrote:
> On 02/12/2022 19:00, Simon Marchi via Gdb-patches wrote:
>> A following patch will change frame_info_ptr to grab the frame id and
>> reinflate automatically.  This makes it so frame_info_ptr will no longer
>> be able to wrap frame_info objects whose ids are being computed.  This
>> means that the frame_unwind::this_id method can't take a frame_info_ptr
>> anymore, since that's the entry point of computing frame ids.  And then,
>> everything that "this_id" implementations can possibly call won't be
>> able to accept a frame_info_ptr anymore.
>>
>> So, this patch started out as changing the prototype of
>> frame_unwind::this_id and adjusting callees recursively until the code
>> compiled again.
>>
>> The question is: doesn't this defeat the purpose of frame_info_ptr?  I
>> think it depends on what we decide the purpose of frame_info_ptr is.
>>
>>   1. catch accesses to stale frame_info ptr (and assert if so)
>>   2. reinflate frame_info objects automatically.
> I agree with Tom's point, both points fix the bugs that we tackled when starting this, and I personally like the second option better.
>>
>> Right now, it does #1.  My goal is to make it to #2.  I think that is
>> more in line with the original "reinflatable frames" idea, and in
>> general more useful.  And since those frames whose id is being computed
>> are by definition not reinflatable (you need to id to reinflate), it's
>> not really useful to have frame_info_ptr on code paths that deal with
>> frames whose id is currently being computed.  Reinflation only really
>> makes sense for the "users" of the frame API, those who receive a frame
>> with a computed id.  And by experience, this is where the "using stale
>> frame_info" bugs happen, a user holding a frame_info across a target
>> resumption.  If a frame cache reinit was to happen in the middle of a
>> frame id computation, I think things would be seriously wrong.  After
>> the reinit, we'd be computing an id for a frame_info that doesn't exist
>> anymore, I don't think it make sense.
>>
>> There are many functions that can be used in both contexts (frames with
>> ids computed, and frames with ids being computed).  value_of_register,
>> for instance.  These functions must be changed back to take a raw
>> frame_info pointer.  But to make things convenient to callers holding a
>> frame_info_ptr, and to avoid having to change too many call sites, I
>> introduced wrappers like this:
>>
>>      static inline value *
>>      value_of_register (int regnum, frame_info_ptr frame)
>>      {
>>        return value_of_register (regnum, frame.get ());
>>      }
>>
>> Since frame_info_ptr::get will, at the end of this series, automatically
>> reinflate the wrapped frame_info object, if necessary, the caller still
>> gets the "protection" offered by frame_info_ptr, at least up to that
>> point.  Now, if a frame cache reinit was to happen inside that
>> get_prev_frame call, we'd be screwed.  But I think the risk of it
>> happening is pretty low, for this kind of functions.  And remember,
>> since get_prev_frame is also used in the context of frame id
>> computation, it's hard to imagine that it could happen.
> 
> s/get_prev_frame/value_of_register ? or am I missing something?
> 
> Regardless, your logic seems solid. I feel like a wrapper like this is better than a second class, unless we start seeing the issues you theorized.
> 
>>
>> Now, we could have two separate class frame_info wrapper classes, one
>> that is today's frame_info_ptr, whose goal is just to catch uses of
>> stale frame_info objects, and one that is the one at the end of this
>> series, whose goal is to do automatic reinflation.  The former one could
>> continue being used in the paths involved in frame id computation.
>> However, I'm not convinced it is worth it, due to (1) the low risk of
>> this problem happening in these paths and (2) the availability of tools
>> like ASan or Valgrind that will tell you precisely when a use-after-free
>> happens.
>>
>> There are no user-visible changes expected with this patch.  I
>> built-tested and regression-tested on Linux x86-64.  I don't think any
>> platform-specific nat file needs to be changed, as they don't deal with
>> frames.
> 
> For the patch itself, I took a look at it and saw nothing wrong, but I'm not confident I understand the code well enough to say it is ok. This is all I got:
> 
> Tested-By: Bruno Larsen <blarsen@redhat.com>

Hi Bruno,

This patch will likely be dropped from the next version (see the thread with Tom).

Simon