Re: [PATCH] MicroBlaze: Add microblaze_software_single_step

[Simon Marchi <simark@simark.ca>]

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