PowerPC, fix gdb.base/retval-large-struct.exp

  GDB maintainers:

Currently the test gdb.base/retval-large-struct.exp is not able to
access the return buffer address for the data being returned by the
function.  The functionality needed to obtain the value of r3 which
holds the return buffer address on entry to the function was recently
added in a new gdb method.  This new method can be used by this test to
allow it to correctly access the data being returned by the function.

This patch adds the needed command line argument to the test compile
line thus allowing GDB to access the data via the return buffer address
stored in register r3.  

The patch has been tested on Power 10 with no additional regressions.

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

                       Carl 


-----------------------------------
PowerPC, fix gdb.base/retval-large-struct.exp

Support for printining non-trivial return values was recently added in
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.

The functionality can no be used by test fix gdb.base/retval-large-struct.exp.
The test just needs to be compiled with -fvar-tracking to enable GDB to
determine the address off the return buffere when the function is called.

The current output from the test:

34        return big_struct;
(gdb) PASS: gdb.base/retval-large-struct.exp: continue to breakpoint: Break in print_large_struct
finish
warning: Cannot determine the function return value.
Try compiling with -fvar-tracking.
Run till exit from #0  return_large_struct () at binutils-gdb-current/gdb/testsuite/gdb.base/retval-large-struct.c:34
main (argc=1, argv=0x7fffffffcd58) at binutils-gdb-current/gdb/testsuite/gdb.base/retval-large-struct.c:44
44        return 0;^M
Value returned has type: struct big_struct_t. Cannot determine contents
(gdb) FAIL: gdb.base/retval-large-struct.exp: finish from return_large_struct
testcase binutils-gdb-current/gdb/testsuite/gdb.base/retval-large-struct.exp completed in 1 seconds

This patch adds the command line argument -fvar-tracking to enable gdb to
determine the return vaule and thus fixing the test.

Patch tested on Power 10 with no regressions.
---
 gdb/testsuite/gdb.base/retval-large-struct.exp | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
  

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

>Currently the test gdb.base/retval-large-struct.exp is not able to
>access the return buffer address for the data being returned by the
>function.  The functionality needed to obtain the value of r3 which
>holds the return buffer address on entry to the function was recently
>added in a new gdb method.  This new method can be used by this test to
>allow it to correctly access the data being returned by the function.

Are there any more test cases that should get -fvar-tracking now?

In the long run, it might be good to work with GCC developers to see
if we can't have entry value DWARF records emitted (at least for
simple cases like the return buffer address) even without variable
tracking ...

In any case, for now this patch certainly seems reasonable.

>PowerPC, fix gdb.base/retval-large-struct.exp

This is OK.

Thanks,
Ulrich
  

Ulrich:

On Thu, 2022-11-17 at 12:57 +0000, Ulrich Weigand wrote:
> > Currently the test gdb.base/retval-large-struct.exp is not able to
> > access the return buffer address for the data being returned by the
> > function.  The functionality needed to obtain the value of r3 which
> > holds the return buffer address on entry to the function was
> > recently
> > added in a new gdb method.  This new method can be used by this
> > test to
> > allow it to correctly access the data being returned by the
> > function.
> 
> Are there any more test cases that should get -fvar-tracking now?
> 
> In the long run, it might be good to work with GCC developers to see
> if we can't have entry value DWARF records emitted (at least for
> simple cases like the return buffer address) even without variable
> tracking ...

At the moment, this is the only one I know of.  The number of test
failures on Power is getting relatively short. There are about 15ish
tests that are currently failing on Power 10.  I have the list and have
been adding notes for each test as to what the issue is or is fixed by
a patch under development.  I have a couple of patches that have been
posted and approved and a couple that I am working on still.  Let me
refresh the list and my notes and I can send it out so you can see
where we are at with the regression test clean up.

                           Carl
  

On 11/16/22 23:11, Carl Love via Gdb-patches wrote:
> -if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
> +set additional_flags ""
> +
> +if {[have_fvar_tracking]} {
> +    set additional_flags "additional_flags= -fvar-tracking"
> +}
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile [list debug $additional_flags]]} {
>       return -1
>   }
>   

AFAIU, needing -fvar-tracking is specific to powerpc, so we should limit 
it's impact to that target.

And it's a gcc compiler flag, so perhaps we should limit it's impact to 
that as well.

And if we indeed need it but it's not available, we should skip the test 
(or xfail the failing bit), because we cannot expect it to succeed.

So how about something like this:
...
set flags {}

lappend flags debug

if { [istarget powerpc*-*-*] && [is_c_compiler_gcc] } {
     if { [have_fvar_tracking] } {
         lappend flags -fvar-tracking
     } else {
         unsupported "gcc used, -fvar-tracking needed"
         return -1
     }
}

if {[prepare_for_testing "failed to prepare" $testfile $srcfile $flags]} {
        return -1
}
...

Thanks,
- Tom
  

Tom de Vries <tdevries@suse.de> wrote:

>AFAIU, needing -fvar-tracking is specific to powerpc, so we should limit 
>it's impact to that target.
>
>And it's a gcc compiler flag, so perhaps we should limit it's impact to 
>that as well.

No, it's not really powerpc specific - the same mechanism can be
used on many other platforms with an ABI that uses a return buffer
address that is not preserved.  (E.g. we're currently looking into
enabling it on s390x.)

And given that the flag is harmless if it's available (which the
test verifies), I think it makes sense to just always enable it.

(In fact, I think the compiler should really provide DWARF entry
value records -for simple cases- always, even if -fvar-tracking
is not enabled.  That would make the problem go away.)

Bye,
Ulrich
  

Tom, Ulrich:

On Fri, 2022-11-18 at 16:04 +0000, Ulrich Weigand wrote:
> Tom de Vries <tdevries@suse.de> wrote:
> 
> > AFAIU, needing -fvar-tracking is specific to powerpc, so we should
> > limit 
> > it's impact to that target.
> > 
> > And it's a gcc compiler flag, so perhaps we should limit it's
> > impact to 
> > that as well.
> 
> No, it's not really powerpc specific - the same mechanism can be
> used on many other platforms with an ABI that uses a return buffer
> address that is not preserved.  (E.g. we're currently looking into
> enabling it on s390x.)
> 
> And given that the flag is harmless if it's available (which the
> test verifies), I think it makes sense to just always enable it.
> 
> (In fact, I think the compiler should really provide DWARF entry
> value records -for simple cases- always, even if -fvar-tracking
> is not enabled.  That would make the problem go away.)

I have run the testcase on Intel X86-64 and it does run successfully
there.

                 Carl
  

On 11/18/22 17:04, Ulrich Weigand wrote:
> Tom de Vries <tdevries@suse.de> wrote:
> 
>> AFAIU, needing -fvar-tracking is specific to powerpc, so we should limit
>> it's impact to that target.
>>
>> And it's a gcc compiler flag, so perhaps we should limit it's impact to
>> that as well.
> 
> No, it's not really powerpc specific - the same mechanism can be
> used on many other platforms with an ABI that uses a return buffer
> address that is not preserved.  (E.g. we're currently looking into
> enabling it on s390x.)
> 

Right, so we can add those archs to the list of archs requiring 
-fvar-tracking support when gcc is used.

> And given that the flag is harmless if it's available (which the
> test verifies), I think it makes sense to just always enable it.
> 

That's fine by me.

Then I get:
...
set flags {}

lappend flags debug

if { [have_fvar_tracking] } {
     lappend flags -fvar-tracking
} else {
     if { ( [istarget powerpc*-*-*] || [istarget s390x*-*-* ])
          && [is_c_compiler_gcc] } {
         unsupported "gcc used, -fvar-tracking needed"
         return -1
     }
}

if {[prepare_for_testing "failed to prepare" $testfile $srcfile $flags]} {
        return -1
}
...

Thanks,
- Tom
  

Tom:

On Fri, 2022-11-18 at 17:25 +0100, Tom de Vries wrote:
> On 11/18/22 17:04, Ulrich Weigand wrote:
> > Tom de Vries <tdevries@suse.de> wrote:
> > 
> > > AFAIU, needing -fvar-tracking is specific to powerpc, so we
> > > should limit
> > > it's impact to that target.
> > > 
> > > And it's a gcc compiler flag, so perhaps we should limit it's
> > > impact to
> > > that as well.
> > 
> > No, it's not really powerpc specific - the same mechanism can be
> > used on many other platforms with an ABI that uses a return buffer
> > address that is not preserved.  (E.g. we're currently looking into
> > enabling it on s390x.)
> > 
> 
> Right, so we can add those archs to the list of archs requiring 
> -fvar-tracking support when gcc is used.
> 
> > And given that the flag is harmless if it's available (which the
> > test verifies), I think it makes sense to just always enable it.
> > 
> 
> That's fine by me.
> 
> Then I get:
> ...
> set flags {}
> 
> lappend flags debug
> 
> if { [have_fvar_tracking] } {
>      lappend flags -fvar-tracking
> } else {
>      if { ( [istarget powerpc*-*-*] || [istarget s390x*-*-* ])
>           && [is_c_compiler_gcc] } {
>          unsupported "gcc used, -fvar-tracking needed"
>          return -1
>      }
> }
> 
> if {[prepare_for_testing "failed to prepare" $testfile $srcfile
> $flags]} {
>         return -1
> }

The [have_fvar_tracking] does a runtime test to see if the compiler
supports the -fvar-tracking option.  So if it passes, then the option
is supported.  The test makes the additional check  [istarget powerpc*-
*-*] || [istarget s390x*-*-* ]) && [is_c_compiler_gcc]  redundant.  

I don't think there is any need for the additional checks.

                  Carl
  

On 11/18/22 20:11, Carl Love wrote:
> Tom:
> 
> On Fri, 2022-11-18 at 17:25 +0100, Tom de Vries wrote:
>> On 11/18/22 17:04, Ulrich Weigand wrote:
>>> Tom de Vries <tdevries@suse.de> wrote:
>>>
>>>> AFAIU, needing -fvar-tracking is specific to powerpc, so we
>>>> should limit
>>>> it's impact to that target.
>>>>
>>>> And it's a gcc compiler flag, so perhaps we should limit it's
>>>> impact to
>>>> that as well.
>>>
>>> No, it's not really powerpc specific - the same mechanism can be
>>> used on many other platforms with an ABI that uses a return buffer
>>> address that is not preserved.  (E.g. we're currently looking into
>>> enabling it on s390x.)
>>>
>>
>> Right, so we can add those archs to the list of archs requiring
>> -fvar-tracking support when gcc is used.
>>
>>> And given that the flag is harmless if it's available (which the
>>> test verifies), I think it makes sense to just always enable it.
>>>
>>
>> That's fine by me.
>>
>> Then I get:
>> ...
>> set flags {}
>>
>> lappend flags debug
>>
>> if { [have_fvar_tracking] } {
>>       lappend flags -fvar-tracking
>> } else {
>>       if { ( [istarget powerpc*-*-*] || [istarget s390x*-*-* ])
>>            && [is_c_compiler_gcc] } {
>>           unsupported "gcc used, -fvar-tracking needed"
>>           return -1
>>       }
>> }
>>
>> if {[prepare_for_testing "failed to prepare" $testfile $srcfile
>> $flags]} {
>>          return -1
>> }
> 
> The [have_fvar_tracking] does a runtime test to see if the compiler
> supports the -fvar-tracking option.  So if it passes, then the option
> is supported.

Agreed.

> The test makes the additional check  [istarget powerpc*-
> *-*] || [istarget s390x*-*-* ]) && [is_c_compiler_gcc]  redundant.
> 

My concern is older gcc compilers that do not support -fvar-tracking. 
Those have no chance of succeeding, so we should bail out for those.

OTOH, non-gcc compilers may or may not provide sufficient debug 
information, so there's no need to bail out for those.

Thanks,
- Tom

> I don't think there is any need for the additional checks.
> 
>                    Carl
>
  

Tom de Vries <tdevries@suse.de> wrote:

>My concern is older gcc compilers that do not support -fvar-tracking. 
>Those have no chance of succeeding, so we should bail out for those.
>
>OTOH, non-gcc compilers may or may not provide sufficient debug 
>information, so there's no need to bail out for those.

I'm skeptical of trying to determine in advance whether or not
the current platform supports the required entry-value records.
This not only leads to rather complicated logic in the test, but
that logic isn't actually accurate anyway ...

Note that entry-value records are only loosely associated with
the -fvar-tracking option.  As you say, compilers are free to
produce entry-value records without that option (even GCC only
requires that option at -O0).  On the other hand, there are
versions of GCC that support -fvar-tracking, but *still* do not
generate entry-value records (-fvar-tracking was added in 2004,
but entry-value records only in 2011).

I personally would be fine with just having the test fail then
(I mean, the feature actually does fail).  Or in the alternative,
recognize the scenario after the fact - the ppc back-end will
emit an error message if it cannot determine the return value
due to lack of entry-value records, so the test case might
recognize that message and downgrade from a FAIL to an
UNSUPPORTED state based on that.

Bye,
Ulrich
  

>>>>> "Ulrich" == Ulrich Weigand via Gdb-patches <gdb-patches@sourceware.org> writes:

Ulrich> No, it's not really powerpc specific - the same mechanism can be
Ulrich> used on many other platforms with an ABI that uses a return buffer
Ulrich> address that is not preserved.  (E.g. we're currently looking into
Ulrich> enabling it on s390x.)

I am curious if it makes sense to enable for 32-bit PPC.

That is, the appended seems to help with a "large struct return" test
case we have internally, but I don't know the PPC ABI well enough to
know if it's really correct, or if it's just working by accident.

Tom

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 261ce77345e..218fb35efed 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -8249,7 +8249,11 @@ rs6000_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
 				       ppc64_sysv_get_return_buf_addr);
     }
   else
-    set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
+    {
+      set_gdbarch_return_value (gdbarch, ppc_sysv_abi_return_value);
+      set_gdbarch_get_return_buf_addr (gdbarch,
+				       ppc64_sysv_get_return_buf_addr);
+    }
 
   /* Set lr_frame_offset.  */
   if (wordsize == 8)
  

Tom Tromey <tromey@adacore.com> wrote:
>Ulrich> No, it's not really powerpc specific - the same mechanism can be
>Ulrich> used on many other platforms with an ABI that uses a return buffer
>Ulrich> address that is not preserved.  (E.g. we're currently looking into
>Ulrich> enabling it on s390x.)
>
>I am curious if it makes sense to enable for 32-bit PPC.

That would make sense, yes.  The 32-bit PPC ABI uses r3 as the
return address pointer just like the 64-bit ABIs.

>That is, the appended seems to help with a "large struct return" test
>case we have internally, but I don't know the PPC ABI well enough to
>know if it's really correct, or if it's just working by accident.

This looks good to me.  The only (cosmetic) nit might be that if
the function is installed for both 32-bit and 64-bit tdeps,
convention seems to be that the function should be called ppc_...
then, not ppc64_...

Bye,
Ulrich
  

Ulrich> This looks good to me.  The only (cosmetic) nit might be that if
Ulrich> the function is installed for both 32-bit and 64-bit tdeps,
Ulrich> convention seems to be that the function should be called ppc_...
Ulrich> then, not ppc64_...

Yep, I'll fix that in the real patch.  I think I'll do it internally
first so I can run it through the test suite, then send to gdb-patches.

thanks,
Tom