From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45101 invoked by alias); 10 Feb 2016 15:34:16 -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 45089 invoked by uid 89); 10 Feb 2016 15:34:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=dig, Hx-languages-length:2760, xxx 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; Wed, 10 Feb 2016 15:34:13 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id BC71C70032; Wed, 10 Feb 2016 15:34:11 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1AFYA6K008446; Wed, 10 Feb 2016 10:34:11 -0500 Message-ID: <56BB5872.2000604@redhat.com> Date: Wed, 10 Feb 2016 15:34: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: "Metzger, Markus T" 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> <56B9D620.2020104@redhat.com> <56BA61C6.8060807@redhat.com> <56BB0A0D.80502@redhat.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-02/txt/msg00299.txt.bz2 On 02/10/2016 03:02 PM, Metzger, Markus T wrote: > No new fails there, as well (64-bit IA). > > I added a comment based on your statement that frame_unwind_caller_xxx > callers should check frame_unwind_caller_id and assert that skip_artificial_frames > does not return NULL. > > Info frame doesn't crash. > > (gdb) info frame > Stack level 0, frame at 0x0: > rip = 0x4005b0 in bar (tailcall-only.c:29); saved rip = 0x4005c2 > called by frame at 0x0 ^^^^^^^^^^^^^^^ > source language c. > Arglist at unknown address. > Locals at unknown address,Registers are not available in btrace record history > > This is from a tailcall-only frame stack in replay mode using the tailcall-only test. > The real caller has not been recorded. Not sure how you got that, since "called by frame" seems to indicates that the frame was not TAILCALL_FRAME: else if (get_frame_type (fi) == TAILCALL_FRAME) puts_filtered (" tail call frame"); else if (get_frame_type (fi) == INLINE_FRAME) printf_filtered (" inlined into frame %d", frame_relative_level (get_prev_frame (fi))); else { printf_filtered (" called by frame at "); fputs_filtered (paddress (gdbarch, get_frame_base (calling_frame_info)), gdb_stdout); } > > The output isn't very helpful for record btrace since we don't record register and > memory changes. So I'm mostly OK with the patch now, but I think you should dig a bit more into the "info frame" output, since I think you _will_ internal error with a TAILCALL_FRAME. My remaining issue is now with the user-visible strings. > @@ -985,6 +1007,10 @@ frame_pop (struct frame_info *this_frame) > entering THISFRAME. */ > prev_frame = skip_tailcall_frames (prev_frame); > > + /* We cannot pop tailcall frames. */ > + if (prev_frame == NULL) > + error (_("Cannot pop a tailcall frame.")); > + I find this confusing, from a user perspective. AFAIK, you can pop a tailcall frame; what you can't do is pop when you don't know anything about the frame that started the tail calling. How about: if (prev_frame == NULL) error (_("Cannot return: tailcall caller frame not found.")); s/pop/return/, as "pop" is an internal implementation detail. (I suggest also dropping the redundant comment.) > + { > + /* Ignore TAILCALL_FRAME type frames, they were executed already before > + entering THISFRAME. */ > + frame = skip_tailcall_frames (frame); > + > + if (frame == NULL) > + error (_("\"finish\" not meaningful for tailcall frames.")); > + if (frame == NULL) error (_("Cannot finish: tailcall caller frame not found.")); (I'd also be fine with dropping the "Cannot xxx:" part to make the error messages the same in both cases.) > + finish_forward (sm, frame); > + } > } Thanks, Pedro Alves