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

[Bruno Larsen <blarsen@redhat.com>]

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 :)

>
> When the commit message says:
>
>    for user-created frames, m_cached_id is a non-null frame id, whereas
>    for the current target frame, m_cached_id is left null.
>
> I could say "is valid" and "is invalid" instead.
No, I think the message is fine, I was mostly getting confused about the 
use of frame_id_p.
>
> Simon
>

-- 
Cheers,
Bruno