From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32956 invoked by alias); 9 Feb 2016 12:05: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 32938 invoked by uid 89); 9 Feb 2016 12:05:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=drill, unwinder, scratch, zeros 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 09 Feb 2016 12:05:55 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 4B72F96F9; Tue, 9 Feb 2016 12:05:54 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u19C5q5f011915; Tue, 9 Feb 2016 07:05:53 -0500 Message-ID: <56B9D620.2020104@redhat.com> Date: Tue, 09 Feb 2016 12:05:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Markus Metzger CC: gdb-patches@sourceware.org Subject: Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type References: <1454681922-2228-1-git-send-email-markus.t.metzger@intel.com> <1454681922-2228-3-git-send-email-markus.t.metzger@intel.com> In-Reply-To: <1454681922-2228-3-git-send-email-markus.t.metzger@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-02/txt/msg00223.txt.bz2 On 02/05/2016 02:18 PM, Markus Metzger wrote: > diff --git a/gdb/frame.c b/gdb/frame.c > index 6ab8834..cea7003 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -420,7 +420,8 @@ fprint_frame (struct ui_file *file, struct frame_info *fi) > > /* Given FRAME, return the enclosing frame as found in real frames read-in from > inferior memory. Skip any previous frames which were made up by GDB. > - Return the original frame if no immediate previous frames exist. */ > + Return FRAME if FRAME is a non-artificial frame. > + Return NULL if FRAME is NULL or the start of an artificial-only chain. */ Missing double-space after period. But, most importantly, I'm not sure I like that this accepts a NULL frame. > > static struct frame_info * > skip_artificial_frames (struct frame_info *frame) > @@ -428,11 +429,13 @@ 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) > + sets a backtrace limit). > + > + Note that for record targets we may get a frame chain that consists > + of artificial frames only. */ > + while (frame != NULL && > + (get_frame_type (frame) == INLINE_FRAME > + || get_frame_type (frame) == TAILCALL_FRAME)) > frame = get_prev_frame_always (frame); && goes on the next line, but can we make these look like: static struct frame_info * skip_artificial_frames (struct frame_info *frame) { ... while (get_frame_type (frame) == INLINE_FRAME || get_frame_type (frame) == TAILCALL_FRAME) { frame = get_prev_frame_always (frame); if (frame == NULL) break; } return frame; } > @@ -511,11 +517,12 @@ frame_unwind_caller_id (struct frame_info *next_frame) > requests the frame ID of "main()"s caller. */ > > next_frame = skip_artificial_frames (next_frame); > - this_frame = get_prev_frame_always (next_frame); > - if (this_frame) > - return get_frame_id (skip_artificial_frames (this_frame)); > - else > + if (next_frame == NULL) > return null_frame_id; > + > + this_frame = get_prev_frame_always (next_frame); > + this_frame = skip_artificial_frames (this_frame); > + return get_frame_id (this_frame); > } > > const struct frame_id null_frame_id = { 0 }; /* All zeros. */ > @@ -795,6 +802,9 @@ frame_find_by_id (struct frame_id id) > static CORE_ADDR > frame_unwind_pc (struct frame_info *this_frame) > { > + if (this_frame == NULL) > + throw_error (NOT_AVAILABLE_ERROR, _("PC not available")); How can this happen? > + > if (this_frame->prev_pc.status == CC_UNKNOWN) > { > if (gdbarch_unwind_pc_p (frame_unwind_arch (this_frame))) > + > /* Make a copy of all the register values unwound from this frame. > Save them in a scratch buffer so that there isn't a race between > trying to extract the old values from the current regcache while > @@ -2575,7 +2589,20 @@ frame_unwind_arch (struct frame_info *next_frame) > struct gdbarch * > frame_unwind_caller_arch (struct frame_info *next_frame) > { > - return frame_unwind_arch (skip_artificial_frames (next_frame)); > + struct frame_info *caller; > + > + /* If the frame chain consists only of artificial frames, use NEXT_FRAME's. > + > + For tailcall frames, we (i.e. the DWARF frame unwinder) assume that they > + have the gdbarch of the bottom (callee) frame. If NEXT_FRAME is an > + artificial frame, as well, we should drill down to the bottom in > + frame_unwind_arch either directly via the tailcall unwinder or via a chain > + of get_frame_arch calls. */ > + caller = skip_artificial_frames (next_frame); > + if (caller == NULL) > + get_frame_arch (next_frame); > + > + return frame_unwind_arch (next_frame); Hmm, this looks odd. Is the get_frame_arch call there for side effects, or did you mean to make that "return get_frame_arch" ? > +# We can step > +gdb_test "record goto begin" ".*foo.*" > +gdb_test "stepi" ".*foo_1.*" > +gdb_test "step" ".*bar.*" > +gdb_test "stepi" ".*bar_1.*" Please watch out for duplicate test messages: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique Thanks, Pedro Alves