Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

[Gopi Kumar Bulusu <gopi@sankhya.com>]

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