Re: [PATCH^8] gdb: mips: Add MIPSR6 support

["Maciej W. Rozycki" <macro@orcam.me.uk>]

On Tue, 7 Jan 2025, Kevin Buettner wrote:

> > +/* Calculate address of next instruction after BLEZ.  */
> > +
> > +static CORE_ADDR
> > +mips32_blez_pc (struct gdbarch *gdbarch, struct regcache *regcache,
> > +	       ULONGEST inst, CORE_ADDR pc, int invert)
> > +{
> > +  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);
> > +  int taken = 0;
> > +  int delay_slot_size = 4;
> > +
> > +  /* BLEZ, BLEZL, BGTZ, BGTZL  */
> > +  if (rt == 0)
> > +    taken = (val_rs <= 0);
> > +  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);
> > +
> > +      /* 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.  */
> > +      /* delay_slot_size = 0;  */
> 
> Should "delay_slot_size = 0;" be commented out?  If so, I suggest
> removing the entire line.  And, if it's removed, the comment preceding
> it probably doesn't make sense either.

 I think the comment refers to the current situation rather than where 
`delay_slot_size' would be set to 0.

 The comment also seems at best misleading to me.  With hosted targets the 
BD bit is accessible via $cause register, which I do believe all the MIPS 
OSes supply unmodified.  With EJTAG targets or bare metal simulators the 
target description should supply $debug register providing the analogous 
DBD bit.  The backend could use that information to determine whether a 
breakpoint has hit in a delay slot; this has been considered before.

 However we have chosen to refrain from placing breakpoints in delay slots 
regardless, as that would complicate handling.  So it seems to me that the 
note should instead be rephrased accordingly and moved to the function's 
introductory comment...

> > +    }
> > +
> > +  if (invert)
> > +    taken = !taken;
> > +
> > +  /* Calculate branch target.  */

 ... and then "or skip over the forbidden" slot added here...

> > +  if (taken)
> > +    pc += mips32_relative_offset (inst);
> > +  else
> > +    pc += delay_slot_size;

 ... and then the otherwise unused variable removed (i.e. "pc += 4" here).

 Similarly in the other place, taking into account the slightly different 
context (so I think the comments ought to refer to delay slots in general 
rather than just the forbidden slots, which are a particular subset).

  Maciej