From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id FM2vBKXxnGhDtgIAWB0awg (envelope-from ) for ; Wed, 13 Aug 2025 16:12:21 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1755115940; bh=sjIm2YoMW3hbtjcxGCXvlrCcdfc8a6h4JGtb09QSjrU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=T3nX2k0/k09Laq1T6v+JHaf9AQN8rivbZUNrdIv1VTiQRAOV/zfY1UGyouOTM6zrU Qm0wjAIDf6nwfHJn+CzzqCVcaMdQ+S+eZJsQn6/dKOhzwE+OUV+YjAzePPdLgJ5+T5 e/CWKBXOU/8VSp29bTgVPjFsgffphv3GZ8DFS16A= Received: by simark.ca (Postfix, from userid 112) id F1F6F1E087; Wed, 13 Aug 2025 16:12:20 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=Lcj79ev9; dkim-atps=neutral Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id CD8931E087 for ; Wed, 13 Aug 2025 16:12:19 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 6A50E385AC2B for ; Wed, 13 Aug 2025 20:12:19 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6A50E385AC2B Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=Lcj79ev9 Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 8C7813858D20 for ; Wed, 13 Aug 2025 20:11:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8C7813858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8C7813858D20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1755115903; cv=none; b=esD3yNM/URIHMo2RTOcqPyaHbTi5pO7r6HNuzc/v0lOrqHE/AN7zLodJ/WhYKMLD10MjFxQOwMoGC5lGd2t+44t1oF/B1vVVA5PGH5vrSsG9EyU1hyfgradD0pCJsFZPt3jiy1vJsDr3jvU5I0SIS3CexHO2XdShP7udQGK798I= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1755115903; c=relaxed/simple; bh=sjIm2YoMW3hbtjcxGCXvlrCcdfc8a6h4JGtb09QSjrU=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=OZiKxFxm4KTsvsEgyR3Z471XBswAI28ypO2WFx+cwkRfJQfnLgZ8r0SRjipOb/W8xJh////Y7WrPVnEsJ1JyhX13hDgNBOSCSY8SFGh/rsmWYWJasiyEyXqT9HZn5aZZbCavOQkOTlOOzVe1Qeoq/Yy4DZyelaouOZayZTUu1QY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8C7813858D20 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1755115903; bh=sjIm2YoMW3hbtjcxGCXvlrCcdfc8a6h4JGtb09QSjrU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Lcj79ev9/Sk6RvBPj1cSM3fK17xRxxfrx8J7Z06yho4MLLx6RW5IyOcTbQbFnSwgy FPWwQsJCDtgAsgZnoRzTReCjLw62ZXO/rLzegKQzwUirOnoG+G6yuQaWFVt0KhXQcn aIRBwLqoUcFBnrmwL6SSlWF3XPF1wVbOX1ktNtj4= Received: by simark.ca (Postfix) id E6AD51E087; Wed, 13 Aug 2025 16:11:42 -0400 (EDT) Message-ID: <3bdebc70-e678-44e7-98ec-18c6b23dccca@simark.ca> Date: Wed, 13 Aug 2025 16:11:42 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] MicroBlaze: Add microblaze_software_single_step To: Gopi Kumar Bulusu , gdb-patches@sourceware.org Cc: Michael Eager References: Content-Language: en-US From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org 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. > 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); > } > > - > +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 > +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 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. > + > + /* 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 ""; } } > + > + /* 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