From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 88493 invoked by alias); 25 Apr 2016 08:40:42 -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 88477 invoked by uid 89); 25 Apr 2016 08:40:41 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=Personally, HX-Received:Mon, 6097 X-HELO: mail-pf0-f182.google.com Received: from mail-pf0-f182.google.com (HELO mail-pf0-f182.google.com) (209.85.192.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Mon, 25 Apr 2016 08:40:28 +0000 Received: by mail-pf0-f182.google.com with SMTP id c189so19098351pfb.3 for ; Mon, 25 Apr 2016 01:40:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=m09YfmwSlITw3bzY7uaO/j4UD9k4KZnk5EM8RzkFXXs=; b=lg6LDv0kJXINPygtsg+JsFhK8Znv9cxEWH1LSH1BVGLrO7Z+opwVjG/aAktX+ruTtd aI3gkupiI2Q1kkwLWM8pUxxvEAG9b+9JiKkBB0hIUuZJHjt2xWfthiE4uXWQSxh+uR8s ZKGIAOVbBSIRAHxHPaIBRKau6/o3DaZgXWhNS7LiDIPzanMqmt2B/uve07O5z35wavS5 KNEut9XXu8CbazIfRSt5XjatIx3PzqQHjFdxlzPcfPj3kYPCgMESUDTSA8KzQbC0Ufpa J96sseFrQgmm1I1ziEBQZysYaiyWQxnXXckoj/fM5MzDk3Tu1Hj58guddFVa4yJek6dE YFiw== X-Gm-Message-State: AOPr4FXo+8USHpu3GtzdMSJI1sOQV2RZpi38d7HS15D7Am7SRev0VXJn/vY0QQvl5PvePA== X-Received: by 10.98.80.206 with SMTP id g75mr47044408pfj.127.1461573626598; Mon, 25 Apr 2016 01:40:26 -0700 (PDT) Received: from E107787-LIN (gcc1-power7.osuosl.org. [140.211.15.137]) by smtp.gmail.com with ESMTPSA id bk8sm27831776pac.3.2016.04.25.01.40.21 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 25 Apr 2016 01:40:25 -0700 (PDT) From: Yao Qi To: Pedro Alves Cc: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH 3/7] Force to insert software single step breakpoint 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> <571A52F9.6060201@redhat.com> Date: Mon, 25 Apr 2016 08:40:00 -0000 In-Reply-To: <571A52F9.6060201@redhat.com> (Pedro Alves's message of "Fri, 22 Apr 2016 17:36:09 +0100") Message-ID: <868u02gjz3.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-04/txt/msg00542.txt.bz2 Pedro Alves writes: > type "brekapoint". > Fixed. >> 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). thread_is_stepping_over_breakpoint is used. > >> * infrun.h (thread_is_being_stepped_over_p): Declaration. >>=20 >> 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; >>=20=20 >> /* 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. */ > Done. >> /* See infrun.h. */ >> @@ -1365,6 +1371,15 @@ stepping_past_instruction_at (struct >> address_space *aspace, >> /* See infrun.h. */ >>=20=20 >> int >> +thread_is_being_stepped_over_p (int thread) >> +{ >> + return (step_over_info.aspace !=3D NULL >> + && thread =3D=3D step_over_info.thread); > > Wouldn't: > > return (step_over_info.thread !=3D -1 > && thread =3D=3D step_over_info.thread); > > be a bit more to the point? Using the aspace field makes me wonder wheth= er > we're caring for a case where step_over_info.thread is set to some > thread, but aspace is NULL. I thought about the case you mentioned, but don't know how can it happen. I check aspace in order to align with the validity check of step_over_info. Checking .thread against -1 is fine to me, as well. Patch below is what I pushed in. --=20 Yao (=E9=BD=90=E5=B0=A7) =46rom 21edc42f4e1ec6fe8cfce171232bab27ad4af372 Mon Sep 17 00:00:00 2001 From: Yao Qi Date: Mon, 25 Apr 2016 09:16:21 +0100 Subject: [PATCH] Force to insert software single step breakpoint GDB doesn't insert software single step breakpoint if the instruction branches to itself, so that the program can't stop after command "si". (gdb) b 32 Breakpoint 2 at 0x8680: file git/gdb/testsuite/gdb.base/branch-to-self.c, l= ine 32. (gdb) c Continuing. Breakpoint 2, main () at gdb/git/gdb/testsuite/gdb.base/branch-to-self.c:32 32 asm (".Lhere: " BRANCH_INSN " .Lhere"); /* loop-line */ (gdb) si infrun: clear_proceed_status_thread (Thread 3991.3991) infrun: proceed (addr=3D0xffffffff, signal=3DGDB_SIGNAL_DEFAULT) infrun: step-over queue now empty infrun: resuming [Thread 3991.3991] for step-over infrun: skipping breakpoint: stepping past insn at: 0x8680 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sending packet: $Z0,8678,4#f3...Packet received: OK infrun: skipping breakpoint: stepping past insn at: 0x8680 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Sending packet: $Z0,b6fe86c8,4#82...Packet received: OK infrun: resume (step=3D1, signal=3DGDB_SIGNAL_0), trap_expected=3D1, curren= t thread [Thread 3991.3991] at 0x868 breakpoint.c:should_be_inserted thinks the breakpoint shouldn't be inserted, which is wrong. This patch restrict the condition that only skip the non-single-step breakpoints if they are inserted at the place we are stepping over, however we don't want to skip single-step breakpoint if its thread is the thread we are stepping over, so in this patch, I add a thread num in 'struct step_over_info' to record the thread we're stepping over. gdb: 2016-04-25 Yao Qi * breakpoint.c (should_be_inserted): Return 0 if the location's owner is not single step breakpoint or single step breakpoint's thread isn't the thread which is stepping past a breakpoint. * 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_stepping_over_breakpoint): New function. * infrun.h (thread_is_stepping_over_breakpoint): Declaration. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 91db2e3..0b08605 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,16 @@ +2016-04-25 Yao Qi + + * breakpoint.c (should_be_inserted): Return 0 if the location's + owner is not single step breakpoint or single step breakpoint's + thread isn't the thread which is stepping past a breakpoint. + * 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_stepping_over_breakpoint): New function. + * infrun.h (thread_is_stepping_over_breakpoint): Declaration. + 2016-04-22 Edjunior Barbosa Machado =20 * ppc-linux-nat.c (ppc_linux_read_description): Use PPC_FEATURE_HAS_VSX diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index f99a7ab..a39a15c 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -2219,11 +2219,22 @@ should_be_inserted (struct bp_location *bl) return 0; =20 /* Don't insert a breakpoint if we're trying to step past its - location. */ + location, except if the breakpoint is a single-step breakpoint, + and the breakpoint's thread is the thread which is stepping past + a breakpoint. */ if ((bl->loc_type =3D=3D bp_loc_software_breakpoint || bl->loc_type =3D=3D bp_loc_hardware_breakpoint) && stepping_past_instruction_at (bl->pspace->aspace, - bl->address)) + bl->address) + /* The single-step breakpoint may be inserted at the location + we're trying to step if the instruction branches to itself. + However, the instruction won't be executed at all and it may + break the semantics of the instruction, for example, the + instruction is a conditional branch or updates some flags. + We can't fix it unless GDB is able to emulate the instruction + or switch to displaced stepping. */ + && !(bl->owner->type =3D=3D bp_single_step + && thread_is_stepping_over_breakpoint (bl->owner->thread))) { if (debug_infrun) { diff --git a/gdb/gdbarch.h b/gdb/gdbarch.h index 252fc4b..859ba85 100644 --- a/gdb/gdbarch.h +++ b/gdb/gdbarch.h @@ -650,7 +650,12 @@ extern void set_gdbarch_addr_bits_remove (struct gdbar= ch *gdbarch, gdbarch_addr_ target can single step. If not, then implement single step using break= points. =20=20=20 A return value of 1 means that the software_single_step breakpoints - were inserted; 0 means they were not. */ + were inserted; 0 means they were not. Multiple breakpoints may be + inserted for some instructions such as conditional branch. However, + each implementation must always evaluate the condition and only put + the breakpoint at the branch destination if the condition is true, so + that we ensure forward progress when stepping past a conditional + branch to self. */ =20 extern int gdbarch_software_single_step_p (struct gdbarch *gdbarch); =20 diff --git a/gdb/gdbarch.sh b/gdb/gdbarch.sh index 37f59b7..c8787c2 100755 --- a/gdb/gdbarch.sh +++ b/gdb/gdbarch.sh @@ -609,7 +609,12 @@ m:CORE_ADDR:addr_bits_remove:CORE_ADDR addr:addr::core= _addr_identity::0 # target can single step. If not, then implement single step using breakp= oints. # # A return value of 1 means that the software_single_step breakpoints -# were inserted; 0 means they were not. +# were inserted; 0 means they were not. Multiple breakpoints may be +# inserted for some instructions such as conditional branch. However, +# each implementation must always evaluate the condition and only put +# the breakpoint at the branch destination if the condition is true, so +# that we ensure forward progress when stepping past a conditional +# branch to self. F:int:software_single_step:struct frame_info *frame:frame =20 # Return non-zero if the processor is executing a delay slot and a diff --git a/gdb/infrun.c b/gdb/infrun.c index 696105d..cfb1d06 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -1296,6 +1296,9 @@ struct step_over_info /* The instruction being stepped over triggers a nonsteppable watchpoint. If true, we'll skip inserting watchpoints. */ int nonsteppable_watchpoint_p; + + /* The thread's global number. */ + int thread; }; =20 /* The step-over info of the location that is being stepped over. @@ -1329,11 +1332,13 @@ static struct step_over_info step_over_info; =20 static void set_step_over_info (struct address_space *aspace, CORE_ADDR address, - int nonsteppable_watchpoint_p) + int nonsteppable_watchpoint_p, + int thread) { step_over_info.aspace =3D aspace; step_over_info.address =3D address; step_over_info.nonsteppable_watchpoint_p =3D nonsteppable_watchpoint_p; + step_over_info.thread =3D thread; } =20 /* Called when we're not longer stepping over a breakpoint / an @@ -1348,6 +1353,7 @@ clear_step_over_info (void) step_over_info.aspace =3D NULL; step_over_info.address =3D 0; step_over_info.nonsteppable_watchpoint_p =3D 0; + step_over_info.thread =3D -1; } =20 /* See infrun.h. */ @@ -1365,6 +1371,15 @@ stepping_past_instruction_at (struct address_space *= aspace, /* See infrun.h. */ =20 int +thread_is_stepping_over_breakpoint (int thread) +{ + return (step_over_info.thread !=3D -1 + && thread =3D=3D step_over_info.thread); +} + +/* See infrun.h. */ + +int stepping_past_nonsteppable_watchpoint (void) { return step_over_info.nonsteppable_watchpoint_p; @@ -2579,7 +2594,7 @@ resume (enum gdb_signal sig) stop_all_threads (); =20 set_step_over_info (get_regcache_aspace (regcache), - regcache_read_pc (regcache), 0); + regcache_read_pc (regcache), 0, tp->global_num); =20 step =3D maybe_software_singlestep (gdbarch, pc); =20 @@ -7750,10 +7765,11 @@ keep_going_pass_signal (struct execution_control_st= ate *ecs) && (remove_wps || !use_displaced_stepping (ecs->event_thread))) { set_step_over_info (get_regcache_aspace (regcache), - regcache_read_pc (regcache), remove_wps); + regcache_read_pc (regcache), remove_wps, + ecs->event_thread->global_num); } else if (remove_wps) - set_step_over_info (NULL, 0, remove_wps); + set_step_over_info (NULL, 0, remove_wps, -1); =20 /* If we now need to do an in-line step-over, we need to stop all other threads. Note this must be done before diff --git a/gdb/infrun.h b/gdb/infrun.h index 61d3b20..e79bf2d 100644 --- a/gdb/infrun.h +++ b/gdb/infrun.h @@ -133,6 +133,10 @@ extern void insert_step_resume_breakpoint_at_sal (stru= ct gdbarch *, extern int stepping_past_instruction_at (struct address_space *aspace, CORE_ADDR address); =20 +/* Returns true if thread whose thread number is THREAD is stepping + over a breakpoint. */ +extern int thread_is_stepping_over_breakpoint (int thread); + /* Returns true if we're trying to step past an instruction that triggers a non-steppable watchpoint. */ extern int stepping_past_nonsteppable_watchpoint (void);