From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15955 invoked by alias); 11 May 2006 22:24:36 -0000 Received: (qmail 15947 invoked by uid 22791); 11 May 2006 22:24:35 -0000 X-Spam-Check-By: sourceware.org Received: from intranet.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.6) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 11 May 2006 22:24:33 +0000 Received: (qmail 19688 invoked from network); 11 May 2006 22:24:32 -0000 Received: from unknown (HELO localhost) (jimb@127.0.0.2) by mail.codesourcery.com with ESMTPA; 11 May 2006 22:24:32 -0000 To: gdb-patches@sourceware.org Subject: Re: [RFC] Move the frame zero PC check earlier References: <20060510180312.GA12606@nevyn.them.org> From: Jim Blandy Date: Thu, 11 May 2006 22:24:00 -0000 In-Reply-To: <20060510180312.GA12606@nevyn.them.org> (Daniel Jacobowitz's message of "Wed, 10 May 2006 14:03:12 -0400") Message-ID: User-Agent: Gnus/5.11 (Gnus v5.11) Emacs/22.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-05/txt/msg00224.txt.bz2 It looks good to me. I'm just curious why we even bother looking up the frame type for the older frame whose PC is zero. (I understand this test is inherited from the existing code.) Daniel Jacobowitz writes: > In this patch: > > http://sourceware.org/ml/gdb-patches/2004-12/msg00328.html > > Andrew added a generic check for two "normal" frames in a row where the > older one has a saved PC of zero. This is pretty well understood as a > convention for terminating backtraces - either intentionally or > unintentionally. > > The problem is, given where that check takes place, it is in my opinion one > frame off from the actual problem. You get backtraces that look like this: > > (gdb) bt > #0 catcher (sig=11) at /space/fsf/commit/src/gdb/testsuite/gdb.base/savedregs.c:43 > #1 0x00002ac148ec6e90 in killpg () from /lib/libc.so.6 > #2 0x0000000000000000 in ?? () > > On the one hand, that third frame is a nice marker for this case in that it > explains noisily that GDB is confused. In this case, if I point GDB at > .debug_frame data for my C library (conveniently found by default in > /usr/lib/debug) it successfully backtraces through to main, so that's good. > > On the other hand, that third frame is ugly and generally useless. The > attached patch moves the check one frame earlier, so that we only get this > backtrace: > > #0 catcher (sig=11) at /space/fsf/commit/src/gdb/testsuite/gdb.base/savedregs.c:43 > #1 0x00002ac148ec6e90 in killpg () from /lib/libc.so.6 > > Benefits: cleaner looking backtraces; the check is only done once per frame > instead of on every call to get_prev_frame. Disadvantages: for all frames > at level > 0, it causes this check to be done when we look at THIS frame > instead of when we unwind to the PREV frame, which forces us to locate the > correct sniffer earlier. If you have a sigtramp sniffer which reads memory, > this might cause an extra memory read. However, it won't happen in the > critical stepping path - we don't need this check when "unwinding frame 0 > from the sentinel frame - so I think this is an acceptable tradeoff. For > any frame other than the top frame, the thing you're most likely to do with > it is backtrace right through it. > > Tested on x86_64-pc-linux-gnu, and by hand against SymbianOS, where it gives > much nicer looking backtraces. > > Any comments? > > -- > Daniel Jacobowitz > CodeSourcery > > 2006-05-10 Daniel Jacobowitz > > * frame.c (get_prev_frame): Move check for pc == 0 ... > (get_prev_frame_1): ... to here. > > Index: frame.c > =================================================================== > RCS file: /cvs/src/src/gdb/frame.c,v > retrieving revision 1.211 > diff -u -p -r1.211 frame.c > --- frame.c 17 Dec 2005 22:33:59 -0000 1.211 > +++ frame.c 10 May 2006 17:48:32 -0000 > @@ -1123,6 +1123,26 @@ get_prev_frame_1 (struct frame_info *thi > this_frame->prev = prev_frame; > prev_frame->next = this_frame; > > + /* Now that the frame chain is in a consistant state, check whether > + this frame is useful. If it is not, unlink it. Its storage will > + be reclaimed the next time the frame cache is flushed, and we > + will not try to unwind THIS_FRAME again. */ > + > + /* Assume that the only way to get a zero PC is through something > + like a SIGSEGV or a dummy frame, and hence that NORMAL frames > + will never unwind a zero PC. This will look up the unwinder > + for the newly created frame, to determine its type. */ > + if (prev_frame->level > 0 > + && get_frame_type (prev_frame) == NORMAL_FRAME > + && get_frame_type (this_frame) == NORMAL_FRAME > + && get_frame_pc (prev_frame) == 0) > + { > + if (frame_debug) > + fprintf_unfiltered (gdb_stdlog, "-> // zero PC}\n"); > + this_frame->prev = NULL; > + return NULL; > + } > + > if (frame_debug) > { > fprintf_unfiltered (gdb_stdlog, "-> "); > @@ -1300,18 +1320,6 @@ get_prev_frame (struct frame_info *this_ > return NULL; > } > > - /* Assume that the only way to get a zero PC is through something > - like a SIGSEGV or a dummy frame, and hence that NORMAL frames > - will never unwind a zero PC. */ > - if (this_frame->level > 0 > - && get_frame_type (this_frame) == NORMAL_FRAME > - && get_frame_type (get_next_frame (this_frame)) == NORMAL_FRAME > - && get_frame_pc (this_frame) == 0) > - { > - frame_debug_got_null_frame (gdb_stdlog, this_frame, "zero PC"); > - return NULL; > - } > - > return get_prev_frame_1 (this_frame); > } >