[PATCH^8] gdb: mips: Add MIPSR6 support
Kevin Buettner
kevinb@redhat.com
Tue Jan 7 20:49:21 GMT 2025
Hi Milica,
While applying your patch locally, I saw:
warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.
I suggest doing a "git am" using the patch that you sent to the list
to identify them.
Additional comments inline, below...
On Fri, 13 Dec 2024 20:29:05 +0000
Milica Matic <milica.matic@htecgroup.com> 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.
> + }
> +
> + 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,58 +1751,66 @@ 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. */
> + if ((inst & 0xe0000000) != 0) /* Not a special, jump or branch instruction. */
> + {
> + if (op >> 2 == 5 && ((op & 0x02) == 0 || itype_rt (inst) == 0))
> + /* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
> {
> - if (op >> 2 == 5)
> - /* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
> + switch (op & 0x03)
> {
> - switch (op & 0x03)
> - {
> - case 0: /* BEQL */
> - goto equal_branch;
> - case 1: /* BNEL */
> - goto neq_branch;
> - case 2: /* BLEZL */
> - goto less_branch;
> - case 3: /* BGTZL */
> - goto greater_branch;
> - default:
> - pc += 4;
> - }
> + case 0: /* BEQL */
> + goto equal_branch;
> + case 1: /* BNEL */
> + goto neq_branch;
> + case 2: /* BLEZL */
> + goto lez_branch;
> + case 3: /* BGTZL */
> + goto greater_branch;
> + default:
> + pc += 4;
> }
> + }
The indentation here somehow got messed up here. After applying your
patch, I see:
if ((inst & 0xe0000000) != 0) /* Not a special, jump or branch instruction. */
{
if (op >> 2 == 5 && ((op & 0x02) == 0 || itype_rt (inst) == 0))
/* BEQL, BNEL, BLEZL, BGTZL: bits 0101xx */
{
switch (op & 0x03)
{
case 0: /* BEQL */
goto equal_branch;
case 1: /* BNEL */
goto neq_branch;
case 2: /* BLEZL */
goto lez_branch;
case 3: /* BGTZL */
goto greater_branch;
default:
pc += 4;
}
}
Note that the second '{' and the block that goes along with it are not
correctly indented for the 'if' statement just above it. I believe
that there are further indentation problems later on too; I'll point
out a few of them, but I'll leave it to you to find and fix the rest.
[...]
> @@ -1763,22 +2000,38 @@ mips32_next_pc (struct regcache *regcache, CORE_ADDR pc)
> pc += 8; /* after the delay slot */
> break;
> case 0x1c: /* BPOSGE32 */
> + case 0x1d: /* BPOSGE32C */
Indentation is off by one space here. This is what I see after applying your patch:
case 0x1c: /* BPOSGE32 */
case 0x1d: /* BPOSGE32C */
case 0x1e: /* BPOSGE64 */
> 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;
Likewise. (Off by one space.)
>
> if (dspctl == -1)
> /* No way to handle; it'll most likely trap anyway. */
> break;
>
> + /* BPOSGE32C */
> + if (op == 0x1d)
> + {
> + if (!is_mipsr6_isa (gdbarch))
> + break;
> +
> + /* 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; */
> + }
Likewise, for the above block.
> +
> if ((regcache_raw_get_unsigned (regcache,
> dspctl) & 0x7f) >= pos)
> pc += mips32_relative_offset (inst);
> else
> - pc += 4;
> + pc += delay_slot_size;
Likewise.
> }
> break;
> /* All of the other instructions in the REGIMM category */
> @@ -1812,19 +2065,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 */
> + lez_branch:
> + pc = mips32_blez_pc (gdbarch, regcache, inst, pc + 4, 0);
Likewise.
> 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 */
> + pc = mips32_blez_pc (gdbarch, regcache, inst, pc + 4, 1);
Likewise.
[...]
> +/* 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)
> +{
> + switch (itype_op (insn))
> + {
> + /* BC */
> + case 50:
> + /* BALC */
> + case 58:
> + if (is_mipsr6_isa (gdbarch))
> + return 1;
> + break;
> + /* BOVC, BEQZALC, BEQC */
> + case 8:
> + /* BNVC, BNEZALC, BNEC */
> + case 24:
> + if (is_mipsr6_isa (gdbarch))
Up to this point, indentation appears correct for the new function
'mips32_instruction_is_compact_branch', but...
> + return 2;
It's wrong for the line above. That line is indented by 7 spaces, but it should be 8, which'll
turn it into a tab.
> + break;
> + /* BEQZC, JIC */
> + case 54:
> + /* BNEZC, JIALC */
> + case 62:
> + if (is_mipsr6_isa (gdbarch))
> + /* JIC, JIALC are unconditional */
> + return (itype_rs (insn) == 0) ? 1 : 2;
Likewise for the two lines above. (7 spaces are used instead of 8 which converts to a tab.)
> + break;
> + /* BLEZC, BGEZC, BGEC */
> + case 22:
> + /* BGTZC, BLTZC, BLTC */
> + case 23:
> + /* BLEZALC, BGEZALC, BGEUC */
> + case 6:
> + /* BGTZALC, BLTZALC, BLTUC */
> + case 7:
> + if (is_mipsr6_isa (gdbarch)
> + && itype_rt (insn) != 0)
> + return 2;
Likewise for the "return 2;" line.
> + break;
> + /* BPOSGE32C */
> + case 1:
> + if (is_mipsr6_isa (gdbarch)
> + && itype_rt (insn) == 0x1d && itype_rs (insn) == 0)
> + return 2;
And here too.
[...]
> static CORE_ADDR
> @@ -3490,7 +3800,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))
> || high_word == 0x67bd) /* daddiu $sp,$sp,-i */
Indentation problem here too. The '||' line that you added should have the same indentation
level as the '|| high_word == 0x67bd) /* daddiu $sp,$sp,-i */' line.
Okay, so I'm going to stop here. Scanning ahead, I saw other indentation problems which
are similar to those mentioned above. I'll leave it to you to find and fix them...
I did notice that you fixed some of the existing whitespace problems in this file, including
removal of extraneous blank lines and spaces at the end of a line. Thanks for doing that!
Kevin
More information about the Gdb-patches
mailing list