From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 0fogKJbNq2r7YxYAWB0awg (envelope-from ) for ; Thu, 17 Sep 2026 07:23:02 -0400 Received: by simark.ca (Postfix, from userid 112) id 9EB261E06B; Thu, 17 Sep 2026 07:23:02 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-5.3 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [IPv6:2620:52:6:3111::32]) (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 A58D91E01F for ; Thu, 17 Sep 2026 07:22:58 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DB7404BA9027 for ; Thu, 17 Sep 2026 11:22:56 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DB7404BA9027 Received: from angie.orcam.me.uk (angie.orcam.me.uk [IPv6:2001:4190:8020::34]) by sourceware.org (Postfix) with ESMTP id DC17C4B9DB61 for ; Thu, 17 Sep 2026 11:22:01 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DC17C4B9DB61 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=orcam.me.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=orcam.me.uk ARC-Filter: OpenARC Filter v1.0.0 sourceware.org DC17C4B9DB61 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=2001:4190:8020::34 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1789644122; cv=none; b=et63WzsfU3V3MCzvKyN4Fl3WfIUY11enz6h0fBMIf8ZHqY/nsrFRRLobZdpGBNfTzG4VhTC+aH5q1vIPd+3wDhD610MnIlUHy0IizYWXKcY0+Yvg2zU7Uwb2PZBLWTj8Dg6Pyw+ea2NyRcTmPNXSPq3AFjrF96UQWXKlnJgDiHU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1789644122; c=relaxed/simple; bh=o+X8ClpN3EYQdkelQ36HY7dv9lUHVWYdjZwvkP31U/0=; h=Date:From:To:Subject:Message-ID:MIME-Version; b=LSnXmFDmHfTudX1JnmyvFMpicg5fUdvepnNISb8bWdOe0x+dTEBSWxRkogqDBbJkswk+P0n5pVK/rTgga6duX/vgRLYwRC0PZLHDBY8RYN/gACXjYjQ3yMDOplFvOACQ81AXD6d+JhUFTG2iCGrLvvsFKIFVwgrCJT3mgFSrhag= ARC-Authentication-Results: i=1; sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DC17C4B9DB61 Received: by angie.orcam.me.uk (Postfix, from userid 500) id 9DA3092009D; Thu, 17 Sep 2026 13:22:00 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by angie.orcam.me.uk (Postfix) with ESMTP id 9857492009C; Thu, 17 Sep 2026 12:22:00 +0100 (BST) Date: Thu, 17 Sep 2026 12:22:00 +0100 (BST) From: "Maciej W. Rozycki" To: Jovan Dmitrovic cc: "gdb-patches@sourceware.org" , Djordje Todorovic , Milica Matic , "Maciej W. Rozycki" Subject: Re: [PATCH v13 2/2] gdb: mips: Add MIPSR6 support In-Reply-To: <20250604123838.501596-3-jovan.dmitrovic@htecgroup.com> Message-ID: References: <20250604123838.501596-1-jovan.dmitrovic@htecgroup.com> <20250604123838.501596-3-jovan.dmitrovic@htecgroup.com> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT 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 Wed, 4 Jun 2025, Jovan Dmitrovic wrote: > From: Milica Matic Thank you for your submission. This is extensive, so I will be going in details through all the hunks individually, even those that I've concluded are fine as is. > Introduce new instruction encodings from Release 6 of the MIPS > architecture [1]. Support breakpoints and single stepping with > compact branches, forbidden slots, new branch instruction and > new atomic load-store instruction encodings. [1] is missing its reference. Double space after a full stop please. > Signed-off-by: Milica Matic > Signed-off-by: Jovan Dmitrović Just as with 1/2 please resubmit under a copyright assignment with FSF. > diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c > index 48284abdd73..9cf73e30540 100644 > --- a/gdb/mips-tdep.c > +++ b/gdb/mips-tdep.c > @@ -76,6 +76,10 @@ static int mips16_insn_at_pc_has_delay_slot (struct gdbarch *gdbarch, > static void mips_print_float_info (struct gdbarch *, struct ui_file *, > const frame_info_ptr &, const char *); > > +static void > +mips_read_fp_register_single (const frame_info_ptr &frame, int regno, > + gdb::array_view rare_buffer); > + OK, forward declaration. > @@ -326,6 +330,17 @@ mips_abi_regsize (struct gdbarch *gdbarch) > } > } > > +/* Return true if the gdbarch is based on MIPS Release 6. */ > + > +static bool > +is_mipsr6_isa (struct gdbarch *gdbarch) > +{ > + const struct bfd_arch_info *info = gdbarch_bfd_arch_info (gdbarch); > + > + return (info->mach == bfd_mach_mipsisa32r6 > + || info->mach == bfd_mach_mipsisa64r6); > +} > + OK, helper to determine R6 presence. > @@ -1554,6 +1569,7 @@ mips_fetch_instruction (struct gdbarch *gdbarch, > #define b0s11_op(x) ((x) & 0x7ff) > #define b0s12_imm(x) ((x) & 0xfff) > #define b0s16_imm(x) ((x) & 0xffff) > +#define b0s21_imm(x) ((x) & 0x1fffff) OK, helper to extract immediate 21-bit branch offset field. > @@ -1590,6 +1606,24 @@ mips32_relative_offset (ULONGEST inst) > return ((itype_immediate (inst) ^ 0x8000) - 0x8000) << 2; > } > > +/* Calculates pc-relative offset from lower 21 bits of instruction. s/Calculates/Calculate/, s/pc-relative/PC-relative/ > + Used by BEQZC, BNEZC. */ > + > +static LONGEST > +mips32_relative_offset21 (ULONGEST insn) > +{ > + return ((b0s21_imm (insn) ^ 0x100000) - 0x100000) << 2; > +} OK, helper to calculate actual branch displacement from its 21-bit offset field. > + > +/* Calculates pc-relative offset from lower 26 bits of an instruction. s/Calculates/Calculate/, s/pc-relative/PC-relative/ > + Used by BC, BALC. */ > + > +static LONGEST > +mips32_relative_offset26 (ULONGEST insn) > +{ > + return ((b0s26_imm (insn) ^ 0x2000000) - 0x2000000) << 2; > +} > + OK, helper to calculate actual branch displacement from its 26-bit offset field. > @@ -1650,6 +1684,77 @@ is_octeon_bbit_op (int op, struct gdbarch *gdbarch) > return 0; > } > > +/* Detects whether overflow occurs when adding two 32-bit integers. */ s/Detects/Detect/ > + > +static bool > +is_add32bit_overflow (int32_t a, int32_t b) > +{ > + int32_t r = (uint32_t) a + (uint32_t) b; > + return (a < 0 && b < 0 && r >= 0) || (a > 0 && b > 0 && r <= 0); > +} > + > +/* Helper function for BOVC and BNVC instructions which are introduced in > + MIPS Release 6. > + > + BOVC performs a signed 32-bit addition of two registers. BOVC discards the > + sum, but detects signed 32-bit integer overflow of the sum (and the inputs, > + in MIPS64), and branches if such overflow is detected. > + > + BNVC does the opposite, i.e. branches if such overflow is not detected. */ Please rewrite in the imperative mood. > + > +static bool > +is_add64bit_overflow (int64_t a, int64_t b) > +{ > + if (a != (int32_t) a) > + return true; > + if (b != (int32_t) b) > + return true; > + return is_add32bit_overflow ((int32_t) a, (int32_t) b); > +} I think this is overly complex. First, there's little point in omitting the input overflow checks with 32-bit targets; they're cheap and they just won't ever trigger. Second, the same check can be applied to the result of the calculation, which will also more closely match the instruction description in the architecture manual, further reducing this code. The name of the function is obviously confusing, since BOVC/BNVC always check for 32-bit overflow. So: static bool is_add32bit_overflow (int64_t a, int64_t b) { int64_t a32 = (int32_t) a, b32 = (int32_t) b, r = a + b; return a != a32 | b != b32 | r != (int32_t) r; } for a short usually branchless sequence. > + > +#define DELAY_SLOT_SIZE 4 This needs to be called MIPS32_DELAY_SLOT_SIZE as it is specific to the regular MIPS ISA; MIPS16 and microMIPS instructions have varying delay slot sizes. > + > +/* Calculate address of next instruction after BLEZ. */ The comment does not match what the function does. > + > +static CORE_ADDR > +mips32_blez_pc (struct gdbarch *gdbarch, struct regcache *regcache, This asks for a better name, even if `mips32_bcond_pc'. And I think this function will best be factored out as a preparatory change to handle BLEZ, BLEZL, BGTZ, BGTZL with all the logic already there. This will make R6 updates clearer. > + ULONGEST inst, CORE_ADDR pc, int invert) Here `invert' is boolean, so make it `bool'. > +{ > + int rs = itype_rs (inst); > + int rt = itype_rt (inst); > + LONGEST val_rs = regcache_raw_get_signed (regcache, rs); > + LONGEST val_rt = regcache_raw_get_signed (regcache, rt); > + ULONGEST uval_rs = regcache_raw_get_unsigned (regcache, rs); > + ULONGEST uval_rt = regcache_raw_get_unsigned (regcache, rt); OK, fetching stuff for branch emulation. > + bool taken = false; No need to preinitialise as this is supposed to be always calculated. Perhaps it would better be called `condition' or `cond' since it's being transformed on the way and a `true' value at one point does not mean the branch will be actually taken in the end. > + > + /* BLEZ, BLEZL, BGTZ, BGTZL */ > + if (rt == 0) > + taken = (val_rs <= 0); This needs to be qualified with `is_mipsr6_isa (gdbarch)' to facilitate pre-R6 silicon that didn't fully subdecode these instructions. Also reformat as per your 1/2. So: if (rt == 0 || !is_mipsr6_isa (gdbarch)) { /* BLEZ, BLEZL, BGTZ, BGTZL */ taken = (val_rs <= 0); } I take it assumption is here that BLEZL/BGTZL will be emulated for R6. > + else if (is_mipsr6_isa (gdbarch)) > + { > + /* BLEZALC, BGTZALC */ > + if (rs == 0 && rt != 0) > + taken = (val_rt <= 0); > + /* BGEZALC, BLTZALC */ > + else if (rs == rt && rt != 0) > + taken = (val_rt >= 0); > + /* BGEUC, BLTUC */ > + else if (rs != rt && rs != 0 && rt != 0) > + taken = (uval_rs >= uval_rt); > + } > + ... and then you can flatten the remaining conditionals while removing superfluous expressions already eliminated by earlier conditions: else if (rs == 0) { /* BLEZALC, BGTZALC */ taken = (val_rt <= 0); } else if (rs == rt) { /* BGEZALC, BLTZALC */ taken = (val_rt >= 0); } else { /* BGEUC, BLTUC */ taken = (uval_rs >= uval_rt); } Please provide bit patterns for opcodes referred through this function, using `micromips_next_pc' as the reference (`mips32_next_pc' is largely old code which wasn't updated, but new additions followed this convention and we should continue doing go). > + if (invert) > + taken = !taken; > + > + /* Calculate branch target. */ > + if (taken) Just: if (taken ^ invert) (or `cond' as per discussion above) instead of the sequence above. > + pc += mips32_relative_offset (inst); > + else > + pc += DELAY_SLOT_SIZE; > + Make sure to also handle landing in the delay/forbidden slot here. > + return pc; > +} OK, returning the PC calculated. > @@ -1660,11 +1765,16 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > struct gdbarch *gdbarch = regcache->arch (); > unsigned long inst; > int op; > + bool mips64bitreg = false; > + > + if (mips_isa_regsize (gdbarch) == 8) > + mips64bitreg = true; > + This can go as per observation re `is_add64bit_overflow'. > inst = mips_fetch_instruction (gdbarch, ISA_MIPS, pc, NULL); > op = itype_op (inst); > if ((inst & 0xe0000000) != 0) /* Not a special, jump or branch instruction. */ > { > - if (op >> 2 == 5) > + if (op >> 2 == 5 && ((op & 0x02) == 0 || itype_rt (inst) == 0)) Similarly this needs to be qualified with `is_mipsr6_isa (gdbarch)' to facilitate pre-R6 silicon that didn't fully subdecode these instructions. Though it seems we don't want to change anything here and instead handle all the stuff in `mips32_blez_pc'; see the relevant note below. Comments referring to BLEZL, BGTZL below will need to be adjusted accordingly then. Also I'd like to see `is_mipsr6_isa (gdbarch)' cached in `is_mipsr6' as in `mips_deal_with_atomic_sequence'. > @@ -1674,7 +1784,7 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > case 1: /* BNEL */ > goto neq_branch; > case 2: /* BLEZL */ > - goto less_branch; > + goto lez_branch; Unrelated change; separate fix in the pipeline already. > @@ -1686,19 +1796,23 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > /* BC1F, BC1FL, BC1T, BC1TL: 010001 01000 */ > pc = mips32_bc1_pc (gdbarch, regcache, inst, pc + 4, 1); > } > - else if (op == 17 && itype_rs (inst) == 9 > + else if (!is_mipsr6_isa (gdbarch) > + && op == 17 > + && itype_rs (inst) == 9 > && (itype_rt (inst) & 2) == 0) > { > /* BC1ANY2F, BC1ANY2T: 010001 01001 xxx0x */ > pc = mips32_bc1_pc (gdbarch, regcache, inst, pc + 4, 2); > } > - else if (op == 17 && itype_rs (inst) == 10 > + else if (!is_mipsr6_isa (gdbarch) > + && op == 17 > + && itype_rs (inst) == 10 > && (itype_rt (inst) & 2) == 0) > { > /* BC1ANY4F, BC1ANY4T: 010001 01010 xxx0x */ > pc = mips32_bc1_pc (gdbarch, regcache, inst, pc + 4, 4); > } > - else if (op == 29) > + else if (!is_mipsr6_isa (gdbarch) && op == 29) > { > /* JALX: 011101 > The new PC will be alternate mode. */ OK, these instructions have been removed in R6 and there's no intent or possibility (due to an opcode overlap) to emulate them. FWIW rather than being grown even further I would be happy to see this spaghetti conditional go in favour to a switch statement, but let's leave it to a follow-up cleanup or we may never get out of this swamp. > @@ -1731,7 +1845,148 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > pc += 8; > } > } > + else if (is_mipsr6_isa (gdbarch)) > + { > + if (op == 8 || op == 24) > + { > + /* BOVC, BEQZALC, BEQC and BNVC, BNEZALC, BNEC */ Again, please provide bit patterns for opcodes. Likewise throughout. > + int rs = rtype_rs (inst); > + int rt = rtype_rt (inst); > + LONGEST val_rs = regcache_raw_get_signed (regcache, rs); > + LONGEST val_rt = regcache_raw_get_signed (regcache, rt); OK, fetching stuff for branch emulation. > + bool taken = false; No need to preinitialise; again suggest calling this `cond'. New line to separate from code please. > + if (rs >= rt) > + { > + /* BOVC (BNVC) */ > + if (mips64bitreg) > + taken = is_add64bit_overflow (val_rs, val_rt); > + else > + taken = is_add32bit_overflow (val_rs, val_rt); Adjust as per observation re `is_add64bit_overflow'. > + } > + else if (rs < rt && rs == 0) Suggest: else if (rs == 0) since `rs < rt' must be true now, owing to the previous condition. > + { > + /* BEQZALC (BNEZALC) */ > + taken = (val_rt == 0); > + } > + else > + { > + /* BEQC (BNEC) */ > + taken = (val_rs == val_rt); > + } OK, this matches instruction descriptions from the ISA spec. > > + if (op == 24) > + { > + /* BNVC, BNEZALC, BNEC */ > + taken = !taken; > + } > + > + if (taken) bool invert = (op == 24); if (taken ^ invert) > + pc += mips32_relative_offset (inst) + 4; > + else > + { > + /* Step through the forbidden slot to avoid repeated > + exceptions. > + We do not currently have access to the BD bit when hitting > + a breakpoint and therefore cannot tell if the breakpoint > + hit on the branch or the forbidden slot. */ Some debug environments do provide access to CP0.Cause.BD, so I think the second sentence will better read: "We may or may not have access to the BD bit when hitting a breakpoint and therefore cannot reliably tell if the breakpoint hit on the branch or the forbidden slot." Also this is poorly formatted and needs paragraph justification applied. And please stay within 74 columns as per our coding style (80 is only the hard limit for cases where there's no other way). Last but not least I'd like to see a similar note in the introductory comment for `mips32_blez_pc', observing that the branches it emulates may have either a delay or a forbidden slot. > + pc += 8; So this will have to skip over the forbidden slot too if the PC lands there. > + } > + } > + else if (op == 17 && (itype_rs (inst) == 9 || itype_rs (inst) == 13)) > + { > + /* BC1EQZ, BC1NEZ */ > + gdb_byte status; > + gdb_byte true_val = 0; Move variable declarations down to the first use. > + unsigned int fp = (gdbarch_num_regs (gdbarch) > + + mips_regnum (gdbarch)->fp0 > + + itype_rt (inst)); > + struct frame_info_ptr frame = get_current_frame (); OK, stuff for the `mips_read_fp_register_single' call. > + gdb_byte *buf_tmp = (gdb_byte *) alloca (sizeof (gdb_byte) * 4); > + gdb::array_view raw_buffer = gdb::make_array_view (buf_tmp, sizeof (gdb_byte) * 4); Argh, why `alloca' for 4 bytes? Just make it: gdb::byte_vector raw_buffer (4); > + mips_read_fp_register_single (frame, fp, raw_buffer); > + > + if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG) > + status = raw_buffer[3]; > + else > + status = raw_buffer[0]; And then obviously raw_buffer.data ()[...]. > + > + if (itype_rs (inst) == 13) > + true_val = 1; bool true_val = (itype_rs (inst) == 13); > + > + if ((status & 0x1) == true_val) > + pc += mips32_relative_offset (inst) + 4; > + else > + pc += 8; Make sure to also handle landing in the forbidden slot here. > + } > + else if (op == 22 || op == 23) > + { > + /* BLEZC, BGEZC, BGEC, BGTZC, BLTZC, BLTC */ > + int rs = rtype_rs (inst); > + int rt = rtype_rt (inst); > + LONGEST val_rs = regcache_raw_get_signed (regcache, rs); > + LONGEST val_rt = regcache_raw_get_signed (regcache, rt); > + bool taken = false; > + /* The R5 rt == 0 case is handled above so we treat it as > + an unknown instruction here for future ISA usage. */ > + if (rs == 0 && rt != 0) > + taken = (val_rt <= 0); > + else if (rs == rt && rt != 0) > + taken = (val_rt >= 0); > + else if (rs != rt && rs != 0 && rt != 0) > + taken = (val_rs >= val_rt); > + > + if (op == 23) > + taken = !taken; > + > + if (taken) > + pc += mips32_relative_offset (inst) + 4; > + else > + { > + /* Step through the forbidden slot to avoid repeated > + exceptions. > + We do not currently have access to the BD bit when hitting > + a breakpoint and therefore cannot tell if the breakpoint > + hit on the branch or the forbidden slot. */ > + pc += 8; > + } > + } Why isn't this stuff handled with `mips32_blez_pc'? AFAICT the code is equivalent except for the BGEUC, BLTUC vs BGEC, BLTC case, which can be easily factored in. > + else if (op == 50 || op == 58) > + { > + /* BC, BALC */ > + pc += mips32_relative_offset26 (inst) + 4; OK, no forbidden slot here. > + } > + else if ((op == 54 || op == 62) > + && rtype_rs (inst) == 0) This fits in one line, no need to fold. > + { > + /* JIC, JIALC */ > + pc = regcache_raw_get_signed (regcache, itype_rt (inst)); > + pc += (itype_immediate (inst) ^ 0x8000) - 0x8000; OK, no forbidden slot here. > + } > + else if (op == 54 || op == 62) > + { > + /* BEQZC, BNEZC */ > + int rs = itype_rs (inst); > + LONGEST rs_val = regcache_raw_get_signed (regcache, rs); > + bool taken = (rs_val == 0); > + if (op == 62) > + taken = !taken; > + if (taken) bool invert = (op == 62); if (taken ^ invert) > + pc += mips32_relative_offset21 (inst) + 4; > + else > + { > + /* Step through the forbidden slot to avoid repeated exceptions > + we do not currently have access to the BD bit when hitting a > + breakpoint and therefore cannot tell if the breakpoint > + hit on the branch or the forbidden slot. */ Overlong lines. > + pc += 8; > + } > + } > + else > + { > + /* Not a branch, next instruction is easy. */ > + pc += 4; > + } > + } > else > pc += 4; /* Not a branch, next instruction is easy. */ So we have two levels of defaults now, one for the outer conditional, and another for the inner one within `else if (is_mipsr6_isa (gdbarch))'. Can we please have the whole thing flattened by removing the `else' clause and embedding the check within each individual conditional, just as earlier on done with `!is_mipsr6_isa (gdbarch)'? I think it'll make reading through this multi-way conditional easier and will also provide for a cleaner move to a switch statement in the future. > @@ -1775,7 +2030,6 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > case 2: /* BLTZL */ > case 16: /* BLTZAL */ > case 18: /* BLTZALL */ > - less_branch: Separate fix in the pipeline already. > @@ -1791,6 +2045,7 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > pc += 8; /* after the delay slot */ > break; > case 0x1c: /* BPOSGE32 */ > + case 0x1d: /* BPOSGE32C */ > case 0x1e: /* BPOSGE64 */ > pc += 4; > if (itype_rs (inst) == 0) > @@ -1804,11 +2059,18 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > break; > } > > + /* BPOSGE32C */ > + if (op == 0x1d) > + { > + if (!is_mipsr6_isa (gdbarch)) > + break; > + } > + I think this is weirdly placed -- why after the check for `dspctl' and why checking for BPOSGE32C twice in the first place. For the two hunks above I'd suggest: case 0x1d: /* BPOSGE32C */ if (!is_mipsr6_isa (gdbarch)) { pc += 4; break; } [[fallthrough]]; case 0x1c: /* BPOSGE32 */ ... > if ((regcache_raw_get_unsigned (regcache, > dspctl) & 0x7f) >= pos) > pc += mips32_relative_offset (inst); > else > - pc += 4; > + pc += DELAY_SLOT_SIZE; Gratuitous change. I don't mind changing pre-existing sites, but that'd have to be done separately; best with a preparatory change for the whole DELAY_SLOT_SIZE stuff, especially as this is getting messy otherwise: in individual places we add either 4 or 8 and there seems no pattern in that. > @@ -1842,19 +2104,14 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc) > else > pc += 8; > break; > - case 6: /* BLEZ, BLEZL */ > - if (regcache_raw_get_signed (regcache, itype_rs (inst)) <= 0) > - pc += mips32_relative_offset (inst) + 4; > - else > - pc += 8; > + case 6: /* BLEZ, BLEZL, BLEZALC, BGEZALC, BGEUC */ OK for the comment, more branch types handled now. > + lez_branch: Fixed separately already. > + pc = mips32_blez_pc (gdbarch, regcache, inst, pc + 4, 0); OK, code factored out to a helper. > break; > case 7: > default: > - greater_branch: /* BGTZ, BGTZL */ > - if (regcache_raw_get_signed (regcache, itype_rs (inst)) > 0) > - pc += mips32_relative_offset (inst) + 4; > - else > - pc += 8; > + greater_branch: /* BGTZ, BGTZL, BGTZALC, BLTZALC, BLTUC */ OK for the comment, more branch types handled now. > + pc = mips32_blez_pc (gdbarch, regcache, inst, pc + 4, 1); OK, code factored out to a helper. > @@ -2487,6 +2744,63 @@ micromips_instruction_is_compact_branch (unsigned short insn) > } > } > Please move this regular-MIPS stuff ahead of microMIPS code. > +/* Return non-zero if the MIPS instruction INSN is a compact branch > + or jump. A value of 1 indicates an unconditional compact branch > + and a value of 2 indicates a conditional compact branch. */ > + > +static int Return enumeration constants rather than magic numbers. > +mips32_instruction_is_compact_branch (struct gdbarch *gdbarch, ULONGEST insn) > +{ Non-MIPSr6 regular-MIPS instructions are never compact branches, so return early if `!is_mipsr6_isa (gdbarch)', making it more prominent and simplifying the switch statement. > + switch (itype_op (insn)) > + { > + case 50: /* BC */ > + case 58: /* BALC */ > + if (is_mipsr6_isa (gdbarch)) > + return 1; > + break; > + case 8: /* BOVC, BEQZALC, BEQC */ > + case 24: /* BNVC, BNEZALC, BNEC */ > + if (is_mipsr6_isa (gdbarch)) > + return 2; > + break; > + case 54: /* BEQZC, JIC */ > + case 62: /* BNEZC, JIALC */ > + if (is_mipsr6_isa (gdbarch)) > + { > + /* JIC, JIALC are unconditional */ > + return (itype_rs (insn) == 0) ? 1 : 2; > + } > + break; > + case 22: /* BLEZC, BGEZC, BGEC */ > + case 23: /* BGTZC, BLTZC, BLTC */ > + case 6: /* BLEZALC, BGEZALC, BGEUC */ > + case 7: /* BGTZALC, BLTZALC, BLTUC */ > + if (is_mipsr6_isa (gdbarch) > + && itype_rt (insn) != 0) > + return 2; > + break; > + case 1: /* BPOSGE32C */ > + if (is_mipsr6_isa (gdbarch) > + && itype_rt (insn) == 0x1d && itype_rs (insn) == 0) > + return 2; > + } Please convert to hex constants in the case labels and sort them by the increasing value, except where falling through of course. Using decimal encoding causes obfuscation since ISA documentation uses binary encoding except for 3-bit opcode pieces, which doesn't help here. Also vertically align comments with tabs. FWIW using octal encoding would possibly be the best choice given how the opcode tables have been arranged, but we have no such code so far, so I'll leave it to analyse separately and possibly apply a no-functional-change mechanical update to make such a switch once the dust has settled. > + return 0; > +} > + > +/* Return true if a standard MIPS instruction at ADDR has a branch > + forbidden slot (i.e. it is a conditional compact branch instruction). */ > + > +static bool > +mips32_insn_at_pc_has_forbidden_slot (struct gdbarch *gdbarch, CORE_ADDR addr) > +{ > + int status; > + ULONGEST insn = mips_fetch_instruction (gdbarch, ISA_MIPS, addr, &status); > + if (status) > + return false; > + > + return mips32_instruction_is_compact_branch (gdbarch, insn) == 2; > +} > + OK, analogous to `mips32_insn_at_pc_has_delay_slot'. See a note on `mips_adjust_breakpoint_address' though. Also doesn't `mips_single_step_through_delay' have to be updated accordingly? In the fall-through case we don't want to hit any forbidden-slot breakpoint, do we? > @@ -3530,7 +3844,8 @@ mips32_scan_prologue (struct gdbarch *gdbarch, > reg = high_word & 0x1f; > > if (high_word == 0x27bd /* addiu $sp,$sp,-i */ > - || high_word == 0x23bd /* addi $sp,$sp,-i */ > + || (high_word == 0x23bd /* addi $sp,$sp,-i */ > + && !is_mipsr6_isa (gdbarch)) OK, ADDI major opcode has been reused in R6 and cannot be emulated. > @@ -3670,7 +3985,9 @@ mips32_scan_prologue (struct gdbarch *gdbarch, > > /* A jump or branch, or enough non-prologue insns seen? If so, > then we must have reached the end of the prologue by now. */ > - if (prev_delay_slot || non_prologue_insns > 1) > + if (prev_delay_slot > + || non_prologue_insns > 1 > + || mips32_instruction_is_compact_branch (gdbarch, inst)) > break; OK, we need to terminate the scan upon encountering a compact branch too, analogously to the microMIPS variant. Please adjust for the enumeration constant return though. > @@ -3976,6 +4293,70 @@ mips_addr_bits_remove (struct gdbarch *gdbarch, CORE_ADDR addr) > #define LLD_OPCODE 0x34 > #define SC_OPCODE 0x38 > #define SCD_OPCODE 0x3c > +#define LLSC_R6_OPCODE 0x1f This is SPECIAL3 AFAICT, used for various other machine operations as well, so this need to be called SPECIAL3_OPCODE. Also this is a pre-R6 definition and has to go in with EVA support; see below. > +#define LL_R6_FUNCT 0x36 OK, R6. > +#define LLE_FUNCT 0x2e EVA. > +#define LLD_R6_FUNCT 0x37 > +#define SC_R6_FUNCT 0x26 OK, R6. > +#define SCE_FUNCT 0x1e EVA. > +#define SCD_R6_FUNCT 0x27 OK, R6. > + > +/* Determine whether instruction 'insn' is of 'load linked X' type. > + LL/SC instructions provide primitives to implement atomic > + read-modify-write operations for synchronizable memory locations. */ > + > +static bool > +is_ll_insn (struct gdbarch *gdbarch, ULONGEST insn) > +{ > + if (itype_op (insn) == LL_OPCODE > + || itype_op (insn) == LLD_OPCODE) > + return true; > + > + if (rtype_op (insn) == LLSC_R6_OPCODE > + && rtype_funct (insn) == LLE_FUNCT > + && (insn & 0x40) == 0) > + return true; This is pre-R6 EVA support. This needs to go in separately as a preparatory change. > + > + /* Handle LL and LLP varieties. */ s/LLP/LLD/ presumably (or LLxP?). Maybe just drop the comment since the code is obvious. > + if (is_mipsr6_isa (gdbarch) > + && rtype_op (insn) == LLSC_R6_OPCODE > + && (rtype_funct (insn) == LL_R6_FUNCT > + || rtype_funct (insn) == LLD_R6_FUNCT > + || rtype_funct (insn) == LLE_FUNCT)) The check for LLE_FUNCT is already handled above. The same check for `(insn & 0x40) == 0' is needed here for the remaining cases. I'd lean towards using `rtype_shamt' for field decoding in both places. > + return true; > + > + return false; > +} > + > +/* Determine whether instruction 'insn' is of 'store conditional X' type. > + SC instructions and varieties perform completion of read-modify-write > + atomic sequence. */ > + > +static bool > +is_sc_insn (struct gdbarch *gdbarch, ULONGEST insn) > +{ > + if (itype_op (insn) == SC_OPCODE > + || itype_op (insn) == SCD_OPCODE) > + return true; > + > + if (rtype_op (insn) == LLSC_R6_OPCODE > + && rtype_funct (insn) == SCE_FUNCT > + && (insn & 0x40) == 0) > + return true; Same observation as to EVA support. > + > + /* Handle SC and SCP varieties. */ Again, maybe just drop the comment since the code is obvious. > + if (is_mipsr6_isa (gdbarch) > + && rtype_op (insn) == LLSC_R6_OPCODE > + && (rtype_funct (insn) == SC_R6_FUNCT > + || rtype_funct (insn) == SCD_R6_FUNCT > + || rtype_funct (insn) == SCE_FUNCT)) And likewise as to SCE_FUNCT and `rtype_shamt'. > + return true; > + > + return false; > +} > + > +/* Handle mips atomic sequence which starts with LL/LLD and ends with s/mips/MIPS/ > @@ -3988,10 +4369,11 @@ mips_deal_with_atomic_sequence (struct gdbarch *gdbarch, CORE_ADDR pc) > int index; > int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed). */ > const int atomic_sequence_length = 16; /* Instruction sequence length. */ > + bool is_mipsr6 = is_mipsr6_isa (gdbarch); OK, caching `is_mipsr6_isa (gdbarch)' for later use. > > insn = mips_fetch_instruction (gdbarch, ISA_MIPS, loc, NULL); > /* Assume all atomic sequences start with a ll/lld instruction. */ > - if (itype_op (insn) != LL_OPCODE && itype_op (insn) != LLD_OPCODE) > + if (!is_ll_insn (gdbarch, insn)) > return {}; OK, code factored out to `is_ll_insn'. > @@ -4021,28 +4403,72 @@ mips_deal_with_atomic_sequence (struct gdbarch *gdbarch, CORE_ADDR pc) > return {}; /* fallback to the standard single-step code. */ > case 4: /* BEQ */ > case 5: /* BNE */ > - case 6: /* BLEZ */ > - case 7: /* BGTZ */ > case 20: /* BEQL */ > case 21: /* BNEL */ > - case 22: /* BLEZL */ > - case 23: /* BGTTL */ > + case 22: /* BLEZL (BLEZC, BGEZC, BGEC) */ > + case 23: /* BGTZL (BGTZC, BLTZC, BLTC) */ OK, new operations added. Rewrite with no parentheses to match the style elsewhere. The BGTTL typo fix seems reasonable to fold into this change. > is_branch = 1; > break; > + case 6: /* BLEZ (BLEZALC, BGEZALC, BGEUC) */ > + case 7: /* BGTZ (BGTZALC, BLTZALC, BLTUC) */ OK, new operations added. Same note as to parentheses. However... > + if (is_mipsr6) > + { > + /* BLEZALC, BGTZALC */ > + if (itype_rs (insn) == 0 && itype_rt (insn) != 0) > + return {}; /* fallback to the standard single-step code. */ > + /* BGEZALC, BLTZALC */ > + else if (itype_rs (insn) == itype_rt (insn) > + && itype_rt (insn) != 0) > + return {}; /* fallback to the standard single-step code. */ > + } > + is_branch = 1; > + break; ... we don't special-case other branch-and-link instructions, why does this change do these? > + case 8: /* BOVC, BEQZALC, BEQC */ > + case 24: /* BNVC, BNEZALC, BNEC */ > + if (is_mipsr6) > + is_branch = 1; > + break; OK, new branches reusing non-branch pre-R6 opcodes. > + case 50: /* BC */ > + case 58: /* BALC */ > + if (is_mipsr6) > + return {}; /* fallback to the standard single-step code. */ > + break; OK, jump-like unconditional branches. Please s/fallback/Fall back/ for correct spelling though. > + case 54: /* BEQZC, JIC */ > + case 62: /* BNEZC, JIALC */ > + if (is_mipsr6) > + { > + if (itype_rs (insn) == 0) /* JIC, JIALC */ > + return {}; /* fallback to the standard single-step code. */ Likewise s/fallback/Fall back/. > + else Drop `else' since the `if' clause has already returned. > + is_branch = 2; /* Marker for branches with a 21-bit offset. */ Leave the current interpretation of `is_branch' alone and handle the two offset sizes with a separate variable. Update handling accordingly throughout. > + } > + break; > case 17: /* COP1 */ > - is_branch = ((itype_rs (insn) == 9 || itype_rs (insn) == 10) > - && (itype_rt (insn) & 0x2) == 0); > - if (is_branch) /* BC1ANY2F, BC1ANY2T, BC1ANY4F, BC1ANY4T */ > + is_branch = ((!is_mipsr6 > + /* BC1ANY2F, BC1ANY2T, BC1ANY4F, BC1ANY4T */ > + && (itype_rs (insn) == 9 || itype_rs (insn) == 10) > + && (itype_rt (insn) & 0x2) == 0) OK, branches removed from R6 and BC1EQZ replacing BC1ANY2F/BC1ANY2T is handled below. > + /* BZ.df: 010001 110xx */ > + || (itype_rs (insn) & 0x18) == 0x18); This is for the MSA module, already present in R5, so it needs to be submitted separately. Do you have complete MSA branch emulation support in the pipeline or is this the only piece actually implemented? > + if (is_branch != 0) > break; > [[fallthrough]]; > case 18: /* COP2 */ > case 19: /* COP3 */ > - is_branch = (itype_rs (insn) == 8); /* BCzF, BCzFL, BCzT, BCzTL */ > + /* BCzF, BCzFL, BCzT, BCzTL, BC*EQZ, BC*NEZ */ > + is_branch = ((itype_rs (insn) == 8) > + || (is_mipsr6 > + && (itype_rs (insn) == 9 > + || itype_rs (insn) == 13))); Spell out new branches as BCzEQZ/BCzNEZ to match the existing pattern. Otherwise OK. > - if (is_branch) > + if (is_branch != 0) > { > - branch_bp = loc + mips32_relative_offset (insn) + 4; > + /* Is this a special PC21_S2 branch? */ > + if (is_branch == 2) > + branch_bp = loc + mips32_relative_offset21 (insn) + 4; > + else > + branch_bp = loc + mips32_relative_offset (insn) + 4; Rewrite using the new variable as noted above. > @@ -4050,12 +4476,12 @@ mips_deal_with_atomic_sequence (struct gdbarch *gdbarch, CORE_ADDR pc) > last_breakpoint++; > } > > - if (itype_op (insn) == SC_OPCODE || itype_op (insn) == SCD_OPCODE) > + if (is_sc_insn (gdbarch, insn)) > break; > } > > /* Assume that the atomic sequence ends with a sc/scd instruction. */ > - if (itype_op (insn) != SC_OPCODE && itype_op (insn) != SCD_OPCODE) > + if (!is_sc_insn (gdbarch, insn)) > return {}; OK, code factored out to `is_sc_insn'. > @@ -4204,7 +4630,7 @@ micromips_deal_with_atomic_sequence (struct gdbarch *gdbarch, > } > break; > } > - if (is_branch) > + if (is_branch != 0) Gratuitous change. Overall `is_branch' should be changed to `bool' type separately, possibly in bulk with other such legacy variables. > @@ -4280,8 +4706,14 @@ mips_about_to_return (struct gdbarch *gdbarch, CORE_ADDR pc) > gdb_assert (mips_pc_is_mips (pc)); > > insn = mips_fetch_instruction (gdbarch, ISA_MIPS, pc, NULL); > - hint = 0x7c0; > - return (insn & ~hint) == 0x3e00008; /* jr(.hb) $ra */ > + /* Mask the hint and the jalr/jr bit. */ > + hint = 0x7c1; > + > + if (is_mipsr6_isa (gdbarch) && insn == 0xd81f0000) /* jrc $31 */ > + return 1; > + > + /* jr(.hb) $ra and "jalr(.hb) $ra" */ > + return ((insn & ~hint) == 0x3e00008); The JR vs JALR masking change is not related to R6 and needs to be a separate preparatory change. The `hint' variable is not named according to semantics anymore; I think we can just drop it and use the constant inline. The comment has to say: "jalr(.hb) $zero, $ra" since "jalr(.hb) $ra" are different instructions (0x3e0f809/0x3e0fc09 machine instructions), their unpredictable execution results notwithstanding. > @@ -6799,7 +7231,9 @@ mips32_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc) > > if (high_word != 0x27bd /* addiu $sp,$sp,offset */ > && high_word != 0x67bd /* daddiu $sp,$sp,offset */ > - && inst != 0x03e00008 /* jr $ra */ > + && (inst & ~0x1) != 0x03e00008 /* jr $31 or jalr $0, $31 */ Same note as to JR vs JALR as above. > + && (!is_mipsr6_isa (gdbarch) > + || inst != 0xd81f0000) /* jrc $31 */ Please use/retain symbolic register names through this hunk, and move comments to the next line if they can't be vertically aligned otherwise. > @@ -7177,6 +7611,7 @@ mips32_instruction_has_delay_slot (struct gdbarch *gdbarch, ULONGEST inst) > int op; > int rs; > int rt; > + bool is_mipsr6 = is_mipsr6_isa (gdbarch); Please move this to the top of the function block (reverse Xmas tree). > @@ -7184,15 +7619,23 @@ mips32_instruction_has_delay_slot (struct gdbarch *gdbarch, ULONGEST inst) > rs = itype_rs (inst); > rt = itype_rt (inst); > return (is_octeon_bbit_op (op, gdbarch) > - || op >> 2 == 5 /* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */ > - || op == 29 /* JALX: bits 011101 */ > + || (op >> 1 == 10) /* BEQL, BNEL: bits 01010x */ > + || (op >> 1 == 11 && rt == 0) /* BLEZL, BGTZL: bits 01011x */ Again, (rt == 0 || !is_mipsr6). Move comments to the next line if they can't be vertically aligned otherwise. > + || (!is_mipsr6 && op == 29) /* JALX: bits 011101 */ > || (op == 17 > && (rs == 8 > /* BC1F, BC1FL, BC1T, BC1TL: 010001 01000 */ > - || (rs == 9 && (rt & 0x2) == 0) > + || (!is_mipsr6 && rs == 9 && (rt & 0x2) == 0) > /* BC1ANY2F, BC1ANY2T: bits 010001 01001 */ > - || (rs == 10 && (rt & 0x2) == 0)))); > + || (!is_mipsr6 && rs == 10 && (rt & 0x2) == 0))) > /* BC1ANY4F, BC1ANY4T: bits 010001 01010 */ OK, instructions removed from R6. > + || (is_mipsr6 > + && ((op == 17 > + && (rs == 9 /* BC1EQZ: 010001 01001 */ > + || rs == 13)) /* BC1NEZ: 010001 01101 */ > + || (op == 18 > + && (rs == 9 /* BC2EQZ: 010010 01001 */ > + || rs == 13))))); /* BC2NEZ: 010010 01101 */ Repeated code: || (is_mipsr6 && ((op == 17 || op == 18) && (rs == 9 || rs == 13))) /* BC1EQZ/BC1NEZ: 010001 01x01 */ /* BC2EQZ/BC2NEZ: 010010 01x01 */ > @@ -7211,7 +7654,11 @@ mips32_instruction_has_delay_slot (struct gdbarch *gdbarch, ULONGEST inst) > || ((rt & 0x1e) == 0x1c && rs == 0)); > /* BPOSGE32, BPOSGE64: bits 1110x */ > break; /* end REGIMM */ > - default: /* J, JAL, BEQ, BNE, BLEZ, BGTZ */ > + case 6: /* BLEZ */ > + case 7: /* BGTZ */ Please align the comments vertically. > + return (itype_rt (inst) == 0); Again, (itype_rt (inst) == 0 || !is_mipsr6). > + break; > + default: /* J, JAL, BEQ, BNE */ Please align the comment vertically. > @@ -7423,7 +7870,18 @@ mips_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr) > > So, we'll use the second solution. To do this we need to know if > the instruction we're trying to set the breakpoint on is in the > - branch delay slot. */ > + branch delay slot. > + > + A similar problem occurs for breakpoints on forbidden slots where > + the trap will be reported for the branch with the BD bit set. > + In this case it would be ideal to recover using solution 1 from > + above as there is no problem with the branch being skipped > + (since the forbidden slot only exists on not-taken branches). > + However, the BD bit is not available in all scenarios currently > + so instead we move the breakpoint on to the next instruction. > + This means that it is not possible to stop on an instruction > + that can be in a forbidden slot even if that instruction is > + jumped to directly. */ I think it's worth mentioning that this breaks the contract written in the GDB manual, which says: "The breakpoint will stop your program just before it executes the instruction at the address of any of the breakpoint's code locations." Moving a breakpoint forwards means that at the time the breakpoint is hit the state of the program, other than just the PC, may have visibly changed compared to the state it was at at the time the PC pointed at the address corresponding to the relevant location requested. This can undoubtedly be confusing to a person chasing an issue with a debugging session, however I do believe it is no different from what happens with a compiler scheduling machine instructions belonging to one source line to the delay slot of a branch belonging to another. So how about: [...] This means that it is not possible to stop on an instruction that can be in a forbidden slot even if that instruction is jumped to directly. So by the time the breakpoint is hit the state of the program, other than just the PC, may have visibly changed compared to the state it was at at the time the PC pointed at the address corresponding to the relevant location requested. For accurate debugging it is therefore advised that any producer refrains from placing instructions other than NOP in forbidden slots. */ ? > @@ -7445,6 +7903,12 @@ mips_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr) > prev_addr = bpaddr - 4; > if (mips32_insn_at_pc_has_delay_slot (gdbarch, prev_addr)) > bpaddr = prev_addr; > + /* If the previous instruction has a forbidden slot, we have to > + move the breakpoint to the following instruction to prevent > + breakpoints in forbidden slots being reported as unknown > + traps. */ > + else if (mips32_insn_at_pc_has_forbidden_slot (gdbarch, prev_addr)) > + bpaddr += 4; So I think this is an acceptable compromise in the current state of affairs, but long-term we probably want to look into handling placing breakpoints in delay slots properly, by checking if access to CP0.Cause register is available, which should cover the majority of environments, and only falling back to the current approach if it's not. I've looked through the sources of GCC and there's currently no code there to prevent forbidden slot from being scheduled with instructions other than NOP, so it seems like something to look into too. I've filed GCC PR target/127449 to track this. > diff --git a/gdb/testsuite/gdb.arch/mips-64-r6.exp b/gdb/testsuite/gdb.arch/mips-64-r6.exp > new file mode 100644 > index 00000000000..fe5d6f58252 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/mips-64-r6.exp > @@ -0,0 +1,74 @@ > +# Copyright (C) 2023-2025 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, see . > + > +################################################################ > +################## MIPS Release 6 patch tests ################## > +################################################################ I've looked through the tests and they seem more like verifying the correctness of target execution rather than GDB operation. For example if a software-stepping breakpoint is placed at the wrong place and the test program runs away and completes successfully, then so is scored the test. I'd rather see tests that verify GDB operations instead. I've reused parts of the `stepi' procedure infrastructure though to implement single-stepping verification for pre-R6 branch instructions; cf. (). I think this can be expanded to cover R6 branches as well. That would be a good starting point. Please have a look into having LL/SC stepping code covered using the same infrastructure as well. I'm happy to see all the heuristics left uncovered, nobody should be relying on it nowadays. Please resubmit with the changes requested made. Maciej