From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33274 invoked by alias); 20 Apr 2016 07:50:02 -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 33195 invoked by uid 89); 20 Apr 2016 07:50:01 -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=sk:softwar, Hx-languages-length:9313, 6507 X-HELO: mail-pf0-f174.google.com Received: from mail-pf0-f174.google.com (HELO mail-pf0-f174.google.com) (209.85.192.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 20 Apr 2016 07:49:50 +0000 Received: by mail-pf0-f174.google.com with SMTP id y69so45221pfb.1 for ; Wed, 20 Apr 2016 00:49:50 -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=8hm1VPmm4cRRDzgLstp5ycyRmEsU3v1YgUmUMlenwvo=; b=GDX4wnac/r6xNzYHpuieK3YfrAJtnQ0arbdZpW8BboIjfN5S+HOpHSO/he0LCtmyGc qjS7YoGdFbHsFZIyP8Zl5voWP+KiMPoYkL9vkYisf64h/xOR/v6EyzGlmvzQFbDK+NFW o3JbPmQ3GjQ8OQBLMz/PM8eXGq4QwfYyw2xr9f7TYVNq1r5GR5rV26ec5ir5qp610T1x gEr9nL1X7TV+iuK8P1WA8dnDX7KTeXNwSl3nPjaZPKu1VgLg+JzVfPeUElbIW3DAKLFS 3vPUHXE0XST3CPhMwBasAsx11CPV/OgOrXxcd1Z4qP7Vt7K45fj0wcoBzmMNLAuJJC+q PdoQ== X-Gm-Message-State: AOPr4FVV2nKkuClrJrm94hMdRfmpR3bydkv24QX6ilXFUJS2ewawITwCmfNqFnlTODYp0w== X-Received: by 10.98.4.67 with SMTP id 64mr10236897pfe.126.1461138588760; Wed, 20 Apr 2016 00:49:48 -0700 (PDT) Received: from E107787-LIN (gcc1-power7.osuosl.org. [140.211.15.137]) by smtp.gmail.com with ESMTPSA id gp9sm13901767pac.8.2016.04.20.00.49.44 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Wed, 20 Apr 2016 00:49:47 -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> Date: Wed, 20 Apr 2016 07:50:00 -0000 In-Reply-To: <570BB52F.7@redhat.com> (Pedro Alves's message of "Mon, 11 Apr 2016 15:31:11 +0100") Message-ID: <861t60k9dl.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/msg00475.txt.bz2 Pedro Alves writes: > Sounds like we may need to look at the single-step breakpoint's thread=20 > id, and only insert it if it is for the thread that is going to be=20 > doing the step-over? We may need to record that in step_over_info and=20 > pass more info to stepping_past_instruction_at. The patch below does this, however, I don't pass more info to stepping_past_instruction_at, instead I add a new function thread_is_being_stepped_over_p. > >> --- a/gdb/gdbarch.sh >> +++ b/gdb/gdbarch.sh >> @@ -609,7 +609,10 @@ 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 >> breakpoints. >> # >> # 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. > > I'd add: > > (...) condition is true, so that we ensure forward progress when=20 > stepping past a conditional branch to self. > > This will help porters evaluate whether that's really necessary > for their ports. Done. --=20 Yao (=E9=BD=90=E5=B0=A7) 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-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 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. * 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; =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. */ 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_being_stepped_over_p (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..1ccd648 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_being_stepped_over_p (int thread) +{ + return (step_over_info.aspace !=3D NULL + && 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..5f98414 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 being + stepped over. */ +extern int thread_is_being_stepped_over_p (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);