From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30645 invoked by alias); 21 Nov 2013 16:13:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 30636 invoked by uid 89); 21 Nov 2013 16:13:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.1 required=5.0 tests=AWL,BAYES_50,RDNS_NONE,SPF_HELO_PASS,SPF_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 21 Nov 2013 16:12:54 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rALGClR6017673 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 21 Nov 2013 11:12:47 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rALGCjuP028296; Thu, 21 Nov 2013 11:12:46 -0500 Message-ID: <528E30FD.3010204@redhat.com> Date: Thu, 21 Nov 2013 16:40:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Tom Tromey CC: gdb-patches@sourceware.org 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) References: <1384375873-32160-1-git-send-email-tromey@redhat.com> <1384375873-32160-2-git-send-email-tromey@redhat.com> <52850730.1060109@redhat.com> <87d2lxpo1l.fsf@fleche.redhat.com> <528B7F15.7040605@redhat.com> <87vbzomm78.fsf@fleche.redhat.com> <528B8FF6.7000406@redhat.com> <87siusl10r.fsf@fleche.redhat.com> <528BB700.4000802@redhat.com> <87fvqskumd.fsf@fleche.redhat.com> <528CFEDE.1040505@redhat.com> <871u2aixbc.fsf@fleche.redhat.com> In-Reply-To: <871u2aixbc.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-11/txt/msg00644.txt.bz2 On 11/20/2013 09:21 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves 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 Tom Tromey * 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