public inbox for gdb-cvs@sourceware.org
help / color / mirror / Atom feed
* [binutils-gdb] gdb/infrun: lazily load curr_frame_id in process_event_stop_test
@ 2024-01-21 16:25 Lancelot SIX
0 siblings, 0 replies; only message in thread
From: Lancelot SIX @ 2024-01-21 16:25 UTC (permalink / raw)
To: gdb-cvs
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=5266f5c25b20ed6411b263952f52032afafd280d
commit 5266f5c25b20ed6411b263952f52032afafd280d
Author: Lancelot SIX <lancelot.six@amd.com>
Date: Thu Dec 28 11:51:31 2023 +0000
gdb/infrun: lazily load curr_frame_id in process_event_stop_test
A recent(ish) change in gdb/infrun.c made process_event_stop_test load
debug information where it would not have done so previously. The
change is:
commit bf2813aff8f2988ad3d53e819a0415abf295c91f
AuthorDate: Fri Sep 1 13:47:32 2023 +0200
CommitDate: Mon Nov 20 10:54:03 2023 +0100
gdb/record: print frame information when exiting a recursive call
Currently, when GDB is reverse stepping out of a function into the same
function due to a recursive call, it doesn't print frame information, as
reported by PR record/29178. This happens because when the inferior
leaves the current frame, GDB decides to refresh the step information,
clobbering the original step_frame_id, making it impossible to figure
out later on that the frame has been changed.
This commit changes GDB so that, if we notice we're in this exact
situation, we won't refresh the step information.
Because of implementation details, this change can cause some debug
information to be read when it normally wouldn't before, which showed up
as a regression on gdb.dwarf2/dw2-out-of-range-end-of-seq. Since that
isn't a problem, the test was changed to allow for the new output.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29178
Although there is nothing wrong with this change in principle, it
happens to break most of the tests in gdb/testsuite/gdb.rocm/*.exp.
This is because those tests do rely on GDB not loading debug
information. This is necessary because the debug information produced
for AMDGPU code is using DWARF extensions which are not supported by GDB
at this point.
In this patch, I propose to use a lazy loading mechanism so the frame_id
for the current frame is only computed when required instead of when
entering process_event_stop_test. The lazy_loader class is currently
defined locally in infrun.c, but if it turns out to be useful elsewhere,
it could go somewhere under gdbsupport.
This patch should restore the behavior GDB had before
bf2813aff8f2988ad3d53e819a0415abf295c91f when it comes to load debug
info.
Another approach could have been to revert
fb84fbf8a51f5be2e78765508ebd9753af96b492 (gdb/infrun: simplify
process_event_stop_test) and adjust the implementation of
bf2813aff8f2988ad3d53e819a0415abf295c91f (gdb/record: print frame
information when exiting a recursive call). However, I think that the
lazy loading works well with the simplification done recently, so I went
down that route.
Regression tested on x86_64-linux (Ubuntu 22.04) with AMDGPU support.
Change-Id: Ib63a162128130d1786a77c98623e9e3dcbc363b7
Approved-by: Kevin Buettner <kevinb@redhat.com>
Diff:
---
gdb/infrun.c | 50 +++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 41 insertions(+), 9 deletions(-)
diff --git a/gdb/infrun.c b/gdb/infrun.c
index d8c1bcae522..e99a1a83070 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -7318,6 +7318,37 @@ update_line_range_start (CORE_ADDR pc, struct execution_control_state *ecs)
return start_line_pc;
}
+namespace {
+
+/* Helper class for process_event_stop_test implementing lazy evaluation. */
+template<typename T>
+class lazy_loader
+{
+ using fetcher_t = std::function<T ()>;
+
+public:
+ explicit lazy_loader (fetcher_t &&f) : m_loader (std::move (f))
+ { }
+
+ T &operator* ()
+ {
+ if (!m_value.has_value ())
+ m_value.emplace (m_loader ());
+ return m_value.value ();
+ }
+
+ T *operator-> ()
+ {
+ return &**this;
+ }
+
+private:
+ std::optional<T> m_value;
+ fetcher_t m_loader;
+};
+
+}
+
/* Come here when we've got some debug event / signal we can explain
(IOW, not a random signal), and test whether it should cause a
stop, or whether we should resume the inferior (transparently).
@@ -7358,7 +7389,8 @@ process_event_stop_test (struct execution_control_state *ecs)
/* Shorthand to make if statements smaller. */
struct frame_id original_frame_id
= ecs->event_thread->control.step_frame_id;
- struct frame_id curr_frame_id = get_frame_id (get_current_frame ());
+ lazy_loader<frame_id> curr_frame_id
+ ([] () { return get_frame_id (get_current_frame ()); });
switch (what.main_action)
{
@@ -7446,7 +7478,7 @@ process_event_stop_test (struct execution_control_state *ecs)
if (init_frame)
{
- if (curr_frame_id == ecs->event_thread->initiating_frame)
+ if (*curr_frame_id == ecs->event_thread->initiating_frame)
{
/* Case 2. Fall through. */
}
@@ -7619,7 +7651,7 @@ process_event_stop_test (struct execution_control_state *ecs)
if (pc_in_thread_step_range (ecs->event_thread->stop_pc (),
ecs->event_thread)
&& (execution_direction != EXEC_REVERSE
- || curr_frame_id == original_frame_id))
+ || *curr_frame_id == original_frame_id))
{
infrun_debug_printf
("stepping inside range [%s-%s]",
@@ -8062,7 +8094,7 @@ process_event_stop_test (struct execution_control_state *ecs)
frame machinery detected some skipped call sites, we have entered
a new inline function. */
- if ((curr_frame_id == original_frame_id)
+ if ((*curr_frame_id == original_frame_id)
&& inline_skipped_frames (ecs->event_thread))
{
infrun_debug_printf ("stepped into inlined function");
@@ -8110,7 +8142,7 @@ process_event_stop_test (struct execution_control_state *ecs)
through a more inlined call beyond its call site. */
if (get_frame_type (get_current_frame ()) == INLINE_FRAME
- && (curr_frame_id != original_frame_id)
+ && (*curr_frame_id != original_frame_id)
&& stepped_in_from (get_current_frame (), original_frame_id))
{
infrun_debug_printf ("stepping through inlined function");
@@ -8164,7 +8196,7 @@ process_event_stop_test (struct execution_control_state *ecs)
end_stepping_range (ecs);
return;
}
- else if (curr_frame_id == original_frame_id)
+ else if (*curr_frame_id == original_frame_id)
{
/* We are not at the start of a statement, and we have not changed
frame.
@@ -8190,9 +8222,9 @@ process_event_stop_test (struct execution_control_state *ecs)
}
}
else if (execution_direction == EXEC_REVERSE
- && curr_frame_id != original_frame_id
- && original_frame_id.code_addr_p && curr_frame_id.code_addr_p
- && original_frame_id.code_addr == curr_frame_id.code_addr)
+ && *curr_frame_id != original_frame_id
+ && original_frame_id.code_addr_p && curr_frame_id->code_addr_p
+ && original_frame_id.code_addr == curr_frame_id->code_addr)
{
/* If we enter here, we're leaving a recursive function call. In this
situation, we shouldn't refresh the step information, because if we
^ permalink raw reply [flat|nested] only message in thread
only message in thread, other threads:[~2024-01-21 16:25 UTC | newest]
Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-21 16:25 [binutils-gdb] gdb/infrun: lazily load curr_frame_id in process_event_stop_test Lancelot SIX
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).