Re: [PATCH 3/6] gdb: make user-created frames reinflatable

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

On 12/12/22 08:33, Bruno Larsen wrote:
> On 12/12/2022 14:17, Simon Marchi wrote:
>>
>>>> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
>>>> index 40a872ea152d..d61fb7ed0e95 100644
>>>> --- a/gdb/frame-info.c
>>>> +++ b/gdb/frame-info.c
>>>> @@ -21,6 +21,9 @@
>>>>      #include "frame-info.h"
>>>>    #include "frame.h"
>>>> +#include "gdbsupport/selftest.h"
>>>> +#include "scoped-mock-context.h"
>>>> +#include "test-target.h"
>>>>      /* See frame-info-ptr.h.  */
>>>>    @@ -33,7 +36,8 @@ frame_info_ptr::prepare_reinflate ()
>>>>    {
>>>>      m_cached_level = frame_relative_level (*this);
>>>>    -  if (m_cached_level != 0)
>>>> +  if (m_cached_level != 0
>>>> +      || (m_ptr != nullptr && frame_is_user_created (m_ptr)))
>>>>        m_cached_id = get_frame_id (*this);
>>>>    }
>>>>    @@ -54,7 +58,13 @@ frame_info_ptr::reinflate ()
>>>>        /* Frame #0 needs special handling, see comment in select_frame.  */
>>>>      if (m_cached_level == 0)
>>>> -    m_ptr = get_current_frame ().get ();
>>>> +    {
>>>> +      if (!frame_id_p (m_cached_id))
>>> You seem to be using the stack being valid to check if the frame is
>> stack -> frame id?
> 
> No, I meant stack because frame_id_p checks stack_status == FID_STACK_INVALID.
> 
>>
>>> user created, but in the commit message you mention that you use null
>>> frame id. Wouldn't it be more reliable to check if m_cached_id ==
>>> null_frame_id ?
>> Comparing anything against null_frame_id (even a null frame id) yields
>> false:
>>
>>    if (stack_status == FID_STACK_INVALID
>>        || r.stack_status == FID_STACK_INVALID)
>>      /* Like a NaN, if either ID is invalid, the result is false.
>>         Note that a frame ID is invalid iff it is the null frame ID.  */
>>      eq = false;
>>
>> From: https://gitlab.com/gnutools/binutils-gdb/-/blob/a28fedbc3f582ce7c8bad2eb017b1dc072bb1da7/gdb/frame.c#L759-763
>>
>> I don't really understand the point, I think it would be useful to be
>> able to compare a frame id to null_frame_id, like we compare pointers to
>> nullptr.  But currently, the correct (and only?) way of checking if we
>> have a frame id or not in a frame_id is frame_id_p.
> 
> Ah right, that explains it. I think we could add a comment to frame_id_p to make it more clear for newer contributors like me.
> 
> However, it isn't the only way to check if a frame_id is null, we can use the fact that null_frame_id != null_frame_id and check for:
> 
> if (m_cached_id != m_cached_id)
>   /* get current frame */
> else
>   /* recreate user frame */
> 
> This is (apparently) very common for folks using Javascript to check if something is NaN. I personally find it hideous and would much prefer checking against null_frame_id directly. I just figured I might as well throw this cursed knowledge out there :)

Huh, I didn't think of that!  I don't like it either, it really doesn't
convey the intent.

Simon