From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17542 invoked by alias); 6 Oct 2013 19:49:50 -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 17529 invoked by uid 89); 6 Oct 2013 19:49:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-4.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 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 ESMTP; Sun, 06 Oct 2013 19:49:49 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r96JnloT018487 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 6 Oct 2013 15:49:47 -0400 Received: from host2.jankratochvil.net (ovpn-116-31.ams2.redhat.com [10.36.116.31]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r96JnhSK023682 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Sun, 6 Oct 2013 15:49:46 -0400 Date: Sun, 06 Oct 2013 19:49:00 -0000 From: Jan Kratochvil To: Markus Metzger Cc: gdb-patches@sourceware.org Subject: Re: [patch v6 18/21] record-btrace: extend unwinder Message-ID: <20131006194943.GD28020@host2.jankratochvil.net> References: <1379676639-31802-1-git-send-email-markus.t.metzger@intel.com> <1379676639-31802-19-git-send-email-markus.t.metzger@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1379676639-31802-19-git-send-email-markus.t.metzger@intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2013-10/txt/msg00168.txt.bz2 On Fri, 20 Sep 2013 13:30:36 +0200, Markus Metzger wrote: [...] > --- a/gdb/frame.c > +++ b/gdb/frame.c [...] > @@ -886,7 +886,7 @@ frame_pop (struct frame_info *this_frame) > > /* Ignore TAILCALL_FRAME type frames, they were executed already before > entering THISFRAME. */ > - while (get_frame_type (prev_frame) == TAILCALL_FRAME) > + while (frame_is_tailcall (prev_frame)) Such kind of refactorization should be in a standalone patch. > prev_frame = get_prev_frame (prev_frame); > > /* Make a copy of all the register values unwound from this frame. > @@ -2126,9 +2126,9 @@ get_frame_address_in_block (struct frame_info *this_frame) > next_frame = next_frame->next; > > if ((get_frame_type (next_frame) == NORMAL_FRAME > - || get_frame_type (next_frame) == TAILCALL_FRAME) > + || frame_is_tailcall (next_frame)) > && (get_frame_type (this_frame) == NORMAL_FRAME > - || get_frame_type (this_frame) == TAILCALL_FRAME > + || frame_is_tailcall (this_frame) > || get_frame_type (this_frame) == INLINE_FRAME)) > return pc - 1; > > diff --git a/gdb/frame.h b/gdb/frame.h > index a5e1629..f0da19e 100644 > --- a/gdb/frame.h > +++ b/gdb/frame.h > @@ -216,7 +216,11 @@ enum frame_type > ARCH_FRAME, > /* Sentinel or registers frame. This frame obtains register values > direct from the inferior's registers. */ > - SENTINEL_FRAME > + SENTINEL_FRAME, > + /* A branch tracing frame. */ > + BTRACE_FRAME, > + /* A branch tracing tail call frame. */ > + BTRACE_TAILCALL_FRAME One should update also fprint_frame_type, otherwise: (gdb) set debug frame 1 { get_prev_frame_1 (this_frame=2) -> {level=3,type=,unwind=0x104aaa0,pc=0x40051f,id=,func=} // cached ^^^^^^^^^^^^^^ (I have fixed it now for TAILCALL_FRAME which I also forgot to add to fprint_frame_type.) > }; > > /* For every stopped thread, GDB tracks two frames: current and > @@ -773,4 +777,15 @@ extern struct frame_info *create_new_frame (CORE_ADDR base, CORE_ADDR pc); > extern int frame_unwinder_is (struct frame_info *fi, > const struct frame_unwind *unwinder); > > +/* Return non-zero if FRAME is a tailcall frame, return zero otherwise. */ > + > +static inline int > +frame_is_tailcall (struct frame_info *frame) > +{ > + enum frame_type type; > + > + type = get_frame_type (frame); > + return (type == TAILCALL_FRAME || type == BTRACE_TAILCALL_FRAME); Excessive parentheses. This function does not need to be / should not be inlined in .h file, this code is not a hot performance code path I think and moreover such optimizations should be left for gcc -flto. And I think when there are already two such unwinders there could be reather a new field in struct frame_unwind: struct frame_unwind { /* The frame's type. Should this instead be a collection of predicates that test the frame for various attributes? */ enum frame_type type; + /* Is this unwinder for tail calls? Call + return replaced by jump + at the end of a function is a tail call. */ + unsigned tail_call : 1; This unfortunately needs a simple change for all existing unwinders... > +} > + > #endif /* !defined (FRAME_H) */ [...] > --- a/gdb/record-btrace.c > +++ b/gdb/record-btrace.c [...] > @@ -923,7 +1033,28 @@ static void > record_btrace_frame_this_id (struct frame_info *this_frame, void **this_cache, > struct frame_id *this_id) > { > - /* Leave there the outer_frame_id value. */ > + const struct btrace_frame_cache *cache; > + const struct btrace_function *bfun; > + CORE_ADDR stack, code, special; > + > + cache = *this_cache; > + > + bfun = cache->bfun; > + gdb_assert (bfun != NULL); > + /* Get unique identifier of the function - frame independent on specific PC in the function. */ > + while (bfun->segment.prev != NULL) > + bfun = bfun->segment.prev; > + > + stack = 0; As discussed elsewhere I do not find correct to set stack_p = 1 && stack = 0. I think frame_id_p() can return true even if special_p == 1 but stack_p == 0. You sure need some new function similar to frame_id_build_special(). > + code = get_frame_func (this_frame); > + special = (CORE_ADDR) bfun; This is not safe, GDB host may be 64-bit and target 32-bit and in such case without --enable-64-bit-bfd there will be sizeof (CORE_ADDR) == 4, therefore different BFUNs may get the same SPECIAL. bfun->insn_offset or bfun->insn_offset should serve better I think. Also there could be a comment like (it still applies if one uses any of bfun, bfun->insn_offset or bfun->insn_offset): /* The btrace_function structures can be rebuilt but only after inferior has run. In such case all btrace frame have been deleted and there remain no stale uses of BFUN addresses. */ I was trying to find countercase but I haven't found any so hopefully it will work. Clearly btrace_function * > + > + *this_id = frame_id_build_special (stack, code, special); > + > + DEBUG ("[frame] %s id: (!stack, pc=%s, special=%s)", > + btrace_get_bfun_name (cache->bfun), > + core_addr_to_string_nz (this_id->code_addr), > + core_addr_to_string_nz (this_id->special_addr)); > } > > /* Implement prev_register method for record_btrace_frame_unwind. */ Thanks, Jan