Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: "Maciej W. Rozycki" <macro@orcam.me.uk>
To: Jovan Dmitrovic <jovan.dmitrovic@htecgroup.com>
Cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>,
	 Djordje Todorovic <Djordje.Todorovic@htecgroup.com>,
	 Milica Matic <milica.matic@htecgroup.com>,
	 "Maciej W. Rozycki" <macro@globalfoundries.com>
Subject: Re: [PATCH v13 2/2] gdb: mips: Add MIPSR6 support
Date: Thu, 17 Sep 2026 12:22:00 +0100 (BST)	[thread overview]
Message-ID: <alpine.DEB.2.21.2608191955570.14132@angie.orcam.me.uk> (raw)
In-Reply-To: <20250604123838.501596-3-jovan.dmitrovic@htecgroup.com>

On Wed, 4 Jun 2025, Jovan Dmitrovic wrote:

> From: Milica Matic <milica.matic@htecgroup.com>

 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 <milica.matic@htecgroup.com>
> Signed-off-by: Jovan Dmitrović <jovan.dmitrovic@htecgroup.com>

 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<gdb_byte> 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<gdb_byte> 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 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=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 <http://www.gnu.org/licenses/>.
> +
> +################################################################
> +################## 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. 
<https://patchwork.sourceware.org/project/gdb/list/?series=65135> 
(<https://inbox.sourceware.org/gdb-patches/alpine.DEB.2.21.2608161728490.14132@angie.orcam.me.uk/>).  
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

      reply	other threads:[~2026-09-17 11:23 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-04 12:38 [PATCH v13 0/2] " Jovan Dmitrovic
2025-06-04 12:39 ` [PATCH v13 1/2] gdb: mips: Apply coding guidelines Jovan Dmitrovic
2026-09-17 11:21   ` Maciej W. Rozycki
2025-06-04 12:39 ` [PATCH v13 2/2] gdb: mips: Add MIPSR6 support Jovan Dmitrovic
2026-09-17 11:22   ` Maciej W. Rozycki [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.21.2608191955570.14132@angie.orcam.me.uk \
    --to=macro@orcam.me.uk \
    --cc=Djordje.Todorovic@htecgroup.com \
    --cc=gdb-patches@sourceware.org \
    --cc=jovan.dmitrovic@htecgroup.com \
    --cc=macro@globalfoundries.com \
    --cc=milica.matic@htecgroup.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox