On Wed, Sep 3, 2025 at 11:27 PM Simon Marchi <simark@simark.ca> 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.
>

ok.


>
> > +
> > +  /* 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.
>
>
Will check.


> > +
> > +      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);
>

ok

>
> > +
> > +      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 ());
>

ok


>
> > +
> > +      /* 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 ();
>
>
ok


>
> > +    } /* if (branch or return instruction).  */
> > +
> > +  /* Create next_pcs vector to return.  */
> > +
> > +  std::vector<CORE_ADDR> next_pcs;
> > +
> > +  if (next_pc)
>
> .has_value ()
>
>
ok


> > +    {
> > +      next_pcs.push_back (next_pc.value () );
>
> We typically use `*next_pc` to access the value (apply elsewhere too).
>

ok


>
> Remove the space before closing parenthesis (apply elsewhere too).
>

ok

>
> > +      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.
>
>
ok


> The patch LGTM with those fixed.  Let me know if you need help pushing
> the patch.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
>

Thank you for the review. I will try to push the patch after addressing the
above.
I will ask for help if needed.

dhanyavaadaaha
gopi


> Simon
>