[PATCH 0/2] Fix gdb.base/frame-view.exp on AArch64

[PATCH 0/2] Fix gdb.base/frame-view.exp on AArch64

[Simon Marchi]

As reported here:

  https://inbox.sourceware.org/gdb-patches/df23963e-1072-90bc-dce4-af68eae2422a@polymtl.ca/T/#m56264871060c1a86f2a93e9f56d6a78c546045ae

... the test gdb.base/frame-view.exp fails on AArch64.  The failure
exposes some conceptual flaw with sentinal frames of user-created
frames.  This series (mostly patch #2) tries to fix it.

Simon Marchi (2):
  gdb: call frame unwinders' dealloc_cache methods through destroying
    the frame cache
  gdb: give sentinel for user frames distinct IDs, register sentinel
    frames to the frame cache

 gdb/frame-id.h       |  10 +++--
 gdb/frame.c          | 102 +++++++++++++++++++++++++++++++------------
 gdb/frame.h          |  10 +++++
 gdb/sentinel-frame.c |   5 ++-
 4 files changed, 94 insertions(+), 33 deletions(-)

-- 
2.39.1

[PATCH 1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache

[Simon Marchi]

Currently, some frame resources are deallocated by iterating on the
frame chain (starting from the sentinel), calling dealloc_cache.  The
problem is that user-created frames are not part of that chain, so we
never call dealloc_cache for them.

I propose to make it so the dealloc_cache callbacks are called when the
frames are removed from the frame_stash hash table, by registering a
deletion function to the hash table.  This happens when
frame_stash_invalidate is called by reinit_frame_cache.  This way, all
frames registered in the cache will get their unwinder's dealloc_cache
callbacks called.

Note that at the moment, the sentinel frames are not registered in the
cache, so we won't call dealloc_cache for them.  However, it's just a
theoritical problem, because the sentinel frame unwinder does not
provide this callback.  Also, a subsequent patch will change things so
that sentinel frames are registered to the cache.

I moved the obstack_free / obstack_init pair below the
frame_stash_invalidate call in reinit_frame_cache, because I assumed
that some dealloc_cache would need to access some data on that obstack,
so it would be better to free it after clearing the hash table.

Change-Id: If4f9b38266b458c4e2f7eb43e933090177c22190
---
 gdb/frame.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index a08a8f47ebc4..fed961b2a8df 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -259,6 +259,22 @@ frame_addr_hash_eq (const void *a, const void *b)
   return f_entry->this_id.value == f_element->this_id.value;
 }
 
+/* Deletion function for the frame cache hash table.  */
+
+static void
+frame_info_del (void *frame_v)
+{
+  frame_info *frame = (frame_info *) frame_v;
+
+  if (frame->prologue_cache != nullptr
+      && frame->unwind->dealloc_cache != nullptr)
+    frame->unwind->dealloc_cache (frame, frame->prologue_cache);
+
+  if (frame->base_cache != nullptr
+      && frame->base->unwind->dealloc_cache != nullptr)
+    frame->base->unwind->dealloc_cache (frame, frame->base_cache);
+}
+
 /* Internal function to create the frame_stash hash table.  100 seems
    to be a good compromise to start the hash table at.  */
 
@@ -268,7 +284,7 @@ frame_stash_create (void)
   frame_stash = htab_create (100,
 			     frame_addr_hash,
 			     frame_addr_hash_eq,
-			     NULL);
+			     frame_info_del);
 }
 
 /* Internal function to add a frame to the frame_stash hash table.
@@ -2048,26 +2064,19 @@ reinit_frame_cache (void)
 {
   ++frame_cache_generation;
 
-  /* Tear down all frame caches.  */
-  for (frame_info *fi = sentinel_frame; fi != NULL; fi = fi->prev)
-    {
-      if (fi->prologue_cache && fi->unwind->dealloc_cache)
-	fi->unwind->dealloc_cache (fi, fi->prologue_cache);
-      if (fi->base_cache && fi->base->unwind->dealloc_cache)
-	fi->base->unwind->dealloc_cache (fi, fi->base_cache);
-    }
-
-  /* Since we can't really be sure what the first object allocated was.  */
-  obstack_free (&frame_cache_obstack, 0);
-  obstack_init (&frame_cache_obstack);
-
   if (sentinel_frame != NULL)
     annotate_frames_invalid ();
 
-  sentinel_frame = NULL;		/* Invalidate cache */
   invalidate_selected_frame ();
+
+  /* Invalidate cache.  */
+  sentinel_frame = NULL;
   frame_stash_invalidate ();
 
+  /* Since we can't really be sure what the first object allocated was.  */
+  obstack_free (&frame_cache_obstack, 0);
+  obstack_init (&frame_cache_obstack);
+
   for (frame_info_ptr &iter : frame_info_ptr::frame_list)
     iter.invalidate ();
 
-- 
2.39.1

[PATCH 2/2] gdb: give sentinel for user frames distinct IDs, register sentinel frames to the frame cache

[Simon Marchi]

The test gdb.base/frame-view.exp fails like this on AArch64:

    frame^M
    #0  baz (z1=hahaha, /home/simark/src/binutils-gdb/gdb/value.c:4056: internal-error: value_fetch_lazy_register: Assertion `next_frame != NULL' failed.^M
    A problem internal to GDB has been detected,^M
    further debugging may prove unreliable.^M
    FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)

The sequence of events leading to this is the following:

 - When we create the user frame (the "select-frame view" command), we
   create a sentinel frame just for our user-created frame, in
   create_new_frame.  This sentinel frame has the same id as the regular
   sentinel frame.

 - When printing the frame, after doing the "select-frame view" command,
   the argument's pretty printer is invoked, which does an inferior
   function call (this is the point of the test).  This clears the frame
   cache, including the "real" sentinel frame, which sets the
   sentinel_frame global to nullptr.

 - Later in the frame-printing process (when printing the second
   argument), the auto-reinflation mechanism re-creates the user frame
   by calling create_new_frame again, creating its own special sentinel
   frame again.  However, note that the "real" sentinel frame, the
   sentinel_frame global, is still nullptr.  If the selected frame had
   been a regular frame, we would have called get_current_frame at some
   point during the reinflation, which would have re-created the "real"
   sentinel frame.  But it's not the case when reinflating a user frame.

 - Deep down the stack, something wants to fill in the unwind stop
   reason for frame 0, which requires trying to unwind frame 1.  This
   leads us to trying to unwind the PC of frame 1:

     #0  gdbarch_unwind_pc (gdbarch=0xffff8d010080, next_frame=...) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:2955
     #1  0x000000000134569c in dwarf2_tailcall_sniffer_first (this_frame=..., tailcall_cachep=0xffff773fcae0, entry_cfa_sp_offsetp=0xfffff7f7d450)
         at /home/simark/src/binutils-gdb/gdb/dwarf2/frame-tailcall.c:390
     #2  0x0000000001355d84 in dwarf2_frame_cache (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1089
     #3  0x00000000013562b0 in dwarf2_frame_unwind_stop_reason (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1101
     #4  0x0000000001990f64 in get_prev_frame_always_1 (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2281
     #5  0x0000000001993034 in get_prev_frame_always (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2376
     #6  0x000000000199b814 in get_frame_unwind_stop_reason (frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:3051
     #7  0x0000000001359cd8 in dwarf2_frame_cfa (this_frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1356
     #8  0x000000000132122c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d8883ee "\217\002", op_end=0xffff8d8883ee "\217\002")
         at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:2110
     #9  0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d8883ed "\234\217\002", len=1) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
     #10 0x000000000131d68c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d88840e "", op_end=0xffff8d88840e "") at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1811
     #11 0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
     #12 0x0000000001314c3c in dwarf_expr_context::evaluate (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2, as_lval=true, per_cu=0xffff90b03700, frame=..., addr_info=0x0,
         type=0xffff8f6c8400, subobj_type=0xffff8f6c8400, subobj_offset=0) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1078
     #13 0x000000000149f9e0 in dwarf2_evaluate_loc_desc_full (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980,
         subobj_type=0xffff8f6c8400, subobj_byte_offset=0, as_lval=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1513
     #14 0x00000000014a0100 in dwarf2_evaluate_loc_desc (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980, as_lval=true)
         at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1557
     #15 0x00000000014aa584 in locexpr_read_variable (symbol=0xffff8f6cd770, frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:3052

 - AArch64 defines a special "prev register" function,
   aarch64_dwarf2_prev_register, to handle unwinding the PC.  This
   function does

     frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);

 - frame_unwind_register_unsigned ultimately creates a lazy register
   value, saving the frame id of this_frame->next.  this_frame is the
   user-created frame, to this_frame->next is the special sentinel frame
   we created for it.  So the saved ID is the sentinel frame ID.

 - When time comes to un-lazify the value, value_fetch_lazy_register
   calls frame_find_by_id, to find the frame with the ID we saved.

 - frame_find_by_id sees it's the sentinel frame ID, so returns the
   sentinel_frame global, which is, if you remember, nullptr.

 - We hit the `gdb_assert (next_frame != NULL)` assertion in
   value_fetch_lazy_register.

The issues I see here are:

 - The ID of the sentinel frame created for the user-created frame is
   not distinguishable from the ID of the regular sentinel frame.  So
   there's no way frame_find_by_id could find the right frame, in
   value_fetch_lazy_register.
 - Even if they had distinguishable IDs, sentinel frames created for
   user frames are not registered anywhere, so there's no easy way
   frame_find_by_id could find it.

This patch addresses these two issues:

 - Give sentinel frames created for user frames their own distinct IDs
 - Register sentinel frames in the frame cache, so they can be found
   with frame_find_by_id.

I initially had this split in two patches, but I then found that it was
easier to explain as a single patch.

Rergarding the first part of the change: with this patch, the sentinel
frames created for user frames (in create_new_frame) still have
stack_status == FID_STACK_SENTINEL, but their code_addr and stack_addr
fields are now filled with the addresses used to create the user frame.
This ensures this sentinel frame ID is different from the "target"
sentinel frame ID, as well as any other "user" sentinel frame ID.  If
the user tries to create the same frame, with the same addresses,
multiple times, create_sentinel_frame just reuses the existing frame.
So we won't end up with multiple user sentinels with the same ID.

Regular "target" sentinel frames remain with code_addr and stack_addr
unset.

The concrete changes for that part are:

 - Remove the sentinel_frame_id constant, since there isn't one
   "sentinel frame ID" now.  Add the frame_id_build_sentinel function
   for building sentinel frame IDs and a is_sentinel_frame_id function
   to check if a frame id represents a sentinel frame.
 - Replace the sentinel_frame_id check in frame_find_by_id with a
   comparison to `frame_id_build_sentinel (0, 0)`.  The sentinel_frame
   global is meant to contain a reference to the "target" sentinel, so
   the one with addresses (0, 0).
 - Add stack and code address parameters to create_sentinel_frame, to be
   able to create the various types of sentinel frames.
 - Adjust get_current_frame to create the regular "target" sentinel.
 - Adjust create_new_frame to create a sentinel with the ID specific to
   the created user frame.
 - Adjust sentinel_frame_prev_register to get the sentinel frame ID from
   the frame_info object, since there isn't a single "sentinel frame ID"
   now.
 - Change get_next_frame_sentinel_okay to check for a
   sentinel-frame-id-like frame ID, rather than for sentinel_frame
   specifically, since this function could be called with another
   sentinel frame (and we would want the assert to catch it).

The rest of the change is about registering the sentinel frame in the
frame cache:

 - Change frame_stash_add's assertion to allow sentinel frame levels
   (-1).
 - Make create_sentinel_frame add the frame to the frame cache.
 - Change the "sentinel_frame != NULL" check in reinit_frame_cache for a
   check that the frame stash is not empty.  The idea is that if we only
   have some user-created frames in the cache when reinit_frame_cache is
   called, we probably want to emit the frames invalid annotation.  The
   goal of that check is to avoid unnecessary repeated annotations, I
   suppose, so the "frame cache not empty" check should achieve that.

After this change, I think we could theoritically get rid of the
sentienl_frame global.  That sentinel frame could always be found by
looking up `frame_id_build_sentinel (0, 0)` in the frame cache.
However, I left the global there to avoid slowing the typical case down
for nothing.  I however, noted in its comment that it is an
optimization.

With this fix applied, the gdb.base/frame-view.exp now passes for me on
AArch64.  value_of_register_lazy now saves the special sentinel frame ID
in the value, and value_fetch_lazy_register is able to find that
sentinel frame after the frame cache reinit and after the user-created
frame was reinflated.

Change-Id: I8b77b3448822c8aab3e1c3dda76ec434eb62704f
---
 gdb/frame-id.h       | 10 ++++---
 gdb/frame.c          | 63 ++++++++++++++++++++++++++++++++++----------
 gdb/frame.h          | 10 +++++++
 gdb/sentinel-frame.c |  5 +++-
 4 files changed, 70 insertions(+), 18 deletions(-)

diff --git a/gdb/frame-id.h b/gdb/frame-id.h
index 5978d99bba8c..8ddf7d114082 100644
--- a/gdb/frame-id.h
+++ b/gdb/frame-id.h
@@ -128,12 +128,16 @@ struct frame_id
 /* For convenience.  All fields are zero.  This means "there is no frame".  */
 extern const struct frame_id null_frame_id;
 
-/* Sentinel frame.  */
-extern const struct frame_id sentinel_frame_id;
-
 /* This means "there is no frame ID, but there is a frame".  It should be
    replaced by best-effort frame IDs for the outermost frame, somehow.
    The implementation is only special_addr_p set.  */
 extern const struct frame_id outer_frame_id;
 
+/* Return true if ID represents a sentinel frame.  */
+static inline bool
+is_sentinel_frame_id (frame_id id)
+{
+  return id.stack_status == FID_STACK_SENTINEL;
+}
+
 #endif /* ifdef GDB_FRAME_ID_H  */
diff --git a/gdb/frame.c b/gdb/frame.c
index fed961b2a8df..9235a2ceb38c 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -49,7 +49,10 @@
    innermost frame.
 
    The current frame, which is the innermost frame, can be found at
-   sentinel_frame->prev.  */
+   sentinel_frame->prev.
+
+   This is an optimization to be able to find the sentinel frame quickly,
+   it could otherwise be found in the frame cache.  */
 
 static frame_info *sentinel_frame;
 
@@ -294,8 +297,8 @@ frame_stash_create (void)
 static bool
 frame_stash_add (frame_info *frame)
 {
-  /* Do not try to stash the sentinel frame.  */
-  gdb_assert (frame->level >= 0);
+  /* Valid frame levels are -1 (sentinel frames) and above.  */
+  gdb_assert (frame->level >= -1);
 
   frame_info **slot = (frame_info **) htab_find_slot (frame_stash,
 						      frame, INSERT);
@@ -681,7 +684,6 @@ frame_unwind_caller_id (frame_info_ptr next_frame)
 }
 
 const struct frame_id null_frame_id = { 0 }; /* All zeros.  */
-const struct frame_id sentinel_frame_id = { 0, 0, 0, FID_STACK_SENTINEL, 0, 1, 0 };
 const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_OUTER, 0, 1, 0 };
 
 struct frame_id
@@ -750,6 +752,29 @@ frame_id_build_wild (CORE_ADDR stack_addr)
   return id;
 }
 
+/* See frame.h.  */
+
+frame_id
+frame_id_build_sentinel (CORE_ADDR stack_addr, CORE_ADDR code_addr)
+{
+  frame_id id = null_frame_id;
+
+  id.stack_status = FID_STACK_SENTINEL;
+  id.special_addr_p = 1;
+
+  if (stack_addr != 0 || code_addr != 0)
+    {
+      /* The purpose of saving these in the sentinel frame ID is to be able to
+	 differentiate the IDs of several sentinel frames that could exist
+	 simultaneously in the frame cache.  */
+      id.stack_addr = stack_addr;
+      id.code_addr = code_addr;
+      id.code_addr_p = 1;
+    }
+
+  return id;
+}
+
 bool
 frame_id_p (frame_id l)
 {
@@ -896,7 +921,7 @@ frame_find_by_id (struct frame_id id)
     return NULL;
 
   /* Check for the sentinel frame.  */
-  if (id == sentinel_frame_id)
+  if (id == frame_id_build_sentinel (0, 0))
     return frame_info_ptr (sentinel_frame);
 
   /* Try using the frame stash first.  Finding it there removes the need
@@ -1587,10 +1612,14 @@ put_frame_register_bytes (frame_info_ptr frame, int regnum,
     }
 }
 
-/* Create a sentinel frame.  */
+/* Create a sentinel frame.
 
-static frame_info *
-create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
+   See frame_id_build_sentinel for the description of STACK_ADDR and
+   CODE_ADDR.  */
+
+static frame_info_ptr
+create_sentinel_frame (struct program_space *pspace, struct regcache *regcache,
+		       CORE_ADDR stack_addr, CORE_ADDR code_addr)
 {
   frame_info *frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
 
@@ -1608,11 +1637,14 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
   frame->next = frame;
   /* The sentinel frame has a special ID.  */
   frame->this_id.p = frame_id_status::COMPUTED;
-  frame->this_id.value = sentinel_frame_id;
+  frame->this_id.value = frame_id_build_sentinel (stack_addr, code_addr);
+
+  bool added = frame_stash_add (frame);
+  gdb_assert (added);
 
   frame_debug_printf ("  -> %s", frame->to_string ().c_str ());
 
-  return frame;
+  return frame_info_ptr (frame);
 }
 
 /* Cache for frame addresses already read by gdb.  Valid only while
@@ -1654,7 +1686,8 @@ get_current_frame (void)
 
   if (sentinel_frame == NULL)
     sentinel_frame =
-      create_sentinel_frame (current_program_space, get_current_regcache ());
+      create_sentinel_frame (current_program_space, get_current_regcache (),
+			     0, 0).get ();
 
   /* Set the current frame before computing the frame id, to avoid
      recursion inside compute_frame_id, in case the frame's
@@ -1980,7 +2013,8 @@ create_new_frame (frame_id id)
   frame_info *fi = FRAME_OBSTACK_ZALLOC (struct frame_info);
 
   fi->next = create_sentinel_frame (current_program_space,
-				    get_current_regcache ());
+				    get_current_regcache (),
+				    id.stack_addr, id.code_addr).get ();
 
   /* Set/update this frame's cached PC value, found in the next frame.
      Do this before looking for this frame's unwinder.  A sniffer is
@@ -2044,7 +2078,8 @@ get_next_frame_sentinel_okay (frame_info_ptr this_frame)
      is the sentinel frame.  But we disallow it here anyway because
      calling get_next_frame_sentinel_okay() on the sentinel frame
      is likely a coding error.  */
-  gdb_assert (this_frame != sentinel_frame);
+  if (this_frame->this_id.p == frame_id_status::COMPUTED)
+    gdb_assert (!is_sentinel_frame_id (this_frame->this_id.value));
 
   return frame_info_ptr (this_frame->next);
 }
@@ -2064,7 +2099,7 @@ reinit_frame_cache (void)
 {
   ++frame_cache_generation;
 
-  if (sentinel_frame != NULL)
+  if (htab_elements (frame_stash) > 0)
     annotate_frames_invalid ();
 
   invalidate_selected_frame ();
diff --git a/gdb/frame.h b/gdb/frame.h
index c4f7c121391b..4a99bd660dfc 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -159,6 +159,16 @@ extern struct frame_id
    as the special identifier address are set to indicate wild cards.  */
 extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
 
+/* Construct a frame ID for a sentinel frame.
+
+   If either STACK_ADDR or CODE_ADDR is not 0, the ID represents a sentinel
+   frame for a user-created frame.  STACK_ADDR and CODE_ADDR are the addresses
+   used to create the frame.
+
+   If STACK_ADDR and CODE_ADDR are both 0, the ID represents a regular sentinel
+   frame (i.e. the "next" frame of the target's current frame).  */
+extern frame_id frame_id_build_sentinel (CORE_ADDR stack_addr, CORE_ADDR code_addr);
+
 /* Returns true when L is a valid frame.  */
 extern bool frame_id_p (frame_id l);
 
diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c
index f323de3bb5d3..3e5b9be2edf6 100644
--- a/gdb/sentinel-frame.c
+++ b/gdb/sentinel-frame.c
@@ -50,8 +50,11 @@ sentinel_frame_prev_register (frame_info_ptr this_frame,
     = (struct frame_unwind_cache *) *this_prologue_cache;
   struct value *value;
 
+  frame_id this_frame_id = get_frame_id (this_frame);
+  gdb_assert (is_sentinel_frame_id (this_frame_id));
+
   value = cache->regcache->cooked_read_value (regnum);
-  VALUE_NEXT_FRAME_ID (value) = sentinel_frame_id;
+  VALUE_NEXT_FRAME_ID (value) = this_frame_id;
 
   return value;
 }
-- 
2.39.1

Re: [PATCH 2/2] gdb: give sentinel for user frames distinct IDs, register sentinel frames to the frame cache

[Alexandra Petlanova Hajkova]

[-- Attachment #1: Type: text/plain, Size: 18732 bytes --]

On Mon, Jan 30, 2023 at 9:04 PM Simon Marchi via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> The test gdb.base/frame-view.exp fails like this on AArch64:
>
>     frame^M
>     #0  baz (z1=hahaha, /home/simark/src/binutils-gdb/gdb/value.c:4056:
> internal-error: value_fetch_lazy_register: Assertion `next_frame != NULL'
> failed.^M
>     A problem internal to GDB has been detected,^M
>     further debugging may prove unreliable.^M
>     FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB
> internal error)
>
> The sequence of events leading to this is the following:
>
>  - When we create the user frame (the "select-frame view" command), we
>    create a sentinel frame just for our user-created frame, in
>    create_new_frame.  This sentinel frame has the same id as the regular
>    sentinel frame.
>
>  - When printing the frame, after doing the "select-frame view" command,
>    the argument's pretty printer is invoked, which does an inferior
>    function call (this is the point of the test).  This clears the frame
>    cache, including the "real" sentinel frame, which sets the
>    sentinel_frame global to nullptr.
>
>  - Later in the frame-printing process (when printing the second
>    argument), the auto-reinflation mechanism re-creates the user frame
>    by calling create_new_frame again, creating its own special sentinel
>    frame again.  However, note that the "real" sentinel frame, the
>    sentinel_frame global, is still nullptr.  If the selected frame had
>    been a regular frame, we would have called get_current_frame at some
>    point during the reinflation, which would have re-created the "real"
>    sentinel frame.  But it's not the case when reinflating a user frame.
>
>  - Deep down the stack, something wants to fill in the unwind stop
>    reason for frame 0, which requires trying to unwind frame 1.  This
>    leads us to trying to unwind the PC of frame 1:
>
>      #0  gdbarch_unwind_pc (gdbarch=0xffff8d010080, next_frame=...) at
> /home/simark/src/binutils-gdb/gdb/gdbarch.c:2955
>      #1  0x000000000134569c in dwarf2_tailcall_sniffer_first
> (this_frame=..., tailcall_cachep=0xffff773fcae0,
> entry_cfa_sp_offsetp=0xfffff7f7d450)
>          at /home/simark/src/binutils-gdb/gdb/dwarf2/frame-tailcall.c:390
>      #2  0x0000000001355d84 in dwarf2_frame_cache (this_frame=...,
> this_cache=0xffff773fc928) at
> /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1089
>      #3  0x00000000013562b0 in dwarf2_frame_unwind_stop_reason
> (this_frame=..., this_cache=0xffff773fc928) at
> /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1101
>      #4  0x0000000001990f64 in get_prev_frame_always_1 (this_frame=...) at
> /home/simark/src/binutils-gdb/gdb/frame.c:2281
>      #5  0x0000000001993034 in get_prev_frame_always (this_frame=...) at
> /home/simark/src/binutils-gdb/gdb/frame.c:2376
>      #6  0x000000000199b814 in get_frame_unwind_stop_reason (frame=...) at
> /home/simark/src/binutils-gdb/gdb/frame.c:3051
>      #7  0x0000000001359cd8 in dwarf2_frame_cfa (this_frame=...) at
> /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1356
>      #8  0x000000000132122c in dwarf_expr_context::execute_stack_op
> (this=0xfffff7f80170, op_ptr=0xffff8d8883ee "\217\002",
> op_end=0xffff8d8883ee "\217\002")
>          at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:2110
>      #9  0x0000000001317b30 in dwarf_expr_context::eval
> (this=0xfffff7f80170, addr=0xffff8d8883ed "\234\217\002", len=1) at
> /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
>      #10 0x000000000131d68c in dwarf_expr_context::execute_stack_op
> (this=0xfffff7f80170, op_ptr=0xffff8d88840e "", op_end=0xffff8d88840e "")
> at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1811
>      #11 0x0000000001317b30 in dwarf_expr_context::eval
> (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2) at
> /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
>      #12 0x0000000001314c3c in dwarf_expr_context::evaluate
> (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2, as_lval=true,
> per_cu=0xffff90b03700, frame=..., addr_info=0x0,
>          type=0xffff8f6c8400, subobj_type=0xffff8f6c8400, subobj_offset=0)
> at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1078
>      #13 0x000000000149f9e0 in dwarf2_evaluate_loc_desc_full
> (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2,
> per_cu=0xffff90b03700, per_objfile=0xffff9070b980,
>          subobj_type=0xffff8f6c8400, subobj_byte_offset=0, as_lval=true)
> at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1513
>      #14 0x00000000014a0100 in dwarf2_evaluate_loc_desc
> (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2,
> per_cu=0xffff90b03700, per_objfile=0xffff9070b980, as_lval=true)
>          at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1557
>      #15 0x00000000014aa584 in locexpr_read_variable
> (symbol=0xffff8f6cd770, frame=...) at
> /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:3052
>
>  - AArch64 defines a special "prev register" function,
>    aarch64_dwarf2_prev_register, to handle unwinding the PC.  This
>    function does
>
>      frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);
>
>  - frame_unwind_register_unsigned ultimately creates a lazy register
>    value, saving the frame id of this_frame->next.  this_frame is the
>    user-created frame, to this_frame->next is the special sentinel frame
>    we created for it.  So the saved ID is the sentinel frame ID.
>
>  - When time comes to un-lazify the value, value_fetch_lazy_register
>    calls frame_find_by_id, to find the frame with the ID we saved.
>
>  - frame_find_by_id sees it's the sentinel frame ID, so returns the
>    sentinel_frame global, which is, if you remember, nullptr.
>
>  - We hit the `gdb_assert (next_frame != NULL)` assertion in
>    value_fetch_lazy_register.
>
> The issues I see here are:
>
>  - The ID of the sentinel frame created for the user-created frame is
>    not distinguishable from the ID of the regular sentinel frame.  So
>    there's no way frame_find_by_id could find the right frame, in
>    value_fetch_lazy_register.
>  - Even if they had distinguishable IDs, sentinel frames created for
>    user frames are not registered anywhere, so there's no easy way
>    frame_find_by_id could find it.
>
> This patch addresses these two issues:
>
>  - Give sentinel frames created for user frames their own distinct IDs
>  - Register sentinel frames in the frame cache, so they can be found
>    with frame_find_by_id.
>
> I initially had this split in two patches, but I then found that it was
> easier to explain as a single patch.
>
> Rergarding the first part of the change: with this patch, the sentinel
> frames created for user frames (in create_new_frame) still have
> stack_status == FID_STACK_SENTINEL, but their code_addr and stack_addr
> fields are now filled with the addresses used to create the user frame.
> This ensures this sentinel frame ID is different from the "target"
> sentinel frame ID, as well as any other "user" sentinel frame ID.  If
> the user tries to create the same frame, with the same addresses,
> multiple times, create_sentinel_frame just reuses the existing frame.
> So we won't end up with multiple user sentinels with the same ID.
>
> Regular "target" sentinel frames remain with code_addr and stack_addr
> unset.
>
> The concrete changes for that part are:
>
>  - Remove the sentinel_frame_id constant, since there isn't one
>    "sentinel frame ID" now.  Add the frame_id_build_sentinel function
>    for building sentinel frame IDs and a is_sentinel_frame_id function
>    to check if a frame id represents a sentinel frame.
>  - Replace the sentinel_frame_id check in frame_find_by_id with a
>    comparison to `frame_id_build_sentinel (0, 0)`.  The sentinel_frame
>    global is meant to contain a reference to the "target" sentinel, so
>    the one with addresses (0, 0).
>  - Add stack and code address parameters to create_sentinel_frame, to be
>    able to create the various types of sentinel frames.
>  - Adjust get_current_frame to create the regular "target" sentinel.
>  - Adjust create_new_frame to create a sentinel with the ID specific to
>    the created user frame.
>  - Adjust sentinel_frame_prev_register to get the sentinel frame ID from
>    the frame_info object, since there isn't a single "sentinel frame ID"
>    now.
>  - Change get_next_frame_sentinel_okay to check for a
>    sentinel-frame-id-like frame ID, rather than for sentinel_frame
>    specifically, since this function could be called with another
>    sentinel frame (and we would want the assert to catch it).
>
> The rest of the change is about registering the sentinel frame in the
> frame cache:
>
>  - Change frame_stash_add's assertion to allow sentinel frame levels
>    (-1).
>  - Make create_sentinel_frame add the frame to the frame cache.
>  - Change the "sentinel_frame != NULL" check in reinit_frame_cache for a
>    check that the frame stash is not empty.  The idea is that if we only
>    have some user-created frames in the cache when reinit_frame_cache is
>    called, we probably want to emit the frames invalid annotation.  The
>    goal of that check is to avoid unnecessary repeated annotations, I
>    suppose, so the "frame cache not empty" check should achieve that.
>
> After this change, I think we could theoritically get rid of the
> sentienl_frame global.  That sentinel frame could always be found by
> looking up `frame_id_build_sentinel (0, 0)` in the frame cache.
> However, I left the global there to avoid slowing the typical case down
> for nothing.  I however, noted in its comment that it is an
> optimization.
>
> With this fix applied, the gdb.base/frame-view.exp now passes for me on
> AArch64.  value_of_register_lazy now saves the special sentinel frame ID
> in the value, and value_fetch_lazy_register is able to find that
> sentinel frame after the frame cache reinit and after the user-created
> frame was reinflated.
>
> Change-Id: I8b77b3448822c8aab3e1c3dda76ec434eb62704f
> ---
>  gdb/frame-id.h       | 10 ++++---
>  gdb/frame.c          | 63 ++++++++++++++++++++++++++++++++++----------
>  gdb/frame.h          | 10 +++++++
>  gdb/sentinel-frame.c |  5 +++-
>  4 files changed, 70 insertions(+), 18 deletions(-)
>
> diff --git a/gdb/frame-id.h b/gdb/frame-id.h
> index 5978d99bba8c..8ddf7d114082 100644
> --- a/gdb/frame-id.h
> +++ b/gdb/frame-id.h
> @@ -128,12 +128,16 @@ struct frame_id
>  /* For convenience.  All fields are zero.  This means "there is no
> frame".  */
>  extern const struct frame_id null_frame_id;
>
> -/* Sentinel frame.  */
> -extern const struct frame_id sentinel_frame_id;
> -
>  /* This means "there is no frame ID, but there is a frame".  It should be
>     replaced by best-effort frame IDs for the outermost frame, somehow.
>     The implementation is only special_addr_p set.  */
>  extern const struct frame_id outer_frame_id;
>
> +/* Return true if ID represents a sentinel frame.  */
> +static inline bool
> +is_sentinel_frame_id (frame_id id)
> +{
> +  return id.stack_status == FID_STACK_SENTINEL;
> +}
> +
>  #endif /* ifdef GDB_FRAME_ID_H  */
> diff --git a/gdb/frame.c b/gdb/frame.c
> index fed961b2a8df..9235a2ceb38c 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -49,7 +49,10 @@
>     innermost frame.
>
>     The current frame, which is the innermost frame, can be found at
> -   sentinel_frame->prev.  */
> +   sentinel_frame->prev.
> +
> +   This is an optimization to be able to find the sentinel frame quickly,
> +   it could otherwise be found in the frame cache.  */
>
>  static frame_info *sentinel_frame;
>
> @@ -294,8 +297,8 @@ frame_stash_create (void)
>  static bool
>  frame_stash_add (frame_info *frame)
>  {
> -  /* Do not try to stash the sentinel frame.  */
> -  gdb_assert (frame->level >= 0);
> +  /* Valid frame levels are -1 (sentinel frames) and above.  */
> +  gdb_assert (frame->level >= -1);
>
>    frame_info **slot = (frame_info **) htab_find_slot (frame_stash,
>                                                       frame, INSERT);
> @@ -681,7 +684,6 @@ frame_unwind_caller_id (frame_info_ptr next_frame)
>  }
>
>  const struct frame_id null_frame_id = { 0 }; /* All zeros.  */
> -const struct frame_id sentinel_frame_id = { 0, 0, 0, FID_STACK_SENTINEL,
> 0, 1, 0 };
>  const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_OUTER, 0, 1,
> 0 };
>
>  struct frame_id
> @@ -750,6 +752,29 @@ frame_id_build_wild (CORE_ADDR stack_addr)
>    return id;
>  }
>
> +/* See frame.h.  */
> +
> +frame_id
> +frame_id_build_sentinel (CORE_ADDR stack_addr, CORE_ADDR code_addr)
> +{
> +  frame_id id = null_frame_id;
> +
> +  id.stack_status = FID_STACK_SENTINEL;
> +  id.special_addr_p = 1;
> +
> +  if (stack_addr != 0 || code_addr != 0)
> +    {
> +      /* The purpose of saving these in the sentinel frame ID is to be
> able to
> +        differentiate the IDs of several sentinel frames that could exist
> +        simultaneously in the frame cache.  */
> +      id.stack_addr = stack_addr;
> +      id.code_addr = code_addr;
> +      id.code_addr_p = 1;
> +    }
> +
> +  return id;
> +}
> +
>  bool
>  frame_id_p (frame_id l)
>  {
> @@ -896,7 +921,7 @@ frame_find_by_id (struct frame_id id)
>      return NULL;
>
>    /* Check for the sentinel frame.  */
> -  if (id == sentinel_frame_id)
> +  if (id == frame_id_build_sentinel (0, 0))
>      return frame_info_ptr (sentinel_frame);
>
>    /* Try using the frame stash first.  Finding it there removes the need
> @@ -1587,10 +1612,14 @@ put_frame_register_bytes (frame_info_ptr frame,
> int regnum,
>      }
>  }
>
> -/* Create a sentinel frame.  */
> +/* Create a sentinel frame.
>
> -static frame_info *
> -create_sentinel_frame (struct program_space *pspace, struct regcache
> *regcache)
> +   See frame_id_build_sentinel for the description of STACK_ADDR and
> +   CODE_ADDR.  */
> +
> +static frame_info_ptr
> +create_sentinel_frame (struct program_space *pspace, struct regcache
> *regcache,
> +                      CORE_ADDR stack_addr, CORE_ADDR code_addr)
>  {
>    frame_info *frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
>
> @@ -1608,11 +1637,14 @@ create_sentinel_frame (struct program_space
> *pspace, struct regcache *regcache)
>    frame->next = frame;
>    /* The sentinel frame has a special ID.  */
>    frame->this_id.p = frame_id_status::COMPUTED;
> -  frame->this_id.value = sentinel_frame_id;
> +  frame->this_id.value = frame_id_build_sentinel (stack_addr, code_addr);
> +
> +  bool added = frame_stash_add (frame);
> +  gdb_assert (added);
>
>    frame_debug_printf ("  -> %s", frame->to_string ().c_str ());
>
> -  return frame;
> +  return frame_info_ptr (frame);
>  }
>
>  /* Cache for frame addresses already read by gdb.  Valid only while
> @@ -1654,7 +1686,8 @@ get_current_frame (void)
>
>    if (sentinel_frame == NULL)
>      sentinel_frame =
> -      create_sentinel_frame (current_program_space, get_current_regcache
> ());
> +      create_sentinel_frame (current_program_space, get_current_regcache
> (),
> +                            0, 0).get ();
>
>    /* Set the current frame before computing the frame id, to avoid
>       recursion inside compute_frame_id, in case the frame's
> @@ -1980,7 +2013,8 @@ create_new_frame (frame_id id)
>    frame_info *fi = FRAME_OBSTACK_ZALLOC (struct frame_info);
>
>    fi->next = create_sentinel_frame (current_program_space,
> -                                   get_current_regcache ());
> +                                   get_current_regcache (),
> +                                   id.stack_addr, id.code_addr).get ();
>
>    /* Set/update this frame's cached PC value, found in the next frame.
>       Do this before looking for this frame's unwinder.  A sniffer is
> @@ -2044,7 +2078,8 @@ get_next_frame_sentinel_okay (frame_info_ptr
> this_frame)
>       is the sentinel frame.  But we disallow it here anyway because
>       calling get_next_frame_sentinel_okay() on the sentinel frame
>       is likely a coding error.  */
> -  gdb_assert (this_frame != sentinel_frame);
> +  if (this_frame->this_id.p == frame_id_status::COMPUTED)
> +    gdb_assert (!is_sentinel_frame_id (this_frame->this_id.value));
>
>    return frame_info_ptr (this_frame->next);
>  }
> @@ -2064,7 +2099,7 @@ reinit_frame_cache (void)
>  {
>    ++frame_cache_generation;
>
> -  if (sentinel_frame != NULL)
> +  if (htab_elements (frame_stash) > 0)
>      annotate_frames_invalid ();
>
>    invalidate_selected_frame ();
> diff --git a/gdb/frame.h b/gdb/frame.h
> index c4f7c121391b..4a99bd660dfc 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -159,6 +159,16 @@ extern struct frame_id
>     as the special identifier address are set to indicate wild cards.  */
>  extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
>
> +/* Construct a frame ID for a sentinel frame.
> +
> +   If either STACK_ADDR or CODE_ADDR is not 0, the ID represents a
> sentinel
> +   frame for a user-created frame.  STACK_ADDR and CODE_ADDR are the
> addresses
> +   used to create the frame.
> +
> +   If STACK_ADDR and CODE_ADDR are both 0, the ID represents a regular
> sentinel
> +   frame (i.e. the "next" frame of the target's current frame).  */
> +extern frame_id frame_id_build_sentinel (CORE_ADDR stack_addr, CORE_ADDR
> code_addr);
> +
>  /* Returns true when L is a valid frame.  */
>  extern bool frame_id_p (frame_id l);
>
> diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c
> index f323de3bb5d3..3e5b9be2edf6 100644
> --- a/gdb/sentinel-frame.c
> +++ b/gdb/sentinel-frame.c
> @@ -50,8 +50,11 @@ sentinel_frame_prev_register (frame_info_ptr this_frame,
>      = (struct frame_unwind_cache *) *this_prologue_cache;
>    struct value *value;
>
> +  frame_id this_frame_id = get_frame_id (this_frame);
> +  gdb_assert (is_sentinel_frame_id (this_frame_id));
> +
>    value = cache->regcache->cooked_read_value (regnum);
> -  VALUE_NEXT_FRAME_ID (value) = sentinel_frame_id;
> +  VALUE_NEXT_FRAME_ID (value) = this_frame_id;
>
>    return value;
>  }
> --
> 2.39.1
>
> The commit message is awesome, very informative, I really enjoyed reading
it.

I can see the patch set fixes gdb.base/frame-view.exp on aarch64, I've seen
this failure before the patch which does not occur after applying it.
frame
#0  baz (z1=hahaha, ../../binutils-gdb/gdb/value.c:4056: internal-error: v
alue_fetch_lazy_register: Assertion `next_frame != NULL' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
----- Backtrace -----
FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB
internal error)
Resyncing due to internal error.
0x5a6aaf gdb_internal_backtrace_1
        ../../binutils-gdb/gdb/bt-utils.c:122

Re: [PATCH 2/2] gdb: give sentinel for user frames distinct IDs, register sentinel frames to the frame cache

[Luis Machado]

On 1/30/23 20:02, Simon Marchi via Gdb-patches wrote:
> The test gdb.base/frame-view.exp fails like this on AArch64:
> 
>      frame^M
>      #0  baz (z1=hahaha, /home/simark/src/binutils-gdb/gdb/value.c:4056: internal-error: value_fetch_lazy_register: Assertion `next_frame != NULL' failed.^M
>      A problem internal to GDB has been detected,^M
>      further debugging may prove unreliable.^M
>      FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB internal error)
> 
> The sequence of events leading to this is the following:
> 
>   - When we create the user frame (the "select-frame view" command), we
>     create a sentinel frame just for our user-created frame, in
>     create_new_frame.  This sentinel frame has the same id as the regular
>     sentinel frame.
> 
>   - When printing the frame, after doing the "select-frame view" command,
>     the argument's pretty printer is invoked, which does an inferior
>     function call (this is the point of the test).  This clears the frame
>     cache, including the "real" sentinel frame, which sets the
>     sentinel_frame global to nullptr.
> 
>   - Later in the frame-printing process (when printing the second
>     argument), the auto-reinflation mechanism re-creates the user frame
>     by calling create_new_frame again, creating its own special sentinel
>     frame again.  However, note that the "real" sentinel frame, the
>     sentinel_frame global, is still nullptr.  If the selected frame had
>     been a regular frame, we would have called get_current_frame at some
>     point during the reinflation, which would have re-created the "real"
>     sentinel frame.  But it's not the case when reinflating a user frame.
> 
>   - Deep down the stack, something wants to fill in the unwind stop
>     reason for frame 0, which requires trying to unwind frame 1.  This
>     leads us to trying to unwind the PC of frame 1:
> 
>       #0  gdbarch_unwind_pc (gdbarch=0xffff8d010080, next_frame=...) at /home/simark/src/binutils-gdb/gdb/gdbarch.c:2955
>       #1  0x000000000134569c in dwarf2_tailcall_sniffer_first (this_frame=..., tailcall_cachep=0xffff773fcae0, entry_cfa_sp_offsetp=0xfffff7f7d450)
>           at /home/simark/src/binutils-gdb/gdb/dwarf2/frame-tailcall.c:390
>       #2  0x0000000001355d84 in dwarf2_frame_cache (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1089
>       #3  0x00000000013562b0 in dwarf2_frame_unwind_stop_reason (this_frame=..., this_cache=0xffff773fc928) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1101
>       #4  0x0000000001990f64 in get_prev_frame_always_1 (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2281
>       #5  0x0000000001993034 in get_prev_frame_always (this_frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:2376
>       #6  0x000000000199b814 in get_frame_unwind_stop_reason (frame=...) at /home/simark/src/binutils-gdb/gdb/frame.c:3051
>       #7  0x0000000001359cd8 in dwarf2_frame_cfa (this_frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/frame.c:1356
>       #8  0x000000000132122c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d8883ee "\217\002", op_end=0xffff8d8883ee "\217\002")
>           at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:2110
>       #9  0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d8883ed "\234\217\002", len=1) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
>       #10 0x000000000131d68c in dwarf_expr_context::execute_stack_op (this=0xfffff7f80170, op_ptr=0xffff8d88840e "", op_end=0xffff8d88840e "") at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1811
>       #11 0x0000000001317b30 in dwarf_expr_context::eval (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1239
>       #12 0x0000000001314c3c in dwarf_expr_context::evaluate (this=0xfffff7f80170, addr=0xffff8d88840c "\221p", len=2, as_lval=true, per_cu=0xffff90b03700, frame=..., addr_info=0x0,
>           type=0xffff8f6c8400, subobj_type=0xffff8f6c8400, subobj_offset=0) at /home/simark/src/binutils-gdb/gdb/dwarf2/expr.c:1078
>       #13 0x000000000149f9e0 in dwarf2_evaluate_loc_desc_full (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980,
>           subobj_type=0xffff8f6c8400, subobj_byte_offset=0, as_lval=true) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1513
>       #14 0x00000000014a0100 in dwarf2_evaluate_loc_desc (type=0xffff8f6c8400, frame=..., data=0xffff8d88840c "\221p", size=2, per_cu=0xffff90b03700, per_objfile=0xffff9070b980, as_lval=true)
>           at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:1557
>       #15 0x00000000014aa584 in locexpr_read_variable (symbol=0xffff8f6cd770, frame=...) at /home/simark/src/binutils-gdb/gdb/dwarf2/loc.c:3052
> 
>   - AArch64 defines a special "prev register" function,
>     aarch64_dwarf2_prev_register, to handle unwinding the PC.  This
>     function does
> 
>       frame_unwind_register_unsigned (this_frame, AARCH64_LR_REGNUM);
> 
>   - frame_unwind_register_unsigned ultimately creates a lazy register
>     value, saving the frame id of this_frame->next.  this_frame is the
>     user-created frame, to this_frame->next is the special sentinel frame
>     we created for it.  So the saved ID is the sentinel frame ID.
> 
>   - When time comes to un-lazify the value, value_fetch_lazy_register
>     calls frame_find_by_id, to find the frame with the ID we saved.
> 
>   - frame_find_by_id sees it's the sentinel frame ID, so returns the
>     sentinel_frame global, which is, if you remember, nullptr.
> 
>   - We hit the `gdb_assert (next_frame != NULL)` assertion in
>     value_fetch_lazy_register.
> 
> The issues I see here are:
> 
>   - The ID of the sentinel frame created for the user-created frame is
>     not distinguishable from the ID of the regular sentinel frame.  So
>     there's no way frame_find_by_id could find the right frame, in
>     value_fetch_lazy_register.
>   - Even if they had distinguishable IDs, sentinel frames created for
>     user frames are not registered anywhere, so there's no easy way
>     frame_find_by_id could find it.
> 
> This patch addresses these two issues:
> 
>   - Give sentinel frames created for user frames their own distinct IDs
>   - Register sentinel frames in the frame cache, so they can be found
>     with frame_find_by_id.
> 
> I initially had this split in two patches, but I then found that it was
> easier to explain as a single patch.
> 
> Rergarding the first part of the change: with this patch, the sentinel
> frames created for user frames (in create_new_frame) still have
> stack_status == FID_STACK_SENTINEL, but their code_addr and stack_addr
> fields are now filled with the addresses used to create the user frame.
> This ensures this sentinel frame ID is different from the "target"
> sentinel frame ID, as well as any other "user" sentinel frame ID.  If
> the user tries to create the same frame, with the same addresses,
> multiple times, create_sentinel_frame just reuses the existing frame.
> So we won't end up with multiple user sentinels with the same ID.
> 
> Regular "target" sentinel frames remain with code_addr and stack_addr
> unset.
> 
> The concrete changes for that part are:
> 
>   - Remove the sentinel_frame_id constant, since there isn't one
>     "sentinel frame ID" now.  Add the frame_id_build_sentinel function
>     for building sentinel frame IDs and a is_sentinel_frame_id function
>     to check if a frame id represents a sentinel frame.
>   - Replace the sentinel_frame_id check in frame_find_by_id with a
>     comparison to `frame_id_build_sentinel (0, 0)`.  The sentinel_frame
>     global is meant to contain a reference to the "target" sentinel, so
>     the one with addresses (0, 0).
>   - Add stack and code address parameters to create_sentinel_frame, to be
>     able to create the various types of sentinel frames.
>   - Adjust get_current_frame to create the regular "target" sentinel.
>   - Adjust create_new_frame to create a sentinel with the ID specific to
>     the created user frame.
>   - Adjust sentinel_frame_prev_register to get the sentinel frame ID from
>     the frame_info object, since there isn't a single "sentinel frame ID"
>     now.
>   - Change get_next_frame_sentinel_okay to check for a
>     sentinel-frame-id-like frame ID, rather than for sentinel_frame
>     specifically, since this function could be called with another
>     sentinel frame (and we would want the assert to catch it).
> 
> The rest of the change is about registering the sentinel frame in the
> frame cache:
> 
>   - Change frame_stash_add's assertion to allow sentinel frame levels
>     (-1).
>   - Make create_sentinel_frame add the frame to the frame cache.
>   - Change the "sentinel_frame != NULL" check in reinit_frame_cache for a
>     check that the frame stash is not empty.  The idea is that if we only
>     have some user-created frames in the cache when reinit_frame_cache is
>     called, we probably want to emit the frames invalid annotation.  The
>     goal of that check is to avoid unnecessary repeated annotations, I
>     suppose, so the "frame cache not empty" check should achieve that.
> 
> After this change, I think we could theoritically get rid of the
> sentienl_frame global.  That sentinel frame could always be found by
> looking up `frame_id_build_sentinel (0, 0)` in the frame cache.
> However, I left the global there to avoid slowing the typical case down
> for nothing.  I however, noted in its comment that it is an
> optimization.
> 
> With this fix applied, the gdb.base/frame-view.exp now passes for me on
> AArch64.  value_of_register_lazy now saves the special sentinel frame ID
> in the value, and value_fetch_lazy_register is able to find that
> sentinel frame after the frame cache reinit and after the user-created
> frame was reinflated.
> 
> Change-Id: I8b77b3448822c8aab3e1c3dda76ec434eb62704f
> ---
>   gdb/frame-id.h       | 10 ++++---
>   gdb/frame.c          | 63 ++++++++++++++++++++++++++++++++++----------
>   gdb/frame.h          | 10 +++++++
>   gdb/sentinel-frame.c |  5 +++-
>   4 files changed, 70 insertions(+), 18 deletions(-)
> 
> diff --git a/gdb/frame-id.h b/gdb/frame-id.h
> index 5978d99bba8c..8ddf7d114082 100644
> --- a/gdb/frame-id.h
> +++ b/gdb/frame-id.h
> @@ -128,12 +128,16 @@ struct frame_id
>   /* For convenience.  All fields are zero.  This means "there is no frame".  */
>   extern const struct frame_id null_frame_id;
>   
> -/* Sentinel frame.  */
> -extern const struct frame_id sentinel_frame_id;
> -
>   /* This means "there is no frame ID, but there is a frame".  It should be
>      replaced by best-effort frame IDs for the outermost frame, somehow.
>      The implementation is only special_addr_p set.  */
>   extern const struct frame_id outer_frame_id;
>   
> +/* Return true if ID represents a sentinel frame.  */
> +static inline bool
> +is_sentinel_frame_id (frame_id id)
> +{
> +  return id.stack_status == FID_STACK_SENTINEL;
> +}
> +
>   #endif /* ifdef GDB_FRAME_ID_H  */
> diff --git a/gdb/frame.c b/gdb/frame.c
> index fed961b2a8df..9235a2ceb38c 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -49,7 +49,10 @@
>      innermost frame.
>   
>      The current frame, which is the innermost frame, can be found at
> -   sentinel_frame->prev.  */
> +   sentinel_frame->prev.
> +
> +   This is an optimization to be able to find the sentinel frame quickly,
> +   it could otherwise be found in the frame cache.  */
>   
>   static frame_info *sentinel_frame;
>   
> @@ -294,8 +297,8 @@ frame_stash_create (void)
>   static bool
>   frame_stash_add (frame_info *frame)
>   {
> -  /* Do not try to stash the sentinel frame.  */
> -  gdb_assert (frame->level >= 0);
> +  /* Valid frame levels are -1 (sentinel frames) and above.  */
> +  gdb_assert (frame->level >= -1);
>   
>     frame_info **slot = (frame_info **) htab_find_slot (frame_stash,
>   						      frame, INSERT);
> @@ -681,7 +684,6 @@ frame_unwind_caller_id (frame_info_ptr next_frame)
>   }
>   
>   const struct frame_id null_frame_id = { 0 }; /* All zeros.  */
> -const struct frame_id sentinel_frame_id = { 0, 0, 0, FID_STACK_SENTINEL, 0, 1, 0 };
>   const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_OUTER, 0, 1, 0 };
>   
>   struct frame_id
> @@ -750,6 +752,29 @@ frame_id_build_wild (CORE_ADDR stack_addr)
>     return id;
>   }
>   
> +/* See frame.h.  */
> +
> +frame_id
> +frame_id_build_sentinel (CORE_ADDR stack_addr, CORE_ADDR code_addr)
> +{
> +  frame_id id = null_frame_id;
> +
> +  id.stack_status = FID_STACK_SENTINEL;
> +  id.special_addr_p = 1;
> +
> +  if (stack_addr != 0 || code_addr != 0)
> +    {
> +      /* The purpose of saving these in the sentinel frame ID is to be able to
> +	 differentiate the IDs of several sentinel frames that could exist
> +	 simultaneously in the frame cache.  */
> +      id.stack_addr = stack_addr;
> +      id.code_addr = code_addr;
> +      id.code_addr_p = 1;
> +    }
> +
> +  return id;
> +}
> +
>   bool
>   frame_id_p (frame_id l)
>   {
> @@ -896,7 +921,7 @@ frame_find_by_id (struct frame_id id)
>       return NULL;
>   
>     /* Check for the sentinel frame.  */
> -  if (id == sentinel_frame_id)
> +  if (id == frame_id_build_sentinel (0, 0))
>       return frame_info_ptr (sentinel_frame);
>   
>     /* Try using the frame stash first.  Finding it there removes the need
> @@ -1587,10 +1612,14 @@ put_frame_register_bytes (frame_info_ptr frame, int regnum,
>       }
>   }
>   
> -/* Create a sentinel frame.  */
> +/* Create a sentinel frame.
>   
> -static frame_info *
> -create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
> +   See frame_id_build_sentinel for the description of STACK_ADDR and
> +   CODE_ADDR.  */
> +
> +static frame_info_ptr
> +create_sentinel_frame (struct program_space *pspace, struct regcache *regcache,
> +		       CORE_ADDR stack_addr, CORE_ADDR code_addr)
>   {
>     frame_info *frame = FRAME_OBSTACK_ZALLOC (struct frame_info);
>   
> @@ -1608,11 +1637,14 @@ create_sentinel_frame (struct program_space *pspace, struct regcache *regcache)
>     frame->next = frame;
>     /* The sentinel frame has a special ID.  */
>     frame->this_id.p = frame_id_status::COMPUTED;
> -  frame->this_id.value = sentinel_frame_id;
> +  frame->this_id.value = frame_id_build_sentinel (stack_addr, code_addr);
> +
> +  bool added = frame_stash_add (frame);
> +  gdb_assert (added);
>   
>     frame_debug_printf ("  -> %s", frame->to_string ().c_str ());
>   
> -  return frame;
> +  return frame_info_ptr (frame);
>   }
>   
>   /* Cache for frame addresses already read by gdb.  Valid only while
> @@ -1654,7 +1686,8 @@ get_current_frame (void)
>   
>     if (sentinel_frame == NULL)
>       sentinel_frame =
> -      create_sentinel_frame (current_program_space, get_current_regcache ());
> +      create_sentinel_frame (current_program_space, get_current_regcache (),
> +			     0, 0).get ();
>   
>     /* Set the current frame before computing the frame id, to avoid
>        recursion inside compute_frame_id, in case the frame's
> @@ -1980,7 +2013,8 @@ create_new_frame (frame_id id)
>     frame_info *fi = FRAME_OBSTACK_ZALLOC (struct frame_info);
>   
>     fi->next = create_sentinel_frame (current_program_space,
> -				    get_current_regcache ());
> +				    get_current_regcache (),
> +				    id.stack_addr, id.code_addr).get ();
>   
>     /* Set/update this frame's cached PC value, found in the next frame.
>        Do this before looking for this frame's unwinder.  A sniffer is
> @@ -2044,7 +2078,8 @@ get_next_frame_sentinel_okay (frame_info_ptr this_frame)
>        is the sentinel frame.  But we disallow it here anyway because
>        calling get_next_frame_sentinel_okay() on the sentinel frame
>        is likely a coding error.  */
> -  gdb_assert (this_frame != sentinel_frame);
> +  if (this_frame->this_id.p == frame_id_status::COMPUTED)
> +    gdb_assert (!is_sentinel_frame_id (this_frame->this_id.value));
>   
>     return frame_info_ptr (this_frame->next);
>   }
> @@ -2064,7 +2099,7 @@ reinit_frame_cache (void)
>   {
>     ++frame_cache_generation;
>   
> -  if (sentinel_frame != NULL)
> +  if (htab_elements (frame_stash) > 0)
>       annotate_frames_invalid ();
>   
>     invalidate_selected_frame ();
> diff --git a/gdb/frame.h b/gdb/frame.h
> index c4f7c121391b..4a99bd660dfc 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -159,6 +159,16 @@ extern struct frame_id
>      as the special identifier address are set to indicate wild cards.  */
>   extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
>   
> +/* Construct a frame ID for a sentinel frame.
> +
> +   If either STACK_ADDR or CODE_ADDR is not 0, the ID represents a sentinel
> +   frame for a user-created frame.  STACK_ADDR and CODE_ADDR are the addresses
> +   used to create the frame.
> +
> +   If STACK_ADDR and CODE_ADDR are both 0, the ID represents a regular sentinel
> +   frame (i.e. the "next" frame of the target's current frame).  */
> +extern frame_id frame_id_build_sentinel (CORE_ADDR stack_addr, CORE_ADDR code_addr);
> +
>   /* Returns true when L is a valid frame.  */
>   extern bool frame_id_p (frame_id l);
>   
> diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c
> index f323de3bb5d3..3e5b9be2edf6 100644
> --- a/gdb/sentinel-frame.c
> +++ b/gdb/sentinel-frame.c
> @@ -50,8 +50,11 @@ sentinel_frame_prev_register (frame_info_ptr this_frame,
>       = (struct frame_unwind_cache *) *this_prologue_cache;
>     struct value *value;
>   
> +  frame_id this_frame_id = get_frame_id (this_frame);
> +  gdb_assert (is_sentinel_frame_id (this_frame_id));
> +
>     value = cache->regcache->cooked_read_value (regnum);
> -  VALUE_NEXT_FRAME_ID (value) = sentinel_frame_id;
> +  VALUE_NEXT_FRAME_ID (value) = this_frame_id;
>   
>     return value;
>   }

I can confirm this fixed the FAIL's for both aarch64-linux and armhf-linux on Ubuntu 22.04/20.04 for me.

Thanks for fixing this.

Re: [PATCH 2/2] gdb: give sentinel for user frames distinct IDs, register sentinel frames to the frame cache

[Simon Marchi]

> 
> I can see the patch set fixes gdb.base/frame-view.exp on aarch64, I've seen
> this failure before the patch which does not occur after applying it.
> frame
> #0  baz (z1=hahaha, ../../binutils-gdb/gdb/value.c:4056: internal-error: v
> alue_fetch_lazy_register: Assertion `next_frame != NULL' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> ----- Backtrace -----
> FAIL: gdb.base/frame-view.exp: with_pretty_printer=true: frame (GDB
> internal error)
> Resyncing due to internal error.
> 0x5a6aaf gdb_internal_backtrace_1
>         ../../binutils-gdb/gdb/bt-utils.c:122

Thanks for the feedback, will add your Tested-By:

Tested-By: Alexandra Petlanova Hajkova <ahajkova@redhat.com>

Simon

Re: [PATCH 2/2] gdb: give sentinel for user frames distinct IDs, register sentinel frames to the frame cache

[Simon Marchi]

> I can confirm this fixed the FAIL's for both aarch64-linux and armhf-linux on Ubuntu 22.04/20.04 for me.
> 
> Thanks for fixing this.
Thanks for the feedback, I will add your:

Tested-By: Luis Machado <luis.machado@arm.com>

I will go ahead and push it.

Simon

Re: [PATCH 1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache

[Tom de Vries]

On 1/30/23 21:02, Simon Marchi via Gdb-patches wrote:
> Currently, some frame resources are deallocated by iterating on the
> frame chain (starting from the sentinel), calling dealloc_cache.  The
> problem is that user-created frames are not part of that chain, so we
> never call dealloc_cache for them.
> 
> I propose to make it so the dealloc_cache callbacks are called when the
> frames are removed from the frame_stash hash table, by registering a
> deletion function to the hash table.  This happens when
> frame_stash_invalidate is called by reinit_frame_cache.  This way, all
> frames registered in the cache will get their unwinder's dealloc_cache
> callbacks called.
> 
> Note that at the moment, the sentinel frames are not registered in the
> cache, so we won't call dealloc_cache for them.  However, it's just a
> theoritical problem, because the sentinel frame unwinder does not
> provide this callback.  Also, a subsequent patch will change things so
> that sentinel frames are registered to the cache.
> 
> I moved the obstack_free / obstack_init pair below the
> frame_stash_invalidate call in reinit_frame_cache, because I assumed
> that some dealloc_cache would need to access some data on that obstack,
> so it would be better to free it after clearing the hash table.
> 

For me this causes:
...
(gdb) PASS: gdb.btrace/record_goto.exp: instruction-history from 19 forwards
record goto 27^M
/data/vries/gdb/src/gdb/record-btrace.c:1654: internal-error: 
bfcache_new: Assertion `*slot == NULL' failed.^M
A problem internal to GDB has been detected,^M
further debugging may prove unreliable.^M
----- Backtrace -----^M
FAIL: gdb.btrace/record_goto.exp: record goto 27 (GDB internal error)
...

Note that I've been having some problems with btrace tests, possible 
related to cpu/kernel combination (PRs 30073 and 30075), so this may be 
difficult to reproduce, I'm not sure.

Thanks,
- Tom

> Change-Id: If4f9b38266b458c4e2f7eb43e933090177c22190
> ---
>   gdb/frame.c | 39 ++++++++++++++++++++++++---------------
>   1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/gdb/frame.c b/gdb/frame.c
> index a08a8f47ebc4..fed961b2a8df 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -259,6 +259,22 @@ frame_addr_hash_eq (const void *a, const void *b)
>     return f_entry->this_id.value == f_element->this_id.value;
>   }
>   
> +/* Deletion function for the frame cache hash table.  */
> +
> +static void
> +frame_info_del (void *frame_v)
> +{
> +  frame_info *frame = (frame_info *) frame_v;
> +
> +  if (frame->prologue_cache != nullptr
> +      && frame->unwind->dealloc_cache != nullptr)
> +    frame->unwind->dealloc_cache (frame, frame->prologue_cache);
> +
> +  if (frame->base_cache != nullptr
> +      && frame->base->unwind->dealloc_cache != nullptr)
> +    frame->base->unwind->dealloc_cache (frame, frame->base_cache);
> +}
> +
>   /* Internal function to create the frame_stash hash table.  100 seems
>      to be a good compromise to start the hash table at.  */
>   
> @@ -268,7 +284,7 @@ frame_stash_create (void)
>     frame_stash = htab_create (100,
>   			     frame_addr_hash,
>   			     frame_addr_hash_eq,
> -			     NULL);
> +			     frame_info_del);
>   }
>   
>   /* Internal function to add a frame to the frame_stash hash table.
> @@ -2048,26 +2064,19 @@ reinit_frame_cache (void)
>   {
>     ++frame_cache_generation;
>   
> -  /* Tear down all frame caches.  */
> -  for (frame_info *fi = sentinel_frame; fi != NULL; fi = fi->prev)
> -    {
> -      if (fi->prologue_cache && fi->unwind->dealloc_cache)
> -	fi->unwind->dealloc_cache (fi, fi->prologue_cache);
> -      if (fi->base_cache && fi->base->unwind->dealloc_cache)
> -	fi->base->unwind->dealloc_cache (fi, fi->base_cache);
> -    }
> -
> -  /* Since we can't really be sure what the first object allocated was.  */
> -  obstack_free (&frame_cache_obstack, 0);
> -  obstack_init (&frame_cache_obstack);
> -
>     if (sentinel_frame != NULL)
>       annotate_frames_invalid ();
>   
> -  sentinel_frame = NULL;		/* Invalidate cache */
>     invalidate_selected_frame ();
> +
> +  /* Invalidate cache.  */
> +  sentinel_frame = NULL;
>     frame_stash_invalidate ();
>   
> +  /* Since we can't really be sure what the first object allocated was.  */
> +  obstack_free (&frame_cache_obstack, 0);
> +  obstack_init (&frame_cache_obstack);
> +
>     for (frame_info_ptr &iter : frame_info_ptr::frame_list)
>       iter.invalidate ();
>   

Re: [PATCH 1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache

[Tom de Vries]

On 2/9/23 08:40, Tom de Vries wrote:
> On 1/30/23 21:02, Simon Marchi via Gdb-patches wrote:
>> Currently, some frame resources are deallocated by iterating on the
>> frame chain (starting from the sentinel), calling dealloc_cache.  The
>> problem is that user-created frames are not part of that chain, so we
>> never call dealloc_cache for them.
>>
>> I propose to make it so the dealloc_cache callbacks are called when the
>> frames are removed from the frame_stash hash table, by registering a
>> deletion function to the hash table.  This happens when
>> frame_stash_invalidate is called by reinit_frame_cache.  This way, all
>> frames registered in the cache will get their unwinder's dealloc_cache
>> callbacks called.
>>
>> Note that at the moment, the sentinel frames are not registered in the
>> cache, so we won't call dealloc_cache for them.  However, it's just a
>> theoritical problem, because the sentinel frame unwinder does not
>> provide this callback.  Also, a subsequent patch will change things so
>> that sentinel frames are registered to the cache.
>>
>> I moved the obstack_free / obstack_init pair below the
>> frame_stash_invalidate call in reinit_frame_cache, because I assumed
>> that some dealloc_cache would need to access some data on that obstack,
>> so it would be better to free it after clearing the hash table.
>>
> 
> For me this causes:
> ...
> (gdb) PASS: gdb.btrace/record_goto.exp: instruction-history from 19 
> forwards
> record goto 27^M
> /data/vries/gdb/src/gdb/record-btrace.c:1654: internal-error: 
> bfcache_new: Assertion `*slot == NULL' failed.^M
> A problem internal to GDB has been detected,^M
> further debugging may prove unreliable.^M
> ----- Backtrace -----^M
> FAIL: gdb.btrace/record_goto.exp: record goto 27 (GDB internal error)
> ...
> 
> Note that I've been having some problems with btrace tests, possible 
> related to cpu/kernel combination (PRs 30073 and 30075), so this may be 
> difficult to reproduce, I'm not sure.
> 

I also managed to reproduce this on openSUSE Tumbleweed, which doesn't 
show the problems with btrace tests, so I'm hoping this is easy to 
reproduce.

Thanks,
- Tom

> Thanks,
> - Tom
> 
>> Change-Id: If4f9b38266b458c4e2f7eb43e933090177c22190
>> ---
>>   gdb/frame.c | 39 ++++++++++++++++++++++++---------------
>>   1 file changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index a08a8f47ebc4..fed961b2a8df 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -259,6 +259,22 @@ frame_addr_hash_eq (const void *a, const void *b)
>>     return f_entry->this_id.value == f_element->this_id.value;
>>   }
>> +/* Deletion function for the frame cache hash table.  */
>> +
>> +static void
>> +frame_info_del (void *frame_v)
>> +{
>> +  frame_info *frame = (frame_info *) frame_v;
>> +
>> +  if (frame->prologue_cache != nullptr
>> +      && frame->unwind->dealloc_cache != nullptr)
>> +    frame->unwind->dealloc_cache (frame, frame->prologue_cache);
>> +
>> +  if (frame->base_cache != nullptr
>> +      && frame->base->unwind->dealloc_cache != nullptr)
>> +    frame->base->unwind->dealloc_cache (frame, frame->base_cache);
>> +}
>> +
>>   /* Internal function to create the frame_stash hash table.  100 seems
>>      to be a good compromise to start the hash table at.  */
>> @@ -268,7 +284,7 @@ frame_stash_create (void)
>>     frame_stash = htab_create (100,
>>                    frame_addr_hash,
>>                    frame_addr_hash_eq,
>> -                 NULL);
>> +                 frame_info_del);
>>   }
>>   /* Internal function to add a frame to the frame_stash hash table.
>> @@ -2048,26 +2064,19 @@ reinit_frame_cache (void)
>>   {
>>     ++frame_cache_generation;
>> -  /* Tear down all frame caches.  */
>> -  for (frame_info *fi = sentinel_frame; fi != NULL; fi = fi->prev)
>> -    {
>> -      if (fi->prologue_cache && fi->unwind->dealloc_cache)
>> -    fi->unwind->dealloc_cache (fi, fi->prologue_cache);
>> -      if (fi->base_cache && fi->base->unwind->dealloc_cache)
>> -    fi->base->unwind->dealloc_cache (fi, fi->base_cache);
>> -    }
>> -
>> -  /* Since we can't really be sure what the first object allocated 
>> was.  */
>> -  obstack_free (&frame_cache_obstack, 0);
>> -  obstack_init (&frame_cache_obstack);
>> -
>>     if (sentinel_frame != NULL)
>>       annotate_frames_invalid ();
>> -  sentinel_frame = NULL;        /* Invalidate cache */
>>     invalidate_selected_frame ();
>> +
>> +  /* Invalidate cache.  */
>> +  sentinel_frame = NULL;
>>     frame_stash_invalidate ();
>> +  /* Since we can't really be sure what the first object allocated 
>> was.  */
>> +  obstack_free (&frame_cache_obstack, 0);
>> +  obstack_init (&frame_cache_obstack);
>> +
>>     for (frame_info_ptr &iter : frame_info_ptr::frame_list)
>>       iter.invalidate ();
> 

Re: [PATCH 1/2] gdb: call frame unwinders' dealloc_cache methods through destroying the frame cache

[Simon Marchi]

On 2/9/23 07:42, Tom de Vries via Gdb-patches wrote:
> On 2/9/23 08:40, Tom de Vries wrote:
>> On 1/30/23 21:02, Simon Marchi via Gdb-patches wrote:
>>> Currently, some frame resources are deallocated by iterating on the
>>> frame chain (starting from the sentinel), calling dealloc_cache.  The
>>> problem is that user-created frames are not part of that chain, so we
>>> never call dealloc_cache for them.
>>>
>>> I propose to make it so the dealloc_cache callbacks are called when the
>>> frames are removed from the frame_stash hash table, by registering a
>>> deletion function to the hash table.  This happens when
>>> frame_stash_invalidate is called by reinit_frame_cache.  This way, all
>>> frames registered in the cache will get their unwinder's dealloc_cache
>>> callbacks called.
>>>
>>> Note that at the moment, the sentinel frames are not registered in the
>>> cache, so we won't call dealloc_cache for them.  However, it's just a
>>> theoritical problem, because the sentinel frame unwinder does not
>>> provide this callback.  Also, a subsequent patch will change things so
>>> that sentinel frames are registered to the cache.
>>>
>>> I moved the obstack_free / obstack_init pair below the
>>> frame_stash_invalidate call in reinit_frame_cache, because I assumed
>>> that some dealloc_cache would need to access some data on that obstack,
>>> so it would be better to free it after clearing the hash table.
>>>
>>
>> For me this causes:
>> ...
>> (gdb) PASS: gdb.btrace/record_goto.exp: instruction-history from 19 forwards
>> record goto 27^M
>> /data/vries/gdb/src/gdb/record-btrace.c:1654: internal-error: bfcache_new: Assertion `*slot == NULL' failed.^M
>> A problem internal to GDB has been detected,^M
>> further debugging may prove unreliable.^M
>> ----- Backtrace -----^M
>> FAIL: gdb.btrace/record_goto.exp: record goto 27 (GDB internal error)
>> ...
>>
>> Note that I've been having some problems with btrace tests, possible related to cpu/kernel combination (PRs 30073 and 30075), so this may be difficult to reproduce, I'm not sure.
>>
> 
> I also managed to reproduce this on openSUSE Tumbleweed, which doesn't show the problems with btrace tests, so I'm hoping this is easy to reproduce.
> 
> Thanks,
> - Tom

Thanks for reporting.  I managed to reproduce on gcc13 (I only have
computers with AMD cpus at the moment, so I can't run the btrace tests).

I sent a patch, the explications of the cause and proposed solution are
there:

  https://inbox.sourceware.org/gdb-patches/20230209195037.100368-1-simon.marchi@efficios.com/T/#u

Simon