Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

Gopi Kumar Bulusu
2025-09-02T13:07:19Z

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