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

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

[Carl Love]

GDB maintainers:

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.

                   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. */
+  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

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

[Joel Brobecker]

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

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

[Luis Machado]

On 3/11/22 22:26, Carl Love via Gdb-patches wrote:
> 
> GDB maintainers:
> 
> 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.
> 
>                     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. */
> +  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

Carl,

Just a heads-up that I pushed a new test 
(gdb.base/retval-large-struct.exp) that exercises returning a large 
struct (> 16 bytes) from a function. You might want to validate that 
test on ppc as well, given this particular scenario wasn't being 
validated properly in the testsuite.

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

[Tom Tromey]

>>>>> "Carl" == Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:

Carl> The following patch fixes an issue on Powerpc where the function return
Carl> value for a C++ program prints garbage when the function return type is
Carl> a class object.  The fix is based on an x86-64 patch for the same
Carl> issue.

Carl> The patch has been tested on Power 10 with no regressions.  Please let
Carl> me know if this patch is acceptable for mainline.   Thanks.

FWIW, here at AdaCore, we carry a very similar patch for
ppc-linux-tdep.c.  I've appended it.

I haven't really submitted this yet since I wanted to investigate the
ABI difference that lead to the change.

Anyway I tend to think your patch is fine.

Tom

commit 693bb3df1fdf2de96e5a6ce338712d6ba370d0c9
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Tue Jan 7 06:07:41 2020 -0500

    GDB on ppc-linux is unable to print return value when value is struct/union
    
    Consider the following C code:
    
        | struct S2 { short s1, s2; };
        | struct S2 function_2 (void)
        | {
        |   struct S2 x = { 27, 28 };
        |   return x;
        | }
    
    When using the "finish" command from this function, GDB currently
    says it cannot print the return value:
    
        | (gdb) finish
        | Run till exit from #0  function_2 () at finish-struct.c:13
        | 0x1000055c in main () at finish-struct.c:20
        | 20    function_2 ();
        | Value returned has type: struct S2. Cannot determine contents
    
    As it happens, the ABI being used on PowerPC Linux is such that
    the address of the returned value is returned via register r3.
    So we can print its contents.
    
    gdb/ChangeLog:
    
            * ppc-linux-tdep.c (ppc_linux_return_value): Fix handling
            of return values which are either structs, unions, or vectors
            with a size different from 8 or 16.
    
    gdb/testsuite/ChangeLog:
    
            * gdb.base/finish-struct.c: New file.
            * gdb.base/finish-struct.exp: New files.
    
    Change-Id: I0b336739f6a1929c1b8910aeb87f2e4dfa504bef
    TN: SC20-032

diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index 901b938f257..9e0d4902595 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -254,7 +254,19 @@ ppc_linux_return_value (struct gdbarch *gdbarch, struct value *function,
        || TYPE_CODE (valtype) == TYPE_CODE_UNION)
       && !((TYPE_LENGTH (valtype) == 16 || TYPE_LENGTH (valtype) == 8)
 	   && TYPE_VECTOR (valtype)))
-    return RETURN_VALUE_STRUCT_CONVENTION;
+    {
+      if (readbuf)
+	{
+	  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+	  ULONGEST addr;
+
+	  regcache_raw_read_unsigned (regcache, tdep->ppc_gp0_regnum + 3,
+				      &addr);
+	  read_memory (addr, readbuf, TYPE_LENGTH (valtype));
+	}
+
+      return RETURN_VALUE_ABI_RETURNS_ADDRESS;
+    }
   else
     return ppc_sysv_abi_return_value (gdbarch, function, valtype, regcache,
 				      readbuf, writebuf);
diff --git a/gdb/testsuite/gdb.base/finish-struct.c b/gdb/testsuite/gdb.base/finish-struct.c
new file mode 100644
index 00000000000..2af02a1a1a3
--- /dev/null
+++ b/gdb/testsuite/gdb.base/finish-struct.c
@@ -0,0 +1,40 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+struct S1 { char c1, c2, c3, c4; };
+
+struct S2 { short s1, s2; };
+
+struct S1
+function_1 (void)
+{
+  struct S1 x = { 'A', 'B', 'C', 'D' };
+  return x;
+}
+
+struct S2
+function_2 (void)
+{
+  struct S2 x = { 27, 28 };
+  return x;
+}
+
+int main ()
+{
+  function_1 ();
+  function_2 ();
+}
diff --git a/gdb/testsuite/gdb.base/finish-struct.exp b/gdb/testsuite/gdb.base/finish-struct.exp
new file mode 100644
index 00000000000..b4e6875e5a7
--- /dev/null
+++ b/gdb/testsuite/gdb.base/finish-struct.exp
@@ -0,0 +1,47 @@
+# Copyright 2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the gdb testsuite.  It is intended to test that
+# gdb can correctly print arrays with indexes for each element of the
+# array.
+
+standard_testfile .c
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "failed to compile"
+    return -1
+}
+
+clean_restart ${binfile}
+
+gdb_breakpoint function_1
+gdb_breakpoint function_2
+
+# Run the program until we reach the first breakpoint, which should be
+# in function_1, and then test the "finish" command from there.
+
+gdb_run_cmd
+gdb_test "" "Breakpoint \[0-9\]+,.*function_1.*" "run to function_1"
+
+gdb_test "finish" \
+    " = {c1 = 65 'A', c2 = 66 'B', c3 = 67 'C', c4 = 68 'D'}" \
+    "finish from function_1"
+
+# Continue the program's execution until reaching the breakpoint inside
+# function_2, and then test the "finish" command from there.
+
+gdb_test "continue" "Breakpoint \[0-9\]+,.*function_2.*" "run to function_2"
+
+gdb_test "finish" " = {s1 = 27, s2 = 28}" "finish from function_2"

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

[Luis Machado]

On 3/14/22 13:40, Tom Tromey via Gdb-patches wrote:
>>>>>> "Carl" == Carl Love via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Carl> The following patch fixes an issue on Powerpc where the function return
> Carl> value for a C++ program prints garbage when the function return type is
> Carl> a class object.  The fix is based on an x86-64 patch for the same
> Carl> issue.
> 
> Carl> The patch has been tested on Power 10 with no regressions.  Please let
> Carl> me know if this patch is acceptable for mainline.   Thanks.
> 
> FWIW, here at AdaCore, we carry a very similar patch for
> ppc-linux-tdep.c.  I've appended it.
> 
> I haven't really submitted this yet since I wanted to investigate the
> ABI difference that lead to the change.

I wonder if the test I pushed, gdb.base/retval-large-struct.exp, 
exercises the same case your attached testcase.

There may be more targets running into these issues, but it seems the 
testsuite wasn't exercising this particular case properly (or something 
else changed along the way).

> 
> Anyway I tend to think your patch is fine.
> 
> Tom
> 
> commit 693bb3df1fdf2de96e5a6ce338712d6ba370d0c9
> Author: Joel Brobecker <brobecker@adacore.com>
> Date:   Tue Jan 7 06:07:41 2020 -0500
> 
>      GDB on ppc-linux is unable to print return value when value is struct/union
>      
>      Consider the following C code:
>      
>          | struct S2 { short s1, s2; };
>          | struct S2 function_2 (void)
>          | {
>          |   struct S2 x = { 27, 28 };
>          |   return x;
>          | }
>      
>      When using the "finish" command from this function, GDB currently
>      says it cannot print the return value:
>      
>          | (gdb) finish
>          | Run till exit from #0  function_2 () at finish-struct.c:13
>          | 0x1000055c in main () at finish-struct.c:20
>          | 20    function_2 ();
>          | Value returned has type: struct S2. Cannot determine contents
>      
>      As it happens, the ABI being used on PowerPC Linux is such that
>      the address of the returned value is returned via register r3.
>      So we can print its contents.
>      
>      gdb/ChangeLog:
>      
>              * ppc-linux-tdep.c (ppc_linux_return_value): Fix handling
>              of return values which are either structs, unions, or vectors
>              with a size different from 8 or 16.
>      
>      gdb/testsuite/ChangeLog:
>      
>              * gdb.base/finish-struct.c: New file.
>              * gdb.base/finish-struct.exp: New files.
>      
>      Change-Id: I0b336739f6a1929c1b8910aeb87f2e4dfa504bef
>      TN: SC20-032
> 
> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
> index 901b938f257..9e0d4902595 100644
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -254,7 +254,19 @@ ppc_linux_return_value (struct gdbarch *gdbarch, struct value *function,
>          || TYPE_CODE (valtype) == TYPE_CODE_UNION)
>         && !((TYPE_LENGTH (valtype) == 16 || TYPE_LENGTH (valtype) == 8)
>   	   && TYPE_VECTOR (valtype)))
> -    return RETURN_VALUE_STRUCT_CONVENTION;
> +    {
> +      if (readbuf)
> +	{
> +	  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +	  ULONGEST addr;
> +
> +	  regcache_raw_read_unsigned (regcache, tdep->ppc_gp0_regnum + 3,
> +				      &addr);
> +	  read_memory (addr, readbuf, TYPE_LENGTH (valtype));
> +	}
> +
> +      return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> +    }
>     else
>       return ppc_sysv_abi_return_value (gdbarch, function, valtype, regcache,
>   				      readbuf, writebuf);
> diff --git a/gdb/testsuite/gdb.base/finish-struct.c b/gdb/testsuite/gdb.base/finish-struct.c
> new file mode 100644
> index 00000000000..2af02a1a1a3
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/finish-struct.c
> @@ -0,0 +1,40 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2020 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +struct S1 { char c1, c2, c3, c4; };
> +
> +struct S2 { short s1, s2; };
> +
> +struct S1
> +function_1 (void)
> +{
> +  struct S1 x = { 'A', 'B', 'C', 'D' };
> +  return x;
> +}
> +
> +struct S2
> +function_2 (void)
> +{
> +  struct S2 x = { 27, 28 };
> +  return x;
> +}
> +
> +int main ()
> +{
> +  function_1 ();
> +  function_2 ();
> +}
> diff --git a/gdb/testsuite/gdb.base/finish-struct.exp b/gdb/testsuite/gdb.base/finish-struct.exp
> new file mode 100644
> index 00000000000..b4e6875e5a7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/finish-struct.exp
> @@ -0,0 +1,47 @@
> +# Copyright 2020 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# This file is part of the gdb testsuite.  It is intended to test that
> +# gdb can correctly print arrays with indexes for each element of the
> +# array.
> +
> +standard_testfile .c
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    untested "failed to compile"
> +    return -1
> +}
> +
> +clean_restart ${binfile}
> +
> +gdb_breakpoint function_1
> +gdb_breakpoint function_2
> +
> +# Run the program until we reach the first breakpoint, which should be
> +# in function_1, and then test the "finish" command from there.
> +
> +gdb_run_cmd
> +gdb_test "" "Breakpoint \[0-9\]+,.*function_1.*" "run to function_1"
> +
> +gdb_test "finish" \
> +    " = {c1 = 65 'A', c2 = 66 'B', c3 = 67 'C', c4 = 68 'D'}" \
> +    "finish from function_1"
> +
> +# Continue the program's execution until reaching the breakpoint inside
> +# function_2, and then test the "finish" command from there.
> +
> +gdb_test "continue" "Breakpoint \[0-9\]+,.*function_2.*" "run to function_2"
> +
> +gdb_test "finish" " = {s1 = 27, s2 = 28}" "finish from function_2"

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

[Ulrich Weigand]

Hi Carl, hi Tom,

>Carl> The following patch fixes an issue on Powerpc where the function
return
>Carl> value for a C++ program prints garbage when the function return
type is
>Carl> a class object.  The fix is based on an x86-64 patch for the
same
>Carl> issue.
>
>Carl> The patch has been tested on Power 10 with no
regressions.  Please let
>Carl> me know if this patch is acceptable for mainline.   Thanks.
>
>FWIW, here at AdaCore, we carry a very similar patch for
>ppc-linux-tdep.c.  I've appended it.
>
>I haven't really submitted this yet since I wanted to investigate the
>ABI difference that lead to the change.

As to my understanding of the ELFv2 ABI, while r3 contains a pointer
to the struct return buffer *on function entry*, the ABI does *not*
guarantee that this will remain live throughout the whole function,
or be returned in r3.

Of course, in many simple examples the compiler will not bother
allocating anything else to r3, in particular if it stores
the return value shortly before returning.  But it's not guaranteed,
and therefore either of your patches is not really correct IMO.

(A possible always-correct fix would be to use the on-entry value
of r3 if we have the relevant on-entry DWARF info.)

Note that this is actually the same on ppc as on many (most?) other
platform ABIs.  Most test case differences are simply due to the
fact that the test case uses a struct that happens to be returned
in registers on x86_64, but in a buffer on ppc64le.


Bye,
Ulrich

[PATCH 0/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

GDB maintainers:

This patch set addresses the five test failures in gdb.cp/non-trivial-
retval.exp on PowerPC.  

The failures started with

   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 return value from the function is not correct due to multiple
problems.  

The PowerPC specific code does not return the correct return value
convention.  This is fixed by the first patach.  However due to the
PowerPC ABI, GDB is still not able to reliably determine the function
return value.

The second patch addresses the issue of not being able to reliably
determine the return buffer address on PowerPC.  The patch adds a new
GDB ABI to use the DW_OP_entryvalue for the DWARF entries to reliably
obtain the return buffer address stored in register r3 on entry to the
function.

These two patches fix the five testsuite failures in gdb.cp/non-
trivial-retval.exp on PowerPC.

                      Carl Love

[PATCH 0/2 version 2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

GDB maintainers:

Version 2:  The patch set has been updated per the review comments from
Kevin Buettner and Bruno Larsen.  Thank you to both for your help with
the pathes.

This patch set addresses the five test failures in gdb.cp/non-trivial-
retval.exp on PowerPC.  

The failures started with

   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 return value from the function is not correct due to multiple
problems.  

The PowerPC specific code does not return the correct return value
convention.  This is fixed by the first patach.  However due to the
PowerPC ABI, GDB is still not able to reliably determine the function
return value.

The second patch addresses the issue of not being able to reliably
determine the return buffer address on PowerPC.  The patch adds a new
GDB ABI to use the DW_OP_entryvalue for the DWARF entries to reliably
obtain the return buffer address stored in register r3 on entry to the
function.

These two patches fix the five testsuite failures in gdb.cp/non-
trivial-retval.exp on PowerPC.

                      Carl Love

[PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

GDB maintainers:

On PowerPC, a non-trivial return value for a function cannot be
reliably determined with the current PowerPC ABI.  The issue is the
address of the return buffer for a non-trivial value is passed to the
function in register r3.  However, at the end of the function the ABI
does not guarantee r3 will not be changed.  Hence it cannot be reliably
used to get the address of the return buffer.

This patch adds a new GDB ABI to allow GDB to obtain the input value of
r3 when the function returns.  This is done by using the DW_OP_entry
value for the DWARF entries to obtain the value of r3 on entry to the
function.  The ABI allows GDB to get the correct address for the return
buffer on exit from the function.

This patch fixes the five test failures in gdb.cp/non-trivial-
retval.exp.

The patch has been tested on PowerPC and X86-64 with no regression
failures.

Please let me know if this patch is acceptable for GDB mainline.
Thanks.

                         Carl Love

--------------------------------------
PowerPC, fix support for printing the function return value for non-trivial values.

Currently, a non-trivial return value from a function cannot currently be
reliably determined on PowerPC.  This is due to the fact that the PowerPC
ABI uses register r3 to store the address of the buffer containing the
non-trivial return value when the function is called.  The PowerPC ABI
does not guarantee the value in register r3 is not modified in the
function.  Thus the value in r3 cannot be reliably used to obtain the
return addreses on exit from the function.

This patch adds a new gdb ABI to allow PowerPC to access the value of r3
on entry to a function. The new gdb ABI attempts to use the DW_OP_entry
value for the DWARF entries, when exiting the function, to determine the
value of r3 on entry to the function.  This requires the use of the
-fvar-tracking compiler option to compile the user application thus
generating the DW_OP_entry_value in the binary.  The DW_OP_entries in the
binary file enables GDB can resolve the DW_TAG_call_sites.  This new API
is used to get the return buffer address, in the case of a function
returning a nontrivial data type, on exit from the function.  The GDB
function should_stop checks to see if RETURN_BUF is non-zero.  By default,
RETURN_BUF will be set to zero by the new ABI call for all architectures
but PowerPC.  The get_return_value function will be used to obtain the
return value on these architectures as is currently being done if
RETURN_BUF is zero.  On PowerPC, the new ABI will return a nonzero address
in RETURN_BUF if the value can be determined.  The value_at function uses
the return buffer address to get the return value.

This patch fixes five testcase failures in gdb.cp/non-trivial-retval.exp.
The correct function return values are now reported.

Note this patch is dependent on patch: "PowerPC, function
ppc64_sysv_abi_return_value add missing return value convention".

This patch has been tested on Power 10 and x86-64 with no regressions.
---
 gdb/arch-utils.c                            |  6 +++
 gdb/arch-utils.h                            |  4 ++
 gdb/dwarf2/loc.c                            |  2 +-
 gdb/dwarf2/loc.h                            | 11 ++++++
 gdb/gdbarch-components.py                   | 15 ++++++++
 gdb/gdbarch-gen.h                           | 11 ++++++
 gdb/gdbarch.c                               | 23 ++++++++++++
 gdb/infcmd.c                                | 41 +++++++++++++++++++--
 gdb/ppc-sysv-tdep.c                         | 37 +++++++++++++++++++
 gdb/ppc-tdep.h                              |  2 +
 gdb/rs6000-tdep.c                           |  6 ++-
 gdb/testsuite/gdb.cp/non-trivial-retval.exp |  9 ++++-
 gdb/testsuite/lib/gdb.exp                   |  8 ++++
 13 files changed, 168 insertions(+), 7 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 5943965bd75..12d6d1cdd75 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1093,6 +1093,12 @@ default_read_core_file_mappings
 {
 }
 
+CORE_ADDR
+default_get_return_buf_addr (struct type *val_type, frame_info *cur_frame)
+{
+  return 0;
+}
+
 /* Non-zero if we want to trace architecture code.  */
 
 #ifndef GDBARCH_DEBUG
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index f850e5fd6e7..a1944ff9c3c 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -300,4 +300,8 @@ extern void default_read_core_file_mappings
    struct bfd *cbfd,
    read_core_file_mappings_pre_loop_ftype pre_loop_cb,
    read_core_file_mappings_loop_ftype loop_cb);
+
+/* Default implementation of gdbarch default_get_return_buf_addr method.  */
+extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
+					      frame_info *cur_frame);
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index ad45d57a654..25a7499fe9b 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -1332,7 +1332,7 @@ static const struct lval_funcs entry_data_value_funcs =
    Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if it
    cannot resolve the parameter for any reason.  */
 
-static struct value *
+extern struct value *
 value_of_dwarf_reg_entry (struct type *type, struct frame_info *frame,
 			  enum call_site_parameter_kind kind,
 			  union call_site_parameter_u kind_u)
diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
index a9834d32066..f2cf93ba28e 100644
--- a/gdb/dwarf2/loc.h
+++ b/gdb/dwarf2/loc.h
@@ -296,4 +296,15 @@ extern struct value *indirect_synthetic_pointer
    dwarf2_per_objfile *per_objfile, struct frame_info *frame,
    struct type *type, bool resolve_abstract_p = false);
 
+/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND and KIND_U
+   are used to match DW_AT_location at the caller's
+   DW_TAG_call_site_parameter.
+
+   Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if
+   it cannot resolve the parameter for any reason.  */
+
+extern struct value *value_of_dwarf_reg_entry (struct type *type,
+					       struct frame_info *frame,
+					       enum call_site_parameter_kind kind,
+					       union call_site_parameter_u kind_u);
 #endif /* DWARF2LOC_H */
diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index ad71bf754de..6c623d668c5 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -845,6 +845,21 @@ for instance).
     invalid=True,
 )
 
+Function(
+    comment="""
+Return the address at which the value being returned from
+the current function will be stored.  This routine is only
+called if the current function uses the the "struct return
+convention".
+
+May return 0 when unable to determine that address.""",
+    type="CORE_ADDR",
+    name="get_return_buf_addr",
+    params=[("struct type *", "val_type"), ("frame_info *", "cur_frame")],
+    predefault="default_get_return_buf_addr",
+    invalid=False,
+)
+
 Method(
     comment="""
 Return true if the return value of function is stored in the first hidden
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index c7a24704c7c..3e363b3a069 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -435,6 +435,17 @@ typedef enum return_value_convention (gdbarch_return_value_ftype) (struct gdbarc
 extern enum return_value_convention gdbarch_return_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
 extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_value_ftype *return_value);
 
+/* Return the address at which the value being returned from
+   the current function will be stored.  This routine is only
+   called if the current function uses the the "struct return
+   convention".
+
+   May return 0 when unable to determine that address. */
+
+typedef CORE_ADDR (gdbarch_get_return_buf_addr_ftype) (struct type *val_type, frame_info *cur_frame);
+extern CORE_ADDR gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info *cur_frame);
+extern void set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, gdbarch_get_return_buf_addr_ftype *get_return_buf_addr);
+
 /* Return true if the return value of function is stored in the first hidden
    parameter.  In theory, this feature should be language-dependent, specified
    by language and its ABI, such as C++.  Unfortunately, compiler may
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 18d46a39d7a..da3e2ad0e36 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -116,6 +116,7 @@ struct gdbarch
   gdbarch_address_to_pointer_ftype *address_to_pointer = nullptr;
   gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
   gdbarch_return_value_ftype *return_value = nullptr;
+  gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = nullptr;
   gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = nullptr;
   gdbarch_skip_prologue_ftype *skip_prologue = nullptr;
   gdbarch_skip_main_prologue_ftype *skip_main_prologue = nullptr;
@@ -313,6 +314,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->value_from_register = default_value_from_register;
   gdbarch->pointer_to_address = unsigned_pointer_to_address;
   gdbarch->address_to_pointer = unsigned_address_to_pointer;
+  gdbarch->get_return_buf_addr = default_get_return_buf_addr;
   gdbarch->return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
   gdbarch->breakpoint_from_pc = default_breakpoint_from_pc;
   gdbarch->sw_breakpoint_from_kind = NULL;
@@ -465,6 +467,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of address_to_pointer, invalid_p == 0 */
   /* Skip verify of integer_to_address, has predicate.  */
   /* Skip verify of return_value, has predicate.  */
+  /* Skip verify of get_return_buf_addr, invalid_p == 0 */
   /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */
   if (gdbarch->skip_prologue == 0)
     log.puts ("\n\tskip_prologue");
@@ -877,6 +880,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
                       "gdbarch_dump: return_value = <%s>\n",
                       host_address_to_string (gdbarch->return_value));
+  gdb_printf (file,
+                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
+                      host_address_to_string (gdbarch->get_return_buf_addr));
   gdb_printf (file,
                       "gdbarch_dump: return_in_first_hidden_param_p = <%s>\n",
                       host_address_to_string (gdbarch->return_in_first_hidden_param_p));
@@ -2680,6 +2686,23 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
   gdbarch->return_value = return_value;
 }
 
+CORE_ADDR
+gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info *cur_frame)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->get_return_buf_addr != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_get_return_buf_addr called\n");
+  return gdbarch->get_return_buf_addr (val_type, cur_frame);
+}
+
+void
+set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch,
+                                 gdbarch_get_return_buf_addr_ftype get_return_buf_addr)
+{
+  gdbarch->get_return_buf_addr = get_return_buf_addr;
+}
+
 int
 gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type)
 {
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b788f454432..7964baa7f92 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -55,6 +55,7 @@
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
+#include "dwarf2/loc.h"
 
 /* Local functions: */
 
@@ -1598,6 +1599,12 @@ struct finish_command_fsm : public thread_fsm
      return value.  */
   struct return_value_info return_value_info {};
 
+  /* If the current function uses the "struct return convention",
+     this holds the address at which the value being returned will
+     be stored, or zero if that address could not be determined or
+     the "struct return convention" is not being used.  */
+  CORE_ADDR return_buf;
+
   explicit finish_command_fsm (struct interp *cmd_interp)
     : thread_fsm (cmd_interp)
   {
@@ -1636,7 +1643,13 @@ finish_command_fsm::should_stop (struct thread_info *tp)
 	  struct value *func;
 
 	  func = read_var_value (function, NULL, get_current_frame ());
-	  rv->value = get_return_value (function, func);
+
+	  if (return_buf != 0)
+	    /* Retrieve return value from the buffer where it was saved.  */
+	      rv->value = value_at (rv->type, return_buf);
+	  else
+	      rv->value = get_return_value (function, func);
+
 	  if (rv->value != NULL)
 	    rv->value_history_index = record_latest_value (rv->value);
 	}
@@ -1851,8 +1864,28 @@ finish_command (const char *arg, int from_tty)
     }
 
   /* Find the function we will return from.  */
-
-  sm->function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
+  frame_info *callee_frame = get_selected_frame (NULL);
+  sm->function = find_pc_function (get_frame_pc (callee_frame));
+
+  /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
+     attempt to determine the address of the return buffer.  */
+  enum return_value_convention return_value;
+  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
+
+  struct type * val_type
+    = check_typedef (sm->function->type()->target_type ());
+
+  return_value = gdbarch_return_value (gdbarch,
+				       read_var_value (sm->function, NULL,
+						       callee_frame),
+				       val_type, NULL, NULL, NULL);
+
+  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
+      && val_type->code() != TYPE_CODE_VOID)
+    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
+						  callee_frame);
+  else
+    sm->return_buf = 0;
 
   /* Print info on the selected frame, including level number but not
      source.  */
@@ -1870,7 +1903,7 @@ finish_command (const char *arg, int from_tty)
 	  gdb_printf (_("Run till exit from "));
 	}
 
-      print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
+      print_stack_frame (callee_frame, 1, LOCATION, 0);
     }
 
   if (execution_direction == EXEC_REVERSE)
diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index 14effb93210..59145f9f166 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -28,6 +28,7 @@
 #include "objfiles.h"
 #include "infcall.h"
 #include "dwarf2.h"
+#include "dwarf2/loc.h"
 #include "target-float.h"
 #include <algorithm>
 
@@ -2156,3 +2157,39 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
   return RETURN_VALUE_STRUCT_CONVENTION;
 }
 
+CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
+					  frame_info *cur_frame)
+{
+  /* On PowerPC, the ABI specifies r3 is the address where an argument
+     and the return value is stored.  The PowerPC ABI does not guarantee
+     r3 will be preserved when the function returns.  The function returns
+     the address of the buffer stored in r3.
+
+     Attempt to determine the value of r3 on entry to the function using the
+     DW_OP_entry_value DWARF entries.  This requires compiling the user
+     program with -fvar-tracking to resolve the DW_TAG_call_sites in the
+     binary file.  */
+  union call_site_parameter_u kind_u;
+  enum call_site_parameter_kind kind;
+  CORE_ADDR return_val = 0;
+
+  kind_u.dwarf_reg = 3;  /* First passed arg/return value is in r3.  */
+  kind = CALL_SITE_PARAMETER_DWARF_REG;
+
+  /* val_type is the type of the return value.  Need the pointer type
+     to the return value.  */
+  val_type = lookup_pointer_type (val_type);
+
+  try
+    {
+      return_val = value_as_address (value_of_dwarf_reg_entry (val_type,
+							       cur_frame,
+							       kind, kind_u));
+    }
+  catch (const gdb_exception_error &e)
+    {
+      warning ("Cannot determine the function return value.\n"
+	       "Try compiling with -fvar-tracking.");
+    }
+  return return_val;
+}
diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
index 44f63b145c6..299c87a169d 100644
--- a/gdb/ppc-tdep.h
+++ b/gdb/ppc-tdep.h
@@ -175,6 +175,8 @@ extern void ppc_collect_vsxregset (const struct regset *regset,
 				  const struct regcache *regcache,
 				  int regnum, void *vsxregs, size_t len);
 
+extern CORE_ADDR ppc64_sysv_get_return_buf_addr (type*, frame_info*);
+
 /* Private data that this module attaches to struct gdbarch.  */
 
 /* ELF ABI version used by the inferior.  */
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 87b2faad36a..0a4f4458845 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -8235,7 +8235,11 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_ps_regnum (gdbarch, tdep->ppc_ps_regnum);
 
   if (wordsize == 8)
-    set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
+    {
+      set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
+      set_gdbarch_get_return_buf_addr (gdbarch,
+				       ppc64_sysv_get_return_buf_addr);
+    }
   else
     set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
 
diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
index 21c390bc937..93a3a6832f7 100644
--- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
+++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
@@ -15,11 +15,18 @@
 
 # This file is part of the gdb testsuite
 
+set additional_flags ""
+
 if {[skip_cplus_tests]} { continue }
 
 standard_testfile .cc
 
-if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
+if {[have_fvar_tracking]} {
+    set additional_flags "additional_flags= -fvar-tracking"
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile [list debug c++ $additional_flags]]} {
+
     return -1
 }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 5ab4df1bcf3..a9ebc11dfea 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -8468,6 +8468,14 @@ gdb_caching_proc have_fuse_ld_gold {
     return [gdb_simple_compile $me $src executable $flags]
 }
 
+# Return 1 if compiler supports fvar-tracking, otherwise return 0.
+gdb_caching_proc have_fvar_tracking {
+    set me "have_fvar_tracking"
+    set flags "additional_flags=-fvar-tracking"
+    set src { int main() { return 0; } }
+    return [gdb_simple_compile $me $src executable $flags]
+}
+
 # Return 1 if linker supports -Ttext-segment, otherwise return 0.
 gdb_caching_proc linker_supports_Ttext_segment_flag {
     set me "linker_supports_Ttext_segment_flag"
-- 
2.31.1

Re: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Kevin Buettner]

Hi Carl,

I'm still working through the patch, but I noticed what I believe
to be incorrect terminology while reading the intro / commit log...

On Thu, 06 Oct 2022 09:37:40 -0700
Carl Love via Gdb-patches <gdb-patches@sourceware.org> wrote:

...
> This patch adds a new gdb ABI to allow PowerPC to access the value of r3
> on entry to a function. The new gdb ABI attempts to use the DW_OP_entry
...

Rather than saying "gdb ABI", I think you meant to say "gdbarch method".

Also, I'd say something like "On PowerPC, the new gdbarch method attempts
to use DW_OP_entry...".  (I.e., this new method doesn't attempt to access r3
on all architectures.)

Kevin

RE: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

Kevin:

On Fri, 2022-10-07 at 21:36 -0700, Kevin Buettner wrote:
> Hi Carl,
> 
> I'm still working through the patch, but I noticed what I believe
> to be incorrect terminology while reading the intro / commit log...
> 
> On Thu, 06 Oct 2022 09:37:40 -0700
> Carl Love via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> ...
> > This patch adds a new gdb ABI to allow PowerPC to access the value
> > of r3
> > on entry to a function. The new gdb ABI attempts to use the
> > DW_OP_entry
> ...
> 
> Rather than saying "gdb ABI", I think you meant to say "gdbarch
> method".
> 
> Also, I'd say something like "On PowerPC, the new gdbarch method
> attempts
> to use DW_OP_entry...".  (I.e., this new method doesn't attempt to
> access r3
> on all architectures.)

Thanks for the feedback on the terminology.  

I was wondering if you had finished working thru the patch?  Do you
have any additional thoughts on the patch, particularly on the
functional part of the patch before I post an updated version of the
patch.   

Thanks for looking at the patch.

                       Carl Love

Re: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Kevin Buettner]

Hi Carl,

While running "git am" to apply your patch, I ran into the following
problems...

1) The patch didn't apply cleanly due to some changes in gdb/dwarf2/loc.c.
It seemed to me that it was likely due to this commit:

bd2b40ac129 Change GDB to use frame_info_ptr

After checking out bd2b40ac129~, I was able to apply your patch
via git am, though I did see some whitespace complaints...

2) Whitespace complaints:

Applying: PowerPC, fix support for printing the function return value for non-trivial values.
...patch:161: indent with spaces.
                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
...patch:162: indent with spaces.
                      host_address_to_string (gdbarch->get_return_buf_addr));
...patch:182: indent with spaces.
                                 gdbarch_get_return_buf_addr_ftype get_return_buf_addr)
warning: 3 lines add whitespace errors.

(I substituted some long paths local to my machines w/ '...'.)

See my remaining remarks inline below...

Kevin

On Thu, 06 Oct 2022 09:37:40 -0700
Carl Love via Gdb-patches <gdb-patches@sourceware.org> wrote:

> PowerPC, fix support for printing the function return value for non-trivial values.
> 
> Currently, a non-trivial return value from a function cannot currently be
> reliably determined on PowerPC.  This is due to the fact that the PowerPC
> ABI uses register r3 to store the address of the buffer containing the
> non-trivial return value when the function is called.  The PowerPC ABI
> does not guarantee the value in register r3 is not modified in the
> function.  Thus the value in r3 cannot be reliably used to obtain the
> return addreses on exit from the function.
> 
> This patch adds a new gdb ABI to allow PowerPC to access the value of r3

I already mentioned this last week, but...

s/GDB ABI/gdbarch method/

I guess you could also refer to it as an API which you did later on
in your commit remarks.  (What I really object to is calling it an
"ABI", which it definitely is not - that said it's easy to mis-type API
as ABI, so I do understand this mistake.)

> on entry to a function. The new gdb ABI attempts to use the DW_OP_entry

s/The new/On PowerPC, the new/
s/GDB ABI/gdbarch method/

> value for the DWARF entries, when exiting the function, to determine the

Missing '_' between "DW_OP_entry" and "value".  I.e. it's "DW_OP_entry_value",
not "DW_OP_entry value".

> value of r3 on entry to the function.  This requires the use of the
> -fvar-tracking compiler option to compile the user application thus
> generating the DW_OP_entry_value in the binary.  The DW_OP_entries in the

I don't think that "DW_OP_entries" is correct here.  Maybe "DW_OP_entry_value
entries"?

> binary file enables GDB can resolve the DW_TAG_call_sites.  This new API

Maybe instead of "enables GDB can resolve the DW_TAG_call_sites", say
"allow GDB to resolve the DW_TAG_call_site entries".

> is used to get the return buffer address, in the case of a function
> returning a nontrivial data type, on exit from the function.  The GDB
> function should_stop checks to see if RETURN_BUF is non-zero.  By default,
> RETURN_BUF will be set to zero by the new ABI call for all architectures

s/ABI/API/

> but PowerPC.  The get_return_value function will be used to obtain the
> return value on these architectures as is currently being done if
> RETURN_BUF is zero.  On PowerPC, the new ABI will return a nonzero address
> in RETURN_BUF if the value can be determined.  The value_at function uses
> the return buffer address to get the return value.
> 
> This patch fixes five testcase failures in gdb.cp/non-trivial-retval.exp.
> The correct function return values are now reported.
> 
> Note this patch is dependent on patch: "PowerPC, function
> ppc64_sysv_abi_return_value add missing return value convention".
> 
> This patch has been tested on Power 10 and x86-64 with no regressions.
> ---
>  gdb/arch-utils.c                            |  6 +++
>  gdb/arch-utils.h                            |  4 ++
>  gdb/dwarf2/loc.c                            |  2 +-
>  gdb/dwarf2/loc.h                            | 11 ++++++
>  gdb/gdbarch-components.py                   | 15 ++++++++
>  gdb/gdbarch-gen.h                           | 11 ++++++
>  gdb/gdbarch.c                               | 23 ++++++++++++
>  gdb/infcmd.c                                | 41 +++++++++++++++++++--
>  gdb/ppc-sysv-tdep.c                         | 37 +++++++++++++++++++
>  gdb/ppc-tdep.h                              |  2 +
>  gdb/rs6000-tdep.c                           |  6 ++-
>  gdb/testsuite/gdb.cp/non-trivial-retval.exp |  9 ++++-
>  gdb/testsuite/lib/gdb.exp                   |  8 ++++
>  13 files changed, 168 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 5943965bd75..12d6d1cdd75 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1093,6 +1093,12 @@ default_read_core_file_mappings
>  {
>  }
>  
> +CORE_ADDR
> +default_get_return_buf_addr (struct type *val_type, frame_info *cur_frame)
> +{
> +  return 0;
> +}
> +
>  /* Non-zero if we want to trace architecture code.  */
>  
>  #ifndef GDBARCH_DEBUG
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index f850e5fd6e7..a1944ff9c3c 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -300,4 +300,8 @@ extern void default_read_core_file_mappings
>     struct bfd *cbfd,
>     read_core_file_mappings_pre_loop_ftype pre_loop_cb,
>     read_core_file_mappings_loop_ftype loop_cb);
> +
> +/* Default implementation of gdbarch default_get_return_buf_addr method.  */
> +extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
> +					      frame_info *cur_frame);
>  #endif /* ARCH_UTILS_H */
> diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> index ad45d57a654..25a7499fe9b 100644
> --- a/gdb/dwarf2/loc.c
> +++ b/gdb/dwarf2/loc.c
> @@ -1332,7 +1332,7 @@ static const struct lval_funcs entry_data_value_funcs =
>     Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if it
>     cannot resolve the parameter for any reason.  */

It appears that the doc above was moved to dwarf2/loc.h.  I think it might be
better to remove the above comment and replace it with "/* See dwarf2/loc.h.  */".

>  
> -static struct value *
> +extern struct value *

Remove 'extern ' here.

>  value_of_dwarf_reg_entry (struct type *type, struct frame_info *frame,
>  			  enum call_site_parameter_kind kind,
>  			  union call_site_parameter_u kind_u)
> diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
> index a9834d32066..f2cf93ba28e 100644
> --- a/gdb/dwarf2/loc.h
> +++ b/gdb/dwarf2/loc.h
> @@ -296,4 +296,15 @@ extern struct value *indirect_synthetic_pointer
>     dwarf2_per_objfile *per_objfile, struct frame_info *frame,
>     struct type *type, bool resolve_abstract_p = false);
>  
> +/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND and KIND_U
> +   are used to match DW_AT_location at the caller's
> +   DW_TAG_call_site_parameter.
> +
> +   Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if
> +   it cannot resolve the parameter for any reason.  */
> +
> +extern struct value *value_of_dwarf_reg_entry (struct type *type,
> +					       struct frame_info *frame,
> +					       enum call_site_parameter_kind kind,
> +					       union call_site_parameter_u kind_u);
>  #endif /* DWARF2LOC_H */
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index ad71bf754de..6c623d668c5 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -845,6 +845,21 @@ for instance).
>      invalid=True,
>  )
>  
> +Function(
> +    comment="""
> +Return the address at which the value being returned from
> +the current function will be stored.  This routine is only
> +called if the current function uses the the "struct return
> +convention".
> +
> +May return 0 when unable to determine that address.""",
> +    type="CORE_ADDR",
> +    name="get_return_buf_addr",
> +    params=[("struct type *", "val_type"), ("frame_info *", "cur_frame")],
> +    predefault="default_get_return_buf_addr",
> +    invalid=False,
> +)
> +
>  Method(
>      comment="""
>  Return true if the return value of function is stored in the first hidden
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index c7a24704c7c..3e363b3a069 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -435,6 +435,17 @@ typedef enum return_value_convention (gdbarch_return_value_ftype) (struct gdbarc
>  extern enum return_value_convention gdbarch_return_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
>  extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_value_ftype *return_value);
>  
> +/* Return the address at which the value being returned from
> +   the current function will be stored.  This routine is only
> +   called if the current function uses the the "struct return
> +   convention".
> +
> +   May return 0 when unable to determine that address. */
> +
> +typedef CORE_ADDR (gdbarch_get_return_buf_addr_ftype) (struct type *val_type, frame_info *cur_frame);
> +extern CORE_ADDR gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info *cur_frame);
> +extern void set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, gdbarch_get_return_buf_addr_ftype *get_return_buf_addr);
> +
>  /* Return true if the return value of function is stored in the first hidden
>     parameter.  In theory, this feature should be language-dependent, specified
>     by language and its ABI, such as C++.  Unfortunately, compiler may
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 18d46a39d7a..da3e2ad0e36 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -116,6 +116,7 @@ struct gdbarch
>    gdbarch_address_to_pointer_ftype *address_to_pointer = nullptr;
>    gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
>    gdbarch_return_value_ftype *return_value = nullptr;
> +  gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = nullptr;
>    gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = nullptr;
>    gdbarch_skip_prologue_ftype *skip_prologue = nullptr;
>    gdbarch_skip_main_prologue_ftype *skip_main_prologue = nullptr;
> @@ -313,6 +314,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
>    gdbarch->value_from_register = default_value_from_register;
>    gdbarch->pointer_to_address = unsigned_pointer_to_address;
>    gdbarch->address_to_pointer = unsigned_address_to_pointer;
> +  gdbarch->get_return_buf_addr = default_get_return_buf_addr;
>    gdbarch->return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
>    gdbarch->breakpoint_from_pc = default_breakpoint_from_pc;
>    gdbarch->sw_breakpoint_from_kind = NULL;
> @@ -465,6 +467,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>    /* Skip verify of address_to_pointer, invalid_p == 0 */
>    /* Skip verify of integer_to_address, has predicate.  */
>    /* Skip verify of return_value, has predicate.  */
> +  /* Skip verify of get_return_buf_addr, invalid_p == 0 */
>    /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */
>    if (gdbarch->skip_prologue == 0)
>      log.puts ("\n\tskip_prologue");
> @@ -877,6 +880,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>    gdb_printf (file,
>                        "gdbarch_dump: return_value = <%s>\n",
>                        host_address_to_string (gdbarch->return_value));
> +  gdb_printf (file,
> +                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
> +                      host_address_to_string (gdbarch->get_return_buf_addr));
>    gdb_printf (file,
>                        "gdbarch_dump: return_in_first_hidden_param_p = <%s>\n",
>                        host_address_to_string (gdbarch->return_in_first_hidden_param_p));
> @@ -2680,6 +2686,23 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
>    gdbarch->return_value = return_value;
>  }
>  
> +CORE_ADDR
> +gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info *cur_frame)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->get_return_buf_addr != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_get_return_buf_addr called\n");
> +  return gdbarch->get_return_buf_addr (val_type, cur_frame);
> +}
> +
> +void
> +set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch,
> +                                 gdbarch_get_return_buf_addr_ftype get_return_buf_addr)
> +{
> +  gdbarch->get_return_buf_addr = get_return_buf_addr;
> +}
> +
>  int
>  gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type)
>  {
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index b788f454432..7964baa7f92 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -55,6 +55,7 @@
>  #include "gdbsupport/gdb_optional.h"
>  #include "source.h"
>  #include "cli/cli-style.h"
> +#include "dwarf2/loc.h"
>  
>  /* Local functions: */
>  
> @@ -1598,6 +1599,12 @@ struct finish_command_fsm : public thread_fsm
>       return value.  */
>    struct return_value_info return_value_info {};
>  
> +  /* If the current function uses the "struct return convention",
> +     this holds the address at which the value being returned will
> +     be stored, or zero if that address could not be determined or
> +     the "struct return convention" is not being used.  */
> +  CORE_ADDR return_buf;
> +
>    explicit finish_command_fsm (struct interp *cmd_interp)
>      : thread_fsm (cmd_interp)
>    {
> @@ -1636,7 +1643,13 @@ finish_command_fsm::should_stop (struct thread_info *tp)
>  	  struct value *func;
>  
>  	  func = read_var_value (function, NULL, get_current_frame ());
> -	  rv->value = get_return_value (function, func);
> +
> +	  if (return_buf != 0)
> +	    /* Retrieve return value from the buffer where it was saved.  */
> +	      rv->value = value_at (rv->type, return_buf);
> +	  else
> +	      rv->value = get_return_value (function, func);
> +
>  	  if (rv->value != NULL)
>  	    rv->value_history_index = record_latest_value (rv->value);
>  	}
> @@ -1851,8 +1864,28 @@ finish_command (const char *arg, int from_tty)
>      }
>  
>    /* Find the function we will return from.  */
> -
> -  sm->function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
> +  frame_info *callee_frame = get_selected_frame (NULL);
> +  sm->function = find_pc_function (get_frame_pc (callee_frame));
> +
> +  /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
> +     attempt to determine the address of the return buffer.  */
> +  enum return_value_convention return_value;
> +  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
> +
> +  struct type * val_type
> +    = check_typedef (sm->function->type()->target_type ());
> +
> +  return_value = gdbarch_return_value (gdbarch,
> +				       read_var_value (sm->function, NULL,
> +						       callee_frame),
> +				       val_type, NULL, NULL, NULL);
> +
> +  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
> +      && val_type->code() != TYPE_CODE_VOID)
> +    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
> +						  callee_frame);
> +  else
> +    sm->return_buf = 0;
>  
>    /* Print info on the selected frame, including level number but not
>       source.  */
> @@ -1870,7 +1903,7 @@ finish_command (const char *arg, int from_tty)
>  	  gdb_printf (_("Run till exit from "));
>  	}
>  
> -      print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
> +      print_stack_frame (callee_frame, 1, LOCATION, 0);
>      }
>  
>    if (execution_direction == EXEC_REVERSE)
> diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> index 14effb93210..59145f9f166 100644
> --- a/gdb/ppc-sysv-tdep.c
> +++ b/gdb/ppc-sysv-tdep.c
> @@ -28,6 +28,7 @@
>  #include "objfiles.h"
>  #include "infcall.h"
>  #include "dwarf2.h"
> +#include "dwarf2/loc.h"
>  #include "target-float.h"
>  #include <algorithm>
>  
> @@ -2156,3 +2157,39 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
>    return RETURN_VALUE_STRUCT_CONVENTION;
>  }
>  
> +CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
> +					  frame_info *cur_frame)
> +{
> +  /* On PowerPC, the ABI specifies r3 is the address where an argument

s/specifies/specifies that/

I'm not sure about this phrase:  "where an argument and".  If you mean
to say that r3 is sometimes used for arguments (when it's not used for
the address of the return value buffer), then that's true, but it's
not relevant here.

So, I guess that I'm asking that this comment be reworded somewhat.

> +     and the return value is stored.  The PowerPC ABI does not guarantee
> +     r3 will be preserved when the function returns.  The function returns
> +     the address of the buffer stored in r3.

I find this part confusing.  If the function returns the address of
the buffer, doesn't it do so by returning it in r3?  (And, if so, it
seems to me that this code would be unnecessary - so, clearly, I must
be missing something...)

> +
> +     Attempt to determine the value of r3 on entry to the function using the
> +     DW_OP_entry_value DWARF entries.  This requires compiling the user
> +     program with -fvar-tracking to resolve the DW_TAG_call_sites in the
> +     binary file.  */
> +  union call_site_parameter_u kind_u;
> +  enum call_site_parameter_kind kind;
> +  CORE_ADDR return_val = 0;
> +
> +  kind_u.dwarf_reg = 3;  /* First passed arg/return value is in r3.  */
> +  kind = CALL_SITE_PARAMETER_DWARF_REG;
> +
> +  /* val_type is the type of the return value.  Need the pointer type
> +     to the return value.  */
> +  val_type = lookup_pointer_type (val_type);
> +
> +  try
> +    {
> +      return_val = value_as_address (value_of_dwarf_reg_entry (val_type,
> +							       cur_frame,
> +							       kind, kind_u));
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      warning ("Cannot determine the function return value.\n"
> +	       "Try compiling with -fvar-tracking.");

Will this warning be printed a lot?  (I'm wondering if there should be a
way to shut it off...)

> +    }
> +  return return_val;
> +}
> diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
> index 44f63b145c6..299c87a169d 100644
> --- a/gdb/ppc-tdep.h
> +++ b/gdb/ppc-tdep.h
> @@ -175,6 +175,8 @@ extern void ppc_collect_vsxregset (const struct regset *regset,
>  				  const struct regcache *regcache,
>  				  int regnum, void *vsxregs, size_t len);
>  
> +extern CORE_ADDR ppc64_sysv_get_return_buf_addr (type*, frame_info*);
> +
>  /* Private data that this module attaches to struct gdbarch.  */
>  
>  /* ELF ABI version used by the inferior.  */
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 87b2faad36a..0a4f4458845 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -8235,7 +8235,11 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    set_gdbarch_ps_regnum (gdbarch, tdep->ppc_ps_regnum);
>  
>    if (wordsize == 8)
> -    set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
> +    {
> +      set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
> +      set_gdbarch_get_return_buf_addr (gdbarch,
> +				       ppc64_sysv_get_return_buf_addr);
> +    }
>    else
>      set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
>  
> diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> index 21c390bc937..93a3a6832f7 100644
> --- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> +++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> @@ -15,11 +15,18 @@
>  
>  # This file is part of the gdb testsuite
>  
> +set additional_flags ""
> +
>  if {[skip_cplus_tests]} { continue }
>  
>  standard_testfile .cc
>  
> -if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
> +if {[have_fvar_tracking]} {
> +    set additional_flags "additional_flags= -fvar-tracking"
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile [list debug c++ $additional_flags]]} {
> +
>      return -1
>  }
>  
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index 5ab4df1bcf3..a9ebc11dfea 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -8468,6 +8468,14 @@ gdb_caching_proc have_fuse_ld_gold {
>      return [gdb_simple_compile $me $src executable $flags]
>  }
>  
> +# Return 1 if compiler supports fvar-tracking, otherwise return 0.
> +gdb_caching_proc have_fvar_tracking {
> +    set me "have_fvar_tracking"
> +    set flags "additional_flags=-fvar-tracking"
> +    set src { int main() { return 0; } }
> +    return [gdb_simple_compile $me $src executable $flags]
> +}
> +
>  # Return 1 if linker supports -Ttext-segment, otherwise return 0.
>  gdb_caching_proc linker_supports_Ttext_segment_flag {
>      set me "linker_supports_Ttext_segment_flag"
> -- 
> 2.31.1
> 
> 

Re: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Bruno Larsen]

On 14/10/2022 04:49, Kevin Buettner via Gdb-patches wrote:
> Hi Carl,
>
> While running "git am" to apply your patch, I ran into the following
> problems...
>
> 1) The patch didn't apply cleanly due to some changes in gdb/dwarf2/loc.c.
> It seemed to me that it was likely due to this commit:
>
> bd2b40ac129 Change GDB to use frame_info_ptr
To expand on this, wherever you used frame_info *, you should switch to 
frame_info_ptr, unless you are dealing with guile. As far as I could see 
in your patch, that simple change should be enough.

Cheers,
Bruno

RE: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

On Fri, 2022-10-14 at 09:36 +0200, Bruno Larsen via Gdb-patches wrote:
> On 14/10/2022 04:49, Kevin Buettner via Gdb-patches wrote:
> > Hi Carl,
> > 
> > While running "git am" to apply your patch, I ran into the
> > following
> > problems...
> > 
> > 1) The patch didn't apply cleanly due to some changes in
> > gdb/dwarf2/loc.c.
> > It seemed to me that it was likely due to this commit:
> > 
> > bd2b40ac129 Change GDB to use frame_info_ptr
> To expand on this, wherever you used frame_info *, you should switch
> to 
> frame_info_ptr, unless you are dealing with guile. As far as I could
> see 
> in your patch, that simple change should be enough.

Yup, made the change you mentioned.  Sorry, I forgot to explicitly add
you to my reply to Kevin.  

                    Carl 

RE: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

On Thu, 2022-10-13 at 19:49 -0700, Kevin Buettner wrote:
> Hi Carl,
> 
> While running "git am" to apply your patch, I ran into the following
> problems...
> 
> 1) The patch didn't apply cleanly due to some changes in
> gdb/dwarf2/loc.c.
> It seemed to me that it was likely due to this commit:
> 
> bd2b40ac129 Change GDB to use frame_info_ptr
> 
> After checking out bd2b40ac129~, I was able to apply your patch
> via git am, though I did see some whitespace complaints...

Yes, mainline has moved a bit.  I refreshed the patch.  I made the
change mentioned by Bruno c/frame_info */frame_info_ptr/.  I was then
able to get gdb to compile.

> 
> 2) Whitespace complaints:
> 
> Applying: PowerPC, fix support for printing the function return value
> for non-trivial values.
> ...patch:161: indent with spaces.
>                       "gdbarch_dump: get_return_buf_addr = <%s>\n",
> ...patch:162: indent with spaces.
>                       host_address_to_string (gdbarch-
> >get_return_buf_addr));
> ...patch:182: indent with spaces.
>                                  gdbarch_get_return_buf_addr_ftype
> get_return_buf_addr)
> warning: 3 lines add whitespace errors.
> 
> (I substituted some long paths local to my machines w/ '...'.)

Yes, I have seen the complaints about the white spaces.  The whitespace
complaints come from file gdb/gdbarch.c which is a generated file.  As
I assume you know, the file was regenerated by the gdb/gdbarch.py based
on the changes to gdb/gdbarch-components.py.  The regenerated file is
set to read only.  Not sure what I can do to rectify the white spaces? 
Any ideas?

> <snip>

> I already mentioned this last week, but...
> 
> s/GDB ABI/gdbarch method/
> 
> I guess you could also refer to it as an API which you did later on
> in your commit remarks.  (What I really object to is calling it an
> "ABI", which it definitely is not - that said it's easy to mis-type
> API
> as ABI, so I do understand this mistake.)

Yea, I think referring to it as a new gdbarch method is probably better
than GDB API/ABI.  I have changed the GDB API/ABI references to gdbarch
method.  I will need to update that in the message to the GDB
maintainers as well when I send the next version of the patch.

> 
> > on entry to a function. The new gdb ABI attempts to use the
> > DW_OP_entry
> 
> s/The new/On PowerPC, the new/
> s/GDB ABI/gdbarch method/
> 
> > value for the DWARF entries, when exiting the function, to
> > determine the
> 
> Missing '_' between "DW_OP_entry" and "value".  I.e. it's
> "DW_OP_entry_value",
> not "DW_OP_entry value".

Fixed.
> 
> > value of r3 on entry to the function.  This requires the use of the
> > -fvar-tracking compiler option to compile the user application thus
> > generating the DW_OP_entry_value in the binary.  The DW_OP_entries
> > in the
> 
> I don't think that "DW_OP_entries" is correct here.  Maybe
> "DW_OP_entry_value
> entries"?

> > binary file enables GDB can resolve the DW_TAG_call_sites.  This
> > new API
> 
> Maybe instead of "enables GDB can resolve the DW_TAG_call_sites", say
> "allow GDB to resolve the DW_TAG_call_site entries".
> 
> 
> > is used to get the return buffer address, in the case of a function
> > returning a nontrivial data type, on exit from the function.  The
> > GDB
> > function should_stop checks to see if RETURN_BUF is non-zero.  By
> > default,
> > RETURN_BUF will be set to zero by the new ABI call for all
> > architectures
> 
> s/ABI/API/
> 
> > but PowerPC.  The get_return_value function will be used to obtain
> > the
> > return value on these architectures as is currently being done if
> > RETURN_BUF is zero.  On PowerPC, the new ABI will return a nonzero
> > address
> > in RETURN_BUF if the value can be determined.  The value_at
> > function uses
> > the return buffer address to get the return value.

So, I tried to address all of the above comments about ABI,
"DW_OP_entries" etc.  The paragraph now reads:

This patch adds a new gdbarch method to allow PowerPC to access the value
of r3 on entry to a function. On PowerPC, the new gdb arch method attempts
to use the DW_OP_entry_value for the DWARF entries, when exiting the
function, to determine the value of r3 on entry to the function.  This
requires the use of the -fvar-tracking compiler option to compile the
user application thus generating the DW_OP_entry_value in the binary.  The
DW_OP_entry_value entries in the binary file allows GDB to resolve the
DW_TAG_call_site entries.  This new gdbarch method is used to get the
return buffer address, in the case of a function returning a nontrivial
data type, on exit from the function.  The GDB function should_stop checks
to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set to
zero by the new gdbarch method call for all architectures except PowerPC.
The get_return_value function will be used to obtain the return value on
all other architectures as is currently being done if RETURN_BUF is zero.
On PowerPC, the new gdbarch method will return a nonzero address in
RETURN_BUF if the value can be determined.  The value_at function uses the
return buffer address to get the return value.

Which I think covers all of the comments above. 

> > 
> > This patch fixes five testcase failures in gdb.cp/non-trivial-
> > retval.exp.
> > 

< snip >

> > 			      frame_info *cur_frame);
> >  #endif /* ARCH_UTILS_H */
> > diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> > index ad45d57a654..25a7499fe9b 100644
> > --- a/gdb/dwarf2/loc.c
> > +++ b/gdb/dwarf2/loc.c
> > @@ -1332,7 +1332,7 @@ static const struct lval_funcs
> > entry_data_value_funcs =
> >     Function always returns non-NULL value.  It throws
> > NO_ENTRY_VALUE_ERROR if it
> >     cannot resolve the parameter for any reason.  */
> 
> It appears that the doc above was moved to dwarf2/loc.h.  I think it
> might be
> better to remove the above comment and replace it with "/* See
> dwarf2/loc.h.  */".
> 
Fixed.

> >  
> > -static struct value *
> > +extern struct value *
> 
> Remove 'extern ' here.

Fixed.

> 

<snip>

> >    if (execution_direction == EXEC_REVERSE)
> > diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> > index 14effb93210..59145f9f166 100644
> > --- a/gdb/ppc-sysv-tdep.c
> > +++ b/gdb/ppc-sysv-tdep.c
> > @@ -28,6 +28,7 @@
> >  #include "objfiles.h"
> >  #include "infcall.h"
> >  #include "dwarf2.h"
> > +#include "dwarf2/loc.h"
> >  #include "target-float.h"
> >  #include <algorithm>
> >  
> > @@ -2156,3 +2157,39 @@ ppc64_sysv_abi_return_value (struct gdbarch
> > *gdbarch, struct value *function,
> >    return RETURN_VALUE_STRUCT_CONVENTION;
> >  }
> >  
> > +CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
> > +					  frame_info *cur_frame)
> > +{
> > +  /* On PowerPC, the ABI specifies r3 is the address where an
> > argument
> 
> s/specifies/specifies that/
> 
> I'm not sure about this phrase:  "where an argument and".  If you
> mean
> to say that r3 is sometimes used for arguments (when it's not used
> for
> the address of the return value buffer), then that's true, but it's
> not relevant here.
> 
> So, I guess that I'm asking that this comment be reworded somewhat.
> 
> > +     and the return value is stored.  The PowerPC ABI does not
> > guarantee
> > +     r3 will be preserved when the function returns.  The function
> > returns
> > +     the address of the buffer stored in r3.
> 
> I find this part confusing.  If the function returns the address of
> the buffer, doesn't it do so by returning it in r3?  (And, if so, it
> seems to me that this code would be unnecessary - so, clearly, I must
> be missing something...)

> > +
> > +     Attempt to determine the value of r3 on entry to the function
> > using the
> > +     DW_OP_entry_value DWARF entries.  This requires compiling the
> > user
> > +     program with -fvar-tracking to resolve the DW_TAG_call_sites
> > in the
> > +     binary file.  */

OK, so some rework here to make things a bit better.  I changed the
header comment for function ppc64_sysv_get_return_buf_addr to the
following.  Hopefully this addresses the various questions and
comments.

  /* The PowerPC ABI specifies aggregates that are not returned by value
     are returned in a storage buffer provided by the caller.  The
     address of the storage buffer is provided as a hidden first input
     arguement in register r3.  The PowerPC ABI does not guarantee that
     register r3 will not be changed while executing the function.  Hence, it
     cannot be assumed that r3 will still contain the address of the storage
     buffer when execution reaches the end of the function.

     This function attempts to determine the value of r3 on entry to the
     function using the DW_OP_entry_value DWARF entries.  This requires
     compiling the user program with -fvar-tracking to resolve the
     DW_TAG_call_sites in the binary file.  */

Hopefully that is clearer and clears up any confusion as to the role of r3.

> > +  union call_site_parameter_u kind_u;
> > +  enum call_site_parameter_kind kind;
> > +  CORE_ADDR return_val = 0;
> > +
> > +  kind_u.dwarf_reg = 3;  /* First passed arg/return value is in
> > r3.  */
> > +  kind = CALL_SITE_PARAMETER_DWARF_REG;
> > +
> > +  /* val_type is the type of the return value.  Need the pointer
> > type
> > +     to the return value.  */
> > +  val_type = lookup_pointer_type (val_type);
> > +
> > +  try
> > +    {
> > +      return_val = value_as_address (value_of_dwarf_reg_entry
> > (val_type,
> > +							       cur_fram
> > e,
> > +							       kind,
> > kind_u));
> > +    }
> > +  catch (const gdb_exception_error &e)
> > +    {
> > +      warning ("Cannot determine the function return value.\n"
> > +	       "Try compiling with -fvar-tracking.");
> 
> Will this warning be printed a lot?  (I'm wondering if there should
> be a
> way to shut it off...)

Hmm, hard to say how often it will get printed.  It is going to be up
to what the user is trying to do.  If they are stopping on the return
of functions that return non-trivial structures, they will see the
message for every function.  Otherwise, the message should not get
printed. 

Not aware of anyway we could prompt the user to ask them if they want
to suppress this message from the command line.  If there is, can you
give me a pointer to an example?

We could do something like:

catch (const gdb_exception_error &e)
    {
      static int warned = 0;

      if (warned++ < 6)
        warning ("Cannot determine the function return value.\n"
                 "Try compiling with -fvar-tracking.");
    }

So the warning is only printed the first five or whatever number of
times we want before suppressing the message.  Not sure what the right
number of times should be? 

Would adding something like this be agreeable?

                        Carl 

Re: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Kevin Buettner]

On Fri, 14 Oct 2022 16:23:58 -0700
Carl Love <cel@us.ibm.com> wrote:

> > 2) Whitespace complaints:
> > 
> > Applying: PowerPC, fix support for printing the function return value
> > for non-trivial values.
> > ...patch:161: indent with spaces.
> >                       "gdbarch_dump: get_return_buf_addr = <%s>\n",
> > ...patch:162: indent with spaces.
> >                       host_address_to_string (gdbarch-  
> > >get_return_buf_addr));  
> > ...patch:182: indent with spaces.
> >                                  gdbarch_get_return_buf_addr_ftype
> > get_return_buf_addr)
> > warning: 3 lines add whitespace errors.
> > 
> > (I substituted some long paths local to my machines w/ '...'.)  
> 
> Yes, I have seen the complaints about the white spaces.  The whitespace
> complaints come from file gdb/gdbarch.c which is a generated file.  As
> I assume you know, the file was regenerated by the gdb/gdbarch.py based
> on the changes to gdb/gdbarch-components.py.  The regenerated file is
> set to read only.  Not sure what I can do to rectify the white spaces? 
> Any ideas?

If the unwanted white space is generated by gdbarch.py,maybe fix that
script?

That said, if those files already have this problem, I'm okay with
it going in with the whitespace complaints.  (It can be fixed in
a later patch addressing just that problem.)

> > I already mentioned this last week, but...
> > 
> > s/GDB ABI/gdbarch method/
> > 
> > I guess you could also refer to it as an API which you did later on
> > in your commit remarks.  (What I really object to is calling it an
> > "ABI", which it definitely is not - that said it's easy to mis-type
> > API
> > as ABI, so I do understand this mistake.)  
> 
> Yea, I think referring to it as a new gdbarch method is probably better
> than GDB API/ABI.  I have changed the GDB API/ABI references to gdbarch
> method.  I will need to update that in the message to the GDB
> maintainers as well when I send the next version of the patch.

Sounds good.

> > > value of r3 on entry to the function.  This requires the use of the
> > > -fvar-tracking compiler option to compile the user application thus
> > > generating the DW_OP_entry_value in the binary.  The DW_OP_entries
> > > in the  
> > 
> > I don't think that "DW_OP_entries" is correct here.  Maybe
> > "DW_OP_entry_value
> > entries"?  
> 
> > > binary file enables GDB can resolve the DW_TAG_call_sites.  This
> > > new API  
> > 
> > Maybe instead of "enables GDB can resolve the DW_TAG_call_sites", say
> > "allow GDB to resolve the DW_TAG_call_site entries".
> > 
> >   
> > > is used to get the return buffer address, in the case of a function
> > > returning a nontrivial data type, on exit from the function.  The
> > > GDB
> > > function should_stop checks to see if RETURN_BUF is non-zero.  By
> > > default,
> > > RETURN_BUF will be set to zero by the new ABI call for all
> > > architectures  
> > 
> > s/ABI/API/
> >   
> > > but PowerPC.  The get_return_value function will be used to obtain
> > > the
> > > return value on these architectures as is currently being done if
> > > RETURN_BUF is zero.  On PowerPC, the new ABI will return a nonzero
> > > address
> > > in RETURN_BUF if the value can be determined.  The value_at
> > > function uses
> > > the return buffer address to get the return value.  
> 
> So, I tried to address all of the above comments about ABI,
> "DW_OP_entries" etc.  The paragraph now reads:
> 
> This patch adds a new gdbarch method to allow PowerPC to access the value
> of r3 on entry to a function. On PowerPC, the new gdb arch method attempts
> to use the DW_OP_entry_value for the DWARF entries, when exiting the
> function, to determine the value of r3 on entry to the function.  This
> requires the use of the -fvar-tracking compiler option to compile the
> user application thus generating the DW_OP_entry_value in the binary.  The
> DW_OP_entry_value entries in the binary file allows GDB to resolve the
> DW_TAG_call_site entries.  This new gdbarch method is used to get the
> return buffer address, in the case of a function returning a nontrivial
> data type, on exit from the function.  The GDB function should_stop checks
> to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set to
> zero by the new gdbarch method call for all architectures except PowerPC.
> The get_return_value function will be used to obtain the return value on
> all other architectures as is currently being done if RETURN_BUF is zero.
> On PowerPC, the new gdbarch method will return a nonzero address in
> RETURN_BUF if the value can be determined.  The value_at function uses the
> return buffer address to get the return value.
> 
> Which I think covers all of the comments above. 

Yes, this looks good, though I'd use 'gdbarch' in place of 'gdb arch'.
This is a small nit though; if you'd prefer to make them separate
words, I won't argue about it.

> > >    if (execution_direction == EXEC_REVERSE)
> > > diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> > > index 14effb93210..59145f9f166 100644
> > > --- a/gdb/ppc-sysv-tdep.c
> > > +++ b/gdb/ppc-sysv-tdep.c
> > > @@ -28,6 +28,7 @@
> > >  #include "objfiles.h"
> > >  #include "infcall.h"
> > >  #include "dwarf2.h"
> > > +#include "dwarf2/loc.h"
> > >  #include "target-float.h"
> > >  #include <algorithm>
> > >  
> > > @@ -2156,3 +2157,39 @@ ppc64_sysv_abi_return_value (struct gdbarch
> > > *gdbarch, struct value *function,
> > >    return RETURN_VALUE_STRUCT_CONVENTION;
> > >  }
> > >  
> > > +CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
> > > +					  frame_info *cur_frame)
> > > +{
> > > +  /* On PowerPC, the ABI specifies r3 is the address where an
> > > argument  
> > 
> > s/specifies/specifies that/
> > 
> > I'm not sure about this phrase:  "where an argument and".  If you
> > mean
> > to say that r3 is sometimes used for arguments (when it's not used
> > for
> > the address of the return value buffer), then that's true, but it's
> > not relevant here.
> > 
> > So, I guess that I'm asking that this comment be reworded somewhat.
> >   
> > > +     and the return value is stored.  The PowerPC ABI does not
> > > guarantee
> > > +     r3 will be preserved when the function returns.  The function
> > > returns
> > > +     the address of the buffer stored in r3.  
> > 
> > I find this part confusing.  If the function returns the address of
> > the buffer, doesn't it do so by returning it in r3?  (And, if so, it
> > seems to me that this code would be unnecessary - so, clearly, I must
> > be missing something...)  
> 
> > > +
> > > +     Attempt to determine the value of r3 on entry to the function
> > > using the
> > > +     DW_OP_entry_value DWARF entries.  This requires compiling the
> > > user
> > > +     program with -fvar-tracking to resolve the DW_TAG_call_sites
> > > in the
> > > +     binary file.  */  
> 
> OK, so some rework here to make things a bit better.  I changed the
> header comment for function ppc64_sysv_get_return_buf_addr to the
> following.  Hopefully this addresses the various questions and
> comments.
> 
>   /* The PowerPC ABI specifies aggregates that are not returned by value
>      are returned in a storage buffer provided by the caller.  The
>      address of the storage buffer is provided as a hidden first input
>      arguement in register r3.  The PowerPC ABI does not guarantee that
>      register r3 will not be changed while executing the function.  Hence, it
>      cannot be assumed that r3 will still contain the address of the storage
>      buffer when execution reaches the end of the function.
> 
>      This function attempts to determine the value of r3 on entry to the
>      function using the DW_OP_entry_value DWARF entries.  This requires
>      compiling the user program with -fvar-tracking to resolve the
>      DW_TAG_call_sites in the binary file.  */
> 
> Hopefully that is clearer and clears up any confusion as to the role of r3.

Thanks for reworking that comment.

> > > +  catch (const gdb_exception_error &e)
> > > +    {
> > > +      warning ("Cannot determine the function return value.\n"
> > > +	       "Try compiling with -fvar-tracking.");  
> > 
> > Will this warning be printed a lot?  (I'm wondering if there should
> > be a
> > way to shut it off...)  
> 
> Hmm, hard to say how often it will get printed.  It is going to be up
> to what the user is trying to do.  If they are stopping on the return
> of functions that return non-trivial structures, they will see the
> message for every function.  Otherwise, the message should not get
> printed. 
> 
> Not aware of anyway we could prompt the user to ask them if they want
> to suppress this message from the command line.  If there is, can you
> give me a pointer to an example?
> 
> We could do something like:
> 
> catch (const gdb_exception_error &e)
>     {
>       static int warned = 0;
> 
>       if (warned++ < 6)
>         warning ("Cannot determine the function return value.\n"
>                  "Try compiling with -fvar-tracking.");
>     }
> 
> So the warning is only printed the first five or whatever number of
> times we want before suppressing the message.  Not sure what the right
> number of times should be? 
> 
> Would adding something like this be agreeable?

Maybe.  If we go that route, I'd recommend printing an additional
message along side the last "Cannot determine the function return value."
message indicating that these messages are now being suppressed.

We have several mechanisms in gdb for suppressing messages already. 
Probably the most used one is the "complaints" facility for
supressing warnings while reading symbols.  (See
gdb/complaints.{h,c}.) I recently added a bfd error supression
mechanism in gdb_bfd.c.  Look for gdb_bfd_error_handler() in that
file.  There's also lim_warning() in ada-lang.c along with an old
FIXME remark indicating that it's doing something similar to that of
complaint().  There may be others, but I don't know for sure.
None of them prompt the user about whether to continue to print
warnings; I'd prefer not to do it that way.  Hard coding it to six (or
something) is okay.  Making it user-settable with some default is
also okay.  That said, this is just one message. It seems like overkill
to add a setting for this one message.

After thinking about this a bit more, I think we should go with your
original code which doesn't suppress any messages.  If we find that
it's too noisy, we can add something along the lines of what you
suggested above.

Kevin

RE: [PATCH 2/2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

On Mon, 2022-10-17 at 18:06 -0700, Kevin Buettner wrote:
> On Fri, 14 Oct 2022 16:23:58 -0700
> Carl Love <cel@us.ibm.com> wrote:
> 
> > > 2) Whitespace complaints:
> > > 
> > > Applying: PowerPC, fix support for printing the function return
> > > value
> > > for non-trivial values.
> > > ...patch:161: indent with spaces.
> > >                       "gdbarch_dump: get_return_buf_addr =
> > > <%s>\n",
> > > ...patch:162: indent with spaces.
> > >                       host_address_to_string (gdbarch-  
> > > > get_return_buf_addr));  
> > > ...patch:182: indent with spaces.
> > >                                  gdbarch_get_return_buf_addr_ftyp
> > > e
> > > get_return_buf_addr)
> > > warning: 3 lines add whitespace errors.
> > > 
> > > (I substituted some long paths local to my machines w/ '...'.)  
> > 
> > Yes, I have seen the complaints about the white spaces.  The
> > whitespace
> > complaints come from file gdb/gdbarch.c which is a generated
> > file.  As
> > I assume you know, the file was regenerated by the gdb/gdbarch.py
> > based
> > on the changes to gdb/gdbarch-components.py.  The regenerated file
> > is
> > set to read only.  Not sure what I can do to rectify the white
> > spaces? 
> > Any ideas?
> 
> If the unwanted white space is generated by gdbarch.py,maybe fix that
> script?
> 
> That said, if those files already have this problem, I'm okay with
> it going in with the whitespace complaints.  (It can be fixed in
> a later patch addressing just that problem.)

I see the same white space issues through out the file.  The script
generates the spaces so yes, it probably needs fixing and then cleanup
the white spaces in the file.  However, that is beyond the scope of
this patch.

<snip
?
> > So, I tried to address all of the above comments about ABI,
> > "DW_OP_entries" etc.  The paragraph now reads:
> > 
> > This patch adds a new gdbarch method to allow PowerPC to access the
> > value
> > of r3 on entry to a function. On PowerPC, the new gdb arch method
> > attempts
> > to use the DW_OP_entry_value for the DWARF entries, when exiting
> > the
> > function, to determine the value of r3 on entry to the
> > function.  This
> > requires the use of the -fvar-tracking compiler option to compile
> > the
> > user application thus generating the DW_OP_entry_value in the
> > binary.  The
> > DW_OP_entry_value entries in the binary file allows GDB to resolve
> > the
> > DW_TAG_call_site entries.  This new gdbarch method is used to get
> > the
> > return buffer address, in the case of a function returning a
> > nontrivial
> > data type, on exit from the function.  The GDB function should_stop
> > checks
> > to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be
> > set to
> > zero by the new gdbarch method call for all architectures except
> > PowerPC.
> > The get_return_value function will be used to obtain the return
> > value on
> > all other architectures as is currently being done if RETURN_BUF is
> > zero.
> > On PowerPC, the new gdbarch method will return a nonzero address in
> > RETURN_BUF if the value can be determined.  The value_at function
> > uses the
> > return buffer address to get the return value.
> > 
> > Which I think covers all of the comments above. 
> 
> Yes, this looks good, though I'd use 'gdbarch' in place of 'gdb
> arch'.
> This is a small nit though; if you'd prefer to make them separate
> words, I won't argue about it.

I intended to use "gdbarch" not "gdb arch".  Fixed.

<snip>

> 
> Thanks for reworking that comment.
> 
> > > > +  catch (const gdb_exception_error &e)
> > > > +    {
> > > > +      warning ("Cannot determine the function return value.\n"
> > > > +	       "Try compiling with -fvar-tracking.");  
> > > 
> > > Will this warning be printed a lot?  (I'm wondering if there
> > > should
> > > be a
> > > way to shut it off...)  
> > 
> > Hmm, hard to say how often it will get printed.  It is going to be
> > up
> > to what the user is trying to do.  If they are stopping on the
> > return
> > of functions that return non-trivial structures, they will see the
> > message for every function.  Otherwise, the message should not get
> > printed. 
> > 
> > Not aware of anyway we could prompt the user to ask them if they
> > want
> > to suppress this message from the command line.  If there is, can
> > you
> > give me a pointer to an example?
> > 
> > We could do something like:
> > 
> > catch (const gdb_exception_error &e)
> >     {
> >       static int warned = 0;
> > 
> >       if (warned++ < 6)
> >         warning ("Cannot determine the function return value.\n"
> >                  "Try compiling with -fvar-tracking.");
> >     }
> > 
> > So the warning is only printed the first five or whatever number of
> > times we want before suppressing the message.  Not sure what the
> > right
> > number of times should be? 
> > 
> > Would adding something like this be agreeable?
> 
> Maybe.  If we go that route, I'd recommend printing an additional
> message along side the last "Cannot determine the function return
> value."
> message indicating that these messages are now being suppressed.
> 
> We have several mechanisms in gdb for suppressing messages already. 
> Probably the most used one is the "complaints" facility for
> supressing warnings while reading symbols.  (See
> gdb/complaints.{h,c}.) I recently added a bfd error supression
> mechanism in gdb_bfd.c.  Look for gdb_bfd_error_handler() in that
> file.  There's also lim_warning() in ada-lang.c along with an old
> FIXME remark indicating that it's doing something similar to that of
> complaint().  There may be others, but I don't know for sure.
> None of them prompt the user about whether to continue to print
> warnings; I'd prefer not to do it that way.  Hard coding it to six
> (or
> something) is okay.  Making it user-settable with some default is
> also okay.  That said, this is just one message. It seems like
> overkill
> to add a setting for this one message.
> 
> After thinking about this a bit more, I think we should go with your
> original code which doesn't suppress any messages.  If we find that
> it's too noisy, we can add something along the lines of what you
> suggested above.

I think that sounds reasonable.  We can always look at suppressing the
messages in the future if it looks like an issue.  I am leaving the
code as it was originally.

Thanks again for the feedback.  Take care.

                          Carl 

[PATCH 2/2 ver 2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

GDB maintainers:

Version 2, updated comments per Kevin's comments.  Changed "frame_info
*" to frame_info_ptr per Bruno's comments.  Discussed supressing the
new warning after printing it with Kevin.  In the end we decided to not
suppress the warnings.  Suppression can be added later if deemed
necessary.  The white space introduced by the gdb/gdbarch-components.py 
needs to be addressed by a future patch to fix the script and the
generated files.

On PowerPC, a non-trivial return value for a function cannot be
reliably determined with the current PowerPC ABI.  The issue is the
address of the return buffer for a non-trivial value is passed to the
function in register r3.  However, at the end of the function the ABI
does not guarantee r3 will not be changed.  Hence it cannot be reliably
used to get the address of the return buffer.

This patch adds a new GDB ABI to allow GDB to obtain the input value of
r3 when the function returns.  This is done by using the DW_OP_entry
value for the DWARF entries to obtain the value of r3 on entry to the
function.  The ABI allows GDB to get the correct address for the return
buffer on exit from the function.

This patch fixes the five test failures in gdb.cp/non-trivial-
retval.exp.

The patch has been re-tested on PowerPC and X86-64 with no regression
failures.

Please let me know if this version of the patch is acceptable for GDB
mainline.  Thanks.

                         Carl Love


-----------------------------------
PowerPC, fix support for printing the function return value for non-trivial values.

Currently, a non-trivial return value from a function cannot currently be
reliably determined on PowerPC.  This is due to the fact that the PowerPC
ABI uses register r3 to store the address of the buffer containing the
non-trivial return value when the function is called.  The PowerPC ABI
does not guarantee the value in register r3 is not modified in the
function.  Thus the value in r3 cannot be reliably used to obtain the
return addreses on exit from the function.

This patch adds a new gdbarch method to allow PowerPC to access the value
of r3 on entry to a function. On PowerPC, the new gdbarch method attempts
to use the DW_OP_entry_value for the DWARF entries, when exiting the
function, to determine the value of r3 on entry to the function.  This
requires the use of the -fvar-tracking compiler option to compile the
user application thus generating the DW_OP_entry_value in the binary.  The
DW_OP_entry_value entries in the binary file allows GDB to resolve the
DW_TAG_call_site entries.  This new gdbarch method is used to get the
return buffer address, in the case of a function returning a nontrivial
data type, on exit from the function.  The GDB function should_stop checks
to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set to
zero by the new gdbarch method call for all architectures except PowerPC.
The get_return_value function will be used to obtain the return value on
all other architectures as is currently being done if RETURN_BUF is zero.
On PowerPC, the new gdbarch method will return a nonzero address in
RETURN_BUF if the value can be determined.  The value_at function uses the
return buffer address to get the return value.

This patch fixes five testcase failures in gdb.cp/non-trivial-retval.exp.
The correct function return values are now reported.

Note this patch is dependent on patch: "PowerPC, function
ppc64_sysv_abi_return_value add missing return value convention".

This patch has been tested on Power 10 and x86-64 with no regressions.

Signed-off-by: Carl Love <cel@us.ibm.com>
---
 gdb/arch-utils.c                            |  6 +++
 gdb/arch-utils.h                            |  4 ++
 gdb/dwarf2/loc.c                            | 10 +----
 gdb/dwarf2/loc.h                            | 11 ++++++
 gdb/gdbarch-components.py                   | 15 ++++++++
 gdb/gdbarch-gen.h                           | 11 ++++++
 gdb/gdbarch.c                               | 23 ++++++++++++
 gdb/infcmd.c                                | 41 +++++++++++++++++++--
 gdb/ppc-sysv-tdep.c                         | 41 +++++++++++++++++++++
 gdb/ppc-tdep.h                              |  2 +
 gdb/rs6000-tdep.c                           |  6 ++-
 gdb/testsuite/gdb.cp/non-trivial-retval.exp |  9 ++++-
 gdb/testsuite/lib/gdb.exp                   |  8 ++++
 13 files changed, 173 insertions(+), 14 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 6a72b3f5efd..1358987c02f 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1092,6 +1092,12 @@ default_read_core_file_mappings
 {
 }
 
+CORE_ADDR
+default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
+{
+  return 0;
+}
+
 /* Non-zero if we want to trace architecture code.  */
 
 #ifndef GDBARCH_DEBUG
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index f6229f434fd..46018a6fbbb 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -300,4 +300,8 @@ extern void default_read_core_file_mappings
    struct bfd *cbfd,
    read_core_file_mappings_pre_loop_ftype pre_loop_cb,
    read_core_file_mappings_loop_ftype loop_cb);
+
+/* Default implementation of gdbarch default_get_return_buf_addr method.  */
+extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
+					      frame_info_ptr cur_frame);
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 791648d6e7e..b88226db610 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -1325,14 +1325,8 @@ static const struct lval_funcs entry_data_value_funcs =
   entry_data_value_free_closure
 };
 
-/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND and KIND_U
-   are used to match DW_AT_location at the caller's
-   DW_TAG_call_site_parameter.
-
-   Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if it
-   cannot resolve the parameter for any reason.  */
-
-static struct value *
+/* See dwarf2/loc.h.  */
+struct value *
 value_of_dwarf_reg_entry (struct type *type, frame_info_ptr frame,
 			  enum call_site_parameter_kind kind,
 			  union call_site_parameter_u kind_u)
diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
index d6709f2e342..9156e1ee533 100644
--- a/gdb/dwarf2/loc.h
+++ b/gdb/dwarf2/loc.h
@@ -296,4 +296,15 @@ extern struct value *indirect_synthetic_pointer
    dwarf2_per_objfile *per_objfile, frame_info_ptr frame,
    struct type *type, bool resolve_abstract_p = false);
 
+/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND and KIND_U
+   are used to match DW_AT_location at the caller's
+   DW_TAG_call_site_parameter.
+
+   Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if
+   it cannot resolve the parameter for any reason.  */
+
+extern struct value *value_of_dwarf_reg_entry (struct type *type,
+					       struct frame_info_ptr frame,
+					       enum call_site_parameter_kind kind,
+					       union call_site_parameter_u kind_u);
 #endif /* DWARF2LOC_H */
diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index 46e7565f293..325801256a2 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -868,6 +868,21 @@ for instance).
     invalid=True,
 )
 
+Function(
+    comment="""
+Return the address at which the value being returned from
+the current function will be stored.  This routine is only
+called if the current function uses the the "struct return
+convention".
+
+May return 0 when unable to determine that address.""",
+    type="CORE_ADDR",
+    name="get_return_buf_addr",
+    params=[("struct type *", "val_type"), ("frame_info_ptr", "cur_frame")],
+    predefault="default_get_return_buf_addr",
+    invalid=False,
+)
+
 Method(
     comment="""
 Return true if the return value of function is stored in the first hidden
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index 840de585869..66e2e761385 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -441,6 +441,17 @@ typedef enum return_value_convention (gdbarch_return_value_ftype) (struct gdbarc
 extern enum return_value_convention gdbarch_return_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
 extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_value_ftype *return_value);
 
+/* Return the address at which the value being returned from
+   the current function will be stored.  This routine is only
+   called if the current function uses the the "struct return
+   convention".
+
+   May return 0 when unable to determine that address. */
+
+typedef CORE_ADDR (gdbarch_get_return_buf_addr_ftype) (struct type *val_type, frame_info_ptr cur_frame);
+extern CORE_ADDR gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info_ptr cur_frame);
+extern void set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, gdbarch_get_return_buf_addr_ftype *get_return_buf_addr);
+
 /* Return true if the return value of function is stored in the first hidden
    parameter.  In theory, this feature should be language-dependent, specified
    by language and its ABI, such as C++.  Unfortunately, compiler may
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 559e92dee58..3bbb70e7be9 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -116,6 +116,7 @@ struct gdbarch
   gdbarch_address_to_pointer_ftype *address_to_pointer = nullptr;
   gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
   gdbarch_return_value_ftype *return_value = nullptr;
+  gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = nullptr;
   gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = nullptr;
   gdbarch_skip_prologue_ftype *skip_prologue = nullptr;
   gdbarch_skip_main_prologue_ftype *skip_main_prologue = nullptr;
@@ -313,6 +314,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
   gdbarch->value_from_register = default_value_from_register;
   gdbarch->pointer_to_address = unsigned_pointer_to_address;
   gdbarch->address_to_pointer = unsigned_address_to_pointer;
+  gdbarch->get_return_buf_addr = default_get_return_buf_addr;
   gdbarch->return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
   gdbarch->breakpoint_from_pc = default_breakpoint_from_pc;
   gdbarch->sw_breakpoint_from_kind = NULL;
@@ -465,6 +467,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of address_to_pointer, invalid_p == 0 */
   /* Skip verify of integer_to_address, has predicate.  */
   /* Skip verify of return_value, has predicate.  */
+  /* Skip verify of get_return_buf_addr, invalid_p == 0 */
   /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */
   if (gdbarch->skip_prologue == 0)
     log.puts ("\n\tskip_prologue");
@@ -877,6 +880,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
                       "gdbarch_dump: return_value = <%s>\n",
                       host_address_to_string (gdbarch->return_value));
+  gdb_printf (file,
+                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
+                      host_address_to_string (gdbarch->get_return_buf_addr));
   gdb_printf (file,
                       "gdbarch_dump: return_in_first_hidden_param_p = <%s>\n",
                       host_address_to_string (gdbarch->return_in_first_hidden_param_p));
@@ -2684,6 +2690,23 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
   gdbarch->return_value = return_value;
 }
 
+CORE_ADDR
+gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info_ptr cur_frame)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->get_return_buf_addr != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_get_return_buf_addr called\n");
+  return gdbarch->get_return_buf_addr (val_type, cur_frame);
+}
+
+void
+set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch,
+                                 gdbarch_get_return_buf_addr_ftype get_return_buf_addr)
+{
+  gdbarch->get_return_buf_addr = get_return_buf_addr;
+}
+
 int
 gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type)
 {
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index d729732c81c..2e7cca1a1aa 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -55,6 +55,7 @@
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
+#include "dwarf2/loc.h"
 
 /* Local functions: */
 
@@ -1598,6 +1599,12 @@ struct finish_command_fsm : public thread_fsm
      return value.  */
   struct return_value_info return_value_info {};
 
+  /* If the current function uses the "struct return convention",
+     this holds the address at which the value being returned will
+     be stored, or zero if that address could not be determined or
+     the "struct return convention" is not being used.  */
+  CORE_ADDR return_buf;
+
   explicit finish_command_fsm (struct interp *cmd_interp)
     : thread_fsm (cmd_interp)
   {
@@ -1636,7 +1643,13 @@ finish_command_fsm::should_stop (struct thread_info *tp)
 	  struct value *func;
 
 	  func = read_var_value (function, NULL, get_current_frame ());
-	  rv->value = get_return_value (function, func);
+
+	  if (return_buf != 0)
+	    /* Retrieve return value from the buffer where it was saved.  */
+	      rv->value = value_at (rv->type, return_buf);
+	  else
+	      rv->value = get_return_value (function, func);
+
 	  if (rv->value != NULL)
 	    rv->value_history_index = record_latest_value (rv->value);
 	}
@@ -1852,8 +1865,28 @@ finish_command (const char *arg, int from_tty)
     }
 
   /* Find the function we will return from.  */
-
-  sm->function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
+  frame_info_ptr callee_frame = get_selected_frame (NULL);
+  sm->function = find_pc_function (get_frame_pc (callee_frame));
+
+  /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
+     attempt to determine the address of the return buffer.  */
+  enum return_value_convention return_value;
+  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
+
+  struct type * val_type
+    = check_typedef (sm->function->type()->target_type ());
+
+  return_value = gdbarch_return_value (gdbarch,
+				       read_var_value (sm->function, NULL,
+						       callee_frame),
+				       val_type, NULL, NULL, NULL);
+
+  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
+      && val_type->code() != TYPE_CODE_VOID)
+    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
+						  callee_frame);
+  else
+    sm->return_buf = 0;
 
   /* Print info on the selected frame, including level number but not
      source.  */
@@ -1871,7 +1904,7 @@ finish_command (const char *arg, int from_tty)
 	  gdb_printf (_("Run till exit from "));
 	}
 
-      print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
+      print_stack_frame (callee_frame, 1, LOCATION, 0);
     }
   frame.reinflate ();
 
diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index 14effb93210..f720e87e63b 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -28,6 +28,7 @@
 #include "objfiles.h"
 #include "infcall.h"
 #include "dwarf2.h"
+#include "dwarf2/loc.h"
 #include "target-float.h"
 #include <algorithm>
 
@@ -2156,3 +2157,43 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
   return RETURN_VALUE_STRUCT_CONVENTION;
 }
 
+CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
+					  frame_info_ptr cur_frame)
+{
+  /* The PowerPC ABI specifies aggregates that are not returned by value
+     are returned in a storage buffer provided by the caller.  The
+     address of the storage buffer is provided as a hidden first input
+     arguement in register r3.  The PowerPC ABI does not guarantee that
+     register r3 will not be changed while executing the function.  Hence, it
+     cannot be assumed that r3 will still contain the address of the storage
+     buffer when execution reaches the end of the function.
+
+     This function attempts to determine the value of r3 on entry to the
+     function using the DW_OP_entry_value DWARF entries.  This requires
+     compiling the user program with -fvar-tracking to resolve the
+     DW_TAG_call_sites in the binary file.  */
+
+  union call_site_parameter_u kind_u;
+  enum call_site_parameter_kind kind;
+  CORE_ADDR return_val = 0;
+
+  kind_u.dwarf_reg = 3;  /* First passed arg/return value is in r3.  */
+  kind = CALL_SITE_PARAMETER_DWARF_REG;
+
+  /* val_type is the type of the return value.  Need the pointer type
+     to the return value.  */
+  val_type = lookup_pointer_type (val_type);
+
+  try
+    {
+      return_val = value_as_address (value_of_dwarf_reg_entry (val_type,
+							       cur_frame,
+							       kind, kind_u));
+    }
+  catch (const gdb_exception_error &e)
+    {
+      warning ("Cannot determine the function return value.\n"
+	       "Try compiling with -fvar-tracking.");
+    }
+  return return_val;
+}
diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
index 34b250849b9..cbccd87820d 100644
--- a/gdb/ppc-tdep.h
+++ b/gdb/ppc-tdep.h
@@ -175,6 +175,8 @@ extern void ppc_collect_vsxregset (const struct regset *regset,
 				  const struct regcache *regcache,
 				  int regnum, void *vsxregs, size_t len);
 
+extern CORE_ADDR ppc64_sysv_get_return_buf_addr (type*, frame_info_ptr);
+
 /* Private data that this module attaches to struct gdbarch.  */
 
 /* ELF ABI version used by the inferior.  */
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 8b6d666bbe7..637cbb41651 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -8234,7 +8234,11 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_ps_regnum (gdbarch, tdep->ppc_ps_regnum);
 
   if (wordsize == 8)
-    set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
+    {
+      set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
+      set_gdbarch_get_return_buf_addr (gdbarch,
+				       ppc64_sysv_get_return_buf_addr);
+    }
   else
     set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
 
diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
index 21c390bc937..93a3a6832f7 100644
--- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
+++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
@@ -15,11 +15,18 @@
 
 # This file is part of the gdb testsuite
 
+set additional_flags ""
+
 if {[skip_cplus_tests]} { continue }
 
 standard_testfile .cc
 
-if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
+if {[have_fvar_tracking]} {
+    set additional_flags "additional_flags= -fvar-tracking"
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile [list debug c++ $additional_flags]]} {
+
     return -1
 }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index bfa9fec628e..d3b77b4869b 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -8582,6 +8582,14 @@ gdb_caching_proc have_fuse_ld_gold {
     return [gdb_simple_compile $me $src executable $flags]
 }
 
+# Return 1 if compiler supports fvar-tracking, otherwise return 0.
+gdb_caching_proc have_fvar_tracking {
+    set me "have_fvar_tracking"
+    set flags "additional_flags=-fvar-tracking"
+    set src { int main() { return 0; } }
+    return [gdb_simple_compile $me $src executable $flags]
+}
+
 # Return 1 if linker supports -Ttext-segment, otherwise return 0.
 gdb_caching_proc linker_supports_Ttext_segment_flag {
     set me "linker_supports_Ttext_segment_flag"
-- 
2.37.2

Re: [PATCH 2/2 ver 2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

Kevin, Bruno:

I haven't seen any comments on the updated patch.  Just wanted to check
and see if the updated patches look OK to you. Thanks.

                   Carl 

On Tue, 2022-10-18 at 11:55 -0700, Carl Love wrote:
> GDB maintainers:
> 
> Version 2, updated comments per Kevin's comments.  Changed
> "frame_info
> *" to frame_info_ptr per Bruno's comments.  Discussed supressing the
> new warning after printing it with Kevin.  In the end we decided to
> not
> suppress the warnings.  Suppression can be added later if deemed
> necessary.  The white space introduced by the gdb/gdbarch-
> components.py 
> needs to be addressed by a future patch to fix the script and the
> generated files.
> 
> On PowerPC, a non-trivial return value for a function cannot be
> reliably determined with the current PowerPC ABI.  The issue is the
> address of the return buffer for a non-trivial value is passed to the
> function in register r3.  However, at the end of the function the ABI
> does not guarantee r3 will not be changed.  Hence it cannot be
> reliably
> used to get the address of the return buffer.
> 
> This patch adds a new GDB ABI to allow GDB to obtain the input value
> of
> r3 when the function returns.  This is done by using the DW_OP_entry
> value for the DWARF entries to obtain the value of r3 on entry to the
> function.  The ABI allows GDB to get the correct address for the
> return
> buffer on exit from the function.
> 
> This patch fixes the five test failures in gdb.cp/non-trivial-
> retval.exp.
> 
> The patch has been re-tested on PowerPC and X86-64 with no regression
> failures.
> 
> Please let me know if this version of the patch is acceptable for GDB
> mainline.  Thanks.
> 
>                          Carl Love
> 
> 
> -----------------------------------
> PowerPC, fix support for printing the function return value for non-
> trivial values.
> 
> Currently, a non-trivial return value from a function cannot
> currently be
> reliably determined on PowerPC.  This is due to the fact that the
> PowerPC
> ABI uses register r3 to store the address of the buffer containing
> the
> non-trivial return value when the function is called.  The PowerPC
> ABI
> does not guarantee the value in register r3 is not modified in the
> function.  Thus the value in r3 cannot be reliably used to obtain the
> return addreses on exit from the function.
> 
> This patch adds a new gdbarch method to allow PowerPC to access the
> value
> of r3 on entry to a function. On PowerPC, the new gdbarch method
> attempts
> to use the DW_OP_entry_value for the DWARF entries, when exiting the
> function, to determine the value of r3 on entry to the
> function.  This
> requires the use of the -fvar-tracking compiler option to compile the
> user application thus generating the DW_OP_entry_value in the
> binary.  The
> DW_OP_entry_value entries in the binary file allows GDB to resolve
> the
> DW_TAG_call_site entries.  This new gdbarch method is used to get the
> return buffer address, in the case of a function returning a
> nontrivial
> data type, on exit from the function.  The GDB function should_stop
> checks
> to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set
> to
> zero by the new gdbarch method call for all architectures except
> PowerPC.
> The get_return_value function will be used to obtain the return value
> on
> all other architectures as is currently being done if RETURN_BUF is
> zero.
> On PowerPC, the new gdbarch method will return a nonzero address in
> RETURN_BUF if the value can be determined.  The value_at function
> uses the
> return buffer address to get the return value.
> 
> This patch fixes five testcase failures in gdb.cp/non-trivial-
> retval.exp.
> The correct function return values are now reported.
> 
> Note this patch is dependent on patch: "PowerPC, function
> ppc64_sysv_abi_return_value add missing return value convention".
> 
> This patch has been tested on Power 10 and x86-64 with no
> regressions.
> 
> Signed-off-by: Carl Love <cel@us.ibm.com>
> ---
>  gdb/arch-utils.c                            |  6 +++
>  gdb/arch-utils.h                            |  4 ++
>  gdb/dwarf2/loc.c                            | 10 +----
>  gdb/dwarf2/loc.h                            | 11 ++++++
>  gdb/gdbarch-components.py                   | 15 ++++++++
>  gdb/gdbarch-gen.h                           | 11 ++++++
>  gdb/gdbarch.c                               | 23 ++++++++++++
>  gdb/infcmd.c                                | 41
> +++++++++++++++++++--
>  gdb/ppc-sysv-tdep.c                         | 41
> +++++++++++++++++++++
>  gdb/ppc-tdep.h                              |  2 +
>  gdb/rs6000-tdep.c                           |  6 ++-
>  gdb/testsuite/gdb.cp/non-trivial-retval.exp |  9 ++++-
>  gdb/testsuite/lib/gdb.exp                   |  8 ++++
>  13 files changed, 173 insertions(+), 14 deletions(-)
> 
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 6a72b3f5efd..1358987c02f 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1092,6 +1092,12 @@ default_read_core_file_mappings
>  {
>  }
> 
> +CORE_ADDR
> +default_get_return_buf_addr (struct type *val_type, frame_info_ptr
> cur_frame)
> +{
> +  return 0;
> +}
> +
>  /* Non-zero if we want to trace architecture code.  */
> 
>  #ifndef GDBARCH_DEBUG
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index f6229f434fd..46018a6fbbb 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -300,4 +300,8 @@ extern void default_read_core_file_mappings
>     struct bfd *cbfd,
>     read_core_file_mappings_pre_loop_ftype pre_loop_cb,
>     read_core_file_mappings_loop_ftype loop_cb);
> +
> +/* Default implementation of gdbarch default_get_return_buf_addr
> method.  */
> +extern CORE_ADDR default_get_return_buf_addr (struct type
> *val_typegdbarch,
> +					      frame_info_ptr
> cur_frame);
>  #endif /* ARCH_UTILS_H */
> diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> index 791648d6e7e..b88226db610 100644
> --- a/gdb/dwarf2/loc.c
> +++ b/gdb/dwarf2/loc.c
> @@ -1325,14 +1325,8 @@ static const struct lval_funcs
> entry_data_value_funcs =
>    entry_data_value_free_closure
>  };
> 
> -/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND
> and KIND_U
> -   are used to match DW_AT_location at the caller's
> -   DW_TAG_call_site_parameter.
> -
> -   Function always returns non-NULL value.  It throws
> NO_ENTRY_VALUE_ERROR if it
> -   cannot resolve the parameter for any reason.  */
> -
> -static struct value *
> +/* See dwarf2/loc.h.  */
> +struct value *
>  value_of_dwarf_reg_entry (struct type *type, frame_info_ptr frame,
>  			  enum call_site_parameter_kind kind,
>  			  union call_site_parameter_u kind_u)
> diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
> index d6709f2e342..9156e1ee533 100644
> --- a/gdb/dwarf2/loc.h
> +++ b/gdb/dwarf2/loc.h
> @@ -296,4 +296,15 @@ extern struct value *indirect_synthetic_pointer
>     dwarf2_per_objfile *per_objfile, frame_info_ptr frame,
>     struct type *type, bool resolve_abstract_p = false);
> 
> +/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND
> and KIND_U
> +   are used to match DW_AT_location at the caller's
> +   DW_TAG_call_site_parameter.
> +
> +   Function always returns non-NULL value.  It throws
> NO_ENTRY_VALUE_ERROR if
> +   it cannot resolve the parameter for any reason.  */
> +
> +extern struct value *value_of_dwarf_reg_entry (struct type *type,
> +					       struct frame_info_ptr
> frame,
> +					       enum
> call_site_parameter_kind kind,
> +					       union
> call_site_parameter_u kind_u);
>  #endif /* DWARF2LOC_H */
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 46e7565f293..325801256a2 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -868,6 +868,21 @@ for instance).
>      invalid=True,
>  )
> 
> +Function(
> +    comment="""
> +Return the address at which the value being returned from
> +the current function will be stored.  This routine is only
> +called if the current function uses the the "struct return
> +convention".
> +
> +May return 0 when unable to determine that address.""",
> +    type="CORE_ADDR",
> +    name="get_return_buf_addr",
> +    params=[("struct type *", "val_type"), ("frame_info_ptr",
> "cur_frame")],
> +    predefault="default_get_return_buf_addr",
> +    invalid=False,
> +)
> +
>  Method(
>      comment="""
>  Return true if the return value of function is stored in the first
> hidden
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index 840de585869..66e2e761385 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -441,6 +441,17 @@ typedef enum return_value_convention
> (gdbarch_return_value_ftype) (struct gdbarc
>  extern enum return_value_convention gdbarch_return_value (struct
> gdbarch *gdbarch, struct value *function, struct type *valtype,
> struct regcache *regcache, gdb_byte *readbuf, const gdb_byte
> *writebuf);
>  extern void set_gdbarch_return_value (struct gdbarch *gdbarch,
> gdbarch_return_value_ftype *return_value);
> 
> +/* Return the address at which the value being returned from
> +   the current function will be stored.  This routine is only
> +   called if the current function uses the the "struct return
> +   convention".
> +
> +   May return 0 when unable to determine that address. */
> +
> +typedef CORE_ADDR (gdbarch_get_return_buf_addr_ftype) (struct type
> *val_type, frame_info_ptr cur_frame);
> +extern CORE_ADDR gdbarch_get_return_buf_addr (struct gdbarch
> *gdbarch, struct type *val_type, frame_info_ptr cur_frame);
> +extern void set_gdbarch_get_return_buf_addr (struct gdbarch
> *gdbarch, gdbarch_get_return_buf_addr_ftype *get_return_buf_addr);
> +
>  /* Return true if the return value of function is stored in the
> first hidden
>     parameter.  In theory, this feature should be language-dependent, 
> specified
>     by language and its ABI, such as C++.  Unfortunately, compiler
> may
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 559e92dee58..3bbb70e7be9 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -116,6 +116,7 @@ struct gdbarch
>    gdbarch_address_to_pointer_ftype *address_to_pointer = nullptr;
>    gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
>    gdbarch_return_value_ftype *return_value = nullptr;
> +  gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = nullptr;
>    gdbarch_return_in_first_hidden_param_p_ftype
> *return_in_first_hidden_param_p = nullptr;
>    gdbarch_skip_prologue_ftype *skip_prologue = nullptr;
>    gdbarch_skip_main_prologue_ftype *skip_main_prologue = nullptr;
> @@ -313,6 +314,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
>    gdbarch->value_from_register = default_value_from_register;
>    gdbarch->pointer_to_address = unsigned_pointer_to_address;
>    gdbarch->address_to_pointer = unsigned_address_to_pointer;
> +  gdbarch->get_return_buf_addr = default_get_return_buf_addr;
>    gdbarch->return_in_first_hidden_param_p =
> default_return_in_first_hidden_param_p;
>    gdbarch->breakpoint_from_pc = default_breakpoint_from_pc;
>    gdbarch->sw_breakpoint_from_kind = NULL;
> @@ -465,6 +467,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>    /* Skip verify of address_to_pointer, invalid_p == 0 */
>    /* Skip verify of integer_to_address, has predicate.  */
>    /* Skip verify of return_value, has predicate.  */
> +  /* Skip verify of get_return_buf_addr, invalid_p == 0 */
>    /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0
> */
>    if (gdbarch->skip_prologue == 0)
>      log.puts ("\n\tskip_prologue");
> @@ -877,6 +880,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct
> ui_file *file)
>    gdb_printf (file,
>                        "gdbarch_dump: return_value = <%s>\n",
>                        host_address_to_string (gdbarch-
> >return_value));
> +  gdb_printf (file,
> +                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
> +                      host_address_to_string (gdbarch-
> >get_return_buf_addr));
>    gdb_printf (file,
>                        "gdbarch_dump: return_in_first_hidden_param_p
> = <%s>\n",
>                        host_address_to_string (gdbarch-
> >return_in_first_hidden_param_p));
> @@ -2684,6 +2690,23 @@ set_gdbarch_return_value (struct gdbarch
> *gdbarch,
>    gdbarch->return_value = return_value;
>  }
> 
> +CORE_ADDR
> +gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type
> *val_type, frame_info_ptr cur_frame)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->get_return_buf_addr != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_get_return_buf_addr called\n");
> +  return gdbarch->get_return_buf_addr (val_type, cur_frame);
> +}
> +
> +void
> +set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch,
> +                                 gdbarch_get_return_buf_addr_ftype
> get_return_buf_addr)
> +{
> +  gdbarch->get_return_buf_addr = get_return_buf_addr;
> +}
> +
>  int
>  gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch,
> struct type *type)
>  {
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index d729732c81c..2e7cca1a1aa 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -55,6 +55,7 @@
>  #include "gdbsupport/gdb_optional.h"
>  #include "source.h"
>  #include "cli/cli-style.h"
> +#include "dwarf2/loc.h"
> 
>  /* Local functions: */
> 
> @@ -1598,6 +1599,12 @@ struct finish_command_fsm : public thread_fsm
>       return value.  */
>    struct return_value_info return_value_info {};
> 
> +  /* If the current function uses the "struct return convention",
> +     this holds the address at which the value being returned will
> +     be stored, or zero if that address could not be determined or
> +     the "struct return convention" is not being used.  */
> +  CORE_ADDR return_buf;
> +
>    explicit finish_command_fsm (struct interp *cmd_interp)
>      : thread_fsm (cmd_interp)
>    {
> @@ -1636,7 +1643,13 @@ finish_command_fsm::should_stop (struct
> thread_info *tp)
>  	  struct value *func;
> 
>  	  func = read_var_value (function, NULL, get_current_frame ());
> -	  rv->value = get_return_value (function, func);
> +
> +	  if (return_buf != 0)
> +	    /* Retrieve return value from the buffer where it was
> saved.  */
> +	      rv->value = value_at (rv->type, return_buf);
> +	  else
> +	      rv->value = get_return_value (function, func);
> +
>  	  if (rv->value != NULL)
>  	    rv->value_history_index = record_latest_value (rv->value);
>  	}
> @@ -1852,8 +1865,28 @@ finish_command (const char *arg, int from_tty)
>      }
> 
>    /* Find the function we will return from.  */
> -
> -  sm->function = find_pc_function (get_frame_pc (get_selected_frame
> (NULL)));
> +  frame_info_ptr callee_frame = get_selected_frame (NULL);
> +  sm->function = find_pc_function (get_frame_pc (callee_frame));
> +
> +  /* Determine the return convention.  If it is
> RETURN_VALUE_STRUCT_CONVENTION,
> +     attempt to determine the address of the return buffer.  */
> +  enum return_value_convention return_value;
> +  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
> +
> +  struct type * val_type
> +    = check_typedef (sm->function->type()->target_type ());
> +
> +  return_value = gdbarch_return_value (gdbarch,
> +				       read_var_value (sm->function,
> NULL,
> +						       callee_frame),
> +				       val_type, NULL, NULL, NULL);
> +
> +  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
> +      && val_type->code() != TYPE_CODE_VOID)
> +    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
> +						  callee_frame);
> +  else
> +    sm->return_buf = 0;
> 
>    /* Print info on the selected frame, including level number but
> not
>       source.  */
> @@ -1871,7 +1904,7 @@ finish_command (const char *arg, int from_tty)
>  	  gdb_printf (_("Run till exit from "));
>  	}
> 
> -      print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
> +      print_stack_frame (callee_frame, 1, LOCATION, 0);
>      }
>    frame.reinflate ();
> 
> diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> index 14effb93210..f720e87e63b 100644
> --- a/gdb/ppc-sysv-tdep.c
> +++ b/gdb/ppc-sysv-tdep.c
> @@ -28,6 +28,7 @@
>  #include "objfiles.h"
>  #include "infcall.h"
>  #include "dwarf2.h"
> +#include "dwarf2/loc.h"
>  #include "target-float.h"
>  #include <algorithm>
> 
> @@ -2156,3 +2157,43 @@ ppc64_sysv_abi_return_value (struct gdbarch
> *gdbarch, struct value *function,
>    return RETURN_VALUE_STRUCT_CONVENTION;
>  }
> 
> +CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
> +					  frame_info_ptr cur_frame)
> +{
> +  /* The PowerPC ABI specifies aggregates that are not returned by
> value
> +     are returned in a storage buffer provided by the caller.  The
> +     address of the storage buffer is provided as a hidden first
> input
> +     arguement in register r3.  The PowerPC ABI does not guarantee
> that
> +     register r3 will not be changed while executing the
> function.  Hence, it
> +     cannot be assumed that r3 will still contain the address of the
> storage
> +     buffer when execution reaches the end of the function.
> +
> +     This function attempts to determine the value of r3 on entry to
> the
> +     function using the DW_OP_entry_value DWARF entries.  This
> requires
> +     compiling the user program with -fvar-tracking to resolve the
> +     DW_TAG_call_sites in the binary file.  */
> +
> +  union call_site_parameter_u kind_u;
> +  enum call_site_parameter_kind kind;
> +  CORE_ADDR return_val = 0;
> +
> +  kind_u.dwarf_reg = 3;  /* First passed arg/return value is in
> r3.  */
> +  kind = CALL_SITE_PARAMETER_DWARF_REG;
> +
> +  /* val_type is the type of the return value.  Need the pointer
> type
> +     to the return value.  */
> +  val_type = lookup_pointer_type (val_type);
> +
> +  try
> +    {
> +      return_val = value_as_address (value_of_dwarf_reg_entry
> (val_type,
> +							       cur_fram
> e,
> +							       kind,
> kind_u));
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      warning ("Cannot determine the function return value.\n"
> +	       "Try compiling with -fvar-tracking.");
> +    }
> +  return return_val;
> +}
> diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
> index 34b250849b9..cbccd87820d 100644
> --- a/gdb/ppc-tdep.h
> +++ b/gdb/ppc-tdep.h
> @@ -175,6 +175,8 @@ extern void ppc_collect_vsxregset (const struct
> regset *regset,
>  				  const struct regcache *regcache,
>  				  int regnum, void *vsxregs, size_t
> len);
> 
> +extern CORE_ADDR ppc64_sysv_get_return_buf_addr (type*,
> frame_info_ptr);
> +
>  /* Private data that this module attaches to struct gdbarch.  */
> 
>  /* ELF ABI version used by the inferior.  */
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 8b6d666bbe7..637cbb41651 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -8234,7 +8234,11 @@ rs6000_gdbarch_init (struct gdbarch_info info,
> struct gdbarch_list *arches)
>    set_gdbarch_ps_regnum (gdbarch, tdep->ppc_ps_regnum);
> 
>    if (wordsize == 8)
> -    set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
> +    {
> +      set_gdbarch_return_value (gdbarch,
> ppc64_sysv_abi_return_value);
> +      set_gdbarch_get_return_buf_addr (gdbarch,
> +				       ppc64_sysv_get_return_buf_addr);
> +    }
>    else
>      set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
> 
> diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> index 21c390bc937..93a3a6832f7 100644
> --- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> +++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> @@ -15,11 +15,18 @@
> 
>  # This file is part of the gdb testsuite
> 
> +set additional_flags ""
> +
>  if {[skip_cplus_tests]} { continue }
> 
>  standard_testfile .cc
> 
> -if {[prepare_for_testing "failed to prepare" $testfile $srcfile
> {debug c++}]} {
> +if {[have_fvar_tracking]} {
> +    set additional_flags "additional_flags= -fvar-tracking"
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile
> [list debug c++ $additional_flags]]} {
> +
>      return -1
>  }
> 
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index bfa9fec628e..d3b77b4869b 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -8582,6 +8582,14 @@ gdb_caching_proc have_fuse_ld_gold {
>      return [gdb_simple_compile $me $src executable $flags]
>  }
> 
> +# Return 1 if compiler supports fvar-tracking, otherwise return 0.
> +gdb_caching_proc have_fvar_tracking {
> +    set me "have_fvar_tracking"
> +    set flags "additional_flags=-fvar-tracking"
> +    set src { int main() { return 0; } }
> +    return [gdb_simple_compile $me $src executable $flags]
> +}
> +
>  # Return 1 if linker supports -Ttext-segment, otherwise return 0.
>  gdb_caching_proc linker_supports_Ttext_segment_flag {
>      set me "linker_supports_Ttext_segment_flag"

Re: [PATCH 2/2 ver 2] PowerPC, fix support for printing the function return value for non-trivial values.

[Bruno Larsen]

Cheers,
Bruno

On 18/10/2022 20:55, Carl Love wrote:
> GDB maintainers:
>
> Version 2, updated comments per Kevin's comments.  Changed "frame_info
> *" to frame_info_ptr per Bruno's comments.  Discussed supressing the
> new warning after printing it with Kevin.  In the end we decided to not
> suppress the warnings.  Suppression can be added later if deemed
> necessary.  The white space introduced by the gdb/gdbarch-components.py
> needs to be addressed by a future patch to fix the script and the
> generated files.
>
> On PowerPC, a non-trivial return value for a function cannot be
> reliably determined with the current PowerPC ABI.  The issue is the
> address of the return buffer for a non-trivial value is passed to the
> function in register r3.  However, at the end of the function the ABI
> does not guarantee r3 will not be changed.  Hence it cannot be reliably
> used to get the address of the return buffer.
>
> This patch adds a new GDB ABI to allow GDB to obtain the input value of
> r3 when the function returns.  This is done by using the DW_OP_entry
> value for the DWARF entries to obtain the value of r3 on entry to the
> function.  The ABI allows GDB to get the correct address for the return
> buffer on exit from the function.
>
> This patch fixes the five test failures in gdb.cp/non-trivial-
> retval.exp.
>
> The patch has been re-tested on PowerPC and X86-64 with no regression
> failures.
>
> Please let me know if this version of the patch is acceptable for GDB
> mainline.  Thanks.
>
>                           Carl Love

This whole area of code is pure sorcery for me, but I do see that the 
patch fails to apply because of conflicts on gdbarch.c, due to Tom 
Tromey's "Inline initialization of gdbarch members" patch, and some 
style nits inlined.

Sorry for taking so long to reply to this that it is not applying again, 
but in the future I wouldn't wait for my review if someone approves the 
patch.

>
> -----------------------------------
> PowerPC, fix support for printing the function return value for non-trivial values.
>
> Currently, a non-trivial return value from a function cannot currently be
> reliably determined on PowerPC.  This is due to the fact that the PowerPC
> ABI uses register r3 to store the address of the buffer containing the
> non-trivial return value when the function is called.  The PowerPC ABI
> does not guarantee the value in register r3 is not modified in the
> function.  Thus the value in r3 cannot be reliably used to obtain the
> return addreses on exit from the function.
>
> This patch adds a new gdbarch method to allow PowerPC to access the value
> of r3 on entry to a function. On PowerPC, the new gdbarch method attempts
> to use the DW_OP_entry_value for the DWARF entries, when exiting the
> function, to determine the value of r3 on entry to the function.  This
> requires the use of the -fvar-tracking compiler option to compile the
> user application thus generating the DW_OP_entry_value in the binary.  The
> DW_OP_entry_value entries in the binary file allows GDB to resolve the
> DW_TAG_call_site entries.  This new gdbarch method is used to get the
> return buffer address, in the case of a function returning a nontrivial
> data type, on exit from the function.  The GDB function should_stop checks
> to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set to
> zero by the new gdbarch method call for all architectures except PowerPC.
> The get_return_value function will be used to obtain the return value on
> all other architectures as is currently being done if RETURN_BUF is zero.
> On PowerPC, the new gdbarch method will return a nonzero address in
> RETURN_BUF if the value can be determined.  The value_at function uses the
> return buffer address to get the return value.
>
> This patch fixes five testcase failures in gdb.cp/non-trivial-retval.exp.
> The correct function return values are now reported.
>
> Note this patch is dependent on patch: "PowerPC, function
> ppc64_sysv_abi_return_value add missing return value convention".
>
> This patch has been tested on Power 10 and x86-64 with no regressions.
>
> Signed-off-by: Carl Love <cel@us.ibm.com>
> ---
>   gdb/arch-utils.c                            |  6 +++
>   gdb/arch-utils.h                            |  4 ++
>   gdb/dwarf2/loc.c                            | 10 +----
>   gdb/dwarf2/loc.h                            | 11 ++++++
>   gdb/gdbarch-components.py                   | 15 ++++++++
>   gdb/gdbarch-gen.h                           | 11 ++++++
>   gdb/gdbarch.c                               | 23 ++++++++++++
>   gdb/infcmd.c                                | 41 +++++++++++++++++++--
>   gdb/ppc-sysv-tdep.c                         | 41 +++++++++++++++++++++
>   gdb/ppc-tdep.h                              |  2 +
>   gdb/rs6000-tdep.c                           |  6 ++-
>   gdb/testsuite/gdb.cp/non-trivial-retval.exp |  9 ++++-
>   gdb/testsuite/lib/gdb.exp                   |  8 ++++
>   13 files changed, 173 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
> index 6a72b3f5efd..1358987c02f 100644
> --- a/gdb/arch-utils.c
> +++ b/gdb/arch-utils.c
> @@ -1092,6 +1092,12 @@ default_read_core_file_mappings
>   {
>   }
>   
> +CORE_ADDR
> +default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
> +{
> +  return 0;
> +}
> +
>   /* Non-zero if we want to trace architecture code.  */
>   
>   #ifndef GDBARCH_DEBUG
> diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
> index f6229f434fd..46018a6fbbb 100644
> --- a/gdb/arch-utils.h
> +++ b/gdb/arch-utils.h
> @@ -300,4 +300,8 @@ extern void default_read_core_file_mappings
>      struct bfd *cbfd,
>      read_core_file_mappings_pre_loop_ftype pre_loop_cb,
>      read_core_file_mappings_loop_ftype loop_cb);
> +
> +/* Default implementation of gdbarch default_get_return_buf_addr method.  */
> +extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
> +					      frame_info_ptr cur_frame);
>   #endif /* ARCH_UTILS_H */
> diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
> index 791648d6e7e..b88226db610 100644
> --- a/gdb/dwarf2/loc.c
> +++ b/gdb/dwarf2/loc.c
> @@ -1325,14 +1325,8 @@ static const struct lval_funcs entry_data_value_funcs =
>     entry_data_value_free_closure
>   };
>   
> -/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND and KIND_U
> -   are used to match DW_AT_location at the caller's
> -   DW_TAG_call_site_parameter.
> -
> -   Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if it
> -   cannot resolve the parameter for any reason.  */
> -
> -static struct value *
> +/* See dwarf2/loc.h.  */
> +struct value *
>   value_of_dwarf_reg_entry (struct type *type, frame_info_ptr frame,
>   			  enum call_site_parameter_kind kind,
>   			  union call_site_parameter_u kind_u)
> diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
> index d6709f2e342..9156e1ee533 100644
> --- a/gdb/dwarf2/loc.h
> +++ b/gdb/dwarf2/loc.h
> @@ -296,4 +296,15 @@ extern struct value *indirect_synthetic_pointer
>      dwarf2_per_objfile *per_objfile, frame_info_ptr frame,
>      struct type *type, bool resolve_abstract_p = false);
>   
> +/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND and KIND_U
> +   are used to match DW_AT_location at the caller's
> +   DW_TAG_call_site_parameter.
> +
> +   Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if
> +   it cannot resolve the parameter for any reason.  */
> +
> +extern struct value *value_of_dwarf_reg_entry (struct type *type,
> +					       struct frame_info_ptr frame,
> +					       enum call_site_parameter_kind kind,
> +					       union call_site_parameter_u kind_u);
>   #endif /* DWARF2LOC_H */
> diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
> index 46e7565f293..325801256a2 100644
> --- a/gdb/gdbarch-components.py
> +++ b/gdb/gdbarch-components.py
> @@ -868,6 +868,21 @@ for instance).
>       invalid=True,
>   )
>   
> +Function(
> +    comment="""
> +Return the address at which the value being returned from
> +the current function will be stored.  This routine is only
> +called if the current function uses the the "struct return
> +convention".
> +
> +May return 0 when unable to determine that address.""",
> +    type="CORE_ADDR",
> +    name="get_return_buf_addr",
> +    params=[("struct type *", "val_type"), ("frame_info_ptr", "cur_frame")],
> +    predefault="default_get_return_buf_addr",
> +    invalid=False,
> +)
> +
>   Method(
>       comment="""
>   Return true if the return value of function is stored in the first hidden
> diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
> index 840de585869..66e2e761385 100644
> --- a/gdb/gdbarch-gen.h
> +++ b/gdb/gdbarch-gen.h
> @@ -441,6 +441,17 @@ typedef enum return_value_convention (gdbarch_return_value_ftype) (struct gdbarc
>   extern enum return_value_convention gdbarch_return_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
>   extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_value_ftype *return_value);
>   
> +/* Return the address at which the value being returned from
> +   the current function will be stored.  This routine is only
> +   called if the current function uses the the "struct return
> +   convention".
> +
> +   May return 0 when unable to determine that address. */
> +
> +typedef CORE_ADDR (gdbarch_get_return_buf_addr_ftype) (struct type *val_type, frame_info_ptr cur_frame);
> +extern CORE_ADDR gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info_ptr cur_frame);
> +extern void set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, gdbarch_get_return_buf_addr_ftype *get_return_buf_addr);
> +
>   /* Return true if the return value of function is stored in the first hidden
>      parameter.  In theory, this feature should be language-dependent, specified
>      by language and its ABI, such as C++.  Unfortunately, compiler may
> diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
> index 559e92dee58..3bbb70e7be9 100644
> --- a/gdb/gdbarch.c
> +++ b/gdb/gdbarch.c
> @@ -116,6 +116,7 @@ struct gdbarch
>     gdbarch_address_to_pointer_ftype *address_to_pointer = nullptr;
>     gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
>     gdbarch_return_value_ftype *return_value = nullptr;
> +  gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = nullptr;
>     gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = nullptr;
>     gdbarch_skip_prologue_ftype *skip_prologue = nullptr;
>     gdbarch_skip_main_prologue_ftype *skip_main_prologue = nullptr;
> @@ -313,6 +314,7 @@ gdbarch_alloc (const struct gdbarch_info *info,
>     gdbarch->value_from_register = default_value_from_register;
>     gdbarch->pointer_to_address = unsigned_pointer_to_address;
>     gdbarch->address_to_pointer = unsigned_address_to_pointer;
> +  gdbarch->get_return_buf_addr = default_get_return_buf_addr;
>     gdbarch->return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
>     gdbarch->breakpoint_from_pc = default_breakpoint_from_pc;
>     gdbarch->sw_breakpoint_from_kind = NULL;
> @@ -465,6 +467,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
>     /* Skip verify of address_to_pointer, invalid_p == 0 */
>     /* Skip verify of integer_to_address, has predicate.  */
>     /* Skip verify of return_value, has predicate.  */
> +  /* Skip verify of get_return_buf_addr, invalid_p == 0 */
>     /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */
>     if (gdbarch->skip_prologue == 0)
>       log.puts ("\n\tskip_prologue");
> @@ -877,6 +880,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
>     gdb_printf (file,
>                         "gdbarch_dump: return_value = <%s>\n",
>                         host_address_to_string (gdbarch->return_value));
> +  gdb_printf (file,
> +                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
> +                      host_address_to_string (gdbarch->get_return_buf_addr));
>     gdb_printf (file,
>                         "gdbarch_dump: return_in_first_hidden_param_p = <%s>\n",
>                         host_address_to_string (gdbarch->return_in_first_hidden_param_p));
> @@ -2684,6 +2690,23 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
>     gdbarch->return_value = return_value;
>   }
>   
> +CORE_ADDR
> +gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info_ptr cur_frame)
> +{
> +  gdb_assert (gdbarch != NULL);
> +  gdb_assert (gdbarch->get_return_buf_addr != NULL);
> +  if (gdbarch_debug >= 2)
> +    gdb_printf (gdb_stdlog, "gdbarch_get_return_buf_addr called\n");
> +  return gdbarch->get_return_buf_addr (val_type, cur_frame);
> +}
> +
> +void
> +set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch,
> +                                 gdbarch_get_return_buf_addr_ftype get_return_buf_addr)
> +{
> +  gdbarch->get_return_buf_addr = get_return_buf_addr;
> +}
> +
>   int
>   gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type)
>   {
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index d729732c81c..2e7cca1a1aa 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -55,6 +55,7 @@
>   #include "gdbsupport/gdb_optional.h"
>   #include "source.h"
>   #include "cli/cli-style.h"
> +#include "dwarf2/loc.h"
>   
>   /* Local functions: */
>   
> @@ -1598,6 +1599,12 @@ struct finish_command_fsm : public thread_fsm
>        return value.  */
>     struct return_value_info return_value_info {};
>   
> +  /* If the current function uses the "struct return convention",
> +     this holds the address at which the value being returned will
> +     be stored, or zero if that address could not be determined or
> +     the "struct return convention" is not being used.  */
> +  CORE_ADDR return_buf;
> +
>     explicit finish_command_fsm (struct interp *cmd_interp)
>       : thread_fsm (cmd_interp)
>     {
> @@ -1636,7 +1643,13 @@ finish_command_fsm::should_stop (struct thread_info *tp)
>   	  struct value *func;
>   
>   	  func = read_var_value (function, NULL, get_current_frame ());
> -	  rv->value = get_return_value (function, func);
> +
> +	  if (return_buf != 0)
> +	    /* Retrieve return value from the buffer where it was saved.  */
> +	      rv->value = value_at (rv->type, return_buf);
> +	  else
> +	      rv->value = get_return_value (function, func);
> +
>   	  if (rv->value != NULL)
>   	    rv->value_history_index = record_latest_value (rv->value);
>   	}
> @@ -1852,8 +1865,28 @@ finish_command (const char *arg, int from_tty)
>       }
>   
>     /* Find the function we will return from.  */
> -
> -  sm->function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
> +  frame_info_ptr callee_frame = get_selected_frame (NULL);
> +  sm->function = find_pc_function (get_frame_pc (callee_frame));
> +
> +  /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
> +     attempt to determine the address of the return buffer.  */
> +  enum return_value_convention return_value;
> +  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
> +
> +  struct type * val_type
> +    = check_typedef (sm->function->type()->target_type ());
missing space before (.
> +
> +  return_value = gdbarch_return_value (gdbarch,
> +				       read_var_value (sm->function, NULL,
> +						       callee_frame),
> +				       val_type, NULL, NULL, NULL);
> +
> +  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
> +      && val_type->code() != TYPE_CODE_VOID)
same here.
> +    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
> +						  callee_frame);
> +  else
> +    sm->return_buf = 0;
>   
>     /* Print info on the selected frame, including level number but not
>        source.  */
> @@ -1871,7 +1904,7 @@ finish_command (const char *arg, int from_tty)
>   	  gdb_printf (_("Run till exit from "));
>   	}
>   
> -      print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
> +      print_stack_frame (callee_frame, 1, LOCATION, 0);
>       }
>     frame.reinflate ();
>   
> diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> index 14effb93210..f720e87e63b 100644
> --- a/gdb/ppc-sysv-tdep.c
> +++ b/gdb/ppc-sysv-tdep.c
> @@ -28,6 +28,7 @@
>   #include "objfiles.h"
>   #include "infcall.h"
>   #include "dwarf2.h"
> +#include "dwarf2/loc.h"
>   #include "target-float.h"
>   #include <algorithm>
>   
> @@ -2156,3 +2157,43 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
>     return RETURN_VALUE_STRUCT_CONVENTION;
>   }
>   
> +CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
> +					  frame_info_ptr cur_frame)
> +{
> +  /* The PowerPC ABI specifies aggregates that are not returned by value
> +     are returned in a storage buffer provided by the caller.  The
> +     address of the storage buffer is provided as a hidden first input
> +     arguement in register r3.  The PowerPC ABI does not guarantee that
> +     register r3 will not be changed while executing the function.  Hence, it
> +     cannot be assumed that r3 will still contain the address of the storage
> +     buffer when execution reaches the end of the function.
> +
> +     This function attempts to determine the value of r3 on entry to the
> +     function using the DW_OP_entry_value DWARF entries.  This requires
> +     compiling the user program with -fvar-tracking to resolve the
> +     DW_TAG_call_sites in the binary file.  */
> +
> +  union call_site_parameter_u kind_u;
> +  enum call_site_parameter_kind kind;
> +  CORE_ADDR return_val = 0;
> +
> +  kind_u.dwarf_reg = 3;  /* First passed arg/return value is in r3.  */
> +  kind = CALL_SITE_PARAMETER_DWARF_REG;
> +
> +  /* val_type is the type of the return value.  Need the pointer type
> +     to the return value.  */
> +  val_type = lookup_pointer_type (val_type);
> +
> +  try
> +    {
> +      return_val = value_as_address (value_of_dwarf_reg_entry (val_type,
> +							       cur_frame,
> +							       kind, kind_u));
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      warning ("Cannot determine the function return value.\n"
> +	       "Try compiling with -fvar-tracking.");
> +    }
> +  return return_val;
> +}
> diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
> index 34b250849b9..cbccd87820d 100644
> --- a/gdb/ppc-tdep.h
> +++ b/gdb/ppc-tdep.h
> @@ -175,6 +175,8 @@ extern void ppc_collect_vsxregset (const struct regset *regset,
>   				  const struct regcache *regcache,
>   				  int regnum, void *vsxregs, size_t len);
>   
> +extern CORE_ADDR ppc64_sysv_get_return_buf_addr (type*, frame_info_ptr);
> +
>   /* Private data that this module attaches to struct gdbarch.  */
>   
>   /* ELF ABI version used by the inferior.  */
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 8b6d666bbe7..637cbb41651 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -8234,7 +8234,11 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>     set_gdbarch_ps_regnum (gdbarch, tdep->ppc_ps_regnum);
>   
>     if (wordsize == 8)
> -    set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
> +    {
> +      set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
> +      set_gdbarch_get_return_buf_addr (gdbarch,
> +				       ppc64_sysv_get_return_buf_addr);
> +    }
>     else
>       set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
>   
> diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> index 21c390bc937..93a3a6832f7 100644
> --- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> +++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
> @@ -15,11 +15,18 @@
>   
>   # This file is part of the gdb testsuite
>   
> +set additional_flags ""
> +
>   if {[skip_cplus_tests]} { continue }
>   
>   standard_testfile .cc
>   
> -if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
> +if {[have_fvar_tracking]} {
> +    set additional_flags "additional_flags= -fvar-tracking"
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile [list debug c++ $additional_flags]]} {
> +
>       return -1
>   }
>   
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index bfa9fec628e..d3b77b4869b 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -8582,6 +8582,14 @@ gdb_caching_proc have_fuse_ld_gold {
>       return [gdb_simple_compile $me $src executable $flags]
>   }
>   
> +# Return 1 if compiler supports fvar-tracking, otherwise return 0.
> +gdb_caching_proc have_fvar_tracking {
> +    set me "have_fvar_tracking"
> +    set flags "additional_flags=-fvar-tracking"
> +    set src { int main() { return 0; } }
> +    return [gdb_simple_compile $me $src executable $flags]
> +}
> +
>   # Return 1 if linker supports -Ttext-segment, otherwise return 0.
>   gdb_caching_proc linker_supports_Ttext_segment_flag {
>       set me "linker_supports_Ttext_segment_flag"

RE: [PATCH 2/2 ver 2] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

Bruno:

On Mon, 2022-11-07 at 15:56 +0100, Bruno Larsen wrote:
> This whole area of code is pure sorcery for me, but I do see that
> the 
> patch fails to apply because of conflicts on gdbarch.c, due to Tom 
> Tromey's "Inline initialization of gdbarch members" patch, and some 
> style nits inlined.
> 
> Sorry for taking so long to reply to this that it is not applying
> again, 
> but in the future I wouldn't wait for my review if someone approves
> the 
> patch.

Thanks for the reply.  I was hoping that Kevin would also let me know
if all of his concerns had been addressed.  I did fix up the two
formatting issues you mentioned, rebased and retested the patch on the
latest gdb source code tree.

I will post version 3 of the patch and ping version 2 of patch 1/2 and
see if we can get a maintainer to review the patches.  Thanks for all
the help and input.

                       Carl 

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

GDB maintainers:

Version 3, fixed two formatting issues noted by Bruno.  Patch was
rebased on the latest gdb source tree and retested with no regression
failures.

Version 2, updated comments per Kevin's comments.  Changed "frame_info
*" to frame_info_ptr per Bruno's comments.  Discussed supressing the
new warning after printing it with Kevin.  In the end we decided to not
suppress the warnings.  Suppression can be added later if deemed
necessary.  The white space introduced by the gdb/gdbarch-
components.py 
needs to be addressed by a future patch to fix the script and the
generated files.

On PowerPC, a non-trivial return value for a function cannot be
reliably determined with the current PowerPC ABI.  The issue is the
address of the return buffer for a non-trivial value is passed to the
function in register r3.  However, at the end of the function the ABI
does not guarantee r3 will not be changed.  Hence it cannot be reliably
used to get the address of the return buffer.

This patch adds a new GDB ABI to allow GDB to obtain the input value of
r3 when the function returns.  This is done by using the DW_OP_entry
value for the DWARF entries to obtain the value of r3 on entry to the
function.  The ABI allows GDB to get the correct address for the return
buffer on exit from the function.

This patch fixes the five test failures in gdb.cp/non-trivial-
retval.exp.

The patch has been re-tested on PowerPC and X86-64 with no regression
failures.

Please let me know if this version of the patch is acceptable for GDB
mainline.  Thanks.

                         Carl Love


-----------------------------------

PowerPC, fix support for printing the function return value for non-trivial values.

Currently, a non-trivial return value from a function cannot currently be
reliably determined on PowerPC.  This is due to the fact that the PowerPC
ABI uses register r3 to store the address of the buffer containing the
non-trivial return value when the function is called.  The PowerPC ABI
does not guarantee the value in register r3 is not modified in the
function.  Thus the value in r3 cannot be reliably used to obtain the
return addreses on exit from the function.

This patch adds a new gdbarch method to allow PowerPC to access the value
of r3 on entry to a function. On PowerPC, the new gdbarch method attempts
to use the DW_OP_entry_value for the DWARF entries, when exiting the
function, to determine the value of r3 on entry to the function.  This
requires the use of the -fvar-tracking compiler option to compile the
user application thus generating the DW_OP_entry_value in the binary.  The
DW_OP_entry_value entries in the binary file allows GDB to resolve the
DW_TAG_call_site entries.  This new gdbarch method is used to get the
return buffer address, in the case of a function returning a nontrivial
data type, on exit from the function.  The GDB function should_stop checks
to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set to
zero by the new gdbarch method call for all architectures except PowerPC.
The get_return_value function will be used to obtain the return value on
all other architectures as is currently being done if RETURN_BUF is zero.
On PowerPC, the new gdbarch method will return a nonzero address in
RETURN_BUF if the value can be determined.  The value_at function uses the
return buffer address to get the return value.

This patch fixes five testcase failures in gdb.cp/non-trivial-retval.exp.
The correct function return values are now reported.

Note this patch is dependent on patch: "PowerPC, function
ppc64_sysv_abi_return_value add missing return value convention".

This patch has been tested on Power 10 and x86-64 with no regressions.
---
 gdb/arch-utils.c                            |  6 +++
 gdb/arch-utils.h                            |  4 ++
 gdb/dwarf2/loc.c                            | 10 +----
 gdb/dwarf2/loc.h                            | 11 ++++++
 gdb/gdbarch-components.py                   | 15 ++++++++
 gdb/gdbarch-gen.h                           | 11 ++++++
 gdb/gdbarch.c                               | 22 +++++++++++
 gdb/infcmd.c                                | 41 +++++++++++++++++++--
 gdb/ppc-sysv-tdep.c                         | 41 +++++++++++++++++++++
 gdb/ppc-tdep.h                              |  2 +
 gdb/rs6000-tdep.c                           |  6 ++-
 gdb/testsuite/gdb.cp/non-trivial-retval.exp |  9 ++++-
 gdb/testsuite/lib/gdb.exp                   |  8 ++++
 13 files changed, 172 insertions(+), 14 deletions(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 5218bfc05e1..7b84daf046e 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -1084,6 +1084,12 @@ default_read_core_file_mappings
 {
 }
 
+CORE_ADDR
+default_get_return_buf_addr (struct type *val_type, frame_info_ptr cur_frame)
+{
+  return 0;
+}
+
 /* Non-zero if we want to trace architecture code.  */
 
 #ifndef GDBARCH_DEBUG
diff --git a/gdb/arch-utils.h b/gdb/arch-utils.h
index f6229f434fd..46018a6fbbb 100644
--- a/gdb/arch-utils.h
+++ b/gdb/arch-utils.h
@@ -300,4 +300,8 @@ extern void default_read_core_file_mappings
    struct bfd *cbfd,
    read_core_file_mappings_pre_loop_ftype pre_loop_cb,
    read_core_file_mappings_loop_ftype loop_cb);
+
+/* Default implementation of gdbarch default_get_return_buf_addr method.  */
+extern CORE_ADDR default_get_return_buf_addr (struct type *val_typegdbarch,
+					      frame_info_ptr cur_frame);
 #endif /* ARCH_UTILS_H */
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index c42359ab96e..8355aa44333 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -1325,14 +1325,8 @@ static const struct lval_funcs entry_data_value_funcs =
   entry_data_value_free_closure
 };
 
-/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND and KIND_U
-   are used to match DW_AT_location at the caller's
-   DW_TAG_call_site_parameter.
-
-   Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if it
-   cannot resolve the parameter for any reason.  */
-
-static struct value *
+/* See dwarf2/loc.h.  */
+struct value *
 value_of_dwarf_reg_entry (struct type *type, frame_info_ptr frame,
 			  enum call_site_parameter_kind kind,
 			  union call_site_parameter_u kind_u)
diff --git a/gdb/dwarf2/loc.h b/gdb/dwarf2/loc.h
index d6709f2e342..9156e1ee533 100644
--- a/gdb/dwarf2/loc.h
+++ b/gdb/dwarf2/loc.h
@@ -296,4 +296,15 @@ extern struct value *indirect_synthetic_pointer
    dwarf2_per_objfile *per_objfile, frame_info_ptr frame,
    struct type *type, bool resolve_abstract_p = false);
 
+/* Read parameter of TYPE at (callee) FRAME's function entry.  KIND and KIND_U
+   are used to match DW_AT_location at the caller's
+   DW_TAG_call_site_parameter.
+
+   Function always returns non-NULL value.  It throws NO_ENTRY_VALUE_ERROR if
+   it cannot resolve the parameter for any reason.  */
+
+extern struct value *value_of_dwarf_reg_entry (struct type *type,
+					       struct frame_info_ptr frame,
+					       enum call_site_parameter_kind kind,
+					       union call_site_parameter_u kind_u);
 #endif /* DWARF2LOC_H */
diff --git a/gdb/gdbarch-components.py b/gdb/gdbarch-components.py
index b2c7b784761..9b688998a7b 100644
--- a/gdb/gdbarch-components.py
+++ b/gdb/gdbarch-components.py
@@ -868,6 +868,21 @@ for instance).
     invalid=True,
 )
 
+Function(
+    comment="""
+Return the address at which the value being returned from
+the current function will be stored.  This routine is only
+called if the current function uses the the "struct return
+convention".
+
+May return 0 when unable to determine that address.""",
+    type="CORE_ADDR",
+    name="get_return_buf_addr",
+    params=[("struct type *", "val_type"), ("frame_info_ptr", "cur_frame")],
+    predefault="default_get_return_buf_addr",
+    invalid=False,
+)
+
 Method(
     comment="""
 Return true if the return value of function is stored in the first hidden
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index e0d7a08ff6a..a663316df16 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -441,6 +441,17 @@ typedef enum return_value_convention (gdbarch_return_value_ftype) (struct gdbarc
 extern enum return_value_convention gdbarch_return_value (struct gdbarch *gdbarch, struct value *function, struct type *valtype, struct regcache *regcache, gdb_byte *readbuf, const gdb_byte *writebuf);
 extern void set_gdbarch_return_value (struct gdbarch *gdbarch, gdbarch_return_value_ftype *return_value);
 
+/* Return the address at which the value being returned from
+   the current function will be stored.  This routine is only
+   called if the current function uses the the "struct return
+   convention".
+
+   May return 0 when unable to determine that address. */
+
+typedef CORE_ADDR (gdbarch_get_return_buf_addr_ftype) (struct type *val_type, frame_info_ptr cur_frame);
+extern CORE_ADDR gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info_ptr cur_frame);
+extern void set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, gdbarch_get_return_buf_addr_ftype *get_return_buf_addr);
+
 /* Return true if the return value of function is stored in the first hidden
    parameter.  In theory, this feature should be language-dependent, specified
    by language and its ABI, such as C++.  Unfortunately, compiler may
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index 9d929da6da9..3227e945880 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -116,6 +116,7 @@ struct gdbarch
   gdbarch_address_to_pointer_ftype *address_to_pointer = unsigned_address_to_pointer;
   gdbarch_integer_to_address_ftype *integer_to_address = nullptr;
   gdbarch_return_value_ftype *return_value = nullptr;
+  gdbarch_get_return_buf_addr_ftype *get_return_buf_addr = default_get_return_buf_addr;
   gdbarch_return_in_first_hidden_param_p_ftype *return_in_first_hidden_param_p = default_return_in_first_hidden_param_p;
   gdbarch_skip_prologue_ftype *skip_prologue = 0;
   gdbarch_skip_main_prologue_ftype *skip_main_prologue = nullptr;
@@ -369,6 +370,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of address_to_pointer, invalid_p == 0 */
   /* Skip verify of integer_to_address, has predicate.  */
   /* Skip verify of return_value, has predicate.  */
+  /* Skip verify of get_return_buf_addr, invalid_p == 0 */
   /* Skip verify of return_in_first_hidden_param_p, invalid_p == 0 */
   if (gdbarch->skip_prologue == 0)
     log.puts ("\n\tskip_prologue");
@@ -780,6 +782,9 @@ gdbarch_dump (struct gdbarch *gdbarch, struct ui_file *file)
   gdb_printf (file,
                       "gdbarch_dump: return_value = <%s>\n",
                       host_address_to_string (gdbarch->return_value));
+  gdb_printf (file,
+                      "gdbarch_dump: get_return_buf_addr = <%s>\n",
+                      host_address_to_string (gdbarch->get_return_buf_addr));
   gdb_printf (file,
                       "gdbarch_dump: return_in_first_hidden_param_p = <%s>\n",
                       host_address_to_string (gdbarch->return_in_first_hidden_param_p));
@@ -2587,6 +2592,23 @@ set_gdbarch_return_value (struct gdbarch *gdbarch,
   gdbarch->return_value = return_value;
 }
 
+CORE_ADDR
+gdbarch_get_return_buf_addr (struct gdbarch *gdbarch, struct type *val_type, frame_info_ptr cur_frame)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->get_return_buf_addr != NULL);
+  if (gdbarch_debug >= 2)
+    gdb_printf (gdb_stdlog, "gdbarch_get_return_buf_addr called\n");
+  return gdbarch->get_return_buf_addr (val_type, cur_frame);
+}
+
+void
+set_gdbarch_get_return_buf_addr (struct gdbarch *gdbarch,
+                                 gdbarch_get_return_buf_addr_ftype get_return_buf_addr)
+{
+  gdbarch->get_return_buf_addr = get_return_buf_addr;
+}
+
 int
 gdbarch_return_in_first_hidden_param_p (struct gdbarch *gdbarch, struct type *type)
 {
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index c03ca103c91..ef00504969f 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -55,6 +55,7 @@
 #include "gdbsupport/gdb_optional.h"
 #include "source.h"
 #include "cli/cli-style.h"
+#include "dwarf2/loc.h"
 
 /* Local functions: */
 
@@ -1598,6 +1599,12 @@ struct finish_command_fsm : public thread_fsm
      return value.  */
   struct return_value_info return_value_info {};
 
+  /* If the current function uses the "struct return convention",
+     this holds the address at which the value being returned will
+     be stored, or zero if that address could not be determined or
+     the "struct return convention" is not being used.  */
+  CORE_ADDR return_buf;
+
   explicit finish_command_fsm (struct interp *cmd_interp)
     : thread_fsm (cmd_interp)
   {
@@ -1635,7 +1642,13 @@ finish_command_fsm::should_stop (struct thread_info *tp)
 	  struct value *func;
 
 	  func = read_var_value (function, NULL, get_current_frame ());
-	  rv->value = get_return_value (function, func);
+
+	  if (return_buf != 0)
+	    /* Retrieve return value from the buffer where it was saved.  */
+	      rv->value = value_at (rv->type, return_buf);
+	  else
+	      rv->value = get_return_value (function, func);
+
 	  if (rv->value != NULL)
 	    rv->value_history_index = record_latest_value (rv->value);
 	}
@@ -1851,8 +1864,28 @@ finish_command (const char *arg, int from_tty)
     }
 
   /* Find the function we will return from.  */
-
-  sm->function = find_pc_function (get_frame_pc (get_selected_frame (NULL)));
+  frame_info_ptr callee_frame = get_selected_frame (NULL);
+  sm->function = find_pc_function (get_frame_pc (callee_frame));
+
+  /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
+     attempt to determine the address of the return buffer.  */
+  enum return_value_convention return_value;
+  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
+
+  struct type * val_type
+    = check_typedef (sm->function->type ()->target_type ());
+
+  return_value = gdbarch_return_value (gdbarch,
+				       read_var_value (sm->function, NULL,
+						       callee_frame),
+				       val_type, NULL, NULL, NULL);
+
+  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
+      && val_type->code () != TYPE_CODE_VOID)
+    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
+						  callee_frame);
+  else
+    sm->return_buf = 0;
 
   /* Print info on the selected frame, including level number but not
      source.  */
@@ -1870,7 +1903,7 @@ finish_command (const char *arg, int from_tty)
 	  gdb_printf (_("Run till exit from "));
 	}
 
-      print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
+      print_stack_frame (callee_frame, 1, LOCATION, 0);
     }
   frame.reinflate ();
 
diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index 450162dd46e..1cbaaf2a4e6 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -28,6 +28,7 @@
 #include "objfiles.h"
 #include "infcall.h"
 #include "dwarf2.h"
+#include "dwarf2/loc.h"
 #include "target-float.h"
 #include <algorithm>
 
@@ -2156,3 +2157,43 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
   return RETURN_VALUE_STRUCT_CONVENTION;
 }
 
+CORE_ADDR ppc64_sysv_get_return_buf_addr (struct type *val_type,
+					  frame_info_ptr cur_frame)
+{
+  /* The PowerPC ABI specifies aggregates that are not returned by value
+     are returned in a storage buffer provided by the caller.  The
+     address of the storage buffer is provided as a hidden first input
+     arguement in register r3.  The PowerPC ABI does not guarantee that
+     register r3 will not be changed while executing the function.  Hence, it
+     cannot be assumed that r3 will still contain the address of the storage
+     buffer when execution reaches the end of the function.
+
+     This function attempts to determine the value of r3 on entry to the
+     function using the DW_OP_entry_value DWARF entries.  This requires
+     compiling the user program with -fvar-tracking to resolve the
+     DW_TAG_call_sites in the binary file.  */
+
+  union call_site_parameter_u kind_u;
+  enum call_site_parameter_kind kind;
+  CORE_ADDR return_val = 0;
+
+  kind_u.dwarf_reg = 3;  /* First passed arg/return value is in r3.  */
+  kind = CALL_SITE_PARAMETER_DWARF_REG;
+
+  /* val_type is the type of the return value.  Need the pointer type
+     to the return value.  */
+  val_type = lookup_pointer_type (val_type);
+
+  try
+    {
+      return_val = value_as_address (value_of_dwarf_reg_entry (val_type,
+							       cur_frame,
+							       kind, kind_u));
+    }
+  catch (const gdb_exception_error &e)
+    {
+      warning ("Cannot determine the function return value.\n"
+	       "Try compiling with -fvar-tracking.");
+    }
+  return return_val;
+}
diff --git a/gdb/ppc-tdep.h b/gdb/ppc-tdep.h
index 34b250849b9..cbccd87820d 100644
--- a/gdb/ppc-tdep.h
+++ b/gdb/ppc-tdep.h
@@ -175,6 +175,8 @@ extern void ppc_collect_vsxregset (const struct regset *regset,
 				  const struct regcache *regcache,
 				  int regnum, void *vsxregs, size_t len);
 
+extern CORE_ADDR ppc64_sysv_get_return_buf_addr (type*, frame_info_ptr);
+
 /* Private data that this module attaches to struct gdbarch.  */
 
 /* ELF ABI version used by the inferior.  */
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 866d43ded7a..cbd84514795 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -8243,7 +8243,11 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_ps_regnum (gdbarch, tdep->ppc_ps_regnum);
 
   if (wordsize == 8)
-    set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
+    {
+      set_gdbarch_return_value (gdbarch, ppc64_sysv_abi_return_value);
+      set_gdbarch_get_return_buf_addr (gdbarch,
+				       ppc64_sysv_get_return_buf_addr);
+    }
   else
     set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
 
diff --git a/gdb/testsuite/gdb.cp/non-trivial-retval.exp b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
index 21c390bc937..93a3a6832f7 100644
--- a/gdb/testsuite/gdb.cp/non-trivial-retval.exp
+++ b/gdb/testsuite/gdb.cp/non-trivial-retval.exp
@@ -15,11 +15,18 @@
 
 # This file is part of the gdb testsuite
 
+set additional_flags ""
+
 if {[skip_cplus_tests]} { continue }
 
 standard_testfile .cc
 
-if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug c++}]} {
+if {[have_fvar_tracking]} {
+    set additional_flags "additional_flags= -fvar-tracking"
+}
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile [list debug c++ $additional_flags]]} {
+
     return -1
 }
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index e2cda30b95a..ed70c08d1f2 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -8796,6 +8796,14 @@ gdb_caching_proc have_fuse_ld_gold {
     return [gdb_simple_compile $me $src executable $flags]
 }
 
+# Return 1 if compiler supports fvar-tracking, otherwise return 0.
+gdb_caching_proc have_fvar_tracking {
+    set me "have_fvar_tracking"
+    set flags "additional_flags=-fvar-tracking"
+    set src { int main() { return 0; } }
+    return [gdb_simple_compile $me $src executable $flags]
+}
+
 # Return 1 if linker supports -Ttext-segment, otherwise return 0.
 gdb_caching_proc linker_supports_Ttext_segment_flag {
     set me "linker_supports_Ttext_segment_flag"
-- 
2.37.2

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Ulrich Weigand]

Carl Love <cel@us.ibm.com> wrote:

>PowerPC, fix support for printing the function return value for non-trivial values.
>
>Currently, a non-trivial return value from a function cannot currently be
>reliably determined on PowerPC.  This is due to the fact that the PowerPC
>ABI uses register r3 to store the address of the buffer containing the
>non-trivial return value when the function is called.  The PowerPC ABI
>does not guarantee the value in register r3 is not modified in the
>function.  Thus the value in r3 cannot be reliably used to obtain the
>return addreses on exit from the function.
>
>This patch adds a new gdbarch method to allow PowerPC to access the value
>of r3 on entry to a function. On PowerPC, the new gdbarch method attempts
>to use the DW_OP_entry_value for the DWARF entries, when exiting the
>function, to determine the value of r3 on entry to the function.  This
>requires the use of the -fvar-tracking compiler option to compile the
>user application thus generating the DW_OP_entry_value in the binary.  The
>DW_OP_entry_value entries in the binary file allows GDB to resolve the
>DW_TAG_call_site entries.  This new gdbarch method is used to get the
>return buffer address, in the case of a function returning a nontrivial
>data type, on exit from the function.  The GDB function should_stop checks
>to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set to
>zero by the new gdbarch method call for all architectures except PowerPC.
>The get_return_value function will be used to obtain the return value on
>all other architectures as is currently being done if RETURN_BUF is zero.
>On PowerPC, the new gdbarch method will return a nonzero address in
>RETURN_BUF if the value can be determined.  The value_at function uses the
>return buffer address to get the return value.
>
>This patch fixes five testcase failures in gdb.cp/non-trivial-retval.exp.
>The correct function return values are now reported.
>
>Note this patch is dependent on patch: "PowerPC, function
>ppc64_sysv_abi_return_value add missing return value convention".
>
>This patch has been tested on Power 10 and x86-64 with no regressions.

I believe all of Kevin's and Bruno's comments have been addressed
in this version.  I've reviewed it myself as well, and it looks
good to me.

This is OK.

Thanks,
Ulrich

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Tom de Vries]

On 11/14/22 17:47, Ulrich Weigand via Gdb-patches wrote:
> Carl Love <cel@us.ibm.com> wrote:
> 
>> PowerPC, fix support for printing the function return value for non-trivial values.
>>
>> Currently, a non-trivial return value from a function cannot currently be
>> reliably determined on PowerPC.  This is due to the fact that the PowerPC
>> ABI uses register r3 to store the address of the buffer containing the
>> non-trivial return value when the function is called.  The PowerPC ABI
>> does not guarantee the value in register r3 is not modified in the
>> function.  Thus the value in r3 cannot be reliably used to obtain the
>> return addreses on exit from the function.
>>
>> This patch adds a new gdbarch method to allow PowerPC to access the value
>> of r3 on entry to a function. On PowerPC, the new gdbarch method attempts
>> to use the DW_OP_entry_value for the DWARF entries, when exiting the
>> function, to determine the value of r3 on entry to the function.  This
>> requires the use of the -fvar-tracking compiler option to compile the
>> user application thus generating the DW_OP_entry_value in the binary.  The
>> DW_OP_entry_value entries in the binary file allows GDB to resolve the
>> DW_TAG_call_site entries.  This new gdbarch method is used to get the
>> return buffer address, in the case of a function returning a nontrivial
>> data type, on exit from the function.  The GDB function should_stop checks
>> to see if RETURN_BUF is non-zero.  By default, RETURN_BUF will be set to
>> zero by the new gdbarch method call for all architectures except PowerPC.
>> The get_return_value function will be used to obtain the return value on
>> all other architectures as is currently being done if RETURN_BUF is zero.
>> On PowerPC, the new gdbarch method will return a nonzero address in
>> RETURN_BUF if the value can be determined.  The value_at function uses the
>> return buffer address to get the return value.
>>
>> This patch fixes five testcase failures in gdb.cp/non-trivial-retval.exp.
>> The correct function return values are now reported.
>>
>> Note this patch is dependent on patch: "PowerPC, function
>> ppc64_sysv_abi_return_value add missing return value convention".
>>
>> This patch has been tested on Power 10 and x86-64 with no regressions.
> 
> I believe all of Kevin's and Bruno's comments have been addressed
> in this version.  I've reviewed it myself as well, and it looks
> good to me.
> 
> This is OK.
> 

On x86_64-linux, I run into a segfault:
...
PASS: gdb.asm/asm-source.exp: info source asmsrc1.s
ERROR: GDB process no longer exists
UNRESOLVED: gdb.asm/asm-source.exp: finish from foo3
...

Reproduced on command line:
...
$ gdb -q -batch -x outputs/gdb.asm/asm-source/gdb.in.1
...

The problem seems to be that:
...
Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
0x000000000043de7a in symbol::type (this=0x0) at 
/home/vries/gdb_versions/devel/src/gdb/symtab.h:1287
1287        return m_type;
...
because:
...
(gdb) up
#1  0x0000000000852d94 in finish_command (arg=0x0, from_tty=0)
     at /home/vries/gdb_versions/devel/src/gdb/infcmd.c:1887
1887        = check_typedef (sm->function->type ()->target_type ());
(gdb) p sm->function
$1 = (symbol *) 0x0
...

Thanks,
- Tom

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Ulrich Weigand]

Tom de Vries <tdevries@suse.de> wrote:
>On 11/14/22 17:47, Ulrich Weigand via Gdb-patches wrote:
> >I believe all of Kevin's and Bruno's comments have been addressed
> >in this version.  I've reviewed it myself as well, and it looks
> >good to me.
> 
> >This is OK.

>On x86_64-linux, I run into a segfault:
>[...]
>(gdb) up
>#1  0x0000000000852d94 in finish_command (arg=0x0, from_tty=0)
>     at /home/vries/gdb_versions/devel/src/gdb/infcmd.c:1887
>1887        = check_typedef (sm->function->type ()->target_type ());
>(gdb) p sm->function
>$1 = (symbol *) 0x0

Ah, I missed that possibility, sorry.

Carl, if sm->function is NULL, then the whole check needs
to be skipped, and sm->return_buf should be set to 0.

Can you come up with a fix along those lines (or else,
just revert the patch for now)?   Thanks!

Bye,
Ulrich

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

On Tue, 2022-11-15 at 10:16 +0000, Ulrich Weigand wrote:
> Tom de Vries <tdevries@suse.de> wrote:
> > On 11/14/22 17:47, Ulrich Weigand via Gdb-patches wrote:
> > > I believe all of Kevin's and Bruno's comments have been addressed
> > > in this version.  I've reviewed it myself as well, and it looks
> > > good to me.
> > > This is OK.
> > On x86_64-linux, I run into a segfault:
> > [...]
> > (gdb) up
> > #1  0x0000000000852d94 in finish_command (arg=0x0, from_tty=0)
> >     at /home/vries/gdb_versions/devel/src/gdb/infcmd.c:1887
> > 1887        = check_typedef (sm->function->type ()->target_type
> > ());
> > (gdb) p sm->function
> > $1 = (symbol *) 0x0
> 
> Ah, I missed that possibility, sorry.
> 
> Carl, if sm->function is NULL, then the whole check needs
> to be skipped, and sm->return_buf should be set to 0.
> 
> Can you come up with a fix along those lines (or else,
> just revert the patch for now)?   Thanks!
> 
Yes, I will take a look and see about fixing or reverting it ASAP.

                 Carl 

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Simon Marchi]

On 11/15/22 11:04, Carl Love via Gdb-patches wrote:
> On Tue, 2022-11-15 at 10:16 +0000, Ulrich Weigand wrote:
>> Tom de Vries <tdevries@suse.de> wrote:
>>> On 11/14/22 17:47, Ulrich Weigand via Gdb-patches wrote:
>>>> I believe all of Kevin's and Bruno's comments have been addressed
>>>> in this version.  I've reviewed it myself as well, and it looks
>>>> good to me.
>>>> This is OK.
>>> On x86_64-linux, I run into a segfault:
>>> [...]
>>> (gdb) up
>>> #1  0x0000000000852d94 in finish_command (arg=0x0, from_tty=0)
>>>     at /home/vries/gdb_versions/devel/src/gdb/infcmd.c:1887
>>> 1887        = check_typedef (sm->function->type ()->target_type
>>> ());
>>> (gdb) p sm->function
>>> $1 = (symbol *) 0x0
>>
>> Ah, I missed that possibility, sorry.
>>
>> Carl, if sm->function is NULL, then the whole check needs
>> to be skipped, and sm->return_buf should be set to 0.
>>
>> Can you come up with a fix along those lines (or else,
>> just revert the patch for now)?   Thanks!
>>
> Yes, I will take a look and see about fixing or reverting it ASAP.
> 
>                  Carl 
> 

Just so you can test properly, I also see these failures that are likely
due to the same bug:

UNRESOLVED: gdb.base/foll-vfork.exp: exec: vfork parent follow, finish after tcatch vfork: finish
UNRESOLVED: gdb.base/foll-vfork.exp: exec: vfork child follow, finish after tcatch vfork: finish
UNRESOLVED: gdb.base/foll-vfork.exp: exit: vfork parent follow, finish after tcatch vfork: finish
UNRESOLVED: gdb.base/foll-vfork.exp: exit: vfork child follow, finish after tcatch vfork: finish
UNRESOLVED: gdb.base/sigaltstack.exp: finish to throw INNER
UNRESOLVED: gdb.base/sigstep.exp: finish from handleri: leave signal trampoline

Simon

RE: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

Ulrich, Simon, Tom, GDB maintainers:

> 
> Just so you can test properly, I also see these failures that are
> likely
> due to the same bug:
> 
> UNRESOLVED: gdb.base/foll-vfork.exp: exec: vfork parent follow,
> finish after tcatch vfork: finish
> UNRESOLVED: gdb.base/foll-vfork.exp: exec: vfork child follow, finish
> after tcatch vfork: finish
> UNRESOLVED: gdb.base/foll-vfork.exp: exit: vfork parent follow,
> finish after tcatch vfork: finish
> UNRESOLVED: gdb.base/foll-vfork.exp: exit: vfork child follow, finish
> after tcatch vfork: finish
> UNRESOLVED: gdb.base/sigaltstack.exp: finish to throw INNER
> UNRESOLVED: gdb.base/sigstep.exp: finish from handleri: leave signal
> trampoline

I have dug into why I didn't catch the issue earlier.  Looking
carefully at the regression test output, the foll-vfork.exp,
sigaltstack.exp, sigstep.exp and asm-source.exp tests are all failing
similarly.  In the GDB regression test output I see:

Running /home/carll/GDB/build-non-trivial/gdb/testsuite/../../../binutils-gdb-n\
on-trivial/gdb/testsuite/gdb.base/foll-vfork.exp ...
ERROR: GDB process no longer exists
ERROR: GDB process no longer exists
ERROR: GDB process no longer exists
ERROR: GDB process no longer exists

Running .../binutils-gdb-non-trivial/gdb/testsuite/gdb.asm/asm-source.exp ...
ERROR: GDB process no longer exists

Running ../binutils-gdb-non-trivial/gdb/testsuite/gdb.base/sigaltstack.exp ...
ERROR: GDB process no longer exists
ERROR: Couldn't send finish to GDB.
ERROR: can not find channel named "exp17"

Running .../binutils-gdb-non-trivial/gdb/testsuite/gdb.base/sigstep.exp ...
ERROR: GDB process no longer exists

The failures then end up being recorded in the summary as "# of
unresolved testcases" not as "# of unexpected failures".  I can see
with the current mainline code base there are 8 unresolved  testcases. 
I updated the fix that I sent to Tom earlier per the comments from
Ulrich.  With that fix applied, the number of unresolved test cases
drops to 1.  If I run the regression tests with the tree backed up the
the commit just before my commits from Monday, I see one unresolved
test case.  So, clearly my commit added 7 unresolved test cases, the
six that Simon mentioned above and the one from Tom.  The original
unresolved test case is:

Running .../binutils-gdb-non-trivial/gdb/testsuite/gdb.base/gdb-sigterm.exp ...
FAIL: gdb.base/gdb-sigterm.exp: pass=0: expect eof (GDB internal error)
ERROR: Could not resync from internal error (recursive internal problem)

I always check that "# of unexpected failures" did not change with my
patch applied.  I didn't notice that the number of unresolved testcases
changed and hence missed the breakage my patch caused.  My bad.  I will
be sure to watch the other stats in the gdb summary a lot closer in the
future.

Thanks for pointing out the failures and the for help resolving the
issue. 

I will post my updated fix for Tom to test/review.  I think we have
this all figured out and fixed.  Sorry for breaking things.

                      Carl Love

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

Tom, Ulrich:

On Tue, 2022-11-15 at 10:16 +0000, Ulrich Weigand wrote:
> Tom de Vries <tdevries@suse.de> wrote:
> > On 11/14/22 17:47, Ulrich Weigand via Gdb-patches wrote:
> > > I believe all of Kevin's and Bruno's comments have been addressed
> > > in this version.  I've reviewed it myself as well, and it looks
> > > good to me.
> > > This is OK.
> > On x86_64-linux, I run into a segfault:
> > [...]
> > (gdb) up
> > #1  0x0000000000852d94 in finish_command (arg=0x0, from_tty=0)
> >     at /home/vries/gdb_versions/devel/src/gdb/infcmd.c:1887
> > 1887        = check_typedef (sm->function->type ()->target_type
> > ());
> > (gdb) p sm->function
> > $1 = (symbol *) 0x0
> 
> Ah, I missed that possibility, sorry.
> 
> Carl, if sm->function is NULL, then the whole check needs
> to be skipped, and sm->return_buf should be set to 0.
> 
> Can you come up with a fix along those lines (or else,
> just revert the patch for now)?   Thanks!
> 

I tested the last mainline code with the command on an X86-64 box.

   make check RUNTESTFLAGS='GDB=/home/carll/bin/gdb  gdb.asm/asm-source.exp  '

Which generated an error.  I then ran the command, from Tom's email:

 gdb -q -batch -x outputs/gdb.asm/asm-source/gdb.in.1

from directory gdb/testsuite which generated a segmentation fault as
expected.

Once I applied the following patch, recompiled and retested, the make
check seemed to run fine and the gdb -q batch test ran without
generating a segmentation fault.

The patch appears to fix the issue.  Not sure why this didn't show up
in my original X86 testing?  I will go back and look some more to see
if I can track down why I didn't see the issue.

Sorry for the bug.  

Tom, can you take a look at the patch and try it out to make sure it
fixes the issues on your system.  Thanks.

                  Carl Love

----------------------------------------------------------------
Bug fix in commit for printing the function return value for non-trivial values

The recent commit:

  commit a0eda3df5b750ae32576a9be092b361281a41787
  Author: Carl Love <cel@us.ibm.com>
  Date:   Mon Nov 14 16:22:37 2022 -0500

    PowerPC, fix support for printing the function return value for non-trivial values.

Is generating a segmentation fault on x86_64-linux.

  segfault:
  ...
  PASS: gdb.asm/asm-source.exp: info source asmsrc1.s
  ERROR: GDB process no longer exists
  UNRESOLVED: gdb.asm/asm-source.exp: finish from foo3
  ...

  Reproduced on command line:
  ...
  $ gdb -q -batch -x outputs/gdb.asm/asm-source/gdb.in.1
  ...

  The problem seems to be that:
  ...
  Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
  0x000000000043de7a in symbol::type (this=0x0) at
  .../gdb_versions/devel/src/gdb/symtab.h:1287
  1287        return m_type;
  ...
  because:
  ...
  (gdb) up
  #1  0x0000000000852d94 in finish_command (arg=0x0, from_tty=0)
     at .../gdb_versions/devel/src/gdb/infcmd.c:1887
  1887        = check_typedef (sm->function->type ()->target_type ());
  (gdb) p sm->function
  $1 = (symbol *) 0x0

The code is not checking if sm->function is NULL.  If sm->function is NULL
the check for the return buffer should be skipped.
---
 gdb/infcmd.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b71dc10370b..2876f6d9c9d 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1880,23 +1880,28 @@ finish_command (const char *arg, int from_tty)
 
   /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
      attempt to determine the address of the return buffer.  */
-  enum return_value_convention return_value;
-  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
+  if (sm->function != NULL)
+    {
+      enum return_value_convention return_value;
+      struct gdbarch *gdbarch = get_frame_arch (callee_frame);
 
-  struct type * val_type
-    = check_typedef (sm->function->type ()->target_type ());
+      struct type * val_type
+	= check_typedef (sm->function->type ()->target_type ());
 
-  return_value = gdbarch_return_value (gdbarch,
-				       read_var_value (sm->function, NULL,
-						       callee_frame),
-				       val_type, NULL, NULL, NULL);
+      return_value = gdbarch_return_value (gdbarch,
+					   read_var_value (sm->function, NULL,
+							   callee_frame),
+					   val_type, NULL, NULL, NULL);
 
-  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
-      && val_type->code () != TYPE_CODE_VOID)
-    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
-						  callee_frame);
+      if (return_value == RETURN_VALUE_STRUCT_CONVENTION
+	  && val_type->code () != TYPE_CODE_VOID)
+	sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
+						      callee_frame);
+      else
+	sm->return_buf = 0;
+    }
   else
-    sm->return_buf = 0;
+    sm->return_buf = 0;  /* Return buffer address is not available.  */
 
   /* Print info on the selected frame, including level number but not
      source.  */
-- 
2.31.1

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Ulrich Weigand]

Carl Love <cel@us.ibm.com> wrote:

>+      else
>+	sm->return_buf = 0;
>+    }
>   else
>-    sm->return_buf = 0;
>+    sm->return_buf = 0;  /* Return buffer address is not available.  */

Just as a minor nit, it might be cleaner to initialize
sm->return_buf to 0 just once up-front.

Patch is OK otherwise.


Thanks,
Ulrich

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

Ulrich, Tom, Simon:

On Tue, 2022-11-15 at 18:05 +0000, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
> 
> > +      else
> > +	sm->return_buf = 0;
> > +    }
> >   else
> > -    sm->return_buf = 0;
> > +    sm->return_buf = 0;  /* Return buffer address is not
> > available.  */
> 
> Just as a minor nit, it might be cleaner to initialize
> sm->return_buf to 0 just once up-front.
> 
> Patch is OK otherwise.
> 
> 
> Thanks,
> Ulrich

Yes, I agree, it would be best to just set sm->return_buf = 0
initially.  Then if the return buffer is available update it as needed.
That makes things a lot cleaner.

The updated patch is below.  Note, it has been tested on PowerPC and
X86_64.  It does not introduce any additional regression errors.  It
does fix the seven unresolved testcases that the initial patch
introduced and were missed by my testing of the initial patch. 

Tom, if you can verify this fix works on your system it would be
greatly appreciated.  Thanks.

                        Carl Love
---------------------------------------------------
Bug fix in commit for printing the function return value for non-trivial values

The recent commit:

  commit a0eda3df5b750ae32576a9be092b361281a41787
  Author: Carl Love <cel@us.ibm.com>
  Date:   Mon Nov 14 16:22:37 2022 -0500

    PowerPC, fix support for printing the function return value for non-trivial values.

Is generating a segmentation fault on x86_64-linux.

  segfault:
  ...
  PASS: gdb.asm/asm-source.exp: info source asmsrc1.s
  ERROR: GDB process no longer exists
  UNRESOLVED: gdb.asm/asm-source.exp: finish from foo3
  ...

  Reproduced on command line:
  ...
  $ gdb -q -batch -x outputs/gdb.asm/asm-source/gdb.in.1
  ...

  The problem seems to be that:
  ...
  Thread 1 "gdb" received signal SIGSEGV, Segmentation fault.
  0x000000000043de7a in symbol::type (this=0x0) at
  .../gdb_versions/devel/src/gdb/symtab.h:1287
  1287        return m_type;
  ...
  because:
  ...
  (gdb) up
  #1  0x0000000000852d94 in finish_command (arg=0x0, from_tty=0)
     at .../gdb_versions/devel/src/gdb/infcmd.c:1887
  1887        = check_typedef (sm->function->type ()->target_type ());
  (gdb) p sm->function
  $1 = (symbol *) 0x0

The code is not checking if sm->function is NULL.  If sm->function is NULL
the check for the return buffer should be skipped.
---
 gdb/infcmd.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b71dc10370b..a72df2d6a01 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -1877,26 +1877,28 @@ finish_command (const char *arg, int from_tty)
   /* Find the function we will return from.  */
   frame_info_ptr callee_frame = get_selected_frame (NULL);
   sm->function = find_pc_function (get_frame_pc (callee_frame));
+  sm->return_buf = 0;    /* Initialize buffer address is not available.  */
 
   /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
      attempt to determine the address of the return buffer.  */
-  enum return_value_convention return_value;
-  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
+  if (sm->function != NULL)
+    {
+      enum return_value_convention return_value;
+      struct gdbarch *gdbarch = get_frame_arch (callee_frame);
 
-  struct type * val_type
-    = check_typedef (sm->function->type ()->target_type ());
+      struct type * val_type
+	= check_typedef (sm->function->type ()->target_type ());
 
-  return_value = gdbarch_return_value (gdbarch,
-				       read_var_value (sm->function, NULL,
-						       callee_frame),
-				       val_type, NULL, NULL, NULL);
+      return_value = gdbarch_return_value (gdbarch,
+					   read_var_value (sm->function, NULL,
+							   callee_frame),
+					   val_type, NULL, NULL, NULL);
 
-  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
-      && val_type->code () != TYPE_CODE_VOID)
-    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
-						  callee_frame);
-  else
-    sm->return_buf = 0;
+      if (return_value == RETURN_VALUE_STRUCT_CONVENTION
+	  && val_type->code () != TYPE_CODE_VOID)
+	sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
+						      callee_frame);
+    }
 
   /* Print info on the selected frame, including level number but not
      source.  */
-- 
2.31.1

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Ulrich Weigand]

Carl Love <cel@us.ibm.com> wrote:

>Bug fix in commit for printing the function return value for non-trivial values

This is OK.

Thanks,
Ulrich

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Tom de Vries]

On 11/16/22 02:01, Carl Love wrote:
> Tom, if you can verify this fix works on your system it would be
> greatly appreciated.  Thanks.
> 

Hi Carl,

it works, all fails are gone, I got my usual result again on openSUSE 
Leap 15.4 x86_64:
...
                 === gdb Summary ===

# of unexpected core files      1
# of expected passes            117502
# of expected failures          150
# of known failures             89
# of untested testcases         26
# of unsupported tests          107
...

Thanks,
- Tom

Re: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Lancelot SIX]

Hi Carl,

Thanks for the fix.

> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index b71dc10370b..a72df2d6a01 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -1877,26 +1877,28 @@ finish_command (const char *arg, int from_tty)
>    /* Find the function we will return from.  */
>    frame_info_ptr callee_frame = get_selected_frame (NULL);
>    sm->function = find_pc_function (get_frame_pc (callee_frame));
> +  sm->return_buf = 0;    /* Initialize buffer address is not available.  */
>  
>    /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
>       attempt to determine the address of the return buffer.  */
> -  enum return_value_convention return_value;
> -  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
> +  if (sm->function != NULL)

I know that Ulrich has already approved the patch, but to follow the
coding standard the NULL should be replaced by nullptr.  If this is not
too much trouble and is OK with maintainers, I think it would be nice to
do this minor tweak.

> +    {
> +      enum return_value_convention return_value;
> +      struct gdbarch *gdbarch = get_frame_arch (callee_frame);
>  
> -  struct type * val_type
> -    = check_typedef (sm->function->type ()->target_type ());
> +      struct type * val_type
> +	= check_typedef (sm->function->type ()->target_type ());
>  
> -  return_value = gdbarch_return_value (gdbarch,
> -				       read_var_value (sm->function, NULL,
> -						       callee_frame),
> -				       val_type, NULL, NULL, NULL);
> +      return_value = gdbarch_return_value (gdbarch,
> +					   read_var_value (sm->function, NULL,
> +							   callee_frame),
> +					   val_type, NULL, NULL, NULL);

While at it, those NULL could probably be updated I guess.

Best,
Lancelot.

>  
> -  if (return_value == RETURN_VALUE_STRUCT_CONVENTION
> -      && val_type->code () != TYPE_CODE_VOID)
> -    sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
> -						  callee_frame);
> -  else
> -    sm->return_buf = 0;
> +      if (return_value == RETURN_VALUE_STRUCT_CONVENTION
> +	  && val_type->code () != TYPE_CODE_VOID)
> +	sm->return_buf = gdbarch_get_return_buf_addr (gdbarch, val_type,
> +						      callee_frame);
> +    }
>  
>    /* Print info on the selected frame, including level number but not
>       source.  */
> -- 
> 2.31.1
> 
> 

RE: [PATCH 2/2 ver 3] PowerPC, fix support for printing the function return value for non-trivial values.

[Carl Love]

On Wed, 2022-11-16 at 10:20 +0000, Lancelot SIX wrote:
> Hi Carl,
> 
<snip>
> > s RETURN_VALUE_STRUCT_CONVENTION,
> >       attempt to determine the address of the return buffer.  */
> > -  enum return_value_convention return_value;
> > -  struct gdbarch *gdbarch = get_frame_arch (callee_frame);
> > +  if (sm->function != NULL)
> 
> I know that Ulrich has already approved the patch, but to follow the
> coding standard the NULL should be replaced by nullptr.  If this is
> not
> too much trouble and is OK with maintainers, I think it would be nice
> to
> do this minor tweak.
> 
> > +    {
> > +      enum return_value_convention return_value;
> > +      struct gdbarch *gdbarch = get_frame_arch (callee_frame);
> >  
> > -  struct type * val_type
> > -    = check_typedef (sm->function->type ()->target_type ());
> > +      struct type * val_type
> > +	= check_typedef (sm->function->type ()->target_type ());
> >  
> > -  return_value = gdbarch_return_value (gdbarch,
> > -				       read_var_value (sm->function,
> > NULL,
> > -						       callee_frame),
> > -				       val_type, NULL, NULL, NULL);
> > +      return_value = gdbarch_return_value (gdbarch,
> > +					   read_var_value (sm-
> > >function, NULL,
> > +							   callee_frame
> > ),
> > +					   val_type, NULL, NULL, NULL);
> 
> While at it, those NULL could probably be updated I guess.

I think at this point it is probably best to do a new clean up patch
particularly since the other NULLs are not part of the fix patch.

I will go ahead and commit the fix and then put together a clean up
patch to address all the NULLs.   Thanks for letting me know about it.

                   Carl Love

[PATCH] Change NULL to nullptr in gdb/infcmd.c and gdb/infrun.c

[Carl Love]

Lancelot, GDB maintainers:

The GDB coding style guide specifies that nullptr should be used
instead of NULL as noted by Lancelot for the recent patch "PowerPC, fix
support for printing the function return value for non-trivial
values.".  This patch changes all of the various NULL statements to
nullptr statements in files gdb/infcmd.c and gdb/infrun.c per the
coding style guide.

The patch does not make any functional changes to the code.  

The patch has been tested on both X86_64 and PowerPC to ensure there
were no new unexpected error, new core files generated, new unresolved
tests etc.  

Please let me know if this patch is acceptable.  Thanks. 

                         Carl Love


------------------------
Change NULL to nullptr in gdb/infcmd.c and gdb/infrun.c

The GDB coding standard specifies that nullptr should be used instead of NULL.
There are numerous uses of NULL and nullptr in files infcmd.c and infrun.c.
This patch replaces the various uses of NULL with nullptr in the source files.
The use of NULL in the comments was not changed.

The patch does not introduce any functional changes.

The patch has been tested on PowerPC and Intel X86_64 with no new unexpected
test failures, unresolved tests, new core files etc.
---
 gdb/infcmd.c | 122 +++++++++++++++++++-------------------
 gdb/infrun.c | 162 +++++++++++++++++++++++++--------------------------
 2 files changed, 142 insertions(+), 142 deletions(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index a72df2d6a01..f7bce0d0399 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -208,10 +208,10 @@ strip_bg_char (const char *args, int *bg_char_p)
 {
   const char *p;
 
-  if (args == NULL || *args == '\0')
+  if (args == nullptr || *args == '\0')
     {
       *bg_char_p = 0;
-      return NULL;
+      return nullptr;
     }
 
   p = args + strlen (args);
@@ -297,7 +297,7 @@ post_create_inferior (int from_tty)
 	  /* If the solist is global across processes, there's no need to
 	     refetch it here.  */
 	  if (!gdbarch_has_global_solist (target_gdbarch ()))
-	    solib_add (NULL, 0, auto_solib_add);
+	    solib_add (nullptr, 0, auto_solib_add);
 	}
     }
 
@@ -450,12 +450,12 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
      want them to go away (PR 2207).  This is probably reasonable.  */
 
   /* If there were other args, beside '&', process them.  */
-  if (args != NULL)
+  if (args != nullptr)
     current_inferior ()->set_args (args);
 
   if (from_tty)
     {
-      uiout->field_string (NULL, "Starting program");
+      uiout->field_string (nullptr, "Starting program");
       uiout->text (": ");
       if (exec_file)
 	uiout->field_string ("execfile", exec_file,
@@ -472,7 +472,7 @@ run_command_1 (const char *args, int from_tty, enum run_how run_how)
 			       from_tty);
   /* to_create_inferior should push the target, so after this point we
      shouldn't refer to run_target again.  */
-  run_target = NULL;
+  run_target = nullptr;
 
   infrun_debug_show_threads ("immediately after create_process",
 			     current_inferior ()->non_exited_threads ());
@@ -630,7 +630,7 @@ continue_1 (int all_threads)
       scoped_disable_commit_resumed disable_commit_resumed
 	("continue all threads in non-stop");
 
-      iterate_over_threads (proceed_thread_callback, NULL);
+      iterate_over_threads (proceed_thread_callback, nullptr);
 
       if (current_ui->prompt_state == PROMPT_BLOCKED)
 	{
@@ -675,27 +675,27 @@ continue_command (const char *args, int from_tty)
   gdb::unique_xmalloc_ptr<char> stripped = strip_bg_char (args, &async_exec);
   args = stripped.get ();
 
-  if (args != NULL)
+  if (args != nullptr)
     {
       if (startswith (args, "-a"))
 	{
 	  all_threads_p = true;
 	  args += sizeof ("-a") - 1;
 	  if (*args == '\0')
-	    args = NULL;
+	    args = nullptr;
 	}
     }
 
   if (!non_stop && all_threads_p)
     error (_("`-a' is meaningless in all-stop mode."));
 
-  if (args != NULL && all_threads_p)
+  if (args != nullptr && all_threads_p)
     error (_("Can't resume all threads and specify "
 	     "proceed count simultaneously."));
 
   /* If we have an argument left, set proceed count of breakpoint we
      stopped at.  */
-  if (args != NULL)
+  if (args != nullptr)
     {
       bpstat *bs = nullptr;
       int num, stat;
@@ -712,7 +712,7 @@ continue_command (const char *args, int from_tty)
 	  get_last_target_status (&last_target, &last_ptid, nullptr);
 	  tp = find_thread_ptid (last_target, last_ptid);
 	}
-      if (tp != NULL)
+      if (tp != nullptr)
 	bs = tp->control.stop_bpstat;
 
       while ((stat = bpstat_num (&bs, &num)) != 0)
@@ -964,7 +964,7 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
 	      && inline_skipped_frames (tp))
 	    {
 	      ptid_t resume_ptid;
-	      const char *fn = NULL;
+	      const char *fn = nullptr;
 	      symtab_and_line sal;
 	      struct symbol *sym;
 
@@ -978,7 +978,7 @@ prepare_one_step (thread_info *tp, struct step_command_fsm *sm)
 	      sal = find_frame_sal (frame);
 	      sym = get_frame_function (frame);
 
-	      if (sym != NULL)
+	      if (sym != nullptr)
 		fn = sym->print_name ();
 
 	      if (sal.line == 0
@@ -1094,7 +1094,7 @@ jump_command (const char *arg, int from_tty)
   /* See if we are trying to jump to another function.  */
   fn = get_frame_function (get_current_frame ());
   sfn = find_pc_function (sal.pc);
-  if (fn != NULL && sfn != fn)
+  if (fn != nullptr && sfn != fn)
     {
       if (!query (_("Line %d is not in `%s'.  Jump anyway? "), sal.line,
 		  fn->print_name ()))
@@ -1104,7 +1104,7 @@ jump_command (const char *arg, int from_tty)
 	}
     }
 
-  if (sfn != NULL)
+  if (sfn != nullptr)
     {
       struct obj_section *section;
 
@@ -1245,7 +1245,7 @@ queue_signal_command (const char *signum_exp, int from_tty)
   ensure_valid_thread ();
   ensure_not_running ();
 
-  if (signum_exp == NULL)
+  if (signum_exp == nullptr)
     error_no_arg (_("signal number"));
 
   /* It would be even slicker to make signal names be valid expressions,
@@ -1356,7 +1356,7 @@ until_next_command (int from_tty)
     {
       struct bound_minimal_symbol msymbol = lookup_minimal_symbol_by_pc (pc);
 
-      if (msymbol.minsym == NULL)
+      if (msymbol.minsym == nullptr)
 	error (_("Execution is not within a known function."));
 
       tp->control.step_range_start = msymbol.value_address ();
@@ -1456,7 +1456,7 @@ advance_command (const char *arg, int from_tty)
   ensure_valid_thread ();
   ensure_not_running ();
 
-  if (arg == NULL)
+  if (arg == nullptr)
     error_no_arg (_("a location"));
 
   /* Find out whether we must run in the background.  */
@@ -1498,17 +1498,17 @@ get_return_value (struct symbol *func_symbol, struct value *function)
      calls are made async, this will likely be made the norm.  */
 
   switch (gdbarch_return_value (gdbarch, function, value_type,
-				NULL, NULL, NULL))
+				nullptr, nullptr, nullptr))
     {
     case RETURN_VALUE_REGISTER_CONVENTION:
     case RETURN_VALUE_ABI_RETURNS_ADDRESS:
     case RETURN_VALUE_ABI_PRESERVES_ADDRESS:
       value = allocate_value (value_type);
       gdbarch_return_value (gdbarch, function, value_type, stop_regs,
-			    value_contents_raw (value).data (), NULL);
+			    value_contents_raw (value).data (), nullptr);
       break;
     case RETURN_VALUE_STRUCT_CONVENTION:
-      value = NULL;
+      value = nullptr;
       break;
     default:
       internal_error (_("bad switch"));
@@ -1539,7 +1539,7 @@ struct return_value_info
 static void
 print_return_value_1 (struct ui_out *uiout, struct return_value_info *rv)
 {
-  if (rv->value != NULL)
+  if (rv->value != nullptr)
     {
       /* Print it.  */
       uiout->text ("Value returned is ");
@@ -1578,7 +1578,7 @@ print_return_value_1 (struct ui_out *uiout, struct return_value_info *rv)
 void
 print_return_value (struct ui_out *uiout, struct return_value_info *rv)
 {
-  if (rv->type == NULL
+  if (rv->type == nullptr
       || check_typedef (rv->type)->code () == TYPE_CODE_VOID)
     return;
 
@@ -1637,22 +1637,22 @@ finish_command_fsm::should_stop (struct thread_info *tp)
 {
   struct return_value_info *rv = &return_value_info;
 
-  if (function != NULL
+  if (function != nullptr
       && bpstat_find_breakpoint (tp->control.stop_bpstat,
-				 breakpoint.get ()) != NULL)
+				 breakpoint.get ()) != nullptr)
     {
       /* We're done.  */
       set_finished ();
 
       rv->type = function->type ()->target_type ();
-      if (rv->type == NULL)
+      if (rv->type == nullptr)
 	internal_error (_("finish_command: function has no target type"));
 
       if (check_typedef (rv->type)->code () != TYPE_CODE_VOID)
 	{
 	  struct value *func;
 
-	  func = read_var_value (function, NULL, get_current_frame ());
+	  func = read_var_value (function, nullptr, get_current_frame ());
 
 	  if (return_buf != 0)
 	    /* Retrieve return value from the buffer where it was saved.  */
@@ -1660,7 +1660,7 @@ finish_command_fsm::should_stop (struct thread_info *tp)
 	  else
 	      rv->value = get_return_value (function, func);
 
-	  if (rv->value != NULL)
+	  if (rv->value != nullptr)
 	    rv->value_history_index = record_latest_value (rv->value);
 	}
     }
@@ -1717,7 +1717,7 @@ finish_backward (struct finish_command_fsm *sm)
 
   pc = get_frame_pc (get_current_frame ());
 
-  if (find_pc_partial_function (pc, NULL, &func_addr, NULL) == 0)
+  if (find_pc_partial_function (pc, nullptr, &func_addr, nullptr) == 0)
     error (_("Cannot find bounds of current function"));
 
   sal = find_pc_line (func_addr, 0);
@@ -1734,7 +1734,7 @@ finish_backward (struct finish_command_fsm *sm)
 
   if (sal.pc != pc)
     {
-      frame_info_ptr frame = get_selected_frame (NULL);
+      frame_info_ptr frame = get_selected_frame (nullptr);
       struct gdbarch *gdbarch = get_frame_arch (frame);
 
       /* Set a step-resume at the function's entry point.  Once that's
@@ -1775,7 +1775,7 @@ finish_forward (struct finish_command_fsm *sm, frame_info_ptr frame)
 					     bp_finish);
 
   /* set_momentary_breakpoint invalidates FRAME.  */
-  frame = NULL;
+  frame = nullptr;
 
   set_longjmp_breakpoint (tp, frame_id);
 
@@ -1797,11 +1797,11 @@ skip_finish_frames (frame_info_ptr frame)
       start = frame;
 
       frame = skip_tailcall_frames (frame);
-      if (frame == NULL)
+      if (frame == nullptr)
 	break;
 
       frame = skip_unwritable_frames (frame);
-      if (frame == NULL)
+      if (frame == nullptr)
 	break;
     }
   while (start != frame);
@@ -1867,7 +1867,7 @@ finish_command (const char *arg, int from_tty)
       if (from_tty)
 	{
 	  gdb_printf (_("Run till exit from "));
-	  print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0);
+	  print_stack_frame (get_selected_frame (nullptr), 1, LOCATION, 0);
 	}
 
       proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
@@ -1875,13 +1875,13 @@ finish_command (const char *arg, int from_tty)
     }
 
   /* Find the function we will return from.  */
-  frame_info_ptr callee_frame = get_selected_frame (NULL);
+  frame_info_ptr callee_frame = get_selected_frame (nullptr);
   sm->function = find_pc_function (get_frame_pc (callee_frame));
   sm->return_buf = 0;    /* Initialize buffer address is not available.  */
 
   /* Determine the return convention.  If it is RETURN_VALUE_STRUCT_CONVENTION,
      attempt to determine the address of the return buffer.  */
-  if (sm->function != NULL)
+  if (sm->function != nullptr)
     {
       enum return_value_convention return_value;
       struct gdbarch *gdbarch = get_frame_arch (callee_frame);
@@ -1890,9 +1890,9 @@ finish_command (const char *arg, int from_tty)
 	= check_typedef (sm->function->type ()->target_type ());
 
       return_value = gdbarch_return_value (gdbarch,
-					   read_var_value (sm->function, NULL,
+					   read_var_value (sm->function, nullptr,
 							   callee_frame),
-					   val_type, NULL, NULL, NULL);
+					   val_type, nullptr, nullptr, nullptr);
 
       if (return_value == RETURN_VALUE_STRUCT_CONVENTION
 	  && val_type->code () != TYPE_CODE_VOID)
@@ -1908,7 +1908,7 @@ finish_command (const char *arg, int from_tty)
 	gdb_printf (_("Run back to call of "));
       else
 	{
-	  if (sm->function != NULL && TYPE_NO_RETURN (sm->function->type ())
+	  if (sm->function != nullptr && TYPE_NO_RETURN (sm->function->type ())
 	      && !query (_("warning: Function %s does not return normally.\n"
 			   "Try to finish anyway? "),
 			 sm->function->print_name ()))
@@ -1926,7 +1926,7 @@ finish_command (const char *arg, int from_tty)
     {
       frame = skip_finish_frames (frame);
 
-      if (frame == NULL)
+      if (frame == nullptr)
 	error (_("Cannot find the caller frame."));
 
       finish_forward (sm, frame);
@@ -2029,7 +2029,7 @@ environment_info (const char *var, int from_tty)
     {
       char **envp = current_inferior ()->environment.envp ();
 
-      for (int idx = 0; envp[idx] != NULL; ++idx)
+      for (int idx = 0; envp[idx] != nullptr; ++idx)
 	{
 	  gdb_puts (envp[idx]);
 	  gdb_puts ("\n");
@@ -2141,7 +2141,7 @@ path_command (const char *dirname, int from_tty)
   mod_path (dirname, exec_path);
   current_inferior ()->environment.set (path_var_name, exec_path.c_str ());
   if (from_tty)
-    path_info (NULL, from_tty);
+    path_info (nullptr, from_tty);
 }
 \f
 
@@ -2288,7 +2288,7 @@ registers_info (const char *addr_exp, int fpregs)
 
   if (!target_has_registers ())
     error (_("The program has no registers now."));
-  frame = get_selected_frame (NULL);
+  frame = get_selected_frame (nullptr);
   gdbarch = get_frame_arch (frame);
 
   if (!addr_exp)
@@ -2364,7 +2364,7 @@ registers_info (const char *addr_exp, int fpregs)
 		break;
 	      }
 	  }
-	if (group != NULL)
+	if (group != nullptr)
 	  {
 	    int regnum;
 
@@ -2430,7 +2430,7 @@ info_vector_command (const char *args, int from_tty)
   if (!target_has_registers ())
     error (_("The program has no registers now."));
 
-  print_vector_info (gdb_stdout, get_selected_frame (NULL), args);
+  print_vector_info (gdb_stdout, get_selected_frame (nullptr), args);
 }
 \f
 /* Kill the inferior process.  Make us have no inferior.  */
@@ -2498,7 +2498,7 @@ setup_inferior (int from_tty)
 
   /* If no exec file is yet known, try to determine it from the
      process itself.  */
-  if (get_exec_file (0) == NULL)
+  if (get_exec_file (0) == nullptr)
     exec_file_locate_attach (inferior_ptid.pid (), 1, from_tty);
   else
     {
@@ -2644,7 +2644,7 @@ attach_command (const char *args, int from_tty)
   attach_target->attach (args, from_tty);
   /* to_attach should push the target, so after this point we
      shouldn't refer to attach_target again.  */
-  attach_target = NULL;
+  attach_target = nullptr;
 
   infrun_debug_show_threads ("immediately after attach",
 			     current_inferior ()->non_exited_threads ());
@@ -2826,7 +2826,7 @@ detach_command (const char *args, int from_tty)
   /* If the solist is global across inferiors, don't clear it when we
      detach from a single inferior.  */
   if (!gdbarch_has_global_solist (target_gdbarch ()))
-    no_shared_libraries (NULL, from_tty);
+    no_shared_libraries (nullptr, from_tty);
 
   if (deprecated_detach_hook)
     deprecated_detach_hook ();
@@ -2852,7 +2852,7 @@ disconnect_command (const char *args, int from_tty)
   query_if_trace_running (from_tty);
   disconnect_tracing ();
   target_disconnect (args, from_tty);
-  no_shared_libraries (NULL, from_tty);
+  no_shared_libraries (nullptr, from_tty);
   init_thread_list ();
   if (deprecated_detach_hook)
     deprecated_detach_hook ();
@@ -2921,7 +2921,7 @@ interrupt_command (const char *args, int from_tty)
 
       dont_repeat ();		/* Not for the faint of heart.  */
 
-      if (args != NULL
+      if (args != nullptr
 	  && startswith (args, "-a"))
 	all_threads = 1;
 
@@ -2959,7 +2959,7 @@ info_float_command (const char *args, int from_tty)
   if (!target_has_registers ())
     error (_("The program has no registers now."));
 
-  frame = get_selected_frame (NULL);
+  frame = get_selected_frame (nullptr);
   gdbarch_print_float_info (get_frame_arch (frame), gdb_stdout, frame, args);
 }
 \f
@@ -3085,7 +3085,7 @@ void
 _initialize_infcmd ()
 {
   static struct cmd_list_element *info_proc_cmdlist;
-  struct cmd_list_element *c = NULL;
+  struct cmd_list_element *c = nullptr;
   const char *cmd_name;
 
   /* Add the filename of the terminal connected to inferior I/O.  */
@@ -3100,8 +3100,8 @@ is restored."),
 				     show_inferior_tty_command,
 				     &setlist, &showlist);
   cmd_name = "inferior-tty";
-  c = lookup_cmd (&cmd_name, setlist, "", NULL, -1, 1);
-  gdb_assert (c != NULL);
+  c = lookup_cmd (&cmd_name, setlist, "", nullptr, -1, 1);
+  gdb_assert (c != nullptr);
   add_alias_cmd ("tty", c, class_run, 0, &cmdlist);
 
   cmd_name = "args";
@@ -3113,8 +3113,8 @@ Follow this command with any number of args, to be passed to the program."),
 				   set_args_command,
 				   show_args_command,
 				   &setlist, &showlist);
-  c = lookup_cmd (&cmd_name, setlist, "", NULL, -1, 1);
-  gdb_assert (c != NULL);
+  c = lookup_cmd (&cmd_name, setlist, "", nullptr, -1, 1);
+  gdb_assert (c != nullptr);
   set_cmd_completer (c, filename_completer);
 
   cmd_name = "cwd";
@@ -3132,8 +3132,8 @@ working directory."),
 				   set_cwd_command,
 				   show_cwd_command,
 				   &setlist, &showlist);
-  c = lookup_cmd (&cmd_name, setlist, "", NULL, -1, 1);
-  gdb_assert (c != NULL);
+  c = lookup_cmd (&cmd_name, setlist, "", nullptr, -1, 1);
+  gdb_assert (c != nullptr);
   set_cmd_completer (c, filename_completer);
 
   c = add_cmd ("environment", no_class, environment_info, _("\
@@ -3404,8 +3404,8 @@ List all available info about the specified process."),
   add_setshow_boolean_cmd ("finish", class_support,
 			   &finish_print, _("\
 Set whether `finish' prints the return value."), _("\
-Show whether `finish' prints the return value."), NULL,
-			   NULL,
+Show whether `finish' prints the return value."), nullptr,
+			   nullptr,
 			   show_print_finish,
 			   &setprintlist, &showprintlist);
 }
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6da46b75ac7..debc6054624 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -388,7 +388,7 @@ static const char follow_fork_mode_parent[] = "parent";
 static const char *const follow_fork_mode_kind_names[] = {
   follow_fork_mode_child,
   follow_fork_mode_parent,
-  NULL
+  nullptr
 };
 
 static const char *follow_fork_mode_string = follow_fork_mode_parent;
@@ -644,8 +644,8 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 
       if (has_vforked)
 	{
-	  gdb_assert (child_inf->vfork_parent == NULL);
-	  gdb_assert (parent_inf->vfork_child == NULL);
+	  gdb_assert (child_inf->vfork_parent == nullptr);
+	  gdb_assert (parent_inf->vfork_child == nullptr);
 	  child_inf->vfork_parent = parent_inf;
 	  child_inf->pending_detach = 0;
 	  parent_inf->vfork_child = child_inf;
@@ -700,12 +700,12 @@ follow_fork ()
      followed fork child thread should have a copy of most of the
      parent thread structure's run control related fields, not just these.
      Initialized to avoid "may be used uninitialized" warnings from gcc.  */
-  struct breakpoint *step_resume_breakpoint = NULL;
-  struct breakpoint *exception_resume_breakpoint = NULL;
+  struct breakpoint *step_resume_breakpoint = nullptr;
+  struct breakpoint *exception_resume_breakpoint = nullptr;
   CORE_ADDR step_range_start = 0;
   CORE_ADDR step_range_end = 0;
   int current_line = 0;
-  symtab *current_symtab = NULL;
+  symtab *current_symtab = nullptr;
   struct frame_id step_frame_id = { 0 };
 
   if (!non_stop)
@@ -930,8 +930,8 @@ handle_vfork_child_exec_or_exit (int exec)
       /* This exec or exit marks the end of the shared memory region
 	 between the parent and the child.  Break the bonds.  */
       inferior *vfork_parent = inf->vfork_parent;
-      inf->vfork_parent->vfork_child = NULL;
-      inf->vfork_parent = NULL;
+      inf->vfork_parent->vfork_child = nullptr;
+      inf->vfork_parent = nullptr;
 
       /* If the user wanted to detach from the parent, now is the
 	 time.  */
@@ -964,8 +964,8 @@ handle_vfork_child_exec_or_exit (int exec)
 
 	  pspace = inf->pspace;
 	  aspace = inf->aspace;
-	  inf->aspace = NULL;
-	  inf->pspace = NULL;
+	  inf->aspace = nullptr;
+	  inf->pspace = nullptr;
 
 	  if (print_inferior_events)
 	    {
@@ -1104,7 +1104,7 @@ static const char *const follow_exec_mode_names[] =
 {
   follow_exec_mode_new,
   follow_exec_mode_same,
-  NULL,
+  nullptr,
 };
 
 static const char *follow_exec_mode_string = follow_exec_mode_same;
@@ -1178,9 +1178,9 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
      breakpoint or similar, it's gone now.  We cannot truly
      step-to-next statement through an exec().  */
   thread_info *th = inferior_thread ();
-  th->control.step_resume_breakpoint = NULL;
-  th->control.exception_resume_breakpoint = NULL;
-  th->control.single_step_breakpoints = NULL;
+  th->control.step_resume_breakpoint = nullptr;
+  th->control.exception_resume_breakpoint = nullptr;
+  th->control.single_step_breakpoints = nullptr;
   th->control.step_range_start = 0;
   th->control.step_range_end = 0;
 
@@ -1203,13 +1203,13 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
   breakpoint_init_inferior (inf_execd);
 
   gdb::unique_xmalloc_ptr<char> exec_file_host
-    = exec_file_find (exec_file_target, NULL);
+    = exec_file_find (exec_file_target, nullptr);
 
   /* If we were unable to map the executable target pathname onto a host
      pathname, tell the user that.  Otherwise GDB's subsequent behavior
      is confusing.  Maybe it would even be better to stop at this point
      so that the user can specify a file manually before continuing.  */
-  if (exec_file_host == NULL)
+  if (exec_file_host == nullptr)
     warning (_("Could not load symbols for executable %s.\n"
 	       "Do you need \"set sysroot\"?"),
 	     exec_file_target);
@@ -1220,7 +1220,7 @@ follow_exec (ptid_t ptid, const char *exec_file_target)
   /* Also, loading a symbol file below may trigger symbol lookups, and
      we don't want those to be satisfied by the libraries of the
      previous incarnation of this process.  */
-  no_shared_libraries (NULL, 0);
+  no_shared_libraries (nullptr, 0);
 
   struct inferior *inf = current_inferior ();
 
@@ -1380,7 +1380,7 @@ static void
 clear_step_over_info (void)
 {
   infrun_debug_printf ("clearing step over info");
-  step_over_info.aspace = NULL;
+  step_over_info.aspace = nullptr;
   step_over_info.address = 0;
   step_over_info.nonsteppable_watchpoint_p = 0;
   step_over_info.thread = -1;
@@ -1392,7 +1392,7 @@ int
 stepping_past_instruction_at (struct address_space *aspace,
 			      CORE_ADDR address)
 {
-  return (step_over_info.aspace != NULL
+  return (step_over_info.aspace != nullptr
 	  && breakpoint_address_match (aspace, address,
 				       step_over_info.aspace,
 				       step_over_info.address));
@@ -1420,7 +1420,7 @@ stepping_past_nonsteppable_watchpoint (void)
 static bool
 step_over_info_valid_p (void)
 {
-  return (step_over_info.aspace != NULL
+  return (step_over_info.aspace != nullptr
 	  || stepping_past_nonsteppable_watchpoint ());
 }
 
@@ -1516,7 +1516,7 @@ step_over_info_valid_p (void)
 static bool
 displaced_step_in_progress_thread (thread_info *thread)
 {
-  gdb_assert (thread != NULL);
+  gdb_assert (thread != nullptr);
 
   return thread->displaced_step_state.in_progress ();
 }
@@ -2101,7 +2101,7 @@ static const char *const scheduler_enums[] = {
   schedlock_on,
   schedlock_step,
   schedlock_replay,
-  NULL
+  nullptr
 };
 static const char *scheduler_mode = schedlock_replay;
 static void
@@ -2195,7 +2195,7 @@ process_stratum_target *
 user_visible_resume_target (ptid_t resume_ptid)
 {
   return (resume_ptid == minus_one_ptid && sched_multi
-	  ? NULL
+	  ? nullptr
 	  : current_inferior ()->process_target ());
 }
 
@@ -2417,7 +2417,7 @@ resume_1 (enum gdb_signal sig)
 	  clear_step_over_info ();
 	  tp->control.trap_expected = 0;
 
-	  if (tp->control.step_resume_breakpoint == NULL)
+	  if (tp->control.step_resume_breakpoint == nullptr)
 	    {
 	      /* Set a "high-priority" step-resume, as we don't want
 		 user breakpoints at PC to trigger (again) when this
@@ -2559,7 +2559,7 @@ resume_1 (enum gdb_signal sig)
 	 a step-resume breakpoint set on the earlier handler.  We cannot
 	 set another step-resume breakpoint; just continue on until the
 	 original breakpoint is hit.  */
-      if (tp->control.step_resume_breakpoint == NULL)
+      if (tp->control.step_resume_breakpoint == nullptr)
 	{
 	  insert_hp_step_resume_breakpoint_at_frame (get_current_frame ());
 	  tp->step_after_step_resume_breakpoint = 1;
@@ -2768,7 +2768,7 @@ clear_proceed_status_thread (struct thread_info *tp)
   tp->control.step_frame_id = null_frame_id;
   tp->control.step_stack_frame_id = null_frame_id;
   tp->control.step_over_calls = STEP_OVER_UNDEBUGGABLE;
-  tp->control.step_start_function = NULL;
+  tp->control.step_start_function = nullptr;
   tp->stop_requested = 0;
 
   tp->control.stop_step = 0;
@@ -3667,12 +3667,12 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
 
       /* We have a specific thread to check.  */
       tp = find_thread_ptid (inf, ptid);
-      gdb_assert (tp != NULL);
+      gdb_assert (tp != nullptr);
       if (!tp->has_pending_waitstatus ())
-	tp = NULL;
+	tp = nullptr;
     }
 
-  if (tp != NULL
+  if (tp != nullptr
       && (tp->stop_reason () == TARGET_STOPPED_BY_SW_BREAKPOINT
 	  || tp->stop_reason () == TARGET_STOPPED_BY_HW_BREAKPOINT))
     {
@@ -3713,7 +3713,7 @@ do_target_wait_1 (inferior *inf, ptid_t ptid,
 	}
     }
 
-  if (tp != NULL)
+  if (tp != nullptr)
     {
       infrun_debug_printf ("Using pending wait status %s for %s.",
 			   tp->pending_waitstatus ().to_string ().c_str (),
@@ -4261,7 +4261,7 @@ fetch_inferior_event ()
 	    if (should_notify_stop)
 	      {
 		/* We may not find an inferior if this was a process exit.  */
-		if (inf == NULL || inf->control.stop_soon == NO_STOP_QUIETLY)
+		if (inf == nullptr || inf->control.stop_soon == NO_STOP_QUIETLY)
 		  proceeded = normal_stop ();
 	      }
 
@@ -4560,7 +4560,7 @@ static bool
 stepped_in_from (frame_info_ptr frame, struct frame_id step_frame_id)
 {
   for (frame = get_prev_frame (frame);
-       frame != NULL;
+       frame != nullptr;
        frame = get_prev_frame (frame))
     {
       if (get_frame_id (frame) == step_frame_id)
@@ -4587,9 +4587,9 @@ inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
   if (prev_frame)
     frame = get_prev_frame (frame);
 
-  for (; frame != NULL; frame = get_prev_frame (frame))
+  for (; frame != nullptr; frame = get_prev_frame (frame))
     {
-      const char *fn = NULL;
+      const char *fn = nullptr;
       symtab_and_line sal;
       struct symbol *sym;
 
@@ -4601,7 +4601,7 @@ inline_frame_is_marked_for_skip (bool prev_frame, struct thread_info *tp)
       sal = find_frame_sal (frame);
       sym = get_frame_function (frame);
 
-      if (sym != NULL)
+      if (sym != nullptr)
 	fn = sym->print_name ();
 
       if (sal.line != 0
@@ -4726,7 +4726,7 @@ get_inferior_stop_soon (execution_control_state *ecs)
 {
   struct inferior *inf = find_inferior_ptid (ecs->target, ecs->ptid);
 
-  gdb_assert (inf != NULL);
+  gdb_assert (inf != nullptr);
   return inf->control.stop_soon;
 }
 
@@ -4764,7 +4764,7 @@ wait_one ()
       for (inferior *inf : all_inferiors ())
 	{
 	  process_stratum_target *target = inf->process_target ();
-	  if (target == NULL
+	  if (target == nullptr
 	      || !target->is_async_p ()
 	      || !target->threads_executing)
 	    continue;
@@ -4795,7 +4795,7 @@ wait_one ()
       for (inferior *inf : all_inferiors ())
 	{
 	  process_stratum_target *target = inf->process_target ();
-	  if (target == NULL
+	  if (target == nullptr
 	      || !target->is_async_p ()
 	      || !target->threads_executing)
 	    continue;
@@ -4811,12 +4811,12 @@ wait_one ()
 	  /* No waitable targets left.  All must be stopped.  */
 	  target_waitstatus ws;
 	  ws.set_no_resumed ();
-	  return {NULL, minus_one_ptid, std::move (ws)};
+	  return {nullptr, minus_one_ptid, std::move (ws)};
 	}
 
       QUIT;
 
-      int numfds = interruptible_select (nfds, &readfds, 0, NULL, 0);
+      int numfds = interruptible_select (nfds, &readfds, 0, nullptr, 0);
       if (numfds < 0)
 	{
 	  if (errno == EINTR)
@@ -4987,7 +4987,7 @@ handle_one (const wait_one_event &event)
   else
     {
       thread_info *t = find_thread_ptid (event.target, event.ptid);
-      if (t == NULL)
+      if (t == nullptr)
 	t = add_thread (event.target, event.ptid);
 
       t->stop_requested = 0;
@@ -5393,7 +5393,7 @@ handle_inferior_event (struct execution_control_state *ecs)
     {
       ecs->event_thread = find_thread_ptid (ecs->target, ecs->ptid);
       /* If it's a new thread, add it to the thread database.  */
-      if (ecs->event_thread == NULL)
+      if (ecs->event_thread == nullptr)
 	ecs->event_thread = add_thread (ecs->target, ecs->ptid);
 
       /* Disable range stepping.  If the next step request could use a
@@ -6064,8 +6064,8 @@ finish_step_over (struct execution_control_state *ecs)
 	return 0;
 
       pending = iterate_over_threads (resumed_thread_with_pending_status,
-				      NULL);
-      if (pending != NULL)
+				      nullptr);
+      if (pending != nullptr)
 	{
 	  struct thread_info *tp = ecs->event_thread;
 	  struct regcache *regcache;
@@ -6542,7 +6542,7 @@ handle_signal_stop (struct execution_control_state *ecs)
 
       if (ecs->event_thread->prev_pc == ecs->event_thread->stop_pc ()
 	  && ecs->event_thread->control.trap_expected
-	  && ecs->event_thread->control.step_resume_breakpoint == NULL)
+	  && ecs->event_thread->control.step_resume_breakpoint == nullptr)
 	{
 	  /* We were just starting a new sequence, attempting to
 	     single-step off of a breakpoint and expecting a SIGTRAP.
@@ -6574,7 +6574,7 @@ handle_signal_stop (struct execution_control_state *ecs)
 	      || ecs->event_thread->control.step_range_end == 1)
 	  && (get_stack_frame_id (frame)
 	      == ecs->event_thread->control.step_stack_frame_id)
-	  && ecs->event_thread->control.step_resume_breakpoint == NULL)
+	  && ecs->event_thread->control.step_resume_breakpoint == nullptr)
 	{
 	  /* The inferior is about to take a signal that will take it
 	     out of the single step range.  Set a breakpoint at the
@@ -6719,7 +6719,7 @@ process_event_stop_test (struct execution_control_state *ecs)
 	infrun_debug_printf ("BPSTAT_WHAT_CLEAR_LONGJMP_RESUME");
 
 	gdb_assert (ecs->event_thread->control.exception_resume_breakpoint
-		    != NULL);
+		    != nullptr);
 	delete_exception_resume_breakpoint (ecs->event_thread);
 
 	if (what.is_longjmp)
@@ -6853,7 +6853,7 @@ process_event_stop_test (struct execution_control_state *ecs)
       struct breakpoint *sr_bp
 	= ecs->event_thread->control.step_resume_breakpoint;
 
-      if (sr_bp != NULL
+      if (sr_bp != nullptr
 	  && sr_bp->loc->permanent
 	  && sr_bp->type == bp_hp_step_resume
 	  && sr_bp->loc->address == ecs->event_thread->prev_pc)
@@ -7269,7 +7269,7 @@ process_event_stop_test (struct execution_control_state *ecs)
      the trampoline processing logic, however, there are some trampolines 
      that have no names, so we should do trampoline handling first.  */
   if (ecs->event_thread->control.step_over_calls == STEP_OVER_UNDEBUGGABLE
-      && ecs->stop_func_name == NULL
+      && ecs->stop_func_name == nullptr
       && stop_pc_sal.line == 0)
     {
       infrun_debug_printf ("stepped into undebuggable function");
@@ -7771,7 +7771,7 @@ static bool
 currently_stepping (struct thread_info *tp)
 {
   return ((tp->control.step_range_end
-	   && tp->control.step_resume_breakpoint == NULL)
+	   && tp->control.step_resume_breakpoint == nullptr)
 	  || tp->control.trap_expected
 	  || tp->stepped_breakpoint
 	  || bpstat_should_step ());
@@ -7789,7 +7789,7 @@ handle_step_into_function (struct gdbarch *gdbarch,
 
   compunit_symtab *cust
     = find_pc_compunit_symtab (ecs->event_thread->stop_pc ());
-  if (cust != NULL && cust->language () != language_asm)
+  if (cust != nullptr && cust->language () != language_asm)
     ecs->stop_func_start
       = gdbarch_skip_prologue_noexcept (gdbarch, ecs->stop_func_start);
 
@@ -7868,7 +7868,7 @@ handle_step_into_function_backward (struct gdbarch *gdbarch,
   fill_in_stop_func (gdbarch, ecs);
 
   cust = find_pc_compunit_symtab (ecs->event_thread->stop_pc ());
-  if (cust != NULL && cust->language () != language_asm)
+  if (cust != nullptr && cust->language () != language_asm)
     ecs->stop_func_start
       = gdbarch_skip_prologue_noexcept (gdbarch, ecs->stop_func_start);
 
@@ -7904,7 +7904,7 @@ insert_step_resume_breakpoint_at_sal_1 (struct gdbarch *gdbarch,
   /* There should never be more than one step-resume or longjmp-resume
      breakpoint per thread, so we should never be setting a new
      step_resume_breakpoint when one is already active.  */
-  gdb_assert (inferior_thread ()->control.step_resume_breakpoint == NULL);
+  gdb_assert (inferior_thread ()->control.step_resume_breakpoint == nullptr);
   gdb_assert (sr_type == bp_step_resume || sr_type == bp_hp_step_resume);
 
   infrun_debug_printf ("inserting step-resume breakpoint at %s",
@@ -7934,7 +7934,7 @@ insert_step_resume_breakpoint_at_sal (struct gdbarch *gdbarch,
 static void
 insert_hp_step_resume_breakpoint_at_frame (frame_info_ptr return_frame)
 {
-  gdb_assert (return_frame != NULL);
+  gdb_assert (return_frame != nullptr);
 
   struct gdbarch *gdbarch = get_frame_arch (return_frame);
 
@@ -7992,7 +7992,7 @@ insert_longjmp_resume_breakpoint (struct gdbarch *gdbarch, CORE_ADDR pc)
   /* There should never be more than one longjmp-resume breakpoint per
      thread, so we should never be setting a new
      longjmp_resume_breakpoint when one is already active.  */
-  gdb_assert (inferior_thread ()->control.exception_resume_breakpoint == NULL);
+  gdb_assert (inferior_thread ()->control.exception_resume_breakpoint == nullptr);
 
   infrun_debug_printf ("inserting longjmp-resume breakpoint at %s",
 		       paddress (gdbarch, pc));
@@ -8036,7 +8036,7 @@ insert_exception_resume_breakpoint (struct thread_info *tp,
 					       bp_exception_resume).release ();
 
 	  /* set_momentary_breakpoint_at_pc invalidates FRAME.  */
-	  frame = NULL;
+	  frame = nullptr;
 
 	  bp->thread = tp->global_num;
 	  inferior_thread ()->control.exception_resume_breakpoint = bp;
@@ -8241,7 +8241,7 @@ keep_going_pass_signal (struct execution_control_state *ecs)
 			      ecs->event_thread->global_num);
 	}
       else if (remove_wps)
-	set_step_over_info (NULL, 0, remove_wps, -1);
+	set_step_over_info (nullptr, 0, remove_wps, -1);
 
       /* If we now need to do an in-line step-over, we need to stop
 	 all other threads.  Note this must be done before
@@ -8403,7 +8403,7 @@ print_signal_received_reason (struct ui_out *uiout, enum gdb_signal siggnal)
       uiout->field_string ("thread-id", print_thread_id (thr));
 
       const char *name = thread_name (thr);
-      if (name != NULL)
+      if (name != nullptr)
 	{
 	  uiout->text (" \"");
 	  uiout->field_string ("name", name);
@@ -8501,7 +8501,7 @@ print_stop_location (const target_waitstatus &ws)
      LOCATION: Print only location
      SRC_AND_LOC: Print location and source line.  */
   if (do_frame_printing)
-    print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1);
+    print_stack_frame (get_selected_frame (nullptr), 0, source_flag, 1);
 }
 
 /* See infrun.h.  */
@@ -8605,7 +8605,7 @@ stop_context::changed () const
     return true;
   if (inf_num != current_inferior ()->num)
     return true;
-  if (thread != NULL && thread->state != THREAD_STOPPED)
+  if (thread != nullptr && thread->state != THREAD_STOPPED)
     return true;
   if (get_stop_id () != stop_id)
     return true;
@@ -8782,7 +8782,7 @@ normal_stop (void)
     gdb::observers::normal_stop.notify (inferior_thread ()->control.stop_bpstat,
 					stop_print_frame);
   else
-    gdb::observers::normal_stop.notify (NULL, stop_print_frame);
+    gdb::observers::normal_stop.notify (nullptr, stop_print_frame);
 
   annotate_stopped ();
 
@@ -8912,7 +8912,7 @@ handle_command (const char *args, int from_tty)
   enum gdb_signal oursig;
   int allsigs;
 
-  if (args == NULL)
+  if (args == nullptr)
     {
       error_no_arg (_("signal to handle"));
     }
@@ -9088,7 +9088,7 @@ handle_completer (struct cmd_list_element *ignore,
       "noignore",
       "noprint",
       "nopass",
-      NULL,
+      nullptr,
     };
 
   signal_completer (ignore, tracker, text, word);
@@ -9172,7 +9172,7 @@ siginfo_value_read (struct value *v)
   transferred =
     target_read (current_inferior ()->top_target (),
 		 TARGET_OBJECT_SIGNAL_INFO,
-		 NULL,
+		 nullptr,
 		 value_contents_all_raw (v).data (),
 		 value_offset (v),
 		 value_type (v)->length ());
@@ -9195,7 +9195,7 @@ siginfo_value_write (struct value *v, struct value *fromval)
 
   transferred = target_write (current_inferior ()->top_target (),
 			      TARGET_OBJECT_SIGNAL_INFO,
-			      NULL,
+			      nullptr,
 			      value_contents_all_raw (fromval).data (),
 			      value_offset (v),
 			      value_type (fromval)->length ());
@@ -9224,7 +9224,7 @@ siginfo_make_value (struct gdbarch *gdbarch, struct internalvar *var,
     {
       struct type *type = gdbarch_get_siginfo_type (gdbarch);
 
-      return allocate_computed_value (type, &siginfo_value_funcs, NULL);
+      return allocate_computed_value (type, &siginfo_value_funcs, nullptr);
     }
 
   return allocate_value (builtin_type (gdbarch)->builtin_void);
@@ -9259,7 +9259,7 @@ class infcall_suspend_state
 	siginfo_data.reset ((gdb_byte *) xmalloc (len));
 
 	if (target_read (current_inferior ()->top_target (),
-			 TARGET_OBJECT_SIGNAL_INFO, NULL,
+			 TARGET_OBJECT_SIGNAL_INFO, nullptr,
 			 siginfo_data.get (), 0, len) != len)
 	  {
 	    /* Errors ignored.  */
@@ -9295,7 +9295,7 @@ class infcall_suspend_state
 
 	/* Errors ignored.  */
 	target_write (current_inferior ()->top_target (),
-		      TARGET_OBJECT_SIGNAL_INFO, NULL,
+		      TARGET_OBJECT_SIGNAL_INFO, nullptr,
 		      m_siginfo_data.get (), 0, type->length ());
       }
 
@@ -9399,8 +9399,8 @@ save_infcall_control_state ()
   inf_status->thread_control = tp->control;
   inf_status->inferior_control = inf->control;
 
-  tp->control.step_resume_breakpoint = NULL;
-  tp->control.exception_resume_breakpoint = NULL;
+  tp->control.step_resume_breakpoint = nullptr;
+  tp->control.exception_resume_breakpoint = nullptr;
 
   /* Save original bpstat chain to INF_STATUS; replace it in TP with copy of
      chain.  If caller's caller is walking the chain, they'll be happier if we
@@ -9490,7 +9490,7 @@ static const char *exec_direction = exec_forward;
 static const char *const exec_direction_names[] = {
   exec_forward,
   exec_reverse,
-  NULL
+  nullptr
 };
 
 static void
@@ -9541,7 +9541,7 @@ show_schedule_multiple (struct ui_file *file, int from_tty,
 static const struct internalvar_funcs siginfo_funcs =
 {
   siginfo_make_value,
-  NULL,
+  nullptr,
 };
 
 /* Callback for infrun's target events source.  This is marked when a
@@ -9637,7 +9637,7 @@ _initialize_infrun ()
 
   /* Register extra event sources in the event loop.  */
   infrun_async_inferior_event_token
-    = create_async_event_handler (infrun_async_inferior_event_handler, NULL,
+    = create_async_event_handler (infrun_async_inferior_event_handler, nullptr,
 				  "infrun");
 
   cmd_list_element *info_signals_cmd
@@ -9683,7 +9683,7 @@ of the program stops."), &cmdlist);
      _("Set inferior debugging."),
      _("Show inferior debugging."),
      _("When non-zero, inferior specific debugging is enabled."),
-     NULL, show_debug_infrun, &setdebuglist, &showdebuglist);
+     nullptr, show_debug_infrun, &setdebuglist, &showdebuglist);
 
   add_setshow_boolean_cmd ("non-stop", no_class,
 			   &non_stop_1, _("\
@@ -9786,7 +9786,7 @@ A fork or vfork creates a new process.  follow-fork-mode can be:\n\
   child   - the new process is debugged after a fork\n\
 The unfollowed process will continue to run.\n\
 By default, the debugger will follow the parent process."),
-			NULL,
+			nullptr,
 			show_follow_fork_mode_string,
 			&setlist, &showlist);
 
@@ -9810,7 +9810,7 @@ the inferior.  Restarting the inferior after the exec call restarts\n\
 the executable the process was running after the exec call.\n\
 \n\
 By default, the debugger will use the same inferior."),
-			NULL,
+			nullptr,
 			show_follow_exec_mode_string,
 			&setlist, &showlist);
 
@@ -9837,7 +9837,7 @@ threads of all processes.  When off (which is the default), execution\n\
 commands only resume the threads of the current process.  The set of\n\
 threads that are resumed is further refined by the scheduler-locking\n\
 mode (see help set scheduler-locking)."),
-			   NULL,
+			   nullptr,
 			   show_schedule_multiple,
 			   &setlist, &showlist);
 
@@ -9847,7 +9847,7 @@ Show mode of the step operation."), _("\
 When set, doing a step over a function without debug line information\n\
 will stop at the first instruction of that function. Otherwise, the\n\
 function is skipped and the step command stops at a different source line."),
-			   NULL,
+			   nullptr,
 			   show_step_stop_if_no_debug,
 			   &setlist, &showlist);
 
@@ -9861,7 +9861,7 @@ stepping to step over breakpoints, even if such is supported by the target\n\
 architecture.  If auto (which is the default), gdb will use displaced stepping\n\
 if the target architecture supports it and non-stop mode is active, but will not\n\
 use it in all-stop mode (see help set non-stop)."),
-				NULL,
+				nullptr,
 				show_can_use_displaced_stepping,
 				&setlist, &showlist);
 
@@ -9879,7 +9879,7 @@ Options are 'forward' or 'reverse'."),
 Set whether gdb will detach the child of a fork."), _("\
 Show whether gdb will detach the child of a fork."), _("\
 Tells gdb whether to detach the child of a fork."),
-			   NULL, NULL, &setlist, &showlist);
+			   nullptr, nullptr, &setlist, &showlist);
 
   /* Set/show disable address space randomization mode.  */
 
@@ -9910,7 +9910,7 @@ enabled by default on some platforms."),
      value with a void typed value, and when we get here, gdbarch
      isn't initialized yet.  At this point, we're quite sure there
      isn't another convenience variable of the same name.  */
-  create_internalvar_type_lazy ("_siginfo", &siginfo_funcs, NULL);
+  create_internalvar_type_lazy ("_siginfo", &siginfo_funcs, nullptr);
 
   add_setshow_boolean_cmd ("observer", no_class,
 			   &observer_mode_1, _("\
-- 
2.31.1

Re: [PATCH] Change NULL to nullptr in gdb/infcmd.c and gdb/infrun.c

[Simon Marchi]

On 11/16/22 15:55, Carl Love wrote:
> Lancelot, GDB maintainers:
> 
> The GDB coding style guide specifies that nullptr should be used
> instead of NULL as noted by Lancelot for the recent patch "PowerPC, fix
> support for printing the function return value for non-trivial
> values.".  This patch changes all of the various NULL statements to
> nullptr statements in files gdb/infcmd.c and gdb/infrun.c per the
> coding style guide.
> 
> The patch does not make any functional changes to the code.  
> 
> The patch has been tested on both X86_64 and PowerPC to ensure there
> were no new unexpected error, new core files generated, new unresolved
> tests etc.  
> 
> Please let me know if this patch is acceptable.  Thanks. 
> 
>                          Carl Love
> 
> 
> ------------------------
> Change NULL to nullptr in gdb/infcmd.c and gdb/infrun.c
> 
> The GDB coding standard specifies that nullptr should be used instead of NULL.
> There are numerous uses of NULL and nullptr in files infcmd.c and infrun.c.
> This patch replaces the various uses of NULL with nullptr in the source files.
> The use of NULL in the comments was not changed.
> 
> The patch does not introduce any functional changes.
> 
> The patch has been tested on PowerPC and Intel X86_64 with no new unexpected
> test failures, unresolved tests, new core files etc.

Thanks, this LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

I appreciate this kind of cleanup that bring the code in line with what
the current recommended practices are.  I was thinking the other day if
we could do a mass find and replace from NULL to nullptr.  I am a just a
bit afraid that there might be instances we don't want to change, in
comments or literal strings, so we have to be a bit more careful.

Simon

[PATCH 1/2] PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

[Carl Love]

GDB maintainers:

The following PowerPC specific patch fixes an issue of GDB reporting a
bogus return value for functions that return a non-trivial value.  The
bogus return values result in five testcase failures for test
gdb.cp/non-trivial-retval.exp.  The issue is the function
ppc64_sysv_abi_return_value does not return the correct value when the
valtype->code() is TYPE_CODE_STRUCT and the language_pass_by_reference
is not trivially_copyable.  This patch adds the needed code to return
RETURN_VALUE_STRUCT_CONVENTION in these cases.  

The testcase gdb.cp/non-trivial-retval.exp still fails as gdb now
correctly reports "Cannot determine contents" instead of the expected
values, which is correct in this case.  The PowerPC ABI uses passes the
return buffer address in r3.  The value of r3 is valid on entry to the
function but the PowerPC ABI does not guarantee it will not be changed
in the function.  Hence the contents of r3 is not reliable on exit from
the function.  This issue will be addressed by the next patch in this
patch series.

The patch has been tested on PowerPC and on Intel X86-64 with no
regression failures.

Please let me know if this patch is acceptable for the GDB mainline. 
Thanks.

                       Carl Love

-----------------------------------------
PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

This patch address five testcase failures in gdb.cp/non-trivial-retval.exp.
The following commit resulted in the five testcases failures on PowerPC.  The
value returned by the function is being reported incorrectly.

  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 function:

  enum return_value_convention
  ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
                               struct type *valtype, struct regcache *regcache,
                               gdb_byte *readbuf, const gdb_byte *writebuf)

should return RETURN_VALUE_STRUCT_CONVENTION if the valtype->code() is
TYPE_CODE_STRUCT and if the language_pass_by_reference is not
trivially_copyable.

This patch adds the need code to return the value
RETURN_VALUE_STRUCT_CONVENTION in the case of this case.

With this patch, the five test cases still fail but with the message "Value
returned has type: A. Cannot determine contents".  The PowerPC ABI stores the
address of the buffer containing the function return value in register r3 on
entry to the function.  However, the PowerPC ABI does not guarentee that r3
will not be modified in the function.  So when the function returns, the return
buffer address cannot be reliably obtained from register r3.  Thus the message
"Cannot determine contents" is appropriate in this case.
---
 gdb/ppc-sysv-tdep.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index f57c261d9dc..14effb93210 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -2099,6 +2099,10 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
       return RETURN_VALUE_REGISTER_CONVENTION;
     }
 
+  if (!language_pass_by_reference (valtype).trivially_copyable
+      && valtype->code () == TYPE_CODE_STRUCT)
+    return RETURN_VALUE_STRUCT_CONVENTION;
+
   /* 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.31.1

Re: [PATCH 1/2] PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

[Kevin Buettner]

Hi Carl,

See my comments below...

On Thu, 06 Oct 2022 09:37:10 -0700
Carl Love via Gdb-patches <gdb-patches@sourceware.org> wrote:

> GDB maintainers:
> 
> The following PowerPC specific patch fixes an issue of GDB reporting a
> bogus return value for functions that return a non-trivial value.  The
> bogus return values result in five testcase failures for test
> gdb.cp/non-trivial-retval.exp.  The issue is the function
> ppc64_sysv_abi_return_value does not return the correct value when the
> valtype->code() is TYPE_CODE_STRUCT and the language_pass_by_reference
> is not trivially_copyable.  This patch adds the needed code to return
> RETURN_VALUE_STRUCT_CONVENTION in these cases.  
> 
> The testcase gdb.cp/non-trivial-retval.exp still fails as gdb now
> correctly reports "Cannot determine contents" instead of the expected
> values, which is correct in this case.  The PowerPC ABI uses passes the
> return buffer address in r3.  The value of r3 is valid on entry to the
> function but the PowerPC ABI does not guarantee it will not be changed
> in the function.  Hence the contents of r3 is not reliable on exit from
> the function.  This issue will be addressed by the next patch in this
> patch series.
> 
> The patch has been tested on PowerPC and on Intel X86-64 with no
> regression failures.
> 
> Please let me know if this patch is acceptable for the GDB mainline. 
> Thanks.
> 
>                        Carl Love
> 
> -----------------------------------------
> PowerPC, function ppc64_sysv_abi_return_value add missing return value convention
> 
> This patch address five testcase failures in gdb.cp/non-trivial-retval.exp.
> The following commit resulted in the five testcases failures on PowerPC.  The
> value returned by the function is being reported incorrectly.
> 
>   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 function:
> 
>   enum return_value_convention
>   ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
>                                struct type *valtype, struct regcache *regcache,
>                                gdb_byte *readbuf, const gdb_byte *writebuf)
> 
> should return RETURN_VALUE_STRUCT_CONVENTION if the valtype->code() is
> TYPE_CODE_STRUCT and if the language_pass_by_reference is not
> trivially_copyable.
> 
> This patch adds the need code to return the value

s/need/needed/

> RETURN_VALUE_STRUCT_CONVENTION in the case of this case.

s/the case of //

> 
> With this patch, the five test cases still fail but with the message "Value
> returned has type: A. Cannot determine contents".  The PowerPC ABI stores the
> address of the buffer containing the function return value in register r3 on
> entry to the function.  However, the PowerPC ABI does not guarentee that r3
> will not be modified in the function.  So when the function returns, the return
> buffer address cannot be reliably obtained from register r3.  Thus the message
> "Cannot determine contents" is appropriate in this case.
> ---
>  gdb/ppc-sysv-tdep.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
> index f57c261d9dc..14effb93210 100644
> --- a/gdb/ppc-sysv-tdep.c
> +++ b/gdb/ppc-sysv-tdep.c
> @@ -2099,6 +2099,10 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
>        return RETURN_VALUE_REGISTER_CONVENTION;
>      }
>  
> +  if (!language_pass_by_reference (valtype).trivially_copyable
> +      && valtype->code () == TYPE_CODE_STRUCT)
> +    return RETURN_VALUE_STRUCT_CONVENTION;
> +
>    /* 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.31.1

This change looks good to me, but note the tweaks to the commit log remarks
above.

Kevin

P.S. I'm still looking at part 2.

RE: [PATCH 1/2] PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

[Carl Love]

On Fri, 2022-10-07 at 21:20 -0700, Kevin Buettner wrote:
> Hi Carl,
> 
> See my comments below...
> 
> On Thu, 06 Oct 2022 09:37:10 -0700
> Carl Love via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 

<snip>

> > should return RETURN_VALUE_STRUCT_CONVENTION if the valtype->code() 
> > is
> > TYPE_CODE_STRUCT and if the language_pass_by_reference is not
> > trivially_copyable.
> > 
> > This patch adds the need code to return the value
> 
> s/need/needed/
fixed

> 
> > RETURN_VALUE_STRUCT_CONVENTION in the case of this case.
> 
> s/the case of //
fixed.

[PATCH 1/2 ver 2] PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

[Carl Love]

GDB maintainers:

Version 2, updated the commit log per feedback.

The following PowerPC specific patch fixes an issue of GDB reporting a
bogus return value for functions that return a non-trivial value.  The
bogus return values result in five testcase failures for test
gdb.cp/non-trivial-retval.exp.  The issue is the function
ppc64_sysv_abi_return_value does not return the correct value when the
valtype->code() is TYPE_CODE_STRUCT and the language_pass_by_reference
is not trivially_copyable.  This patch adds the needed code to return
RETURN_VALUE_STRUCT_CONVENTION in these cases.  

The testcase gdb.cp/non-trivial-retval.exp still fails as gdb now
correctly reports "Cannot determine contents" instead of the expected
values, which is correct in this case.  The PowerPC ABI uses passes the
return buffer address in r3.  The value of r3 is valid on entry to the
function but the PowerPC ABI does not guarantee it will not be changed
in the function.  Hence the contents of r3 is not reliable on exit from
the function.  This issue will be addressed by the next patch in this
patch series.

The patch has been re-tested on PowerPC and on Intel X86-64 with no
regression failures.

Please let me know if this version of the patch is acceptable for the
GDB mainline.  Thanks.

                       Carl Love


----------------------------
PowerPC, function ppc64_sysv_abi_return_value add missing  return value convention

This patch address five testcase failures in gdb.cp/non-trivial-retval.exp.
The following commit resulted in the five testcases failures on PowerPC.  The
value returned by the function is being reported incorrectly.

  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 function:

  enum return_value_convention
  ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
                               struct type *valtype, struct regcache *regcache,
                               gdb_byte *readbuf, const gdb_byte *writebuf)

should return RETURN_VALUE_STRUCT_CONVENTION if the valtype->code() is
TYPE_CODE_STRUCT and if the language_pass_by_reference is not
trivially_copyable.

This patch adds the need code to return the value
RETURN_VALUE_STRUCT_CONVENTION in the case of this case.

With this patch, the five test cases still fail but with the message "Value
returned has type: A. Cannot determine contents".  The PowerPC ABI stores the
address of the buffer containing the function return value in register r3 on
entry to the function.  However, the PowerPC ABI does not guarentee that r3
will not be modified in the function.  So when the function returns, the return
buffer address cannot be reliably obtained from register r3.  Thus the message
"Cannot determine contents" is appropriate in this case.
---
 gdb/ppc-sysv-tdep.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index f57c261d9dc..14effb93210 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -2099,6 +2099,10 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
       return RETURN_VALUE_REGISTER_CONVENTION;
     }
 
+  if (!language_pass_by_reference (valtype).trivially_copyable
+      && valtype->code () == TYPE_CODE_STRUCT)
+    return RETURN_VALUE_STRUCT_CONVENTION;
+
   /* 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.37.2

Re: [PATCH 1/2 ver 3] PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

[Carl Love]

Ping on this patch.  I have not received any additional comments on the
patch.  I have rebased and retested both patches in the series onto the
latest gdb source tree. This patch applied cleanly on the latest tree.

GDB maintainers:

Version 3, rebased patch on the latest tree.  No additional changes
made to the patch.  Patch has been retested with no regressions.

Version 2, updated the commit log per feedback.

The following PowerPC specific patch fixes an issue of GDB reporting a
bogus return value for functions that return a non-trivial value.  The
bogus return values result in five testcase failures for test
gdb.cp/non-trivial-retval.exp.  The issue is the function
ppc64_sysv_abi_return_value does not return the correct value when the
valtype->code() is TYPE_CODE_STRUCT and the language_pass_by_reference
is not trivially_copyable.  This patch adds the needed code to return
RETURN_VALUE_STRUCT_CONVENTION in these cases.  

The testcase gdb.cp/non-trivial-retval.exp still fails as gdb now
correctly reports "Cannot determine contents" instead of the expected
values, which is correct in this case.  The PowerPC ABI uses passes the
return buffer address in r3.  The value of r3 is valid on entry to the
function but the PowerPC ABI does not guarantee it will not be changed
in the function.  Hence the contents of r3 is not reliable on exit from
the function.  This issue will be addressed by the next patch in this
patch series.

The patch has been re-tested on PowerPC and on Intel X86-64 with no
regression failures.

Please let me know if this version of the patch is acceptable for the
GDB mainline.  Thanks.

                       Carl Love

-------------------------------
PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

This patch address five testcase failures in gdb.cp/non-trivial-retval.exp.
The following commit resulted in the five testcases failures on PowerPC.  The
value returned by the function is being reported incorrectly.

  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 function:

  enum return_value_convention
  ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
                               struct type *valtype, struct regcache *regcache,
                               gdb_byte *readbuf, const gdb_byte *writebuf)

should return RETURN_VALUE_STRUCT_CONVENTION if the valtype->code() is
TYPE_CODE_STRUCT and if the language_pass_by_reference is not
trivially_copyable.

This patch adds the need code to return the value
RETURN_VALUE_STRUCT_CONVENTION in the case of this case.

With this patch, the five test cases still fail but with the message "Value
returned has type: A. Cannot determine contents".  The PowerPC ABI stores the
address of the buffer containing the function return value in register r3 on
entry to the function.  However, the PowerPC ABI does not guarentee that r3
will not be modified in the function.  So when the function returns, the return
buffer address cannot be reliably obtained from register r3.  Thus the message
"Cannot determine contents" is appropriate in this case.
---
 gdb/ppc-sysv-tdep.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/ppc-sysv-tdep.c b/gdb/ppc-sysv-tdep.c
index d7f05ddc6b4..450162dd46e 100644
--- a/gdb/ppc-sysv-tdep.c
+++ b/gdb/ppc-sysv-tdep.c
@@ -2099,6 +2099,10 @@ ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
       return RETURN_VALUE_REGISTER_CONVENTION;
     }
 
+  if (!language_pass_by_reference (valtype).trivially_copyable
+      && valtype->code () == TYPE_CODE_STRUCT)
+    return RETURN_VALUE_STRUCT_CONVENTION;
+
   /* 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.37.2

Re: [PATCH 1/2 ver 3] PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

[Ulrich Weigand]

Carl Love <cel@us.ibm.com> wrote:

>-------------------------------
>PowerPC, function ppc64_sysv_abi_return_value add missing return value convention
>
>This patch address five testcase failures in gdb.cp/non-trivial-retval.exp.
>The following commit resulted in the five testcases failures on PowerPC.  The
>value returned by the function is being reported incorrectly.
>
>  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 function:
>
>  enum return_value_convention
>  ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value *function,
>                               struct type *valtype, struct regcache *regcache,
>                               gdb_byte *readbuf, const gdb_byte *writebuf)
>
>should return RETURN_VALUE_STRUCT_CONVENTION if the valtype->code() is
>TYPE_CODE_STRUCT and if the language_pass_by_reference is not
>trivially_copyable.
>
>This patch adds the need code to return the value
>RETURN_VALUE_STRUCT_CONVENTION in the case of this case.

This still has the typos Kevin pointed out.

>With this patch, the five test cases still fail but with the message "Value
>returned has type: A. Cannot determine contents".  The PowerPC ABI stores the
>address of the buffer containing the function return value in register r3 on
>entry to the function.  However, the PowerPC ABI does not guarentee that r3
>will not be modified in the function.  So when the function returns, the return
>buffer address cannot be reliably obtained from register r3.  Thus the message
>"Cannot determine contents" is appropriate in this case.

OK with the above typos fixed.

Bye,
Ulrich

Re: [PATCH 1/2 ver 3] PowerPC, function ppc64_sysv_abi_return_value add missing return value convention

[Carl Love]

On Mon, 2022-11-14 at 16:45 +0000, Ulrich Weigand wrote:
> Carl Love <cel@us.ibm.com> wrote:
> 
> > -------------------------------
> > PowerPC, function ppc64_sysv_abi_return_value add missing return
> > value convention
> > 
> > This patch address five testcase failures in gdb.cp/non-trivial-
> > retval.exp.
> > The following commit resulted in the five testcases failures on
> > PowerPC.  The
> > value returned by the function is being reported incorrectly.
> > 
> >  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 function:
> > 
> >  enum return_value_convention
> >  ppc64_sysv_abi_return_value (struct gdbarch *gdbarch, struct value
> > *function,
> >                               struct type *valtype, struct regcache
> > *regcache,
> >                               gdb_byte *readbuf, const gdb_byte
> > *writebuf)
> > 
> > should return RETURN_VALUE_STRUCT_CONVENTION if the valtype->code() 
> > is
> > TYPE_CODE_STRUCT and if the language_pass_by_reference is not
> > trivially_copyable.
> > 
> > This patch adds the need code to return the value
> > RETURN_VALUE_STRUCT_CONVENTION in the case of this case.
> 
> This still has the typos Kevin pointed out.
> 
> > With this patch, the five test cases still fail but with the
> > message "Value
> > returned has type: A. Cannot determine contents".  The PowerPC ABI
> > stores the
> > address of the buffer containing the function return value in
> > register r3 on
> > entry to the function.  However, the PowerPC ABI does not guarentee
> > that r3
> > will not be modified in the function.  So when the function
> > returns, the return
> > buffer address cannot be reliably obtained from register r3.  Thus
> > the message
> > "Cannot determine contents" is appropriate in this case.
> 
> OK with the above typos fixed.
> 

Yes, I had fixed the typos once.  But it seems I didn't get the patch
updated on all of the systems where I was testing/developing the patch
and inadvertently lost the updates to this patch. 

I have fixed the typos again and updated the patch on the various
test/development systems.

Sorry about that.

                                  Carl