From patchwork Mon Apr 14 23:14:37 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 551 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx22.g.dreamhost.com (mx2.sub5.homie.mail.dreamhost.com [208.113.200.128]) by wilcox.dreamhost.com (Postfix) with ESMTP id 2CE7D360072 for ; Mon, 14 Apr 2014 16:14:49 -0700 (PDT) Received: by homiemail-mx22.g.dreamhost.com (Postfix, from userid 14314964) id D4988497A942; Mon, 14 Apr 2014 16:14:48 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx22.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx22.g.dreamhost.com (Postfix) with ESMTPS id AEA8C4965975 for ; Mon, 14 Apr 2014 16:14:48 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=LSwaPMlCOfg41FUh 71mlVhzkEvT2c5cF2Hry/kTlG3SjoU3L+4naMNWI0g888vcwt5TlENWpoXkKtD4q 1GIJKFfKKbjdyf4ycqAQYQIt/7Fq4ajcIQKonlOzHkQsZV5mHFwibQh6xLrAaJCs bygC9o/+gWTGF60c2DtXjsKqgCM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:date:from:mime-version:to:cc :subject:references:in-reply-to:content-type :content-transfer-encoding; s=default; bh=ksiFFrFbNQCnQIsJJmoYWA o+ujk=; b=LeqFPjv43lkq7WHil/0b4m5h+OHN67YBLs1SJRVNyFoEJ8+dal9xrw bTzFnwPBnIB4If/gutF0i/BDge4sUwqgMUiOD6i6xdlcQZQywkC1zBjQfLeKei3l 03y0pcTUF5F/SxwdMO3A7Ms56XA9z2e24R9Cb15G2rl9h9PK7cN0U= Received: (qmail 15863 invoked by alias); 14 Apr 2014 23:14:46 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 15850 invoked by uid 89); 14 Apr 2014 23:14:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 14 Apr 2014 23:14:42 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s3ENEd0u023823 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 14 Apr 2014 19:14:39 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s3ENEb0W017773; Mon, 14 Apr 2014 19:14:38 -0400 Message-ID: <534C6BDD.2090805@redhat.com> Date: Tue, 15 Apr 2014 00:14:37 +0100 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: Andrew Pinski , "gdb-patches@sourceware.org" Subject: Re: RFC: fix PR backtrace/15558 References: <87li6nghhz.fsf@fleche.redhat.com> <51B11E66.70102@redhat.com> <87mwqjw613.fsf@fleche.redhat.com> <87ha5vrdki.fsf@fleche.redhat.com> <534C2C19.1010503@redhat.com> <87tx9vpwvb.fsf@fleche.redhat.com> In-Reply-To: <87tx9vpwvb.fsf@fleche.redhat.com> X-DH-Original-To: gdb@patchwork.siddhesh.in On 04/14/2014 07:52 PM, Tom Tromey wrote: > Tom> I never got back to fixing it according to Pedro's review. > > Tom> As I recall it isn't entirely trivial because for a good fix one would > Tom> need to expose some new Python methods for use by the frame filter code. > > Pedro> Hmm. Not sure what you're thinking of. > > There's a good chance I'm confusing it with some other approach I'd > considered. > > Pedro> We discussed auditing all > Pedro> get_prev_frame uses and see if they are better replaced by > Pedro> get_prev_frame_1, but I don't think that should hold back > Pedro> fixing the inline frame unwinder. > > Ok. See patch below. I've simplified your test a bit, and added a test that doesn't depend on Python, to the existing gdb.opt/inline-bt.exp. > I think what I was thinking is that if one starts this change, then the > question arises: which should the Python unwinder call? Calling the > user-facing one is plainly incorrect. Not sure. I guess Python code might want to be implementing some new frame-related CLI command, where the limits make sense. Maybe Python should really have access to both variants, somehow? > But, calling the internal one is > also incorrect, as some termination conditions may be skipped. > > Perhaps this only applies if we move all the checks into get_prev_frame_1. > I don't recall whether that was part of the plan or not. Well, the plan didn't go as far as prescribe what to do exactly, only auditing/thinking through this. :-) The only termination condition in get_prev_frame that is a not user setting is the "zero PC" one, and it's arguable whether that one should be there or in the internal one. ------------- [PATCH] Fix PR backtrace/15558 This PR is about an assertion failure in GDB that can be triggered by setting "backtrace limit" to a value that causes GDB to stop unwinding after an inline frame. In this case, an assertion in inline_frame_this_id will trigger: /* We need a valid frame ID, so we need to be based on a valid frame. FSF submission NOTE: this would be a good assertion to apply to all frames, all the time. That would fix the ambiguity of null_frame_id (between "no/any frame" and "the outermost frame"). This will take work. */ gdb_assert (frame_id_p (*this_id)); Looking at the function: static void inline_frame_this_id (struct frame_info *this_frame, void **this_cache, struct frame_id *this_id) { struct symbol *func; /* In order to have a stable frame ID for a given inline function, we must get the stack / special addresses from the underlying real frame's this_id method. So we must call get_prev_frame. Because we are inlined into some function, there must be previous frames, so this is safe - as long as we're careful not to create any cycles. */ *this_id = get_frame_id (get_prev_frame (this_frame)); we see we're computing the frame id for the inline frame. If this is an inline frame, which is a virtual frame constructed based on debug info, on top of a real stack frame, we should _always_ be able to find where the frame was inlined into, as that ultimately just means peeling off the virtual frames on top of the real stack frame. If there ultimately was no prev (real) stack frame, then we wouldn't have been able to construct the inline frame either, by design. That's what the assertion catches. So we have an inline frame, we should _always_ be able to compute its ID, even if that means bypassing the user backtrace limits to get at the real stack frame's info. The problem is that inline_frame_id calls get_prev_frame, and that takes user backtrace limits into account. Code that wants to bypass the limits calls get_prev_frame_1 instead. (A note on that NOTE above. It's stale: we have outer_frame_id to distinguish from null_frame_id now.) Note how get_prev_frame_1 already skips all checks for inline frames: /* If we are unwinding from an inline frame, all of the below tests were already performed when we unwound from the next non-inline frame. We must skip them, since we can not get THIS_FRAME's ID 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_if_no_cycle (this_frame); And note how the related frame_unwind_caller_id function also uses get_prev_frame_1: struct frame_id frame_unwind_caller_id (struct frame_info *next_frame) { struct frame_info *this_frame; /* Use get_prev_frame_1, and not get_prev_frame. The latter will truncate the frame chain, leading to this function unintentionally returning a null_frame_id (e.g., when a caller requests the frame ID of "main()"s caller. */ next_frame = skip_artificial_frames (next_frame); this_frame = get_prev_frame_1 (next_frame); if (this_frame) return get_frame_id (skip_artificial_frames (this_frame)); else return null_frame_id; } get_prev_frame_1 is currently static in frame.c. As a _1 suffix is not a good name for an extern function, I've renamed it. Tested on x86-64 Fedora 17. gdb/ 2014-04-14 Pedro alves Tom Tromey PR backtrace/15558 * frame.c (get_prev_frame_1): Rename to ... (get_prev_frame_always): ... this, and make extern. Adjust. (skip_artificial_frames): Use get_prev_frame_always. (frame_unwind_caller_id, frame_pop, get_prev_frame) (get_frame_unwind_stop_reason): Adjust to rename. * frame.h (get_prev_frame_always): Declare. * inline-frame.c: Include frame.h. (inline_frame_this_id): Use get_prev_frame_always. gdb/testsuite/ 2014-04-14 Tom Tromey Pedro alves PR backtrace/15558 * gdb.opt/inline-bt.exp: Test backtracing from an inline function with a backtrace limit. * py-frame-inline.exp: Test running to an inline function with a bactrace limit, and printing the newest frame. --- gdb/frame.c | 31 ++++++++++++++++------------ gdb/frame.h | 7 +++++++ gdb/inline-frame.c | 16 +++++++------- gdb/testsuite/gdb.opt/inline-bt.exp | 16 ++++++++++++++ gdb/testsuite/gdb.python/py-frame-inline.c | 4 +++- gdb/testsuite/gdb.python/py-frame-inline.exp | 14 +++++++++++++ 6 files changed, 65 insertions(+), 23 deletions(-) diff --git a/gdb/frame.c b/gdb/frame.c index 97d54e9..013d602 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -46,7 +46,6 @@ #include "hashtab.h" #include "valprint.h" -static struct frame_info *get_prev_frame_1 (struct frame_info *this_frame); static struct frame_info *get_prev_frame_raw (struct frame_info *this_frame); static const char *frame_stop_reason_symbol_string (enum unwind_stop_reason reason); @@ -425,9 +424,15 @@ fprint_frame (struct ui_file *file, struct frame_info *fi) static struct frame_info * skip_artificial_frames (struct frame_info *frame) { + /* Note we use get_prev_frame_always, and not get_prev_frame. The + latter will truncate the frame chain, leading to this function + unintentionally returning a null_frame_id (e.g., when the user + sets a backtrace limit). This is safe, because as these frames + are made up by GDB, there must be a real frame in the chain + below. */ while (get_frame_type (frame) == INLINE_FRAME || get_frame_type (frame) == TAILCALL_FRAME) - frame = get_prev_frame (frame); + frame = get_prev_frame_always (frame); return frame; } @@ -484,13 +489,13 @@ frame_unwind_caller_id (struct frame_info *next_frame) { struct frame_info *this_frame; - /* Use get_prev_frame_1, and not get_prev_frame. The latter will truncate - the frame chain, leading to this function unintentionally - returning a null_frame_id (e.g., when a caller requests the frame - ID of "main()"s caller. */ + /* Use get_prev_frame_always, and not get_prev_frame. The latter + will truncate the frame chain, leading to this function + unintentionally returning a null_frame_id (e.g., when a caller + requests the frame ID of "main()"s caller. */ next_frame = skip_artificial_frames (next_frame); - this_frame = get_prev_frame_1 (next_frame); + this_frame = get_prev_frame_always (next_frame); if (this_frame) return get_frame_id (skip_artificial_frames (this_frame)); else @@ -956,7 +961,7 @@ frame_pop (struct frame_info *this_frame) } /* Ensure that we have a frame to pop to. */ - prev_frame = get_prev_frame_1 (this_frame); + prev_frame = get_prev_frame_always (this_frame); if (!prev_frame) error (_("Cannot pop the initial frame.")); @@ -1775,8 +1780,8 @@ get_prev_frame_if_no_cycle (struct frame_info *this_frame) Unlike get_prev_frame, this function always tries to unwind the frame. */ -static struct frame_info * -get_prev_frame_1 (struct frame_info *this_frame) +struct frame_info * +get_prev_frame_always (struct frame_info *this_frame) { struct gdbarch *gdbarch; @@ -1785,7 +1790,7 @@ get_prev_frame_1 (struct frame_info *this_frame) if (frame_debug) { - fprintf_unfiltered (gdb_stdlog, "{ get_prev_frame_1 (this_frame="); + fprintf_unfiltered (gdb_stdlog, "{ get_prev_frame_always (this_frame="); if (this_frame != NULL) fprintf_unfiltered (gdb_stdlog, "%d", this_frame->level); else @@ -2137,7 +2142,7 @@ get_prev_frame (struct frame_info *this_frame) return NULL; } - return get_prev_frame_1 (this_frame); + return get_prev_frame_always (this_frame); } CORE_ADDR @@ -2523,7 +2528,7 @@ enum unwind_stop_reason get_frame_unwind_stop_reason (struct frame_info *frame) { /* Fill-in STOP_REASON. */ - get_prev_frame_1 (frame); + get_prev_frame_always (frame); gdb_assert (frame->prev_p); return frame->stop_reason; diff --git a/gdb/frame.h b/gdb/frame.h index e451a93..b88bd28 100644 --- a/gdb/frame.h +++ b/gdb/frame.h @@ -307,6 +307,13 @@ extern void select_frame (struct frame_info *); extern struct frame_info *get_prev_frame (struct frame_info *); extern struct frame_info *get_next_frame (struct frame_info *); +/* Return a "struct frame_info" corresponding to the frame that called + THIS_FRAME. Returns NULL if there is no such frame. + + Unlike get_prev_frame, this function always tries to unwind the + frame. */ +extern struct frame_info *get_prev_frame_always (struct frame_info *); + /* Given a frame's ID, relocate the frame. Returns NULL if the frame is not found. */ extern struct frame_info *frame_find_by_id (struct frame_id id); diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index 05ba9ff..6255630 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -26,6 +26,7 @@ #include "regcache.h" #include "symtab.h" #include "vec.h" +#include "frame.h" #include "gdb_assert.h" @@ -154,17 +155,14 @@ inline_frame_this_id (struct frame_info *this_frame, /* In order to have a stable frame ID for a given inline function, we must get the stack / special addresses from the underlying - real frame's this_id method. So we must call get_prev_frame. - Because we are inlined into some function, there must be previous - frames, so this is safe - as long as we're careful not to - create any cycles. */ - *this_id = get_frame_id (get_prev_frame (this_frame)); + real frame's this_id method. So we must call + get_prev_frame_always. Because we are inlined into some + function, there must be previous frames, so this is safe - as + long as we're careful not to create any cycles. */ + *this_id = get_frame_id (get_prev_frame_always (this_frame)); /* We need a valid frame ID, so we need to be based on a valid - frame. FSF submission NOTE: this would be a good assertion to - apply to all frames, all the time. That would fix the ambiguity - of null_frame_id (between "no/any frame" and "the outermost - frame"). This will take work. */ + frame. */ gdb_assert (frame_id_p (*this_id)); /* For now, require we don't match outer_frame_id either (see diff --git a/gdb/testsuite/gdb.opt/inline-bt.exp b/gdb/testsuite/gdb.opt/inline-bt.exp index c437383..ce73623 100644 --- a/gdb/testsuite/gdb.opt/inline-bt.exp +++ b/gdb/testsuite/gdb.opt/inline-bt.exp @@ -50,3 +50,19 @@ gdb_test "up" "#1 .*func1.*" "up from bar (3)" gdb_test "info frame" ".*inlined into frame.*" "func1 inlined (3)" gdb_test "up" "#2 .*func2.*" "up from func1 (3)" gdb_test "info frame" ".*inlined into frame.*" "func2 inlined (3)" + +# A regression test for having a backtrace limit that forces unwinding +# to stop after an inline frame. GDB needs to compute the frame_id of +# the inline frame, which requires unwinding past all the inline +# frames to the real stack frame, even if that means bypassing the +# user visible backtrace limit. See PR backtrace/15558. +# +# Set a backtrace limit that forces an unwind stop after an inline +# function. +gdb_test_no_output "set backtrace limit 2" +# Force flushing the frame cache. +gdb_test "flushregs" "Register cache flushed." +gdb_test "up" "#1 .*func1.*" "up from bar (4)" +gdb_test "info frame" ".*in func1.*" "info frame still works" +# Verify the user visible limit works as expected. +gdb_test "up" "Initial frame selected; you cannot go up." "up hits limit" diff --git a/gdb/testsuite/gdb.python/py-frame-inline.c b/gdb/testsuite/gdb.python/py-frame-inline.c index a3669bc..f08e84b 100644 --- a/gdb/testsuite/gdb.python/py-frame-inline.c +++ b/gdb/testsuite/gdb.python/py-frame-inline.c @@ -39,5 +39,7 @@ g (void) int main (void) { - return g (); + int x = g (); + x += f (); + return x; } diff --git a/gdb/testsuite/gdb.python/py-frame-inline.exp b/gdb/testsuite/gdb.python/py-frame-inline.exp index f5cf33e..8851d87 100644 --- a/gdb/testsuite/gdb.python/py-frame-inline.exp +++ b/gdb/testsuite/gdb.python/py-frame-inline.exp @@ -37,3 +37,17 @@ gdb_test "info frame" "inlined into frame 1\r\n.*" gdb_test "up" "#1 g .*" gdb_test "python print (gdb.selected_frame().read_var('l'))" "\r\n42" + +# A regression test for having a backtrace limit that forces unwinding +# to stop after an inline frame. GDB needs to compute the frame_id of +# the inline frame, which requires unwinding past all the inline +# frames to the real stack frame, even if that means bypassing the +# user visible backtrace limit. See PR backtrace/15558. +# +# Set the limit, and run to an inline function. It's important that +# the frame cache is flushed somehow after setting the limit, to force +# frame id recomputation. +gdb_test_no_output "set backtrace limit 1" +gdb_continue_to_breakpoint "Block break here." + +gdb_test "python print (gdb.newest_frame())" ".*"