Re: [PATCH 1/2] Add recording support for the ISA 3.1 Powerpc instructions

[Joel Brobecker <brobecker@adacore.com>]

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