From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 72592 invoked by alias); 22 Apr 2016 16:36:22 -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 72575 invoked by uid 89); 22 Apr 2016 16:36:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= 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; Fri, 22 Apr 2016 16:36:11 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B01F6C04B307; Fri, 22 Apr 2016 16:36:10 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u3MGa9S3007978; Fri, 22 Apr 2016 12:36:10 -0400 Subject: Re: [PATCH 3/7] Force to insert software single step breakpoint To: Yao Qi References: <1458749384-19793-1-git-send-email-yao.qi@linaro.org> <1458749384-19793-4-git-send-email-yao.qi@linaro.org> <570BB52F.7@redhat.com> <861t60k9dl.fsf@gmail.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <571A52F9.6060201@redhat.com> Date: Fri, 22 Apr 2016 16:36:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 MIME-Version: 1.0 In-Reply-To: <861t60k9dl.fsf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-04/txt/msg00538.txt.bz2 Looks OK to me now. A couple nits below. On 04/20/2016 08:49 AM, Yao Qi wrote: > > 2016-04-20 Yao Qi > > * breakpoint.c (should_be_inserted): Return 0 if the location's > owner is not single step breakpoint or single step brekapoint's type "brekapoint". > owner isn't the thread we are stepping over. > * gdbarch.sh (software_single_step): Update comments. > * gdbarch.h: Regenerated. > * infrun.c (struct step_over_info) : New field. > (set_step_over_info): New argument 'thread'. Callers updated. > (clear_step_over_info): Set field thread to -1. > (thread_is_being_stepped_over_p): New function. We don't step over threads, but rather threads step over breakpoints. I'd suggest: thread_is_stepping_over_breakpoint_p (Personally. I don't see the need for a _p / predicate suffix when the function is clearly a predicate, due to use of the "is". thread_being_stepped_over_p / thread_is_being_stepped_over). > * infrun.h (thread_is_being_stepped_over_p): Declaration. > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index f99a7ab..64e97c6 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -2219,11 +2219,22 @@ should_be_inserted (struct bp_location *bl) > return 0; > > /* Don't insert a breakpoint if we're trying to step past its > - location. */ > + location except that the breakpoint is single-step breakpoint > + and the single-step breakpoint's owner is the thread we're > + stepping over. */ "breakpoint's owner" is kind of possible confusing with "bp location owner", which is itself a breakpoint. I'd find it clearer to copy&edit it to say: /* Don't insert a breakpoint if we're trying to step past its location, except if the breakpoint is a single-step breakpoint, and the breakpoint's thread is the thread that is stepping past a breakpoint. */ > /* See infrun.h. */ > @@ -1365,6 +1371,15 @@ stepping_past_instruction_at (struct address_space *aspace, > /* See infrun.h. */ > > int > +thread_is_being_stepped_over_p (int thread) > +{ > + return (step_over_info.aspace != NULL > + && thread == step_over_info.thread); Wouldn't: return (step_over_info.thread != -1 && thread == step_over_info.thread); be a bit more to the point? Using the aspace field makes me wonder whether we're caring for a case where step_over_info.thread is set to some thread, but aspace is NULL. Thanks, Pedro Alves