From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23550 invoked by alias); 1 Dec 2017 19:50:59 -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 23540 invoked by uid 89); 1 Dec 2017 19:50:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=sk:build_b, Careful, wash 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; Fri, 01 Dec 2017 19:50:57 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 2A265C0587CC for ; Fri, 1 Dec 2017 19:50:56 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5FF8A6031E; Fri, 1 Dec 2017 19:50:55 +0000 (UTC) Subject: Re: [PATCH 2/2] Report stop locations in inlined functions. To: Keith Seitz References: <20171030211841.15394-1-keiths@redhat.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Fri, 01 Dec 2017 19:50:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20171030211841.15394-1-keiths@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-12/txt/msg00032.txt.bz2 Hi Keith, On 10/30/2017 09:18 PM, Keith Seitz wrote: > On 10/27/2017 05:37 AM, Pedro Alves wrote: >> >> Can you send me these as full patches, or send me the >> patch they apply on top of too? I'll need to play with >> it a bit to understand it all better. > > Sure, here they are. The two are from my stgit sandbox so #2 applies on top > of #1. [That allowed me to easily flip between the two and maintain tests.] Finally managed to play a bit with this. > > First up is the more "complex" solution, which does /not/ move the call > to bpstat_stop_status, but instead uses (a new function) build_bpstat_chain. > This is used to get at the current breakpoint (if any) to pass to > skip_inline_frames. Yes, let's use this one. The "simpler" one is too ad hoc. > { > - skip_inline_frames (ecs->ptid); > + struct breakpoint *bpt = NULL; > + > + stop_chain = build_bpstat_chain (aspace, stop_pc, &ecs->ws); > + if (stop_chain != NULL) > + bpt = stop_chain->breakpoint_at; > + > + skip_inline_frames (ecs->ptid, bpt); I think you should pass down the stop_chain directly to skip_inline_frames. This is because a stop can be explained by more than one breakpoint (that's why it's a chain), and the user breakpoint may not be the head of the chain. > > /* Re-fetch current thread's frame in case that invalidated > the frame cache. */ > @@ -5945,7 +5952,7 @@ handle_signal_stop (struct execution_control_state *ecs) > handles this event. */ > ecs->event_thread->control.stop_bpstat > = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()), > - stop_pc, ecs->ptid, &ecs->ws); > + stop_pc, ecs->ptid, &ecs->ws, stop_chain); > > /* Following in case break condition called a > function. */ > diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c > index b6154cdcc5..f06ecf61a6 100644 > --- a/gdb/inline-frame.c > +++ b/gdb/inline-frame.c > @@ -301,7 +301,7 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block) > user steps into them. */ > > void > -skip_inline_frames (ptid_t ptid) > +skip_inline_frames (ptid_t ptid, struct breakpoint *bpt) > { > CORE_ADDR this_pc; > const struct block *frame_block, *cur_block; > @@ -327,7 +327,25 @@ skip_inline_frames (ptid_t ptid) > if (BLOCK_START (cur_block) == this_pc > || block_starting_point_at (this_pc, cur_block)) > { > - skip_count++; > + bool skip_this_frame = true; > + > + if (bpt != NULL > + && user_breakpoint_p (bpt) > + && breakpoint_address_is_meaningful (bpt)) > + { > + for (bp_location *loc = bpt->loc; loc != NULL; > + loc = loc->next) > + { > + if (loc->address == this_pc) > + { > + skip_this_frame = false; > + break; > + } > + } > + } So here you'd check each breakpoint in the bpstat chain, not just the head. Also, to look at the locations, you should look at bpstat->bp_location_at, not at the locations of the breakpoint, because some of the locations of the breakpoint may be disabled/not-inserted, for example. There's one bpstat entry for each _location_ that actually caused a stop, so checking bp_location_at directly saves one loop. (Though you'll add one loop back to walk the bpstat chain, so it's a wash). Careful to not bpstat->bp_location_at->owner though, see comments in breakpoint.h. (I think you could look at bpstat->bp_location_at->loc_type, match against bp_loc_software_breakpoint / bp_loc_hardware_breakpoint, instead of exposing breakpoint_address_is_meaningful.) > + > + if (skip_this_frame) > + skip_count++; > last_sym = BLOCK_FUNCTION (cur_block); Couldn't this break out of the outer loop if !skip_this_frame ? Like: if (!skip_this_frame) break; skip_count++; last_sym = BLOCK_FUNCTION (cur_block); ? Thanks, Pedro Alves