From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127421 invoked by alias); 3 Apr 2017 15:18:50 -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 127408 invoked by uid 89); 3 Apr 2017 15:18:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.4 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr0-f195.google.com Received: from mail-wr0-f195.google.com (HELO mail-wr0-f195.google.com) (209.85.128.195) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 03 Apr 2017 15:18:48 +0000 Received: by mail-wr0-f195.google.com with SMTP id w43so34453488wrb.1 for ; Mon, 03 Apr 2017 08:18:49 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=4CLpKF8OYjYAPJzypPN4S9N11PAcVyPbEV/wHi4kkgA=; b=dySi+R9GzQ+0/WZHCVvA6Jel+pNZNtDkHiIITheibWBgyocx81HSRXJ+rVRorIH6BN s6koH+AP3bzezyJAINFiUH/a7kKPSbdQVFRJMBI8rLrxCGpp7Olb3d/R2dt/ua1S1qg3 sNZ+zcHgse07d+fBTuaqoN4//kS0Y09q5b7PG1fo3zFAipZdNHB8jRAGiQVWCJarJ0Du fDwhswkLeIPz6nVzDPFTm+tcYtEv0RJ08gjR/F9WsR1P3fttEyoAZcbht2NpDlW2W6el kYeRpVB2pcPQpymL7MMoso1xd7AfMM9baBV/U/LzlWechHlRBoU9toWjIUF4XSx1kXn4 piSg== X-Gm-Message-State: AFeK/H21C2zU4B6SwxBMjtLaWUhAMXeinhzQ0xe1cVIF6UISjSoHOhY97t2GLOcUSN4FZg== X-Received: by 10.223.128.194 with SMTP id 60mr9565385wrl.175.1491232727344; Mon, 03 Apr 2017 08:18:47 -0700 (PDT) Received: from E107787-LIN ([194.214.185.158]) by smtp.gmail.com with ESMTPSA id m14sm4031749wrb.13.2017.04.03.08.18.44 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 03 Apr 2017 08:18:46 -0700 (PDT) From: Yao Qi To: Antoine Tremblay Cc: Pedro Alves , "gdb-patches\@sourceware.org" Subject: Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping References: <20161129120702.9490-1-antoine.tremblay@ericsson.com> <20170127150139.GB24676@E107787-LIN> <2255ed6f-a146-026c-f871-00e9a33dfcf0@redhat.com> <86d1cy4umo.fsf@gmail.com> <86d1cxwgpk.fsf@gmail.com> <86h925ll3f.fsf@gmail.com> Date: Mon, 03 Apr 2017 15:18:00 -0000 In-Reply-To: (Antoine Tremblay's message of "Mon, 3 Apr 2017 09:18:15 -0400") Message-ID: <8637dpldta.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: 2017-04/txt/msg00003.txt.bz2 Antoine Tremblay writes: > + if ((inst1 & 0xff00) =3D=3D 0xbf00 && (inst1 & 0x000f) !=3D 0) > + { > + /* An IT instruction. Because this instruction does not > + modify the flags, we can accurately predict the next > + executed instruction. */ > + itstate =3D inst1 & 0x00ff; > + pc +=3D thumb_insn_size (inst1); > + > + while (itstate !=3D 0 && ! condition_true (itstate >> 4, status)) > + { > + inst1 =3D read_mem_uint (pc, 2,byte_order_for_code); > + pc +=3D thumb_insn_size (inst1); > + itstate =3D thumb_advance_itstate (itstate); > + } > + next_pcs.push_back (std::pair > + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); It is incorrect to choose ARM_BP_KIND_THUMB2 if the instruction is 16-bit. IMO, this function should only tell whether PC is in IT block nor not. It shouldn't involve any breakpoint kinds selection. > + return next_pcs; > + } > + else if (itstate !=3D 0) > + { > + /* We are in a conditional block. Check the condition. */ > + if (! condition_true (itstate >> 4, status)) > + { > + /* Advance to the next executed instruction. */ > + pc +=3D thumb_insn_size (inst1); > + itstate =3D thumb_advance_itstate (itstate); > + > + while (itstate !=3D 0 && ! condition_true (itstate >> 4, status)) > + { > + inst1 =3D read_mem_uint (pc, 2, byte_order_for_code); > + > + pc +=3D thumb_insn_size (inst1); > + itstate =3D thumb_advance_itstate (itstate); > + } > + If all the following instructions' condition is false, breakpoint should be set on the first instruction out side of IT block. We can still use 16-bit thumb breakpoint. > + next_pcs.push_back (std::pair > + (MAKE_THUMB_ADDR (pc), > ARM_BP_KIND_THUMB2)); The same issue. > + return next_pcs; > + } > + else if ((itstate & 0x0f) =3D=3D 0x08) > + { > + /* This is the last instruction of the conditional > + block, and it is executed. We can handle it normally > + because the following instruction is not conditional, > + and we must handle it normally because it is > + permitted to branch. Fall through. */ How do we fall through now? > + } > + else > + { > + int cond_negated; > + > + /* There are conditional instructions after this one. > + If this instruction modifies the flags, then we can > + not predict what the next executed instruction will > + be. Fortunately, this instruction is archi2tecturally > + forbidden to branch; we know it will fall through. > + Start by skipping past it. */ > + pc +=3D thumb_insn_size (inst1); > + itstate =3D thumb_advance_itstate (itstate); > + > + /* Set a breakpoint on the following instruction. */ > + gdb_assert ((itstate & 0x0f) !=3D 0); > + next_pcs.push_back (std::pair > + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); > + > + cond_negated =3D (itstate >> 4) & 1; > + > + /* Skip all following instructions with the same > + condition. If there is a later instruction in the IT > + block with the opposite condition, set the other > + breakpoint there. If not, then set a breakpoint on > + the instruction after the IT block. */ > + do > + { > + inst1 =3D read_mem_uint (pc, 2, byte_order_for_code); > + pc +=3D thumb_insn_size (inst1); > + itstate =3D thumb_advance_itstate (itstate); > + } > + while (itstate !=3D 0 && ((itstate >> 4) & 1) =3D=3D cond_negated); > + > + if (itstate !=3D 0 && ((itstate >> 4) & 1) =3D=3D cond_negated) > + { > + next_pcs.push_back (std::pair > + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); > + } > + else > + { > + next_pcs.push_back (std::pair > + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB)); > + } Why do you choose breakpoint in this way? > diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c > index 6e6926a..f3845cf 100644 > --- a/gdb/gdbserver/mem-break.c > +++ b/gdb/gdbserver/mem-break.c > @@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_AD= DR where, > { > int err_ignored; > CORE_ADDR placed_address =3D where; > - int breakpoint_kind =3D target_breakpoint_kind_from_pc (&placed_addres= s); > + int breakpoint_kind; > + > + /* Get the kind of breakpoint to PLACED_ADDRESS except single-step > + breakpoint. Get the kind of single-step breakpoint according to > + the current register state. */ > + if (type =3D=3D single_step_breakpoint) > + { > + breakpoint_kind > + =3D target_breakpoint_kind_from_current_state (&placed_address); I read my patch again, but it looks wrong. If we single-step an instruction with a state change, like bx or blx, current get_next_pcs correctly marked the address bit. However, with the change like this, we'll get the wrong breakpoint kind. --=20 Yao (=E9=BD=90=E5=B0=A7)