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

[Simon Marchi <simon.marchi@polymtl.ca>]

Hi Dragan,

I can do a superficial review (formatting / coding style / general
feeling), but can't speak about the behavior.  Ideally Maciej would look
at it, but if he's not available, I would be fine approving the patch,
since it's isolated to the MIPS support.  The code looks nice and
well-organized.

Not absolutely necessary, but it would be nice to have tests
(arch-specific tests in assembly in gdb.arch) for stepping and putting
breakpoints on those instructions that you add support for.

See superficial comments below.

Simon

On 10/12/22 14:02, Dragan Mladjenovic wrote:
> @@ -1633,6 +1660,70 @@ is_octeon_bbit_op (int op, struct gdbarch *gdbarch)
>    return 0;
>  }
>  
> +/* Return true if addition produces 32-bit overflow.  */
> +
> +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);
> +}
> +
> +/* Return true if addition produces 32-bit overflow or
> +   one of the inputs is not sign-extended 32-bit value.  */
> +
> +static bool
> +is_add64bit_overflow (int64_t a, int64_t b)
> +{
> +  if (a != (int32_t) a)
> +    return 1;
> +  if (b != (int32_t) b)
> +    return 1;

return true

> @@ -1704,7 +1801,119 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc)
>  	  else
>  	    pc += 8;        /* After the delay slot.  */
>  	}
> +      else if (is_mipsr6_isa (gdbarch))
> +	{
> +	  /* BOVC, BEQZALC, BEQC and BNVC, BNEZALC, BNEC */
> +	  if (op == 8 || op == 24)
> +	    {
> +	      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;
> +	      /* BOVC (BNVC) */
> +	      if (rs >= rt)
> +		{
> +		  if (mips64bitreg)
> +		    taken = is_add64bit_overflow (val_rs, val_rt);
> +		  else
> +		    taken = is_add32bit_overflow (val_rs, val_rt);
> +		}
> +	      else if (rs < rt && rs == 0)
> +	      /* BEQZALC (BNEZALC) */
> +		taken = (val_rt == 0);

Because of the comment, you would need to add braces here.

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

So we would typically write:

  else if (rs < rt && rs == 0)
    {
      /* BEQZALC (BNEZALC) */
      taken = (val_rt == 0);
    }

But I realize this may clash with the existing style used in the file,
which deviates a bit from the rest of GDB.  So I'm also fine with
leaving it like that on the account of following surrounding style.

> @@ -2448,6 +2659,72 @@ micromips_instruction_is_compact_branch (unsigned short insn)
>      }
>  }
>  
> +/* 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
> +mips32_instruction_is_compact_branch (struct gdbarch *gdbarch, ULONGEST insn)

Instead of hardcoded integer values, can you change this function to
make it return an enum?

Simon