From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31680 invoked by alias); 6 Jun 2013 23:42:34 -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 31670 invoked by uid 89); 6 Jun 2013 23:42:34 -0000 X-Spam-SWARE-Status: No, score=-7.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Thu, 06 Jun 2013 23:42:33 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r56NgWIn031887 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 6 Jun 2013 19:42:32 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r56NgUOI004112; Thu, 6 Jun 2013 19:42:31 -0400 Message-ID: <51B11E66.70102@redhat.com> Date: Thu, 06 Jun 2013 23:42:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 MIME-Version: 1.0 To: Tom Tromey CC: gdb-patches@sourceware.org Subject: Re: RFC: fix PR backtrace/15558 References: <87li6nghhz.fsf@fleche.redhat.com> In-Reply-To: <87li6nghhz.fsf@fleche.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-06/txt/msg00134.txt.bz2 Hi Tom, On 06/06/2013 08:35 PM, Tom Tromey wrote: > PR backtrace/15558 concerns an assertion failure in gdb that can be > triggered by setting "backtrace limit" to a value that causes gdb to > stop unwinding in the middle of a series of inlined frames. In this > case, an assertion in inline_frame_this_id will trigger. > > The bug is essentially that get_prev_frame checks backtrace_limit. > However, this is not needed or desirable in most cases. E.g., the > Python API to unwinding should probably not be limited by this setting. > > This patch removes the check from get_prev_frame and introduces a new > checking function that is used when printing a stack trace. > > Pedro had suggested removing all the other checks from get_prev_frame. Yeah, I was suggesting that "internal" / non-user-facing code should not be using get_prev_frame, but get_prev_frame_1 instead, bypassing all the checks. (Or rather wondering why that isn't so). Strongly more so in an unwinder's innards. get_prev_frame uses need to be investigated on a case-by-case manner manner to decide the best course of action, IMO. Focusing on inline_frame_this_id first, the case in question for the bug: 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're building the frame id for the inline frame. I say the CLI "set backtrace limit/past-entry/past-main" settings should really have no bearing on this. 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 it 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 stack frame, then we wouldn't have an inline frame either, by design. It's just logically impossible. That's what the assertion catches: /* 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)); A note on that NOTE. It's stale; we have outer_frame_id to distinguish from null_frame_id now. (though that should really die.) Hard to imagine the main function being inlined (at least on c-like languages), but if any of the NULL returns in get_prev_frame hit here, you'll trip on this assertion again. It seems like the zero PC case at the bottom of get_prev_frame can trigger. I can't say I understand what exactly that is supposed to catch. 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_raw (this_frame); And note how the somewhat 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; } So conceptually, in this case, I think what makes most sense it to skip _all_ the checks in get_prev_frame* that might return NULL, as there should always be a prev frame for an inline frame. IOW, in this case, I believe we should be making inline_frame_this_id call get_prev_frame_1, or whatever it gets renamed to, or equivalent. -- Pedro Alves