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 wrote: > > > On Thu, Aug 14, 2025 at 1:41 AM Simon Marchi 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 >> > 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 >> > +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 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. >> > > 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 ""; >> } >> } >> >> > + >> > + /* 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 > > >