Re: [PATCH v2 ] MicroBlaze: Add microblaze_get_next_pcs

Tom de Vries
2025-09-05T09:30:15Z

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