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

[Kevin Buettner <kevinb@redhat.com>]

Hi Milica,

While things are looking much better, I still see some formatting nits. 
See below.

Kevin

On Wed, 22 Jan 2025 21:06:44 +0000
Milica Matic <milica.matic@htecgroup.com> wrote:

> @@ -1074,21 +1090,26 @@ mips_register_type (struct gdbarch *gdbarch, int regnum)
>        else if (gdbarch_osabi (gdbarch) != GDB_OSABI_LINUX
>  	       && rawnum >= MIPS_FIRST_EMBED_REGNUM
>  	       && rawnum <= MIPS_LAST_EMBED_REGNUM)
> +	{
>  	/* The pseudo/cooked view of the embedded registers is always
>  	   32-bit.  The raw view is handled below.  */
> -	return builtin_type (gdbarch)->builtin_int32;
> +	  return builtin_type (gdbarch)->builtin_int32;
> +	}

You've added curly braces here and have correctly indented the return
statement, but forgot to also indent the comment to match the
indentation level of the return statement.

>        else if (tdep->mips64_transfers_32bit_regs_p)
> +	{
>  	/* The target, while possibly using a 64-bit register buffer,
>  	   is only transferring 32-bits of each integer register.
>  	   Reflect this in the cooked/pseudo (ABI) register value.  */
> -	return builtin_type (gdbarch)->builtin_int32;
> +	  return builtin_type (gdbarch)->builtin_int32;
> +	}

Likewise.

>        else if (mips_abi_regsize (gdbarch) == 4)
> +	{
>  	/* The ABI is restricted to 32-bit registers (the ISA could be
>  	   32- or 64-bit).  */
> -	return builtin_type (gdbarch)->builtin_int32;
> +	  return builtin_type (gdbarch)->builtin_int32;
> +	}

Here too.

>        else
> -	/* 64-bit ABI.  */
> -	return builtin_type (gdbarch)->builtin_int64;
> +	return builtin_type (gdbarch)->builtin_int64; /* 64-bit ABI.  */

For this one, I think you should leave the comment on a line of its own
and add the curly braces.  Rationale:

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Prefer_Function-Body_Comments_On_Lines_By_Themselves

> @@ -1593,8 +1636,10 @@ mips32_bc1_pc (struct gdbarch *gdbarch, struct regcache *regcache,
>    int cond;
>  
>    if (fcsr == -1)
> -    /* No way to handle; it'll most likely trap anyway.  */
> -    return pc;
> +    {
> +      /* No way to handle; it'll most likely trap anyway.  */
> +      return pc;
> +    }

This one is correct - do the ones above like this one.

> +/* Detects whether overflow occurs when adding two 32-bit integers.  */
> +
> +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);

FWIW, for the ">= 0" cases on the line above, I don't think that
overflow can occur when either a or b is 0.

> @@ -1645,14 +1760,18 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc)
>    struct gdbarch *gdbarch = regcache->arch ();
>    unsigned long inst;
>    int op;
> +  int mips64bitreg = 0;

I think that the type of mips64bitreg could be 'bool' instead of 'int'.

> +
> +  if (mips_isa_regsize (gdbarch) == 8)
> +    mips64bitreg = 1;

And, of course, you'd then use 'true' here instead of '1'.

> +	      if (taken)
> +		 pc += mips32_relative_offset (inst) + 4;
> +	      else
> +		{
> +		/* Step through the forbidden slot to avoid repeated exceptions

It seems like there ought to be some kind of punctuation (a period, hyphen,
or semicolon) after "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;
> +		}
> +	    }
> +	  else if (op == 50 || op == 58)
> +	    {
> +	      /* BC, BALC  */
> +	      pc += mips32_relative_offset26 (inst) + 4;
> +	    }
> +	  else if ((op == 54 || op == 62)
> +		   && rtype_rs (inst) == 0)
> +	    {
> +	      /* JIC, JIALC  */
> +	      pc = regcache_raw_get_signed (regcache, itype_rt (inst));
> +	      pc += (itype_immediate (inst) ^ 0x8000) - 0x8000;
> +	    }
> +	  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)
> +		pc += mips32_relative_offset21 (inst) + 4;
> +	      else
> +		{
> +		/* Step through the forbidden slot to avoid repeated exceptions

Likewise, here.

> +		   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;
> +		}
> +	    }
> +	  else
> +	    {
> +	      /* Not a branch, next instruction is easy.  */
> +	      pc += 4;
> +	    }
> +	}
>        else
>  	pc += 4;		/* Not a branch, next instruction is easy.  */
>      }
>    else
> -    {				/* This gets way messy.  */
> -
> -      /* Further subdivide into SPECIAL, REGIMM and other.  */
> +    {  /* This gets way messy.
> +	  Further subdivide into SPECIAL, REGIMM and other.  */

I think that the comment should be on a line of its own instead of on
the same line as the left curly brace.  As is, I suspect that the
indentation for the following line isn't quite right either, but
that'll presumably get fixed if you separate the comment from the
curly brace line.

> @@ -1766,22 +2037,33 @@ 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)
>  		  {
>  		    unsigned int pos = (op & 2) ? 64 : 32;
>  		    int dspctl = mips_regnum (gdbarch)->dspctl;
> +		    int delay_slot_size = 4;

I'm guessing that you're introducing delay_slot_size here so that
things are more readable later on when doing "pc += delay_slot_size;"
(instead of "pc += 4") later on.  It appears that it's never modified,
nor should it be.  So, maybe make it a "const int" instead of "int" ?

When I search for "delay_slot_size" in the rest of your patch, I see
that 'delay_slot_size' is also defined in mips32_blez_pc also, so
perhaps 'delay_slot_size" should be at the global (file-scope) level
and should be defined via either #define or const ? (That way you're
not defining it repeatedly in each function where it's needed.)

>  
>  		    if (dspctl == -1)
> +		      {
>  		      /* No way to handle; it'll most likely trap anyway.  */
> -		      break;
> +			break;
> +		      }

I think that the comment in the block above should be indented so that
it's at the same indentation level as the break.

> +
> +		    /* BPOSGE32C  */
> +		    if (op == 0x1d)
> +		      {
> +			if (!is_mipsr6_isa (gdbarch))
> +			  break;
> +		      }
>  
>  		    if ((regcache_raw_get_unsigned (regcache,
>  						    dspctl) & 0x7f) >= pos)
>  		      pc += mips32_relative_offset (inst);
>  		    else
> -		      pc += 4;
> +		      pc += delay_slot_size;

Here's where delay_slot_size is used.  FWIW, I like this change since
it make it clear what's going on without the need for a comment.

>  		  }
>  		break;
>  		/* All of the other instructions in the REGIMM category */
> @@ -1789,9 +2071,9 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc)
>  		pc += 4;
>  	      }
>  	  }
> -	  break;		/* end REGIMM */
> -	case 2:		/* J */
> -	case 3:		/* JAL */
> +	  break;  /* end REGIMM */
> +	case 2: 	/* J */
> +	case 3: 	/* JAL */

The above seems to replace some tabs with spaces prior to the comment.
In particular, the case statements originally had two tabs and they
now have a space and then a tab.  I'm wondering about why this was done?

I don't think the GDB coding standard addresses this case though, so
I'm not asking you to change it.  (Though I will say that changes like this
make the patch harder to review.)

> @@ -2600,10 +2940,12 @@ mips16_scan_prologue (struct gdbarch *gdbarch,
>  	  if (offset < 0)	/* Negative stack adjustment?  */
>  	    frame_offset -= offset;
>  	  else
> +	    {
>  	    /* Exit loop if a positive stack adjustment is found, which
>  	       usually means that the stack cleanup code in the function
>  	       epilogue is reached.  */
> -	    break;
> +	      break;
> +	    }

I think this is another place where the comment needs to be indented
to the same level as the code enclosed within the curly braces.

>  	}
>        else if ((inst & 0xf800) == 0xd000)	/* sw reg,n($sp) */
>  	{
> @@ -2888,7 +3230,7 @@ mips_insn16_frame_cache (const frame_info_ptr &this_frame, void **this_cache)
>      mips16_scan_prologue (gdbarch, start_addr, pc, this_frame,
>  			  (struct mips_frame_cache *) *this_cache);
>    }
> -  
> +
>    /* gdbarch_sp_regnum contains the value and not the address.  */
>    cache->saved_regs[gdbarch_num_regs (gdbarch)
>  		    + MIPS_SP_REGNUM].set_value (cache->base);
> @@ -3063,7 +3405,7 @@ micromips_scan_prologue (struct gdbarch *gdbarch,
>  		  && dreg == MIPS_SP_REGNUM && sreg == MIPS_SP_REGNUM
>  		  && treg == 3)
>  				/* (D)SUBU $sp, $v1 */
> -		    sp_adj = v1_off;
> +		sp_adj = v1_off;
>  	      else if (op != 0x150
>  				/* ADDU: bits 000000 00101010000 */
>  				/* DADDU: bits 010110 00101010000 */
> @@ -3292,7 +3634,7 @@ micromips_scan_prologue (struct gdbarch *gdbarch,
>  
>  /* Heuristic unwinder for procedures using microMIPS instructions.
>     Procedures that use the 32-bit instruction set are handled by the
> -   mips_insn32 unwinder.  Likewise MIPS16 and the mips_insn16 unwinder. */
> +   mips_insn32 unwinder.  Likewise MIPS16 and the mips_insn16 unwinder.  */
>  
>  static struct mips_frame_cache *
>  mips_micro_frame_cache (const frame_info_ptr &this_frame, void **this_cache)
> @@ -3425,7 +3767,7 @@ reset_saved_regs (struct gdbarch *gdbarch, struct mips_frame_cache *this_cache)
>  }
>  
>  /* Analyze the function prologue from START_PC to LIMIT_PC.  Builds
> -   the associated FRAME_CACHE if not null.  
> +   the associated FRAME_CACHE if not null.
>     Return the address of the first instruction past the prologue.  */
>  
>  static CORE_ADDR
> @@ -3493,16 +3835,19 @@ 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))

I think this should be indented so that the leading '&' appears under
the 'h' in "high_word".

>  	  || high_word == 0x67bd)	/* daddiu $sp,$sp,-i */
>  	{
>  	  if (offset < 0)		/* Negative stack adjustment?  */
>  	    frame_offset -= offset;
>  	  else
> -	    /* Exit loop if a positive stack adjustment is found, which
> -	       usually means that the stack cleanup code in the function
> -	       epilogue is reached.  */
> -	    break;
> +	    {
> +	     /* Exit loop if a positive stack adjustment is found, which
> +		usually means that the stack cleanup code in the function
> +		epilogue is reached.  */
> +	      break;
> +	    }

The first line of the comment in the block above is indented by one
tab and five spaces.  It should be one tab and six spaces, in order to
match the break below it.  I think that the other two lines of the
comment will require some adjustment too.

> @@ -3981,28 +4394,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)  */
>  	  is_branch = 1;
>  	  break;
> +	case 6: /* BLEZ (BLEZALC, BGEZALC, BGEUC)  */
> +	case 7: /* BGTZ (BGTZALC, BLTZALC, BLTUC)  */
> +	  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;
> +	case 8: /* BOVC, BEQZALC, BEQC  */
> +	case 24: /* BNVC, BNEZALC, BNEC  */
> +	  if (is_mipsr6)
> +	    is_branch = 1;
> +	  break;
> +	case 50: /* BC  */
> +	case 58: /* BALC  */
> +	  if (is_mipsr6)
> +	    return {}; /* fallback to the standard single-step code.  */
> +	  break;
> +	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.  */
> +	      else
> +		is_branch = 2; /* Marker for branches with a 21-bit offset.  */
> +	    }
> +	  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)
> +		      /* BZ.df:  010001 110xx  */
> +		       || (itype_rs (insn) & 0x18) == 0x18);

Check the indentation of the comments for the expression of the
is_branch assignment, above.  Indentation of the rest of it looks
correct to me though.

> @@ -7352,7 +7826,7 @@ mips_segment_boundary (CORE_ADDR bpaddr)
>  }
>  
>  /* Move the breakpoint at BPADDR out of any branch delay slot by shifting
> -   it backwards if necessary.  Return the address of the new location.  */
> +   it backwards if necessary. Return the address of the new location.  */

The original line was correct.  There should be two spaces (instead of
just one) following the period after "necessary".  See:

  https://www.gnu.org/prep/standards/standards.html#Comments

> @@ -7404,10 +7889,16 @@ mips_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
>  	return bpaddr;
>  
>        /* If the previous instruction has a branch delay slot, we have
> -	 to move the breakpoint to the branch instruction. */
> +	 to move the breakpoint to the branch instruction.  */
>        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.  */

Check indentation of the three comment lines above.  As written, I don't
think that they line up under the 'I' in "If" on the first line of the
comment.

> @@ -7437,18 +7928,23 @@ mips_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
>  	    break;
>  	  addr -= MIPS_INSN16_SIZE;
>  	  if (i == 1 && insn_at_pc_has_delay_slot (gdbarch, addr, 0))
> +	    {
>  	    /* Looks like a JR/JALR at [target-1], but it could be
>  	       the second word of a previous JAL/JALX, so record it
>  	       and check back one more.  */
> -	    jmpaddr = addr;
> +	      jmpaddr = addr;
> +	    }

Another case where the comment needs to be indented within the block.

>  	  else if (i > 1 && insn_at_pc_has_delay_slot (gdbarch, addr, 1))
>  	    {
>  	      if (i == 2)
> +		{
>  		/* Looks like a JAL/JALX at [target-2], but it could also
>  		   be the second word of a previous JAL/JALX, record it,
>  		   and check back one more.  */
> -		jmpaddr = addr;
> +		  jmpaddr = addr;
> +		}

Same here.

>  	      else
> +		{
>  		/* Looks like a JAL/JALX at [target-3], so any previously
>  		   recorded JAL/JALX or JR/JALR must be wrong, because:
>  
> @@ -7466,7 +7962,8 @@ mips_adjust_breakpoint_address (struct gdbarch *gdbarch, CORE_ADDR bpaddr)
>  		    -2: bdslot (can't be jmp)
>  		    -1: JR/JALR
>  		     0: target insn  */
> -		jmpaddr = 0;
> +		  jmpaddr = 0;
> +		}

And here too.

>  	    }
>  	  else
>  	    {
> @@ -7733,15 +8230,19 @@ mips_skip_mips16_trampoline_code (const frame_info_ptr &frame, CORE_ADDR pc)
>  	       && mips_is_stub_suffix (name + prefixlen + 3, 0))
>  	{
>  	  if (pc == start_addr)
> +	    {
>  	    /* This is the 'call' part of a call stub.  The return
>  	       address is in $2.  */
> -	    return get_frame_register_signed
> +	      return get_frame_register_signed
>  		     (frame, gdbarch_num_regs (gdbarch) + MIPS_V0_REGNUM);
> +	    }

Likewise.

>  	  else
> +	    {
>  	    /* This is the 'return' part of a call stub.  The return
>  	       address is in $18.  */
> -	    return get_frame_register_signed
> +	      return get_frame_register_signed
>  		     (frame, gdbarch_num_regs (gdbarch) + MIPS_S2_REGNUM);
> +	    }
>  	}

Likewise.

>        else
>  	return 0;		/* Not a stub.  */
> @@ -7753,15 +8254,19 @@ mips_skip_mips16_trampoline_code (const frame_info_ptr &frame, CORE_ADDR pc)
>        || startswith (name, mips_str_call_stub))
>      {
>        if (pc == start_addr)
> +	{
>  	/* This is the 'call' part of a call stub.  Call this helper
>  	   to scan through this code for interesting instructions
>  	   and determine the final PC.  */
> -	return mips_get_mips16_fn_stub_pc (frame, pc);
> +	  return mips_get_mips16_fn_stub_pc (frame, pc);
> +	}

Likewise.

>        else
> +	{
>  	/* This is the 'return' part of a call stub.  The return address
>  	   is in $18.  */
> -	return get_frame_register_signed
> +	  return get_frame_register_signed
>  		 (frame, gdbarch_num_regs (gdbarch) + MIPS_S2_REGNUM);
> +	}

Likewise.