From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id CWKIHk/HQ2VINz0AWB0awg (envelope-from ) for ; Thu, 02 Nov 2023 11:59:11 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=dabbelt-com.20230601.gappssmtp.com header.i=@dabbelt-com.20230601.gappssmtp.com header.a=rsa-sha256 header.s=20230601 header.b=r5z+qNGS; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 7004F1E11B; Thu, 2 Nov 2023 11:59:11 -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 3CFBE1E098 for ; Thu, 2 Nov 2023 11:59:09 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B9D8B3858C74 for ; Thu, 2 Nov 2023 15:59:08 +0000 (GMT) Received: from mail-ot1-x336.google.com (mail-ot1-x336.google.com [IPv6:2607:f8b0:4864:20::336]) by sourceware.org (Postfix) with ESMTPS id 9421E385841D for ; Thu, 2 Nov 2023 15:58:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9421E385841D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=dabbelt.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dabbelt.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 9421E385841D Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::336 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698940738; cv=none; b=NypFXWoYIlixnvifaCjapjixR4ZcysiHHkYKDbE7Q5o8SxhcGi2MATAder7EFlyeRACiOm0JztHPBwge3kuenaSKokfvXsuPFBuNWqkOQ05kUwEETll+VnKqycCbsD6avObO++fXBCClH6njjwZ4BmU+uyVtu8tCnz2eWbk4q+8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698940738; c=relaxed/simple; bh=wMbI9jeSx8GowiA41JAsN0iUAZLLVf4yJfmn2AX/dCg=; h=DKIM-Signature:Date:Subject:From:To:Message-ID:Mime-Version; b=Cci4QTzqW12pbRR4gHhvQY8qOAREwG7lmYPJ4PiFPkacaiMEsZZRZinX6xeEoqr5eWcp3uUQn1sewvHksWA0WuamkOYOqezCK8zA3MWZ3Gk64QhcJYmYCwhgIwMarL1J6XsrGdrDhzjxLAykHdn8XXV22mvc+pRxxX4KlHa8TpM= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-ot1-x336.google.com with SMTP id 46e09a7af769-6ce532451c7so585204a34.2 for ; Thu, 02 Nov 2023 08:58:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dabbelt-com.20230601.gappssmtp.com; s=20230601; t=1698940734; x=1699545534; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:from:to:cc:subject:date:message-id :reply-to; bh=ApDPQ38cpQzqKcyyENBP3WQzEjdbZr9Av/2JgjO6+HI=; b=r5z+qNGSlr7k+9zLWnk607boGHx2zr01ox8E70eMHf4kTyI92/sD+7gO0MUIatW+v6 pO2WDySHZ/ObNMUaJBaAomStvW362FBSUQGsBdvcajC+dS0T1EqbZZqePP0hww5GGkr7 duSZR18QYUMdO9SShLn2rH7RZCJKVO5SUnNOKKWh99lGH5csuXxxXKr2Wd+Iu/3oFEJ/ FILDVf9kYoQCPlWnQAOJrYBzw+lXmSTUwCt1hN0E10AkYPJj1w/uvf+6gWpX+X80knyL AxrobSVJPKbqqkn59QAyl5ikugkozvpPpmYb1V/bAUaVhdHm2bdOSpRzmt4f1jyfkXqJ 2xZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698940734; x=1699545534; h=content-transfer-encoding:mime-version:message-id:to:from:cc :in-reply-to:subject:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=ApDPQ38cpQzqKcyyENBP3WQzEjdbZr9Av/2JgjO6+HI=; b=b0eEbs38Nb6pLTiuznWHh+MUhPZlxe/NjnNsHrb1KjHTr8K+rEFfI+/TAdcFeFY/OK 53h63eApUhduhjKCIk+0eVYOUdTSoCybkiJftZ8Yk4mQEfGvOtxiYvbZG+ZnihXffsjb QdBK+PVVHXggoh2LPN/5cbrcQrJ7ihLCmOeLLRqrztpj8jcRkvOZqCeVY2J/m5Re7eGr j3EE8RDq0Vvc6qdvcGp/sST53MvPjbGGCgnCMYvYP9LvblVQLPx4GZIEFLOyjNUo5YZz RKWqvyeZfFEa6b3xeHtgYzMdbJxkB5CGTzmNFle7prp+nkKqVYMUBXWMrP0iIeytbGnu I2oA== X-Gm-Message-State: AOJu0Yyiuo6hyyjvTmdFkDZ8QA8T+K0YN3qt+7I9ysxBn85OzhVKrseX nxOe1FcIV4p2yUKTrp0YgBC2V0fM8BMwdaqyt0c= X-Google-Smtp-Source: AGHT+IH8e+VuIoGZbRi2lfoc60SH2q9LdAcx5fRGS47E9hky2NrtAViAqTuiR0goVTNsW2xe+7Y2qg== X-Received: by 2002:a05:6830:7190:b0:6b8:9932:b8ad with SMTP id el16-20020a056830719000b006b89932b8admr19352500otb.1.1698940734336; Thu, 02 Nov 2023 08:58:54 -0700 (PDT) Received: from localhost ([192.184.165.199]) by smtp.gmail.com with ESMTPSA id f24-20020a9d6c18000000b006cd0a04b56esm510428otq.56.2023.11.02.08.58.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Nov 2023 08:58:53 -0700 (PDT) Date: Thu, 02 Nov 2023 08:58:53 -0700 (PDT) X-Google-Original-Date: Thu, 02 Nov 2023 08:58:51 PDT (-0700) Subject: Re: [PATCH] gdb: RISC-V: Refine lr/sc sequence support In-Reply-To: <8bd610eb-2713-4f1a-b928-ad9fa3020dd0@iscas.ac.cn> CC: gdb-patches@sourceware.org, Andrew Burgess , simon.marchi@polymtl.ca From: Palmer Dabbelt To: liuyang22@iscas.ac.cn Message-ID: Mime-Version: 1.0 (MHng) Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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 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... >> 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. >> + >> + 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. 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? > 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}; >> }