* [PATCH v2 1/4] Remove frame_id_eq
2022-07-08 16:07 [PATCH v2 0/4] Smart pointer wrapper for frame_info Bruno Larsen
@ 2022-07-08 16:07 ` Bruno Larsen
2022-07-08 16:07 ` [PATCH v2 2/4] Introduce frame_info_ptr smart pointer class Bruno Larsen
2022-07-08 16:07 ` [PATCH v2 4/4] gdb/frame: Add reinflation method for frame_info_ptr Bruno Larsen
2 siblings, 0 replies; 6+ messages in thread
From: Bruno Larsen @ 2022-07-08 16:07 UTC (permalink / raw)
To: gdb-patches; +Cc: tom
From: Tom Tromey <tom@tromey.com>
This replaces frame_id_eq with operator== and operator!=. I wrote
this for a version of this series that I later abandoned; but since it
simplifies the code, I left this patch in.
---
gdb/breakpoint.c | 2 +-
gdb/dummy-frame.c | 4 +--
gdb/elfread.c | 2 +-
gdb/frame-unwind.c | 2 +-
gdb/frame.c | 27 +++++++++----------
gdb/frame.h | 30 ++++++++-------------
gdb/guile/scm-frame.c | 2 +-
gdb/ia64-tdep.c | 4 +--
gdb/infrun.c | 46 +++++++++++++++-----------------
gdb/mi/mi-cmd-stack.c | 4 +--
gdb/mi/mi-main.c | 2 +-
gdb/python/py-finishbreakpoint.c | 2 +-
gdb/python/py-frame.c | 2 +-
gdb/record-btrace.c | 6 ++---
gdb/stack.c | 4 +--
gdb/tracepoint.c | 3 +--
gdb/value.c | 2 +-
17 files changed, 65 insertions(+), 79 deletions(-)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a3be12557f6..02141e8abbb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5322,7 +5322,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
breakpoint or a single step breakpoint. */
if (frame_id_p (b->frame_id)
- && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
+ && b->frame_id != get_stack_frame_id (get_current_frame ()))
{
bs->stop = 0;
return;
diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index 40b455c7e4a..2fef6eae562 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -47,7 +47,7 @@ static int
dummy_frame_id_eq (struct dummy_frame_id *id1,
struct dummy_frame_id *id2)
{
- return frame_id_eq (id1->id, id2->id) && id1->thread == id2->thread;
+ return id1->id == id2->id && id1->thread == id2->thread;
}
/* List of dummy_frame destructors. */
@@ -130,7 +130,7 @@ static bool
pop_dummy_frame_bpt (struct breakpoint *b, struct dummy_frame *dummy)
{
if (b->thread == dummy->id.thread->global_num
- && b->disposition == disp_del && frame_id_eq (b->frame_id, dummy->id.id))
+ && b->disposition == disp_del && b->frame_id == dummy->id.id)
{
while (b->related_breakpoint != b)
delete_breakpoint (b->related_breakpoint);
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 8ff62a1fed5..dd5d6b5bb65 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -946,7 +946,7 @@ elf_gnu_ifunc_resolver_stop (code_breakpoint *b)
if (b_return->thread == thread_id
&& b_return->loc->requested_address == prev_pc
- && frame_id_eq (b_return->frame_id, prev_frame_id))
+ && b_return->frame_id == prev_frame_id)
break;
}
diff --git a/gdb/frame-unwind.c b/gdb/frame-unwind.c
index 302ce1efb99..91c40361d13 100644
--- a/gdb/frame-unwind.c
+++ b/gdb/frame-unwind.c
@@ -225,7 +225,7 @@ default_frame_unwind_stop_reason (struct frame_info *this_frame,
{
struct frame_id this_id = get_frame_id (this_frame);
- if (frame_id_eq (this_id, outer_frame_id))
+ if (this_id == outer_frame_id)
return UNWIND_OUTERMOST;
else
return UNWIND_NO_REASON;
diff --git a/gdb/frame.c b/gdb/frame.c
index ae45e22d613..c0cf3d585bf 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -244,7 +244,7 @@ frame_addr_hash (const void *ap)
}
/* Internal equality function for the hash table. This function
- defers equality operations to frame_id_eq. */
+ defers equality operations to frame_id::operator==. */
static int
frame_addr_hash_eq (const void *a, const void *b)
@@ -252,8 +252,7 @@ frame_addr_hash_eq (const void *a, const void *b)
const struct frame_info *f_entry = (const struct frame_info *) a;
const struct frame_info *f_element = (const struct frame_info *) b;
- return frame_id_eq (f_entry->this_id.value,
- f_element->this_id.value);
+ return f_entry->this_id.value == f_element->this_id.value;
}
/* Internal function to create the frame_stash hash table. 100 seems
@@ -752,28 +751,28 @@ frame_id_artificial_p (frame_id l)
}
bool
-frame_id_eq (frame_id l, frame_id r)
+frame_id::operator== (const frame_id &r) const
{
bool eq;
- if (l.stack_status == FID_STACK_INVALID
+ if (stack_status == FID_STACK_INVALID
|| r.stack_status == FID_STACK_INVALID)
/* Like a NaN, if either ID is invalid, the result is false.
Note that a frame ID is invalid iff it is the null frame ID. */
eq = false;
- else if (l.stack_status != r.stack_status || l.stack_addr != r.stack_addr)
+ else if (stack_status != r.stack_status || stack_addr != r.stack_addr)
/* If .stack addresses are different, the frames are different. */
eq = false;
- else if (l.code_addr_p && r.code_addr_p && l.code_addr != r.code_addr)
+ else if (code_addr_p && r.code_addr_p && code_addr != r.code_addr)
/* An invalid code addr is a wild card. If .code addresses are
different, the frames are different. */
eq = false;
- else if (l.special_addr_p && r.special_addr_p
- && l.special_addr != r.special_addr)
+ else if (special_addr_p && r.special_addr_p
+ && special_addr != r.special_addr)
/* An invalid special addr is a wild card (or unused). Otherwise
if special addresses are different, the frames are different. */
eq = false;
- else if (l.artificial_depth != r.artificial_depth)
+ else if (artificial_depth != r.artificial_depth)
/* If artificial depths are different, the frames must be different. */
eq = false;
else
@@ -781,7 +780,7 @@ frame_id_eq (frame_id l, frame_id r)
eq = true;
frame_debug_printf ("l=%s, r=%s -> %d",
- l.to_string ().c_str (), r.to_string ().c_str (), eq);
+ to_string ().c_str (), r.to_string ().c_str (), eq);
return eq;
}
@@ -875,7 +874,7 @@ frame_find_by_id (struct frame_id id)
return NULL;
/* Check for the sentinel frame. */
- if (frame_id_eq (id, sentinel_frame_id))
+ if (id == sentinel_frame_id)
return sentinel_frame;
/* Try using the frame stash first. Finding it there removes the need
@@ -894,7 +893,7 @@ frame_find_by_id (struct frame_id id)
{
struct frame_id self = get_frame_id (frame);
- if (frame_id_eq (id, self))
+ if (id == self)
/* An exact match. */
return frame;
@@ -1738,7 +1737,7 @@ lookup_selected_frame (struct frame_id a_frame_id, int frame_level)
it's highly unlikely the search by level finds the wrong
frame, it's 99.9(9)% of the time (for all practical purposes)
safe. */
- && frame_id_eq (get_frame_id (frame), a_frame_id))
+ && get_frame_id (frame) == a_frame_id)
{
/* Cool, all is fine. */
select_frame (frame);
diff --git a/gdb/frame.h b/gdb/frame.h
index b9a3793cc23..75bb3bd2aa0 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -173,6 +173,16 @@ struct frame_id
/* Return a string representation of this frame id. */
std::string to_string () const;
+
+ /* Returns true when this frame_id and R identify the same
+ frame. */
+ bool operator== (const frame_id &r) const;
+
+ /* Inverse of ==. */
+ bool operator!= (const frame_id &r) const
+ {
+ return !(*this == r);
+ }
};
/* Save and restore the currently selected frame. */
@@ -269,9 +279,6 @@ extern bool frame_id_p (frame_id l);
TAILCALL_FRAME. */
extern bool frame_id_artificial_p (frame_id l);
-/* Returns true when L and R identify the same frame. */
-extern bool frame_id_eq (frame_id l, frame_id r);
-
/* Frame types. Some are real, some are signal trampolines, and some
are completely artificial (dummy). */
@@ -498,22 +505,7 @@ extern CORE_ADDR get_frame_base (struct frame_info *);
/* Return the per-frame unique identifer. Can be used to relocate a
frame after a frame cache flush (and other similar operations). If
- FI is NULL, return the null_frame_id.
-
- NOTE: kettenis/20040508: These functions return a structure. On
- platforms where structures are returned in static storage (vax,
- m68k), this may trigger compiler bugs in code like:
-
- if (frame_id_eq (get_frame_id (l), get_frame_id (r)))
-
- where the return value from the first get_frame_id (l) gets
- overwritten by the second get_frame_id (r). Please avoid writing
- code like this. Use code like:
-
- struct frame_id id = get_frame_id (l);
- if (frame_id_eq (id, get_frame_id (r)))
-
- instead, since that avoids the bug. */
+ FI is NULL, return the null_frame_id. */
extern struct frame_id get_frame_id (struct frame_info *fi);
extern struct frame_id get_stack_frame_id (struct frame_info *fi);
extern struct frame_id frame_unwind_caller_id (struct frame_info *next_frame);
diff --git a/gdb/guile/scm-frame.c b/gdb/guile/scm-frame.c
index 6bbb6f81d68..d49233661e6 100644
--- a/gdb/guile/scm-frame.c
+++ b/gdb/guile/scm-frame.c
@@ -106,7 +106,7 @@ frscm_eq_frame_smob (const void *ap, const void *bp)
const frame_smob *a = (const frame_smob *) ap;
const frame_smob *b = (const frame_smob *) bp;
- return (frame_id_eq (a->frame_id, b->frame_id)
+ return (a->frame_id == b->frame_id
&& a->inferior == b->inferior
&& a->inferior != NULL);
}
diff --git a/gdb/ia64-tdep.c b/gdb/ia64-tdep.c
index b94225b5e13..ab4e52d16d7 100644
--- a/gdb/ia64-tdep.c
+++ b/gdb/ia64-tdep.c
@@ -2901,7 +2901,7 @@ ia64_libunwind_frame_this_id (struct frame_info *this_frame, void **this_cache,
CORE_ADDR bsp;
libunwind_frame_this_id (this_frame, this_cache, &id);
- if (frame_id_eq (id, outer_frame_id))
+ if (id == outer_frame_id)
{
(*this_id) = outer_frame_id;
return;
@@ -3032,7 +3032,7 @@ ia64_libunwind_sigtramp_frame_this_id (struct frame_info *this_frame,
struct frame_id id = outer_frame_id;
libunwind_frame_this_id (this_frame, this_cache, &id);
- if (frame_id_eq (id, outer_frame_id))
+ if (id == outer_frame_id)
{
(*this_id) = outer_frame_id;
return;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 02c98b50c8c..88def64caa6 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -4551,7 +4551,7 @@ stepped_in_from (struct frame_info *frame, struct frame_id step_frame_id)
frame != NULL;
frame = get_prev_frame (frame))
{
- if (frame_id_eq (get_frame_id (frame), step_frame_id))
+ if (get_frame_id (frame) == step_frame_id)
return true;
if (get_frame_type (frame) != INLINE_FRAME)
@@ -4581,7 +4581,7 @@ inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
symtab_and_line sal;
struct symbol *sym;
- if (frame_id_eq (get_frame_id (frame), tp->control.step_frame_id))
+ if (get_frame_id (frame) == tp->control.step_frame_id)
break;
if (get_frame_type (frame) != INLINE_FRAME)
break;
@@ -6562,8 +6562,8 @@ handle_signal_stop (struct execution_control_state *ecs)
&& (pc_in_thread_step_range (ecs->event_thread->stop_pc (),
ecs->event_thread)
|| ecs->event_thread->control.step_range_end == 1)
- && frame_id_eq (get_stack_frame_id (frame),
- ecs->event_thread->control.step_stack_frame_id)
+ && (get_stack_frame_id (frame)
+ == ecs->event_thread->control.step_stack_frame_id)
&& ecs->event_thread->control.step_resume_breakpoint == NULL)
{
/* The inferior is about to take a signal that will take it
@@ -6730,8 +6730,7 @@ process_event_stop_test (struct execution_control_state *ecs)
{
struct frame_id current_id
= get_frame_id (get_current_frame ());
- if (frame_id_eq (current_id,
- ecs->event_thread->initiating_frame))
+ if (current_id == ecs->event_thread->initiating_frame)
{
/* Case 2. Fall through. */
}
@@ -6904,8 +6903,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
- || frame_id_eq (get_frame_id (frame),
- ecs->event_thread->control.step_frame_id)))
+ || get_frame_id (frame) == ecs->event_thread->control.step_frame_id))
{
infrun_debug_printf
("stepping inside range [%s-%s]",
@@ -7039,7 +7037,7 @@ process_event_stop_test (struct execution_control_state *ecs)
previous frame's ID is sufficient - but it is a common case and
cheaper than checking the previous frame's ID.
- NOTE: frame_id_eq will never report two invalid frame IDs as
+ NOTE: frame_id::operator== will never report two invalid frame IDs as
being equal, so to get into this block, both the current and
previous frame must have valid frame IDs. */
/* The outer_frame_id check is a heuristic to detect stepping
@@ -7049,14 +7047,14 @@ process_event_stop_test (struct execution_control_state *ecs)
"outermost" function. This could be fixed by marking
outermost frames as !stack_p,code_p,special_p. Then the
initial outermost frame, before sp was valid, would
- have code_addr == &_start. See the comment in frame_id_eq
+ have code_addr == &_start. See the comment in frame_id::operator==
for more. */
- if (!frame_id_eq (get_stack_frame_id (frame),
- ecs->event_thread->control.step_stack_frame_id)
- && (frame_id_eq (frame_unwind_caller_id (get_current_frame ()),
- ecs->event_thread->control.step_stack_frame_id)
- && (!frame_id_eq (ecs->event_thread->control.step_stack_frame_id,
- outer_frame_id)
+ if ((get_stack_frame_id (frame)
+ != ecs->event_thread->control.step_stack_frame_id)
+ && ((frame_unwind_caller_id (get_current_frame ())
+ == ecs->event_thread->control.step_stack_frame_id)
+ && ((ecs->event_thread->control.step_stack_frame_id
+ != outer_frame_id)
|| (ecs->event_thread->control.step_start_function
!= find_pc_function (ecs->event_thread->stop_pc ())))))
{
@@ -7313,8 +7311,8 @@ process_event_stop_test (struct execution_control_state *ecs)
frame machinery detected some skipped call sites, we have entered
a new inline function. */
- if (frame_id_eq (get_frame_id (get_current_frame ()),
- ecs->event_thread->control.step_frame_id)
+ if ((get_frame_id (get_current_frame ())
+ == ecs->event_thread->control.step_frame_id)
&& inline_skipped_frames (ecs->event_thread))
{
infrun_debug_printf ("stepped into inlined function");
@@ -7362,8 +7360,8 @@ 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
- && !frame_id_eq (get_frame_id (get_current_frame ()),
- ecs->event_thread->control.step_frame_id)
+ && (get_frame_id (get_current_frame ())
+ != ecs->event_thread->control.step_frame_id)
&& stepped_in_from (get_current_frame (),
ecs->event_thread->control.step_frame_id))
{
@@ -7395,8 +7393,8 @@ process_event_stop_test (struct execution_control_state *ecs)
end_stepping_range (ecs);
return;
}
- else if (frame_id_eq (get_frame_id (get_current_frame ()),
- ecs->event_thread->control.step_frame_id))
+ else if (get_frame_id (get_current_frame ())
+ == ecs->event_thread->control.step_frame_id)
{
/* We are not at the start of a statement, and we have not changed
frame.
@@ -8458,8 +8456,8 @@ print_stop_location (const target_waitstatus &ws)
should) carry around the function and does (or should) use
that when doing a frame comparison. */
if (tp->control.stop_step
- && frame_id_eq (tp->control.step_frame_id,
- get_frame_id (get_current_frame ()))
+ && (tp->control.step_frame_id
+ == get_frame_id (get_current_frame ()))
&& (tp->control.step_start_function
== find_pc_function (tp->stop_pc ())))
{
diff --git a/gdb/mi/mi-cmd-stack.c b/gdb/mi/mi-cmd-stack.c
index 0fe204dbc66..40c1345404d 100644
--- a/gdb/mi/mi-cmd-stack.c
+++ b/gdb/mi/mi-cmd-stack.c
@@ -729,7 +729,7 @@ parse_frame_specification (const char *frame_exp)
fid != NULL;
fid = get_prev_frame (fid))
{
- if (frame_id_eq (id, get_frame_id (fid)))
+ if (id == get_frame_id (fid))
{
struct frame_info *prev_frame;
@@ -737,7 +737,7 @@ parse_frame_specification (const char *frame_exp)
{
prev_frame = get_prev_frame (fid);
if (!prev_frame
- || !frame_id_eq (id, get_frame_id (prev_frame)))
+ || id != get_frame_id (prev_frame))
break;
fid = prev_frame;
}
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 68868e49e99..ee2e9acf634 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -2003,7 +2003,7 @@ struct user_selected_context
reports not-equal, we only do the equality test if we have something
other than the innermost frame selected. */
if (current_frame_level != -1
- && !frame_id_eq (current_frame_id, m_previous_frame_id))
+ && current_frame_id != m_previous_frame_id)
return true;
/* Nothing changed! */
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index c80096f6810..2d476114b35 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -207,7 +207,7 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
else
{
frame_id = get_frame_id (prev_frame);
- if (frame_id_eq (frame_id, null_frame_id))
+ if (frame_id == null_frame_id)
PyErr_SetString (PyExc_ValueError,
_("Invalid ID for the `frame' object."));
}
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 9a28c36c1cc..9b3a0ac142b 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -704,7 +704,7 @@ frapy_richcompare (PyObject *self, PyObject *other, int op)
frame_object *other_frame = (frame_object *) other;
if (self_frame->frame_id_is_next == other_frame->frame_id_is_next
- && frame_id_eq (self_frame->frame_id, other_frame->frame_id))
+ && self_frame->frame_id == other_frame->frame_id)
result = Py_EQ;
else
result = Py_NE;
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 3f8a69dd04f..e5a1c630618 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2033,10 +2033,8 @@ record_btrace_start_replaying (struct thread_info *tp)
frame_id = get_thread_current_frame_id (tp);
/* Check if we need to update any stepping-related frame id's. */
- upd_step_frame_id = frame_id_eq (frame_id,
- tp->control.step_frame_id);
- upd_step_stack_frame_id = frame_id_eq (frame_id,
- tp->control.step_stack_frame_id);
+ upd_step_frame_id = (frame_id == tp->control.step_frame_id);
+ upd_step_stack_frame_id = (frame_id == tp->control.step_stack_frame_id);
/* We start replaying at the end of the branch trace. This corresponds
to the current instruction. */
diff --git a/gdb/stack.c b/gdb/stack.c
index 80801ccdb01..6e0f58565fe 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -3252,7 +3252,7 @@ find_frame_for_address (CORE_ADDR address)
fid != NULL;
fid = get_prev_frame (fid))
{
- if (frame_id_eq (id, get_frame_id (fid)))
+ if (id == get_frame_id (fid))
{
struct frame_info *prev_frame;
@@ -3260,7 +3260,7 @@ find_frame_for_address (CORE_ADDR address)
{
prev_frame = get_prev_frame (fid);
if (!prev_frame
- || !frame_id_eq (id, get_frame_id (prev_frame)))
+ || id != get_frame_id (prev_frame))
break;
fid = prev_frame;
}
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 75ac0cef3b0..98f1563a89a 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2190,8 +2190,7 @@ tfind_1 (enum trace_find_type type, int num,
function and it's arguments) -- otherwise we'll just show the
new source line. */
- if (frame_id_eq (old_frame_id,
- get_frame_id (get_current_frame ())))
+ if (old_frame_id == get_frame_id (get_current_frame ()))
print_what = SRC_LINE;
else
print_what = SRC_AND_LOC;
diff --git a/gdb/value.c b/gdb/value.c
index 022fca91a42..e3de2b1f6e3 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -3975,7 +3975,7 @@ value_fetch_lazy_register (struct value *val)
in this situation. */
if (VALUE_LVAL (new_val) == lval_register
&& value_lazy (new_val)
- && frame_id_eq (VALUE_NEXT_FRAME_ID (new_val), next_frame_id))
+ && VALUE_NEXT_FRAME_ID (new_val) == next_frame_id)
internal_error (__FILE__, __LINE__,
_("infinite loop while fetching a register"));
}
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2 2/4] Introduce frame_info_ptr smart pointer class
2022-07-08 16:07 [PATCH v2 0/4] Smart pointer wrapper for frame_info Bruno Larsen
2022-07-08 16:07 ` [PATCH v2 1/4] Remove frame_id_eq Bruno Larsen
@ 2022-07-08 16:07 ` Bruno Larsen
2022-07-09 3:26 ` Tom Tromey
2022-07-08 16:07 ` [PATCH v2 4/4] gdb/frame: Add reinflation method for frame_info_ptr Bruno Larsen
2 siblings, 1 reply; 6+ messages in thread
From: Bruno Larsen @ 2022-07-08 16:07 UTC (permalink / raw)
To: gdb-patches; +Cc: tom
From: Tom Tromey <tom@tromey.com>
This adds frame_info_ptr, a smart pointer class. Every instance of
the class is kept on a circular, doubly-linked 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.
---
gdb/frame-info.h | 185 +++++++++++++++++++++++++++++++++++++++++++++++
gdb/frame.c | 8 ++
gdb/frame.h | 2 +
3 files changed, 195 insertions(+)
create mode 100644 gdb/frame-info.h
diff --git a/gdb/frame-info.h b/gdb/frame-info.h
new file mode 100644
index 00000000000..001e8984c90
--- /dev/null
+++ b/gdb/frame-info.h
@@ -0,0 +1,185 @@
+/* Frame info pointer
+
+ Copyright (C) 2022 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef GDB_FRAME_INFO_H
+#define GDB_FRAME_INFO_H
+
+struct frame_info;
+
+extern void reinit_frame_cache ();
+
+/* A wrapper for "frame_info *". frame_info objects are invalidated
+ whenever reinit_frame_cache is called. This class arranges to
+ invalidate the pointer when appropriate. This is done to help
+ detect a GDB bug that was relatively common.
+
+ A small amount of code must still operate on raw pointers, so a
+ "get" method is provided. However, you should normally not use
+ this in new code. */
+
+class frame_info_ptr
+{
+public:
+ /* Create a frame_info_ptr from a raw pointer. */
+ explicit frame_info_ptr (struct frame_info *ptr)
+ : m_ptr (ptr),
+ m_next (&root),
+ m_prev (root.m_prev)
+ {
+ root.m_prev->m_next = this;
+ root.m_prev = this;
+ }
+
+ /* Create a null frame_info_ptr. */
+ frame_info_ptr ()
+ : frame_info_ptr ((struct frame_info *) nullptr)
+ {
+ }
+
+ frame_info_ptr (std::nullptr_t)
+ : frame_info_ptr ((struct frame_info *) nullptr)
+ {
+ }
+
+ frame_info_ptr (const frame_info_ptr &other)
+ : frame_info_ptr (other.m_ptr)
+ {
+ }
+
+ frame_info_ptr (frame_info_ptr &&other)
+ : frame_info_ptr (other.m_ptr)
+ {
+ }
+
+ ~frame_info_ptr ()
+ {
+ m_next->m_prev = m_prev;
+ m_prev->m_next = m_next;
+ }
+
+ frame_info_ptr &operator= (const frame_info_ptr &other)
+ {
+ m_ptr = other.m_ptr;
+ return *this;
+ }
+
+ frame_info_ptr &operator= (std::nullptr_t)
+ {
+ m_ptr = nullptr;
+ return *this;
+ }
+
+ frame_info_ptr &operator= (frame_info_ptr &&other)
+ {
+ m_ptr = other.m_ptr;
+ return *this;
+ }
+
+ frame_info *operator-> () const
+ {
+ return m_ptr;
+ }
+
+ /* Fetch the underlying pointer. Note that new code should
+ generally not use this -- avoid it if at all possible. */
+ frame_info *get () const
+ {
+ return m_ptr;
+ }
+
+ /* This exists for compatibility with pre-existing code that checked
+ a "frame_info *" using "!". */
+ bool operator! () const
+ {
+ return m_ptr == nullptr;
+ }
+
+ /* This exists for compatibility with pre-existing code that checked
+ a "frame_info *" like "if (ptr)". */
+ explicit operator bool () const
+ {
+ return m_ptr != nullptr;
+ }
+
+private:
+
+ /* This constructor is used only for the root of the doubly-linked
+ list. See "root", below. It is explicit and given a parameter
+ to readily distinguish it from ordinary constructors. */
+ explicit frame_info_ptr (bool ignored)
+ : m_ptr (nullptr),
+ m_next (this),
+ m_prev (this)
+ {
+ }
+
+ /* The underlying pointer. */
+ frame_info *m_ptr;
+ /* Point to next and previous items in the circular list. */
+ frame_info_ptr *m_next;
+ frame_info_ptr *m_prev;
+
+ /* All frame_info_ptr objects are kept on a circular doubly-linked
+ list. This keeps their construction and destruction costs
+ reasonably small. To make the implementation a little simpler,
+ we guarantee that there is always at least one object on the list
+ -- this "root". */
+ static frame_info_ptr root;
+
+ /* A friend so it can invalidate the pointers. */
+ friend void reinit_frame_cache ();
+};
+
+static inline bool
+operator== (const frame_info *self, const frame_info_ptr &other)
+{
+ return self == other.get ();
+}
+
+static inline bool
+operator== (const frame_info_ptr &self, const frame_info_ptr &other)
+{
+ return self.get () == other.get ();
+}
+
+static inline bool
+operator== (const frame_info_ptr &self, const frame_info *other)
+{
+ return self.get () == other;
+}
+
+static inline bool
+operator!= (const frame_info *self, const frame_info_ptr &other)
+{
+ return self != other.get ();
+}
+
+static inline bool
+operator!= (const frame_info_ptr &self, const frame_info_ptr &other)
+{
+ return self.get () != other.get ();
+}
+
+static inline bool
+operator!= (const frame_info_ptr &self, const frame_info *other)
+{
+ return self.get () != other;
+}
+
+#endif /* GDB_FRAME_INFO_H */
diff --git a/gdb/frame.c b/gdb/frame.c
index c0cf3d585bf..0354cf2dbd7 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -56,6 +56,9 @@ static struct frame_info *sentinel_frame;
/* Number of calls to reinit_frame_cache. */
static unsigned int frame_cache_generation = 0;
+/* See frame-info.h. */
+frame_info_ptr frame_info_ptr::root (true);
+
/* See frame.h. */
unsigned int
@@ -2006,6 +2009,11 @@ reinit_frame_cache (void)
select_frame (NULL);
frame_stash_invalidate ();
+ for (frame_info_ptr *iter = frame_info_ptr::root.m_next;
+ iter != &frame_info_ptr::root;
+ iter = iter->m_next)
+ *iter = nullptr;
+
frame_debug_printf ("generation=%d", frame_cache_generation);
}
diff --git a/gdb/frame.h b/gdb/frame.h
index 75bb3bd2aa0..9ad2599331f 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -20,6 +20,8 @@
#if !defined (FRAME_H)
#define FRAME_H 1
+#include "frame-info.h"
+
/* The following is the intended naming schema for frame functions.
It isn't 100% consistent, but it is approaching that. Frame naming
schema:
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/4] Introduce frame_info_ptr smart pointer class
2022-07-08 16:07 ` [PATCH v2 2/4] Introduce frame_info_ptr smart pointer class Bruno Larsen
@ 2022-07-09 3:26 ` Tom Tromey
2022-07-11 12:18 ` Bruno Larsen
0 siblings, 1 reply; 6+ messages in thread
From: Tom Tromey @ 2022-07-09 3:26 UTC (permalink / raw)
To: Bruno Larsen; +Cc: gdb-patches, tom
>>>>> "Bruno" == Bruno Larsen <blarsen@redhat.com> writes:
Bruno> + /* The underlying pointer. */
Bruno> + frame_info *m_ptr;
Bruno> + /* Point to next and previous items in the circular list. */
Bruno> + frame_info_ptr *m_next;
Bruno> + frame_info_ptr *m_prev;
The reason I didn't resubmit this is that there was a request to make
this use some pre-existing template class, and my attempts to do that
failed. I think the WIP is in my github, but if not I can push it.
Anyway I don't think this can go in as is.
Tom
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/4] Introduce frame_info_ptr smart pointer class
2022-07-09 3:26 ` Tom Tromey
@ 2022-07-11 12:18 ` Bruno Larsen
0 siblings, 0 replies; 6+ messages in thread
From: Bruno Larsen @ 2022-07-11 12:18 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 7/9/22 00:26, Tom Tromey wrote:
>>>>>> "Bruno" == Bruno Larsen <blarsen@redhat.com> writes:
>
> Bruno> + /* The underlying pointer. */
> Bruno> + frame_info *m_ptr;
> Bruno> + /* Point to next and previous items in the circular list. */
> Bruno> + frame_info_ptr *m_next;
> Bruno> + frame_info_ptr *m_prev;
>
> The reason I didn't resubmit this is that there was a request to make
> this use some pre-existing template class, and my attempts to do that
> failed. I think the WIP is in my github, but if not I can push it.
> Anyway I don't think this can go in as is.
>
Ah, sorry, I seem to have squashed my commits incorrectly. Patch 3 makes frame_info_ptr
an intrusive lis, when it should have been here all along.
I did get the WIP from github, fixed the regressions I could see (Some tailcall problems),
along with adding a manual reinflate method.
Cheers!
Bruno Larsen
> Tom
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 4/4] gdb/frame: Add reinflation method for frame_info_ptr
2022-07-08 16:07 [PATCH v2 0/4] Smart pointer wrapper for frame_info Bruno Larsen
2022-07-08 16:07 ` [PATCH v2 1/4] Remove frame_id_eq Bruno Larsen
2022-07-08 16:07 ` [PATCH v2 2/4] Introduce frame_info_ptr smart pointer class Bruno Larsen
@ 2022-07-08 16:07 ` Bruno Larsen
2 siblings, 0 replies; 6+ messages in thread
From: Bruno Larsen @ 2022-07-08 16:07 UTC (permalink / raw)
To: gdb-patches; +Cc: tom, Bruno Larsen
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 and
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 because the get method and the -> operator must not change
the internals of the class, and 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.
---
gdb/frame-id.h | 135 +++++++++++++++++
gdb/frame-info.h | 32 ++++-
gdb/frame.h | 116 ---------------
gdb/infcmd.c | 2 +
gdb/stack.c | 11 ++
.../gdb.python/pretty-print-call-by-hand.c | 53 +++++++
.../gdb.python/pretty-print-call-by-hand.exp | 136 ++++++++++++++++++
.../gdb.python/pretty-print-call-by-hand.py | 41 ++++++
8 files changed, 408 insertions(+), 118 deletions(-)
create mode 100644 gdb/frame-id.h
create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
create mode 100644 gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
diff --git a/gdb/frame-id.h b/gdb/frame-id.h
new file mode 100644
index 00000000000..142488f7319
--- /dev/null
+++ b/gdb/frame-id.h
@@ -0,0 +1,135 @@
+/* Definitions for dealing with stack frames, for GDB, the GNU debugger.
+
+ Copyright (C) 1986-2022 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef GDB_FRAME_ID_H
+#define GDB_FRAME_ID_H 1
+
+/* Status of a given frame's stack. */
+
+enum frame_id_stack_status
+{
+ /* Stack address is invalid. */
+ FID_STACK_INVALID = 0,
+
+ /* Stack address is valid, and is found in the stack_addr field. */
+ FID_STACK_VALID = 1,
+
+ /* Sentinel frame. */
+ FID_STACK_SENTINEL = 2,
+
+ /* Outer frame. Since a frame's stack address is typically defined as the
+ value the stack pointer had prior to the activation of the frame, an outer
+ frame doesn't have a stack address. The frame ids of frames inlined in the
+ outer frame are also of this type. */
+ FID_STACK_OUTER = 3,
+
+ /* Stack address is unavailable. I.e., there's a valid stack, but
+ we don't know where it is (because memory or registers we'd
+ compute it from were not collected). */
+ FID_STACK_UNAVAILABLE = -1
+};
+
+/* The frame object's ID. This provides a per-frame unique identifier
+ that can be used to relocate a `struct frame_info' after a target
+ resume or a frame cache destruct. It of course assumes that the
+ inferior hasn't unwound the stack past that frame. */
+
+struct frame_id
+{
+ /* The frame's stack address. This shall be constant through out
+ the lifetime of a frame. Note that this requirement applies to
+ not just the function body, but also the prologue and (in theory
+ at least) the epilogue. Since that value needs to fall either on
+ the boundary, or within the frame's address range, the frame's
+ outer-most address (the inner-most address of the previous frame)
+ is used. Watch out for all the legacy targets that still use the
+ function pointer register or stack pointer register. They are
+ wrong.
+
+ This field is valid only if frame_id.stack_status is
+ FID_STACK_VALID. It will be 0 for other
+ FID_STACK_... statuses. */
+ CORE_ADDR stack_addr;
+
+ /* The frame's code address. This shall be constant through out the
+ lifetime of the frame. While the PC (a.k.a. resume address)
+ changes as the function is executed, this code address cannot.
+ Typically, it is set to the address of the entry point of the
+ frame's function (as returned by get_frame_func).
+
+ For inlined functions (INLINE_DEPTH != 0), this is the address of
+ the first executed instruction in the block corresponding to the
+ inlined function.
+
+ This field is valid only if code_addr_p is true. Otherwise, this
+ frame is considered to have a wildcard code address, i.e. one that
+ matches every address value in frame comparisons. */
+ CORE_ADDR code_addr;
+
+ /* The frame's special address. This shall be constant through out the
+ lifetime of the frame. This is used for architectures that may have
+ frames that do not change the stack but are still distinct and have
+ some form of distinct identifier (e.g. the ia64 which uses a 2nd
+ stack for registers). This field is treated as unordered - i.e. will
+ not be used in frame ordering comparisons.
+
+ This field is valid only if special_addr_p is true. Otherwise, this
+ frame is considered to have a wildcard special address, i.e. one that
+ matches every address value in frame comparisons. */
+ CORE_ADDR special_addr;
+
+ /* Flags to indicate the above fields have valid contents. */
+ ENUM_BITFIELD(frame_id_stack_status) stack_status : 3;
+ unsigned int code_addr_p : 1;
+ unsigned int special_addr_p : 1;
+
+ /* It is non-zero for a frame made up by GDB without stack data
+ representation in inferior, such as INLINE_FRAME or TAILCALL_FRAME.
+ Caller of inlined function will have it zero, each more inner called frame
+ will have it increasingly one, two etc. Similarly for TAILCALL_FRAME. */
+ int artificial_depth;
+
+ /* Return a string representation of this frame id. */
+ std::string to_string () const;
+
+ /* Returns true when this frame_id and R identify the same
+ frame. */
+ bool operator== (const frame_id &r) const;
+
+ /* Inverse of ==. */
+ bool operator!= (const frame_id &r) const
+ {
+ return !(*this == r);
+ }
+};
+
+/* Methods for constructing and comparing Frame IDs. */
+
+/* For convenience. All fields are zero. This means "there is no frame". */
+extern const struct frame_id null_frame_id;
+
+/* Sentinel frame. */
+extern const struct frame_id sentinel_frame_id;
+
+/* This means "there is no frame ID, but there is a frame". It should be
+ replaced by best-effort frame IDs for the outermost frame, somehow.
+ The implementation is only special_addr_p set. */
+extern const struct frame_id outer_frame_id;
+
+#endif /* ifdef GDB_FRAME_ID_H */
diff --git a/gdb/frame-info.h b/gdb/frame-info.h
index ecbf4a56545..801499bc25f 100644
--- a/gdb/frame-info.h
+++ b/gdb/frame-info.h
@@ -21,10 +21,15 @@
#define GDB_FRAME_INFO_H
#include "gdbsupport/intrusive_list.h"
+#include "frame-id.h"
struct frame_info;
+/* Forward declarations of functions, needed for the frame_info_ptr
+ to work correctly. */
extern void reinit_frame_cache ();
+extern struct frame_id get_frame_id (frame_info_ptr);
+extern frame_info_ptr frame_find_by_id (struct frame_id id);
/* A wrapper for "frame_info *". frame_info objects are invalidated
whenever reinit_frame_cache is called. This class arranges to
@@ -57,13 +62,13 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
}
frame_info_ptr (const frame_info_ptr &other)
- : m_ptr (other.m_ptr)
+ : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id)
{
root.push_back (*this);
}
frame_info_ptr (frame_info_ptr &&other)
- : m_ptr (other.m_ptr)
+ : m_ptr (other.m_ptr), m_cached_id (other.m_cached_id)
{
other.m_ptr = nullptr;
root.push_back (*this);
@@ -77,19 +82,23 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
frame_info_ptr &operator= (const frame_info_ptr &other)
{
m_ptr = other.m_ptr;
+ m_cached_id = other.m_cached_id;
return *this;
}
frame_info_ptr &operator= (std::nullptr_t)
{
m_ptr = nullptr;
+ m_cached_id = null_frame_id;
return *this;
}
frame_info_ptr &operator= (frame_info_ptr &&other)
{
m_ptr = other.m_ptr;
+ m_cached_id = other.m_cached_id;
other.m_ptr = nullptr;
+ other.m_cached_id = null_frame_id;
return *this;
}
@@ -125,11 +134,30 @@ class frame_info_ptr : public intrusive_list_node<frame_info_ptr>
m_ptr = nullptr;
}
+ /* Cache the frame_id that the pointer will use to reinflate. */
+ void prepare_reinflate ()
+ {
+ m_cached_id = get_frame_id(*this);
+ }
+
+ /* Use the cached frame_id to reinflate the pointer. */
+ void reinflate ()
+ {
+ gdb_assert (m_cached_id != null_frame_id);
+
+ if(m_ptr == nullptr)
+ m_ptr = frame_find_by_id (m_cached_id).get ();
+ gdb_assert (m_ptr != nullptr);
+ }
+
private:
/* The underlying pointer. */
frame_info *m_ptr = nullptr;
+ /* The frame_id of the underlying pointer. */
+ frame_id m_cached_id = null_frame_id;
+
/* All frame_info_ptr objects are kept on a circular doubly-linked
list. This keeps their construction and destruction costs
reasonably small. To make the implementation a little simpler,
diff --git a/gdb/frame.h b/gdb/frame.h
index 759cd32c5e3..5ee96cee46c 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -84,109 +84,10 @@ struct ui_file;
struct ui_out;
struct frame_print_options;
-/* Status of a given frame's stack. */
-
-enum frame_id_stack_status
-{
- /* Stack address is invalid. */
- FID_STACK_INVALID = 0,
-
- /* Stack address is valid, and is found in the stack_addr field. */
- FID_STACK_VALID = 1,
-
- /* Sentinel frame. */
- FID_STACK_SENTINEL = 2,
-
- /* Outer frame. Since a frame's stack address is typically defined as the
- value the stack pointer had prior to the activation of the frame, an outer
- frame doesn't have a stack address. The frame ids of frames inlined in the
- outer frame are also of this type. */
- FID_STACK_OUTER = 3,
-
- /* Stack address is unavailable. I.e., there's a valid stack, but
- we don't know where it is (because memory or registers we'd
- compute it from were not collected). */
- FID_STACK_UNAVAILABLE = -1
-};
-
/* The frame object. */
class frame_info_ptr;
-/* The frame object's ID. This provides a per-frame unique identifier
- that can be used to relocate a `struct frame_info' after a target
- resume or a frame cache destruct. It of course assumes that the
- inferior hasn't unwound the stack past that frame. */
-
-struct frame_id
-{
- /* The frame's stack address. This shall be constant through out
- the lifetime of a frame. Note that this requirement applies to
- not just the function body, but also the prologue and (in theory
- at least) the epilogue. Since that value needs to fall either on
- the boundary, or within the frame's address range, the frame's
- outer-most address (the inner-most address of the previous frame)
- is used. Watch out for all the legacy targets that still use the
- function pointer register or stack pointer register. They are
- wrong.
-
- This field is valid only if frame_id.stack_status is
- FID_STACK_VALID. It will be 0 for other
- FID_STACK_... statuses. */
- CORE_ADDR stack_addr;
-
- /* The frame's code address. This shall be constant through out the
- lifetime of the frame. While the PC (a.k.a. resume address)
- changes as the function is executed, this code address cannot.
- Typically, it is set to the address of the entry point of the
- frame's function (as returned by get_frame_func).
-
- For inlined functions (INLINE_DEPTH != 0), this is the address of
- the first executed instruction in the block corresponding to the
- inlined function.
-
- This field is valid only if code_addr_p is true. Otherwise, this
- frame is considered to have a wildcard code address, i.e. one that
- matches every address value in frame comparisons. */
- CORE_ADDR code_addr;
-
- /* The frame's special address. This shall be constant through out the
- lifetime of the frame. This is used for architectures that may have
- frames that do not change the stack but are still distinct and have
- some form of distinct identifier (e.g. the ia64 which uses a 2nd
- stack for registers). This field is treated as unordered - i.e. will
- not be used in frame ordering comparisons.
-
- This field is valid only if special_addr_p is true. Otherwise, this
- frame is considered to have a wildcard special address, i.e. one that
- matches every address value in frame comparisons. */
- CORE_ADDR special_addr;
-
- /* Flags to indicate the above fields have valid contents. */
- ENUM_BITFIELD(frame_id_stack_status) stack_status : 3;
- unsigned int code_addr_p : 1;
- unsigned int special_addr_p : 1;
-
- /* It is non-zero for a frame made up by GDB without stack data
- representation in inferior, such as INLINE_FRAME or TAILCALL_FRAME.
- Caller of inlined function will have it zero, each more inner called frame
- will have it increasingly one, two etc. Similarly for TAILCALL_FRAME. */
- int artificial_depth;
-
- /* Return a string representation of this frame id. */
- std::string to_string () const;
-
- /* Returns true when this frame_id and R identify the same
- frame. */
- bool operator== (const frame_id &r) const;
-
- /* Inverse of ==. */
- bool operator!= (const frame_id &r) const
- {
- return !(*this == r);
- }
-};
-
/* Save and restore the currently selected frame. */
class scoped_restore_selected_frame
@@ -212,19 +113,6 @@ class scoped_restore_selected_frame
enum language m_lang;
};
-/* Methods for constructing and comparing Frame IDs. */
-
-/* For convenience. All fields are zero. This means "there is no frame". */
-extern const struct frame_id null_frame_id;
-
-/* Sentinel frame. */
-extern const struct frame_id sentinel_frame_id;
-
-/* This means "there is no frame ID, but there is a frame". It should be
- replaced by best-effort frame IDs for the outermost frame, somehow.
- The implementation is only special_addr_p set. */
-extern const struct frame_id outer_frame_id;
-
/* Flag to control debugging. */
extern bool frame_debug;
@@ -399,10 +287,6 @@ extern frame_info_ptr get_next_frame_sentinel_okay (frame_info_ptr );
frame. */
extern frame_info_ptr get_prev_frame_always (frame_info_ptr );
-/* Given a frame's ID, relocate the frame. Returns NULL if the frame
- is not found. */
-extern frame_info_ptr frame_find_by_id (struct frame_id id);
-
/* Base attributes of a frame: */
/* The frame's `resume' address. Where the program will resume in
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index f7f292e6a68..c5b22249063 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1814,6 +1814,7 @@ finish_command (const char *arg, int from_tty)
frame = get_prev_frame (get_selected_frame (_("No selected frame.")));
if (frame == 0)
error (_("\"finish\" not meaningful in the outermost frame."));
+ frame.prepare_reinflate ();
clear_proceed_status (0);
@@ -1872,6 +1873,7 @@ finish_command (const char *arg, int from_tty)
print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
}
+ frame.reinflate ();
if (execution_direction == EXEC_REVERSE)
finish_backward (sm);
diff --git a/gdb/stack.c b/gdb/stack.c
index 8a8936535f8..8c54e822d1f 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -362,11 +362,13 @@ print_stack_frame (frame_info_ptr frame, int print_level,
if (current_uiout->is_mi_like_p ())
print_what = LOC_AND_ADDRESS;
+ frame.prepare_reinflate ();
try
{
print_frame_info (user_frame_print_options,
frame, print_level, print_what, 1 /* print_args */,
set_current_sal);
+ frame.reinflate ();
if (set_current_sal)
set_current_sal_from_frame (frame);
}
@@ -742,6 +744,11 @@ print_frame_args (const frame_print_options &fp_opts,
= (print_names
&& fp_opts.print_frame_arguments != print_frame_arguments_none);
+ /* If one of the arguments has a pretty printer that calls a
+ function of the inferior to print it, the pointer must be
+ reinflatable. */
+ frame.prepare_reinflate ();
+
/* Temporarily change the selected frame to the given FRAME.
This allows routines that rely on the selected frame instead
of being given a frame as parameter to use the correct frame. */
@@ -902,6 +909,7 @@ print_frame_args (const frame_print_options &fp_opts,
}
first = 0;
+ frame.reinflate ();
}
}
@@ -1172,6 +1180,7 @@ print_frame_info (const frame_print_options &fp_opts,
print_source_lines (sal.symtab, sal.line, sal.line + 1, 0);
}
+ frame.reinflate ();
/* If disassemble-next-line is set to on and there is line debug
messages, output assembly codes for next line. */
@@ -2061,6 +2070,7 @@ backtrace_command_1 (const frame_print_options &fp_opts,
for (fi = trailing; fi && count--; fi = get_prev_frame (fi))
{
QUIT;
+ fi.prepare_reinflate ();
/* Don't use print_stack_frame; if an error() occurs it probably
means further attempts to backtrace would fail (on the other
@@ -2085,6 +2095,7 @@ backtrace_command_1 (const frame_print_options &fp_opts,
}
/* Save the last frame to check for error conditions. */
+ fi.reinflate ();
trailing = fi;
}
diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
new file mode 100644
index 00000000000..3be5675b096
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.c
@@ -0,0 +1,53 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2022 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+struct mytype
+{
+ char *x;
+};
+
+void
+rec (int i)
+{
+ if (i <= 0)
+ return;
+ rec (i-1);
+}
+
+int
+f ()
+{
+ rec (100);
+ return 2;
+}
+
+void
+g (struct mytype mt, int depth)
+{
+ if (depth <= 0)
+ return; /* TAG: final frame */
+ g (mt, depth - 1); /* TAG: first frame */
+}
+
+int
+main ()
+{
+ struct mytype mt;
+ mt.x = "hello world";
+ g (mt, 10); /* TAG: outside the frame */
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
new file mode 100644
index 00000000000..0aeb2218f91
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.exp
@@ -0,0 +1,136 @@
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite. It tests a pretty printer that
+# calls an inferior function by hand, triggering a Use-after-Free bug
+# (PR gdb/28856).
+
+load_lib gdb-python.exp
+
+standard_testfile
+
+# gdb needs to be started here for skip_python_tests to work.
+# prepare_for_testing could be used instead, but it could compile the program
+# unnecessarily, so starting GDB like this is preferable.
+gdb_start
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+ untested "failed to compile"
+ return -1
+}
+
+# This proc restarts GDB, makes the inferior reach the desired spot - marked
+# by a comment in the .c file - and turns on the pretty printer for testing.
+# Starting with a new GDB is important because the test may crash GDB. The
+# return values are here to avoid us trying to test the pretty printer if
+# there was a problem getting to main.
+proc start_test { breakpoint_comment } {
+ global srcdir subdir testfile binfile
+
+ # Start with a fresh gdb.
+ # This is important because the test can crash GDB.
+
+ clean_restart ${binfile}
+
+ if { ![runto_main] } then {
+ untested "couldn't run to breakpoint"
+ return -1
+ }
+
+ # Let GDB get to the return line.
+ gdb_breakpoint [gdb_get_line_number ${breakpoint_comment} ${testfile}.c ]
+ gdb_continue_to_breakpoint ${breakpoint_comment} ".*"
+
+ gdb_test_no_output "set print pretty on" "starting to pretty print"
+
+ set remote_python_file [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+ gdb_test_no_output "source ${remote_python_file}" "load python file"
+
+ return 0
+}
+
+# Start by testing the "run" command, it can't leverage start_test
+with_test_prefix "run to frame" {
+ if { ![runto_main] } then {
+ untested "couldn't run to main"
+ }
+
+ gdb_test_no_output "set print pretty on" "starting to pretty print"
+
+ set remote_python_file [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
+ gdb_test_no_output "source ${remote_python_file}" "load python file"
+
+ gdb_breakpoint [gdb_get_line_number "TAG: final frame" ${testfile}.c]
+ gdb_continue_to_breakpoint "TAG: final frame" ".*"
+}
+
+# Testing the backtrace command.
+with_test_prefix "frame print" {
+ if { [start_test "TAG: final frame"] == 0 } {
+ gdb_test "backtrace -frame-arguments all" [multi_line \
+ "#0 .*g \\(mt=mytype is .*\\, depth=0\\).*"\
+ "#1 .*g \\(mt=mytype is .*\\, depth=1\\).*"\
+ "#2 .*g \\(mt=mytype is .*\\, depth=2\\).*"\
+ "#3 .*g \\(mt=mytype is .*\\, depth=3\\).*"\
+ "#4 .*g \\(mt=mytype is .*\\, depth=4\\).*"\
+ "#5 .*g \\(mt=mytype is .*\\, depth=5\\).*"\
+ "#6 .*g \\(mt=mytype is .*\\, depth=6\\).*"\
+ "#7 .*g \\(mt=mytype is .*\\, depth=7\\).*"\
+ "#8 .*g \\(mt=mytype is .*\\, depth=8\\).*"\
+ "#9 .*g \\(mt=mytype is .*\\, depth=9\\).*"\
+ "#10 .*g \\(mt=mytype is .*\\, depth=10\\).*"\
+ "#11 .*main \\(\\).*"] \
+ "backtrace test"
+ }
+}
+# Testing the down command.
+with_test_prefix "frame movement down" {
+ if { [start_test "TAG: first frame"] == 0 } {
+ gdb_test "up" [multi_line "#1 .*in main \\(\\) at .*" ".*outside the frame.*"]
+ gdb_test "down" [multi_line "#0\\s+g \\(mt=mytype is .*\\, depth=10\\).*" ".*first frame.*"]
+ }
+}
+
+# Testing the up command.
+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.*"]
+ }
+}
+
+# Testing the finish command.
+with_test_prefix "frame exit through finish" {
+ if { [start_test "TAG: final frame"] == 0 } {
+ gdb_test "finish" [multi_line ".*.*g \\(mt=mytype is .*\\, depth=0\\).*" ".*g \\(mt=mytype is .*\\, depth=1\\).*" ".*"]
+ }
+}
+
+# Testing the step command.
+with_test_prefix "frame enter through step" {
+ if { [start_test "TAG: outside the frame"] == 0 } {
+ gdb_test "step" [multi_line "g \\(mt=mytype is .*\\, depth=10\\).*" "41.*if \\(depth \\<= 0\\)"]
+ }
+}
+
+# Testing the continue command.
+with_test_prefix "frame enter through continue" {
+ if { [start_test "TAG: outside the frame"] == 0 } {
+ gdb_breakpoint [gdb_get_line_number "TAG: first frame" ${testfile}.c ]
+ gdb_continue_to_breakpoint "TAG: first frame" ".*TAG: first frame.*"
+ }
+}
diff --git a/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
new file mode 100644
index 00000000000..f8f5df678f8
--- /dev/null
+++ b/gdb/testsuite/gdb.python/pretty-print-call-by-hand.py
@@ -0,0 +1,41 @@
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+
+class MytypePrinter:
+ """pretty print my type"""
+
+ def __init__(self, val):
+ self.val = val
+
+ def to_string(self):
+ calls = gdb.parse_and_eval('f()')
+ return "mytype is %s" % self.val['x']
+
+def ec_lookup_function(val):
+ typ = val.type
+ if typ.code == gdb.TYPE_CODE_REF:
+ typ = typ.target()
+ if str(typ) == 'struct mytype':
+ return MytypePrinter(val)
+ return None
+
+def disable_lookup_function():
+ ec_lookup_function.enabled = False
+
+def enable_lookup_function():
+ ec_lookup_function.enabled = True
+
+gdb.pretty_printers.append(ec_lookup_function)
--
2.31.1
^ permalink raw reply [flat|nested] 6+ messages in thread