From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 0cejBXniQ2VBRD0AWB0awg (envelope-from ) for ; Thu, 02 Nov 2023 13:55:05 -0400 Received: by simark.ca (Postfix, from userid 112) id 08AF01E11B; Thu, 2 Nov 2023 13:55:05 -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 E84A41E098 for ; Thu, 2 Nov 2023 13:55:02 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 59FDE3858CDA for ; Thu, 2 Nov 2023 17:55:02 +0000 (GMT) Received: from cstnet.cn (smtp81.cstnet.cn [159.226.251.81]) by sourceware.org (Postfix) with ESMTPS id 6F1003858C00 for ; Thu, 2 Nov 2023 17:54:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6F1003858C00 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 6F1003858C00 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=159.226.251.81 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698947690; cv=none; b=vzVlURR6+LWKlU8IPAW9k34arFl4FvDdnEKC+KLNxYgNvur9KhWY2Mo+TIse7fSgMLrIOX3NbW09PHgTa7Tb0GC4YD6BQaaD+Z4rkPM8qx+SDm5+p/tsHd8Y9rUGXVEgnOElBCDZi2VQ+o2FrGYp7QEyZSc2oXSwCY6UnryEnYw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698947690; c=relaxed/simple; bh=zyVAgsIholzmzdAi5NJKzKP1ZZVtkK40RhdssmlfGe4=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=nAJ1OGrssHDG+UksA5vPlyk1DOTLHJtLDiCBfHhQ2wYlKBwlYrqa6sieIkmNQDHFsnGxNaL3PbuwX+Y+7IEOlS4EpLqGBN4DP18GD3+LLjJupnHN6Mqmza85pyNHViTTJW/kXCfXOkPTw494AeRf12jI+iGCpnDK4TbEIXD5Vzk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [10.38.1.120] (unknown [113.120.165.216]) by APP-03 (Coremail) with SMTP id rQCowADHzZVe4kNlt59FAg--.53917S2; Fri, 03 Nov 2023 01:54:38 +0800 (CST) Message-ID: Date: Fri, 3 Nov 2023 01:54:38 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] gdb: RISC-V: Refine lr/sc sequence support To: Palmer Dabbelt , gdb-patches@sourceware.org Cc: Andrew Burgess , simon.marchi@polymtl.ca References: From: Yang Liu In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-CM-TRANSID: rQCowADHzZVe4kNlt59FAg--.53917S2 X-Coremail-Antispam: 1UD129KBjvAXoWfGF47Gw1rZF18tryUKw13twb_yoW8WF1DZo W0gr4fGF15XrnF9r1jyr4UJw13X3W8Jrsrtr1UK3s3tr1Fk348Z3y3J3yjv3yUtF4xGryU Ar18JrW0vF9rZr13n29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7v73VFW2AGmfu7bjvjm3 AaLaJ3UjIYCTnIWjp_UUUY17k0a2IF6w4kM7kC6x804xWl14x267AKxVWUJVW8JwAFc2x0 x2IEx4CE42xK8VAvwI8IcIk0rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj4 1l84x0c7CEw4AK67xGY2AK021l84ACjcxK6xIIjxv20xvE14v26r1j6r1xM28EF7xvwVC0 I7IYx2IY6xkF7I0E14v26r1j6r4UM28EF7xvwVC2z280aVAFwI0_Jr0_Gr1l84ACjcxK6I 8E87Iv6xkF7I0E14v26r4j6r4UJwAac4AC62xK8xCEY4vEwIxC4wAS0I0E0xvYzxvE52x0 82IY62kv0487Mc02F40EFcxC0VAKzVAqx4xG6I80ewAv7VC0I7IYx2IY67AKxVWUGVWUXw Av7VC2z280aVAFwI0_Jr0_Gr1lOx8S6xCaFVCjc4AY6r1j6r4UM4x0Y48IcVAKI48JMxAI w28IcxkI7VAKI48JMxC20s026xCaFVCjc4AY6r1j6r4UMI8I3I0E5I8CrVAFwI0_Jr0_Jr 4lx2IqxVCjr7xvwVAFwI0_JrI_JrWlx4CE17CEb7AF67AKxVWUAVWUtwCIc40Y0x0EwIxG rwCI42IY6xIIjxv20xvE14v26r1j6r1xMIIF0xvE2Ix0cI8IcVCY1x0267AKxVWUJVW8Jw CI42IY6xAIw20EY4v20xvaj40_Jr0_JF4lIxAIcVC2z280aVAFwI0_Jr0_Gr1lIxAIcVC2 z280aVCY1x0267AKxVWUJVW8JbIYCTnIWIevJa73UjIFyTuYvjxUg1IUUUUUU X-Originating-IP: [113.120.165.216] X-CM-SenderInfo: 5olx5tdqjsjq5lvft2wodfhubq/ X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, BODY_8BITS, 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 23:58, Palmer Dabbelt wrote: > On Thu, 02 Nov 2023 08:21:16 PDT (-0700), liuyang22@iscas.ac.cn wrote: >> >> 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. > > Thanks.  The general idea here seems pretty reasonable, but I haven't > looked that closely at the code.  I'm super behind on Linux stuff > right now, so I might be slower that usual... > No rush, thank you for your time! >>> 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.  */ > > That ends up with some odd interactions with the Zc extensions.  I > opened https://github.com/riscv/riscv-isa-manual/issues/1157 to check. > Okay, let's track this concern there. >>> + >>> +      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. > V2 posted. > 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? > Yeah, it seems difficult to get the lr/sc sequence completely correct, so can we implement a version that is correct in most cases (as this patch does)? >> 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}; >>>   }