From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cstnet.cn (smtp84.cstnet.cn [159.226.251.84]) by sourceware.org (Postfix) with ESMTPS id 6DBBF3858D1E for ; Tue, 7 Nov 2023 07:01:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6DBBF3858D1E 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 6DBBF3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=159.226.251.84 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699340476; cv=none; b=omf5DRrTsrBtLuWRRtOAV8ZGCLtGzSPg07mOczKFQPbIAuOkQcNClPJgvgdXtUqJPZ1pXs1yc/mIH10vsqjLa2iBMCih5iXNGeYB06JSAsD2tWMQ+iCNeDJAfQTk2MC1Zm8XGsSCRPm+5KjMAuTAz1wTeNmdR0FPn43U1XSo9To= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699340476; c=relaxed/simple; bh=voy49JvqH+AKOW2s52VbC28H2Vhvq4/ZvxMqX0VIe5E=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=nB5VGL+hs1RE6A5xQOmz9DMoUImSI+1/0/qhDiP9eAXiAXOoOvUu4nsR2Egd8LC+6IE5jgOcUY9dEYMefIxr1RvjWzGyOq3fWvVq4hu9tygeo9q/fDbsLxoLMBJhHawOViIgHlj3hZHXF660Xp61Hg4b8GTpsO2kUyq8x5Pz9kQ= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [10.38.1.120] (unknown [113.120.165.216]) by APP-05 (Coremail) with SMTP id zQCowACHsx2x4EllAaXeAA--.64034S2; Tue, 07 Nov 2023 15:01:05 +0800 (CST) Message-ID: <078cc940-1d90-4bf7-9b83-f3b36ce1208d@iscas.ac.cn> Date: Tue, 7 Nov 2023 15:01:05 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] gdb: RISC-V: Refine lr/sc sequence support To: Andrew Burgess , gdb-patches@sourceware.org Cc: palmer@dabbelt.com, simon.marchi@polymtl.ca References: <20231102174418.46080-1-liuyang22@iscas.ac.cn> <87il6fupfx.fsf@redhat.com> From: Yang Liu In-Reply-To: <87il6fupfx.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-CM-TRANSID:zQCowACHsx2x4EllAaXeAA--.64034S2 X-Coremail-Antispam: 1UD129KBjvAXoWfGF47CF1kAF4xCFy5Ary8uFg_yoW8Cry7Xo WFgrs7GF1xJr1xCF1jkr1xtr1rWa48XF4aqr1UK3s5t3Z0q348urW3Jw4DZa9IyFWxAa4D AFyxAayjyFZ7Z3Wfn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7v73VFW2AGmfu7bjvjm3 AaLaJ3UjIYCTnIWjp_UUUY07k0a2IF6w4kM7kC6x804xWl14x267AKxVWUJVW8JwAFc2x0 x2IEx4CE42xK8VAvwI8IcIk0rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj4 1l84x0c7CEw4AK67xGY2AK021l84ACjcxK6xIIjxv20xvE14v26r4j6ryUM28EF7xvwVC0 I7IYx2IY6xkF7I0E14v26r4j6F4UM28EF7xvwVC2z280aVAFwI0_Gr1j6F4UJwA2z4x0Y4 vEx4A2jsIEc7CjxVAFwI0_Cr1j6rxdM2vYz4IE04k24VAvwVAKI4IrM2AIxVAIcxkEcVAq 07x20xvEncxIr21l5I8CrVACY4xI64kE6c02F40Ex7xfMcIj6xIIjxv20xvE14v26r106r 15McIj6I8E87Iv67AKxVWUJVW8JwAm72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41l 42xK82IYc2Ij64vIr41l4I8I3I0E4IkC6x0Yz7v_Jr0_Gr1lx2IqxVAqx4xG67AKxVWUJV WUGwC20s026x8GjcxK67AKxVWUGVWUWwC2zVAF1VAY17CE14v26r126r1DMIIYrxkI7VAK I48JMIIF0xvE2Ix0cI8IcVAFwI0_Jr0_JF4lIxAIcVC0I7IYx2IY6xkF7I0E14v26r1j6r 4UMIIF0xvE42xK8VAvwI8IcIk0rVWUJVWUCwCI42IY6I8E87Iv67AKxVWUJVW8JwCI42IY 6I8E87Iv6xkF7I0E14v26r1j6r4UYxBIdaVFxhVjvjDU0xZFpf9x07b5FxUUUUUU= 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/6 18:49, 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 doing this. I can't comment too much on whether this code > complies with the ISA requirements as I don't follow the ISA at this > level these days. However, the code seems mostly clear, except for one > part of the logic that confused me for a while, I've commented on this > inline below. I also spotted a few GDB/GNU style nits that I'd like to > see fixed, I've detailed them inline below too. Thank you for the very detailed review.  I've submitted a v3 patch, which appled all the code style suggestions and fixed typos.  The comments before the for loop are also refined as suggested. > > Looking at Palmer's comment from your V1 thread: > > On 2023/11/2 23:58, Palmer Dabbelt wrote: > > > I also think the branch detection logic here isn't quite right: the > > LR/SC sequence only becomes unconstrained if the backwards branch is > > taken at runtime.  I can't find any examples of people doing that, but > > one could imagine some sort of spin-type patterns with an early retry > > that do this sort of thing. > > > > The same logic applies to disallowed instructions, but I don't think > > it's super likely those end up conditionally executed in LR/SC > > sequences so maybe we can ignore that? > > I think if we wanted to implement this then we'd have to actually > emulate all of the instructions within the sequence, which seems like a > much bigger task. > > As you suggested in your reply, I'd be happy to see this change merged > as it is an improvement over what we currently have, but maybe it's > worth adding a comment somewhere to indicate that we understand that > there are still limitations with this code? I think at a minimum, we > should probably discuss this issue in the commit message. > Added limitations statements before the for loop in v3. >> 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. > There's no problem including a ChangeLog style entry in the commit > message if you like this, but this is no longer required for commits to > GDB. The change is instead expected to be fully discussed in the > initial part of the commit message. > Thanks, I thought this was required and I've removed it in v3. >> Signed-off-by: Yang Liu >> --- >> gdb/riscv-tdep.c | 275 ++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 236 insertions(+), 39 deletions(-) >> >> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c >> index 3a2891c2c92..3c5afea99ca 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,149 @@ 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) > You need to add some white-space after the function name and before the > opening '('. In this case, the line is too long (> 80 characters), so > you should format it as: > > static bool > riscv_insn_is_non_cti_and_allowed_in_atomic_sequence > (const struct riscv_insn &insn) > { > ... etc ... > >> +{ >> + 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. */ > Typo: insctruction -> instruction. > >> + >> +static bool >> +riscv_insn_is_direct_branch(const struct riscv_insn &insn) > And here the formatting should be: > > static bool > riscv_insn_is_direct_branch (const struct riscv_insn &insn) > { > ... etc ... > >> +{ >> + 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; > The comment explaining the "magic" constant (16) should appear at the > point that the constant is introduced. However, I wonder in this case > if we should just move the constant declaration to later in the function > at the point where it's used (see below)... I removed the constant and used the literal directly in the for loop in v3, see below. >> + 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) > ... I'd be tempted to move the constant declaration to here, like: > > /* A lr/sc sequence comprise at most 16 instructions placed sequentially > in memory. */ > static const int max_atomic_sequence_length = 16; > for (int insn_count = 0; > insn_count < max_atomic_sequence_length; > ++insn_count) > { > ... etc ... > > though given the constant is only used in this one place, maybe I'd just > fold the constant in-line and rely on the comment to explain it? > > Notice I also renamed the constant to max_atomic_sequence_length as this > seems like a better description of what the constant is used for, but > then I had to line wrap the for (...) in order to keep the line under 80 > characters. In v3, I changed the for loop to look like this:   for (int insn_count = 0; insn_count < 16 - 2; ++insn_count) Notice that 16 has now become 14 -- I realized that we had used 2 of the 16 quotas for the first and last instructions.  I also stated this in the comments. > > The GNU style also requires two spaces after a '.' in comments, which > you'd missing here, and in a couple of other places below. > > I'm not 100% sure that the comment here is detailed enough. This caused > some of the confusion I had when reading this patch. I'll describe this > more later in this email. > >> + { >> + 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. */ > Some of these lines are too long. >> + >> + if (riscv_insn_is_non_cti_and_allowed_in_atomic_sequence (insn)) >> + { >> + continue; >> + } > Single statement blocks should not have the { ... } around them, so > just: > > 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. */ > Add an extra space after final '.' in comment. > >> + else if (riscv_insn_is_direct_branch (insn)) >> + { >> + if (insn.imm_signed () > 0) >> + { >> + next_pc = cur_step_pc + insn.imm_signed (); >> + next_pcs.push_back (next_pc); >> + } >> + else >> + break; >> + } >> + /* Look for a paired SC instruction which closes the atomic sequence. */ > Add an extra space after final '.' in comment. >> + 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)) > These lines are too long. > >> + { >> + found_valid_atomic_sequence = true; >> + } > And this should have the { ... } remove as it is a single statement > block. > > This is where I got confused the first time I read this patch. > > The comment earlier says: > > /* A lr/sc sequence comprise at most 16 instructions placed > sequentially in memory. */ > > My expectation after reading this was that upon finding the SC > instruction we'd be done, and break out of the loop ... which isn't what > happens. > > It was only when I went and read the ISA manual that I realised that the > 16 instruction limit includes both the lr/sc loop _and_ the code to > repeat the loop. > > I think the comment above the for loop should be extended to include > this information, otherwise, I think this is a little confusing. > > I think this comment above the for loop would also be the right place to > comment about the limitations of what we're checking for here. I think > there's two broad things we're not doing: > > (a) As Palmer pointed out, the atomic limitations only apply to the > code that is actually executed, so really we should be emulating > each instruction and checking what is actually going to be > executed, and > > (b) The lr/sc are required to be for the same target address. > However, to check this in the general case we would need to > emulate all the instructions up to the sc instruction in order to > check that the address it uses has not been modified. > > Thanks for the explanation, I've updated the comment as suggested. >> + 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 (); >> + return {}; >> + cur_step_pc += insn.length (); >> >> - /* We should now be at the end of the sequence. */ >> - if (cur_step_pc != last_addr) >> - return false; >> + /* Remove all PCs that jump within the sequence. */ >> + auto matcher = [cur_step_pc] (const CORE_ADDR addr) > My preference would be to write: > > auto matcher = [cur_step_pc] (const CORE_ADDR addr) -> bool > > we seem to use a mix in GDB, so if you are really opposed to this change > then that's fine. I like being able to see the type of MATCHER without > having to read the function body. Agreed, added the return type. Best regards, Yang > > Thanks, > Andrew > >> + { >> + return addr < cur_step_pc; >> + }; >> + auto it = std::remove_if (next_pcs.begin (), next_pcs.end (), matcher); >> + next_pcs.erase (it, next_pcs.end ()); >> >> - *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 +4655,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.34.1