Re: [PATCH] PowerPC, add support for printing non-trivial C++ object for the finish command.

[Joel Brobecker <brobecker@adacore.com>]

Hello,

> The following patch fixes an issue on Powerpc where the function return
> value for a C++ program prints garbage when the function return type is
> a class object.  The fix is based on an x86-64 patch for the same
> issue.
> 
> The patch has been tested on Power 10 with no regressions.  Please let
> me know if this patch is acceptable for mainline.   Thanks.

Thanks for the patch.

I don't know the precise specifications regarding returning C++ objects
on PowerPC targets, but I think we can trust you on that. For the rest,
the patch looks OK to me.

Maybe one small teeny weeny wittle nitpick, erm, I mean, suggestion.
Which lead me to realize that there was a small GNU Coding Style issue.
See below:

> 
>                    Carl Love
> ---------------------------------------------------
> PowerPC, add support for printing non-trivial C++ object for the finish command.
> 
> This patch fixes five testcase failures in gdb/cpp/non-trival-retval.exp.
> The testcases that fail were added by commit:
> 
>   commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608
>   Author: Andrew Burgess <aburgess@redhat.com>
>   Date:   Mon Dec 13 16:56:16 2021 +0000
> 
>       gdb: on x86-64 non-trivial C++ objects are returned in memory
> 
>       Fixes PR gdb/28681.  It was observed that after using the `finish`
>       command an incorrect value was displayed in some cases.  Specifically,
>       this behaviour was observed on an x86-64 target.
> 
> The commit adds new tests for printing the return value of a C++ function.
> The return value of the function is a C++ Class.  The current PowerPC code
> is looking for the return value in register r3.  The return value is actually
> in memory as described in the commit message for x86-64 above.
> 
> The fix for Powerpc is based on the fix in the above commit plus examining
> how x86-64 handles the value in function amd64_return_value in file
> gdb/amd64-tdep.c.
> ---
>  gdb/ppc-sysv-tdep.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> index 9a3b02f028d..50277e7bdd1 100644
> --- a/gdb/ppc-sysv-tdep.c
> +++ b/gdb/ppc-sysv-tdep.c
> @@ -2035,6 +2035,20 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
>        return RETURN_VALUE_REGISTER_CONVENTION;
>      }
>  
> +  /* If the object is a non-trivial C++ object the object is in memory. */

nitpick: I would add a coma after "non-trivial C++ object".

Also, can you add a second space after the period, before the "*/"?
(it's a GNU Coding Style requirement)

Thank you!

> +  if (!language_pass_by_reference (valtype).trivially_copyable)
> +    {
> +      if (readbuf)
> +        {
> +          ULONGEST addr;
> +	  int regnum = tdep->ppc_gp0_regnum + 3;
> +
> +          regcache_raw_read_unsigned (regcache, regnum, &addr);
> +          read_memory (addr, readbuf, TYPE_LENGTH (valtype));
> +        }
> +      return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> +    }
> +
>    /* In the ELFv2 ABI, aggregate types of up to 16 bytes are
>       returned in registers r3:r4.  */
>    if (tdep->elf_abi == POWERPC_ELF_V2
> -- 
> 2.32.0
> 
> 

-- 
Joel