From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cstnet.cn (smtp21.cstnet.cn [159.226.251.21]) by sourceware.org (Postfix) with ESMTPS id B3CF93858D32 for ; Thu, 2 Nov 2023 14:53:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B3CF93858D32 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 B3CF93858D32 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=1698936810; cv=none; b=hxHyz6LnZsKlYeriKHV7/mjlkAFKc3VCK3/akfdmrneqdqhnLRcEbCKbsNQOQcZUjFEOApKW8Slzb1B1SonsvtzI1o0cCy05snDqN2IGtOq54xgzyZ2ICComGhObGn3SGrDm7hGqzQOCCfaHxIkyN+9TN32q12z/zw3ZcCBCnnw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698936810; c=relaxed/simple; bh=+lnimXbT53gzmnj/1Wj2WzavnCwxjJpIfR9tUBaGfDE=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=VfTHGiYJEKYCmNl9Xqfbs8R8GTLVQgYHxuiC6HZtkUP0Lj0/SN60aeP0daeQJTFPdIQ7beqpJlsZv/mSEiPUwn+jeUhRPiDUpuQFmJVzfaRTuHAyEMgG+vk6zjyt8jYpnSWRFMt5kgVpgQi9ogeLTWkCAqy46E1XTseFw1Zj7z0= 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 qwCowACnMzEjt0NlJ6ZCAg--.4228S2; Thu, 02 Nov 2023 22:50:11 +0800 (CST) Message-ID: Date: Thu, 2 Nov 2023 22:53:17 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] gdb: RISC-V: Refine lr/sc sequence support To: Andrew Burgess , binutils@sourceware.org Cc: palmer@dabbelt.com, simon.marchi@polymtl.ca References: <20231031173922.4836-1-liuyang22@iscas.ac.cn> <87lebguypz.fsf@redhat.com> From: Yang Liu In-Reply-To: <87lebguypz.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CM-TRANSID:qwCowACnMzEjt0NlJ6ZCAg--.4228S2 X-Coremail-Antispam: 1UD129KBjvAXoWfGF47ZF1DWFWfGryrJF47Jwb_yoW8Jr15Xo W0gFs7GF1xJrnxCF1jkr1Iqr18W348JF4agr1UKwn5t3Z0v34kurW3Ja1DZa9IkF47Aa4D AFZ7Xa4ktFs7Zw13n29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7v73VFW2AGmfu7bjvjm3 AaLaJ3UjIYCTnIWjp_UUUY87k0a2IF6w4kM7kC6x804xWl14x267AKxVWUJVW8JwAFc2x0 x2IEx4CE42xK8VAvwI8IcIk0rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj4 1l84x0c7CEw4AK67xGY2AK021l84ACjcxK6xIIjxv20xvE14v26r1j6r1xM28EF7xvwVC0 I7IYx2IY6xkF7I0E14v26r1j6r4UM28EF7xvwVC2z280aVAFwI0_Cr1j6rxdM28EF7xvwV C2z280aVCY1x0267AKxVW0oVCq3wAac4AC62xK8xCEY4vEwIxC4wAS0I0E0xvYzxvE52x0 82IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUXVWUAw Av7VC2z280aVAFwI0_Gr0_Cr1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMxAI w28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I0E5I8CrVAFwI0_Jr0_Jr 4lx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVWUAVWUtwCIc40Y0x0EwIxG rwCI42IY6xIIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI8IcVCY1x0267AKxVWUJVW8Jw CI42IY6xAIw20EY4v20xvaj40_Jr0_JF4lIxAIcVC2z280aVAFwI0_Jr0_Gr1lIxAIcVC2 z280aVCY1x0267AKxVW8JVW8JrUvcSsGvfC2KfnxnUUI43ZEXa7IU5WbytUUUUU== 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 List-Id: On 2023/11/2 20:27, Andrew Burgess wrote: > Yang Liu writes: > >> 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. > Thanks for working on this. GDB patches are handled on: > > gdb-patches@sourceware.org > > could you re-post to that list please. Sorry for the inconvenience, re-posted. Best regards, Yang > > Thanks, > Andrew > >> 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 (); >> + 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}; >> } >> -- >> 2.39.2 (Apple Git-143)