[PATCH 0/4] Smart pointer wrapper for frame_info

[PATCH 0/4] Smart pointer wrapper for frame_info

[Tom Tromey]

GDB occasionally gets bugs where a frame_info is kept alive across a
call to reinit_frame_cache.  This causes a use-after-free and, if
you're lucky, a crash.

This series aims to make this setup more "reliable", in the sense that
you'll always get a crash if you break the rules.  This is done by
wrapping frame_info in a smart pointer class, and having
reinit_frame_cache invalidate all the pointers.

I looked into a more ambitious approach: having the frame_info
wrappers automatically "reinflate" on demand after invalidation.
However, I couldn't readily make this work.  I am not completely sure,
but I think there are a few issues.

First, sometimes reinit_frame_cache is called after other global state
has been changed.  This means that one can't call get_frame_id from
there.

Second, I think computing the frame_id requires some unwinding.  This
means that it doesn't make sense to eagerly compute the frame_id when
constructing a frame_info.

This brings up another point -- most frame_info_ptr objects will not
really be needed after a reinit.  So, saving them all is an
unnecessary expense.  But accepting this means, essentially, accepting
the status quo.

It's possible to add a "stash" method to the new smart pointer class,
to make selected instances auto-inflate.  However I haven't done so.
That may be more expensive as well, as these objects are often passed
by value in the current series; and again it doesn't seem hugely
better than the status quo.

Regression tested on x86-64 Fedora 34.

Let me know what you think.  I tend to think that at least patch #2
could go in regardless of whether the rest of this series does.

Tom

[PATCH 1/4] Switch order of comparison in some spots

[Tom Tromey]

In the smart wrapper for "frame_info *", I've implemented the
comparison operators as members.  This means that they are
order-dependent -- "x == NULL" works, but "NULL == x" does not.

I don't consider this a big deal, since the former is the normal
spelling anyway.  And, when building GDB, it only affected three
spots, fixed here.

It would be possible to reimplement these operators outside the class,
to allow both orders, if someone feels this is important.
---
 gdb/or1k-tdep.c             | 2 +-
 gdb/python/py-framefilter.c | 2 +-
 gdb/s12z-tdep.c             | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/or1k-tdep.c b/gdb/or1k-tdep.c
index 2b906fa69d3..6d5cc244dae 100644
--- a/gdb/or1k-tdep.c
+++ b/gdb/or1k-tdep.c
@@ -923,7 +923,7 @@ or1k_frame_cache (struct frame_info *this_frame, void **prologue_cache)
 
   /* Get the stack pointer if we have one (if there's no process executing
      yet we won't have a frame.  */
-  this_sp = (NULL == this_frame) ? 0 :
+  this_sp = (this_frame == NULL) ? 0 :
     get_frame_register_unsigned (this_frame, OR1K_SP_REGNUM);
 
   /* Return early if GDB couldn't find the function.  */
diff --git a/gdb/python/py-framefilter.c b/gdb/python/py-framefilter.c
index 366f3745b9d..cc0d30db7dd 100644
--- a/gdb/python/py-framefilter.c
+++ b/gdb/python/py-framefilter.c
@@ -871,7 +871,7 @@ py_print_frame (PyObject *filter, frame_filter_flags flags,
 	 where elided synthetic dummy-frames have to 'borrow' the frame
 	 architecture from the eliding frame.  If that is the case, do
 	 not print 'level', but print spaces.  */
-      if (*slot == frame)
+      if (frame == *slot)
 	out->field_skip ("level");
       else
 	{
diff --git a/gdb/s12z-tdep.c b/gdb/s12z-tdep.c
index 5394c1bbf5e..fdcb3f76aa0 100644
--- a/gdb/s12z-tdep.c
+++ b/gdb/s12z-tdep.c
@@ -288,7 +288,7 @@ s12z_frame_cache (struct frame_info *this_frame, void **prologue_cache)
 
   /* Get the stack pointer if we have one (if there's no process executing
      yet we won't have a frame.  */
-  this_sp = (NULL == this_frame) ? 0 :
+  this_sp = (this_frame == NULL) ? 0 :
     get_frame_register_unsigned (this_frame, REG_S);
 
   /* Return early if GDB couldn't find the function.  */
-- 
2.34.1

[PATCH 2/4] Remove frame_id_eq

[Tom Tromey]

This replaces frame_id_eq with operator== and operator!=.  I wrote
this for a version of this series that I later abandoned; but since it
simplifies the code, I left this patch in.
---
 gdb/breakpoint.c                 |  2 +-
 gdb/dummy-frame.c                |  4 +--
 gdb/elfread.c                    |  2 +-
 gdb/frame-unwind.c               |  2 +-
 gdb/frame.c                      | 27 +++++++++----------
 gdb/frame.h                      | 30 ++++++++-------------
 gdb/guile/scm-frame.c            |  2 +-
 gdb/ia64-tdep.c                  |  4 +--
 gdb/infrun.c                     | 46 +++++++++++++++-----------------
 gdb/mi/mi-cmd-stack.c            |  4 +--
 gdb/mi/mi-main.c                 |  2 +-
 gdb/python/py-finishbreakpoint.c |  2 +-
 gdb/python/py-frame.c            |  2 +-
 gdb/record-btrace.c              |  6 ++---
 gdb/stack.c                      |  4 +--
 gdb/tracepoint.c                 |  3 +--
 gdb/value.c                      |  2 +-
 17 files changed, 65 insertions(+), 79 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b21d83d8019..25ecd2a4447 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5218,7 +5218,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
      breakpoint or a single step breakpoint.  */
 
   if (frame_id_p (b->frame_id)
-      && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
+      && b->frame_id != get_stack_frame_id (get_current_frame ()))
     {
       bs->stop = 0;
       return;
diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index 40b455c7e4a..2fef6eae562 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -47,7 +47,7 @@ static int
 dummy_frame_id_eq (struct dummy_frame_id *id1,
 		   struct dummy_frame_id *id2)
 {
-  return frame_id_eq (id1->id, id2->id) && id1->thread == id2->thread;
+  return id1->id == id2->id && id1->thread == id2->thread;
 }
 
 /* List of dummy_frame destructors.  */
@@ -130,7 +130,7 @@ static bool
 pop_dummy_frame_bpt (struct breakpoint *b, struct dummy_frame *dummy)
 {
   if (b->thread == dummy->id.thread->global_num
-      && b->disposition == disp_del && frame_id_eq (b->frame_id, dummy->id.id))
+      && b->disposition == disp_del && b->frame_id == dummy->id.id)
     {
       while (b->related_breakpoint != b)
 	delete_breakpoint (b->related_breakpoint);
diff --git a/gdb/elfread.c b/gdb/elfread.c
index fb40032c505..bbec13d9772 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -946,7 +946,7 @@ elf_gnu_ifunc_resolver_stop (struct breakpoint *b)
 
       if (b_return->thread == thread_id
 	  && b_return->loc->requested_address == prev_pc
-	  && frame_id_eq (b_return->frame_id, prev_frame_id))
+	  && b_return->frame_id == prev_frame_id)
 	break;
     }
 
diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 302ce1efb99..91c40361d13 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -225,7 +225,7 @@ default_frame_unwind_stop_reason (struct frame_info *this_frame,
 {
   struct frame_id this_id = get_frame_id (this_frame);
 
-  if (frame_id_eq (this_id, outer_frame_id))
+  if (this_id == outer_frame_id)
     return UNWIND_OUTERMOST;
   else
     return UNWIND_NO_REASON;
diff --git a/gdb/frame.c b/gdb/frame.c
index 2b296ed6b96..2118805e00e 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -244,7 +244,7 @@ frame_addr_hash (const void *ap)
 }
 
 /* Internal equality function for the hash table.  This function
-   defers equality operations to frame_id_eq.  */
+   defers equality operations to frame_id::operator==.  */
 
 static int
 frame_addr_hash_eq (const void *a, const void *b)
@@ -252,8 +252,7 @@ frame_addr_hash_eq (const void *a, const void *b)
   const struct frame_info *f_entry = (const struct frame_info *) a;
   const struct frame_info *f_element = (const struct frame_info *) b;
 
-  return frame_id_eq (f_entry->this_id.value,
-		      f_element->this_id.value);
+  return f_entry->this_id.value == f_element->this_id.value;
 }
 
 /* Internal function to create the frame_stash hash table.  100 seems
@@ -752,28 +751,28 @@ frame_id_artificial_p (frame_id l)
 }
 
 bool
-frame_id_eq (frame_id l, frame_id r)
+frame_id::operator== (const frame_id &r) const
 {
   bool eq;
 
-  if (l.stack_status == FID_STACK_INVALID
+  if (stack_status == FID_STACK_INVALID
       || r.stack_status == FID_STACK_INVALID)
     /* Like a NaN, if either ID is invalid, the result is false.
        Note that a frame ID is invalid iff it is the null frame ID.  */
     eq = false;
-  else if (l.stack_status != r.stack_status || l.stack_addr != r.stack_addr)
+  else if (stack_status != r.stack_status || stack_addr != r.stack_addr)
     /* If .stack addresses are different, the frames are different.  */
     eq = false;
-  else if (l.code_addr_p && r.code_addr_p && l.code_addr != r.code_addr)
+  else if (code_addr_p && r.code_addr_p && code_addr != r.code_addr)
     /* An invalid code addr is a wild card.  If .code addresses are
        different, the frames are different.  */
     eq = false;
-  else if (l.special_addr_p && r.special_addr_p
-	   && l.special_addr != r.special_addr)
+  else if (special_addr_p && r.special_addr_p
+	   && special_addr != r.special_addr)
     /* An invalid special addr is a wild card (or unused).  Otherwise
        if special addresses are different, the frames are different.  */
     eq = false;
-  else if (l.artificial_depth != r.artificial_depth)
+  else if (artificial_depth != r.artificial_depth)
     /* If artificial depths are different, the frames must be different.  */
     eq = false;
   else
@@ -781,7 +780,7 @@ frame_id_eq (frame_id l, frame_id r)
     eq = true;
 
   frame_debug_printf ("l=%s, r=%s -> %d",
-		      l.to_string ().c_str (), r.to_string ().c_str (), eq);
+		      to_string ().c_str (), r.to_string ().c_str (), eq);
 
   return eq;
 }
@@ -875,7 +874,7 @@ frame_find_by_id (struct frame_id id)
     return NULL;
 
   /* Check for the sentinel frame.  */
-  if (frame_id_eq (id, sentinel_frame_id))
+  if (id == sentinel_frame_id)
     return sentinel_frame;
 
   /* Try using the frame stash first.  Finding it there removes the need
@@ -894,7 +893,7 @@ frame_find_by_id (struct frame_id id)
     {
       struct frame_id self = get_frame_id (frame);
 
-      if (frame_id_eq (id, self))
+      if (id == self)
 	/* An exact match.  */
 	return frame;
 
@@ -1738,7 +1737,7 @@ lookup_selected_frame (struct frame_id a_frame_id, int frame_level)
 	 it's highly unlikely the search by level finds the wrong
 	 frame, it's 99.9(9)% of the time (for all practical purposes)
 	 safe.  */
-      && frame_id_eq (get_frame_id (frame), a_frame_id))
+      && get_frame_id (frame) == a_frame_id)
     {
       /* Cool, all is fine.  */
       select_frame (frame);
diff --git a/gdb/frame.h b/gdb/frame.h
index b9a3793cc23..75bb3bd2aa0 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -173,6 +173,16 @@ struct frame_id
 
   /* Return a string representation of this frame id.  */
   std::string to_string () const;
+
+  /* Returns true when this frame_id and R identify the same
+     frame.  */
+  bool operator== (const frame_id &r) const;
+
+  /* Inverse of ==.  */
+  bool operator!= (const frame_id &r) const
+  {
+    return !(*this == r);
+  }
 };
 
 /* Save and restore the currently selected frame.  */
@@ -269,9 +279,6 @@ extern bool frame_id_p (frame_id l);
    TAILCALL_FRAME.  */
 extern bool frame_id_artificial_p (frame_id l);
 
-/* Returns true when L and R identify the same frame.  */
-extern bool frame_id_eq (frame_id l, frame_id r);
-
 /* Frame types.  Some are real, some are signal trampolines, and some
    are completely artificial (dummy).  */
 
@@ -498,22 +505,7 @@ extern CORE_ADDR get_frame_base (struct frame_info *);
 
 /* Return the per-frame unique identifer.  Can be used to relocate a
    frame after a frame cache flush (and other similar operations).  If
-   FI is NULL, return the null_frame_id.
-
-   NOTE: kettenis/20040508: These functions return a structure.  On
-   platforms where structures are returned in static storage (vax,
-   m68k), this may trigger compiler bugs in code like:
-
-   if (frame_id_eq (get_frame_id (l), get_frame_id (r)))
-
-   where the return value from the first get_frame_id (l) gets
-   overwritten by the second get_frame_id (r).  Please avoid writing
-   code like this.  Use code like:
-
-   struct frame_id id = get_frame_id (l);
-   if (frame_id_eq (id, get_frame_id (r)))
-
-   instead, since that avoids the bug.  */
+   FI is NULL, return the null_frame_id.  */
 extern struct frame_id get_frame_id (struct frame_info *fi);
 extern struct frame_id get_stack_frame_id (struct frame_info *fi);
 extern struct frame_id frame_unwind_caller_id (struct frame_info *next_frame);
diff --git a/gdb/guile/scm-frame.c b/gdb/guile/scm-frame.c
index 68fa35cf94c..ba9a33ed717 100644
--- a/gdb/guile/scm-frame.c
+++ b/gdb/guile/scm-frame.c
@@ -106,7 +106,7 @@ frscm_eq_frame_smob (const void *ap, const void *bp)
   const frame_smob *a = (const frame_smob *) ap;
   const frame_smob *b = (const frame_smob *) bp;
 
-  return (frame_id_eq (a->frame_id, b->frame_id)
+  return (a->frame_id == b->frame_id
 	  && a->inferior == b->inferior
 	  && a->inferior != NULL);
 }
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index 69a4b4db4fd..141e5b1337e 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -2901,7 +2901,7 @@ ia64_libunwind_frame_this_id (struct frame_info *this_frame, void **this_cache,
   CORE_ADDR bsp;
 
   libunwind_frame_this_id (this_frame, this_cache, &id);
-  if (frame_id_eq (id, outer_frame_id))
+  if (id == outer_frame_id)
     {
       (*this_id) = outer_frame_id;
       return;
@@ -3032,7 +3032,7 @@ ia64_libunwind_sigtramp_frame_this_id (struct frame_info *this_frame,
   struct frame_id id = outer_frame_id;
 
   libunwind_frame_this_id (this_frame, this_cache, &id);
-  if (frame_id_eq (id, outer_frame_id))
+  if (id == outer_frame_id)
     {
       (*this_id) = outer_frame_id;
       return;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 0b88eb8cfdf..28a826b23ef 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4531,7 +4531,7 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
        frame != NULL;
        frame = get_prev_frame (frame))
     {
-      if (frame_id_eq (get_frame_id (frame), step_frame_id))
+      if (get_frame_id (frame) == step_frame_id)
 	return true;
 
       if (get_frame_type (frame) != INLINE_FRAME)
@@ -4561,7 +4561,7 @@ inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
       symtab_and_line sal;
       struct symbol *sym;
 
-      if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+      if (get_frame_id (frame) == tp->control.step_frame_id)
 	break;
       if (get_frame_type (frame) != INLINE_FRAME)
 	break;
@@ -6542,8 +6542,8 @@ handle_signal_stop (struct execution_control_state *ecs)
 	  && (pc_in_thread_step_range (ecs->event_thread->stop_pc (),
 				       ecs->event_thread)
 	      || ecs->event_thread->control.step_range_end == 1)
-	  && frame_id_eq (get_stack_frame_id (frame),
-			  ecs->event_thread->control.step_stack_frame_id)
+	  && (get_stack_frame_id (frame)
+	      == ecs->event_thread->control.step_stack_frame_id)
 	  && ecs->event_thread->control.step_resume_breakpoint == NULL)
 	{
 	  /* The inferior is about to take a signal that will take it
@@ -6710,8 +6710,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 	  {
 	    struct frame_id current_id
 	      = get_frame_id (get_current_frame ());
-	    if (frame_id_eq (current_id,
-			     ecs->event_thread->initiating_frame))
+	    if (current_id == ecs->event_thread->initiating_frame)
 	      {
 		/* Case 2.  Fall through.  */
 	      }
@@ -6884,8 +6883,7 @@ process_event_stop_test (struct execution_control_state *ecs)
   if (pc_in_thread_step_range (ecs->event_thread->stop_pc (),
 			       ecs->event_thread)
       && (execution_direction != EXEC_REVERSE
-	  || frame_id_eq (get_frame_id (frame),
-			  ecs->event_thread->control.step_frame_id)))
+	  || get_frame_id (frame) == ecs->event_thread->control.step_frame_id))
     {
       infrun_debug_printf
 	("stepping inside range [%s-%s]",
@@ -7019,7 +7017,7 @@ process_event_stop_test (struct execution_control_state *ecs)
      previous frame's ID is sufficient - but it is a common case and
      cheaper than checking the previous frame's ID.
 
-     NOTE: frame_id_eq will never report two invalid frame IDs as
+     NOTE: frame_id::operator== will never report two invalid frame IDs as
      being equal, so to get into this block, both the current and
      previous frame must have valid frame IDs.  */
   /* The outer_frame_id check is a heuristic to detect stepping
@@ -7029,14 +7027,14 @@ process_event_stop_test (struct execution_control_state *ecs)
      "outermost" function.  This could be fixed by marking
      outermost frames as !stack_p,code_p,special_p.  Then the
      initial outermost frame, before sp was valid, would
-     have code_addr == &_start.  See the comment in frame_id_eq
+     have code_addr == &_start.  See the comment in frame_id::operator==
      for more.  */
-  if (!frame_id_eq (get_stack_frame_id (frame),
-		    ecs->event_thread->control.step_stack_frame_id)
-      && (frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
-		       ecs->event_thread->control.step_stack_frame_id)
-	  && (!frame_id_eq (ecs->event_thread->control.step_stack_frame_id,
-			    outer_frame_id)
+  if ((get_stack_frame_id (frame)
+       != ecs->event_thread->control.step_stack_frame_id)
+      && ((frame_unwind_caller_id (get_current_frame ())
+	   == ecs->event_thread->control.step_stack_frame_id)
+	  && ((ecs->event_thread->control.step_stack_frame_id
+	       != outer_frame_id)
 	      || (ecs->event_thread->control.step_start_function
 		  != find_pc_function (ecs->event_thread->stop_pc ())))))
     {
@@ -7293,8 +7291,8 @@ process_event_stop_test (struct execution_control_state *ecs)
      frame machinery detected some skipped call sites, we have entered
      a new inline function.  */
 
-  if (frame_id_eq (get_frame_id (get_current_frame ()),
-		   ecs->event_thread->control.step_frame_id)
+  if ((get_frame_id (get_current_frame ())
+       == ecs->event_thread->control.step_frame_id)
       && inline_skipped_frames (ecs->event_thread))
     {
       infrun_debug_printf ("stepped into inlined function");
@@ -7342,8 +7340,8 @@ process_event_stop_test (struct execution_control_state *ecs)
      through a more inlined call beyond its call site.  */
 
   if (get_frame_type (get_current_frame ()) == INLINE_FRAME
-      && !frame_id_eq (get_frame_id (get_current_frame ()),
-		       ecs->event_thread->control.step_frame_id)
+      && (get_frame_id (get_current_frame ())
+	  != ecs->event_thread->control.step_frame_id)
       && stepped_in_from (get_current_frame (),
 			  ecs->event_thread->control.step_frame_id))
     {
@@ -7375,8 +7373,8 @@ process_event_stop_test (struct execution_control_state *ecs)
 	  end_stepping_range (ecs);
 	  return;
 	}
-      else if (frame_id_eq (get_frame_id (get_current_frame ()),
-                           ecs->event_thread->control.step_frame_id))
+      else if (get_frame_id (get_current_frame ())
+               == ecs->event_thread->control.step_frame_id)
 	{
 	  /* We are not at the start of a statement, and we have not changed
 	     frame.
@@ -8438,8 +8436,8 @@ print_stop_location (const target_waitstatus &ws)
 	 should) carry around the function and does (or should) use
 	 that when doing a frame comparison.  */
       if (tp->control.stop_step
-	  && frame_id_eq (tp->control.step_frame_id,
-			  get_frame_id (get_current_frame ()))
+	  && (tp->control.step_frame_id
+	      == get_frame_id (get_current_frame ()))
 	  && (tp->control.step_start_function
 	      == find_pc_function (tp->stop_pc ())))
 	{
diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index e894411765a..65bf44ef66c 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -729,7 +729,7 @@ parse_frame_specification (const char *frame_exp)
        fid != NULL;
        fid = get_prev_frame (fid))
     {
-      if (frame_id_eq (id, get_frame_id (fid)))
+      if (id == get_frame_id (fid))
 	{
 	  struct frame_info *prev_frame;
 
@@ -737,7 +737,7 @@ parse_frame_specification (const char *frame_exp)
 	    {
 	      prev_frame = get_prev_frame (fid);
 	      if (!prev_frame
-		  || !frame_id_eq (id, get_frame_id (prev_frame)))
+		  || id != get_frame_id (prev_frame))
 		break;
 	      fid = prev_frame;
 	    }
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 18707bf62e7..6156a82b928 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2003,7 +2003,7 @@ struct user_selected_context
        reports not-equal, we only do the equality test if we have something
        other than the innermost frame selected.  */
     if (current_frame_level != -1
-	&& !frame_id_eq (current_frame_id, m_previous_frame_id))
+	&& current_frame_id != m_previous_frame_id)
       return true;
 
     /* Nothing changed!  */
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index 083694fbcce..7c0e9032dc5 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -207,7 +207,7 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	  else
 	    {
 	      frame_id = get_frame_id (prev_frame);
-	      if (frame_id_eq (frame_id, null_frame_id))
+	      if (frame_id == null_frame_id)
 		PyErr_SetString (PyExc_ValueError,
 				 _("Invalid ID for the `frame' object."));
 	    }
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index bf9eba89c5f..1c328176308 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -681,7 +681,7 @@ frapy_richcompare (PyObject *self, PyObject *other, int op)
   frame_object *other_frame = (frame_object *) other;
 
   if (self_frame->frame_id_is_next == other_frame->frame_id_is_next
-      && frame_id_eq (self_frame->frame_id, other_frame->frame_id))
+      && self_frame->frame_id == other_frame->frame_id)
     result = Py_EQ;
   else
     result = Py_NE;
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 1d5f839385c..c692c1c12bd 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2033,10 +2033,8 @@ record_btrace_start_replaying (struct thread_info *tp)
       frame_id = get_thread_current_frame_id (tp);
 
       /* Check if we need to update any stepping-related frame id's.  */
-      upd_step_frame_id = frame_id_eq (frame_id,
-				       tp->control.step_frame_id);
-      upd_step_stack_frame_id = frame_id_eq (frame_id,
-					     tp->control.step_stack_frame_id);
+      upd_step_frame_id = (frame_id == tp->control.step_frame_id);
+      upd_step_stack_frame_id = (frame_id == tp->control.step_stack_frame_id);
 
       /* We start replaying at the end of the branch trace.  This corresponds
 	 to the current instruction.  */
diff --git a/gdb/stack.c b/gdb/stack.c
index 1ccad83a2dd..efef62c3b72 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -3251,7 +3251,7 @@ find_frame_for_address (CORE_ADDR address)
        fid != NULL;
        fid = get_prev_frame (fid))
     {
-      if (frame_id_eq (id, get_frame_id (fid)))
+      if (id == get_frame_id (fid))
 	{
 	  struct frame_info *prev_frame;
 
@@ -3259,7 +3259,7 @@ find_frame_for_address (CORE_ADDR address)
 	    {
 	      prev_frame = get_prev_frame (fid);
 	      if (!prev_frame
-		  || !frame_id_eq (id, get_frame_id (prev_frame)))
+		  || id != get_frame_id (prev_frame))
 		break;
 	      fid = prev_frame;
 	    }
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index b45763ec02b..1dd718539f3 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2190,8 +2190,7 @@ tfind_1 (enum trace_find_type type, int num,
 	 function and it's arguments) -- otherwise we'll just show the
 	 new source line.  */
 
-      if (frame_id_eq (old_frame_id,
-		       get_frame_id (get_current_frame ())))
+      if (old_frame_id == get_frame_id (get_current_frame ()))
 	print_what = SRC_LINE;
       else
 	print_what = SRC_AND_LOC;
diff --git a/gdb/value.c b/gdb/value.c
index 24f1151c03f..b48a99ccdf0 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3975,7 +3975,7 @@ value_fetch_lazy_register (struct value *val)
 	 in this situation.  */
       if (VALUE_LVAL (new_val) == lval_register
 	  && value_lazy (new_val)
-	  && frame_id_eq (VALUE_NEXT_FRAME_ID (new_val), next_frame_id))
+	  && VALUE_NEXT_FRAME_ID (new_val) == next_frame_id)
 	internal_error (__FILE__, __LINE__,
 			_("infinite loop while fetching a register"));
     }
-- 
2.34.1

[PATCH 3/4] Introduce frame_info_ptr smart pointer class

[Tom Tromey]

This adds frame_info_ptr, a smart pointer class.  Every instance of
the class is kept on a circular, doubly-linked list.  When
reinit_frame_cache is called, the list is traversed and all the
pointers are invalidated.  This should help catch the typical GDB bug
of keeping a frame_info pointer alive where a frame ID was needed
instead.
---
 gdb/frame-info.h | 169 +++++++++++++++++++++++++++++++++++++++++++++++
 gdb/frame.c      |   8 +++
 gdb/frame.h      |   2 +
 3 files changed, 179 insertions(+)
 create mode 100644 gdb/frame-info.h

diff --git a/gdb/frame-info.h b/gdb/frame-info.h
new file mode 100644
index 00000000000..b567a53ccde
--- /dev/null
+++ b/gdb/frame-info.h
@@ -0,0 +1,169 @@
+/* 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/>.  */
+
+#ifndef GDB_FRAME_INFO_H
+#define GDB_FRAME_INFO_H
+
+struct frame_info;
+
+extern void reinit_frame_cache ();
+
+/* 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
+   detect a GDB bug that was relatively common.
+
+   A small amount of code must still operate on raw pointers, so a
+   "get" method is provided.  However, you should normally not use
+   this in new code.  */
+
+class frame_info_ptr
+{
+public:
+  /* Create a frame_info_ptr from a raw pointer.  */
+  explicit frame_info_ptr (struct frame_info *ptr)
+    : m_ptr (ptr),
+      m_next (&root),
+      m_prev (root.m_prev)
+  {
+    root.m_prev->m_next = this;
+    root.m_prev = this;
+  }
+
+  /* Create a null frame_info_ptr.  */
+  frame_info_ptr ()
+    : frame_info_ptr ((struct frame_info *) nullptr)
+  {
+  }
+
+  frame_info_ptr (std::nullptr_t)
+    : frame_info_ptr ((struct frame_info *) nullptr)
+  {
+  }
+
+  frame_info_ptr (const frame_info_ptr &other)
+    : frame_info_ptr (other.m_ptr)
+  {
+  }
+
+  frame_info_ptr (frame_info_ptr &&other)
+    : frame_info_ptr (other.m_ptr)
+  {
+  }
+
+  ~frame_info_ptr ()
+  {
+    m_next->m_prev = m_prev;
+    m_prev->m_next = m_next;
+  }
+
+  frame_info_ptr &operator= (const frame_info_ptr &other)
+  {
+    m_ptr = other.m_ptr;
+    return *this;
+  }
+
+  frame_info_ptr &operator= (std::nullptr_t)
+  {
+    m_ptr = nullptr;
+    return *this;
+  }
+
+  frame_info_ptr &operator= (frame_info_ptr &&other)
+  {
+    m_ptr = other.m_ptr;
+    return *this;
+  }
+
+  frame_info *operator-> () const
+  {
+    return m_ptr;
+  }
+
+  /* Fetch the underlying pointer.  Note that new code should
+     generally not use this -- avoid it if at all possible.  */
+  frame_info *get () const
+  {
+    return m_ptr;
+  }
+
+  /* This exists for compatibility with pre-existing code that checked
+     a "frame_info *" using "!".  */
+  bool operator! () const
+  {
+    return m_ptr == nullptr;
+  }
+
+  /* This exists for compatibility with pre-existing code that checked
+     a "frame_info *" like "if (ptr)".  */
+  explicit operator bool () const
+  {
+    return m_ptr != nullptr;
+  }
+
+  bool operator== (const frame_info_ptr &other) const
+  {
+    return m_ptr == other.m_ptr;
+  }
+
+  bool operator!= (const frame_info_ptr &other) const
+  {
+    return m_ptr != other.m_ptr;
+  }
+
+  bool operator== (const frame_info *other) const
+  {
+    return m_ptr == other;
+  }
+
+  bool operator!= (const frame_info *other) const
+  {
+    return m_ptr != other;
+  }
+
+private:
+
+  /* This constructor is used only for the root of the doubly-linked
+     list.  See "root", below.  It is explicit and given a parameter
+     to readily distinguish it from ordinary constructors.  */
+  explicit frame_info_ptr (bool ignored)
+    : m_ptr (nullptr),
+      m_next (this),
+      m_prev (this)
+  {
+  }
+
+  /* The underlying pointer.  */
+  frame_info *m_ptr;
+  /* Point to next and previous items in the circular list.  */
+  frame_info_ptr *m_next;
+  frame_info_ptr *m_prev;
+
+  /* All frame_info_ptr objects are kept on a circular doubly-linked
+     list.  This keeps their construction and destruction costs
+     reasonably small.  To make the implementation a little simpler,
+     we guarantee that there is always at least one object on the list
+     -- this "root".  */
+  static frame_info_ptr root;
+
+  /* A friend so it can invalidate the pointers.  */
+  friend void reinit_frame_cache ();
+};
+
+#endif /* GDB_FRAME_INFO_H */
diff --git a/gdb/frame.c b/gdb/frame.c
index 2118805e00e..35c8d7cf435 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -56,6 +56,9 @@ static struct frame_info *sentinel_frame;
 /* Number of calls to reinit_frame_cache.  */
 static unsigned int frame_cache_generation = 0;
 
+/* See frame-info.h.  */
+frame_info_ptr frame_info_ptr::root (true);
+
 /* See frame.h.  */
 
 unsigned int
@@ -2006,6 +2009,11 @@ reinit_frame_cache (void)
   select_frame (NULL);
   frame_stash_invalidate ();
 
+  for (frame_info_ptr *iter = frame_info_ptr::root.m_next;
+       iter != &frame_info_ptr::root;
+       iter = iter->m_next)
+    *iter = nullptr;
+
   frame_debug_printf ("generation=%d", frame_cache_generation);
 }
 
diff --git a/gdb/frame.h b/gdb/frame.h
index 75bb3bd2aa0..9ad2599331f 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -20,6 +20,8 @@
 #if !defined (FRAME_H)
 #define FRAME_H 1
 
+#include "frame-info.h"
+
 /* The following is the intended naming schema for frame functions.
    It isn't 100% consistent, but it is approaching that.  Frame naming
    schema:
-- 
2.34.1

Re: [PATCH 1/4] Switch order of comparison in some spots

[John Baldwin]

On 4/7/22 4:19 PM, Tom Tromey wrote:
> In the smart wrapper for "frame_info *", I've implemented the
> comparison operators as members.  This means that they are
> order-dependent -- "x == NULL" works, but "NULL == x" does not.
> 
> I don't consider this a big deal, since the former is the normal
> spelling anyway.  And, when building GDB, it only affected three
> spots, fixed here.
> 
> It would be possible to reimplement these operators outside the class,
> to allow both orders, if someone feels this is important.

Perhaps s/NULL/nullptr/ while you are here (that seems to be our practice
to switch to nullptr if we are already changing a line)?

-- 
John Baldwin

Re: [PATCH 2/4] Remove frame_id_eq

[John Baldwin]

On 4/7/22 4:19 PM, Tom Tromey wrote:
> This replaces frame_id_eq with operator== and operator!=.  I wrote
> this for a version of this series that I later abandoned; but since it
> simplifies the code, I left this patch in.

Definitely seems nicer and more C++y.

-- 
John Baldwin

Re: [PATCH 3/4] Introduce frame_info_ptr smart pointer class

[Bruno Larsen]

On 4/7/22 20:19, Tom Tromey wrote:
> This adds frame_info_ptr, a smart pointer class.  Every instance of
> the class is kept on a circular, doubly-linked list.  When
> reinit_frame_cache is called, the list is traversed and all the
> pointers are invalidated.  This should help catch the typical GDB bug
> of keeping a frame_info pointer alive where a frame ID was needed
> instead.

Thanks for taking this big first step. While I really like the idea, I am not convinced this is the best approach.

My first thought is using an STL data structure instead of re-implementing something by hand. std::list is exactly a doubly-linked list, and you could keep a reference to the list as a static member of the class, the constructor would add itself to the list, and the destructor would remove itself. While it isn't a beautiful approach, I feel like this could be better by the principle of not re-inventing the wheel. At the end I have a version that uses std::list instead (and fixes the c++ nits) so it's easier to see what I am thinking. It doesn't work, but I think it would be good enough for a PoC.

As for the cover letter issue of re-inflating the pointers, I can think of a few solutions: You could have an optional frame_id parameter, normally set to null, that when set will reinflate but won't calculate the ID when the pointer is requested, or you could use inheritance to have an obviously different behavior happening with obviously different types. I am not sure which is a better choice.

Some minor C++ nits follow, but I'm not sure where GDB stands with respect to those "C++ Good practices" I learned in uni. They are all assuming we are keeping the doubly-linked list. Also, I think patch 4/4 wasn't sent. Can you resend it?

> ---
>   gdb/frame-info.h | 169 +++++++++++++++++++++++++++++++++++++++++++++++
>   gdb/frame.c      |   8 +++
>   gdb/frame.h      |   2 +
>   3 files changed, 179 insertions(+)
>   create mode 100644 gdb/frame-info.h
> 
> diff --git a/gdb/frame-info.h b/gdb/frame-info.h
> new file mode 100644
> index 00000000000..b567a53ccde
> --- /dev/null
> +++ b/gdb/frame-info.h
> @@ -0,0 +1,169 @@
> +/* 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/>.  */
> +
> +#ifndef GDB_FRAME_INFO_H
> +#define GDB_FRAME_INFO_H
> +
> +struct frame_info;
> +
> +extern void reinit_frame_cache ();
> +
> +/* 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
> +   detect a GDB bug that was relatively common.
> +
> +   A small amount of code must still operate on raw pointers, so a
> +   "get" method is provided.  However, you should normally not use
> +   this in new code.  */
> +
> +class frame_info_ptr
> +{
> +public:
> +  /* Create a frame_info_ptr from a raw pointer.  */
> +  explicit frame_info_ptr (struct frame_info *ptr)
> +    : m_ptr (ptr),
> +      m_next (&root),
> +      m_prev (root.m_prev)
> +  {
> +    root.m_prev->m_next = this;
> +    root.m_prev = this;
> +  }
> +
> +  /* Create a null frame_info_ptr.  */
> +  frame_info_ptr ()
> +    : frame_info_ptr ((struct frame_info *) nullptr)
> +  {
> +  }
> +
> +  frame_info_ptr (std::nullptr_t)
> +    : frame_info_ptr ((struct frame_info *) nullptr)
> +  {
> +  }
> +
> +  frame_info_ptr (const frame_info_ptr &other)
> +    : frame_info_ptr (other.m_ptr)
> +  {
> +  }
> +
> +  frame_info_ptr (frame_info_ptr &&other)
> +    : frame_info_ptr (other.m_ptr)
> +  {
> +  }

The move constructor should probably use the m_next and m_prev from other - and set m_next->m_prev and m_prev->next accordingly - and set other->m_(next|prev) to nullptr or `this`, so we don't end up accidentally corrupting the list (all the more reason to use STL data structures IMHO). And move constructors usually set all data fields of other to 0 to avoid leaking data

> +
> +  ~frame_info_ptr ()
> +  {
> +    m_next->m_prev = m_prev;
> +    m_prev->m_next = m_next;
> +  }
> +
> +  frame_info_ptr &operator= (const frame_info_ptr &other)
> +  {
> +    m_ptr = other.m_ptr;
> +    return *this;
> +  }
> +
> +  frame_info_ptr &operator= (std::nullptr_t)
> +  {
> +    m_ptr = nullptr;
> +    return *this;
> +  }
> +
> +  frame_info_ptr &operator= (frame_info_ptr &&other)
> +  {
> +    m_ptr = other.m_ptr;
> +    return *this;
> +  }

Move operators also usually set other.m_ptr to nullptr, to avoid leaking data, but since all constructors should be including the class in the list, m_prev and m_next shouldn't be touched (different to the move constructor)

> +
> +  frame_info *operator-> () const
> +  {
> +    return m_ptr;
> +  }
> +
> +  /* Fetch the underlying pointer.  Note that new code should
> +     generally not use this -- avoid it if at all possible.  */
> +  frame_info *get () const
> +  {
> +    return m_ptr;
> +  }
> +
> +  /* This exists for compatibility with pre-existing code that checked
> +     a "frame_info *" using "!".  */
> +  bool operator! () const
> +  {
> +    return m_ptr == nullptr;
> +  }
> +
> +  /* This exists for compatibility with pre-existing code that checked
> +     a "frame_info *" like "if (ptr)".  */
> +  explicit operator bool () const
> +  {
> +    return m_ptr != nullptr;
> +  }

Usually C++ will work with whatever operators you give it, so if you just had the operator bool, the operator ! would deal with `! class.bool()`, which is what you are doing here, so I think there is no need to define the operator! like you did

> +
> +  bool operator== (const frame_info_ptr &other) const
> +  {
> +    return m_ptr == other.m_ptr;
> +  }
> +
> +  bool operator!= (const frame_info_ptr &other) const
> +  {
> +    return m_ptr != other.m_ptr;
> +  }

Same goes for this. a != b can be evaluated as !(a==b), so no need to define both.

> +
> +  bool operator== (const frame_info *other) const
> +  {
> +    return m_ptr == other;
> +  }
> +
> +  bool operator!= (const frame_info *other) const
> +  {
> +    return m_ptr != other;
> +  }
> +
> +private:
> +
> +  /* This constructor is used only for the root of the doubly-linked
> +     list.  See "root", below.  It is explicit and given a parameter
> +     to readily distinguish it from ordinary constructors.  */
> +  explicit frame_info_ptr (bool ignored)
> +    : m_ptr (nullptr),
> +      m_next (this),
> +      m_prev (this)
> +  {
> +  }

Since all constructors should set next and prev, and the empty constructor has no way to know where to point things, I think you could move the root initalization to use it and we wouldn't need this private constructor.

If you decide to use nullptrs for next and prev on the empty ctor, though, I'm ok with this.

> +
> +  /* The underlying pointer.  */
> +  frame_info *m_ptr;
> +  /* Point to next and previous items in the circular list.  */
> +  frame_info_ptr *m_next;
> +  frame_info_ptr *m_prev;
> +
> +  /* All frame_info_ptr objects are kept on a circular doubly-linked
> +     list.  This keeps their construction and destruction costs
> +     reasonably small.  To make the implementation a little simpler,
> +     we guarantee that there is always at least one object on the list
> +     -- this "root".  */
> +  static frame_info_ptr root;
> +
> +  /* A friend so it can invalidate the pointers.  */
> +  friend void reinit_frame_cache ();
> +};
> +
> +#endif /* GDB_FRAME_INFO_H */
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 2118805e00e..35c8d7cf435 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -56,6 +56,9 @@ static struct frame_info *sentinel_frame;
>   /* Number of calls to reinit_frame_cache.  */
>   static unsigned int frame_cache_generation = 0;
>   
> +/* See frame-info.h.  */
> +frame_info_ptr frame_info_ptr::root (true);
> +
>   /* See frame.h.  */
>   
>   unsigned int
> @@ -2006,6 +2009,11 @@ reinit_frame_cache (void)
>     select_frame (NULL);
>     frame_stash_invalidate ();
>   
> +  for (frame_info_ptr *iter = frame_info_ptr::root.m_next;
> +       iter != &frame_info_ptr::root;
> +       iter = iter->m_next)
> +    *iter = nullptr;
> +

I think an "invalidate" or "maybe_invalidate" method should be added to frame_info_ptr, which does this behind the scenes. This would make it trivial to use either approach to reinflate the pointers when we get to that.

>     frame_debug_printf ("generation=%d", frame_cache_generation);
>   }
>   
> diff --git a/gdb/frame.h b/gdb/frame.h
> index 75bb3bd2aa0..9ad2599331f 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -20,6 +20,8 @@
>   #if !defined (FRAME_H)
>   #define FRAME_H 1
>   
> +#include "frame-info.h"
> +
>   /* The following is the intended naming schema for frame functions.
>      It isn't 100% consistent, but it is approaching that.  Frame naming
>      schema:

My idea for using std::list is:

 From 7f24fe55a6df2e464f899d077f5a811593e6ded6 Mon Sep 17 00:00:00 2001
From: Tom Tromey <tom@tromey.com>
Date: Thu, 7 Apr 2022 17:19:04 -0600
Subject: [PATCH] Introduce frame_info_ptr smart pointer class

This adds frame_info_ptr, a smart pointer class.  Every instance of
the class is kept on a circular, doubly-linked list.  When
reinit_frame_cache is called, the list is traversed and all the
pointers are invalidated.  This should help catch the typical GDB bug
of keeping a frame_info pointer alive where a frame ID was needed
instead.

Co-Authored-By: Bruno Larsen <blarsen@redhat.com>
---
  gdb/frame-info.h | 150 +++++++++++++++++++++++++++++++++++++++++++++++
  gdb/frame.c      |   6 ++
  gdb/frame.h      |   2 +
  3 files changed, 158 insertions(+)
  create mode 100644 gdb/frame-info.h

diff --git a/gdb/frame-info.h b/gdb/frame-info.h
new file mode 100644
index 00000000000..bede86ad101
--- /dev/null
+++ b/gdb/frame-info.h
@@ -0,0 +1,150 @@
+/* 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/>.  */
+
+#ifndef GDB_FRAME_INFO_H
+#define GDB_FRAME_INFO_H
+
+#include <list>
+
+struct frame_info;
+
+extern void reinit_frame_cache ();
+
+/* 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
+   detect a GDB bug that was relatively common.
+
+   A small amount of code must still operate on raw pointers, so a
+   "get" method is provided.  However, you should normally not use
+   this in new code.  */
+
+class frame_info_ptr
+{
+public:
+  /* Create a frame_info_ptr from a raw pointer.  */
+  explicit frame_info_ptr (struct frame_info *ptr)
+    : m_ptr (ptr)
+  {
+    list.push_back(this);
+  }
+
+  /* Create a null frame_info_ptr.  */
+  frame_info_ptr ()
+    : frame_info_ptr ((struct frame_info *) nullptr)
+  {
+  }
+
+  frame_info_ptr (std::nullptr_t)
+    : frame_info_ptr ((struct frame_info *) nullptr)
+  {
+  }
+
+  frame_info_ptr (const frame_info_ptr &other)
+    : frame_info_ptr (other.m_ptr)
+  {
+  }
+
+  frame_info_ptr (frame_info_ptr &&other)
+    : m_ptr (other.m_ptr)
+  {
+      other.m_ptr = nullptr;
+      list.push_back(this);
+  }
+
+  ~frame_info_ptr ()
+  {
+    list.remove(this);
+  }
+
+  frame_info_ptr &operator= (const frame_info_ptr &other)
+  {
+    m_ptr = other.m_ptr;
+    return *this;
+  }
+
+  frame_info_ptr &operator= (std::nullptr_t)
+  {
+    m_ptr = nullptr;
+    return *this;
+  }
+
+  frame_info_ptr &operator= (frame_info_ptr &&other)
+  {
+    m_ptr = other.m_ptr;
+    other.m_ptr = nullptr;
+    return *this;
+  }
+
+  frame_info *operator-> () const
+  {
+    return m_ptr;
+  }
+
+  /* Fetch the underlying pointer.  Note that new code should
+     generally not use this -- avoid it if at all possible.  */
+  frame_info *get () const
+  {
+    return m_ptr;
+  }
+
+  /* This exists for compatibility with pre-existing code that checked
+     a "frame_info *" like "if (ptr)" or "if (!ptr)".  */
+  explicit operator bool () const
+  {
+    return m_ptr != nullptr;
+  }
+
+  bool operator== (const frame_info_ptr &other) const
+  {
+    return m_ptr == other.m_ptr;
+  }
+
+  bool operator== (const frame_info *other) const
+  {
+    return m_ptr == other;
+  }
+
+  bool operator!= (const frame_info *other) const
+  {
+    return m_ptr != other;
+  }
+
+  /* Invalidate or reinflate the pointers as requested upon
+     object initialization.  For now, always invalidate.  */
+  void maybe_invalidate ()
+  {
+    m_ptr = nullptr;
+  }
+
+private:
+
+  /* The underlying pointer.  */
+  frame_info *m_ptr;
+
+  /* All frame_info_ptr objects are kept on an STL queue, so we can
+     invalidate or reinflate them all when the frame cache is
+     reinitialized.  */
+  static std::list<frame_info_ptr *> list;
+
+  /* A friend so it can invalidate the pointers.  */
+  friend void reinit_frame_cache ();
+};
+
+#endif /* GDB_FRAME_INFO_H */
diff --git a/gdb/frame.c b/gdb/frame.c
index 2b296ed6b96..e50599c92d8 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -53,6 +53,9 @@
  
  static struct frame_info *sentinel_frame;
  
+/* see frame-info.h */
+std::list<frame_info_ptr *> frame_info_ptr::list;
+
  /* Number of calls to reinit_frame_cache.  */
  static unsigned int frame_cache_generation = 0;
  
@@ -2007,6 +2010,9 @@ reinit_frame_cache (void)
    select_frame (NULL);
    frame_stash_invalidate ();
  
+  for (frame_info_ptr *ptr : frame_info_ptr::list)
+    ptr->maybe_invalidate();
+
    frame_debug_printf ("generation=%d", frame_cache_generation);
  }
  
diff --git a/gdb/frame.h b/gdb/frame.h
index b9a3793cc23..2aee30c3137 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -20,6 +20,8 @@
  #if !defined (FRAME_H)
  #define FRAME_H 1
  
+#include "frame-info.h"
+
  /* The following is the intended naming schema for frame functions.
     It isn't 100% consistent, but it is approaching that.  Frame naming
     schema:
-- 
2.31.1


-- 
Cheers!
Bruno Larsen

Re: [PATCH 3/4] Introduce frame_info_ptr smart pointer class

[Tom Tromey]

Bruno> My first thought is using an STL data structure instead of
Bruno> re-implementing something by hand. std::list is exactly a
Bruno> doubly-linked list, and you could keep a reference to the list as a
Bruno> static member of the class, the constructor would add itself to the
Bruno> list, and the destructor would remove itself. While it isn't a
Bruno> beautiful approach, I feel like this could be better by the principle
Bruno> of not re-inventing the wheel

Removing an element from std::list is O(n), but for this implementation,
deletion is O(1).  This is important because the objects are passed by
value, so there are many creations and destructions.

Bruno> As for the cover letter issue of re-inflating the pointers, I can
Bruno> think of a few solutions: You could have an optional frame_id
Bruno> parameter, normally set to null, that when set will reinflate but
Bruno> won't calculate the ID when the pointer is requested

Yeah, I considered this but rejected it as no better than the current
approach.  It adds a word to frame_info_ptr -- a cost paid everywhere --
for the few uses that can just use get_frame_id, like they do now.

Bruno> Also, I think patch 4/4 wasn't sent. Can you resend it?

It's too large and is pending moderation.  I think I may have moderation
rights but I don't remember how.

>> +  frame_info_ptr (frame_info_ptr &&other)
>> +    : frame_info_ptr (other.m_ptr)
>> +  {
>> +  }

Bruno> The move constructor should probably use the m_next and m_prev from
Bruno> other - and set m_next->m_prev and m_prev->next accordingly - and set
Bruno> other-> m_(next|prev) to nullptr or `this`, so we don't end up
Bruno> accidentally corrupting the list (all the more reason to use STL data
Bruno> structures IMHO).

I don't think anything can be corrupted with this approach.
Each frame_info_ptr claims a spot on the global list at construction,
and that spot is "fixed" until destruction.

The bookkeeping of also moving the next/prev pointers seems more
complicated to me, but without much gain.

>> +  frame_info_ptr &operator= (frame_info_ptr &&other)
>> +  {
>> +    m_ptr = other.m_ptr;
>> +    return *this;
>> +  }

Bruno> Move operators also usually set other.m_ptr to nullptr, to avoid
Bruno> leaking data, but since all constructors should be including the
Bruno> class in the list, m_prev and m_next shouldn't be touched
Bruno> (different to the move constructor)

There's no possibility of leaks here.  A frame_info has a funny lifetime
and is owned by the frame cache, not by the frame_info_ptr.  I suppose
it is clearer to do the reset, though, to try to catch reuse of
moved-from objects.

>> +  bool operator!= (const frame_info_ptr &other) const
>> +  {
>> +    return m_ptr != other.m_ptr;
>> +  }

Bruno> Same goes for this. a != b can be evaluated as !(a==b), so no need to define both.

You'd have to rewrite the code to say that, though; but existing code
just uses !=.  I think C++20 provides some ways to automatically
generate these things, but gdb is still on C++11.

>> +  /* This constructor is used only for the root of the doubly-linked
>> +     list.  See "root", below.  It is explicit and given a parameter
>> +     to readily distinguish it from ordinary constructors.  */
>> +  explicit frame_info_ptr (bool ignored)
>> +    : m_ptr (nullptr),
>> +      m_next (this),
>> +      m_prev (this)
>> +  {
>> +  }

Bruno> Since all constructors should set next and prev, and the empty
Bruno> constructor has no way to know where to point things, I think you
Bruno> could move the root initalization to use it and we wouldn't need
Bruno> this private constructor.

I didn't want to do more work in the ordinary constructor -- my view is
that, because the objects are passed by value, it's important that the
constructors and destructors do as little work as possible.  So, I tried
to avoid even an 'if'.

I suppose the list doesn't need to be circular and there could be a
single root pointer, since the list is only ever traversed in one
direction.

Tom

Re: [PATCH 3/4] Introduce frame_info_ptr smart pointer class

[Pedro Alves]

I haven't read even half the patch, but I thought I'd fire away a quick comment here as the subject kind of
picked in a subthread.

On 2022-04-08 00:19, Tom Tromey wrote:
> +  /* Point to next and previous items in the circular list.  */
> +  frame_info_ptr *m_next;
> +  frame_info_ptr *m_prev;
> +

Did you consider using an intrusive_list instead of coding yet another linked list manually?
See thread_info, using an intrusive_list_node as a field:

  /* Step-over chain.  A thread is in the step-over queue if this node is
     linked.  */
  intrusive_list_node<thread_info> step_over_list_node;

Re: [PATCH 3/4] Introduce frame_info_ptr smart pointer class

[Bruno Larsen]

On 4/8/22 12:18, Tom Tromey wrote:
> Bruno> My first thought is using an STL data structure instead of
> Bruno> re-implementing something by hand. std::list is exactly a
> Bruno> doubly-linked list, and you could keep a reference to the list as a
> Bruno> static member of the class, the constructor would add itself to the
> Bruno> list, and the destructor would remove itself. While it isn't a
> Bruno> beautiful approach, I feel like this could be better by the principle
> Bruno> of not re-inventing the wheel
> 
> Removing an element from std::list is O(n), but for this implementation,
> deletion is O(1).  This is important because the objects are passed by
> value, so there are many creations and destructions.

This is one of the things that a move constructor with all the bookkeeping I mentioned below. You just swap the position instead of adding a new one. My version was incomplete, but you could have an O(1) replacing with a move constructor, which eliminates a lot of the problems with passing by value. In this case, we'd probably want a std::vector instead, because of the O(1) accessing time, but I understand your point and I agree that a doubly-linked list could be better.

> 
> Bruno> As for the cover letter issue of re-inflating the pointers, I can
> Bruno> think of a few solutions: You could have an optional frame_id
> Bruno> parameter, normally set to null, that when set will reinflate but
> Bruno> won't calculate the ID when the pointer is requested
> 
> Yeah, I considered this but rejected it as no better than the current
> approach.  It adds a word to frame_info_ptr -- a cost paid everywhere --
> for the few uses that can just use get_frame_id, like they do now.

Yeah, that's fair. This is one reason why I wasn't sure if it was a good idea. The inheritance model doesn't have this problem, so we could have reinflatable_frame_info for instance, which will save the frame_id.

> 
> Bruno> Also, I think patch 4/4 wasn't sent. Can you resend it?
> 
> It's too large and is pending moderation.  I think I may have moderation
> rights but I don't remember how.

got it.

> 
>>> +  frame_info_ptr (frame_info_ptr &&other)
>>> +    : frame_info_ptr (other.m_ptr)
>>> +  {
>>> +  }
> 
> Bruno> The move constructor should probably use the m_next and m_prev from
> Bruno> other - and set m_next->m_prev and m_prev->next accordingly - and set
> Bruno> other-> m_(next|prev) to nullptr or `this`, so we don't end up
> Bruno> accidentally corrupting the list (all the more reason to use STL data
> Bruno> structures IMHO).
> 
> I don't think anything can be corrupted with this approach.
> Each frame_info_ptr claims a spot on the global list at construction,
> and that spot is "fixed" until destruction.
> 
> The bookkeeping of also moving the next/prev pointers seems more
> complicated to me, but without much gain.

I meant that if you swapped the pointers, the old ones would need to be changed to avoid corruption. It isn't really needed, but it is more in-line with what usually happens on move constructors, otherwise what you have here is a copy constructor with extra steps.

> 
>>> +  frame_info_ptr &operator= (frame_info_ptr &&other)
>>> +  {
>>> +    m_ptr = other.m_ptr;
>>> +    return *this;
>>> +  }
> 
> Bruno> Move operators also usually set other.m_ptr to nullptr, to avoid
> Bruno> leaking data, but since all constructors should be including the
> Bruno> class in the list, m_prev and m_next shouldn't be touched
> Bruno> (different to the move constructor)
> 
> There's no possibility of leaks here.  A frame_info has a funny lifetime
> and is owned by the frame cache, not by the frame_info_ptr.  I suppose
> it is clearer to do the reset, though, to try to catch reuse of
> moved-from objects.
> 
>>> +  bool operator!= (const frame_info_ptr &other) const
>>> +  {
>>> +    return m_ptr != other.m_ptr;
>>> +  }
> 
> Bruno> Same goes for this. a != b can be evaluated as !(a==b), so no need to define both.
> 
> You'd have to rewrite the code to say that, though; but existing code
> just uses !=.  I think C++20 provides some ways to automatically
> generate these things, but gdb is still on C++11.

Ah, I tested the operator bool() and ! and it worked as far back as gcc 8.5, so I assumed that == and != would be the same.

> 
>>> +  /* This constructor is used only for the root of the doubly-linked
>>> +     list.  See "root", below.  It is explicit and given a parameter
>>> +     to readily distinguish it from ordinary constructors.  */
>>> +  explicit frame_info_ptr (bool ignored)
>>> +    : m_ptr (nullptr),
>>> +      m_next (this),
>>> +      m_prev (this)
>>> +  {
>>> +  }
> 
> Bruno> Since all constructors should set next and prev, and the empty
> Bruno> constructor has no way to know where to point things, I think you
> Bruno> could move the root initalization to use it and we wouldn't need
> Bruno> this private constructor.
> 
> I didn't want to do more work in the ordinary constructor -- my view is
> that, because the objects are passed by value, it's important that the
> constructors and destructors do as little work as possible.  So, I tried
> to avoid even an 'if'.
> 
> I suppose the list doesn't need to be circular and there could be a
> single root pointer, since the list is only ever traversed in one
> direction.

If the list is no longer circular, destructors aren't O(1) anymore. In this case, we are back at using the vector, or needing the move constructor bookkeeping.

> 
> Tom
> 


-- 
Cheers!
Bruno Larsen

Re: [PATCH 1/4] Switch order of comparison in some spots

[Pedro Alves]

On 2022-04-08 00:19, Tom Tromey wrote:
> In the smart wrapper for "frame_info *", I've implemented the
> comparison operators as members.  This means that they are
> order-dependent -- "x == NULL" works, but "NULL == x" does not.
> 
> I don't consider this a big deal, since the former is the normal
> spelling anyway.  And, when building GDB, it only affected three
> spots, fixed here.
> 
> It would be possible to reimplement these operators outside the class,
> to allow both orders, if someone feels this is important.

Offhand, I do, but just because feels weirdly unnecessary to not make it completely
correct.  I don't think implementing the operators outside a class should
be any harder than as methods.  If the problem is access to internal details,
you can declare the global operator== as a friend:

  friend bool operator==(const mytype&, const mytype&);

Re: [PATCH 3/4] Introduce frame_info_ptr smart pointer class

[Tom Tromey]

Pedro> Did you consider using an intrusive_list instead of coding yet
Pedro> another linked list manually?

No, I don't think I remember this one.

I'll do it but I notice that intrusive_list_node leaves the members
public, which seems sub-optimal to me.

Tom

Re: [PATCH 3/4] Introduce frame_info_ptr smart pointer class

[Pedro Alves]

On 2022-04-08 20:03, Tom Tromey wrote:
> Pedro> Did you consider using an intrusive_list instead of coding yet
> Pedro> another linked list manually?
> 
> No, I don't think I remember this one.
> 
> I'll do it but I notice that intrusive_list_node leaves the members
> public, which seems sub-optimal to me.
> 
> Tom

True, though there's no real need for it.  GDB compiles fine with this:

From 2c7c5814e7028962e1a4dd95afd81d13f78024b2 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 8 Apr 2022 20:03:46 +0100
Subject: [PATCH] private

Change-Id: Ia8b306b40344cc218d423c8dfb8355207a612ac5
---
 gdbsupport/intrusive_list.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gdbsupport/intrusive_list.h b/gdbsupport/intrusive_list.h
index 77eeeeec5fd..1d87e47a8d9 100644
--- a/gdbsupport/intrusive_list.h
+++ b/gdbsupport/intrusive_list.h
@@ -31,8 +31,18 @@ struct intrusive_list_node
     return next != INTRUSIVE_LIST_UNLINKED_VALUE;
   }
 
+private:
   T *next = INTRUSIVE_LIST_UNLINKED_VALUE;
   T *prev = INTRUSIVE_LIST_UNLINKED_VALUE;
+
+  template<typename T2, typename AsNode>
+  friend struct intrusive_list_iterator;
+
+  template<typename T2, typename AsNode>
+  friend struct intrusive_list_reverse_iterator;
+
+  template<typename T2, typename AsNode>
+  friend struct intrusive_list;
 };
 
 /* Follows a couple types used by intrusive_list as template parameter to find

Re: [PATCH 0/4] Smart pointer wrapper for frame_info

[Bruno Larsen]

How goes this series? I was waiting on this to move forward with my adjacent series fixing the crash when python calls a function by hand.

Cheers!
Bruno Larsen

On 4/7/22 20:19, Tom Tromey wrote:
> GDB occasionally gets bugs where a frame_info is kept alive across a
> call to reinit_frame_cache.  This causes a use-after-free and, if
> you're lucky, a crash.
> 
> This series aims to make this setup more "reliable", in the sense that
> you'll always get a crash if you break the rules.  This is done by
> wrapping frame_info in a smart pointer class, and having
> reinit_frame_cache invalidate all the pointers.
> 
> I looked into a more ambitious approach: having the frame_info
> wrappers automatically "reinflate" on demand after invalidation.
> However, I couldn't readily make this work.  I am not completely sure,
> but I think there are a few issues.
> 
> First, sometimes reinit_frame_cache is called after other global state
> has been changed.  This means that one can't call get_frame_id from
> there.
> 
> Second, I think computing the frame_id requires some unwinding.  This
> means that it doesn't make sense to eagerly compute the frame_id when
> constructing a frame_info.
> 
> This brings up another point -- most frame_info_ptr objects will not
> really be needed after a reinit.  So, saving them all is an
> unnecessary expense.  But accepting this means, essentially, accepting
> the status quo.
> 
> It's possible to add a "stash" method to the new smart pointer class,
> to make selected instances auto-inflate.  However I haven't done so.
> That may be more expensive as well, as these objects are often passed
> by value in the current series; and again it doesn't seem hugely
> better than the status quo.
> 
> Regression tested on x86-64 Fedora 34.
> 
> Let me know what you think.  I tend to think that at least patch #2
> could go in regardless of whether the rest of this series does.
> 
> Tom
> 
> 

Re: [PATCH 0/4] Smart pointer wrapper for frame_info

[Tom Tromey]

Bruno> How goes this series? I was waiting on this to move forward with
Bruno> my adjacent series fixing the crash when python calls a function
Bruno> by hand.

It got bogged down in the intrusive list change.
You shouldn't wait for it.

Tom