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

Message ID 71926c391f43cee2051ea0c9b449ec0aecc847ec.camel@us.ibm.com
State Committed
Commit f68eca29d3b3e551a45a1bb02b05a0b3a4856f59
Headers
Series PowerPC, fix gdb.base/retval-large-struct.exp |

Commit Message

Carl Love Nov. 16, 2022, 10:11 p.m. UTC
  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(-)
  

Comments

Ulrich Weigand Nov. 17, 2022, 12:57 p.m. UTC | #1
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
  
Carl Love Nov. 17, 2022, 4:18 p.m. UTC | #2
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
  
Tom de Vries Nov. 18, 2022, 2:46 p.m. UTC | #3
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
  
Ulrich Weigand Nov. 18, 2022, 4:04 p.m. UTC | #4
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
  
Carl Love Nov. 18, 2022, 4:14 p.m. UTC | #5
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
  
Tom de Vries Nov. 18, 2022, 4:25 p.m. UTC | #6
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
  
Carl Love Nov. 18, 2022, 7:11 p.m. UTC | #7
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
  
Tom de Vries Nov. 19, 2022, 6:42 a.m. UTC | #8
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
>
  
Ulrich Weigand Nov. 23, 2022, 12:44 p.m. UTC | #9
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
  

Patch

diff --git a/gdb/testsuite/gdb.base/retval-large-struct.exp b/gdb/testsuite/gdb.base/retval-large-struct.exp
index 11bc4d5fbdf..f7aa1df81fa 100644
--- a/gdb/testsuite/gdb.base/retval-large-struct.exp
+++ b/gdb/testsuite/gdb.base/retval-large-struct.exp
@@ -20,7 +20,13 @@ 
 
 standard_testfile
 
-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
 }