From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id qGrnN4y+Q2XfND0AWB0awg (envelope-from ) for ; Thu, 02 Nov 2023 11:21:48 -0400 Received: by simark.ca (Postfix, from userid 112) id E30181E11B; Thu, 2 Nov 2023 11:21:48 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id C80721E0D2 for ; Thu, 2 Nov 2023 11:21:46 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 479483858D28 for ; Thu, 2 Nov 2023 15:21:46 +0000 (GMT) Received: from cstnet.cn (smtp21.cstnet.cn [159.226.251.21]) by sourceware.org (Postfix) with ESMTPS id B15653858D28 for ; Thu, 2 Nov 2023 15:21:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B15653858D28 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=iscas.ac.cn Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=iscas.ac.cn ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B15653858D28 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=159.226.251.21 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698938494; cv=none; b=ol8cicKMj4P6tKUK/C0ctqvPZ3RMv/XR6tnujmJ2ip3N4iOc4zgg80w5LLA0U9btAdZq3hOUpycaMe3OreLSI/6r2bL+fYkLvAsRuT/XwE+owkC9TeBD9OEyrutugURAC1/8MfzrQhQafuy7WFjd6P4WZVSqMHNqrOowlXZ4rj4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698938494; c=relaxed/simple; bh=mT6s2On9gHKn8s4ElxM8UcBvLzBTmPd0hH7xdqjMjes=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=k3TlW/YDa2mtJ6VzNHjskM4seeTpSPaj6rPSFAgzgMLh/Vr2+Cj1Z0fSWdO8AjezBFCtdbxipMMqoWuZXvg4d3e4bAQdCITQR4hfBFeFuf7bxQJ5RvWRSfQxAvE+WVuwYN1FvfK8J9/1wk7h3D3i/9y17mcKJ2f5LiUwb66gDhM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [10.38.1.120] (unknown [113.120.165.216]) by APP-01 (Coremail) with SMTP id qwCowABHqCmyvUNlKGZDAg--.8624S2; Thu, 02 Nov 2023 23:18:11 +0800 (CST) Message-ID: <8bd610eb-2713-4f1a-b928-ad9fa3020dd0@iscas.ac.cn> Date: Thu, 2 Nov 2023 23:21:16 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] gdb: RISC-V: Refine lr/sc sequence support To: gdb-patches@sourceware.org Cc: aburgess@redhat.com, palmer@dabbelt.com, simon.marchi@polymtl.ca References: <20231102140334.40372-1-liuyang22@iscas.ac.cn> From: Yang Liu In-Reply-To: <20231102140334.40372-1-liuyang22@iscas.ac.cn> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CM-TRANSID: qwCowABHqCmyvUNlKGZDAg--.8624S2 X-Coremail-Antispam: 1UD129KBjvJXoW3uFyUtryfAw1DWr4rury5twb_yoWkCryrpr ZYkw48Ar4UJayfArn7JF4DAr1rtw45CFZ5Jr1vq3yvyasIqw4fWFyUKw1Sv3WkCF12vw17 Wa1akr4Dua1ay3JanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUUk2b7Iv0xC_Kw4lb4IE77IF4wAFF20E14v26r1j6r4UM7CY07I2 0VC2zVCF04k26cxKx2IYs7xG6rWj6s0DM7CIcVAFz4kK6r1j6r18M28lY4IEw2IIxxk0rw A2F7IY1VAKz4vEj48ve4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Jr0_JF4l84ACjcxK6xII jxv20xvEc7CjxVAFwI0_Jr0_Gr1l84ACjcxK6I8E87Iv67AKxVWxJr0_GcWl84ACjcxK6I 8E87Iv6xkF7I0E14v26rxl6s0DM2vYz4IE04k24VAvwVAKI4IrM2AIxVAIcxkEcVAq07x2 0xvEncxIr21l5I8CrVACY4xI64kE6c02F40Ex7xfMcIj6xIIjxv20xvE14v26r106r15Mc Ij6I8E87Iv67AKxVW8JVWxJwAm72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41l42xK 82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUJVWUGw C20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r126r1DMIIYrxkI7VAKI48J MIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r1j6r4UMI IF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv67AKxVWUJVW8JwCI42IY6I8E 87Iv6xkF7I0E14v26r4j6r4UJbIYCTnIWIevJa73UjIFyTuYvjxUgJDGDUUUU X-Originating-IP: [113.120.165.216] X-CM-SenderInfo: 5olx5tdqjsjq5lvft2wodfhubq/ X-Spam-Status: No, score=-13.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org On 2023/11/2 22:03, Yang Liu wrote: > Per RISC-V spec, the lr/sc sequence can consist of up to 16 instructions, and we > cannot insert breakpoints in the middle of this sequence. Before this, we only > detected a specific pattern (the most common one). This patch improves this part > and now supports more complex pattern detection. > > gdb/ChangeLog: > > * gdb/riscv-tdep.c (class riscv_insn): Add more needed opcode enums. > (riscv_insn::decode): Decode newly added opcodes. > (riscv_insn_is_non_cti_and_allowed_in_atomic_sequence): New. > (riscv_insn_is_direct_branch): New. > (riscv_next_pc_atomic_sequence): Removed. > (riscv_deal_with_atomic_sequence): Rename from riscv_next_pc_atomic_sequence. > (riscv_software_single_step): Adjust to use the renamed one. > > Signed-off-by: Yang Liu > --- > gdb/riscv-tdep.c | 266 ++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 226 insertions(+), 40 deletions(-) > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c > index 3a2891c2c92..faff1aab420 100644 > --- a/gdb/riscv-tdep.c > +++ b/gdb/riscv-tdep.c > @@ -1578,8 +1578,34 @@ class riscv_insn > BLTU, > BGEU, > /* These are needed for stepping over atomic sequences. */ > - LR, > - SC, > + SLTI, > + SLTIU, > + XORI, > + ORI, > + ANDI, > + SLLI, > + SLLIW, > + SRLI, > + SRLIW, > + SRAI, > + SRAIW, > + SUB, > + SUBW, > + SLL, > + SLLW, > + SLT, > + SLTU, > + XOR, > + SRL, > + SRLW, > + SRA, > + SRAW, > + OR, > + AND, > + LR_W, > + LR_D, > + SC_W, > + SC_D, > /* This instruction is used to do a syscall. */ > ECALL, > > @@ -1768,6 +1794,13 @@ class riscv_insn > m_imm.s = EXTRACT_CBTYPE_IMM (ival); > } > > + void decode_ca_type_insn (enum opcode opcode, ULONGEST ival) > + { > + m_opcode = opcode; > + m_rs1 = decode_register_index_short (ival, OP_SH_CRS1S); > + m_rs2 = decode_register_index_short (ival, OP_SH_CRS2S); > + } > + > /* Fetch instruction from target memory at ADDR, return the content of > the instruction, and update LEN with the instruction length. */ > static ULONGEST fetch_instruction (struct gdbarch *gdbarch, > @@ -1882,14 +1915,62 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc) > decode_b_type_insn (BLTU, ival); > else if (is_bgeu_insn (ival)) > decode_b_type_insn (BGEU, ival); > + else if (is_slti_insn(ival)) > + decode_i_type_insn (SLTI, ival); > + else if (is_sltiu_insn(ival)) > + decode_i_type_insn (SLTIU, ival); > + else if (is_xori_insn(ival)) > + decode_i_type_insn (XORI, ival); > + else if (is_ori_insn(ival)) > + decode_i_type_insn (ORI, ival); > + else if (is_andi_insn(ival)) > + decode_i_type_insn (ANDI, ival); > + else if (is_slli_insn(ival)) > + decode_i_type_insn (SLLI, ival); > + else if (is_slliw_insn(ival)) > + decode_i_type_insn (SLLIW, ival); > + else if (is_srli_insn(ival)) > + decode_i_type_insn (SRLI, ival); > + else if (is_srliw_insn(ival)) > + decode_i_type_insn (SRLIW, ival); > + else if (is_srai_insn(ival)) > + decode_i_type_insn (SRAI, ival); > + else if (is_sraiw_insn(ival)) > + decode_i_type_insn (SRAIW, ival); > + else if (is_sub_insn(ival)) > + decode_r_type_insn (SUB, ival); > + else if (is_subw_insn(ival)) > + decode_r_type_insn (SUBW, ival); > + else if (is_sll_insn(ival)) > + decode_r_type_insn (SLL, ival); > + else if (is_sllw_insn(ival)) > + decode_r_type_insn (SLLW, ival); > + else if (is_slt_insn(ival)) > + decode_r_type_insn (SLT, ival); > + else if (is_sltu_insn(ival)) > + decode_r_type_insn (SLTU, ival); > + else if (is_xor_insn(ival)) > + decode_r_type_insn (XOR, ival); > + else if (is_srl_insn(ival)) > + decode_r_type_insn (SRL, ival); > + else if (is_srlw_insn(ival)) > + decode_r_type_insn (SRLW, ival); > + else if (is_sra_insn(ival)) > + decode_r_type_insn (SRA, ival); > + else if (is_sraw_insn(ival)) > + decode_r_type_insn (SRAW, ival); > + else if (is_or_insn(ival)) > + decode_r_type_insn (OR, ival); > + else if (is_and_insn(ival)) > + decode_r_type_insn (AND, ival); > else if (is_lr_w_insn (ival)) > - decode_r_type_insn (LR, ival); > + decode_r_type_insn (LR_W, ival); > else if (is_lr_d_insn (ival)) > - decode_r_type_insn (LR, ival); > + decode_r_type_insn (LR_D, ival); > else if (is_sc_w_insn (ival)) > - decode_r_type_insn (SC, ival); > + decode_r_type_insn (SC_W, ival); > else if (is_sc_d_insn (ival)) > - decode_r_type_insn (SC, ival); > + decode_r_type_insn (SC_D, ival); > else if (is_ecall_insn (ival)) > decode_i_type_insn (ECALL, ival); > else if (is_ld_insn (ival)) > @@ -1944,6 +2025,24 @@ riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc) > m_rd = decode_register_index (ival, OP_SH_CRS1S); > m_imm.s = EXTRACT_CITYPE_LUI_IMM (ival); > } > + else if (is_c_srli_insn (ival)) > + decode_cb_type_insn (SRLI, ival); > + else if (is_c_srai_insn (ival)) > + decode_cb_type_insn (SRAI, ival); > + else if (is_c_andi_insn (ival)) > + decode_cb_type_insn (ANDI, ival); > + else if (is_c_sub_insn (ival)) > + decode_ca_type_insn (SUB, ival); > + else if (is_c_xor_insn (ival)) > + decode_ca_type_insn (XOR, ival); > + else if (is_c_or_insn (ival)) > + decode_ca_type_insn (OR, ival); > + else if (is_c_and_insn (ival)) > + decode_ca_type_insn (AND, ival); > + else if (is_c_subw_insn (ival)) > + decode_ca_type_insn (SUBW, ival); > + else if (is_c_addw_insn (ival)) > + decode_ca_type_insn (ADDW, ival); > else if (is_c_li_insn (ival)) > decode_ci_type_insn (LI, ival); > /* C_SD and C_FSW have the same opcode. C_SD is RV64 and RV128 only, > @@ -4404,51 +4503,138 @@ riscv_next_pc (struct regcache *regcache, CORE_ADDR pc) > return next_pc; > } > > +/* Return true if INSN is not a control transfer instruction and is allowed to > + appear in the middle of the lr/sc sequence. */ > + > +static bool > +riscv_insn_is_non_cti_and_allowed_in_atomic_sequence(const struct riscv_insn &insn) > +{ > + switch (insn.opcode ()) > + { > + case riscv_insn::LUI: > + case riscv_insn::AUIPC: > + case riscv_insn::ADDI: > + case riscv_insn::ADDIW: > + case riscv_insn::SLTI: > + case riscv_insn::SLTIU: > + case riscv_insn::XORI: > + case riscv_insn::ORI: > + case riscv_insn::ANDI: > + case riscv_insn::SLLI: > + case riscv_insn::SLLIW: > + case riscv_insn::SRLI: > + case riscv_insn::SRLIW: > + case riscv_insn::SRAI: > + case riscv_insn::ADD: > + case riscv_insn::ADDW: > + case riscv_insn::SRAIW: > + case riscv_insn::SUB: > + case riscv_insn::SUBW: > + case riscv_insn::SLL: > + case riscv_insn::SLLW: > + case riscv_insn::SLT: > + case riscv_insn::SLTU: > + case riscv_insn::XOR: > + case riscv_insn::SRL: > + case riscv_insn::SRLW: > + case riscv_insn::SRA: > + case riscv_insn::SRAW: > + case riscv_insn::OR: > + case riscv_insn::AND: > + return true; > + } > + > + return false; > +} > + > +/* Return true if INSN is a direct branch insctruction. */ > + > +static bool > +riscv_insn_is_direct_branch(const struct riscv_insn &insn) > +{ > + switch (insn.opcode ()) > + { > + case riscv_insn::BEQ: > + case riscv_insn::BNE: > + case riscv_insn::BLT: > + case riscv_insn::BGE: > + case riscv_insn::BLTU: > + case riscv_insn::BGEU: > + case riscv_insn::JAL: > + return true; > + } > + > + return false; > +} > + > /* We can't put a breakpoint in the middle of a lr/sc atomic sequence, so look > for the end of the sequence and put the breakpoint there. */ > > -static bool > -riscv_next_pc_atomic_sequence (struct regcache *regcache, CORE_ADDR pc, > - CORE_ADDR *next_pc) > +static std::vector > +riscv_deal_with_atomic_sequence (struct regcache *regcache, CORE_ADDR pc) > { > struct gdbarch *gdbarch = regcache->arch (); > struct riscv_insn insn; > - CORE_ADDR cur_step_pc = pc; > - CORE_ADDR last_addr = 0; > + CORE_ADDR cur_step_pc = pc, next_pc; > + std::vector next_pcs; > + const int atomic_sequence_length = 16; > + bool found_valid_atomic_sequence = false; > + enum riscv_insn::opcode lr_opcode; > > /* First instruction has to be a load reserved. */ > insn.decode (gdbarch, cur_step_pc); > - if (insn.opcode () != riscv_insn::LR) > - return false; > - cur_step_pc = cur_step_pc + insn.length (); > + lr_opcode = insn.opcode (); > + if (lr_opcode != riscv_insn::LR_D && lr_opcode != riscv_insn::LR_W) > + return {}; > > - /* Next instruction should be branch to exit. */ > - insn.decode (gdbarch, cur_step_pc); > - if (insn.opcode () != riscv_insn::BNE) > - return false; > - last_addr = cur_step_pc + insn.imm_signed (); > - cur_step_pc = cur_step_pc + insn.length (); > + /* A lr/sc sequence comprise at most 16 instructions placed sequentially in memory. */ > + for (int insn_count = 0; insn_count < atomic_sequence_length; ++insn_count) > + { > + cur_step_pc += insn.length (); > + insn.decode (gdbarch, cur_step_pc); > > - /* Next instruction should be store conditional. */ > - insn.decode (gdbarch, cur_step_pc); > - if (insn.opcode () != riscv_insn::SC) > - return false; > - cur_step_pc = cur_step_pc + insn.length (); > + /* The dynamic code executed between lr/sc can only contain instructions > + from the base I instruction set, excluding loads, stores, backward jumps, > + taken backward branches, JALR, FENCE, FENCE.I, and SYSTEM instructions. > + If the C extension is supported, then compressed forms of the aforementioned > + I instructions are also permitted. */ > + > + if (riscv_insn_is_non_cti_and_allowed_in_atomic_sequence (insn)) > + { > + continue; > + } > + /* Look for a conditional branch instruction, check if it's taken forward or not. */ > + else if (riscv_insn_is_direct_branch (insn)) > + { > + if (insn.imm_signed () > 0) > + next_pc = cur_step_pc + insn.imm_signed (); Oh I forgot to do a push_back here and my simple tests didn't catch this. I'll send a V2 patch soon. Sorry. Best regards, Yang > + else > + break; > + } > + /* Look for a paired SC instruction which closes the atomic sequence. */ > + else if ((insn.opcode () == riscv_insn::SC_D && lr_opcode == riscv_insn::LR_D) > + || (insn.opcode () == riscv_insn::SC_W && lr_opcode == riscv_insn::LR_W)) > + { > + found_valid_atomic_sequence = true; > + } > + else > + break; > + } > + > + if (!found_valid_atomic_sequence) > + return {}; > > /* Next instruction should be branch to start. */ > insn.decode (gdbarch, cur_step_pc); > if (insn.opcode () != riscv_insn::BNE) > - return false; > + return {}; > if (pc != (cur_step_pc + insn.imm_signed ())) > - return false; > - cur_step_pc = cur_step_pc + insn.length (); > - > - /* We should now be at the end of the sequence. */ > - if (cur_step_pc != last_addr) > - return false; > + return {}; > + cur_step_pc += insn.length (); > > - *next_pc = cur_step_pc; > - return true; > + next_pc = cur_step_pc; > + next_pcs.push_back (next_pc); > + return next_pcs; > } > > /* This is called just before we want to resume the inferior, if we want to > @@ -4458,14 +4644,14 @@ riscv_next_pc_atomic_sequence (struct regcache *regcache, CORE_ADDR pc, > std::vector > riscv_software_single_step (struct regcache *regcache) > { > - CORE_ADDR pc, next_pc; > - > - pc = regcache_read_pc (regcache); > + CORE_ADDR cur_pc = regcache_read_pc (regcache), next_pc; > + std::vector next_pcs > + = riscv_deal_with_atomic_sequence (regcache, cur_pc); > > - if (riscv_next_pc_atomic_sequence (regcache, pc, &next_pc)) > - return {next_pc}; > + if (!next_pcs.empty ()) > + return next_pcs; > > - next_pc = riscv_next_pc (regcache, pc); > + next_pc = riscv_next_pc (regcache, cur_pc); > > return {next_pc}; > }