From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x329.google.com (mail-wm1-x329.google.com [IPv6:2a00:1450:4864:20::329]) by sourceware.org (Postfix) with ESMTPS id 954DC3858D1E for ; Sun, 6 Mar 2022 11:53:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 954DC3858D1E Received: by mail-wm1-x329.google.com with SMTP id k29-20020a05600c1c9d00b003817fdc0f00so7686790wms.4 for ; Sun, 06 Mar 2022 03:53:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=eatRtbhfLTYYKAYw/gNsoy7MrDUCrxMeyYLnthL2vYE=; b=21f7mAZhmAAir71Cwy7BuecRNh+AprIpA5Bp4qE/9uTjvDLV7iWUA+M1IztJbAUI3w MGdEsduR5iiZXmdLG0GeeqG7fzAnfxhd46toep7x0z1MsbRnT/tE2cCKY7Unp/XDYX9y gDlWTt1q75+MUHuV+0t9M3bPY+TSY2xeI8qSYGjVPYJ6INRaL1/K4nIBhuTsmjC2P1k3 nB0kdKPVR1fr1ULnCIVEFJO10mqti0cQV4CxoaxYS6+P2ZgrTByN9IJMwLTsvWBZ79kH ADXsLS5u7M1OBoI4anSqsP5ZCLek6rDkOzEyGlmJb03NI/93cpriZy0bzwFfHY2UgnWO bekg== X-Gm-Message-State: AOAM530LZui7gYZPw7fRD0ZYdFxB45KrmJQu+N/VAVrTG4xqzdL4ZOg7 PGsOlG/IvH6VAa9I791jod5C X-Google-Smtp-Source: ABdhPJxIjyozqvNH/ry25E611dS3ZMUlJOO/yVMEXmQuhN0Ei25l86uVJi6K5v02m1GxYFlMeLx4Ng== X-Received: by 2002:a7b:c383:0:b0:381:1b50:a9d with SMTP id s3-20020a7bc383000000b003811b500a9dmr5587944wmj.90.1646567619638; Sun, 06 Mar 2022 03:53:39 -0800 (PST) Received: from takamaka.home (lfbn-reu-1-503-119.w92-130.abo.wanadoo.fr. [92.130.90.119]) by smtp.gmail.com with ESMTPSA id g1-20020a1c4e01000000b003899c8053e1sm5378064wmh.41.2022.03.06.03.53.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 06 Mar 2022 03:53:39 -0800 (PST) Received: by takamaka.home (Postfix, from userid 1000) id 1A610A4A87; Sun, 6 Mar 2022 15:53:36 +0400 (+04) Date: Sun, 6 Mar 2022 15:53:36 +0400 From: Joel Brobecker To: Carl Love via Gdb-patches Cc: Ulrich Weigand , Rogerio Alves , Tulio Magno , Joel Brobecker Subject: Re: [PATCH 1/2] Add recording support for the ISA 3.1 Powerpc instructions Message-ID: References: <7ff0655bead2a8245018fbf9624f207cd38a5b7f.camel@us.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 06 Mar 2022 11:53:42 -0000 Hello, On Fri, Mar 04, 2022 at 11:52:43AM -0800, Carl Love via Gdb-patches wrote: > GDB maintainers: > > The gdb record functionality does not have support for the the > PowerpcISA 3.1 instructions. The following patch adds the missing gdb > record support for power. > > As mentioned in message [0/2], I have used the Valgrind ISA 3.1 > testsuite to verify that all of the ISA 3.1 instructions are recognized > by gdb record with this patch applied. Please let me know if this patch > is accepatble to commit to the gdb mainline repository. Thanks. > > Carl Love > > ------------------------------------------------ > > Add recording support for the ISA 3.1 Powerpc instructions. > > This patch adds support for the Powerpc ISA 3.1 instructions to the Powerpc > gdb instruction recording routines. Case statement entries are added to a > number of the existing routines for recording the 32-bit word instructions. > A few new functions were added to handle the new word instructions. The 64-bit > prefix instructions are all handled by a set of new routines. The function > ppc_process_prefix_instruction() is the primary function to handle the > prefixed instructions. It calls additional functions to handle specific > sets of prefixed instructions. These new functions are: > > ppc_process_record_prefix_vsx_d_form(), > ppc_process_record_prefix_store_vsx_ds_form(), > ppc_process_record_prefix_op34(), > ppc_process_record_prefix_op33(), > ppc_process_record_prefix_op32(), > ppc_process_record_prefix_store(), > ppc_process_record_prefix_op59_XX3(), > ppc_process_record_prefix_op42(). Thanks for this patch. I mostly skimmed through the patch, as I do not know the PowerPC architecture all that well, especially the newer ISAs. My general remarks is that the code looks reasonable, and you seem to have tested it relatively well. So I'll focus on the higher-level comments. In particular, I think every new function we introduce deserves an intro comment describing what the function does, and documenting all the parameters and return value. > @@ -4152,6 +4170,52 @@ ppc_record_vsr (struct regcache *regcache, ppc_gdbarch_tdep *tdep, int vsr) > return 0; > } > > +#define RECORD_FPSCR 1 > +#define DO_NOT_RECORD_FPSCR 0 I think these macros deserves documentation as well. > + NOTE: > + In ISA 3.1 the ACC is mapped on top of VSR[0] thru VSR[31]. > + > + In the future, the ACC may be implemented as an independent register file > + rather then mapping on top of the VSRs. This will then require the ACC to Small typo: "then" -> "than". Also, can you add an extra space after the period (just before "This will [...]" above)? > + be assigned its on register number and the ptrace interface to be able > + access the ACC. > + */ > + > + /* ACC maps over the same VSR space as the fp registers. */ > + for (i = 0; i<4; i++) { GNU Coding style: Can you place the opening curly brace on the next line, please, indented 2 characters relative to the "for". You'll probably want to adjust the indentation of the block accordingly, including the closing curly brace. > @@ -5174,15 +5517,13 @@ ppc_process_record_op31 (struct gdbarch *gdbarch, struct regcache *regcache, > return -1; > } > > -/* Parse and record instructions of primary opcode-59 at ADDR. > - Return 0 if successful. */ > - > static int > ppc_process_record_op59 (struct gdbarch *gdbarch, struct regcache *regcache, Can you explain why you are removing this function documentation? Is it now OBE? Either way, can you make sure we either keep the doc, or replace it with an updated version? > + > + if (type == 1) { > + if (ST4 == 0) { GNU Coding Style: Same as the "for" above. Can you go through perhaps your patch for me, to see if I might have missed some other style issues? Let me give you, JIC, the link to our coding standards: https://sourceware.org/gdb/wiki/Internals%20Coding-Standards In particular, the C/C++ CS: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards > + } else Coding standard: The "else" should be on the next line. > + return -1; > + > + } else if (type == 2) { Same here, please, and also watch out for the training "{" which should also be moved to the next line. The following are also other coding style violations I noticed through skimming your patch: > + if (type == 1) { > + } else > + if (type == 1) { > + } else if (type == 2) { > + } else > + if ((type == 0) && (ST1 == 0)) { > + if (R == 0) { > + } else { > + if ((type == 0) && (ST1 == 0)) { > + if (R == 0) { > + } else { > + } else Hopefully you can do a pass of your own and catch the ones I didn't see. Thank you, -- Joel