[PATCH 1/7] gdb: clear other.m_cached_id in frame_info_ptr's move ctor

[PATCH 1/7] gdb: clear other.m_cached_id in frame_info_ptr's move ctor

[Simon Marchi]

We do it in the move assignment operator, so I think it makes sense to
do it here too for consistency.  I don't think it's absolutely necessary
to clear the other object's fields (in other words, copy constructor and
move constructor could be the same), as there is no exclusive resource
being transfered.  The important thing is to leave the moved-from object
in an unknown, but valid state.  But still, I think that clearing the
fields of the moved-from object is not a bad idea, it helps ensure we
don't rely on the moved-from object after.

Change-Id: Iee900ff9d25dad51d62765d694f2e01524351340
---
 gdb/frame-info.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdb/frame-info.h b/gdb/frame-info.h
index 665f4bdae3bf..7159f82b1962 100644
--- a/gdb/frame-info.h
+++ b/gdb/frame-info.h
@@ -71,6 +71,7 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
     : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id)
   {
     other.m_ptr = nullptr;
+    other.m_cached_id = null_frame_id;
     frame_list.push_back (*this);
   }
 

base-commit: 240e07bd94a8da9270c57cde394f6883e43b8497
-- 
2.38.1

[PATCH 2/7] gdb: add prepare_reinflate/reinflate around print_frame_args in info_frame_command_core

[Simon Marchi]

I noticed this crash:

    $ ./gdb --data-directory=data-directory -nx -q \
          testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand \
	  -x testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand.py \
	  -ex "b g" -ex r
    (gdb) info frame
    Stack level 0, frame at 0x7fffffffdd80:
     rip = 0x555555555160 in g
        (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c:41); saved rip = 0x5555555551a3
     called by frame at 0x7fffffffdda0
     source language c.
     Arglist at 0x7fffffffdd70, args: mt=mytype is 0x555555556004 "hello world",
        depth=10

    Fatal signal: Segmentation fault

This is another case of frame_info being invalidated under a function's
feet.  The stack trace when the frame_info get invalidated looks like:

    ... many frames to pretty print the arg, that eventually invalidate the frame_infos ...
    #35 0x00005568d0a8ab24 in print_frame_arg (fp_opts=..., arg=0x7ffc3216bcb0) at /home/simark/src/binutils-gdb/gdb/stack.c:489
    #36 0x00005568d0a8cc75 in print_frame_args (fp_opts=..., func=0x621000233210, frame=..., num=-1, stream=0x60b000000300)
        at /home/simark/src/binutils-gdb/gdb/stack.c:898
    #37 0x00005568d0a9536d in info_frame_command_core (fi=..., selected_frame_p=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1682

print_frame_args knows that print_frame_arg can invalidate frame_info
objects, and therefore calls prepare_reinflate/reinflate.  However,
info_frame_command_core has a separate frame_info_ptr instance (it is
passed by value / copy).  So info_frame_command_core needs to know that
print_frame_args can invalidate frame_info objects, and therefore needs
to prepare_reinflate/reinflate as well.  Add those calls, and enhance
the gdb.python/pretty-print-call-by-hand.exp test to test that command.

Change-Id: I9edaae06d62e97ffdb30938d364437737238a960
---
 gdb/stack.c                                            | 4 ++++
 gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp | 8 ++++++++
 2 files changed, 12 insertions(+)

diff --git a/gdb/stack.c b/gdb/stack.c
index 653251c200b4..4e2342c2a8d9 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1679,8 +1679,12 @@ info_frame_command_core (frame_info_ptr fi, bool selected_frame_p)
 	    else
 	      gdb_printf (" %d args: ", numargs);
 	  }
+
+	fi.prepare_reinflate ();
 	print_frame_args (user_frame_print_options,
 			  func, fi, numargs, gdb_stdout);
+	fi.reinflate ();
+
 	gdb_puts ("\n");
       }
   }
diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
index 0aeb2218f911..eb3fc9e35faf 100644
--- a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
@@ -98,6 +98,14 @@ with_test_prefix "frame print" {
 	"backtrace test"
     }
 }
+
+# Test the "info frame" command
+with_test_prefix "info frame" {
+    if { [start_test "TAG: first frame"] == 0 } {
+	gdb_test "info frame" "mytype is $hex \"hello world\".*"
+    }
+}
+
 # Testing the down command.
 with_test_prefix "frame movement down" {
     if { [start_test "TAG: first frame"] == 0 } {
-- 
2.38.1

Re: [PATCH 2/7] gdb: add prepare_reinflate/reinflate around print_frame_args in info_frame_command_core

[Bruno Larsen]

On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
> I noticed this crash:
>
>      $ ./gdb --data-directory=data-directory -nx -q \
>            testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand \
> 	  -x testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand.py \
> 	  -ex "b g" -ex r
>      (gdb) info frame
>      Stack level 0, frame at 0x7fffffffdd80:
>       rip = 0x555555555160 in g
>          (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c:41); saved rip = 0x5555555551a3
>       called by frame at 0x7fffffffdda0
>       source language c.
>       Arglist at 0x7fffffffdd70, args: mt=mytype is 0x555555556004 "hello world",
>          depth=10
>
>      Fatal signal: Segmentation fault
>
> This is another case of frame_info being invalidated under a function's
> feet.  The stack trace when the frame_info get invalidated looks like:
>
>      ... many frames to pretty print the arg, that eventually invalidate the frame_infos ...
>      #35 0x00005568d0a8ab24 in print_frame_arg (fp_opts=..., arg=0x7ffc3216bcb0) at /home/simark/src/binutils-gdb/gdb/stack.c:489
>      #36 0x00005568d0a8cc75 in print_frame_args (fp_opts=..., func=0x621000233210, frame=..., num=-1, stream=0x60b000000300)
>          at /home/simark/src/binutils-gdb/gdb/stack.c:898
>      #37 0x00005568d0a9536d in info_frame_command_core (fi=..., selected_frame_p=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1682
>
> print_frame_args knows that print_frame_arg can invalidate frame_info
> objects, and therefore calls prepare_reinflate/reinflate.  However,
> info_frame_command_core has a separate frame_info_ptr instance (it is
> passed by value / copy).  So info_frame_command_core needs to know that
> print_frame_args can invalidate frame_info objects, and therefore needs
> to prepare_reinflate/reinflate as well.  Add those calls, and enhance
> the gdb.python/pretty-print-call-by-hand.exp test to test that command.

Can confirm that the crash is reproducible and that the patch fixes the 
problem. Sorry for missing it the first time. Makes me wonder if I also 
missed this in print_frame... Either way:

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

-- 
Cheers,
Bruno

Re: [PATCH 2/7] gdb: add prepare_reinflate/reinflate around print_frame_args in info_frame_command_core

[Simon Marchi]

On 11/8/22 04:32, Bruno Larsen wrote:
> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>> I noticed this crash:
>>
>>      $ ./gdb --data-directory=data-directory -nx -q \
>>            testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand \
>>       -x testsuite/outputs/gdb.python/pretty-print-call-by-hand/pretty-print-call-by-hand.py \
>>       -ex "b g" -ex r
>>      (gdb) info frame
>>      Stack level 0, frame at 0x7fffffffdd80:
>>       rip = 0x555555555160 in g
>>          (/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c:41); saved rip = 0x5555555551a3
>>       called by frame at 0x7fffffffdda0
>>       source language c.
>>       Arglist at 0x7fffffffdd70, args: mt=mytype is 0x555555556004 "hello world",
>>          depth=10
>>
>>      Fatal signal: Segmentation fault
>>
>> This is another case of frame_info being invalidated under a function's
>> feet.  The stack trace when the frame_info get invalidated looks like:
>>
>>      ... many frames to pretty print the arg, that eventually invalidate the frame_infos ...
>>      #35 0x00005568d0a8ab24 in print_frame_arg (fp_opts=..., arg=0x7ffc3216bcb0) at /home/simark/src/binutils-gdb/gdb/stack.c:489
>>      #36 0x00005568d0a8cc75 in print_frame_args (fp_opts=..., func=0x621000233210, frame=..., num=-1, stream=0x60b000000300)
>>          at /home/simark/src/binutils-gdb/gdb/stack.c:898
>>      #37 0x00005568d0a9536d in info_frame_command_core (fi=..., selected_frame_p=true) at /home/simark/src/binutils-gdb/gdb/stack.c:1682
>>
>> print_frame_args knows that print_frame_arg can invalidate frame_info
>> objects, and therefore calls prepare_reinflate/reinflate.  However,
>> info_frame_command_core has a separate frame_info_ptr instance (it is
>> passed by value / copy).  So info_frame_command_core needs to know that
>> print_frame_args can invalidate frame_info objects, and therefore needs
>> to prepare_reinflate/reinflate as well.  Add those calls, and enhance
>> the gdb.python/pretty-print-call-by-hand.exp test to test that command.
> 
> Can confirm that the crash is reproducible and that the patch fixes the problem. Sorry for missing it the first time. Makes me wonder if I also missed this in print_frame... Either way:

I checked print_frame, it could use prepare_reinflate/reinflate.  But
the only way for the frame variable to be re-used after the
print_frame_args call is if:

   (funname == NULL || sal.symtab == NULL)

If that expression is true, then it's likely that we don't have debug
info for the function, so it's not likely that some pretty printer was
used.

But this shows how the current frame_info_ptr is error-prone: you have
two know which functions can, deep down their call tree, reinit the
frame cache.  And all their callers that have a frame_info_ptr object,
recursively, must explicitly do prepare_reinflate / reinflate to protect
themselves against their frame_info object being invalidated.  It's very
easy to forget some spots.

I'm currently working on making frame_info_ptr work automatically,
meaning it would grab the wrapped frame id automatically on
construction, and reinflate the frame automatically if needed in
operator-> or operator*.  The thing that currently throws a wrench in
all that is the "frame view" command, which allows you to create and
select frames out of thin air.  And these frames are currently not
reinflatable.  And that prevents using my version of frame_info_ptr in
code paths that can process that kind of frame, like print_frame_args.
It defeats the purpose of frame_info_ptr, because those are the places
that would benefit from that the most.  So I'm currently checking if I
can improve "frame view" and the support for those user-created frames
just enough to make frame_info_ptr support them.

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

Thanks,

Simon

Re: [PATCH 2/7] gdb: add prepare_reinflate/reinflate around print_frame_args in info_frame_command_core

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> But this shows how the current frame_info_ptr is error-prone: you have
Simon> two know which functions can, deep down their call tree, reinit the
Simon> frame cache.  And all their callers that have a frame_info_ptr object,
Simon> recursively, must explicitly do prepare_reinflate / reinflate to protect
Simon> themselves against their frame_info object being invalidated.  It's very
Simon> easy to forget some spots.

Yeah.  This problem already existed, and the rationale behind
frame_info_ptr wasn't to fix it, but rather to expose it when it happens
-- by crashing rather than allowing a UAF.

Simon> I'm currently working on making frame_info_ptr work automatically,
Simon> meaning it would grab the wrapped frame id automatically on
Simon> construction, and reinflate the frame automatically if needed

We tried this a bit, but the problem we hit was that computing the
frame id require unwinding a bit, and since the code generally uses
frame_info_ptr everywhere, gdb would end up unwinding everything.

If this can be overcome then that would be great.

Tom

Re: [PATCH 2/7] gdb: add prepare_reinflate/reinflate around print_frame_args in info_frame_command_core

[Simon Marchi]

On 11/8/22 14:39, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> But this shows how the current frame_info_ptr is error-prone: you have
> Simon> two know which functions can, deep down their call tree, reinit the
> Simon> frame cache.  And all their callers that have a frame_info_ptr object,
> Simon> recursively, must explicitly do prepare_reinflate / reinflate to protect
> Simon> themselves against their frame_info object being invalidated.  It's very
> Simon> easy to forget some spots.
> 
> Yeah.  This problem already existed, and the rationale behind
> frame_info_ptr wasn't to fix it, but rather to expose it when it happens
> -- by crashing rather than allowing a UAF.
> 
> Simon> I'm currently working on making frame_info_ptr work automatically,
> Simon> meaning it would grab the wrapped frame id automatically on
> Simon> construction, and reinflate the frame automatically if needed
> 
> We tried this a bit, but the problem we hit was that computing the
> frame id require unwinding a bit, and since the code generally uses
> frame_info_ptr everywhere, gdb would end up unwinding everything.

With the final patch of this series ("gdb: add special handling for
frame level 0 in frame_info_ptr"), frame_info_ptr won't compute frame
0's frame id in order to prepare_reinflate or reinflate, so won't cause
extra unwinding.  For other frames, the frame id is already computed
when the frame is created, so frame_info_ptr won't cause any more
unwinding that already happens by creating the frame_info objects.

With my prototype that grabs the frame id in frame_info_ptr's
constructor (except for frame 0), it requires that for the frame id to
be computed already.  This means that the code paths involved in
computing the frame id wouldn't use frame_info_ptr anymore.  So, for
instance, I would revert frame_this_id_ftype to take a plain
`frame_info *` (and that leads to pretty much the whole struct
frame_unwind API to go back to `frame_info *`.  The point being that as
long as a frame doesn't have a computed id, it is not reinflatable
anyway.  And this is not really the kind of spot that is at risk of
using a stale frame_info.

However, if we really want to keep using frame_info_ptr everywhere (even
in things like frame_this_id_ftype, just to be extra safe), then perhaps
we can have two classes.  One that would provide the protection against
stale frame_info objects, which could be used everywhere, and one that
would do that + automatic reinflation.

Simon

[PATCH 3/7] gdb: move frame_info_ptr method implementations to frame-info.c

[Simon Marchi]

I don't see any particular reason why the implementations of the
frame_info_ptr object are in the header file.  It only seems to add some
complexity.  Since we can't include frame.h in frame-info.h, we have to
add declarations of functions defined in frame.c, in frame-info.h.  By
moving the implementations to a new frame-info.c, we can avoid that.

Change-Id: I435c828f81b8a3392c43ef018af31effddf6be9c
---
 gdb/Makefile.in  |  1 +
 gdb/frame-info.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 gdb/frame-info.h | 20 ++------------------
 gdb/frame.c      |  4 +---
 gdb/frame.h      |  4 ++++
 5 files changed, 55 insertions(+), 21 deletions(-)
 create mode 100644 gdb/frame-info.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index c528ee5aa806..32dbbb9c7003 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -1086,6 +1086,7 @@ COMMON_SFILES = \
 	findvar.c \
 	frame.c \
 	frame-base.c \
+	frame-info.c \
 	frame-unwind.c \
 	gcore.c \
 	gdb-demangle.c \
diff --git a/gdb/frame-info.c b/gdb/frame-info.c
new file mode 100644
index 000000000000..84791205d906
--- /dev/null
+++ b/gdb/frame-info.c
@@ -0,0 +1,47 @@
+/* Frame info pointer
+
+   Copyright (C) 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+
+#include "frame-info.h"
+#include "frame.h"
+
+/* See frame-info-ptr.h.  */
+
+intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
+
+/* See frame-info-ptr.h.  */
+
+void
+frame_info_ptr::prepare_reinflate ()
+{
+  m_cached_id = get_frame_id (*this);
+}
+
+/* See frame-info-ptr.h.  */
+
+void
+frame_info_ptr::reinflate ()
+{
+  gdb_assert (m_cached_id != null_frame_id);
+
+  if (m_ptr == nullptr)
+    m_ptr = frame_find_by_id (m_cached_id).get ();
+  gdb_assert (m_ptr != nullptr);
+}
diff --git a/gdb/frame-info.h b/gdb/frame-info.h
index 7159f82b1962..1d2d4bdc7e68 100644
--- a/gdb/frame-info.h
+++ b/gdb/frame-info.h
@@ -25,12 +25,6 @@
 
 struct frame_info;
 
-/* Forward declarations of functions, needed for the frame_info_ptr
-   to work correctly.  */
-extern void reinit_frame_cache ();
-extern struct frame_id get_frame_id (frame_info_ptr);
-extern frame_info_ptr frame_find_by_id (struct frame_id id);
-
 /* A wrapper for "frame_info *".  frame_info objects are invalidated
    whenever reinit_frame_cache is called.  This class arranges to
    invalidate the pointer when appropriate.  This is done to help
@@ -136,20 +130,10 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   }
 
   /* Cache the frame_id that the pointer will use to reinflate.  */
-  void prepare_reinflate ()
-  {
-    m_cached_id = get_frame_id (*this);
-  }
+  void prepare_reinflate ();
 
   /* Use the cached frame_id to reinflate the pointer.  */
-  void reinflate ()
-  {
-    gdb_assert (m_cached_id != null_frame_id);
-
-    if (m_ptr == nullptr)
-      m_ptr = frame_find_by_id (m_cached_id).get ();
-    gdb_assert (m_ptr != nullptr);
-  }
+  void reinflate ();
 
 private:
 
diff --git a/gdb/frame.c b/gdb/frame.c
index 5cf9186e43d7..997fda77d019 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 #include "frame.h"
+#include "frame-info.h"
 #include "target.h"
 #include "value.h"
 #include "inferior.h"	/* for inferior_ptid */
@@ -56,9 +57,6 @@ static frame_info *sentinel_frame;
 /* Number of calls to reinit_frame_cache.  */
 static unsigned int frame_cache_generation = 0;
 
-/* See frame-info.h.  */
-intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
-
 /* See frame.h.  */
 
 unsigned int
diff --git a/gdb/frame.h b/gdb/frame.h
index f61ea63c290b..5f7dcb69ee90 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -287,6 +287,10 @@ extern frame_info_ptr get_next_frame_sentinel_okay (frame_info_ptr);
    frame.  */
 extern frame_info_ptr get_prev_frame_always (frame_info_ptr);
 
+/* Given a frame's ID, relocate the frame.  Returns NULL if the frame
+   is not found.  */
+extern frame_info_ptr frame_find_by_id (frame_id id);
+
 /* Base attributes of a frame: */
 
 /* The frame's `resume' address.  Where the program will resume in
-- 
2.38.1

Re: [PATCH 3/7] gdb: move frame_info_ptr method implementations to frame-info.c

[Bruno Larsen]

On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
> I don't see any particular reason why the implementations of the
> frame_info_ptr object are in the header file.  It only seems to add some
> complexity.  Since we can't include frame.h in frame-info.h, we have to
> add declarations of functions defined in frame.c, in frame-info.h.  By
> moving the implementations to a new frame-info.c, we can avoid that.

My guess is that Tom was prototyping the solution like this, and I 
caught it and sent it before finishing his train of thoughts. (That or I 
wasn't sure of how to add a new .c file, always a possibility...)

This patch makes sense and generates no build issues or regressions on 
my system, so:

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

-- 
Cheers,
Bruno

Re: [PATCH 3/7] gdb: move frame_info_ptr method implementations to frame-info.c

[Tom Tromey]

>>>>> "Bruno" == Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:

Bruno> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>> I don't see any particular reason why the implementations of the
>> frame_info_ptr object are in the header file.  It only seems to add some
>> complexity.  Since we can't include frame.h in frame-info.h, we have to
>> add declarations of functions defined in frame.c, in frame-info.h.  By
>> moving the implementations to a new frame-info.c, we can avoid that.

Bruno> My guess is that Tom was prototyping the solution like this, and I
Bruno> caught it and sent it before finishing his train of thoughts.

I don't remember but this seems like an improvement to me.

Tom

[PATCH 4/7] gdb: remove manual frame_info reinflation code in backtrace_command_1

[Simon Marchi]

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

With the following patch applied (gdb: use frame_id_p instead of
comparing to null_frame_id in frame_info_ptr::reinflate), I would get:

    $ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame -ex "b breakpt" -ex r -ex "bt full"
    Reading symbols from testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame...
    Breakpoint 1 at 0x1131: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c, line 22.
    Starting program: /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".

    Breakpoint 1, breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
    22      }
    #0  breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
    No locals.
    /home/smarchi/src/binutils-gdb/gdb/frame-info.c:42: internal-error: reinflate: Assertion `frame_id_p (m_cached_id)' failed.

This is because the code in backtrace_command_1 to manually reinflate
`fi` steps overs frame_info_ptr's toes.

When calling

    fi.prepare_reinflate ();

`fi` gets properly filled with the cached frame id.  But when this
happens:

    fi = frame_find_by_id (frame_id);

`fi` gets replaced by a brand new frame_info_ptr that doesn't have a
cached frame id.  Then this is called without a cached frame id:

    fi.reinflate ();

That doesn't cause any problem currently, since

 - the gdb_assert in the reinflate method doesn't actually do anything
   (the following patch fixes that)
 - `fi.m_ptr` will always be non-nullptr, since we just got it from
   frame_find_by_id, so reinflate will not do anything, it won't try to
   use m_cached_id

Fix that by removing the code to manually re-fetch the frame.  That
should be taken care of by frame_info_ptr::reinflate.

Note that the old code checked if we successfully re-inflated the frame
or not, and if not it did emit a warning.  The equivalent in
frame_info_ptr::reinflate asserts that the frame has been successfully
re-inflated.  It's not clear if / when this can happen, but if it can
happen, we'll need to find a solution to this problem globally
(everywhere a frame_info_ptr can be re-inflated), not just here.  So I
propose to leave it like this, until it does become a problem.

Change-Id: I07b783d94e2853e0a2d058fe7deaf04eddf24835
---
 gdb/stack.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index 4e2342c2a8d9..5f29566fcfe9 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2082,20 +2082,7 @@ backtrace_command_1 (const frame_print_options &fp_opts,
 
 	  print_frame_info (fp_opts, fi, 1, LOCATION, 1, 0);
 	  if ((flags & PRINT_LOCALS) != 0)
-	    {
-	      struct frame_id frame_id = get_frame_id (fi);
-
-	      print_frame_local_vars (fi, false, NULL, NULL, 1, gdb_stdout);
-
-	      /* print_frame_local_vars invalidates FI.  */
-	      fi = frame_find_by_id (frame_id);
-	      if (fi == NULL)
-		{
-		  trailing = NULL;
-		  warning (_("Unable to restore previously selected frame."));
-		  break;
-		}
-	    }
+	    print_frame_local_vars (fi, false, NULL, NULL, 1, gdb_stdout);
 
 	  /* Save the last frame to check for error conditions.  */
 	  fi.reinflate ();
-- 
2.38.1

Re: [PATCH 4/7] gdb: remove manual frame_info reinflation code in backtrace_command_1

[Bruno Larsen]

On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> With the following patch applied (gdb: use frame_id_p instead of
> comparing to null_frame_id in frame_info_ptr::reinflate), I would get:
>
>      $ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame -ex "b breakpt" -ex r -ex "bt full"
>      Reading symbols from testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame...
>      Breakpoint 1 at 0x1131: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c, line 22.
>      Starting program: /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame
>      [Thread debugging using libthread_db enabled]
>      Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>
>      Breakpoint 1, breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
>      22      }
>      #0  breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
>      No locals.
>      /home/smarchi/src/binutils-gdb/gdb/frame-info.c:42: internal-error: reinflate: Assertion `frame_id_p (m_cached_id)' failed.
>
> This is because the code in backtrace_command_1 to manually reinflate
> `fi` steps overs frame_info_ptr's toes.
>
> When calling
>
>      fi.prepare_reinflate ();
>
> `fi` gets properly filled with the cached frame id.  But when this
> happens:
>
>      fi = frame_find_by_id (frame_id);
>
> `fi` gets replaced by a brand new frame_info_ptr that doesn't have a
> cached frame id.  Then this is called without a cached frame id:
>
>      fi.reinflate ();
>
> That doesn't cause any problem currently, since
>
>   - the gdb_assert in the reinflate method doesn't actually do anything
>     (the following patch fixes that)
>   - `fi.m_ptr` will always be non-nullptr, since we just got it from
>     frame_find_by_id, so reinflate will not do anything, it won't try to
>     use m_cached_id
>
> Fix that by removing the code to manually re-fetch the frame.  That
> should be taken care of by frame_info_ptr::reinflate.
>
> Note that the old code checked if we successfully re-inflated the frame
> or not, and if not it did emit a warning.  The equivalent in
> frame_info_ptr::reinflate asserts that the frame has been successfully
> re-inflated.  It's not clear if / when this can happen, but if it can
> happen, we'll need to find a solution to this problem globally
> (everywhere a frame_info_ptr can be re-inflated), not just here.  So I
> propose to leave it like this, until it does become a problem.
>
I think this patch could be squashed with the one you mention at the top 
of the commit message. I think they are both related to the same thing 
(fixing the assert and dealing with fallout) and I don't see much point 
in separating how you did here.

-- 
Cheers,
Bruno

Re: [PATCH 4/7] gdb: remove manual frame_info reinflation code in backtrace_command_1

[Simon Marchi]

On 11/8/22 05:14, Bruno Larsen wrote:
> On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> With the following patch applied (gdb: use frame_id_p instead of
>> comparing to null_frame_id in frame_info_ptr::reinflate), I would get:
>>
>>      $ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame -ex "b breakpt" -ex r -ex "bt full"
>>      Reading symbols from testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame...
>>      Breakpoint 1 at 0x1131: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c, line 22.
>>      Starting program: /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame
>>      [Thread debugging using libthread_db enabled]
>>      Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
>>
>>      Breakpoint 1, breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
>>      22      }
>>      #0  breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22
>>      No locals.
>>      /home/smarchi/src/binutils-gdb/gdb/frame-info.c:42: internal-error: reinflate: Assertion `frame_id_p (m_cached_id)' failed.
>>
>> This is because the code in backtrace_command_1 to manually reinflate
>> `fi` steps overs frame_info_ptr's toes.
>>
>> When calling
>>
>>      fi.prepare_reinflate ();
>>
>> `fi` gets properly filled with the cached frame id.  But when this
>> happens:
>>
>>      fi = frame_find_by_id (frame_id);
>>
>> `fi` gets replaced by a brand new frame_info_ptr that doesn't have a
>> cached frame id.  Then this is called without a cached frame id:
>>
>>      fi.reinflate ();
>>
>> That doesn't cause any problem currently, since
>>
>>   - the gdb_assert in the reinflate method doesn't actually do anything
>>     (the following patch fixes that)
>>   - `fi.m_ptr` will always be non-nullptr, since we just got it from
>>     frame_find_by_id, so reinflate will not do anything, it won't try to
>>     use m_cached_id
>>
>> Fix that by removing the code to manually re-fetch the frame.  That
>> should be taken care of by frame_info_ptr::reinflate.
>>
>> Note that the old code checked if we successfully re-inflated the frame
>> or not, and if not it did emit a warning.  The equivalent in
>> frame_info_ptr::reinflate asserts that the frame has been successfully
>> re-inflated.  It's not clear if / when this can happen, but if it can
>> happen, we'll need to find a solution to this problem globally
>> (everywhere a frame_info_ptr can be re-inflated), not just here.  So I
>> propose to leave it like this, until it does become a problem.
>>
> I think this patch could be squashed with the one you mention at the top of the commit message. I think they are both related to the same thing (fixing the assert and dealing with fallout) and I don't see much point in separating how you did here.

I think it would be fine to have them merged or have them separate, but
at this point I'd prefer to keep it like this, since it's done.

Simon

[PATCH 5/7] gdb: use frame_id_p instead of comparing to null_frame_id in frame_info_ptr::reinflate

[Simon Marchi]

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

The assertion

    gdb_assert (m_cached_id != null_frame_id);

is always true, as comparing equal to null_frame_id is always false
(it's the first case in frame_id::operator==, not sure why it's not this
way, but that's what it is).

Replace the comparison with a call to frame_id_p.

Change-Id: I93986e6a85ac56353690792552e5b3b4cedec7fb
---
 gdb/frame-info.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/frame-info.c b/gdb/frame-info.c
index 84791205d906..584222dc490f 100644
--- a/gdb/frame-info.c
+++ b/gdb/frame-info.c
@@ -39,7 +39,7 @@ frame_info_ptr::prepare_reinflate ()
 void
 frame_info_ptr::reinflate ()
 {
-  gdb_assert (m_cached_id != null_frame_id);
+  gdb_assert (frame_id_p (m_cached_id));
 
   if (m_ptr == nullptr)
     m_ptr = frame_find_by_id (m_cached_id).get ();
-- 
2.38.1

Re: [PATCH 5/7] gdb: use frame_id_p instead of comparing to null_frame_id in frame_info_ptr::reinflate

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

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

Simon>     gdb_assert (m_cached_id != null_frame_id);

Simon> is always true, as comparing equal to null_frame_id is always false
Simon> (it's the first case in frame_id::operator==, not sure why it's not this
Simon> way, but that's what it is).

Simon> Replace the comparison with a call to frame_id_p.

Looks good.

Tom

[PATCH 6/7] gdb: add missing prepare_reinflate call in print_frame_info

[Simon Marchi]

print_frame_info calls frame_info_ptr::reinflate, but not
frame_info_ptr::prepare_reinflate, add the call to reinflate.  It works
right now, because all callers of print_frame_info that could possibly
lead to the pretty printers being called, and the frame_info objects
being invalidated, do call prepare_reinflate themselves.  And since the
cached frame id is copied when passing a frame_info_ptr by value,
print_frame_info does have a cached frame id on entry.  So technically,
this change isn't needed.  But I don't think it's good for a function to
rely on its callers to have called prepare_reinflate, if it intends to
call reinflate.

Change-Id: Ie332b2d5479aef46f83fdc1120c7c83f4e84d1b0
---
 gdb/stack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/stack.c b/gdb/stack.c
index 5f29566fcfe9..4ad51c2eb50e 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1047,6 +1047,8 @@ print_frame_info (const frame_print_options &fp_opts,
   int location_print;
   struct ui_out *uiout = current_uiout;
 
+  frame.prepare_reinflate ();
+
   if (!current_uiout->is_mi_like_p ()
       && fp_opts.print_frame_info != print_frame_info_auto)
     {
-- 
2.38.1

Re: [PATCH 6/7] gdb: add missing prepare_reinflate call in print_frame_info

[Bruno Larsen]

On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
> print_frame_info calls frame_info_ptr::reinflate, but not
> frame_info_ptr::prepare_reinflate, add the call to reinflate.  It works
> right now, because all callers of print_frame_info that could possibly
> lead to the pretty printers being called, and the frame_info objects
> being invalidated, do call prepare_reinflate themselves.  And since the
> cached frame id is copied when passing a frame_info_ptr by value,
> print_frame_info does have a cached frame id on entry.  So technically,
> this change isn't needed.  But I don't think it's good for a function to
> rely on its callers to have called prepare_reinflate, if it intends to
> call reinflate.

oops, my bad. Looks simple enough:

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

-- 
Cheers,
Bruno

Re: [PATCH 6/7] gdb: add missing prepare_reinflate call in print_frame_info

[Lancelot SIX]

On Mon, Nov 07, 2022 at 10:53:09AM -0500, Simon Marchi via Gdb-patches wrote:
> print_frame_info calls frame_info_ptr::reinflate, but not
> frame_info_ptr::prepare_reinflate, add the call to reinflate.  It works

Did you mean "add the call to prepare_reinflate" instead?

Best,
Lancelot.

> right now, because all callers of print_frame_info that could possibly
> lead to the pretty printers being called, and the frame_info objects
> being invalidated, do call prepare_reinflate themselves.  And since the
> cached frame id is copied when passing a frame_info_ptr by value,
> print_frame_info does have a cached frame id on entry.  So technically,
> this change isn't needed.  But I don't think it's good for a function to
> rely on its callers to have called prepare_reinflate, if it intends to
> call reinflate.
> 
> Change-Id: Ie332b2d5479aef46f83fdc1120c7c83f4e84d1b0
> ---
>  gdb/stack.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 5f29566fcfe9..4ad51c2eb50e 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -1047,6 +1047,8 @@ print_frame_info (const frame_print_options &fp_opts,
>    int location_print;
>    struct ui_out *uiout = current_uiout;
>  
> +  frame.prepare_reinflate ();
> +
>    if (!current_uiout->is_mi_like_p ()
>        && fp_opts.print_frame_info != print_frame_info_auto)
>      {
> -- 
> 2.38.1
> 

Re: [PATCH 6/7] gdb: add missing prepare_reinflate call in print_frame_info

[Simon Marchi]

On 11/8/22 06:31, Lancelot SIX wrote:
> On Mon, Nov 07, 2022 at 10:53:09AM -0500, Simon Marchi via Gdb-patches wrote:
>> print_frame_info calls frame_info_ptr::reinflate, but not
>> frame_info_ptr::prepare_reinflate, add the call to reinflate.  It works
> 
> Did you mean "add the call to prepare_reinflate" instead?

Indeed, thanks.

Simon

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

[Simon Marchi]

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

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);
+
+  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);
+
+  if (m_ptr != nullptr)
+    {
+      /* The frame_info wasn't invalidated, no need to reinflate.  */
+      return;
+    }
+
+  if (m_cached_level == 0)
+    m_ptr = get_current_frame ().get ();
+  else
+    {
+      gdb_assert (frame_id_p (m_cached_id));
+      m_ptr = frame_find_by_id (m_cached_id).get ();
+    }
 
-  if (m_ptr == nullptr)
-    m_ptr = frame_find_by_id (m_cached_id).get ();
   gdb_assert (m_ptr != nullptr);
 }
diff --git a/gdb/frame-info.h b/gdb/frame-info.h
index 1d2d4bdc7e68..3369b114326f 100644
--- a/gdb/frame-info.h
+++ b/gdb/frame-info.h
@@ -56,16 +56,21 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   }
 
   frame_info_ptr (const frame_info_ptr &other)
-    : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id)
+    : m_ptr (other.m_ptr),
+      m_cached_id (other.m_cached_id),
+      m_cached_level (other.m_cached_level)
   {
     frame_list.push_back (*this);
   }
 
   frame_info_ptr (frame_info_ptr &&other)
-    : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id)
+    : m_ptr (other.m_ptr),
+      m_cached_id (other.m_cached_id),
+      m_cached_level (other.m_cached_level)
   {
     other.m_ptr = nullptr;
     other.m_cached_id = null_frame_id;
+    other.m_cached_level = invalid_level;
     frame_list.push_back (*this);
   }
 
@@ -78,6 +83,7 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   {
     m_ptr = other.m_ptr;
     m_cached_id = other.m_cached_id;
+    m_cached_level = other.m_cached_level;
     return *this;
   }
 
@@ -85,6 +91,7 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   {
     m_ptr = nullptr;
     m_cached_id = null_frame_id;
+    m_cached_level = invalid_level;
     return *this;
   }
 
@@ -92,8 +99,10 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   {
     m_ptr = other.m_ptr;
     m_cached_id = other.m_cached_id;
+    m_cached_level = other.m_cached_level;
     other.m_ptr = nullptr;
     other.m_cached_id = null_frame_id;
+    other.m_cached_level = invalid_level;
     return *this;
   }
 
@@ -136,6 +145,10 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   void reinflate ();
 
 private:
+  /* We sometimes need to construct frame_info_ptr objects around the
+     sentinel_frame, which has level -1.  Therefore, make the invalid frame
+     level value -2.  */
+  static constexpr int invalid_level = -2;
 
   /* The underlying pointer.  */
   frame_info *m_ptr = nullptr;
@@ -143,6 +156,9 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   /* The frame_id of the underlying pointer.  */
   frame_id m_cached_id = null_frame_id;
 
+  /* The frame level of the underlying pointer.  */
+  int m_cached_level = invalid_level;
+
   /* All frame_info_ptr objects are kept on an intrusive list.
      This keeps their construction and destruction costs
      reasonably small.  */
-- 
2.38.1

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

[Bruno Larsen]

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

-- 
Cheers,
Bruno

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

[Simon Marchi]

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

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

[Bruno Larsen]

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>

-- 
Cheers,
Bruno

>
> Simon
>

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

[Simon Marchi]

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

Re: [PATCH 1/7] gdb: clear other.m_cached_id in frame_info_ptr's move ctor

[Bruno Larsen]

On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote:
> We do it in the move assignment operator, so I think it makes sense to
> do it here too for consistency.  I don't think it's absolutely necessary
> to clear the other object's fields (in other words, copy constructor and
> move constructor could be the same), as there is no exclusive resource
> being transfered.  The important thing is to leave the moved-from object
> in an unknown, but valid state.  But still, I think that clearing the
> fields of the moved-from object is not a bad idea, it helps ensure we
> don't rely on the moved-from object after.
>
> Change-Id: Iee900ff9d25dad51d62765d694f2e01524351340

Seems obvious enough.

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

-- 
Cheers,
Bruno