RE: [PATCH 1/1] gdb: set the cache information in 'get_prev_frame_maybe_check_cycle'

[Andrew Burgess <aburgess@redhat.com>]

"Rohr, Stephan" <stephan.rohr@intel.com> writes:

> Hi Tom,
>
> thank you for your feedback!  I will fix the typos in v2 of the patch.
>
> I'll try to explain the problem in more detail now:
>
>   1. If you run an inferior call after the up command invalidates
>       the selected frame.
>   2. The selected frame is rebuild following the chain
>
>         get_selected_frame () -> lookup_selected_frame ()
>         -> get_prev_frame_always_1 ()
>         -> get_prev_frame_maybe_check_cycle ()
>
>   3. At this point, the frame_info_ptr is constructed before the
>       frame-id is computed.  The cache Information is not updated as
>       'compute_frame_id' only updates the frame id of the internal
>       'frame_info' pointer.
>
>   4. If we now run the "frame" command in combination with a
>       pretty-printer, this invokes another infcall to evaluate the
>       pretty-printer.
>
>   5. This clears the 'm_ptr' member of the frame_info_ptr. Since the cache
>        id is not set, a subsequent reinflate fails.
>
> A very simple fix for this could be:
>
> @@ -2332,7 +2361,7 @@ get_prev_frame_maybe_check_cycle (const frame_info_ptr &this_frame)
>        throw;
>      }
>
> -  return prev_frame;
> +  return frame_info_ptr (prev_frame.get ());
>  }
>
> It constructs a new frame_info_ptr using the updated raw pointer
> of 'prev_frame'.  This is somewhat redundant as another frame_info_ptr
> is added to the frame list in the frame_info_ptr ctor.
>
> The frame list entry from 
>
>    frame_info_ptr prev_frame = get_prev_frame_raw (this_frame);
>
> is removed again by the frame_info_ptr's dtor.
>
> This would add some overhead. 
>
> I didn't find a better solution.  We could modify 'get_prev_frame_raw' to
> return a raw pointer instead (it is the only location where is this called at
> all).  But we'd need a temporary copy of the frame_info_ptr anyways,
> either to pass a frame_info_ptr to 'compute_frame_id' or inside of
> 'compute_frame_id' (to forward the frame id to
> 'frame_unwind_find_by_frame').  Changing these functions to
> accept a raw frame_info pointer is not desired in my point of view.
>
> I appreciate your feedback.
>

Hi Stephan,

I ran into a similar problem myself recently, and had a different fix
queued up, but it was part of a larger change.  I've pulled it out and
the patch is below, along with your test change.  The test still passes.

There's no commit message yet, I still need to think about exactly
what's going on in this case a bit more, it's still not clear to me how
(or where) the frame_info_ptr without the frame-id is actually created,
as I thought frames (after 0) always got a frame-id before they were
placed into a frame_info_ptr.

I'm sharing this now just so you can take a look at the change and let
me know what you think, I'll dig into this a little more and write up a
commit message tomorrow, but I'd like to hear what you think of this
approach.

Thanks,
Andrew

---

diff --git a/gdb/frame.c b/gdb/frame.c
index cefdde5ed1e..7a0f313f4db 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -2190,15 +2190,15 @@ reinit_frame_cache (void)
       sentinel_frame = nullptr;
     }
 
+  for (frame_info_ptr &iter : frame_info_ptr::frame_list)
+    iter.invalidate ();
+
   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 ();
-
   frame_debug_printf ("generation=%d", frame_cache_generation);
 }
 
@@ -3435,10 +3435,21 @@ frame_info_ptr::frame_info_ptr (struct frame_info *ptr)
   if (m_ptr == nullptr)
     return;
 
-  m_cached_level = ptr->level;
+  m_cached_level = m_ptr->level;
+}
+
+void
+frame_info_ptr::invalidate ()
+{
+  if (m_ptr == nullptr)
+    return;
+
+  gdb_assert (m_cached_level == m_ptr->level);
 
   if (m_cached_level != 0 || m_ptr->this_id.value.user_created_p)
     m_cached_id = m_ptr->this_id.value;
+
+  m_ptr = nullptr;
 }
 
 /* See frame-info-ptr.h.  */
diff --git a/gdb/frame.h b/gdb/frame.h
index f6553fb7b6d..b386303d895 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -327,10 +327,7 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
   }
 
   /* Invalidate this pointer.  */
-  void invalidate ()
-  {
-    m_ptr = nullptr;
-  }
+  void invalidate ();
 
 private:
   /* We sometimes need to construct frame_info_ptr objects around the
diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
index 52162fc9952..a2a29c4d0f8 100644
--- a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
@@ -108,6 +108,8 @@ with_test_prefix "frame movement down" {
 with_test_prefix "frame movement up" {
     if { [start_test "TAG: final frame"] == 0 } {
 	gdb_test "up" [multi_line "#1 .*in g \\(mt=mytype is .*\\, depth=1\\).*" ".*first frame.*"]
+	gdb_test "p f ()" " = 2"
+	gdb_test "frame" [multi_line "#1 .*in g \\(mt=mytype is .*\\, depth=1\\).*" ".*first frame.*"]
     }
 }