[7/7] gdb: add special handling for frame level 0 in frame_info_ptr

  From: Simon Marchi <simon.marchi@efficios.com>

I noticed this problem while preparing the initial submission for the
ROCm GDB port.  One particularity of this patch set is that it does not
support unwinding frames, that requires support of some DWARF extensions
that will come later.  It was still possible to run to a breakpoint and
print frame #0, though.

When rebasing on top of the frame_info_ptr work, GDB started tripping on
a prepare_reinflate call, making it not possible anymore to event print
the frame when stopping on a breakpoint.  One thing to know about frame
0 is that its id is lazily computed when something requests it through
get_frame_id.  See:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L2070-2080

So, up to that prepare_reinflate call, frame 0's id was not computed,
and prepare_reinflate, calling get_frame_id, forces it to be computed.
Computing the frame id generally requires unwinding the previous frame,
which with my ROCm GDB patch fails.  An exception is thrown and the
printing of the frame is simply abandonned.

Regardless of this ROCm GDB problem (which is admittedly temporary, it
will be possible to unwind with subsequent patches), we want to avoid
prepare_reinflate to force the computing of the frame id, for the same
reasons we lazily compute it in the first place.

In addition, frame 0's id is subject to change across a frame cache
reset.  This is why save_selected_frame and restore_selected_frame have
special handling for frame 0:

  https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L1841-1863

For this last reason, we also need to handle frame 0 specially in
prepare_reinflate / reinflate.  Because the frame id of frame 0 can
change across a frame cache reset, we must not rely on the frame id from
that frame to reinflate it.  We should instead just re-fetch the current
frame at that point.

This patch adds a frame_info_ptr::m_cached_level field, set in
frame_info_ptr::prepare_reinflate, so we can tell if a frame is frame 0.
There are cases where a frame_info_ptr object wraps a sentinel frame,
for which frame_relative_level returns -1, so I have chosen the value -2
to represent "invalid frame level", for when the frame_info_ptr object
is empty.

In frame_info_ptr::prepare_reinflate, only cache the frame id if the
frame level is not 0.  It's fine to cache the frame id for the sentinel
frame, it will be properly handled by frame_find_by_id later.

In frame_info_ptr::reinflate, if the frame level is 0, call
get_current_frame to get the target's current frame.  Otherwise, use
frame_find_by_id just as before.

This patch should not have user-visible changes with upstream GDB.  But
it will avoid forcing the computation of frame 0's when calling
prepare_reinflate.  And, well, it fixes the upcoming ROCm GDB patch
series.

Change-Id: I176ed7ee9317ddbb190acee8366e087e08e4d266
---
 gdb/frame-info.c | 24 ++++++++++++++++++++----
 gdb/frame-info.h | 20 ++++++++++++++++++--
 2 files changed, 38 insertions(+), 6 deletions(-)
  

On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> I noticed this problem while preparing the initial submission for the
> ROCm GDB port.  One particularity of this patch set is that it does not
> support unwinding frames, that requires support of some DWARF extensions
> that will come later.  It was still possible to run to a breakpoint and
> print frame #0, though.
>
> When rebasing on top of the frame_info_ptr work, GDB started tripping on
> a prepare_reinflate call, making it not possible anymore to event print
> the frame when stopping on a breakpoint.  One thing to know about frame
> 0 is that its id is lazily computed when something requests it through
> get_frame_id.  See:
>
>    https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L2070-2080
>
> So, up to that prepare_reinflate call, frame 0's id was not computed,
> and prepare_reinflate, calling get_frame_id, forces it to be computed.
> Computing the frame id generally requires unwinding the previous frame,
> which with my ROCm GDB patch fails.  An exception is thrown and the
> printing of the frame is simply abandonned.
>
> Regardless of this ROCm GDB problem (which is admittedly temporary, it
> will be possible to unwind with subsequent patches), we want to avoid
> prepare_reinflate to force the computing of the frame id, for the same
> reasons we lazily compute it in the first place.
>
> In addition, frame 0's id is subject to change across a frame cache
> reset.  This is why save_selected_frame and restore_selected_frame have
> special handling for frame 0:
>
>    https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L1841-1863
>
> For this last reason, we also need to handle frame 0 specially in
> prepare_reinflate / reinflate.  Because the frame id of frame 0 can
> change across a frame cache reset, we must not rely on the frame id from
> that frame to reinflate it.  We should instead just re-fetch the current
> frame at that point.
>
> This patch adds a frame_info_ptr::m_cached_level field, set in
> frame_info_ptr::prepare_reinflate, so we can tell if a frame is frame 0.
> There are cases where a frame_info_ptr object wraps a sentinel frame,
> for which frame_relative_level returns -1, so I have chosen the value -2
> to represent "invalid frame level", for when the frame_info_ptr object
> is empty.
>
> In frame_info_ptr::prepare_reinflate, only cache the frame id if the
> frame level is not 0.  It's fine to cache the frame id for the sentinel
> frame, it will be properly handled by frame_find_by_id later.
>
> In frame_info_ptr::reinflate, if the frame level is 0, call
> get_current_frame to get the target's current frame.  Otherwise, use
> frame_find_by_id just as before.
>
> This patch should not have user-visible changes with upstream GDB.  But
> it will avoid forcing the computation of frame 0's when calling
> prepare_reinflate.  And, well, it fixes the upcoming ROCm GDB patch
> series.
>
> Change-Id: I176ed7ee9317ddbb190acee8366e087e08e4d266

This all makes sense. I have a small style preference below, but even if 
you dislike it, the code is still fine.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

> ---
>   gdb/frame-info.c | 24 ++++++++++++++++++++----
>   gdb/frame-info.h | 20 ++++++++++++++++++--
>   2 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
> index 584222dc490f..e3ee9f8174e1 100644
> --- a/gdb/frame-info.c
> +++ b/gdb/frame-info.c
> @@ -31,7 +31,11 @@ intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
>   void
>   frame_info_ptr::prepare_reinflate ()
>   {
> -  m_cached_id = get_frame_id (*this);
> +  m_cached_level = frame_relative_level (*this);
> +  gdb_assert (m_cached_level >= -1);

Since you have declared invalid_level = -2 for this class, I feel like 
it would be more better to have the assert be

gdb_assert (m_cached_level > invalid_level);

This way there is no need to wonder why -1 is a valid level, and makes 
it easier to grep for the comment in the file, should someone want to know.

> +
> +  if (m_cached_level != 0)
> +    m_cached_id = get_frame_id (*this);
>   }
>   
>   /* See frame-info-ptr.h.  */
> @@ -39,9 +43,21 @@ frame_info_ptr::prepare_reinflate ()
>   void
>   frame_info_ptr::reinflate ()
>   {
> -  gdb_assert (frame_id_p (m_cached_id));
> +  gdb_assert (m_cached_level >= -1);
Likewise
  

On 11/8/22 05:40, Bruno Larsen wrote:
> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> I noticed this problem while preparing the initial submission for the
>> ROCm GDB port.  One particularity of this patch set is that it does not
>> support unwinding frames, that requires support of some DWARF extensions
>> that will come later.  It was still possible to run to a breakpoint and
>> print frame #0, though.
>>
>> When rebasing on top of the frame_info_ptr work, GDB started tripping on
>> a prepare_reinflate call, making it not possible anymore to event print
>> the frame when stopping on a breakpoint.  One thing to know about frame
>> 0 is that its id is lazily computed when something requests it through
>> get_frame_id.  See:
>>
>>    https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L2070-2080
>>
>> So, up to that prepare_reinflate call, frame 0's id was not computed,
>> and prepare_reinflate, calling get_frame_id, forces it to be computed.
>> Computing the frame id generally requires unwinding the previous frame,
>> which with my ROCm GDB patch fails.  An exception is thrown and the
>> printing of the frame is simply abandonned.
>>
>> Regardless of this ROCm GDB problem (which is admittedly temporary, it
>> will be possible to unwind with subsequent patches), we want to avoid
>> prepare_reinflate to force the computing of the frame id, for the same
>> reasons we lazily compute it in the first place.
>>
>> In addition, frame 0's id is subject to change across a frame cache
>> reset.  This is why save_selected_frame and restore_selected_frame have
>> special handling for frame 0:
>>
>>    https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L1841-1863
>>
>> For this last reason, we also need to handle frame 0 specially in
>> prepare_reinflate / reinflate.  Because the frame id of frame 0 can
>> change across a frame cache reset, we must not rely on the frame id from
>> that frame to reinflate it.  We should instead just re-fetch the current
>> frame at that point.
>>
>> This patch adds a frame_info_ptr::m_cached_level field, set in
>> frame_info_ptr::prepare_reinflate, so we can tell if a frame is frame 0.
>> There are cases where a frame_info_ptr object wraps a sentinel frame,
>> for which frame_relative_level returns -1, so I have chosen the value -2
>> to represent "invalid frame level", for when the frame_info_ptr object
>> is empty.
>>
>> In frame_info_ptr::prepare_reinflate, only cache the frame id if the
>> frame level is not 0.  It's fine to cache the frame id for the sentinel
>> frame, it will be properly handled by frame_find_by_id later.
>>
>> In frame_info_ptr::reinflate, if the frame level is 0, call
>> get_current_frame to get the target's current frame.  Otherwise, use
>> frame_find_by_id just as before.
>>
>> This patch should not have user-visible changes with upstream GDB.  But
>> it will avoid forcing the computation of frame 0's when calling
>> prepare_reinflate.  And, well, it fixes the upcoming ROCm GDB patch
>> series.
>>
>> Change-Id: I176ed7ee9317ddbb190acee8366e087e08e4d266
> 
> This all makes sense. I have a small style preference below, but even if you dislike it, the code is still fine.
> 
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>
> 
>> ---
>>   gdb/frame-info.c | 24 ++++++++++++++++++++----
>>   gdb/frame-info.h | 20 ++++++++++++++++++--
>>   2 files changed, 38 insertions(+), 6 deletions(-)
>>
>> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
>> index 584222dc490f..e3ee9f8174e1 100644
>> --- a/gdb/frame-info.c
>> +++ b/gdb/frame-info.c
>> @@ -31,7 +31,11 @@ intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
>>   void
>>   frame_info_ptr::prepare_reinflate ()
>>   {
>> -  m_cached_id = get_frame_id (*this);
>> +  m_cached_level = frame_relative_level (*this);
>> +  gdb_assert (m_cached_level >= -1);
> 
> Since you have declared invalid_level = -2 for this class, I feel like it would be more better to have the assert be
> 
> gdb_assert (m_cached_level > invalid_level);

This form assumes that invalid_level is -2, defeating the purpose to
have the abstraction in the first place.  If we changed invalid_level to
be INT_MAX, for intsance, the assertion wouldn't be right anymore.

> This way there is no need to wonder why -1 is a valid level, and makes it easier to grep for the comment in the file, should someone want to know.

In my vision of things, the sentinel frame having level -1 is well
known, because it's the frame just below the current frame, which is
known to have level 0.  So while it looks like a magic random value,
it's not really.  The numerical value has a meaning.  We wouldn't want
to change the sentinel frame's level value to be any other arbitrary
numerical value.

Here, I can just drop the assert.  It's basically just checking that
frame_relative_level didn't return something that doesn't make sense.
But there's no reason for frame_relative_level to return something that
doesn't make sense in the first place.  Other callers of
frame_relative_level don't do this assert, they just trust that
frame_relative_level returns something that makes sense, nothing really
different here.

>> +
>> +  if (m_cached_level != 0)
>> +    m_cached_id = get_frame_id (*this);
>>   }
>>     /* See frame-info-ptr.h.  */
>> @@ -39,9 +43,21 @@ frame_info_ptr::prepare_reinflate ()
>>   void
>>   frame_info_ptr::reinflate ()
>>   {
>> -  gdb_assert (frame_id_p (m_cached_id));
>> +  gdb_assert (m_cached_level >= -1);
> Likewise

Here, I could add a comment like:

  /* Ensure we have a valid frame level, indicating prepare_reinflate
     was called.  */

Simon
  

On 08/11/2022 17:19, Simon Marchi wrote:
> On 11/8/22 05:40, Bruno Larsen wrote:
>> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>>> From: Simon Marchi <simon.marchi@efficios.com>
>>>
>>> I noticed this problem while preparing the initial submission for the
>>> ROCm GDB port.  One particularity of this patch set is that it does not
>>> support unwinding frames, that requires support of some DWARF extensions
>>> that will come later.  It was still possible to run to a breakpoint and
>>> print frame #0, though.
>>>
>>> When rebasing on top of the frame_info_ptr work, GDB started tripping on
>>> a prepare_reinflate call, making it not possible anymore to event print
>>> the frame when stopping on a breakpoint.  One thing to know about frame
>>> 0 is that its id is lazily computed when something requests it through
>>> get_frame_id.  See:
>>>
>>>     https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L2070-2080
>>>
>>> So, up to that prepare_reinflate call, frame 0's id was not computed,
>>> and prepare_reinflate, calling get_frame_id, forces it to be computed.
>>> Computing the frame id generally requires unwinding the previous frame,
>>> which with my ROCm GDB patch fails.  An exception is thrown and the
>>> printing of the frame is simply abandonned.
>>>
>>> Regardless of this ROCm GDB problem (which is admittedly temporary, it
>>> will be possible to unwind with subsequent patches), we want to avoid
>>> prepare_reinflate to force the computing of the frame id, for the same
>>> reasons we lazily compute it in the first place.
>>>
>>> In addition, frame 0's id is subject to change across a frame cache
>>> reset.  This is why save_selected_frame and restore_selected_frame have
>>> special handling for frame 0:
>>>
>>>     https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L1841-1863
>>>
>>> For this last reason, we also need to handle frame 0 specially in
>>> prepare_reinflate / reinflate.  Because the frame id of frame 0 can
>>> change across a frame cache reset, we must not rely on the frame id from
>>> that frame to reinflate it.  We should instead just re-fetch the current
>>> frame at that point.
>>>
>>> This patch adds a frame_info_ptr::m_cached_level field, set in
>>> frame_info_ptr::prepare_reinflate, so we can tell if a frame is frame 0.
>>> There are cases where a frame_info_ptr object wraps a sentinel frame,
>>> for which frame_relative_level returns -1, so I have chosen the value -2
>>> to represent "invalid frame level", for when the frame_info_ptr object
>>> is empty.
>>>
>>> In frame_info_ptr::prepare_reinflate, only cache the frame id if the
>>> frame level is not 0.  It's fine to cache the frame id for the sentinel
>>> frame, it will be properly handled by frame_find_by_id later.
>>>
>>> In frame_info_ptr::reinflate, if the frame level is 0, call
>>> get_current_frame to get the target's current frame.  Otherwise, use
>>> frame_find_by_id just as before.
>>>
>>> This patch should not have user-visible changes with upstream GDB.  But
>>> it will avoid forcing the computation of frame 0's when calling
>>> prepare_reinflate.  And, well, it fixes the upcoming ROCm GDB patch
>>> series.
>>>
>>> Change-Id: I176ed7ee9317ddbb190acee8366e087e08e4d266
>> This all makes sense. I have a small style preference below, but even if you dislike it, the code is still fine.
>>
>> Reviewed-By: Bruno Larsen <blarsen@redhat.com>
>>
>>> ---
>>>    gdb/frame-info.c | 24 ++++++++++++++++++++----
>>>    gdb/frame-info.h | 20 ++++++++++++++++++--
>>>    2 files changed, 38 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
>>> index 584222dc490f..e3ee9f8174e1 100644
>>> --- a/gdb/frame-info.c
>>> +++ b/gdb/frame-info.c
>>> @@ -31,7 +31,11 @@ intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
>>>    void
>>>    frame_info_ptr::prepare_reinflate ()
>>>    {
>>> -  m_cached_id = get_frame_id (*this);
>>> +  m_cached_level = frame_relative_level (*this);
>>> +  gdb_assert (m_cached_level >= -1);
>> Since you have declared invalid_level = -2 for this class, I feel like it would be more better to have the assert be
>>
>> gdb_assert (m_cached_level > invalid_level);
> This form assumes that invalid_level is -2, defeating the purpose to
> have the abstraction in the first place.  If we changed invalid_level to
> be INT_MAX, for intsance, the assertion wouldn't be right anymore.
>
>> This way there is no need to wonder why -1 is a valid level, and makes it easier to grep for the comment in the file, should someone want to know.
> In my vision of things, the sentinel frame having level -1 is well
> known, because it's the frame just below the current frame, which is
> known to have level 0.  So while it looks like a magic random value,
> it's not really.  The numerical value has a meaning.  We wouldn't want
> to change the sentinel frame's level value to be any other arbitrary
> numerical value.
Ok, I see your point. It does make sense when you put it like that.
>
> Here, I can just drop the assert.  It's basically just checking that
> frame_relative_level didn't return something that doesn't make sense.
> But there's no reason for frame_relative_level to return something that
> doesn't make sense in the first place.  Other callers of
> frame_relative_level don't do this assert, they just trust that
> frame_relative_level returns something that makes sense, nothing really
> different here.
>
>>> +
>>> +  if (m_cached_level != 0)
>>> +    m_cached_id = get_frame_id (*this);
>>>    }
>>>      /* See frame-info-ptr.h.  */
>>> @@ -39,9 +43,21 @@ frame_info_ptr::prepare_reinflate ()
>>>    void
>>>    frame_info_ptr::reinflate ()
>>>    {
>>> -  gdb_assert (frame_id_p (m_cached_id));
>>> +  gdb_assert (m_cached_level >= -1);
>> Likewise
> Here, I could add a comment like:
>
>    /* Ensure we have a valid frame level, indicating prepare_reinflate
>       was called.  */

Yeah, this comment fixes any possible confusion. You convinced me :-)

Reviewed-By: Bruno Larsen <blarsen@redhat.com>
  

On 11/10/22 11:28, Bruno Larsen wrote:
> On 08/11/2022 17:19, Simon Marchi wrote:
>> On 11/8/22 05:40, Bruno Larsen wrote:
>>> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>>>> From: Simon Marchi <simon.marchi@efficios.com>
>>>>
>>>> I noticed this problem while preparing the initial submission for the
>>>> ROCm GDB port.  One particularity of this patch set is that it does not
>>>> support unwinding frames, that requires support of some DWARF extensions
>>>> that will come later.  It was still possible to run to a breakpoint and
>>>> print frame #0, though.
>>>>
>>>> When rebasing on top of the frame_info_ptr work, GDB started tripping on
>>>> a prepare_reinflate call, making it not possible anymore to event print
>>>> the frame when stopping on a breakpoint.  One thing to know about frame
>>>> 0 is that its id is lazily computed when something requests it through
>>>> get_frame_id.  See:
>>>>
>>>>     https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L2070-2080
>>>>
>>>> So, up to that prepare_reinflate call, frame 0's id was not computed,
>>>> and prepare_reinflate, calling get_frame_id, forces it to be computed.
>>>> Computing the frame id generally requires unwinding the previous frame,
>>>> which with my ROCm GDB patch fails.  An exception is thrown and the
>>>> printing of the frame is simply abandonned.
>>>>
>>>> Regardless of this ROCm GDB problem (which is admittedly temporary, it
>>>> will be possible to unwind with subsequent patches), we want to avoid
>>>> prepare_reinflate to force the computing of the frame id, for the same
>>>> reasons we lazily compute it in the first place.
>>>>
>>>> In addition, frame 0's id is subject to change across a frame cache
>>>> reset.  This is why save_selected_frame and restore_selected_frame have
>>>> special handling for frame 0:
>>>>
>>>>     https://gitlab.com/gnutools/binutils-gdb/-/blob/23912acd402f5af9caf91b257e5209ec4c58a09c/gdb/frame.c#L1841-1863
>>>>
>>>> For this last reason, we also need to handle frame 0 specially in
>>>> prepare_reinflate / reinflate.  Because the frame id of frame 0 can
>>>> change across a frame cache reset, we must not rely on the frame id from
>>>> that frame to reinflate it.  We should instead just re-fetch the current
>>>> frame at that point.
>>>>
>>>> This patch adds a frame_info_ptr::m_cached_level field, set in
>>>> frame_info_ptr::prepare_reinflate, so we can tell if a frame is frame 0.
>>>> There are cases where a frame_info_ptr object wraps a sentinel frame,
>>>> for which frame_relative_level returns -1, so I have chosen the value -2
>>>> to represent "invalid frame level", for when the frame_info_ptr object
>>>> is empty.
>>>>
>>>> In frame_info_ptr::prepare_reinflate, only cache the frame id if the
>>>> frame level is not 0.  It's fine to cache the frame id for the sentinel
>>>> frame, it will be properly handled by frame_find_by_id later.
>>>>
>>>> In frame_info_ptr::reinflate, if the frame level is 0, call
>>>> get_current_frame to get the target's current frame.  Otherwise, use
>>>> frame_find_by_id just as before.
>>>>
>>>> This patch should not have user-visible changes with upstream GDB.  But
>>>> it will avoid forcing the computation of frame 0's when calling
>>>> prepare_reinflate.  And, well, it fixes the upcoming ROCm GDB patch
>>>> series.
>>>>
>>>> Change-Id: I176ed7ee9317ddbb190acee8366e087e08e4d266
>>> This all makes sense. I have a small style preference below, but even if you dislike it, the code is still fine.
>>>
>>> Reviewed-By: Bruno Larsen <blarsen@redhat.com>
>>>
>>>> ---
>>>>    gdb/frame-info.c | 24 ++++++++++++++++++++----
>>>>    gdb/frame-info.h | 20 ++++++++++++++++++--
>>>>    2 files changed, 38 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/gdb/frame-info.c b/gdb/frame-info.c
>>>> index 584222dc490f..e3ee9f8174e1 100644
>>>> --- a/gdb/frame-info.c
>>>> +++ b/gdb/frame-info.c
>>>> @@ -31,7 +31,11 @@ intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
>>>>    void
>>>>    frame_info_ptr::prepare_reinflate ()
>>>>    {
>>>> -  m_cached_id = get_frame_id (*this);
>>>> +  m_cached_level = frame_relative_level (*this);
>>>> +  gdb_assert (m_cached_level >= -1);
>>> Since you have declared invalid_level = -2 for this class, I feel like it would be more better to have the assert be
>>>
>>> gdb_assert (m_cached_level > invalid_level);
>> This form assumes that invalid_level is -2, defeating the purpose to
>> have the abstraction in the first place.  If we changed invalid_level to
>> be INT_MAX, for intsance, the assertion wouldn't be right anymore.
>>
>>> This way there is no need to wonder why -1 is a valid level, and makes it easier to grep for the comment in the file, should someone want to know.
>> In my vision of things, the sentinel frame having level -1 is well
>> known, because it's the frame just below the current frame, which is
>> known to have level 0.  So while it looks like a magic random value,
>> it's not really.  The numerical value has a meaning.  We wouldn't want
>> to change the sentinel frame's level value to be any other arbitrary
>> numerical value.
> Ok, I see your point. It does make sense when you put it like that.
>>
>> Here, I can just drop the assert.  It's basically just checking that
>> frame_relative_level didn't return something that doesn't make sense.
>> But there's no reason for frame_relative_level to return something that
>> doesn't make sense in the first place.  Other callers of
>> frame_relative_level don't do this assert, they just trust that
>> frame_relative_level returns something that makes sense, nothing really
>> different here.
>>
>>>> +
>>>> +  if (m_cached_level != 0)
>>>> +    m_cached_id = get_frame_id (*this);
>>>>    }
>>>>      /* See frame-info-ptr.h.  */
>>>> @@ -39,9 +43,21 @@ frame_info_ptr::prepare_reinflate ()
>>>>    void
>>>>    frame_info_ptr::reinflate ()
>>>>    {
>>>> -  gdb_assert (frame_id_p (m_cached_id));
>>>> +  gdb_assert (m_cached_level >= -1);
>>> Likewise
>> Here, I could add a comment like:
>>
>>    /* Ensure we have a valid frame level, indicating prepare_reinflate
>>       was called.  */
> 
> Yeah, this comment fixes any possible confusion. You convinced me :-)
> 
> Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Thanks to you and Tom for your review on all patches, I will push
shortly.

Simon