[binutils-gdb] gdb: add special handling for frame level 0 in frame_info_ptr

[binutils-gdb] gdb: add special handling for frame level 0 in frame_info_ptr

[Simon Marchi]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=f71e3f86e83c9c22e3d86112b5ddb61919390a1a

commit f71e3f86e83c9c22e3d86112b5ddb61919390a1a
Author: Simon Marchi <simon.marchi@efficios.com>
Date:   Mon Oct 24 16:18:43 2022 -0400

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

Diff:
---
 gdb/frame-info.c | 26 ++++++++++++++++++++++----
 gdb/frame-info.h | 20 ++++++++++++++++++--
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/gdb/frame-info.c b/gdb/frame-info.c
index 584222dc490..40a872ea152 100644
--- a/gdb/frame-info.c
+++ b/gdb/frame-info.c
@@ -31,7 +31,10 @@ intrusive_list<frame_info_ptr> frame_info_ptr::frame_list;
 void
 frame_info_ptr::prepare_reinflate ()
 {
-  m_cached_id = get_frame_id (*this);
+  m_cached_level = frame_relative_level (*this);
+
+  if (m_cached_level != 0)
+    m_cached_id = get_frame_id (*this);
 }
 
 /* See frame-info-ptr.h.  */
@@ -39,9 +42,24 @@ frame_info_ptr::prepare_reinflate ()
 void
 frame_info_ptr::reinflate ()
 {
-  gdb_assert (frame_id_p (m_cached_id));
+  /* Ensure we have a valid frame level (sentinel frame or above), indicating
+     prepare_reinflate was called.  */
+  gdb_assert (m_cached_level >= -1);
+
+  if (m_ptr != nullptr)
+    {
+      /* The frame_info wasn't invalidated, no need to reinflate.  */
+      return;
+    }
+
+  /* Frame #0 needs special handling, see comment in select_frame.  */
+  if (m_cached_level == 0)
+    m_ptr = get_current_frame ().get ();
+  else
+    {
+      gdb_assert (frame_id_p (m_cached_id));
+      m_ptr = frame_find_by_id (m_cached_id).get ();
+    }
 
-  if (m_ptr == nullptr)
-    m_ptr = frame_find_by_id (m_cached_id).get ();
   gdb_assert (m_ptr != nullptr);
 }
diff --git a/gdb/frame-info.h b/gdb/frame-info.h
index 1d2d4bdc7e6..3369b114326 100644
--- a/gdb/frame-info.h
+++ b/gdb/frame-info.h
@@ -56,16 +56,21 @@ public:
   }
 
   frame_info_ptr (const frame_info_ptr &other)
-    : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id)
+    : m_ptr (other.m_ptr),
+      m_cached_id (other.m_cached_id),
+      m_cached_level (other.m_cached_level)
   {
     frame_list.push_back (*this);
   }
 
   frame_info_ptr (frame_info_ptr &&other)
-    : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id)
+    : m_ptr (other.m_ptr),
+      m_cached_id (other.m_cached_id),
+      m_cached_level (other.m_cached_level)
   {
     other.m_ptr = nullptr;
     other.m_cached_id = null_frame_id;
+    other.m_cached_level = invalid_level;
     frame_list.push_back (*this);
   }
 
@@ -78,6 +83,7 @@ public:
   {
     m_ptr = other.m_ptr;
     m_cached_id = other.m_cached_id;
+    m_cached_level = other.m_cached_level;
     return *this;
   }
 
@@ -85,6 +91,7 @@ public:
   {
     m_ptr = nullptr;
     m_cached_id = null_frame_id;
+    m_cached_level = invalid_level;
     return *this;
   }
 
@@ -92,8 +99,10 @@ public:
   {
     m_ptr = other.m_ptr;
     m_cached_id = other.m_cached_id;
+    m_cached_level = other.m_cached_level;
     other.m_ptr = nullptr;
     other.m_cached_id = null_frame_id;
+    other.m_cached_level = invalid_level;
     return *this;
   }
 
@@ -136,6 +145,10 @@ public:
   void reinflate ();
 
 private:
+  /* We sometimes need to construct frame_info_ptr objects around the
+     sentinel_frame, which has level -1.  Therefore, make the invalid frame
+     level value -2.  */
+  static constexpr int invalid_level = -2;
 
   /* The underlying pointer.  */
   frame_info *m_ptr = nullptr;
@@ -143,6 +156,9 @@ private:
   /* The frame_id of the underlying pointer.  */
   frame_id m_cached_id = null_frame_id;
 
+  /* The frame level of the underlying pointer.  */
+  int m_cached_level = invalid_level;
+
   /* All frame_info_ptr objects are kept on an intrusive list.
      This keeps their construction and destruction costs
      reasonably small.  */