diff --git a/_gdb.spec.Patch.include b/_gdb.spec.Patch.include index ba7cbe9..ce58612 100644 --- a/_gdb.spec.Patch.include +++ b/_gdb.spec.Patch.include @@ -407,3 +407,9 @@ Patch099: gdb-rhbz1930528-fix-gnulib-build-error.patch # (RH BZ 1932645). Patch100: gdb-rhbz1932645-aarch64-ptrace-header-order.patch +# Backport fix for frame_id_p assertion failure (RH BZ 1909902). +Patch101: gdb-rhbz1909902-frame_id_p-assert-1.patch + +# Backport patch #2 which fixes a frame_id_p assertion failure (RH BZ 1909902). +Patch102: gdb-rhbz1909902-frame_id_p-assert-2.patch + diff --git a/_gdb.spec.patch.include b/_gdb.spec.patch.include index fd3cf02..48f09cd 100644 --- a/_gdb.spec.patch.include +++ b/_gdb.spec.patch.include @@ -98,3 +98,5 @@ %patch098 -p1 %patch099 -p1 %patch100 -p1 +%patch101 -p1 +%patch102 -p1 diff --git a/_patch_order b/_patch_order index 76a6a2f..9229088 100644 --- a/_patch_order +++ b/_patch_order @@ -98,3 +98,5 @@ gdb-rhbz1905996-fix-off-by-one-error-in-ada_fold_name.patch gdb-rhbz1912985-libstdc++-assert.patch gdb-rhbz1930528-fix-gnulib-build-error.patch gdb-rhbz1932645-aarch64-ptrace-header-order.patch +gdb-rhbz1909902-frame_id_p-assert-1.patch +gdb-rhbz1909902-frame_id_p-assert-2.patch diff --git a/gdb-rhbz1909902-frame_id_p-assert-1.patch b/gdb-rhbz1909902-frame_id_p-assert-1.patch new file mode 100644 index 0000000..7bced66 --- /dev/null +++ b/gdb-rhbz1909902-frame_id_p-assert-1.patch @@ -0,0 +1,593 @@ +From FEDORA_PATCHES Mon Sep 17 00:00:00 2001 +From: Pedro Alves +Date: Fri, 30 Oct 2020 18:26:15 +0100 +Subject: gdb-rhbz1909902-frame_id_p-assert-1.patch + +;; Backport fix for frame_id_p assertion failure (RH BZ 1909902). + +Make scoped_restore_current_thread's cdtors exception free (RFC) + +If the remote target closes while we're reading registers/memory for +restoring the selected frame in scoped_restore_current_thread's dtor, +the corresponding TARGET_CLOSE_ERROR error is swallowed by the +scoped_restore_current_thread's dtor, because letting exceptions +escape from a dtor is bad. It isn't great to lose that errors like +that, though. I've been thinking about how to avoid it, and I came up +with this patch. + +The idea here is to make scoped_restore_current_thread's dtor do as +little as possible, to avoid any work that might throw in the first +place. And to do that, instead of having the dtor call +restore_selected_frame, which re-finds the previously selected frame, +just record the frame_id/level of the desired selected frame, and have +get_selected_frame find the frame the next time it is called. In +effect, this implements most of Cagney's suggestion, here: + + /* On demand, create the selected frame and then return it. If the + selected frame can not be created, this function prints then throws + an error. When MESSAGE is non-NULL, use it for the error message, + otherwize use a generic error message. */ + /* FIXME: cagney/2002-11-28: At present, when there is no selected + frame, this function always returns the current (inner most) frame. + It should instead, when a thread has previously had its frame + selected (but not resumed) and the frame cache invalidated, find + and then return that thread's previously selected frame. */ + extern struct frame_info *get_selected_frame (const char *message); + +The only thing missing to fully implement that would be to make +reinit_frame_cache just clear selected_frame instead of calling +select_frame(NULL), and the call select_frame(NULL) explicitly in the +places where we really wanted reinit_frame_cache to go back to the +current frame too. That can done separately, though, I'm not +proposing to do that in this patch. + +Note that this patch renames restore_selected_frame to +lookup_selected_frame, and adds a new restore_selected_frame function +that doesn't throw, to be paired with the also-new save_selected_frame +function. + +There's a restore_selected_frame function in infrun.c that I think can +be replaced by the new one in frame.c. + +Also done in this patch is make the get_selected_frame's parameter be +optional, so that we don't have to pass down nullptr explicitly all +over the place. + +lookup_selected_frame should really move from thread.c to frame.c, but +I didn't do that here, just to avoid churn in the patch while it +collects comments. I did make it extern and declared it in frame.h +already, preparing for the move. I will do the move as a follow up +patch if people agree with this approach. + +Incidentally, this patch alone would fix the crashes fixed by the +previous patches in the series, because with this, +scoped_restore_current_thread's constructor doesn't throw either. + +gdb/ChangeLog: + + * blockframe.c (block_innermost_frame): Use get_selected_frame. + * frame.c + (scoped_restore_selected_frame::scoped_restore_selected_frame): + Use save_selected_frame. Save language as well. + (scoped_restore_selected_frame::~scoped_restore_selected_frame): + Use restore_selected_frame, and restore language as well. + (selected_frame_id, selected_frame_level): New. + (selected_frame): Update comments. + (save_selected_frame, restore_selected_frame): New. + (get_selected_frame): Use lookup_selected_frame. + (get_selected_frame_if_set): Delete. + (select_frame): Record selected_frame_level and selected_frame_id. + * frame.h (scoped_restore_selected_frame) : New + fields. + (get_selected_frame): Make 'message' parameter optional. + (get_selected_frame_if_set): Delete declaration. + (select_frame): Update comments. + (save_selected_frame, restore_selected_frame) + (lookup_selected_frame): Declare. + * gdbthread.h (scoped_restore_current_thread) : New field. + * infrun.c (struct infcall_control_state) : + New field. + (save_infcall_control_state): Use save_selected_frame. + (restore_selected_frame): Delete. + (restore_infcall_control_state): Use restore_selected_frame. + * stack.c (select_frame_command_core, frame_command_core): Use + get_selected_frame. + * thread.c (restore_selected_frame): Rename to ... + (lookup_selected_frame): ... this and make extern. Select the + current frame if the frame level is -1. + (scoped_restore_current_thread::restore): Also restore the + language. + (scoped_restore_current_thread::~scoped_restore_current_thread): + Don't try/catch. + (scoped_restore_current_thread::scoped_restore_current_thread): + Save the language as well. Use save_selected_frame. + +Change-Id: I73fd1cfc40d8513c28e5596383b7ecd8bcfe700f + +diff --git a/gdb/blockframe.c b/gdb/blockframe.c +--- a/gdb/blockframe.c ++++ b/gdb/blockframe.c +@@ -464,14 +464,10 @@ find_gnu_ifunc_target_type (CORE_ADDR resolver_funaddr) + struct frame_info * + block_innermost_frame (const struct block *block) + { +- struct frame_info *frame; +- + if (block == NULL) + return NULL; + +- frame = get_selected_frame_if_set (); +- if (frame == NULL) +- frame = get_current_frame (); ++ frame_info *frame = get_selected_frame (); + while (frame != NULL) + { + const struct block *frame_block = get_frame_block (frame, NULL); +diff --git a/gdb/frame.c b/gdb/frame.c +--- a/gdb/frame.c ++++ b/gdb/frame.c +@@ -317,17 +317,15 @@ frame_stash_invalidate (void) + /* See frame.h */ + scoped_restore_selected_frame::scoped_restore_selected_frame () + { +- m_fid = get_frame_id (get_selected_frame (NULL)); ++ m_lang = current_language->la_language; ++ save_selected_frame (&m_fid, &m_level); + } + + /* See frame.h */ + scoped_restore_selected_frame::~scoped_restore_selected_frame () + { +- frame_info *frame = frame_find_by_id (m_fid); +- if (frame == NULL) +- warning (_("Unable to restore previously selected frame.")); +- else +- select_frame (frame); ++ restore_selected_frame (m_fid, m_level); ++ set_language (m_lang); + } + + /* Flag to control debugging. */ +@@ -1685,10 +1683,63 @@ get_current_frame (void) + } + + /* The "selected" stack frame is used by default for local and arg +- access. May be zero, for no selected frame. */ +- ++ access. ++ ++ The "single source of truth" for the selected frame is the ++ SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL pair. ++ ++ Frame IDs can be saved/restored across reinitializing the frame ++ cache, while frame_info pointers can't (frame_info objects are ++ invalidated). If we know the corresponding frame_info object, it ++ is cached in SELECTED_FRAME. ++ ++ If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, ++ and the target has stack and is stopped, the selected frame is the ++ current (innermost) frame. This means that SELECTED_FRAME_LEVEL is ++ never 0 and SELECTED_FRAME_ID is never the ID of the innermost ++ frame. ++ ++ If SELECTED_FRAME_ID / SELECTED_FRAME_LEVEL are null_frame_id / -1, ++ and the target has no stack or is executing, then there's no ++ selected frame. */ ++static frame_id selected_frame_id = null_frame_id; ++static int selected_frame_level = -1; ++ ++/* The cached frame_info object pointing to the selected frame. ++ Looked up on demand by get_selected_frame. */ + static struct frame_info *selected_frame; + ++/* See frame.h. */ ++ ++void ++save_selected_frame (frame_id *frame_id, int *frame_level) ++ noexcept ++{ ++ *frame_id = selected_frame_id; ++ *frame_level = selected_frame_level; ++} ++ ++/* See frame.h. */ ++ ++void ++restore_selected_frame (frame_id frame_id, int frame_level) ++ noexcept ++{ ++ /* save_selected_frame never returns level == 0, so we shouldn't see ++ it here either. */ ++ gdb_assert (frame_level != 0); ++ ++ /* FRAME_ID can be null_frame_id only IFF frame_level is -1. */ ++ gdb_assert ((frame_level == -1 && !frame_id_p (frame_id)) ++ || (frame_level != -1 && frame_id_p (frame_id))); ++ ++ selected_frame_id = frame_id; ++ selected_frame_level = frame_level; ++ ++ /* Will be looked up later by get_selected_frame. */ ++ selected_frame = nullptr; ++} ++ + bool + has_stack_frames () + { +@@ -1715,9 +1766,7 @@ has_stack_frames () + return true; + } + +-/* Return the selected frame. Always non-NULL (unless there isn't an +- inferior sufficient for creating a frame) in which case an error is +- thrown. */ ++/* See frame.h. */ + + struct frame_info * + get_selected_frame (const char *message) +@@ -1726,24 +1775,14 @@ get_selected_frame (const char *message) + { + if (message != NULL && !has_stack_frames ()) + error (("%s"), message); +- /* Hey! Don't trust this. It should really be re-finding the +- last selected frame of the currently selected thread. This, +- though, is better than nothing. */ +- select_frame (get_current_frame ()); ++ ++ lookup_selected_frame (selected_frame_id, selected_frame_level); + } + /* There is always a frame. */ + gdb_assert (selected_frame != NULL); + return selected_frame; + } + +-/* If there is a selected frame, return it. Otherwise, return NULL. */ +- +-struct frame_info * +-get_selected_frame_if_set (void) +-{ +- return selected_frame; +-} +- + /* This is a variant of get_selected_frame() which can be called when + the inferior does not have a frame; in that case it will return + NULL instead of calling error(). */ +@@ -1756,12 +1795,42 @@ deprecated_safe_get_selected_frame (void) + return get_selected_frame (NULL); + } + +-/* Select frame FI (or NULL - to invalidate the current frame). */ ++/* Select frame FI (or NULL - to invalidate the selected frame). */ + + void + select_frame (struct frame_info *fi) + { + selected_frame = fi; ++ selected_frame_level = frame_relative_level (fi); ++ if (selected_frame_level == 0) ++ { ++ /* Treat the current frame especially -- we want to always ++ save/restore it without warning, even if the frame ID changes ++ (see lookup_selected_frame). E.g.: ++ ++ // The current frame is selected, the target had just stopped. ++ { ++ scoped_restore_selected_frame restore_frame; ++ some_operation_that_changes_the_stack (); ++ } ++ // scoped_restore_selected_frame's dtor runs, but the ++ // original frame_id can't be found. No matter whether it ++ // is found or not, we still end up with the now-current ++ // frame selected. Warning in lookup_selected_frame in this ++ // case seems pointless. ++ ++ Also get_frame_id may access the target's registers/memory, ++ and thus skipping get_frame_id optimizes the common case. ++ ++ Saving the selected frame this way makes get_selected_frame ++ and restore_current_frame return/re-select whatever frame is ++ the innermost (current) then. */ ++ selected_frame_level = -1; ++ selected_frame_id = null_frame_id; ++ } ++ else ++ selected_frame_id = get_frame_id (fi); ++ + /* NOTE: cagney/2002-05-04: FI can be NULL. This occurs when the + frame is being invalidated. */ + +diff --git a/gdb/frame.h b/gdb/frame.h +--- a/gdb/frame.h ++++ b/gdb/frame.h +@@ -186,8 +186,14 @@ class scoped_restore_selected_frame + + private: + +- /* The ID of the previously selected frame. */ ++ /* The ID and level of the previously selected frame. */ + struct frame_id m_fid; ++ int m_level; ++ ++ /* Save/restore the language as well, because selecting a frame ++ changes the current language to the frame's language if "set ++ language auto". */ ++ enum language m_lang; + }; + + /* Methods for constructing and comparing Frame IDs. */ +@@ -316,24 +322,49 @@ extern bool has_stack_frames (); + modifies the target invalidating the frame cache). */ + extern void reinit_frame_cache (void); + +-/* On demand, create the selected frame and then return it. If the +- selected frame can not be created, this function prints then throws +- an error. When MESSAGE is non-NULL, use it for the error message, ++/* Return the selected frame. Always returns non-NULL. If there ++ isn't an inferior sufficient for creating a frame, an error is ++ thrown. When MESSAGE is non-NULL, use it for the error message, + otherwise use a generic error message. */ + /* FIXME: cagney/2002-11-28: At present, when there is no selected + frame, this function always returns the current (inner most) frame. + It should instead, when a thread has previously had its frame + selected (but not resumed) and the frame cache invalidated, find + and then return that thread's previously selected frame. */ +-extern struct frame_info *get_selected_frame (const char *message); +- +-/* If there is a selected frame, return it. Otherwise, return NULL. */ +-extern struct frame_info *get_selected_frame_if_set (void); ++extern struct frame_info *get_selected_frame (const char *message = nullptr); + +-/* Select a specific frame. NULL, apparently implies re-select the +- inner most frame. */ ++/* Select a specific frame. NULL implies re-select the inner most ++ frame. */ + extern void select_frame (struct frame_info *); + ++/* Save the frame ID and frame level of the selected frame in FRAME_ID ++ and FRAME_LEVEL, to be restored later with restore_selected_frame. ++ ++ This is preferred over getting the same info out of ++ get_selected_frame directly because this function does not create ++ the selected-frame's frame_info object if it hasn't been created ++ yet, and thus is more efficient and doesn't throw. */ ++extern void save_selected_frame (frame_id *frame_id, int *frame_level) ++ noexcept; ++ ++/* Restore selected frame as saved with save_selected_frame. ++ ++ Does not try to find the corresponding frame_info object. Instead ++ the next call to get_selected_frame will look it up and cache the ++ result. ++ ++ This function does not throw. It is designed to be safe to called ++ from the destructors of RAII types. */ ++extern void restore_selected_frame (frame_id frame_id, int frame_level) ++ noexcept; ++ ++/* Lookup the frame_info object for the selected frame FRAME_ID / ++ FRAME_LEVEL and cache the result. ++ ++ If FRAME_LEVEL > 0 and the originally selected frame isn't found, ++ warn and select the innermost (current) frame. */ ++extern void lookup_selected_frame (frame_id frame_id, int frame_level); ++ + /* Given a FRAME, return the next (more inner, younger) or previous + (more outer, older) frame. */ + extern struct frame_info *get_prev_frame (struct frame_info *); +diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h +--- a/gdb/gdbthread.h ++++ b/gdb/gdbthread.h +@@ -673,6 +673,10 @@ class scoped_restore_current_thread + frame_id m_selected_frame_id; + int m_selected_frame_level; + bool m_was_stopped; ++ /* Save/restore the language as well, because selecting a frame ++ changes the current language to the frame's language if "set ++ language auto". */ ++ enum language m_lang; + }; + + /* Returns a pointer into the thread_info corresponding to +diff --git a/gdb/infrun.c b/gdb/infrun.c +--- a/gdb/infrun.c ++++ b/gdb/infrun.c +@@ -9017,8 +9017,10 @@ struct infcall_control_state + enum stop_stack_kind stop_stack_dummy = STOP_NONE; + int stopped_by_random_signal = 0; + +- /* ID if the selected frame when the inferior function call was made. */ ++ /* ID and level of the selected frame when the inferior function ++ call was made. */ + struct frame_id selected_frame_id {}; ++ int selected_frame_level = -1; + }; + + /* Save all of the information associated with the inferior<==>gdb +@@ -9047,27 +9049,12 @@ save_infcall_control_state () + inf_status->stop_stack_dummy = stop_stack_dummy; + inf_status->stopped_by_random_signal = stopped_by_random_signal; + +- inf_status->selected_frame_id = get_frame_id (get_selected_frame (NULL)); ++ save_selected_frame (&inf_status->selected_frame_id, ++ &inf_status->selected_frame_level); + + return inf_status; + } + +-static void +-restore_selected_frame (const frame_id &fid) +-{ +- frame_info *frame = frame_find_by_id (fid); +- +- /* If inf_status->selected_frame_id is NULL, there was no previously +- selected frame. */ +- if (frame == NULL) +- { +- warning (_("Unable to restore previously selected frame.")); +- return; +- } +- +- select_frame (frame); +-} +- + /* Restore inferior session state to INF_STATUS. */ + + void +@@ -9095,21 +9082,8 @@ restore_infcall_control_state (struct infcall_control_state *inf_status) + + if (target_has_stack) + { +- /* The point of the try/catch is that if the stack is clobbered, +- walking the stack might encounter a garbage pointer and +- error() trying to dereference it. */ +- try +- { +- restore_selected_frame (inf_status->selected_frame_id); +- } +- catch (const gdb_exception_error &ex) +- { +- exception_fprintf (gdb_stderr, ex, +- "Unable to restore previously selected frame:\n"); +- /* Error in restoring the selected frame. Select the +- innermost frame. */ +- select_frame (get_current_frame ()); +- } ++ restore_selected_frame (inf_status->selected_frame_id, ++ inf_status->selected_frame_level); + } + + delete inf_status; +diff --git a/gdb/stack.c b/gdb/stack.c +--- a/gdb/stack.c ++++ b/gdb/stack.c +@@ -1842,9 +1842,9 @@ trailing_outermost_frame (int count) + static void + select_frame_command_core (struct frame_info *fi, bool ignored) + { +- struct frame_info *prev_frame = get_selected_frame_if_set (); ++ frame_info *prev_frame = get_selected_frame (); + select_frame (fi); +- if (get_selected_frame_if_set () != prev_frame) ++ if (get_selected_frame () != prev_frame) + gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); + } + +@@ -1863,10 +1863,9 @@ select_frame_for_mi (struct frame_info *fi) + static void + frame_command_core (struct frame_info *fi, bool ignored) + { +- struct frame_info *prev_frame = get_selected_frame_if_set (); +- ++ frame_info *prev_frame = get_selected_frame (); + select_frame (fi); +- if (get_selected_frame_if_set () != prev_frame) ++ if (get_selected_frame () != prev_frame) + gdb::observers::user_selected_context_changed.notify (USER_SELECTED_FRAME); + else + print_selected_thread_frame (current_uiout, USER_SELECTED_FRAME); +diff --git a/gdb/thread.c b/gdb/thread.c +--- a/gdb/thread.c ++++ b/gdb/thread.c +@@ -1325,20 +1325,26 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) + switch_to_thread (thr); + } + +-static void +-restore_selected_frame (struct frame_id a_frame_id, int frame_level) ++/* See frame.h. */ ++ ++void ++lookup_selected_frame (struct frame_id a_frame_id, int frame_level) + { + struct frame_info *frame = NULL; + int count; + +- /* This means there was no selected frame. */ ++ /* This either means there was no selected frame, or the selected ++ frame was the current frame. In either case, select the current ++ frame. */ + if (frame_level == -1) + { +- select_frame (NULL); ++ select_frame (get_current_frame ()); + return; + } + +- gdb_assert (frame_level >= 0); ++ /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we ++ shouldn't see it here. */ ++ gdb_assert (frame_level > 0); + + /* Restore by level first, check if the frame id is the same as + expected. If that fails, try restoring by frame id. If that +@@ -1409,64 +1415,28 @@ scoped_restore_current_thread::restore () + && target_has_stack + && target_has_memory) + restore_selected_frame (m_selected_frame_id, m_selected_frame_level); ++ ++ set_language (m_lang); + } + + scoped_restore_current_thread::~scoped_restore_current_thread () + { + if (!m_dont_restore) +- { +- try +- { +- restore (); +- } +- catch (const gdb_exception &ex) +- { +- /* We're in a dtor, there's really nothing else we can do +- but swallow the exception. */ +- } +- } ++ restore (); + } + + scoped_restore_current_thread::scoped_restore_current_thread () + { + m_inf = inferior_ref::new_reference (current_inferior ()); + ++ m_lang = current_language->la_language; ++ + if (inferior_ptid != null_ptid) + { + m_thread = thread_info_ref::new_reference (inferior_thread ()); + +- struct frame_info *frame; +- + m_was_stopped = m_thread->state == THREAD_STOPPED; +- if (m_was_stopped +- && target_has_registers +- && target_has_stack +- && target_has_memory) +- { +- /* When processing internal events, there might not be a +- selected frame. If we naively call get_selected_frame +- here, then we can end up reading debuginfo for the +- current frame, but we don't generally need the debuginfo +- at this point. */ +- frame = get_selected_frame_if_set (); +- } +- else +- frame = NULL; +- +- try +- { +- m_selected_frame_id = get_frame_id (frame); +- m_selected_frame_level = frame_relative_level (frame); +- } +- catch (const gdb_exception_error &ex) +- { +- m_selected_frame_id = null_frame_id; +- m_selected_frame_level = -1; +- +- /* Better let this propagate. */ +- if (ex.error == TARGET_CLOSE_ERROR) +- throw; +- } ++ save_selected_frame (&m_selected_frame_id, &m_selected_frame_level); + } + } + diff --git a/gdb-rhbz1909902-frame_id_p-assert-2.patch b/gdb-rhbz1909902-frame_id_p-assert-2.patch new file mode 100644 index 0000000..d804bbe --- /dev/null +++ b/gdb-rhbz1909902-frame_id_p-assert-2.patch @@ -0,0 +1,169 @@ +From FEDORA_PATCHES Mon Sep 17 00:00:00 2001 +From: Pedro Alves +Date: Sat, 31 Oct 2020 00:27:18 +0000 +Subject: gdb-rhbz1909902-frame_id_p-assert-2.patch + +;; Backport patch #2 which fixes a frame_id_p assertion failure (RH BZ 1909902). + +Fix frame cycle detection + +The recent commit to make scoped_restore_current_thread's cdtors +exception free regressed gdb.base/eh_return.exp: + + Breakpoint 1, 0x00000000004012bb in eh2 (gdb/frame.c:641: internal-error: frame_id get_frame_id(frame_info*): Assertion `stashed' failed. + A problem internal to GDB has been detected, + further debugging may prove unreliable. + Quit this debugging session? (y or n) FAIL: gdb.base/eh_return.exp: hit breakpoint (GDB internal error) + +That testcase uses __builtin_eh_return and, before the regression, the +backtrace at eh2 looked like this: + + (gdb) bt + #0 0x00000000004006eb in eh2 (p=0x4006ec ) at src/gdb/testsuite/gdb.base/eh_return.c:54 + Backtrace stopped: previous frame identical to this frame (corrupt stack?) + +That "previous frame identical to this frame" is caught by the cycle +detection based on frame id. + +The assertion failing is this one: + + 638 /* Since this is the first frame in the chain, this should + 639 always succeed. */ + 640 bool stashed = frame_stash_add (fi); + 641 gdb_assert (stashed); + +originally added by + + commit f245535cf583ae4ca13b10d47b3c7d3334593ece + Author: Pedro Alves + AuthorDate: Mon Sep 5 18:41:38 2016 +0100 + + Fix PR19927: Avoid unwinder recursion if sniffer uses calls parse_and_eval + +The assertion is failing because frame #1's frame id was stashed +before the id of frame #0 is stashed. The frame id of frame #1 was +stashed here: + + (top-gdb) bt + #0 frame_stash_add (frame=0x1e24c90) at src/gdb/frame.c:276 + #1 0x0000000000669c1b in get_prev_frame_if_no_cycle (this_frame=0x19f8370) at src/gdb/frame.c:2120 + #2 0x000000000066a339 in get_prev_frame_always_1 (this_frame=0x19f8370) at src/gdb/frame.c:2303 + #3 0x000000000066a360 in get_prev_frame_always (this_frame=0x19f8370) at src/gdb/frame.c:2319 + #4 0x000000000066b56c in get_frame_unwind_stop_reason (frame=0x19f8370) at src/gdb/frame.c:3028 + #5 0x000000000059f929 in dwarf2_frame_cfa (this_frame=0x19f8370) at src/gdb/dwarf2/frame.c:1462 + #6 0x00000000005ce434 in dwarf_evaluate_loc_desc::get_frame_cfa (this=0x7fffffffc070) at src/gdb/dwarf2/loc.c:666 + #7 0x00000000005989a9 in dwarf_expr_context::execute_stack_op (this=0x7fffffffc070, op_ptr=0x1b2a053 "\364\003", op_end=0x1b2a053 "\364\003") at src/gdb/dwarf2/expr.c:1161 + #8 0x0000000000596af6 in dwarf_expr_context::eval (this=0x7fffffffc070, addr=0x1b2a052 "\234\364\003", len=1) at src/gdb/dwarf2/expr.c:303 + #9 0x0000000000597b4e in dwarf_expr_context::execute_stack_op (this=0x7fffffffc070, op_ptr=0x1b2a063 "", op_end=0x1b2a063 "") at src/gdb/dwarf2/expr.c:865 + #10 0x0000000000596af6 in dwarf_expr_context::eval (this=0x7fffffffc070, addr=0x1b2a061 "\221X", len=2) at src/gdb/dwarf2/expr.c:303 + #11 0x00000000005c8b5a in dwarf2_evaluate_loc_desc_full (type=0x1b564d0, frame=0x19f8370, data=0x1b2a061 "\221X", size=2, per_cu=0x1b28760, per_objfile=0x1a84930, subobj_type=0x1b564d0, subobj_byte_offset=0) at src/gdb/dwarf2/loc.c:2260 + #12 0x00000000005c9243 in dwarf2_evaluate_loc_desc (type=0x1b564d0, frame=0x19f8370, data=0x1b2a061 "\221X", size=2, per_cu=0x1b28760, per_objfile=0x1a84930) at src/gdb/dwarf2/loc.c:2444 + #13 0x00000000005cb769 in locexpr_read_variable (symbol=0x1b59840, frame=0x19f8370) at src/gdb/dwarf2/loc.c:3687 + #14 0x0000000000663137 in language_defn::read_var_value (this=0x122ea60 , var=0x1b59840, var_block=0x0, frame=0x19f8370) at src/gdb/findvar.c:618 + #15 0x0000000000663c3b in read_var_value (var=0x1b59840, var_block=0x0, frame=0x19f8370) at src/gdb/findvar.c:822 + #16 0x00000000008c7d9f in read_frame_arg (fp_opts=..., sym=0x1b59840, frame=0x19f8370, argp=0x7fffffffc470, entryargp=0x7fffffffc490) at src/gdb/stack.c:542 + #17 0x00000000008c89cd in print_frame_args (fp_opts=..., func=0x1b597c0, frame=0x19f8370, num=-1, stream=0x1aba860) at src/gdb/stack.c:890 + #18 0x00000000008c9bf8 in print_frame (fp_opts=..., frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, print_args=1, sal=...) at src/gdb/stack.c:1394 + #19 0x00000000008c92b9 in print_frame_info (fp_opts=..., frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, print_args=1, set_current_sal=1) at src/gdb/stack.c:1119 + #20 0x00000000008c75f0 in print_stack_frame (frame=0x19f8370, print_level=0, print_what=SRC_AND_LOC, set_current_sal=1) at src/gdb/stack.c:366 + #21 0x000000000070250b in print_stop_location (ws=0x7fffffffc9e0) at src/gdb/infrun.c:8110 + #22 0x0000000000702569 in print_stop_event (uiout=0x1a8b9e0, displays=true) at src/gdb/infrun.c:8126 + #23 0x000000000096d04b in tui_on_normal_stop (bs=0x1bcd1c0, print_frame=1) at src/gdb/tui/tui-interp.c:98 + ... + +Before the commit to make scoped_restore_current_thread's cdtors +exception free, scoped_restore_current_thread's dtor would call +get_frame_id on the selected frame, and we use +scoped_restore_current_thread pervasively. That had the side effect +of stashing the frame id of frame #0 before reaching the path shown in +the backtrace. I.e., the frame id of frame #0 happened to be stashed +before the frame id of frame #1. But that was by chance, not by +design. + +This commit: + + commit 256ae5dbc73d1348850f86ee77a0dc3b04bc7cc0 + Author: Kevin Buettner + AuthorDate: Mon Oct 31 12:47:42 2016 -0700 + + Stash frame id of current frame before stashing frame id for previous frame + +Fixed a similar problem, by making sure get_prev_frame computes the +frame id of the current frame before unwinding the previous frame, so +that the cycle detection works properly. That fix misses the scenario +we're now running against, because if you notice, the backtrace above +shows that frame #4 calls get_prev_frame_always, not get_prev_frame. +I.e., nothing is calling get_frame_id on the current frame. + +The fix here is to move Kevin's fix down from get_prev_frame to +get_prev_frame_always. Or actually, a bit further down to +get_prev_frame_always_1 -- note that inline_frame_this_id calls +get_prev_frame_always, so we need to be careful to avoid recursion in +that scenario. + +gdb/ChangeLog: + + * frame.c (get_prev_frame): Move get_frame_id call from here ... + (get_prev_frame_always_1): ... to here. + * inline-frame.c (inline_frame_this_id): Mention + get_prev_frame_always_1 in comment. + +Change-Id: Id960c98ab2d072c48a436c3eb160cc4b2a5cfd1d + +diff --git a/gdb/frame.c b/gdb/frame.c +--- a/gdb/frame.c ++++ b/gdb/frame.c +@@ -2133,6 +2133,23 @@ get_prev_frame_always_1 (struct frame_info *this_frame) + if (get_frame_type (this_frame) == INLINE_FRAME) + return get_prev_frame_if_no_cycle (this_frame); + ++ /* If this_frame is the current frame, then compute and stash its ++ frame id prior to fetching and computing the frame id of the ++ previous frame. Otherwise, the cycle detection code in ++ get_prev_frame_if_no_cycle() will not work correctly. When ++ get_frame_id() is called later on, an assertion error will be ++ triggered in the event of a cycle between the current frame and ++ its previous frame. ++ ++ Note we do this after the INLINE_FRAME check above. That is ++ because the inline frame's frame id computation needs to fetch ++ the frame id of its previous real stack frame. I.e., we need to ++ avoid recursion in that case. This is OK since we're sure the ++ inline frame won't create a cycle with the real stack frame. See ++ inline_frame_this_id. */ ++ if (this_frame->level == 0) ++ get_frame_id (this_frame); ++ + /* Check that this frame is unwindable. If it isn't, don't try to + unwind to the prev frame. */ + this_frame->stop_reason +@@ -2410,16 +2427,6 @@ get_prev_frame (struct frame_info *this_frame) + something should be calling get_selected_frame() or + get_current_frame(). */ + gdb_assert (this_frame != NULL); +- +- /* If this_frame is the current frame, then compute and stash +- its frame id prior to fetching and computing the frame id of the +- previous frame. Otherwise, the cycle detection code in +- get_prev_frame_if_no_cycle() will not work correctly. When +- get_frame_id() is called later on, an assertion error will +- be triggered in the event of a cycle between the current +- frame and its previous frame. */ +- if (this_frame->level == 0) +- get_frame_id (this_frame); + + frame_pc_p = get_frame_pc_if_available (this_frame, &frame_pc); + +diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c +--- a/gdb/inline-frame.c ++++ b/gdb/inline-frame.c +@@ -161,7 +161,8 @@ inline_frame_this_id (struct frame_info *this_frame, + real frame's this_id method. So we must call + get_prev_frame_always. Because we are inlined into some + function, there must be previous frames, so this is safe - as +- long as we're careful not to create any cycles. */ ++ long as we're careful not to create any cycles. See related ++ comments in get_prev_frame_always_1. */ + *this_id = get_frame_id (get_prev_frame_always (this_frame)); + + /* We need a valid frame ID, so we need to be based on a valid diff --git a/gdb.spec b/gdb.spec index 5ef511d..19cd878 100644 --- a/gdb.spec +++ b/gdb.spec @@ -37,7 +37,7 @@ Version: 10.1 # The release always contains a leading reserved number, start it at 1. # `upstream' is not a part of `name' to stay fully rpm dependencies compatible for the testing. -Release: 13%{?dist} +Release: 14%{?dist} License: GPLv3+ and GPLv3+ with exceptions and GPLv2+ and GPLv2+ with exceptions and GPL+ and LGPLv2+ and LGPLv3+ and BSD and Public Domain and GFDL # Do not provide URL for snapshots as the file lasts there only for 2 days. @@ -1195,6 +1195,10 @@ fi %endif %changelog +* Fri Mar 05 2021 Kevin Buettner - 10.1-14 +- Backport patches which fix frame_id_p assertion failure (RHBZ 1909902, + Pedro Alves). + * Fri Mar 5 2021 Jan Kratochvil - 10.1-13 - Drop gdb-vla-intel-fortran-vla-strings.patch as it was still regressing the testsuite.