From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 70440 invoked by alias); 10 Feb 2016 09:59:46 -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 70416 invoked by uid 89); 10 Feb 2016 09:59:45 -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=unwind, Something, retrieve, she 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 09:59:44 +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 (Postfix) with ESMTPS id F3BCC8EA38; Wed, 10 Feb 2016 09:59:42 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1A9xfpB006368; Wed, 10 Feb 2016 04:59:42 -0500 Message-ID: <56BB0A0D.80502@redhat.com> Date: Wed, 10 Feb 2016 09:59: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> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-02/txt/msg00272.txt.bz2 On 02/10/2016 07:40 AM, Metzger, Markus T wrote: >> -----Original Message----- >> From: Pedro Alves [mailto:palves@redhat.com] >> Sent: Tuesday, February 9, 2016 11:02 PM >> To: Metzger, Markus T >> Cc: gdb-patches@sourceware.org >> Subject: Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type >> >> On 02/09/2016 02:42 PM, Metzger, Markus T wrote: >> >>>>> 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? >>> >>> One of its callers, frame_unwind_caller_pc, calls it with the result >>> of skip_artificial_frames like this: >>> >>> CORE_ADDR >>> frame_unwind_caller_pc (struct frame_info *this_frame) { >>> return frame_unwind_pc (skip_artificial_frames (this_frame)); } >>> >>> Rather than handling the skip_artificial_frames() NULL return here, I >>> made frame_unwind_pc handle a NULL frame argument. >>> >>> I can move the check into frame_unwind_caller_pc if you prefer. >> >> Yes, please. >> >> Though, I think all these frame_unwind_caller_XXX methods should be >> consistent in how they handle skip_artificial_frames (this_frame) returning >> NULL, because they're all called together, assuming they're referring to the >> same frame. If we throw error here, then I think we should throw in >> frame_unwind_caller_arch too, instead of having that one return the arch of >> the next frame. > > get_frame_arch and frame_unwind_arch don't seem to throw any error today. Because they assume there is always a prev frame. The case under discussion is different, it's about getting at the "real caller frame". Something has to change. > I'd rather not introduce new exceptions if not strictly necessary. Its callers may > not be prepared to handle them. Callers shouldn't be handed back potentially bogus results. If there's no return that makes sense, then throw and let the callers handle it (or don't handle it and let the user know what she tried to do doesn't make sense), or internal-error. > > In the absence of an arch unwinder, frame_unwind_arch uses the gdbarch of > the next frame. And DWARF tailcall frames use the gdbarch of the bottom > non-tailcall frame. This is consistent with the proposed changes. In that case, there _is_ a prev frame, and then we default to assuming the unwound arch is the same as the next frame's arch. But in this situation at hand, the difference is that we found that there's _no_ "real caller frame" at all. > > We may want to return frame_unwind_arch (next_frame) instead of > get_frame_arch (next_frame). OTOH gdb/dwarf2-frame-tailcall.c's > tailcall_frame_prev_arch returns get_frame_arch (cache->next_bottom_frame). > I'm currently mimicking that behavior. See above. That situation looks similar on the surface, but the distinction is important. The "frame_unwind_CALLER" methods want to drill down past artificial frames, and return info on the "real caller", while the "frame_unwind" methods want to unwind one frame only, no matter artificial or not. All the frame_unwind_caller_XXX methods should retrieve information about the same frame that frame_unwind_caller_id returns. They should all really be guarded by a frame_unwind_caller_id check, like: if (frame_id_p (frame_unwind_caller_id (frame))) { scope_breakpoint = create_internal_breakpoint (frame_unwind_caller_arch (frame), frame_unwind_caller_pc (frame), bp_watchpoint_scope, &momentary_breakpoint_ops); and indeed most are. Those that aren't, either should be, or are not because they're called when we know the current frame is a non-artificial frame. Put another way, all the frame_unwind_caller_xxx methods should behave merely as convenience to writing the alternative form that has the caller skip artificial frames itself: frame = skip_artificial_frames (frame); if (frame_id_p (get_frame_id (frame))) { scope_breakpoint = create_internal_breakpoint (get_frame_arch (frame), get_frame_pc (frame), bp_watchpoint_scope, &momentary_breakpoint_ops); As such, if something calls frame_unwind_caller_pc|arch directly without checking whether frame_unwind_caller_id returns a valid frame, then the caller code should not continue as if there was indeed a valid caller frame. Throwing an error in that case is better than silently continuing with mocked info. So, what would break if we internal_error'ed in frame_unwind_caller_arch/pc instead or normal error, in the case that there's no non-artificial frame? I'm thinking that maybe only "info frame" (frame_info) would trip on it, but then that one should probably check frame_unwind_caller_id itself too. Thanks, Pedro Alves