From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id mYaxEWHAnWg1OgMAWB0awg (envelope-from ) for ; Thu, 14 Aug 2025 06:54:25 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=sankhya-com.20230601.gappssmtp.com header.i=@sankhya-com.20230601.gappssmtp.com header.a=rsa-sha256 header.s=20230601 header.b=ZzYIMUGY; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 440951E0B3; Thu, 14 Aug 2025 06:54:25 -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.3 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,HTML_MESSAGE,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 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 DF7BE1E04C for ; Thu, 14 Aug 2025 06:54:23 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 814613858CD9 for ; Thu, 14 Aug 2025 10:54:23 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 814613858CD9 Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=sankhya-com.20230601.gappssmtp.com header.i=@sankhya-com.20230601.gappssmtp.com header.a=rsa-sha256 header.s=20230601 header.b=ZzYIMUGY Received: from mail-ej1-x631.google.com (mail-ej1-x631.google.com [IPv6:2a00:1450:4864:20::631]) by sourceware.org (Postfix) with ESMTPS id BDBFE3858D3C for ; Thu, 14 Aug 2025 10:53:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BDBFE3858D3C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sankhya.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=sankhya.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org BDBFE3858D3C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::631 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1755168826; cv=none; b=kjc6YWUnPCkGHz+lKYjmY+VT+OjVsGK970jAaLoUMwk01fKtuMxBynxCDbeOzM9nMLzGLcQScBl892vsek1owt0XQK+Ke5N2HiAToJROs1MG/AReCci79QUpssqrQPYa2lwdCN61rxgfR2ppXvIx0w4s164ZOY7RrwVp78l9fN8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1755168826; c=relaxed/simple; bh=23C6Roavw77GaOk3ab46nO9WJLIY4s287YqaHdgXSQA=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=G7cICB/7ebRqLcUYreZuTT7PyyQ3zUeJoCIcOjQTatrp+lJhGPSL7amVXsiR6+EHRhrAK5UKFNdCcnwgKqOGYblba8jVcsAtVrwjHO786ehQ/7QQxlWhJjx/zTrnu0GrCMTKxxiK1f5RzNApIQ28BdtNHHsKt7R4OxtX/779Ars= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BDBFE3858D3C Received: by mail-ej1-x631.google.com with SMTP id a640c23a62f3a-afcb78fb04cso125119566b.1 for ; Thu, 14 Aug 2025 03:53:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sankhya-com.20230601.gappssmtp.com; s=20230601; t=1755168824; x=1755773624; darn=sourceware.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=4N/HqUHqRelk1F8XuZnSrO2l6M1jfKv/fF4nvk1KXk4=; b=ZzYIMUGYAdZBHpVq8LcF26DzIdw3eC5LukhfEVYjX7V8pXI4hpp0RnchRDCzU9+me8 s1qQOwJSzRbNXb9xOcB/Bwba+Mow26WWoLwzpQd5YBTYK0dGGbOrwYVQl/aHrfLRDsV/ 2OBd9Fvjva9lpsq1IBgc7CoMT0z3dvTCpefnoGcbo8veUaDmtWQqwgbQzqOO6bH2pGbV hEXCp0YwSzu0jvMOm/F9/A2RKuU9stLUxpRYmdQMU5FiOcYmT3odHXcDOfBXIFHpXEkC +mw7+6hWMODu3iNBbwgsZKdzL8IEe3I87vL8Wwc2wHk6I0ehc/WS9854pODe64fqhYc9 Y/FQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1755168824; x=1755773624; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=4N/HqUHqRelk1F8XuZnSrO2l6M1jfKv/fF4nvk1KXk4=; b=iQBBz7qQkyaX1mZVkKmdDerNcRv5XWD04gYBRWjuN1hu4Vtclzo6jh24bWLg++XWOx u/tqd5s5n2tDX44443GQ02X5BkBELvCDlFaGk2+lM7+BPXSvwhmxBbC7Ge8AKJN1POkh h+5+3gKEMbugXhaSB6lqL491p0OjKTB4mhpADCkHX78kfvxs2JYgvv6y8V5R54qEAAKy okRb5ly/NXN/7/RsKj06qHSrbAkKjYqKxd9YPosLdkhM+AAV1I5QdWWcBc50acEp9gIR nAVUi6TiySAoNLyOg4aQJg3JiN1wKK9M/7NhIy/GH0fSkE4iHg0Xo+kYzGGYQ9//y+FO j7kQ== X-Gm-Message-State: AOJu0YyWlCk/AfXFGy2Wxj+4WbHipa7Z3g4UU6ida441F+P3ckK502sE TARli/is61xJkD8gG90gy5BjL3uZ83m3+U1qSCrqrDoJsgk+I7W55pwbQuLSpwvpasC66jFsasQ 4aGf8w2EUR4K7bY8d1SmaJL4jlDAqryYcjdnZoQAHeVJO7hTGL22qwDudNA== X-Gm-Gg: ASbGncsw/heyTy/EQxAsuuaQDb7GXGxOLB3ZpRTX2dT3HJ3XTBIy0tmmnK09o3XLiOu 0vCDvVS5zUK9dz0qRpb5LRaizvRV+6mFeWDA0rTO54JRsclwSdajTNhfYMPIms13VJ+aB4myDB8 b4lur9T8W0Ydv7mbdm7j0gWE5U/g+aSS1ZpPbDYjr/rMQ045BbD3ZY1XYFWj/HT73yNTYhnL2Mw UmldE2d X-Google-Smtp-Source: AGHT+IHlG1WXhtfYOInvrQRq8S70SCFMFPpyig50sWaC8d5sgxpfpyvDuUZssqyqfnZIYxWwC7SYNXXGFrLo3ibNW7k= X-Received: by 2002:a17:907:3f9a:b0:af9:6bfb:58b7 with SMTP id a640c23a62f3a-afcb981e496mr261635666b.5.1755168823916; Thu, 14 Aug 2025 03:53:43 -0700 (PDT) MIME-Version: 1.0 References: <3bdebc70-e678-44e7-98ec-18c6b23dccca@simark.ca> In-Reply-To: <3bdebc70-e678-44e7-98ec-18c6b23dccca@simark.ca> From: Gopi Kumar Bulusu Date: Thu, 14 Aug 2025 15:58:01 +0530 X-Gm-Features: Ac12FXxZ54h_anp2ydhnrQxa9rukx90upQu8HKqO1-9_eVnCkJIB2KKNTI6cz2I Message-ID: Subject: Re: [PATCH] MicroBlaze: Add microblaze_software_single_step To: Simon Marchi Cc: gdb-patches@sourceware.org, Michael Eager Content-Type: multipart/alternative; boundary="000000000000454cfc063c511639" 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 --000000000000454cfc063c511639 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Aug 14, 2025 at 1:41=E2=80=AFAM Simon Marchi wro= te: > > > 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, cons= t > 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 =3D 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 !=3D 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 gdbarc= h > *gdbarch, struct type *type) > > return (type->length () =3D=3D 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 =3D 0; > > + > > + if (regno >=3D 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 >=3D 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 =3D regcache_raw_get_unsigned(regcache, regno); > > Missing paren. > > > + } > > + > > + return regval; > > +} > > + > > +/* Return next pc values : next in sequence and/or branch/return targe= t > */ > > +static std::vector > > +microblaze_software_single_step (regcache *regcache) > > Empty line after comment. Finish comment with period and two spaces. > > ok > > +{ > > + > > + gdbarch *arch =3D 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 =3D 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 =3D regcache_read_pc (regcache); > > + insn =3D microblaze_fetch_instruction (pc); > > + minstr =3D get_insn_microblaze (insn, &isunsignednum, &insn_type= , > &delay_slots); > > + /* If the current instruction is an imm, look at the inst after = */ > > + if (insn_type =3D=3D immediate_inst) > > + { > > + int rd, ra, rb; > > + immfound =3D true; > > + minstr =3D microblaze_decode_insn (insn, &rd, &ra, &rb, &imm); > > + pc =3D pc + INST_WORD_SIZE; > > pc +=3D INST_WORD_SIZE; > > ok ! > > + insn =3D microblaze_fetch_instruction (pc); > > + minstr =3D get_insn_microblaze (insn, &isunsignednum, &insn_typ= e, > &delay_slots); > > + } > > + > > + candidates[0].valid =3D (insn_type !=3D return_inst); > > + > > + /* Compute next instruction address - skip delay slots if any */ > > + candidates[0].address =3D pc + INST_WORD_SIZE + > > + (delay_slots * INST_WORD_SIZE); > > + > > + microblaze_debug ("single-step insn_type=3D%x pc=3D%lx insn=3D%l= x\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 =3D false; > > + if (insn_type =3D=3D branch_inst || insn_type =3D=3D 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 =3D microblaze_getreg (regcache, lra); > > + rb =3D microblaze_getreg (regcache, lrb); > > + > > + candidates[1].address =3D microblaze_get_target_address ( > > + insn, immfound, imm, pc, ra, rb, > > + &targetvalid, > &unconditionalbranch); > > + > > + microblaze_debug ( > > + "single-step uncondbr=3D%d targetvalid=3D%d target=3D%lx\n", > > + unconditionalbranch, targetvalid, candidates[1].address); > > + > > + /* Can reach next address ? */ > > + candidates[0].valid =3D > > + candidates[0].valid && (unconditionalbranch =3D=3D FALSE); > > + > > + /* Can reach a distinct (not here) target address ? */ > > + candidates[1].valid =3D FALSE; > > candidates[1].valid was already set to false above. > Yes, will remove. > > > + if (targetvalid && > > + candidates[1].address !=3D pc && > > + (candidates[0].valid =3D=3D FALSE || > > + (candidates[0].address !=3D candidates[1].address))) > > + { > > + candidates[1].valid =3D TRUE; > > + } > > Use true/false instead of TRUE/FALSE. > ok. > > > + } /* if (branch or return instruction) */ > > + > > + /* Create next_pcs vector */ > > + for (int ci =3D 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 --000000000000454cfc063c511639 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
=C2=A0

On Thu, A= ug 14, 2025 at 1:41=E2=80=AFAM 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 b= etween
> /
> /AMD/Xilinx and=C2=A0 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 (microblaze= el-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.=C2=A0 Just describe wh= at the
patch adds.

ok.
=C2=A0

> 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, con= st frame_info_ptr &next_frame)
>=C2=A0 =C2=A0 gdb_byte buf[4];
>=C2=A0 =C2=A0 CORE_ADDR pc;
>=C2=A0
> +=C2=A0 microblaze_debug ("microblaze_unwind_pc called\n");<= br>
Could you please make a patch that changes microblaze_debug to align it
with our current best practices:
=C2=A0 - rename to microblaze_debug_printf
=C2=A0- change the log tag from MICROBLAZE to microblaze
=C2=A0- change the macro to use debug_prefixed_printf_cond instead of
=C2=A0 =C2=A0debug_prefixed_printf_cond_nofunc (so that the function name i= s
=C2=A0 =C2=A0automatically included)
=C2=A0- update the calls to not add the function name manually
=C2=A0- update the calls to not include a trailing \n
=
=C2=A0
Will look into this for a separate patch. Will dr= op the debug statements in patch v2.


IMO, I don't find this "called" logging statement by itself v= ery 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).=C2=A0 It's not related to adding
microblaze_software_single_step.

Ok.
=C2=A0

> @@ -423,8 +429,13 @@ microblaze_frame_cache (const frame_info_ptr &= ;next_frame, void **this_cache)
>=C2=A0 =C2=A0 struct gdbarch *gdbarch =3D get_frame_arch (next_frame);<= br> >=C2=A0 =C2=A0 int rn;
>=C2=A0
> -=C2=A0 if (*this_cache)
> +=C2=A0 microblaze_debug ("microblaze_frame_cache called.\n"= );

Same comment about "called".=C2=A0 I would at least log next_fram= e's number.

ok.
=C2=A0
> +
> +=C2=A0 if (*this_cache) {
> +=C2=A0 =C2=A0 microblaze_debug ("microblaze_frame_cache: returni= ng cache hit\n");
>=C2=A0 =C2=A0 =C2=A0 return (struct microblaze_frame_cache *) *this_cac= he;
> +=C2=A0 }

Formatting and other nits:

=C2=A0 if (*this_cache !=3D null)
=C2=A0 =C2=A0 {
=C2=A0 =C2=A0 =C2=A0 microblaze_debug_printf ("returning cache hit&quo= t;);=C2=A0 // do you want to log some information about the result here, li= ke you do below?
=C2=A0 =C2=A0 =C2=A0 return (struct microblaze_frame_cache *) *this_cache;<= br> =C2=A0 =C2=A0 }

I will drop the unrelat= ed debug log changes in V2 as mentioned above.


> @@ -590,7 +603,116 @@ microblaze_stabs_argument_has_addr (struct gdbar= ch *gdbarch, struct type *type)
>=C2=A0 =C2=A0 return (type->length () =3D=3D 16);
>=C2=A0 }
>=C2=A0
> -=0C
> +long
> +microblaze_getreg(struct regcache *regcache, int regno)

Does this really compile?=C2=A0 You don't get a compiler warning about<= br> missing prototype or something like that?

=C2=A0Oops, I will clean up the warnings for v2.


Missing space before parenthesis.

Please add a comment explaining what the function does and returns.

> +{
> +=C2=A0 =C2=A0 =C2=A0long regval =3D 0;
> +
> +=C2=A0 =C2=A0 =C2=A0if (regno >=3D 0 && regno < MICROBL= AZE_NUM_REGS)

I think that calling this function with an invalid regno would be an
internal error.=C2=A0 IOW you could do:

=C2=A0 gdb_assert (regno >=3D 0 && regno < MICROBLAZE_NUM_REG= S);

Or don't because the regcache will do it itself (see
reg_buffer::assert_regnum).=C2=A0 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

=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0regval =3D regcache_raw_get_unsigne= d(regcache, regno);

Missing paren.

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0return regval;
> +}
> +
> +/* Return next pc values : next in sequence and/or branch/return targ= et */
> +static std::vector<CORE_ADDR>
> +microblaze_software_single_step (regcache *regcache)

Empty line after comment.=C2=A0 Finish comment with period and two spaces.<= br>
ok
=C2=A0
> +{
> +
> +=C2=A0 =C2=A0 =C2=A0 gdbarch *arch =3D regcache->arch ();

Please fix the indentation in this while function to match the coding
style.

> +
> +=C2=A0 =C2=A0 =C2=A0 CORE_ADDR pc;
> +=C2=A0 =C2=A0 =C2=A0 std::vector<CORE_ADDR> next_pcs;
> +=C2=A0 =C2=A0 =C2=A0 long insn;
> +=C2=A0 =C2=A0 =C2=A0 enum microblaze_instr minstr;
> +=C2=A0 =C2=A0 =C2=A0 enum microblaze_instr_type insn_type;
> +=C2=A0 =C2=A0 =C2=A0 short delay_slots;
> +=C2=A0 =C2=A0 =C2=A0 int imm;
> +=C2=A0 =C2=A0 =C2=A0 bool isunsignednum;
> +=C2=A0 =C2=A0 =C2=A0 bool immfound =3D false;
> +=C2=A0 =C2=A0 =C2=A0 struct address_candidates
> +=C2=A0 =C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0CORE_ADDR address;
> +=C2=A0 =C2=A0 =C2=A0bool valid;
> +=C2=A0 =C2=A0 =C2=A0 } candidates [2];

Declare variables at first use.

Will ch= eck and change wherever appropriate.
=C2=A0

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_t= arget_insn".

Also, instead of a "valid" flag, they could probably be
std::optional<CORE_ADDR>.

Will re= write - did not want to significantly change the original code that was par= t of the Xilinx repo for long.
=C2=A0

> +
> +=C2=A0 =C2=A0 =C2=A0 /* Get instruction */
> +=C2=A0 =C2=A0 =C2=A0 pc =3D regcache_read_pc (regcache);
> +=C2=A0 =C2=A0 =C2=A0 insn =3D microblaze_fetch_instruction (pc);
> +=C2=A0 =C2=A0 =C2=A0 minstr =3D get_insn_microblaze (insn, &isuns= ignednum, &insn_type, &delay_slots);
> +=C2=A0 =C2=A0 =C2=A0 /* If the current instruction is an imm, look at= the inst after */
> +=C2=A0 =C2=A0 =C2=A0 if (insn_type =3D=3D immediate_inst)
> +=C2=A0 =C2=A0 =C2=A0{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0int rd, ra, rb;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0immfound =3D true;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0minstr =3D microblaze_decode_insn (insn, &= amp;rd, &ra, &rb, &imm);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0pc =3D pc + INST_WORD_SIZE;

=C2=A0 pc +=3D INST_WORD_SIZE;


ok !
=C2=A0
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0insn =3D microblaze_fetch_instruction (pc)= ;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0minstr =3D get_insn_microblaze (insn, &= ;isunsignednum, &insn_type, &delay_slots);
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0 candidates[0].valid =3D (insn_type !=3D return_i= nst);
> +
> +=C2=A0 =C2=A0 =C2=A0 /* Compute next instruction address - skip delay= slots if any */
> +=C2=A0 =C2=A0 =C2=A0 candidates[0].address =3D pc + INST_WORD_SIZE +<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0(delay_slots * INST_WORD_SIZE);
> +
> +=C2=A0 =C2=A0 =C2=A0 microblaze_debug ("single-step insn_type=3D= %x pc=3D%lx insn=3D%lx\n",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0insn_type, pc, insn);

I would suggest prefixing the hex values with "0x" to make it cle= ar
(valid for the other logging statements below as well).

ok
=C2=A0

I think it would be good to add a function "microblaze_instr_type_str&= quot;
and then use it to print the insn_type value.

Not sure if that is appropriate. I added some of these log statement= s 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 functional= ity.
=C2=A0
static const char *
microblaze_instr_type_str (microblaze_instr_type type)
{
=C2=A0 switch (type)
=C2=A0 =C2=A0 {
=C2=A0 =C2=A0 case arithmetic_inst:
=C2=A0 =C2=A0 =C2=A0 return "arithmetic";
=C2=A0 =C2=A0 case logical_inst:
=C2=A0 =C2=A0 =C2=A0 return "logical";
=C2=A0 =C2=A0 case mult_inst:
=C2=A0 =C2=A0 =C2=A0 return "mult";
=C2=A0 =C2=A0 case div_inst:
=C2=A0 =C2=A0 =C2=A0 return "div";
=C2=A0 =C2=A0 case branch_inst:
=C2=A0 =C2=A0 =C2=A0 return "branch";
=C2=A0 =C2=A0 case return_inst:
=C2=A0 =C2=A0 =C2=A0 return "return";
=C2=A0 =C2=A0 case immediate_inst:
=C2=A0 =C2=A0 =C2=A0 return "immediate";
=C2=A0 =C2=A0 case special_inst:
=C2=A0 =C2=A0 =C2=A0 return "special";
=C2=A0 =C2=A0 case memory_load_inst:
=C2=A0 =C2=A0 =C2=A0 return "memory_load";
=C2=A0 =C2=A0 case memory_store_inst:
=C2=A0 =C2=A0 =C2=A0 return "memory_store";
=C2=A0 =C2=A0 case barrel_shift_inst:
=C2=A0 =C2=A0 =C2=A0 return "barrel_shift";
=C2=A0 =C2=A0 case anyware_inst:
=C2=A0 =C2=A0 =C2=A0 return "anyware";
=C2=A0 =C2=A0 default:
=C2=A0 =C2=A0 =C2=A0 return "<unknown>";
=C2=A0 =C2=A0 }
}

> +
> +=C2=A0 =C2=A0 =C2=A0 /* Compute target instruction address for branch= or return instruction */
> +=C2=A0 =C2=A0 =C2=A0 candidates[1].valid =3D false;
> +=C2=A0 =C2=A0 =C2=A0 if (insn_type =3D=3D branch_inst || insn_type = =3D=3D return_inst)
> +=C2=A0 =C2=A0 =C2=A0{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0int limm;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0int lrd, lra, lrb;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0long ra, rb;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0bfd_boolean targetvalid;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0bfd_boolean unconditionalbranch;

Looking at the signature of microblaze_get_target_address, I think those bfd_boolean could be bool instead.

Will= check and change.
=C2=A0

> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0microblaze_decode_insn(insn, &lrd, &am= p;lra, &lrb, &limm);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0ra =3D microblaze_getreg (regcache, lra);<= br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0rb =3D microblaze_getreg (regcache, lrb);<= br> > +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0candidates[1].address =3D microblaze_get_t= arget_address (
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0insn, immfou= nd, imm, pc, ra, rb,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0&targetv= alid, &unconditionalbranch);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0microblaze_debug (
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0"single-step uncondbr=3D%d tar= getvalid=3D%d target=3D%lx\n",
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unconditionalbranch, targetvalid, c= andidates[1].address);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Can reach next address ? */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0candidates[0].valid =3D
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0candidates[0].valid && (unc= onditionalbranch =3D=3D FALSE);
> +
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Can reach a distinct (not here) target = address ? */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0candidates[1].valid =3D FALSE;

candidates[1].valid was already set to false above.
Yes, will remove.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 =C2=A0if (targetvalid &&
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0candidates[1].= address !=3D pc &&
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(candidates[0]= .valid =3D=3D FALSE ||
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(candid= ates[0].address !=3D candidates[1].address)))
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0{
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0candidates[1].valid =3D TRUE= ;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}

Use true/false instead of TRUE/FALSE.

o= k.
=C2=A0

> +=C2=A0 =C2=A0 =C2=A0 } /* if (branch or return instruction) */
> +
> +=C2=A0 =C2=A0 =C2=A0 /* Create next_pcs vector=C2=A0 */
> +=C2=A0 =C2=A0 =C2=A0 for (int ci =3D 0; ci < 2; ci++)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (candidates[ci].valid)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0next_pcs.push_back (c= andidates[ci].address);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0microblaze_debug (&qu= ot;push_back (%lx)\n", candidates[ci].address);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

Drop the outer curly braces (the braces for the "for").

ok
=C2=A0

Simon

Thanks a ton for the quick review= .

dhanyavaadaaha
gopi

=C2=A0
--000000000000454cfc063c511639--