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

[Kevin Buettner <kevinb@redhat.com>]

Hi Milica,

I don't claim to know anything about MIPSR6, but I do see some
things which ought to be fixed before this patch goes in.  See
below...

On Fri, 8 Nov 2024 19:20:14 +0000
Milica Matic <milica.matic@htecgroup.com> wrote:

> 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.
> 
> Changes from v5: Sync with current master.
> 
> [1] "MIPS64 Architecture for Programmers Volume II-A: The MIPS64
>     Instruction Set Reference Manual", Document Number: MD00087,
>     Revision 6.06, December 15, 2016, Section 3 "The MIPS64
>     Instruction Set", pp. 42-530
> https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00087-2B-MIPS64BIS-AFP-6.06.pdf
> 
> 2024-05-15
> Andrew Bennett  <andrew.bennett@imgtec.com>
> Matthew Fortune  <matthew.fortune@mips.com>
> Faraz Shahbazker  <fshahbazker@wavecomp.com>
> 
> gdb/ChangeLog:
>         * mips-tdep.c (is_mipsr6_isa): New.
>         (b0s21_imm): New define.
>         (mips32_relative_offset21, mips32_relative_offset26): New.
>         (is_add32bit_overflow, is_add64bit_overflow): New.
>         (mips32_next_pc): Handle r6 compact and fpu coprocessor branches.
>         Move handling of BLEZ, BGTZ opcode into ...
>         (mips32_blez_pc): New.
>         (mips32_instruction_is_compact_branch): New.
>         (mips32_insn_at_pc_has_forbidden_slot):  New.
>         (mips32_scan_prologue): Ignore pre-r6 addi encoding on r6.
>         Stop at compact branch also.
>         (LLSC_R6_OPCODE,LL_R6_FUNCT,LLE_FUNCT,
>         LLD_R6_FUNCT,SC_R6_FUNCT,SCE_FUNCT,
>         SCD_R6_FUNCT: New defines.
>         (is_ll_insn, is_sc_insn): New.
>         (mips_deal_with_atomic_sequence): Use is_ll_insn/is_sc_insn.
>         Handle compact branches.
>         (mips_about_to_return): Handle jrc and macro jr.
>         (mips32_stack_frame_destroyed_p): Likewise.
>         (mips32_instruction_has_delay_slot): Don't handle JALX on r6.
>         Handle compact branches and coprocessor branches.
>         (mips_adjust_breakpoint_address): Skip forbidden slot for
>         compact branches.

It's okay to include ChangeLog entries as part of the commit log if
you want, but you should not try to place these in any of GDB's
ChangeLog files.  (The GDB project doesn't use ChangeLog files any
longer, though you'll still see ChangeLog-YYYY.  These are the old
ChangeLog files.)

My preference is to describe the highlights of what's changed and
why it's changed the way it is in the commit log.  For your commit,
you might describe new functions and how they're used by existing
code, but you can probably avoid mentioning new defines.

> ---
>  gdb/mips-tdep.c                       | 534 +++++++++++++--
>  gdb/testsuite/gdb.arch/mips-64-r6.c   | 918 ++++++++++++++++++++++++++
>  gdb/testsuite/gdb.arch/mips-64-r6.exp |  76 +++
>  3 files changed, 1484 insertions(+), 44 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.arch/mips-64-r6.c
>  create mode 100644 gdb/testsuite/gdb.arch/mips-64-r6.exp
> 
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index c00efbd02ad..173bfe0b8f8 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_byte *rare_buffer);
> +
>  /* A useful bit in the CP0 status register (MIPS_PS_REGNUM).  */
>  /* This bit is set if we are emulating 32-bit FPRs on a 64-bit chip.  */
>  #define ST0_FR (1 << 26)
> @@ -325,6 +329,16 @@ 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);
> +}
> +
>  /* MIPS16/microMIPS function addresses are odd (bit 0 is set).  Here
>     are some functions to handle addresses associated with compressed
>     code including but not limited to testing, setting, or clearing
> @@ -1496,6 +1510,7 @@ mips_fetch_instruction (struct gdbarch *gdbarch,
>    return extract_unsigned_integer (buf, instlen, byte_order);
>  }
> 
> +

You've introduced an extraneous blank line here.

>  /* These are the fields of 32 bit mips instructions.  */
>  #define mips32_op(x) (x >> 26)
>  #define itype_op(x) (x >> 26)
> @@ -1538,6 +1553,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)
>  #define b0s26_imm(x) ((x) & 0x3ffffff)
>  #define b6s10_ext(x) (((x) >> 6) & 0x3ff)
>  #define b11s5_reg(x) (((x) >> 11) & 0x1f)
> @@ -1574,6 +1590,18 @@ mips32_relative_offset (ULONGEST inst)
>    return ((itype_immediate (inst) ^ 0x8000) - 0x8000) << 2;
>  }
> 
> +static LONGEST
> +mips32_relative_offset21 (ULONGEST insn)
> +{
> +  return ((b0s21_imm (insn) ^ 0x100000) - 0x100000) << 2;
> +}
> +
> +static LONGEST
> +mips32_relative_offset26 (ULONGEST insn)
> +{
> +  return ((b0s26_imm (insn) ^ 0x2000000) - 0x2000000) << 2;
> +}

Please add brief descriptive comments before each of the above new
functions.  (I realize that there may be existing functions in
mips-tdep.c which do not have such comments.  It'd be nice if they
did, but you're only responsible for documenting new or changed
functions.)

> +
>  /* Determine the address of the next instruction executed after the INST
>     floating condition branch instruction at PC.  COUNT specifies the
>     number of the floating condition bits tested by the branch.  */
> @@ -1632,6 +1660,71 @@ is_octeon_bbit_op (int op, struct gdbarch *gdbarch)
>    return 0;
>  }
> 
> +static int
> +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);
> +}
> +
> +static int
> +is_add64bit_overflow (int64_t a, int64_t b)
> +{
> +  if (a != (int32_t)a)
> +    return 1;
> +  if (b != (int32_t)b)
> +    return 1;
> +  return is_add32bit_overflow ((int32_t)a, (int32_t)b);
> +}

Likewise for the above two functions.

> +
> +/* 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;  */
> +    }
> +
> +  if (invert)
> +    taken = !taken;
> +
> +  /* Calculate branch target.  */
> +  if (taken)
> +    pc += mips32_relative_offset (inst);
> +  else
> +    pc += delay_slot_size;
> +
> +  return pc;
> +}
> 
>  /* Determine where to set a single step breakpoint while considering
>     branch prediction.  */
> @@ -1642,12 +1735,17 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc)
>    struct gdbarch *gdbarch = regcache->arch ();
>    unsigned long inst;
>    int op;
> +  int mips64bitreg = 0;
> +
> +  if (mips_isa_regsize (gdbarch) == 8)
> +    mips64bitreg = 1;
> +
>    inst = mips_fetch_instruction (gdbarch, ISA_MIPS, pc, NULL);
>    op = itype_op (inst);
>    if ((inst & 0xe0000000) != 0)______________________________/* Not a special, jump or branch
>  _______________________________________________________________________________________   instruction.  */

For the above two lines and a bunch below, my email client replaces
non-UTF characters with '_'.  (Or something like that - it's an
annoyance to me, but in this case it seems to have flagged that
something funny is going on.)  Check your local sources and make
sure that tabs or spaces are used here.  It may just be that your
email client changed some things without your knowledge.  If you
use "git send-email", these problems will likely be avoided.

(I've stopped quoting the patch here, because I didn't notice anything
further that I should comment on.)

Kevin