Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

Gopi Kumar Bulusu
2025-09-04T12:16:39Z

[-- Attachment #1: Type: text/plain, Size: 6251 bytes --]

Hello Mike,

On Wed, Sep 3, 2025 at 11:32 PM Michael Eager <eager@eagercon.com> wrote:

> Please submit revised patch.
>

Working on it.

I will push the patch v3 and post it here as well.

dhanyavaadaaha
gopi

> On 9/3/25 10:57 AM, Simon Marchi wrote:
> > On 9/2/25 8:27 AM, Gopi Kumar Bulusu wrote:
> >> From 7f2bacc6fffa6830991da36c2044431237bacba1 Mon Sep 17 00:00:00 2001
> >> From: Gopi Kumar Bulusu <gopi@sankhya.com>
> >> Date: Tue, 12 Aug 2025 09:42:48 +0530
> >> Subject: [PATCH v2] MicroBlaze: Add microblaze_get_next_pcs
> >>
> >> This patch enables software single stepping for gdbserver target
> >>
> >> * gdb/microblaze-tdep.c: Add microblaze_get_next_pcs
> >>
> >> Signed-off-by: David Holsgrove <david.holsgrove@petalogix.com>
> >> Signed-off-by: Nathan Rossi <nathan.rossi@petalogix.com>
> >> Signed-off-by: Mahesh Bodapati <mbodapat@xilinx.com>
> >> Signed-off-by: Gopi Kumar Bulusu <gopi@sankhya.com>
> >> ---
> >> gdb/microblaze-tdep.c | 91 +++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 91 insertions(+)
> >>
> >> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
> >> index 7b58220871c..cacec8ba186 100644
> >> --- a/gdb/microblaze-tdep.c
> >> +++ b/gdb/microblaze-tdep.c
> >> @@ -590,6 +590,95 @@ microblaze_stabs_argument_has_addr (struct gdbarch
> *gdbarch, struct type *type)
> >> return (type->length () == 16);
> >> }
> >>
> >> +/* Return next pc values : next in sequence and/or branch/return
> target. */
> >> +
> >> +static std::vector<CORE_ADDR>
> >> +microblaze_get_next_pcs (regcache *regcache)
> >> +{
> >> + CORE_ADDR pc = regcache_read_pc (regcache);
> >> + long insn = microblaze_fetch_instruction (pc);
> >> +
> >> + enum microblaze_instr_type insn_type;
> >> + short delay_slots;
> >> + bool isunsignednum;
> >> +
> >> + /* If the current instruction is an imm, look at the inst after. */
> >> +
> >> + get_insn_microblaze (insn, &isunsignednum, &insn_type, &delay_slots);
> >> +
> >> + int imm;
> >> + bool immfound = false;
> >> +
> >> + if (insn_type == immediate_inst)
> >> + {
> >> + int rd, ra, rb;
> >> + immfound = true;
> >> + microblaze_decode_insn (insn, &rd, &ra, &rb, &imm);
> >> + pc += INST_WORD_SIZE;
> >> + insn = microblaze_fetch_instruction (pc);
> >> + get_insn_microblaze (insn, &isunsignednum, &insn_type,
> &delay_slots);
> >> + }
> >> +
> >> + std::optional<CORE_ADDR> next_pc, branch_or_return_pc;
> >> +
> >> + /* Compute next instruction address - skip delay slots if any. */
> >> +
> >> + if (insn_type != return_inst)
> >> + next_pc = pc + INST_WORD_SIZE + (delay_slots * INST_WORD_SIZE);
> >> +
> >> + microblaze_debug ("single-step insn_type=0x%x pc=0x%lx insn=0x%lx\n",
> >> + insn_type, pc, insn);
> >
> > You shouldn't need the trailing \n. Apply elsewhere too.
> >
> >> +
> >> + /* Compute target instruction address for branch or return
> instruction. */
> >> + if (insn_type == branch_inst || insn_type == return_inst)
> >> + {
> >> + int limm;
> >> + int lrd, lra, lrb;
> >> + long ra, rb;
> >> + bool targetvalid;
> >> + bool unconditionalbranch;
> >
> > Declare when first use, where possible (ra and rb).
> >
> > ra and rb should probably be of the type ULONGEST.
> >
> >> +
> >> + microblaze_decode_insn (insn, &lrd, &lra, &lrb, &limm);
> >> + ra = regcache_raw_get_unsigned (regcache, lra);
> >> + rb = regcache_raw_get_unsigned (regcache, lrb);
> >> +
> >> + branch_or_return_pc = microblaze_get_target_address (insn,
> immfound,
> >> + imm, pc, ra, rb, &targetvalid, &unconditionalbranch);
> >
> > Format like this:
> >
> > branch_or_return_pc
> > = microblaze_get_target_address (insn, immfound,
> > imm, pc, ra, rb, &targetvalid,
> > &unconditionalbranch);
> >
> >> +
> >> + microblaze_debug (
> >> + "single-step uncondbr=%d targetvalid=%d
> target=0x%lx\n",
> >> + unconditionalbranch, targetvalid,
> >> + branch_or_return_pc.value () );
> >
> > Format like this:
> >
> > microblaze_debug ("single-step uncondbr=%d targetvalid=%d
> target=0x%lx",
> > unconditionalbranch, targetvalid,
> > branch_or_return_pc.value ());
> >
> >> +
> >> + /* Can't reach next address. */
> >> + if (unconditionalbranch)
> >> + next_pc.reset ();
> >> +
> >> + /* Can't reach a distinct (not here) target address. */
> >> + if (! targetvalid || branch_or_return_pc == pc ||
> >> + (next_pc && (branch_or_return_pc == next_pc)))
> >> + branch_or_return_pc.reset ();
> >
> > Format like this:
> >
> > /* Can't reach a distinct (not here) target address. */
> > if (!targetvalid
> > || branch_or_return_pc == pc
> > || (next_pc.has_value () && branch_or_return_pc == next_pc))
> > branch_or_return_pc.reset ();
> >
> >
> >> + } /* if (branch or return instruction). */
> >> +
> >> + /* Create next_pcs vector to return. */
> >> +
> >> + std::vector<CORE_ADDR> next_pcs;
> >> +
> >> + if (next_pc)
> >
> > .has_value ()
> >
> >> + {
> >> + next_pcs.push_back (next_pc.value () );
> >
> > We typically use `*next_pc` to access the value (apply elsewhere too).
> >
> > Remove the space before closing parenthesis (apply elsewhere too).
> >
> >> + microblaze_debug ("push_back next_pc(0x%lx)\n", next_pc.value ()
> );
> >> + }
> >> +
> >> + if (branch_or_return_pc)
> >> + {
> >> + next_pcs.push_back ( branch_or_return_pc.value () );
> >> + microblaze_debug ("push_back branch_or_return_pc(0x%lx)\n",
> >> + branch_or_return_pc.value ());
> >
> > Align the last line properly.
> >
> > The patch LGTM with those fixed. Let me know if you need help pushing
> > the patch.
> >
> > Approved-By: Simon Marchi <simon.marchi@efficios.com>
> >
> > Simon
> >
>
> --
> Michael Eager
>
>

[-- Attachment #2: Type: text/html, Size: 8837 bytes --]