Re: [downstream patch FYI] workaround stale frame_info * (PR 13866)

Pedro Alves
2012-06-05T19:17:01Z

On 04/04/2012 08:14 PM, Jan Kratochvil wrote:

> Hi,
>
> I did not look at which commit caused this regression but apparently it was
> introduced at least with multi-inferiors.
>
> I understand this fix is not right fix of the crash; but in most GDB cases one
> does not use multi-inferior so why to regress single-inferior by it.
> Some more simple solutions still fix the single-inferior mode but they
> regressed the multi-inferior mode
> gdb.threads/no-unwaited-for-left.exp
> gdb.multi/base.exp
> so I had to put there that sorting magic.
>
> With proper C++ sanity check of stale live frame_info references the testcase
> would be simple without the "frame_garbage_collection" reproducer below.
> It is also reproducible just with valgrind but regularly running the whole
> testsuite under valgrind I did not find feasible.
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora17-linux-gnu.
>

I've used this frame validation patch, based on yours, which sprinkles
validate_frame calls around frame.c to catch uses of already dead frame_info
objects. It makes the "until" bug 100% reproducible (with an internal_error) without
valgrind. I've then ran the whole testsuite with this on, and that didn't catch
any other problem. You mention in PR13866 many situations with stale
frame_info; did you have some other way to catch those, or was that just a hunch?

---

gdb/frame.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
gdb/top.c | 5 ++++
2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index e012f2d..285b60d 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -129,6 +129,14 @@ struct frame_info
enum unwind_stop_reason stop_reason;
};

+#define DEAD_FRAME_LEVEL (-55)
+
+static void
+validate_frame (struct frame_info *frame)
+{
+ gdb_assert (frame->level != DEAD_FRAME_LEVEL);
+}
+
/* A frame stash used to speed up frame lookups. */

/* We currently only stash one frame at a time, as this seems to be
@@ -324,6 +332,8 @@ get_frame_id (struct frame_info *fi)
if (fi == NULL)
return null_frame_id;

+ validate_frame (fi);
+
if (!fi->this_id.p)
{
if (frame_debug)
@@ -621,6 +631,8 @@ frame_find_by_id (struct frame_id id)
static int
frame_unwind_pc_if_available (struct frame_info *this_frame, CORE_ADDR *pc)
{
+ validate_frame (this_frame);
+
if (!this_frame->prev_pc.p)
{
if (gdbarch_unwind_pc_p (frame_unwind_arch (this_frame)))
@@ -1522,12 +1534,30 @@ frame_observer_target_changed (struct target_ops *target)
reinit_frame_cache ();
}

+typedef struct obstack obstack_s;
+DEF_VEC_O (obstack_s);
+static VEC (obstack_s) *frame_poison_vec;
+
+void frame_garbage_collection (void);
+void
+frame_garbage_collection (void)
+{
+ struct obstack *obstack_p;
+ int ix;
+
+ for (ix = 0; VEC_iterate (obstack_s, frame_poison_vec, ix, obstack_p); ix++)
+ obstack_free (obstack_p, 0);
+
+ VEC_free (obstack_s, frame_poison_vec);
+ frame_poison_vec = NULL;
+}
+
/* Flush the entire frame cache. */

void
reinit_frame_cache (void)
{
- struct frame_info *fi;
+ struct frame_info *fi, *fi_prev;

/* Tear down all frame caches. */
for (fi = current_frame; fi != NULL; fi = fi->prev)
@@ -1538,8 +1568,15 @@ reinit_frame_cache (void)
fi->base->unwind->dealloc_cache (fi, fi->base_cache);
}

+ for (fi = current_frame; fi != NULL; fi = fi_prev)
+ {
+ fi_prev = fi->prev;
+ memset (fi, 0, sizeof (*fi));
+ fi->level = DEAD_FRAME_LEVEL;
+ }
+ VEC_safe_push (obstack_s, frame_poison_vec, &frame_cache_obstack);
+
/* Since we can't really be sure what the first object allocated was. */
- obstack_free (&frame_cache_obstack, 0);
obstack_init (&frame_cache_obstack);

if (current_frame != NULL)
@@ -2070,6 +2107,8 @@ get_frame_address_in_block_if_available (struct frame_info *this_frame,
{
volatile struct gdb_exception ex;

+ validate_frame (this_frame);
+
TRY_CATCH (ex, RETURN_MASK_ERROR)
{
*pc = get_frame_address_in_block (this_frame);
@@ -2089,6 +2128,8 @@ find_frame_sal (struct frame_info *frame, struct symtab_and_line *sal)
int notcurrent;
CORE_ADDR pc;

+ validate_frame (frame);
+
/* If the next frame represents an inlined function call, this frame's
sal is the "call site" of that inlined function, which can not
be inferred from get_frame_pc. */
@@ -2145,6 +2186,8 @@ find_frame_sal (struct frame_info *frame, struct symtab_and_line *sal)
CORE_ADDR
get_frame_base (struct frame_info *fi)
{
+ validate_frame (fi);
+
return get_frame_id (fi).stack_addr;
}

@@ -2220,6 +2263,8 @@ frame_relative_level (struct frame_info *fi)
enum frame_type
get_frame_type (struct frame_info *frame)
{
+ validate_frame (frame);
+
if (frame->unwind == NULL)
/* Initialize the frame's unwinder because that's what
provides the frame's type. */
@@ -2230,6 +2275,8 @@ get_frame_type (struct frame_info *frame)
struct program_space *
get_frame_program_space (struct frame_info *frame)
{
+ validate_frame (frame);
+
return frame->pspace;
}

@@ -2238,6 +2285,8 @@ frame_unwind_program_space (struct frame_info *this_frame)
{
gdb_assert (this_frame);

+ validate_frame (this_frame);
+
/* This is really a placeholder to keep the API consistent --- we
assume for now that we don't have frame chains crossing
spaces. */
@@ -2247,6 +2296,8 @@ frame_unwind_program_space (struct frame_info *this_frame)
struct address_space *
get_frame_address_space (struct frame_info *frame)
{
+ validate_frame (frame);
+
return frame->aspace;
}

@@ -2256,6 +2307,8 @@ void
get_frame_memory (struct frame_info *this_frame, CORE_ADDR addr,
gdb_byte *buf, int len)
{
+ validate_frame (this_frame);
+
read_memory (addr, buf, len);
}

@@ -2266,6 +2319,8 @@ get_frame_memory_signed (struct frame_info *this_frame, CORE_ADDR addr,
struct gdbarch *gdbarch = get_frame_arch (this_frame);
enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

+ validate_frame (this_frame);
+
return read_memory_integer (addr, len, byte_order);
}

@@ -2276,6 +2331,8 @@ get_frame_memory_unsigned (struct frame_info *this_frame, CORE_ADDR addr,
struct gdbarch *gdbarch = get_frame_arch (this_frame);
enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

+ validate_frame (this_frame);
+
return read_memory_unsigned_integer (addr, len, byte_order);
}

@@ -2283,6 +2340,8 @@ int
safe_frame_unwind_memory (struct frame_info *this_frame,
CORE_ADDR addr, gdb_byte *buf, int len)
{
+ validate_frame (this_frame);
+
/* NOTE: target_read_memory returns zero on success! */
return !target_read_memory (addr, buf, len);
}
@@ -2292,12 +2351,15 @@ safe_frame_unwind_memory (struct frame_info *this_frame,
struct gdbarch *
get_frame_arch (struct frame_info *this_frame)
{
+ validate_frame (this_frame);
return frame_unwind_arch (this_frame->next);
}

struct gdbarch *
frame_unwind_arch (struct frame_info *next_frame)
{
+ validate_frame (next_frame);
+
if (!next_frame->prev_arch.p)
{
struct gdbarch *arch;
@@ -2326,6 +2388,8 @@ frame_unwind_arch (struct frame_info *next_frame)
struct gdbarch *
frame_unwind_caller_arch (struct frame_info *next_frame)
{
+ validate_frame (next_frame);
+
return frame_unwind_arch (skip_inlined_frames (next_frame));
}

@@ -2336,6 +2400,8 @@ get_frame_sp (struct frame_info *this_frame)
{
struct gdbarch *gdbarch = get_frame_arch (this_frame);

+ validate_frame (this_frame);
+
/* Normality - an architecture that provides a way of obtaining any
frame inner-most address. */
if (gdbarch_unwind_sp_p (gdbarch))
@@ -2355,6 +2421,8 @@ get_frame_sp (struct frame_info *this_frame)
enum unwind_stop_reason
get_frame_unwind_stop_reason (struct frame_info *frame)
{
+ validate_frame (frame);
+
/* If we haven't tried to unwind past this point yet, then assume
that unwinding would succeed. */
if (frame->prev_p == 0)
diff --git a/gdb/top.c b/gdb/top.c
index 061ad48..d797aff 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -360,6 +360,11 @@ prepare_execute_command (void)
if (non_stop)
target_dcache_invalidate ();

+ {
+ extern void frame_garbage_collection (void);
+ frame_garbage_collection ();
+ }
+
return cleanup;
}