RE: [PATCH 1/1] gdb: set the cache information in 'get_prev_frame_maybe_check_cycle'
Andrew Burgess <[email protected]>
| Newsgroups | gmane.comp.gdb.patches |
|---|---|
| Message-ID | <[email protected]> |
"Rohr, Stephan" <[email protected]> writes: > Hi Andrew, > > Two more nits for the commit message, see my comment bellow. Hi Stephan, Sorry for the delay, I took some time to think about one of the issue you raised before responding. > >> -----Original Message----- >> From: Andrew Burgess <[email protected]> >> Sent: Wednesday, 8 July 2026 15:59 >> To: Rohr, Stephan <[email protected]> >> Cc: [email protected]; Tom Tromey <[email protected]> >> Subject: RE: [PATCH 1/1] gdb: set the cache information in >> 'get_prev_frame_maybe_check_cycle' >> >> "Rohr, Stephan" <[email protected]> writes: >> >> > HI Andrew, >> > >> > Thanks for sharing. The patch itself looks good. >> > >> > I have a few comments regarding the commit message, see below. >> >> Thanks Stephan. Below is an updated patch with an improved commit >> message. I also tweaked some of the comments in the actual code as, >> upon re-reading, I found some of them not ideal. >> >> Let me know what you think. >> >> Thanks, >> Andrew >> >> --- >> >> commit cec35caa5c173de5e98a9ac6cbc0b8ee8e15d912 >> Author: Andrew Burgess <[email protected]> >> Date: Thu Jun 25 14:58:50 2026 +0000 >> >> gdb: set frame_info_ptr::m_cached_id during invalidation >> >> Currently frame_info_ptr caches the frame_id at construction time, see >> frame_info_ptr::frame_info_ptr in frame.c. The problem with this is >> that a frame's frame-id might not be known at this point. >> >> Consider get_prev_frame_maybe_check_cycle, this calls >> get_prev_frame_raw to create the previous frame, placing the result >> into a frame_info_ptr PREV_FRAME. Then (for frames other than frame >> 0) compute_frame_id is called, however, this only computes the >> frame_id for the frame_info object pointed to by the frame_info_ptr, >> the cached frame_id within the frame_info_ptr itself is not updated. >> > > This is a long sentence. Consider splitting into two to improve readability. > I've reworked this, see the updated commit message below. >> What this means is that in get_prev_frame_maybe_check_cycle, the >> PREV_FRAME local has no cached frame-id. >> >> If we consider the call stack: >> >> get_selected_frame >> lookup_selected_frame >> frame_find_by_id >> get_prev_frame >> get_prev_frame_always >> get_prev_frame_always_1 >> get_prev_frame_maybe_check_cycle >> >> Then what we see is that the frame_info_ptr created in >> get_prev_frame_maybe_check_cycle, which lacks a cached frame_id, can >> be passed all the way back to lookup_selected_frame, where it will be >> stored in the SELECTED_FRAME global by a call to select_frame. The >> outer get_selected_frame call (in the above backtrace) will then >> return the SELECTED_FRAME global, which lacks a cached frame-id. >> >> If GDB ever tries to reinflate the SELECTED_FRAME frame_info_ptr (or a >> copy of it), then we will trigger the assert: >> `gdb_assert (frame_id_p (m_cached_id));` which can be found in >> `frame_info_ptr::reinflate` in frame.c. >> >> An example of how this can be triggered is included in the updated >> test case: >> >> - The 'up' command sets the selected frame to a frame with >> level > 0. >> - An inferior call invalidates the selected frame. >> - The selected frame is rebuilt following the call-stack above. >> The wrapping frame_info_ptr object doesn't cache the frame-id. >> - The 'frame' command invokes another inferior call for the pretty >> printer, which flushes the frame cache. >> - The frame_info_ptr is reinflated, e.g., to print the next >> argument, and this hits the assertion mentioned above. >> >> The problem is that frames don't always know their frame-id when they >> are placed into a frame_info_ptr, but they always do (for frames other >> than #0) after get_prev_frame_maybe_check_cycle has finished. This >> commit defers caching the frame-id in the frame_info_ptr until the >> frame cache is being flushed, at which point the frame-id is known. >> > > I think we shouldn't focus on 'get_prev_frame_maybe_check_cycle' solely, though > I didn't find any other location where the described behaviour could reproduce > (which doesn't mean it doesn't exist). I think in GDB right now get_prev_frame_maybe_check_cycle is the only place this bug exists. There are 3 places where new frame_info objects are created: create_sentinel_frame, create_new_frame, and get_prev_frame_raw. In the first two of these the frame_info is assigned an ID before being placed into the frame_info_ptr, so these are not problems. Only in get_prev_frame_raw is the frame_info placed into a frame_info_ptr before having an ID assigned, and that is only called from get_prev_frame_maybe_check_cycle. As we discussed in another thread, an ideal solution would be to have get_prev_frame_raw not create the frame_info_ptr at all, and defer this until get_prev_frame_maybe_check_cycle has finished, but this would require changing the frame sniffer API to not expect a frame_info_ptr, which seems like a bigger change than I'd like to make right now. But as I wrote the above I'm finding it harder to justify the churn of moving the frame_id caching into the frame_info_ptr::invalidate method. So, sorry to pivot again, but how about the patch below? It's far simpler than the original suggestion and is targets just get_prev_frame_maybe_check_cycle, which is where the broken frame_info_ptr objects always come from. Let me know what you think. Thanks, Andrew -- commit b6dba04e0e21f1a83b43ca616ae5fcce1a66eb9b Author: Andrew Burgess <[email protected]> Date: Thu Jun 25 14:58:50 2026 +0000 gdb: recreate the frame_info_ptr in get_prev_frame_maybe_check_cycle Currently frame_info_ptr caches the frame_id at construction time, see frame_info_ptr::frame_info_ptr in frame.c. The problem with this is that a frame's frame-id might not be known at this point. The function get_prev_frame_maybe_check_cycle calls get_prev_frame_raw to create the previous frame, placing the result into a frame_info_ptr PREV_FRAME. For frames other than frame 0, compute_frame_id is then called computing the frame-id. However, the call to compute_frame_id only updates the frame_info object itself, the frame_info_ptr PREV_FRAME is not updated with the new frame-id. What this means is that in get_prev_frame_maybe_check_cycle, the PREV_FRAME local has no cached frame-id. Consider the call stack: get_selected_frame lookup_selected_frame frame_find_by_id get_prev_frame get_prev_frame_always get_prev_frame_always_1 get_prev_frame_maybe_check_cycle What we see is that the frame_info_ptr created in get_prev_frame_maybe_check_cycle, which lacks a cached frame_id, can be passed all the way back to lookup_selected_frame, where it will be stored in the SELECTED_FRAME global by a call to select_frame. The outer get_selected_frame call (in the above backtrace) will then return the SELECTED_FRAME global, which lacks a cached frame-id. If GDB ever tries to reinflate the SELECTED_FRAME frame_info_ptr (or a copy of it), then we will trigger the assert: `gdb_assert (frame_id_p (m_cached_id));` which can be found in `frame_info_ptr::reinflate` in frame.c. An example of how this can be triggered is included in the updated test case: - The 'up' command sets the selected frame to a frame with level > 0. - An inferior call invalidates the selected frame. - The selected frame is rebuilt following the call-stack above. The wrapping frame_info_ptr object doesn't cache the frame-id. - The 'frame' command invokes another inferior call for the pretty printer, which flushes the frame cache. - The frame_info_ptr is reinflated, e.g., to print the next argument, and this hits the assertion mentioned above. There are only 3 places in GDB where new frame_info objects are created: create_sentinel_frame, create_new_frame, and get_prev_frame_raw. Of these, the first two always calculate the frame_id before placing the frame_info object into a frame_info_ptr. Only get_prev_frame_raw, which is only called from get_prev_frame_maybe_check_cycle, creates the frame_info_ptr before the frame_id is calculated. There are two places where PREV_FRAME is returned from get_prev_frame_maybe_check_cycle. The first is only for frame #0. The frame_info_ptr::reinflate method doesn't need a frame_id for frame #0, so the first return is not a problem. The second return from get_prev_frame_maybe_check_cycle is done after the frame_id has been calculated, and it is here that the problem can be fixed. If we create a new frame_info_ptr to replace PREV_FRAME then this new frame_info_ptr will have a cached frame_id and the problem described above will no longer occur. Co-Authored-By: Rohr, Stephan <[email protected]> diff --git a/gdb/frame.c b/gdb/frame.c index cefdde5ed1e..b91e18fad99 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -2332,7 +2332,16 @@ get_prev_frame_maybe_check_cycle (const frame_info_ptr &this_frame) throw; } - return prev_frame; + /* When PREV_FRAME was initially created it had no cached frame_id as the + frame_id had not yet been computed. Without a frame_id however + PREV_FRAME will not be able to reinflate. Recreate the frame_info_ptr + now that the frame_id is known, this new frame_info_ptr will have a + cached frame_id. + + You might wonder about the earlier return of PREV_FRAME within the + function. That is fine as reinflating a frame_info_ptr at level 0 + doesn't require a cached frame_id. */ + return frame_info_ptr (prev_frame.get ()); } /* Helper function for get_prev_frame_always, this is called inside a 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.*"] } }