From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25908 invoked by alias); 10 Feb 2016 10:29:33 -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 25894 invoked by uid 89); 10 Feb 2016 10:29:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=wednesday, Wednesday, motivated, Hx-languages-length:5509 X-HELO: mga03.intel.com Received: from mga03.intel.com (HELO mga03.intel.com) (134.134.136.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 10 Feb 2016 10:29:31 +0000 Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 10 Feb 2016 02:29:29 -0800 X-ExtLoop1: 1 Received: from irsmsx103.ger.corp.intel.com ([163.33.3.157]) by orsmga002.jf.intel.com with ESMTP; 10 Feb 2016 02:29:28 -0800 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX103.ger.corp.intel.com (163.33.3.157) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 10 Feb 2016 10:29:27 +0000 Received: from irsmsx104.ger.corp.intel.com ([169.254.5.210]) by irsmsx112.ger.corp.intel.com ([169.254.1.159]) with mapi id 14.03.0248.002; Wed, 10 Feb 2016 10:29:27 +0000 From: "Metzger, Markus T" To: Pedro Alves CC: "gdb-patches@sourceware.org" Subject: RE: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type Date: Wed, 10 Feb 2016 10:29:00 -0000 Message-ID: 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: <56BB0A0D.80502@redhat.com> Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00274.txt.bz2 > -----Original Message----- > From: Pedro Alves [mailto:palves@redhat.com] > Sent: Wednesday, February 10, 2016 11:00 AM > To: Metzger, Markus T > Cc: gdb-patches@sourceware.org > Subject: Re: [PATCH v2 3/3] btrace, frame: fix crash in get_frame_type > >>>>> CORE_ADDR frame_unwind_pc (struct frame_info *this_frame) { > >>>>> + if (this_frame =3D=3D 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. >=20 > Because they assume there is always a prev frame. >=20 > The case under discussion is different, it's about getting at the "real c= aller > frame". Something has to change. >=20 > > I'd rather not introduce new exceptions if not strictly necessary. > > Its callers may not be prepared to handle them. >=20 > 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. >=20 > > > > 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 change= s. >=20 > 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 fram= e" at > all. >=20 > > > > 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. >=20 > See above. That situation looks similar on the surface, but the distinct= ion 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. >=20 > 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: >=20 > if (frame_id_p (frame_unwind_caller_id (frame))) > { > scope_breakpoint > =3D create_internal_breakpoint (frame_unwind_caller_arch (frame), > frame_unwind_caller_pc (frame), > bp_watchpoint_scope, > &momentary_breakpoint_ops); >=20 > and indeed most are. Those that aren't, either should be, or are not bec= ause > they're called when we know the current frame is a non-artificial frame. >=20 > 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: >=20 > frame =3D skip_artificial_frames (frame); > if (frame_id_p (get_frame_id (frame))) > { > scope_breakpoint > =3D create_internal_breakpoint (get_frame_arch (frame), > get_frame_pc (frame), > bp_watchpoint_scope, > &momentary_breakpoint_ops); >=20 >=20 > 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 fra= me. > Throwing an error in that case is better than silently continuing with mo= cked > info. >=20 > 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? Likely nothing. The case of only-artificial-frames is not covered by the t= est suite. I'm not sure it is even possible during live debug (ignoring the di= scussion about get_prev_frame_always). The changes were not motivated by fails but by my attempt to make the calle= rs of skip_artificial_frames handle the now-possible NULL return gracefully. I ran the btrace suite including the new tests I added to cover the record = btrace tailcall-only-frames case and they pass. I'm running the full test suite n= ow. Regards, Markus. Intel Deutschland GmbH Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Christian Lamprechter Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928