Re: [PATCH] MicroBlaze: Add microblaze_software_single_step

Simon Marchi
2025-08-13T20:12:21Z

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