This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo)
- From: Pedro Alves <palves at redhat dot com>
- To: Tom Tromey <tromey at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 21 Nov 2013 16:12:45 +0000
- Subject: Re: [PATCH] Don't let two frames with the same id end up in the frame chain. (Re: [PATCH 1/2] avoid infinite loop with bad debuginfo)
- Authentication-results: sourceware.org; auth=none
- References: <1384375873-32160-1-git-send-email-tromey at redhat dot com> <1384375873-32160-2-git-send-email-tromey at redhat dot com> <52850730 dot 1060109 at redhat dot com> <87d2lxpo1l dot fsf at fleche dot redhat dot com> <528B7F15 dot 7040605 at redhat dot com> <87vbzomm78 dot fsf at fleche dot redhat dot com> <528B8FF6 dot 7000406 at redhat dot com> <87siusl10r dot fsf at fleche dot redhat dot com> <528BB700 dot 4000802 at redhat dot com> <87fvqskumd dot fsf at fleche dot redhat dot com> <528CFEDE dot 1040505 at redhat dot com> <871u2aixbc dot fsf at fleche dot redhat dot com>
On 11/20/2013 09:21 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
>
> Tom> Really not looking forward to writing the test.
>
> Pedro> Yeah, me neither. :-P
>
> Well, I took at stab at it today, and totally failed.
> I will try to catch you on irc tomorrow to pick your brain, if that's ok
> with you, to try to understand how I could get a test case.
I wonder if it would be as "simple" as coming up with
dwarf that says all registers are found via DWARF2_FRAME_REG_SAME_VALUE?
>
> Pedro> Subject: Don't let two frames with the same id end up in the frame chain.
>
> Pedro> The UNWIND_SAME_ID check is done between THIS_FRAME and the next
> Pedro> frame. But at this point, it's already too late -- we ended up with
> Pedro> two frames with the same ID in the frame chain. Each frame having its
> Pedro> own ID is an invariant assumed throughout GDB. So this patch applies
> Pedro> the UNWIND_SAME_ID detection earlier, right after the previous frame
> Pedro> is unwond, discarding the dup frame if a cycle is detected.
>
> s/unwond/unwound/
>
> FWIW I have nearly the identical patch here :)
> I think it's a good idea.
>
> I also have the appended, which makes the frame stash behave a little
> nicer if an unwinder gives a duplicate frame id. Probably not needed in
> addition to the patch you sent, but on the other hand, cheap.
Yeah, that makes the stash behave like if there was no stash
in this scenario.
As the stash now hold the ids of all frames in the chain, this
looks like the place we can detect these bad issues, for easier
diagnosis of whatever problem this could cause. Perhaps we
should complaint() or warn() ?
Hmm, wait...
Why not go the full mile? We could use this to go one
step further, and detect corrupted stacks that create
stack cycles with non-consecutive dup frame ids:
#0 frame_id1
#1 frame_id2
#2 frame_id3
#3 frame_id1
#4 frame_id2
#5 frame_id3
#6 frame_id1
... forever ...
Given we already have the stash, it's just as cheap as just
detecting the cycle in consecutive frames. See below, for
a patch on top of the previous one.
----
Make use of the frame stash to detect wider stack cycles.
Tested on x86_64 Fedora 17.
gdb/
2013-11-21 Pedro Alves <palves@redhat.com>
Tom Tromey <tromey@redhat.com>
* frame.c (frame_stash_add): Now returns whether a frame with the
same ID was already known.
(compute_frame_id): New function, factored out from get_frame_id.
(get_frame_id): No longer lazilly compute the frame id here.
(get_prev_frame_if_no_cycle): New function. Detects wider stack
cycles.
(get_prev_frame_1): Use it instead of get_prev_frame_raw directly,
and checking for stack cycles here.
---
gdb/frame.c | 152 +++++++++++++++++++++++++++++++++++------------------------
1 file changed, 89 insertions(+), 63 deletions(-)
diff --git a/gdb/frame.c b/gdb/frame.c
index 535a5a6..891b4ea 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -188,23 +188,31 @@ frame_stash_create (void)
NULL);
}
-/* Internal function to add a frame to the frame_stash hash table. Do
- not store frames below 0 as they may not have any addresses to
- calculate a hash. */
+/* Internal function to add a frame to the frame_stash hash table.
+ Returns false if a frame with the same ID was already stashed, true
+ otherwise. */
-static void
+static int
frame_stash_add (struct frame_info *frame)
{
- /* Do not stash frames below level 0. */
- if (frame->level >= 0)
- {
- struct frame_info **slot;
+ struct frame_info **slot;
- slot = (struct frame_info **) htab_find_slot (frame_stash,
- frame,
- INSERT);
- *slot = frame;
- }
+ /* Do not try to stash the sentinel frame. */
+ gdb_assert (frame->level >= 0);
+
+ slot = (struct frame_info **) htab_find_slot (frame_stash,
+ frame,
+ INSERT);
+
+ /* If we already have a frame in the stack with the same id, we
+ either have a stack cycle (corrupted stack?), or some bug
+ elsewhere in GDB. In any case, ignore the duplicate and return
+ an indication to the caller. */
+ if (*slot != NULL)
+ return 0;
+
+ *slot = frame;
+ return 1;
}
/* Internal function to search the frame stash for an entry with the
@@ -389,6 +397,34 @@ skip_artificial_frames (struct frame_info *frame)
return frame;
}
+/* Compute the frame's uniq ID that can be used to, later, re-find the
+ frame. */
+
+static void
+compute_frame_id (struct frame_info *fi)
+{
+ gdb_assert (!fi->this_id.p);
+
+ if (frame_debug)
+ fprintf_unfiltered (gdb_stdlog, "{ compute_frame_id (fi=%d) ",
+ fi->level);
+ /* Find the unwinder. */
+ if (fi->unwind == NULL)
+ frame_unwind_find_by_frame (fi, &fi->prologue_cache);
+ /* Find THIS frame's ID. */
+ /* Default to outermost if no ID is found. */
+ fi->this_id.value = outer_frame_id;
+ fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+ gdb_assert (frame_id_p (fi->this_id.value));
+ fi->this_id.p = 1;
+ if (frame_debug)
+ {
+ fprintf_unfiltered (gdb_stdlog, "-> ");
+ fprint_frame_id (gdb_stdlog, fi->this_id.value);
+ fprintf_unfiltered (gdb_stdlog, " }\n");
+ }
+}
+
/* Return a frame uniq ID that can be used to, later, re-find the
frame. */
@@ -398,29 +434,7 @@ get_frame_id (struct frame_info *fi)
if (fi == NULL)
return null_frame_id;
- if (!fi->this_id.p)
- {
- if (frame_debug)
- fprintf_unfiltered (gdb_stdlog, "{ get_frame_id (fi=%d) ",
- fi->level);
- /* Find the unwinder. */
- if (fi->unwind == NULL)
- frame_unwind_find_by_frame (fi, &fi->prologue_cache);
- /* Find THIS frame's ID. */
- /* Default to outermost if no ID is found. */
- fi->this_id.value = outer_frame_id;
- fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
- gdb_assert (frame_id_p (fi->this_id.value));
- fi->this_id.p = 1;
- if (frame_debug)
- {
- fprintf_unfiltered (gdb_stdlog, "-> ");
- fprint_frame_id (gdb_stdlog, fi->this_id.value);
- fprintf_unfiltered (gdb_stdlog, " }\n");
- }
- frame_stash_add (fi);
- }
-
+ gdb_assert (fi->this_id.p);
return fi->this_id.value;
}
@@ -1655,6 +1669,43 @@ frame_register_unwind_location (struct frame_info *this_frame, int regnum,
}
}
+/* Get the previous raw frame, and check that it is not identical to
+ same other frame frame already in the chain. If it is, there is
+ most likely a stack cycle, so we discard it, and mark THIS_FRAME as
+ outermost, with UNWIND_SAME_ID stop reason. Unlike the other
+ validity tests, that compare THIS_FRAME and the next frame, we do
+ this right after creating the prev frame, to avoid ever ending up
+ with two frames with the same id in the frame chain. */
+
+static struct frame_info *
+get_prev_frame_if_no_cycle (struct frame_info *this_frame)
+{
+ struct frame_info *prev_frame;
+
+ prev_frame = get_prev_frame_raw (this_frame);
+ if (prev_frame == NULL)
+ return NULL;
+
+ gdb_assert (!prev_frame->this_id.p);
+ compute_frame_id (prev_frame);
+ if (frame_stash_add (prev_frame))
+ return prev_frame;
+
+ /* Another frame with the same id was already in the stash. We just
+ detected a cycle. */
+ if (frame_debug)
+ {
+ fprintf_unfiltered (gdb_stdlog, "-> ");
+ fprint_frame (gdb_stdlog, NULL);
+ fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
+ }
+ this_frame->stop_reason = UNWIND_SAME_ID;
+ /* Unlink. */
+ prev_frame->next = NULL;
+ this_frame->prev = NULL;
+ return NULL;
+}
+
/* Return a "struct frame_info" corresponding to the frame that called
THIS_FRAME. Returns NULL if there is no such frame.
@@ -1666,7 +1717,6 @@ get_prev_frame_1 (struct frame_info *this_frame)
{
struct frame_id this_id;
struct gdbarch *gdbarch;
- struct frame_info *prev_frame;
gdb_assert (this_frame != NULL);
gdbarch = get_frame_arch (this_frame);
@@ -1709,7 +1759,7 @@ get_prev_frame_1 (struct frame_info *this_frame)
until we have unwound all the way down to the previous non-inline
frame. */
if (get_frame_type (this_frame) == INLINE_FRAME)
- return get_prev_frame_raw (this_frame);
+ return get_prev_frame_if_no_cycle (this_frame);
/* Check that this frame is unwindable. If it isn't, don't try to
unwind to the prev frame. */
@@ -1815,31 +1865,7 @@ get_prev_frame_1 (struct frame_info *this_frame)
}
}
- prev_frame = get_prev_frame_raw (this_frame);
-
- /* Check that this and the prev frame are not identical. If they
- are, there is most likely a stack cycle. Unlike the tests above,
- we do this right after creating the prev frame, to avoid ever
- ending up with two frames with the same id in the frame
- chain. */
- if (prev_frame != NULL
- && frame_id_eq (get_frame_id (prev_frame),
- get_frame_id (this_frame)))
- {
- if (frame_debug)
- {
- fprintf_unfiltered (gdb_stdlog, "-> ");
- fprint_frame (gdb_stdlog, NULL);
- fprintf_unfiltered (gdb_stdlog, " // this frame has same ID }\n");
- }
- this_frame->stop_reason = UNWIND_SAME_ID;
- /* Unlink. */
- prev_frame->next = NULL;
- this_frame->prev = NULL;
- return NULL;
- }
-
- return prev_frame;
+ return get_prev_frame_if_no_cycle (this_frame);
}
/* Construct a new "struct frame_info" and link it previous to