From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 106648 invoked by alias); 11 May 2018 15:55:56 -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 106536 invoked by uid 89); 11 May 2018 15:55:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=quoted X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 11 May 2018 15:55:53 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1D658402335F for ; Fri, 11 May 2018 15:55:52 +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 7B45D2026DEF; Fri, 11 May 2018 15:55:51 +0000 (UTC) Subject: Re: [PATCH v2] Don't elide all inlined frames To: Keith Seitz , gdb-patches@sourceware.org References: <20180228203324.17579-1-keiths@redhat.com> From: Pedro Alves Message-ID: Date: Fri, 11 May 2018 16:53:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-05/txt/msg00304.txt.bz2 On 05/09/2018 08:17 PM, Keith Seitz wrote: > On 04/09/2018 05:25 AM, Pedro Alves wrote: > I'm sorry, I don't follow. The locations are gotten from `s' which is from the bpstat chain. Aside from the breakpoint's type (and whether it is a user breakpoint), the actual breakpoint is not used. You're right, sorry about that. > > However, the inner for-loop (over the location->next) here can/should be removed. I've changed that. > > Otherwise, I'm afraid I just don't understand what you're saying. You might have to hit me with the clue stick. Sorry. I was thinking of this comment: /* Breakpoint that caused the stop. This is nullified if the breakpoint ends up being deleted. See comments on `bp_location_at' above for why do we need this field instead of following the location's owner. */ struct breakpoint *breakpoint_at; and noticed that the code uses "s->breakpoint_at" and somehow misread it as still following the breakpoint's location list. >> The part about breakint out of the outer loop no longer >> applies as is, but, AFAICT, the current code is still letting the >> outer stop_chain loop continue iterating even 'if (!skip_this_frame)'? > > The quoted code above (yours) is exactly the code in the patch. Did I send the wrong patch? The point is that the code still continues iterating this middle loop unnecessarily: for (bpstat s = stop_chain; s != NULL; s = s->next) { I think the easiest and clearest way to address that is to move those loops to a separate function, like this: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >From be6bc201c84fcbe94783bdf08557658b824df0d0 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 11 May 2018 16:22:09 +0100 Subject: [PATCH] split --- gdb/inline-frame.c | 65 +++++++++++++++++++++++++++++------------------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c index 68467d04406..3e258423099 100644 --- a/gdb/inline-frame.c +++ b/gdb/inline-frame.c @@ -297,6 +297,35 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block) return 1; } +/* Loop over the stop chain and determine if execution stopped in an + inlined frame because of a user breakpoint. THIS_PC is the current + frame's PC. */ + +static bool +stopped_by_user_bp_inline_frame (CORE_ADDR this_pc, bpstat stop_chain) +{ + for (bpstat s = stop_chain; s != NULL; s = s->next) + { + struct breakpoint *bpt = s->breakpoint_at; + + if (bpt != NULL && user_breakpoint_p (bpt)) + { + for (bp_location *loc = s->bp_location_at; + loc != NULL; loc = loc->next) + { + enum bp_loc_type t = loc->loc_type; + + if (loc->address == this_pc + && (t == bp_loc_software_breakpoint + || t == bp_loc_hardware_breakpoint)) + return true; + } + } + } + + return false; +} + /* See inline-frame.h. */ void @@ -326,38 +355,14 @@ skip_inline_frames (ptid_t ptid, bpstat stop_chain) if (BLOCK_START (cur_block) == this_pc || block_starting_point_at (this_pc, cur_block)) { - bool skip_this_frame = true; - - /* Loop over the stop chain and determine if execution - stopped in an inlined frame because of a user breakpoint. - If so do not skip the inlined frame. */ - for (bpstat s = stop_chain; s != NULL; s = s->next) + /* Do not skip the inlined frame if execution + stopped in an inlined frame because of a user + breakpoint. */ + if (!stopped_by_user_bp_inline_frame (this_pc, stop_chain)) { - struct breakpoint *bpt = s->breakpoint_at; - - if (bpt != NULL && user_breakpoint_p (bpt)) - { - for (bp_location *loc = s->bp_location_at; - loc != NULL; loc = loc->next) - { - enum bp_loc_type t = loc->loc_type; - - if (loc->address == this_pc - && (t == bp_loc_software_breakpoint - || t == bp_loc_hardware_breakpoint)) - { - skip_this_frame = false; - break; - } - } - } + skip_count++; + last_sym = BLOCK_FUNCTION (cur_block); } - - if (!skip_this_frame) - break; - - skip_count++; - last_sym = BLOCK_FUNCTION (cur_block); } else break; -- 2.14.3 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > +# Insert breakpoints for all inline_func? and not_inline_func? and check > +# that we actually stop where we think we should. > + > +for {set i 1} {$i < 4} {incr i} { > + foreach inline {"not_inline" "inline"} { > + gdb_breakpoint "${inline}_func$i" message > + } > +} So that discussion above about "s->breakpoint_at" made me think that it'd be good to also test all this with "tbreak", in case this frame skipping code ends up moving around and ends up after the breakpoint is deleted. Could you add that to the test? I'm thinking that you'd basically add a new outer loop around the new tests, like: # Also test "tbreak" to make sure that we display the stop in # the inline frame even the breakpoint is immediately deleted. foreach_with_prefix cmd "break" "tbreak" { #restart #test } Thanks, Pedro Alves