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