| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
This commit is the result of running the gdb/copyright.py script,
which automated the update of the copyright year range for all
source files managed by the GDB project to be updated to include
year 2023.
|
|
On x86_64-windows, since 04e2ac7b2a7, we observe this internal error:
[...]/gdbsupport/intrusive_list.h:458: internal-error: erase_element:
Assertion `elem_node->prev != INTRUSIVE_LIST_UNLINKED_VALUE' failed.
Breaking in the destructors for intrusive_list and frame_info_ptr shows that
in this configuration, the destructors for frame.c's statically-stored
objects are run before frame-info.c's:
Thread 1 hit Breakpoint 7, intrusive_list<frame_info_ptr,
intrusive_base_node<frame_info_ptr> >::~intrusive_list (this=0x7ff69c418c90
<frame_info_ptr::frame_list>, __in_chrg=<optimized out>)
[...]/../gdbsupport/intrusive_list.h:250
250 clear ();
(gdb) bt
#0 intrusive_list<frame_info_ptr, intrusive_base_node<frame_info_ptr> >
::~intrusive_list (this=0x7ff69c418c90 <frame_info_ptr::frame_list>,
__in_chrg=<optimized out>) [...]/../gdbsupport/intrusive_list.h:250
#1 0x00007ff69b78edba in __tcf_1 () [...]/frame-info.c:27
#2 0x00007ff9c457aa9f in msvcrt!_initterm_e ()
from C:\Windows\System32\msvcrt.dll
#3 0x00007ff69b8246a6 in captured_main_1 (context=0x5ffe00)
[...]/main.c:1111
#4 0x00007ff69b825149 in captured_main (data=0x5ffe00) [...]/main.c:1320
#5 0x00007ff69b8251b1 in gdb_main (args=0x5ffe00) [...]/main.c:1345
#6 0x00007ff69b5d1730 in main (argc=2, argv=0x751730) [...]/gdb.c:32
(gdb) c
Continuing.
Thread 1 hit Breakpoint 8, frame_info_ptr::~frame_info_ptr
(this=0x7ff69c418e20 <selected_frame>, __in_chrg=<optimized out>)
[...]/frame-info.h:74
74 if (is_linked ())
(gdb) bt
#0 frame_info_ptr::~frame_info_ptr (this=0x7ff69c418e20 <selected_frame>,
__in_chrg=<optimized out>) [...]/frame-info.h:74
#1 0x00007ff69b79a643 in __tcf_1 () [...]/frame.c:1675
#2 0x00007ff9c457aa9f in msvcrt!_initterm_e () from
C:\Windows\System32\msvcrt.dll
#3 0x00007ff69b8246a6 in captured_main_1 (context=0x5ffe00)
[...]/main.c:1111
#4 0x00007ff69b825149 in captured_main (data=0x5ffe00) [...]/main.c:1320
#5 0x00007ff69b8251b1 in gdb_main (args=0x5ffe00) [...]/main.c:1345
#6 0x00007ff69b5d1730 in main (argc=2, argv=0x751730) [...]/gdb.c:32
Approved-By: Simon Marchi <simon.marchi@efficios.com>
|
|
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>
|
|
I don't see any particular reason why the implementations of the
frame_info_ptr object are in the header file. It only seems to add some
complexity. Since we can't include frame.h in frame-info.h, we have to
add declarations of functions defined in frame.c, in frame-info.h. By
moving the implementations to a new frame-info.c, we can avoid that.
Change-Id: I435c828f81b8a3392c43ef018af31effddf6be9c
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
Reviewed-By: Tom Tromey <tom@tromey.com>
|
|
We do it in the move assignment operator, so I think it makes sense to
do it here too for consistency. I don't think it's absolutely necessary
to clear the other object's fields (in other words, copy constructor and
move constructor could be the same), as there is no exclusive resource
being transfered. The important thing is to leave the moved-from object
in an unknown, but valid state. But still, I think that clearing the
fields of the moved-from object is not a bad idea, it helps ensure we
don't rely on the moved-from object after.
Change-Id: Iee900ff9d25dad51d62765d694f2e01524351340
Reviewed-By: Bruno Larsen <blarsen@redhat.com>
|
|
Currently, despite having a smart pointer for frame_infos, GDB may
attempt to use an invalidated frame_info_ptr, which would cause internal
errors to happen. One such example has been documented as PR
python/28856, that happened when printing frame arguments calls an
inferior function.
To avoid failures, the smart wrapper was changed to also cache the frame
id, so the pointer can be reinflated later. For this to work, the
frame-id stuff had to be moved to their own .h file, which is included
by frame-info.h.
Frame_id caching is done explicitly using the prepare_reinflate method.
Caching is done manually so that only the pointers that need to be saved
will be, and reinflating has to be done manually using the reinflate
method because the get method and the -> operator must not change
the internals of the class. Finally, attempting to reinflate when the
pointer is being invalidated causes the following assertion errors:
check_ptrace_stopped_lwp_gone: assertion `lp->stopped` failed.
get_frame_pc: Assertion `frame->next != NULL` failed.
As for performance concerns, my personal testing with `time make
chec-perf GDB_PERFTEST_MODE=run` showed an actual reduction of around
10% of time running.
This commit also adds a testcase that exercises the python/28856 bug with
7 different triggers, run, continue, step, backtrace, finish, up and down.
Some of them can seem to be testing the same thing twice, but since this
test relies on stale pointers, there is always a chance that GDB got lucky
when testing, so better to test extra.
Regression tested on x86_64, using both gcc and clang.
Approved-by: Tom Tomey <tom@tromey.com>
|
|
This adds frame_info_ptr, a smart pointer class. Every instance of
the class is kept on an intrusive 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>
Approved-by: Tom Tomey <tom@tromey.com>
|
