[PATCH] MicroBlaze: Add microblaze_software_single_step

[PATCH] MicroBlaze: Add microblaze_software_single_step

[Gopi Kumar Bulusu]

[-- Attachment #1.1: Type: text/plain, Size: 734 bytes --]

namaskaaram


*This is part of a series of patches being reviewed, modified, tested and
upstreamed *

*from Xilinx/AMD repository on behalf of AMD/Xilinx under a contract
between *
*AMD/Xilinx and  Sankhya Technologies Private Limited*

This patch supports native linux port of gdbserver for MicroBlaze

*Files Changed*

* gdb/microblaze-tdep.c: Add microblaze_software_single_step, retained
a few useful debug statements added for testing the patch.

There is one more patch in this series of patches for adding gdbserver
support
in gdb for microblazeel-linux and this can be considered a preparatory
patch.

*Build and Test *

Built microblazeel-amd-linux-gdb and tested with gdbserver
(microblazeel-amd-linux)

dhanyavaadaaha
gopi

[-- Attachment #1.2: Type: text/html, Size: 1211 bytes --]

[-- Attachment #2: 0001-MicroBlaze-Add-microblaze_software_single_step.patch --]
[-- Type: text/x-patch, Size: 6352 bytes --]

From f64079c1b08b61276aabfa9c1563f508f4f5373f 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] MicroBlaze: Add microblaze_software_single_step

This patch supports native linux port of gdbserver for MicroBlaze

* gdb/microblaze-tdep.c: Add microblaze_software_single_step, retained
a few useful debug statements added for testing the patch.

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 | 128 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 2 deletions(-)

diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
index 7b58220871c..b0a84b2fe0f 100644
--- a/gdb/microblaze-tdep.c
+++ b/gdb/microblaze-tdep.c
@@ -373,6 +373,9 @@ microblaze_unwind_pc (struct gdbarch *gdbarch, const frame_info_ptr &next_frame)
   gdb_byte buf[4];
   CORE_ADDR pc;
 
+  microblaze_debug ("microblaze_unwind_pc called\n");
+
+
   frame_unwind_register (next_frame, MICROBLAZE_PC_REGNUM, buf);
   pc = extract_typed_address (buf, builtin_type (gdbarch)->builtin_func_ptr);
   /* For sentinel frame, return address is actual PC.  For other frames,
@@ -380,6 +383,9 @@ microblaze_unwind_pc (struct gdbarch *gdbarch, const frame_info_ptr &next_frame)
      generate correct return address in CIE.  */
   if (frame_relative_level (next_frame) >= 0)
     pc += 8;
+
+  microblaze_debug ("microblaze_unwind_pc returning pc: %x \n", pc);
+
   return pc;
 }
 
@@ -423,8 +429,13 @@ microblaze_frame_cache (const frame_info_ptr &next_frame, void **this_cache)
   struct gdbarch *gdbarch = get_frame_arch (next_frame);
   int rn;
 
-  if (*this_cache)
+  microblaze_debug ("microblaze_frame_cache called.\n");
+
+  if (*this_cache) {
+    microblaze_debug ("microblaze_frame_cache: returning cache hit\n");
     return (struct microblaze_frame_cache *) *this_cache;
+  }
+
 
   cache = microblaze_alloc_frame_cache ();
   *this_cache = cache;
@@ -439,6 +450,8 @@ microblaze_frame_cache (const frame_info_ptr &next_frame, void **this_cache)
 
   cache->pc = get_frame_address_in_block (next_frame);
 
+  microblaze_debug ("microblaze_frame_cache - returning (pc = %x).\n", cache->pc);
+
   return cache;
 }
 
@@ -590,7 +603,116 @@ microblaze_stabs_argument_has_addr (struct gdbarch *gdbarch, struct type *type)
   return (type->length () == 16);
 }
 
-\f
+long
+microblaze_getreg(struct regcache *regcache, int regno)
+{
+	long regval = 0;
+
+	if (regno >= 0 && regno < MICROBLAZE_NUM_REGS)
+	  {
+	    regval = regcache_raw_get_unsigned(regcache, regno);
+	  }
+
+	return regval;
+}
+
+/* Return next pc values : next in sequence and/or branch/return target */
+static std::vector<CORE_ADDR>
+microblaze_software_single_step (regcache *regcache)
+{
+
+      gdbarch *arch = regcache->arch ();
+
+      CORE_ADDR pc;
+      std::vector<CORE_ADDR> next_pcs;
+      long insn;
+      enum microblaze_instr minstr;
+      enum microblaze_instr_type insn_type;
+      short delay_slots;
+      int imm;
+      bool isunsignednum;
+      bool immfound = false;
+      struct address_candidates
+      {
+	CORE_ADDR address;
+	bool valid;
+      } candidates [2];
+
+      /* Get instruction */
+      pc = regcache_read_pc (regcache);
+      insn = microblaze_fetch_instruction (pc);
+      minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type, &delay_slots);
+      /* If the current instruction is an imm, look at the inst after */
+      if (insn_type == immediate_inst)
+	{
+	  int rd, ra, rb;
+	  immfound = true;
+	  minstr = microblaze_decode_insn (insn, &rd, &ra, &rb, &imm);
+	  pc = pc + INST_WORD_SIZE;
+	  insn = microblaze_fetch_instruction (pc);
+	  minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type, &delay_slots);
+	}
+
+      candidates[0].valid = (insn_type != return_inst);
+
+      /* Compute next instruction address - skip delay slots if any */
+      candidates[0].address = pc + INST_WORD_SIZE +
+			(delay_slots * INST_WORD_SIZE);
+
+      microblaze_debug ("single-step insn_type=%x pc=%lx insn=%lx\n",
+			insn_type, pc, insn);
+
+      /* Compute target instruction address for branch or return instruction */
+      candidates[1].valid = false;
+      if (insn_type == branch_inst || insn_type == return_inst)
+	{
+	  int limm;
+	  int lrd, lra, lrb;
+	  long ra, rb;
+	  bfd_boolean targetvalid;
+	  bfd_boolean unconditionalbranch;
+
+	  microblaze_decode_insn(insn, &lrd, &lra, &lrb, &limm);
+
+	  ra = microblaze_getreg (regcache, lra);
+	  rb = microblaze_getreg (regcache, lrb);
+
+	  candidates[1].address = microblaze_get_target_address (
+					insn, immfound, imm, pc, ra, rb,
+					&targetvalid, &unconditionalbranch);
+
+	  microblaze_debug (
+	    "single-step uncondbr=%d targetvalid=%d target=%lx\n",
+	    unconditionalbranch, targetvalid, candidates[1].address);
+
+	  /* Can reach next address ? */
+	  candidates[0].valid =
+	    candidates[0].valid && (unconditionalbranch == FALSE);
+
+	  /* Can reach a distinct (not here) target address ? */
+	  candidates[1].valid = FALSE;
+	  if (targetvalid &&
+		  candidates[1].address != pc &&
+		  (candidates[0].valid == FALSE ||
+		    (candidates[0].address != candidates[1].address)))
+	    {
+	      candidates[1].valid = TRUE;
+	    }
+	 } /* if (branch or return instruction) */
+
+	 /* Create next_pcs vector  */
+	 for (int ci = 0; ci < 2; ci++)
+	   {
+	     if (candidates[ci].valid)
+	       {
+		next_pcs.push_back (candidates[ci].address);
+		microblaze_debug ("push_back (%lx)\n", candidates[ci].address);
+	       }
+	   }
+
+      return next_pcs;
+}
+
 static int dwarf2_to_reg_map[78] =
 { 0  /* r0  */,   1  /* r1  */,   2  /* r2  */,   3  /* r3  */,  /*  0- 3 */
   4  /* r4  */,   5  /* r5  */,   6  /* r6  */,   7  /* r7  */,  /*  4- 7 */
@@ -715,6 +837,8 @@ microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_sw_breakpoint_from_kind (gdbarch,
 				       microblaze_breakpoint::bp_from_kind);
 
+  set_gdbarch_software_single_step (gdbarch, microblaze_software_single_step);
+
   set_gdbarch_frame_args_skip (gdbarch, 8);
 
   set_gdbarch_unwind_pc (gdbarch, microblaze_unwind_pc);
-- 
2.47.1

Re: [PATCH] MicroBlaze: Add microblaze_software_single_step

[Simon Marchi]

On 2025-08-12 01:30, Gopi Kumar Bulusu wrote:
> namaskaaram
> 
> /This is part of a series of patches being reviewed, modified, tested and upstreamed
> /
> /from Xilinx/AMD repository on behalf of AMD/Xilinx under a contract between
> /
> /AMD/Xilinx and  Sankhya Technologies Private Limited/
> 
> This patch supports native linux port of gdbserver for MicroBlaze
> 
> *Files Changed*
> 
> * gdb/microblaze-tdep.c: Add microblaze_software_single_step, retained
> a few useful debug statements added for testing the patch.
> 
> There is one more patch in this series of patches for adding gdbserver support
> in gdb for microblazeel-linux and this can be considered a preparatory patch.
> 
> *Build and Test *
> 
> Built microblazeel-amd-linux-gdb and tested with gdbserver (microblazeel-amd-linux)
> 
> 
> 
> From 509f7ff490b7d50e4af52966f27f795ff9d8f9e9 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] MicroBlaze: Add microblaze_software_single_step
> 
> This patch supports native linux port of gdbserver for MicroBlaze
> 
> * gdb/microblaze-tdep.c: Add microblaze_software_single_step, retained
> a few useful debug statements added for testing the patch.

You don't need to use the ChangeLog format here.  Just describe what the
patch adds.

> diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
> index 7b58220871c3..b0a84b2fe0fc 100644
> --- a/gdb/microblaze-tdep.c
> +++ b/gdb/microblaze-tdep.c
> @@ -373,6 +373,9 @@ microblaze_unwind_pc (struct gdbarch *gdbarch, const frame_info_ptr &next_frame)
>    gdb_byte buf[4];
>    CORE_ADDR pc;
>  
> +  microblaze_debug ("microblaze_unwind_pc called\n");

Could you please make a patch that changes microblaze_debug to align it
with our current best practices:

 - rename to microblaze_debug_printf
 - change the log tag from MICROBLAZE to microblaze
 - change the macro to use debug_prefixed_printf_cond instead of
   debug_prefixed_printf_cond_nofunc (so that the function name is
   automatically included)
 - update the calls to not add the function name manually
 - update the calls to not include a trailing \n


IMO, I don't find this "called" logging statement by itself very useful.
It should perhaps log the number of "next_frame", so that it's possible
to correlate the frame number with the return value.

Finally, ff you want to improve the logging in these functions
(microblaze_frame_cache and microblaze_unwind_pc), please send a patch
that does just that (it could be a mini-series with the improvements
mentioned above).  It's not related to adding
microblaze_software_single_step.

> @@ -423,8 +429,13 @@ microblaze_frame_cache (const frame_info_ptr &next_frame, void **this_cache)
>    struct gdbarch *gdbarch = get_frame_arch (next_frame);
>    int rn;
>  
> -  if (*this_cache)
> +  microblaze_debug ("microblaze_frame_cache called.\n");

Same comment about "called".  I would at least log next_frame's number.

> +
> +  if (*this_cache) {
> +    microblaze_debug ("microblaze_frame_cache: returning cache hit\n");
>      return (struct microblaze_frame_cache *) *this_cache;
> +  }

Formatting and other nits:

  if (*this_cache != null)
    {
      microblaze_debug_printf ("returning cache hit");  // do you want to log some information about the result here, like you do below?
      return (struct microblaze_frame_cache *) *this_cache;
    }

> @@ -590,7 +603,116 @@ microblaze_stabs_argument_has_addr (struct gdbarch *gdbarch, struct type *type)
>    return (type->length () == 16);
>  }
>  
> -\f
> +long
> +microblaze_getreg(struct regcache *regcache, int regno)

Does this really compile?  You don't get a compiler warning about
missing prototype or something like that?

Missing space before parenthesis.

Please add a comment explaining what the function does and returns.

> +{
> +	long regval = 0;
> +
> +	if (regno >= 0 && regno < MICROBLAZE_NUM_REGS)

I think that calling this function with an invalid regno would be an
internal error.  IOW you could do:

  gdb_assert (regno >= 0 && regno < MICROBLAZE_NUM_REGS);

Or don't because the regcache will do it itself (see
reg_buffer::assert_regnum).  So actually, I think this function is not
necessary, just use regcache_raw_get_unsigned in your code.

> +	  {
> +	    regval = regcache_raw_get_unsigned(regcache, regno);

Missing paren.

> +	  }
> +
> +	return regval;
> +}
> +
> +/* Return next pc values : next in sequence and/or branch/return target */
> +static std::vector<CORE_ADDR>
> +microblaze_software_single_step (regcache *regcache)

Empty line after comment.  Finish comment with period and two spaces.

> +{
> +
> +      gdbarch *arch = regcache->arch ();

Please fix the indentation in this while function to match the coding
style.

> +
> +      CORE_ADDR pc;
> +      std::vector<CORE_ADDR> next_pcs;
> +      long insn;
> +      enum microblaze_instr minstr;
> +      enum microblaze_instr_type insn_type;
> +      short delay_slots;
> +      int imm;
> +      bool isunsignednum;
> +      bool immfound = false;
> +      struct address_candidates
> +      {
> +	CORE_ADDR address;
> +	bool valid;
> +      } candidates [2];

Declare variables at first use.

Instead of an array of two of these, I think you could use two variables
with descriptive names, e.g. "following_insn" and "jmp_ret_target_insn".

Also, instead of a "valid" flag, they could probably be
std::optional<CORE_ADDR>.

> +
> +      /* Get instruction */
> +      pc = regcache_read_pc (regcache);
> +      insn = microblaze_fetch_instruction (pc);
> +      minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type, &delay_slots);
> +      /* If the current instruction is an imm, look at the inst after */
> +      if (insn_type == immediate_inst)
> +	{
> +	  int rd, ra, rb;
> +	  immfound = true;
> +	  minstr = microblaze_decode_insn (insn, &rd, &ra, &rb, &imm);
> +	  pc = pc + INST_WORD_SIZE;

  pc += INST_WORD_SIZE;

> +	  insn = microblaze_fetch_instruction (pc);
> +	  minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type, &delay_slots);
> +	}
> +
> +      candidates[0].valid = (insn_type != return_inst);
> +
> +      /* Compute next instruction address - skip delay slots if any */
> +      candidates[0].address = pc + INST_WORD_SIZE +
> +			(delay_slots * INST_WORD_SIZE);
> +
> +      microblaze_debug ("single-step insn_type=%x pc=%lx insn=%lx\n",
> +			insn_type, pc, insn);

I would suggest prefixing the hex values with "0x" to make it clear
(valid for the other logging statements below as well).

I think it would be good to add a function "microblaze_instr_type_str"
and then use it to print the insn_type value.

static const char *
microblaze_instr_type_str (microblaze_instr_type type)
{
  switch (type)
    {
    case arithmetic_inst:
      return "arithmetic";
    case logical_inst:
      return "logical";
    case mult_inst:
      return "mult";
    case div_inst:
      return "div";
    case branch_inst:
      return "branch";
    case return_inst:
      return "return";
    case immediate_inst:
      return "immediate";
    case special_inst:
      return "special";
    case memory_load_inst:
      return "memory_load";
    case memory_store_inst:
      return "memory_store";
    case barrel_shift_inst:
      return "barrel_shift";
    case anyware_inst:
      return "anyware";
    default:
      return "<unknown>";
    }
}

> +
> +      /* Compute target instruction address for branch or return instruction */
> +      candidates[1].valid = false;
> +      if (insn_type == branch_inst || insn_type == return_inst)
> +	{
> +	  int limm;
> +	  int lrd, lra, lrb;
> +	  long ra, rb;
> +	  bfd_boolean targetvalid;
> +	  bfd_boolean unconditionalbranch;

Looking at the signature of microblaze_get_target_address, I think those
bfd_boolean could be bool instead.

> +
> +	  microblaze_decode_insn(insn, &lrd, &lra, &lrb, &limm);
> +
> +	  ra = microblaze_getreg (regcache, lra);
> +	  rb = microblaze_getreg (regcache, lrb);
> +
> +	  candidates[1].address = microblaze_get_target_address (
> +					insn, immfound, imm, pc, ra, rb,
> +					&targetvalid, &unconditionalbranch);
> +
> +	  microblaze_debug (
> +	    "single-step uncondbr=%d targetvalid=%d target=%lx\n",
> +	    unconditionalbranch, targetvalid, candidates[1].address);
> +
> +	  /* Can reach next address ? */
> +	  candidates[0].valid =
> +	    candidates[0].valid && (unconditionalbranch == FALSE);
> +
> +	  /* Can reach a distinct (not here) target address ? */
> +	  candidates[1].valid = FALSE;

candidates[1].valid was already set to false above.

> +	  if (targetvalid &&
> +		  candidates[1].address != pc &&
> +		  (candidates[0].valid == FALSE ||
> +		    (candidates[0].address != candidates[1].address)))
> +	    {
> +	      candidates[1].valid = TRUE;
> +	    }

Use true/false instead of TRUE/FALSE.

> +	 } /* if (branch or return instruction) */
> +
> +	 /* Create next_pcs vector  */
> +	 for (int ci = 0; ci < 2; ci++)
> +	   {
> +	     if (candidates[ci].valid)
> +	       {
> +		next_pcs.push_back (candidates[ci].address);
> +		microblaze_debug ("push_back (%lx)\n", candidates[ci].address);
> +	       }
> +	   }

Drop the outer curly braces (the braces for the "for").

Simon

Re: [PATCH] MicroBlaze: Add microblaze_software_single_step

[Gopi Kumar Bulusu]

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

On Thu, Aug 14, 2025 at 1:41 AM Simon Marchi <simark@simark.ca> wrote:

>
>
> On 2025-08-12 01:30, Gopi Kumar Bulusu wrote:
> > namaskaaram
> >
> > /This is part of a series of patches being reviewed, modified, tested
> and upstreamed
> > /
> > /from Xilinx/AMD repository on behalf of AMD/Xilinx under a contract
> between
> > /
> > /AMD/Xilinx and  Sankhya Technologies Private Limited/
> >
> > This patch supports native linux port of gdbserver for MicroBlaze
> >
> > *Files Changed*
> >
> > * gdb/microblaze-tdep.c: Add microblaze_software_single_step, retained
> > a few useful debug statements added for testing the patch.
> >
> > There is one more patch in this series of patches for adding gdbserver
> support
> > in gdb for microblazeel-linux and this can be considered a preparatory
> patch.
> >
> > *Build and Test *
> >
> > Built microblazeel-amd-linux-gdb and tested with gdbserver
> (microblazeel-amd-linux)
> >
> >
> >
> > From 509f7ff490b7d50e4af52966f27f795ff9d8f9e9 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] MicroBlaze: Add microblaze_software_single_step
> >
> > This patch supports native linux port of gdbserver for MicroBlaze
> >
> > * gdb/microblaze-tdep.c: Add microblaze_software_single_step, retained
> > a few useful debug statements added for testing the patch.
>
> You don't need to use the ChangeLog format here.  Just describe what the
> patch adds.
>

ok.


>
> > diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
> > index 7b58220871c3..b0a84b2fe0fc 100644
> > --- a/gdb/microblaze-tdep.c
> > +++ b/gdb/microblaze-tdep.c
> > @@ -373,6 +373,9 @@ microblaze_unwind_pc (struct gdbarch *gdbarch, const
> frame_info_ptr &next_frame)
> >    gdb_byte buf[4];
> >    CORE_ADDR pc;
> >
> > +  microblaze_debug ("microblaze_unwind_pc called\n");
>
> Could you please make a patch that changes microblaze_debug to align it
> with our current best practices:
>
  - rename to microblaze_debug_printf
>  - change the log tag from MICROBLAZE to microblaze
>  - change the macro to use debug_prefixed_printf_cond instead of
>    debug_prefixed_printf_cond_nofunc (so that the function name is
>    automatically included)
>  - update the calls to not add the function name manually
>  - update the calls to not include a trailing \n
>


Will look into this for a separate patch. Will drop the debug statements in
patch v2.


> IMO, I don't find this "called" logging statement by itself very useful.
> It should perhaps log the number of "next_frame", so that it's possible
> to correlate the frame number with the return value.
>
> Finally, ff you want to improve the logging in these functions
> (microblaze_frame_cache and microblaze_unwind_pc), please send a patch
> that does just that (it could be a mini-series with the improvements
> mentioned above).  It's not related to adding
> microblaze_software_single_step.
>

Ok.


>
> > @@ -423,8 +429,13 @@ microblaze_frame_cache (const frame_info_ptr
> &next_frame, void **this_cache)
> >    struct gdbarch *gdbarch = get_frame_arch (next_frame);
> >    int rn;
> >
> > -  if (*this_cache)
> > +  microblaze_debug ("microblaze_frame_cache called.\n");
>
> Same comment about "called".  I would at least log next_frame's number.
>
> ok.


> > +
> > +  if (*this_cache) {
> > +    microblaze_debug ("microblaze_frame_cache: returning cache hit\n");
> >      return (struct microblaze_frame_cache *) *this_cache;
> > +  }
>
> Formatting and other nits:
>
>   if (*this_cache != null)
>     {
>       microblaze_debug_printf ("returning cache hit");  // do you want to
> log some information about the result here, like you do below?
>       return (struct microblaze_frame_cache *) *this_cache;
>     }
>

I will drop the unrelated debug log changes in V2 as mentioned above.



> > @@ -590,7 +603,116 @@ microblaze_stabs_argument_has_addr (struct gdbarch
> *gdbarch, struct type *type)
> >    return (type->length () == 16);
> >  }
> >
> > -
> > +long
> > +microblaze_getreg(struct regcache *regcache, int regno)
>
> Does this really compile?  You don't get a compiler warning about
> missing prototype or something like that?
>

 Oops, I will clean up the warnings for v2.


> Missing space before parenthesis.
>
> Please add a comment explaining what the function does and returns.
>
> > +{
> > +     long regval = 0;
> > +
> > +     if (regno >= 0 && regno < MICROBLAZE_NUM_REGS)
>
> I think that calling this function with an invalid regno would be an
> internal error.  IOW you could do:
>
>   gdb_assert (regno >= 0 && regno < MICROBLAZE_NUM_REGS);
>
> Or don't because the regcache will do it itself (see
> reg_buffer::assert_regnum).  So actually, I think this function is not
> necessary, just use regcache_raw_get_unsigned in your code.
>
>
Will check and switch to regcache_raw_get_unsigned



> > +       {
> > +         regval = regcache_raw_get_unsigned(regcache, regno);
>
> Missing paren.
>
> > +       }
> > +
> > +     return regval;
> > +}
> > +
> > +/* Return next pc values : next in sequence and/or branch/return target
> */
> > +static std::vector<CORE_ADDR>
> > +microblaze_software_single_step (regcache *regcache)
>
> Empty line after comment.  Finish comment with period and two spaces.
>
> ok


> > +{
> > +
> > +      gdbarch *arch = regcache->arch ();
>
> Please fix the indentation in this while function to match the coding
> style.
>
> > +
> > +      CORE_ADDR pc;
> > +      std::vector<CORE_ADDR> next_pcs;
> > +      long insn;
> > +      enum microblaze_instr minstr;
> > +      enum microblaze_instr_type insn_type;
> > +      short delay_slots;
> > +      int imm;
> > +      bool isunsignednum;
> > +      bool immfound = false;
> > +      struct address_candidates
> > +      {
> > +     CORE_ADDR address;
> > +     bool valid;
> > +      } candidates [2];
>
> Declare variables at first use.
>

Will check and change wherever appropriate.


>
> Instead of an array of two of these, I think you could use two variables
> with descriptive names, e.g. "following_insn" and "jmp_ret_target_insn".
>
> Also, instead of a "valid" flag, they could probably be
> std::optional<CORE_ADDR>.
>

Will rewrite - did not want to significantly change the original code that
was part of the Xilinx repo for long.


>
> > +
> > +      /* Get instruction */
> > +      pc = regcache_read_pc (regcache);
> > +      insn = microblaze_fetch_instruction (pc);
> > +      minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type,
> &delay_slots);
> > +      /* If the current instruction is an imm, look at the inst after */
> > +      if (insn_type == immediate_inst)
> > +     {
> > +       int rd, ra, rb;
> > +       immfound = true;
> > +       minstr = microblaze_decode_insn (insn, &rd, &ra, &rb, &imm);
> > +       pc = pc + INST_WORD_SIZE;
>
>   pc += INST_WORD_SIZE;
>
>
ok !


> > +       insn = microblaze_fetch_instruction (pc);
> > +       minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type,
> &delay_slots);
> > +     }
> > +
> > +      candidates[0].valid = (insn_type != return_inst);
> > +
> > +      /* Compute next instruction address - skip delay slots if any */
> > +      candidates[0].address = pc + INST_WORD_SIZE +
> > +                     (delay_slots * INST_WORD_SIZE);
> > +
> > +      microblaze_debug ("single-step insn_type=%x pc=%lx insn=%lx\n",
> > +                     insn_type, pc, insn);
>
> I would suggest prefixing the hex values with "0x" to make it clear
> (valid for the other logging statements below as well).
>

ok


>
> I think it would be good to add a function "microblaze_instr_type_str"
> and then use it to print the insn_type value.
>

Not sure if that is appropriate. I added some of these log statements to
debug the function
and get this function to work. Will analyze further for v2 patch and
include if that makes sense
from the perpsective of some one troubleshooting a broken breakpoint
functionality.


> static const char *
> microblaze_instr_type_str (microblaze_instr_type type)
> {
>   switch (type)
>     {
>     case arithmetic_inst:
>       return "arithmetic";
>     case logical_inst:
>       return "logical";
>     case mult_inst:
>       return "mult";
>     case div_inst:
>       return "div";
>     case branch_inst:
>       return "branch";
>     case return_inst:
>       return "return";
>     case immediate_inst:
>       return "immediate";
>     case special_inst:
>       return "special";
>     case memory_load_inst:
>       return "memory_load";
>     case memory_store_inst:
>       return "memory_store";
>     case barrel_shift_inst:
>       return "barrel_shift";
>     case anyware_inst:
>       return "anyware";
>     default:
>       return "<unknown>";
>     }
> }
>
> > +
> > +      /* Compute target instruction address for branch or return
> instruction */
> > +      candidates[1].valid = false;
> > +      if (insn_type == branch_inst || insn_type == return_inst)
> > +     {
> > +       int limm;
> > +       int lrd, lra, lrb;
> > +       long ra, rb;
> > +       bfd_boolean targetvalid;
> > +       bfd_boolean unconditionalbranch;
>
> Looking at the signature of microblaze_get_target_address, I think those
> bfd_boolean could be bool instead.
>

Will check and change.


>
> > +
> > +       microblaze_decode_insn(insn, &lrd, &lra, &lrb, &limm);
> > +
> > +       ra = microblaze_getreg (regcache, lra);
> > +       rb = microblaze_getreg (regcache, lrb);
> > +
> > +       candidates[1].address = microblaze_get_target_address (
> > +                                     insn, immfound, imm, pc, ra, rb,
> > +                                     &targetvalid,
> &unconditionalbranch);
> > +
> > +       microblaze_debug (
> > +         "single-step uncondbr=%d targetvalid=%d target=%lx\n",
> > +         unconditionalbranch, targetvalid, candidates[1].address);
> > +
> > +       /* Can reach next address ? */
> > +       candidates[0].valid =
> > +         candidates[0].valid && (unconditionalbranch == FALSE);
> > +
> > +       /* Can reach a distinct (not here) target address ? */
> > +       candidates[1].valid = FALSE;
>
> candidates[1].valid was already set to false above.
>

Yes, will remove.


>
> > +       if (targetvalid &&
> > +               candidates[1].address != pc &&
> > +               (candidates[0].valid == FALSE ||
> > +                 (candidates[0].address != candidates[1].address)))
> > +         {
> > +           candidates[1].valid = TRUE;
> > +         }
>
> Use true/false instead of TRUE/FALSE.
>

ok.


>
> > +      } /* if (branch or return instruction) */
> > +
> > +      /* Create next_pcs vector  */
> > +      for (int ci = 0; ci < 2; ci++)
> > +        {
> > +          if (candidates[ci].valid)
> > +            {
> > +             next_pcs.push_back (candidates[ci].address);
> > +             microblaze_debug ("push_back (%lx)\n",
> candidates[ci].address);
> > +            }
> > +        }
>
> Drop the outer curly braces (the braces for the "for").
>

ok


>
> Simon
>

Thanks a ton for the quick review.

dhanyavaadaaha
gopi

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

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Gopi Kumar Bulusu]

[-- Attachment #1.1: Type: text/plain, Size: 12267 bytes --]

namaskaaram,

This patch adds microblaze_get_nex_pcs

Includes changes as per my response to PATCH v1

[Built microblazeel-amd-linux-gdb and tested with
microblazeel-amd-linux-gdbserver]

dhanyavaadaaha
gopi

On Thu, Aug 14, 2025 at 3:58 PM Gopi Kumar Bulusu <gopi@sankhya.com> wrote:

>
>
> On Thu, Aug 14, 2025 at 1:41 AM Simon Marchi <simark@simark.ca> wrote:
>
>>
>>
>> On 2025-08-12 01:30, Gopi Kumar Bulusu wrote:
>> > namaskaaram
>> >
>> > /This is part of a series of patches being reviewed, modified, tested
>> and upstreamed
>> > /
>> > /from Xilinx/AMD repository on behalf of AMD/Xilinx under a contract
>> between
>> > /
>> > /AMD/Xilinx and  Sankhya Technologies Private Limited/
>> >
>> > This patch supports native linux port of gdbserver for MicroBlaze
>> >
>> > *Files Changed*
>> >
>> > * gdb/microblaze-tdep.c: Add microblaze_software_single_step, retained
>> > a few useful debug statements added for testing the patch.
>> >
>> > There is one more patch in this series of patches for adding gdbserver
>> support
>> > in gdb for microblazeel-linux and this can be considered a preparatory
>> patch.
>> >
>> > *Build and Test *
>> >
>> > Built microblazeel-amd-linux-gdb and tested with gdbserver
>> (microblazeel-amd-linux)
>> >
>> >
>> >
>> > From 509f7ff490b7d50e4af52966f27f795ff9d8f9e9 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] MicroBlaze: Add microblaze_software_single_step
>> >
>> > This patch supports native linux port of gdbserver for MicroBlaze
>> >
>> > * gdb/microblaze-tdep.c: Add microblaze_software_single_step, retained
>> > a few useful debug statements added for testing the patch.
>>
>> You don't need to use the ChangeLog format here.  Just describe what the
>> patch adds.
>>
>
> ok.
>
>
>>
>> > diff --git a/gdb/microblaze-tdep.c b/gdb/microblaze-tdep.c
>> > index 7b58220871c3..b0a84b2fe0fc 100644
>> > --- a/gdb/microblaze-tdep.c
>> > +++ b/gdb/microblaze-tdep.c
>> > @@ -373,6 +373,9 @@ microblaze_unwind_pc (struct gdbarch *gdbarch,
>> const frame_info_ptr &next_frame)
>> >    gdb_byte buf[4];
>> >    CORE_ADDR pc;
>> >
>> > +  microblaze_debug ("microblaze_unwind_pc called\n");
>>
>> Could you please make a patch that changes microblaze_debug to align it
>> with our current best practices:
>>
>   - rename to microblaze_debug_printf
>>  - change the log tag from MICROBLAZE to microblaze
>>  - change the macro to use debug_prefixed_printf_cond instead of
>>    debug_prefixed_printf_cond_nofunc (so that the function name is
>>    automatically included)
>>  - update the calls to not add the function name manually
>>  - update the calls to not include a trailing \n
>>
>
>
> Will look into this for a separate patch. Will drop the debug statements
> in patch v2.
>
>
>> IMO, I don't find this "called" logging statement by itself very useful.
>> It should perhaps log the number of "next_frame", so that it's possible
>> to correlate the frame number with the return value.
>>
>> Finally, ff you want to improve the logging in these functions
>> (microblaze_frame_cache and microblaze_unwind_pc), please send a patch
>> that does just that (it could be a mini-series with the improvements
>> mentioned above).  It's not related to adding
>> microblaze_software_single_step.
>>
>
> Ok.
>
>
>>
>> > @@ -423,8 +429,13 @@ microblaze_frame_cache (const frame_info_ptr
>> &next_frame, void **this_cache)
>> >    struct gdbarch *gdbarch = get_frame_arch (next_frame);
>> >    int rn;
>> >
>> > -  if (*this_cache)
>> > +  microblaze_debug ("microblaze_frame_cache called.\n");
>>
>> Same comment about "called".  I would at least log next_frame's number.
>>
>> ok.
>
>
>> > +
>> > +  if (*this_cache) {
>> > +    microblaze_debug ("microblaze_frame_cache: returning cache hit\n");
>> >      return (struct microblaze_frame_cache *) *this_cache;
>> > +  }
>>
>> Formatting and other nits:
>>
>>   if (*this_cache != null)
>>     {
>>       microblaze_debug_printf ("returning cache hit");  // do you want to
>> log some information about the result here, like you do below?
>>       return (struct microblaze_frame_cache *) *this_cache;
>>     }
>>
>
> I will drop the unrelated debug log changes in V2 as mentioned above.
>
>
>
>> > @@ -590,7 +603,116 @@ microblaze_stabs_argument_has_addr (struct
>> gdbarch *gdbarch, struct type *type)
>> >    return (type->length () == 16);
>> >  }
>> >
>> > -
>> > +long
>> > +microblaze_getreg(struct regcache *regcache, int regno)
>>
>> Does this really compile?  You don't get a compiler warning about
>> missing prototype or something like that?
>>
>
>  Oops, I will clean up the warnings for v2.
>
>
>> Missing space before parenthesis.
>>
>> Please add a comment explaining what the function does and returns.
>>
>> > +{
>> > +     long regval = 0;
>> > +
>> > +     if (regno >= 0 && regno < MICROBLAZE_NUM_REGS)
>>
>> I think that calling this function with an invalid regno would be an
>> internal error.  IOW you could do:
>>
>>   gdb_assert (regno >= 0 && regno < MICROBLAZE_NUM_REGS);
>>
>> Or don't because the regcache will do it itself (see
>> reg_buffer::assert_regnum).  So actually, I think this function is not
>> necessary, just use regcache_raw_get_unsigned in your code.
>>
>>
> Will check and switch to regcache_raw_get_unsigned
>
>
>
>> > +       {
>> > +         regval = regcache_raw_get_unsigned(regcache, regno);
>>
>> Missing paren.
>>
>> > +       }
>> > +
>> > +     return regval;
>> > +}
>> > +
>> > +/* Return next pc values : next in sequence and/or branch/return
>> target */
>> > +static std::vector<CORE_ADDR>
>> > +microblaze_software_single_step (regcache *regcache)
>>
>> Empty line after comment.  Finish comment with period and two spaces.
>>
>> ok
>
>
>> > +{
>> > +
>> > +      gdbarch *arch = regcache->arch ();
>>
>> Please fix the indentation in this while function to match the coding
>> style.
>>
>> > +
>> > +      CORE_ADDR pc;
>> > +      std::vector<CORE_ADDR> next_pcs;
>> > +      long insn;
>> > +      enum microblaze_instr minstr;
>> > +      enum microblaze_instr_type insn_type;
>> > +      short delay_slots;
>> > +      int imm;
>> > +      bool isunsignednum;
>> > +      bool immfound = false;
>> > +      struct address_candidates
>> > +      {
>> > +     CORE_ADDR address;
>> > +     bool valid;
>> > +      } candidates [2];
>>
>> Declare variables at first use.
>>
>
> Will check and change wherever appropriate.
>
>
>>
>> Instead of an array of two of these, I think you could use two variables
>> with descriptive names, e.g. "following_insn" and "jmp_ret_target_insn".
>>
>> Also, instead of a "valid" flag, they could probably be
>> std::optional<CORE_ADDR>.
>>
>
> Will rewrite - did not want to significantly change the original code that
> was part of the Xilinx repo for long.
>
>
>>
>> > +
>> > +      /* Get instruction */
>> > +      pc = regcache_read_pc (regcache);
>> > +      insn = microblaze_fetch_instruction (pc);
>> > +      minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type,
>> &delay_slots);
>> > +      /* If the current instruction is an imm, look at the inst after
>> */
>> > +      if (insn_type == immediate_inst)
>> > +     {
>> > +       int rd, ra, rb;
>> > +       immfound = true;
>> > +       minstr = microblaze_decode_insn (insn, &rd, &ra, &rb, &imm);
>> > +       pc = pc + INST_WORD_SIZE;
>>
>>   pc += INST_WORD_SIZE;
>>
>>
> ok !
>
>
>> > +       insn = microblaze_fetch_instruction (pc);
>> > +       minstr = get_insn_microblaze (insn, &isunsignednum, &insn_type,
>> &delay_slots);
>> > +     }
>> > +
>> > +      candidates[0].valid = (insn_type != return_inst);
>> > +
>> > +      /* Compute next instruction address - skip delay slots if any */
>> > +      candidates[0].address = pc + INST_WORD_SIZE +
>> > +                     (delay_slots * INST_WORD_SIZE);
>> > +
>> > +      microblaze_debug ("single-step insn_type=%x pc=%lx insn=%lx\n",
>> > +                     insn_type, pc, insn);
>>
>> I would suggest prefixing the hex values with "0x" to make it clear
>> (valid for the other logging statements below as well).
>>
>
> ok
>
>
>>
>> I think it would be good to add a function "microblaze_instr_type_str"
>> and then use it to print the insn_type value.
>>
>
> Not sure if that is appropriate. I added some of these log statements to
> debug the function
> and get this function to work. Will analyze further for v2 patch and
> include if that makes sense
> from the perpsective of some one troubleshooting a broken breakpoint
> functionality.
>
>
>> static const char *
>> microblaze_instr_type_str (microblaze_instr_type type)
>> {
>>   switch (type)
>>     {
>>     case arithmetic_inst:
>>       return "arithmetic";
>>     case logical_inst:
>>       return "logical";
>>     case mult_inst:
>>       return "mult";
>>     case div_inst:
>>       return "div";
>>     case branch_inst:
>>       return "branch";
>>     case return_inst:
>>       return "return";
>>     case immediate_inst:
>>       return "immediate";
>>     case special_inst:
>>       return "special";
>>     case memory_load_inst:
>>       return "memory_load";
>>     case memory_store_inst:
>>       return "memory_store";
>>     case barrel_shift_inst:
>>       return "barrel_shift";
>>     case anyware_inst:
>>       return "anyware";
>>     default:
>>       return "<unknown>";
>>     }
>> }
>>
>> > +
>> > +      /* Compute target instruction address for branch or return
>> instruction */
>> > +      candidates[1].valid = false;
>> > +      if (insn_type == branch_inst || insn_type == return_inst)
>> > +     {
>> > +       int limm;
>> > +       int lrd, lra, lrb;
>> > +       long ra, rb;
>> > +       bfd_boolean targetvalid;
>> > +       bfd_boolean unconditionalbranch;
>>
>> Looking at the signature of microblaze_get_target_address, I think those
>> bfd_boolean could be bool instead.
>>
>
> Will check and change.
>
>
>>
>> > +
>> > +       microblaze_decode_insn(insn, &lrd, &lra, &lrb, &limm);
>> > +
>> > +       ra = microblaze_getreg (regcache, lra);
>> > +       rb = microblaze_getreg (regcache, lrb);
>> > +
>> > +       candidates[1].address = microblaze_get_target_address (
>> > +                                     insn, immfound, imm, pc, ra, rb,
>> > +                                     &targetvalid,
>> &unconditionalbranch);
>> > +
>> > +       microblaze_debug (
>> > +         "single-step uncondbr=%d targetvalid=%d target=%lx\n",
>> > +         unconditionalbranch, targetvalid, candidates[1].address);
>> > +
>> > +       /* Can reach next address ? */
>> > +       candidates[0].valid =
>> > +         candidates[0].valid && (unconditionalbranch == FALSE);
>> > +
>> > +       /* Can reach a distinct (not here) target address ? */
>> > +       candidates[1].valid = FALSE;
>>
>> candidates[1].valid was already set to false above.
>>
>
> Yes, will remove.
>
>
>>
>> > +       if (targetvalid &&
>> > +               candidates[1].address != pc &&
>> > +               (candidates[0].valid == FALSE ||
>> > +                 (candidates[0].address != candidates[1].address)))
>> > +         {
>> > +           candidates[1].valid = TRUE;
>> > +         }
>>
>> Use true/false instead of TRUE/FALSE.
>>
>
> ok.
>
>
>>
>> > +      } /* if (branch or return instruction) */
>> > +
>> > +      /* Create next_pcs vector  */
>> > +      for (int ci = 0; ci < 2; ci++)
>> > +        {
>> > +          if (candidates[ci].valid)
>> > +            {
>> > +             next_pcs.push_back (candidates[ci].address);
>> > +             microblaze_debug ("push_back (%lx)\n",
>> candidates[ci].address);
>> > +            }
>> > +        }
>>
>> Drop the outer curly braces (the braces for the "for").
>>
>
> ok
>
>
>>
>> Simon
>>
>
> Thanks a ton for the quick review.
>
> dhanyavaadaaha
> gopi
>
>
>

[-- Attachment #1.2: Type: text/html, Size: 17781 bytes --]

[-- Attachment #2: v2-0001-MicroBlaze-Add-microblaze_get_next_pcs.patch --]
[-- Type: text/x-patch, Size: 4245 bytes --]

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);
+
+  /* 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;
+
+      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);
+
+      microblaze_debug (
+		      "single-step uncondbr=%d targetvalid=%d target=0x%lx\n",
+		      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 ();
+    } /* if (branch or return instruction).  */
+
+  /* Create next_pcs vector to return.  */
+
+  std::vector<CORE_ADDR> next_pcs;
+
+  if (next_pc)
+    {
+      next_pcs.push_back (next_pc.value () );
+      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 ());
+    }
+
+  return next_pcs;
+}
 \f
 static int dwarf2_to_reg_map[78] =
 { 0  /* r0  */,   1  /* r1  */,   2  /* r2  */,   3  /* r3  */,  /*  0- 3 */
@@ -715,6 +804,8 @@ microblaze_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_sw_breakpoint_from_kind (gdbarch,
 				       microblaze_breakpoint::bp_from_kind);
 
+  set_gdbarch_get_next_pcs (gdbarch, microblaze_get_next_pcs);
+
   set_gdbarch_frame_args_skip (gdbarch, 8);
 
   set_gdbarch_unwind_pc (gdbarch, microblaze_unwind_pc);
-- 
2.47.1

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Simon Marchi]

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

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Michael Eager]

Please submit revised patch.

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

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Gopi Kumar Bulusu]

[-- 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 --]

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Gopi Kumar Bulusu]

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

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
>

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

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Tom de Vries]

On 9/3/25 19:57, 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.
> 

This caused a buildbreaker on arm-linux with --enable-targets=all and 
--enable-64-bit-bfd.

I've filed a PR ( https://sourceware.org/bugzilla/show_bug.cgi?id=33381 ).

Thanks,
- Tom

> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> 
> Simon

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Gopi Kumar Bulusu]

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

On Fri, Sep 5, 2025 at 2:59 PM Tom de Vries <tdevries@suse.de> wrote:

> On 9/3/25 19:57, 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.
> >
>
> This caused a buildbreaker on arm-linux with --enable-targets=all and
> --enable-64-bit-bfd.
>
>
reverted commit c6df5d79aac5c4a77c06314fd26c837470360f70

dhanyavaadaaha
gopi


I've filed a PR ( https://sourceware.org/bugzilla/show_bug.cgi?id=33381 ).


> Thanks,
> - Tom
>
> > Approved-By: Simon Marchi <simon.marchi@efficios.com>
> >
> > Simon
>
>

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

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Tom Tromey]

>  This caused a buildbreaker on arm-linux with --enable-targets=all and 
>  --enable-64-bit-bfd.

> reverted commit c6df5d79aac5c4a77c06314fd26c837470360f70

Reverting seems a little heavy-handed for what looks like a pretty
simple fix.

FWIW in gdb, printing a CORE_ADDR is typically done with
core_addr_to_string or core_addr_to_string_nz.  Then in a printf, "%s"
is used.  E.g.:

    aarch64_debug_printf ("prologue analysis gave up "
                          "addr=%s opcode=0x%x (orr x register)",
                          core_addr_to_string_nz (start), insn);

Tom

Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Gopi Kumar Bulusu]

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

On Fri, Sep 5, 2025 at 6:54 PM Tom Tromey <tom@tromey.com> wrote:

> >  This caused a buildbreaker on arm-linux with --enable-targets=all and
> >  --enable-64-bit-bfd.
>
> > reverted commit c6df5d79aac5c4a77c06314fd26c837470360f70
>
> Reverting seems a little heavy-handed for what looks like a pretty
> simple fix.
>
> FWIW in gdb, printing a CORE_ADDR is typically done with
> core_addr_to_string or core_addr_to_string_nz.  Then in a printf, "%s"
> is used.  E.g.:
>
>     aarch64_debug_printf ("prologue analysis gave up "
>                           "addr=%s opcode=0x%x (orr x register)",
>                           core_addr_to_string_nz (start), insn);
>

I fixed it using paddress and retested it - but, will change that to
core_addr_to_string_nz and push it.

dhanyavaadaaha
gopi


> Tom
>

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