[gdb/tdep] Fix gdb.base/finish-pretty.exp on s390x

  On s390x-linux, with test-case gdb.base/finish-pretty.exp I ran into:
...
(gdb) finish
Run till exit from #0  foo () at finish-pretty.c:28
main () at finish-pretty.c:40
40	  return v.a + v.b;
Value returned has type: struct s. Cannot determine contents
(gdb) FAIL: $exp: finish foo prettyprinted function result
...

The function being finished is foo, which returns a value of type struct s.

The ABI [1] specifies:
- that the value is returned in a storage buffer allocated by the caller, and
- that the address of this buffer is passed as a hidden argument in r2.

GDB fails to print the value when finishing foo, because it doesn't know the
address of the buffer.

Implement the gdbarch_get_return_buf_addr hook for s390x to fix this.

This is based on ppc_sysv_get_return_buf_addr, the only other implementation
of gdbarch_get_return_buf_addr.  For readability I've factored out
dwarf_reg_on_entry.

There is one difference with ppc_sysv_get_return_buf_addr: only
NO_ENTRY_VALUE_ERROR is caught.  If this patch is approved, I intend to submit
a follow-up patch to fix this in ppc_sysv_get_return_buf_addr as well.

The hook is not guaranteed to work, because it attempts to get the value r2
had at function entry.

The hook can be called after function entry, and the ABI doesn't guarantee
that r2 is the same throughout the function.

Using -fvar-tracking adds debug information, which allows the hook to succeed
more often, and indeed after adding this to the test-case, it passes.

Do likewise in one more test-case.

Tested on s390x-linux.

Fixes:
- gdb.ada/finish-large.exp
- gdb.base/finish-pretty.exp
- gdb.base/retval-large-struct.exp
- gdb.cp/non-trivial-retval.exp
- gdb.ada/array_return.exp

AFAICT, I've also enabled the hook for s390 and from the ABI [1] I get the
impression that it should work, but I haven't been able to test it.

[1] https://refspecs.linuxbase.org/ELF/zSeries/lzsabi0_zSeries.html
[2] https://uclibc.org/docs/psABI-s390.pdf
---
 gdb/s390-tdep.c                          | 58 ++++++++++++++++++++++++
 gdb/testsuite/gdb.ada/array_return.exp   |  8 +++-
 gdb/testsuite/gdb.base/finish-pretty.exp |  8 +++-
 3 files changed, 72 insertions(+), 2 deletions(-)


base-commit: a3011beced048e66d200921b2a11c836fae31abf
  

Hello Tom,

On Fri, Dec 06 2024, Tom de Vries wrote:

> On s390x-linux, with test-case gdb.base/finish-pretty.exp I ran into:
> ...
> (gdb) finish
> Run till exit from #0  foo () at finish-pretty.c:28
> main () at finish-pretty.c:40
> 40	  return v.a + v.b;
> Value returned has type: struct s. Cannot determine contents
> (gdb) FAIL: $exp: finish foo prettyprinted function result
> ...
>
> The function being finished is foo, which returns a value of type struct s.
>
> The ABI [1] specifies:
> - that the value is returned in a storage buffer allocated by the caller, and
> - that the address of this buffer is passed as a hidden argument in r2.
>
> GDB fails to print the value when finishing foo, because it doesn't know the
> address of the buffer.
>
> Implement the gdbarch_get_return_buf_addr hook for s390x to fix this.

That's fantastic!  Thanks for doing this!

> This is based on ppc_sysv_get_return_buf_addr, the only other implementation
> of gdbarch_get_return_buf_addr.  For readability I've factored out
> dwarf_reg_on_entry.
>
> There is one difference with ppc_sysv_get_return_buf_addr: only
> NO_ENTRY_VALUE_ERROR is caught.  If this patch is approved, I intend to submit
> a follow-up patch to fix this in ppc_sysv_get_return_buf_addr as well.
>
> The hook is not guaranteed to work, because it attempts to get the value r2
> had at function entry.
>
> The hook can be called after function entry, and the ABI doesn't guarantee
> that r2 is the same throughout the function.
>
> Using -fvar-tracking adds debug information, which allows the hook to succeed
> more often, and indeed after adding this to the test-case, it passes.

OK.  I think that's acceptable.

> Do likewise in one more test-case.
>
> Tested on s390x-linux.
>
> Fixes:
> - gdb.ada/finish-large.exp
> - gdb.base/finish-pretty.exp
> - gdb.base/retval-large-struct.exp
> - gdb.cp/non-trivial-retval.exp
> - gdb.ada/array_return.exp
>
> AFAICT, I've also enabled the hook for s390 and from the ABI [1] I get the
> impression that it should work, but I haven't been able to test it.

That's perfectly fine.  Establishing a test environment for s390 has
become increasingly difficult.

> [1] https://refspecs.linuxbase.org/ELF/zSeries/lzsabi0_zSeries.html

Please use the updated version of the s390x ABI:

  https://github.com/IBM/s390x-abi

It contains various fixes, as well as some new stuff like vector
registers.

> [2] https://uclibc.org/docs/psABI-s390.pdf

For that one no updated PDF is published.  It can be built from the new
sources, if needed.

> ---
>  gdb/s390-tdep.c                          | 58 ++++++++++++++++++++++++
>  gdb/testsuite/gdb.ada/array_return.exp   |  8 +++-
>  gdb/testsuite/gdb.base/finish-pretty.exp |  8 +++-
>  3 files changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
> index be176f07c6f..3c4ae5abdb3 100644
> --- a/gdb/s390-tdep.c
> +++ b/gdb/s390-tdep.c
> @@ -40,6 +40,7 @@
>  #include "trad-frame.h"
>  #include "value.h"
>  #include "inferior.h"
> +#include "dwarf2/loc.h"
>  
>  #include "features/s390-linux32.c"
>  #include "features/s390x-linux64.c"
> @@ -2119,6 +2120,62 @@ s390_return_value (struct gdbarch *gdbarch, struct value *function,
>    return rvc;
>  }
>  
> +/* Try to get the value of DWARF_REG in FRAME at function entry.  If successful,
> +   return it as value of type VAL_TYPE.  */
> +
> +static struct value *
> +dwarf_reg_on_entry (int dwarf_reg, struct type *val_type,
> +		    const frame_info_ptr &frame)
> +{
> +  enum call_site_parameter_kind kind = CALL_SITE_PARAMETER_DWARF_REG;
> +  union call_site_parameter_u kind_u = { .dwarf_reg = dwarf_reg };
> +
> +  try
> +    {
> +      return value_of_dwarf_reg_entry (val_type, frame, kind, kind_u);
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      if (e.error == NO_ENTRY_VALUE_ERROR)
> +	return nullptr;
> +
> +      throw;
> +    }
> +}

I like factoring this out.  It seems that this wrapper is only needed
because value_of_dwarf_reg_entry() has an unusable interface.  Maybe we
want to fix that in the long run?

> +
> +/* Both the 32-bit and 64-bit ABIs specify that values of some types are
> +   returned in a storage buffer provided by the caller.  Return the address of
> +   that storage buffer, if possible.  Implements the
> +   gdbarch_get_return_buf_addr hook.  */
> +
> +static CORE_ADDR
> +s390_get_return_buf_addr (struct type *val_type,
> +			  const frame_info_ptr &cur_frame)
> +{
> +  /* The address of the storage buffer is provided as a hidden argument in
> +     register r2.  */
> +  int dwarf_reg = 2;
> +
> +  /* The ABI does not guarantee that the register will not be changed while
> +     executing the function.  Hence, it cannot be assumed that it will still
> +     contain the address of the storage buffer when execution reaches the end
> +     of the function.
> +
> +     Attempt to determine the value on entry using the DW_OP_entry_value DWARF
> +     entries.  This requires compiling the user program with -fvar-tracking.  */
> +  struct value *val_on_entry
> +    = dwarf_reg_on_entry (dwarf_reg, lookup_pointer_type (val_type), cur_frame);
> +
> +  if (val_on_entry == nullptr)
> +    {
> +      warning ("Cannot determine the function return value.\n"
> +	       "Try compiling with -fvar-tracking.");
> +      return 0;
> +    }
> +
> +  return value_as_address (val_on_entry);
> +}
> +
>  /* Frame unwinding.  */
>  
>  /* Implement the stack_frame_destroyed_p gdbarch method.  */
> @@ -7183,6 +7240,7 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>    set_gdbarch_dummy_id (gdbarch, s390_dummy_id);
>    set_gdbarch_frame_align (gdbarch, s390_frame_align);
>    set_gdbarch_return_value (gdbarch, s390_return_value);
> +  set_gdbarch_get_return_buf_addr (gdbarch, s390_get_return_buf_addr);
>  
>    /* Frame handling.  */
>    /* Stack grows downward.  */
> diff --git a/gdb/testsuite/gdb.ada/array_return.exp b/gdb/testsuite/gdb.ada/array_return.exp
> index c6edee11f17..d1fc2ac2c98 100644
> --- a/gdb/testsuite/gdb.ada/array_return.exp
> +++ b/gdb/testsuite/gdb.ada/array_return.exp
> @@ -19,7 +19,13 @@ require allow_ada_tests
>  
>  standard_ada_testfile p
>  
> -if {[gdb_compile_ada "${srcfile}" "${binfile}" executable {debug}] != ""} {
> +set opts {}
> +lappend opts debug
> +if { [have_fvar_tracking] } {
> +    lappend opts "additional_flags=-fvar-tracking"
> +}
> +
> +if {[gdb_compile_ada "${srcfile}" "${binfile}" executable $opts] != ""} {
>      return -1
>  }
>  
> diff --git a/gdb/testsuite/gdb.base/finish-pretty.exp b/gdb/testsuite/gdb.base/finish-pretty.exp
> index 44f3340f41c..0b6bea6681d 100644
> --- a/gdb/testsuite/gdb.base/finish-pretty.exp
> +++ b/gdb/testsuite/gdb.base/finish-pretty.exp
> @@ -18,7 +18,13 @@
>  
>  standard_testfile
>  
> -if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
> +set opts {}
> +lappend opts debug
> +if { [have_fvar_tracking] } {
> +    lappend opts "additional_flags=-fvar-tracking"
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] } {
>      return -1
>  }
>  
>
> base-commit: a3011beced048e66d200921b2a11c836fae31abf

LGTM.  We might still need someone to approve the changes to the test
cases, though.
  

On 12/17/24 19:49, Andreas Arnez wrote:
> Hello Tom,
> 
> On Fri, Dec 06 2024, Tom de Vries wrote:
> 
>> On s390x-linux, with test-case gdb.base/finish-pretty.exp I ran into:
>> ...
>> (gdb) finish
>> Run till exit from #0  foo () at finish-pretty.c:28
>> main () at finish-pretty.c:40
>> 40	  return v.a + v.b;
>> Value returned has type: struct s. Cannot determine contents
>> (gdb) FAIL: $exp: finish foo prettyprinted function result
>> ...
>>
>> The function being finished is foo, which returns a value of type struct s.
>>
>> The ABI [1] specifies:
>> - that the value is returned in a storage buffer allocated by the caller, and
>> - that the address of this buffer is passed as a hidden argument in r2.
>>
>> GDB fails to print the value when finishing foo, because it doesn't know the
>> address of the buffer.
>>
>> Implement the gdbarch_get_return_buf_addr hook for s390x to fix this.
> 
> That's fantastic!  Thanks for doing this!
> 

Hi Andreas,

thanks for the review.

>> This is based on ppc_sysv_get_return_buf_addr, the only other implementation
>> of gdbarch_get_return_buf_addr.  For readability I've factored out
>> dwarf_reg_on_entry.
>>
>> There is one difference with ppc_sysv_get_return_buf_addr: only
>> NO_ENTRY_VALUE_ERROR is caught.  If this patch is approved, I intend to submit
>> a follow-up patch to fix this in ppc_sysv_get_return_buf_addr as well.
>>
>> The hook is not guaranteed to work, because it attempts to get the value r2
>> had at function entry.
>>
>> The hook can be called after function entry, and the ABI doesn't guarantee
>> that r2 is the same throughout the function.
>>
>> Using -fvar-tracking adds debug information, which allows the hook to succeed
>> more often, and indeed after adding this to the test-case, it passes.
> 
> OK.  I think that's acceptable.
> 
 >> Do likewise in one more test-case.>>
>> Tested on s390x-linux.
>>
>> Fixes:
>> - gdb.ada/finish-large.exp
>> - gdb.base/finish-pretty.exp
>> - gdb.base/retval-large-struct.exp
>> - gdb.cp/non-trivial-retval.exp
>> - gdb.ada/array_return.exp
>>
>> AFAICT, I've also enabled the hook for s390 and from the ABI [1] I get the
>> impression that it should work, but I haven't been able to test it.
> 
> That's perfectly fine.  Establishing a test environment for s390 has
> become increasingly difficult.
> 
>> [1] https://refspecs.linuxbase.org/ELF/zSeries/lzsabi0_zSeries.html
> 
> Please use the updated version of the s390x ABI:
> 
>    https://github.com/IBM/s390x-abi
> 

Done.

> It contains various fixes, as well as some new stuff like vector
> registers.
> 

Thanks, it's good to have updated information.

>> [2] https://uclibc.org/docs/psABI-s390.pdf
> 
> For that one no updated PDF is published.  It can be built from the new
> sources, if needed.
> 

I've managed to build both pdfs now, thanks.  I wouldn't mind if the 
s390 one would got published though.


>> ---
>>   gdb/s390-tdep.c                          | 58 ++++++++++++++++++++++++
>>   gdb/testsuite/gdb.ada/array_return.exp   |  8 +++-
>>   gdb/testsuite/gdb.base/finish-pretty.exp |  8 +++-
>>   3 files changed, 72 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
>> index be176f07c6f..3c4ae5abdb3 100644
>> --- a/gdb/s390-tdep.c
>> +++ b/gdb/s390-tdep.c
>> @@ -40,6 +40,7 @@
>>   #include "trad-frame.h"
>>   #include "value.h"
>>   #include "inferior.h"
>> +#include "dwarf2/loc.h"
>>   
>>   #include "features/s390-linux32.c"
>>   #include "features/s390x-linux64.c"
>> @@ -2119,6 +2120,62 @@ s390_return_value (struct gdbarch *gdbarch, struct value *function,
>>     return rvc;
>>   }
>>   
>> +/* Try to get the value of DWARF_REG in FRAME at function entry.  If successful,
>> +   return it as value of type VAL_TYPE.  */
>> +
>> +static struct value *
>> +dwarf_reg_on_entry (int dwarf_reg, struct type *val_type,
>> +		    const frame_info_ptr &frame)
>> +{
>> +  enum call_site_parameter_kind kind = CALL_SITE_PARAMETER_DWARF_REG;
>> +  union call_site_parameter_u kind_u = { .dwarf_reg = dwarf_reg };
>> +
>> +  try
>> +    {
>> +      return value_of_dwarf_reg_entry (val_type, frame, kind, kind_u);
>> +    }
>> +  catch (const gdb_exception_error &e)
>> +    {
>> +      if (e.error == NO_ENTRY_VALUE_ERROR)
>> +	return nullptr;
>> +
>> +      throw;
>> +    }
>> +}
> 
> I like factoring this out.  It seems that this wrapper is only needed
> because value_of_dwarf_reg_entry() has an unusable interface.  Maybe we
> want to fix that in the long run?
> 

My guess is that the interface is appropriate for usage in the dwarf 
expression evaluator, just not for this type of usage.

>> +
>> +/* Both the 32-bit and 64-bit ABIs specify that values of some types are
>> +   returned in a storage buffer provided by the caller.  Return the address of
>> +   that storage buffer, if possible.  Implements the
>> +   gdbarch_get_return_buf_addr hook.  */
>> +
>> +static CORE_ADDR
>> +s390_get_return_buf_addr (struct type *val_type,
>> +			  const frame_info_ptr &cur_frame)
>> +{
>> +  /* The address of the storage buffer is provided as a hidden argument in
>> +     register r2.  */
>> +  int dwarf_reg = 2;
>> +
>> +  /* The ABI does not guarantee that the register will not be changed while
>> +     executing the function.  Hence, it cannot be assumed that it will still
>> +     contain the address of the storage buffer when execution reaches the end
>> +     of the function.
>> +
>> +     Attempt to determine the value on entry using the DW_OP_entry_value DWARF
>> +     entries.  This requires compiling the user program with -fvar-tracking.  */
>> +  struct value *val_on_entry
>> +    = dwarf_reg_on_entry (dwarf_reg, lookup_pointer_type (val_type), cur_frame);
>> +
>> +  if (val_on_entry == nullptr)
>> +    {
>> +      warning ("Cannot determine the function return value.\n"
>> +	       "Try compiling with -fvar-tracking.");
>> +      return 0;
>> +    }
>> +
>> +  return value_as_address (val_on_entry);
>> +}
>> +
>>   /* Frame unwinding.  */
>>   
>>   /* Implement the stack_frame_destroyed_p gdbarch method.  */
>> @@ -7183,6 +7240,7 @@ s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>     set_gdbarch_dummy_id (gdbarch, s390_dummy_id);
>>     set_gdbarch_frame_align (gdbarch, s390_frame_align);
>>     set_gdbarch_return_value (gdbarch, s390_return_value);
>> +  set_gdbarch_get_return_buf_addr (gdbarch, s390_get_return_buf_addr);
>>   
>>     /* Frame handling.  */
>>     /* Stack grows downward.  */
>> diff --git a/gdb/testsuite/gdb.ada/array_return.exp b/gdb/testsuite/gdb.ada/array_return.exp
>> index c6edee11f17..d1fc2ac2c98 100644
>> --- a/gdb/testsuite/gdb.ada/array_return.exp
>> +++ b/gdb/testsuite/gdb.ada/array_return.exp
>> @@ -19,7 +19,13 @@ require allow_ada_tests
>>   
>>   standard_ada_testfile p
>>   
>> -if {[gdb_compile_ada "${srcfile}" "${binfile}" executable {debug}] != ""} {
>> +set opts {}
>> +lappend opts debug
>> +if { [have_fvar_tracking] } {
>> +    lappend opts "additional_flags=-fvar-tracking"
>> +}
>> +
>> +if {[gdb_compile_ada "${srcfile}" "${binfile}" executable $opts] != ""} {
>>       return -1
>>   }
>>   
>> diff --git a/gdb/testsuite/gdb.base/finish-pretty.exp b/gdb/testsuite/gdb.base/finish-pretty.exp
>> index 44f3340f41c..0b6bea6681d 100644
>> --- a/gdb/testsuite/gdb.base/finish-pretty.exp
>> +++ b/gdb/testsuite/gdb.base/finish-pretty.exp
>> @@ -18,7 +18,13 @@
>>   
>>   standard_testfile
>>   
>> -if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
>> +set opts {}
>> +lappend opts debug
>> +if { [have_fvar_tracking] } {
>> +    lappend opts "additional_flags=-fvar-tracking"
>> +}
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] } {
>>       return -1
>>   }
>>   
>>
>> base-commit: a3011beced048e66d200921b2a11c836fae31abf
> 
> LGTM.  We might still need someone to approve the changes to the test
> cases, though.
> 

The changes in the test-cases are trivial, low-impact, and have 
precedent in similar changes for ppc.  So I think self-approving those 
will be non-controversial.

I'll send a v2 with the updated link.

[ FWIW, you could indicate your formal approval for just the tdep part 
by replying with a tag like this:
...
Approved-By: Andreas Arnez <arnez@linux.ibm.com> (s390x)
... ]

Thanks,
- Tom