Change riscv_return_value to use RETURN_VALUE_ABI_PRESERVES_ADDRESS

Message ID 20220105172406.2308648-1-tromey@adacore.com
State New
Headers show
Series
  • Change riscv_return_value to use RETURN_VALUE_ABI_PRESERVES_ADDRESS
Related show

Commit Message

Simon Marchi via Gdb-patches Jan. 5, 2022, 5:24 p.m.
Internally, AdaCore has a test that is equivalent to (really a direct
translation of) gdb.base/gnu_vector.exp.  On 32-bit RISC-V, the
"return" part of this test fails.

Joel tracked this down to riscv_return_value returning
RETURN_VALUE_ABI_RETURNS_ADDRESS.  Using
RETURN_VALUE_ABI_PRESERVES_ADDRESS is more correct here, and fixes the
bug.

I don't have a good way to test this using dejagnu, but I tested it
for both 32- and 64-bit RISC-V using the AdaCore internal test suite.
---
 gdb/riscv-tdep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.31.1

Comments

Simon Marchi via Gdb-patches Jan. 5, 2022, 5:53 p.m. | #1
* Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> [2022-01-05 10:24:06 -0700]:

> Internally, AdaCore has a test that is equivalent to (really a direct

> translation of) gdb.base/gnu_vector.exp.  On 32-bit RISC-V, the

> "return" part of this test fails.

> 

> Joel tracked this down to riscv_return_value returning

> RETURN_VALUE_ABI_RETURNS_ADDRESS.  Using

> RETURN_VALUE_ABI_PRESERVES_ADDRESS is more correct here, and fixes the

> bug.

> 

> I don't have a good way to test this using dejagnu, but I tested it

> for both 32- and 64-bit RISC-V using the AdaCore internal test

> suite.


I'm confused.  Are you saying that with 32-bit risc-v the
gdb.base/gnu_vector.exp test passes just fine, but your internal test
fails?

The change itself is fine, but I'd like to see a test for this added
if the gnu_vector test isn't good enough.

[ NOTE: the RV tools I had around are pretty old and don't support the
  vector size stuff, I'm rebuilding the latest GCC to check this, but
  am unlikely to get back to this until tomorrow now. ]

Thanks,
Andrew


> ---

>  gdb/riscv-tdep.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c

> index 5a6269887e7..886996ce5b7 100644

> --- a/gdb/riscv-tdep.c

> +++ b/gdb/riscv-tdep.c

> @@ -3301,7 +3301,7 @@ riscv_return_value (struct gdbarch  *gdbarch,

>      case riscv_arg_info::location::in_reg:

>        return RETURN_VALUE_REGISTER_CONVENTION;

>      case riscv_arg_info::location::by_ref:

> -      return RETURN_VALUE_ABI_RETURNS_ADDRESS;

> +      return RETURN_VALUE_ABI_PRESERVES_ADDRESS;

>      case riscv_arg_info::location::on_stack:

>      default:

>        error (_("invalid argument location"));

> -- 

> 2.31.1

>
Simon Marchi via Gdb-patches Jan. 5, 2022, 6:06 p.m. | #2
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:


>> Internally, AdaCore has a test that is equivalent to (really a direct

>> translation of) gdb.base/gnu_vector.exp.  On 32-bit RISC-V, the

>> "return" part of this test fails.

>> 

>> Joel tracked this down to riscv_return_value returning

>> RETURN_VALUE_ABI_RETURNS_ADDRESS.  Using

>> RETURN_VALUE_ABI_PRESERVES_ADDRESS is more correct here, and fixes the

>> bug.

>> 

>> I don't have a good way to test this using dejagnu, but I tested it

>> for both 32- and 64-bit RISC-V using the AdaCore internal test

>> suite.


Andrew> I'm confused.  Are you saying that with 32-bit risc-v the
Andrew> gdb.base/gnu_vector.exp test passes just fine, but your internal test
Andrew> fails?

I've never been able to run the dejagnu tests for this target (or
actually anything other than native, at least not since the Cygnus
days).

The AdaCore internal test suite knows how to build using the appropriate
compiler (and BSP or whatever) and then run the program with qemu.  And,
the internal test suite has a translation of gnu_vector.exp.

Under this setup, the 64-bit RISC-V port passes this test, but the
32-bit port fails.

Andrew> The change itself is fine, but I'd like to see a test for this added
Andrew> if the gnu_vector test isn't good enough.

I think it ought to be, but I just don't know how to try it.  I'm happy
to give it a shot if you have a qemu board file or the like to try out.

Tom
Simon Marchi via Gdb-patches Jan. 5, 2022, 6:48 p.m. | #3
* Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> [2022-01-05 11:06:10 -0700]:

> >>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

> 

> >> Internally, AdaCore has a test that is equivalent to (really a direct

> >> translation of) gdb.base/gnu_vector.exp.  On 32-bit RISC-V, the

> >> "return" part of this test fails.

> >> 

> >> Joel tracked this down to riscv_return_value returning

> >> RETURN_VALUE_ABI_RETURNS_ADDRESS.  Using

> >> RETURN_VALUE_ABI_PRESERVES_ADDRESS is more correct here, and fixes the

> >> bug.

> >> 

> >> I don't have a good way to test this using dejagnu, but I tested it

> >> for both 32- and 64-bit RISC-V using the AdaCore internal test

> >> suite.

> 

> Andrew> I'm confused.  Are you saying that with 32-bit risc-v the

> Andrew> gdb.base/gnu_vector.exp test passes just fine, but your internal test

> Andrew> fails?

> 

> I've never been able to run the dejagnu tests for this target (or

> actually anything other than native, at least not since the Cygnus

> days).


The risc-v testing is indeed a mess.  For ages I maintained a RV sim
out of tree, which would merge cleanly, then we could test using the
simulator.  Unfortunately, since then, part of the simulator got
merged, but not all of it, so the in-tree simulator lacks I think,
things like float support [NOTE: I might be out of date here, I've not
checked in an age].

> 

> The AdaCore internal test suite knows how to build using the appropriate

> compiler (and BSP or whatever) and then run the program with qemu.  And,

> the internal test suite has a translation of gnu_vector.exp.


I've never had much luck with the qemu setup, it always seems super
slow, and a pain to setup.

Anyway, I managed to bodge together my old RV sim, and ran the RV32
tests, and can confirm that gnu_vector.exp does show this failure.
The gnu_vector.exp test is a bit rubbish due to this block:

  gdb_test_multiple "return (int4) \{4, 2, 7, 6\}" $test {
      -re "#0 .* main .*$gdb_prompt $" {
  	pass $test
      }
      -re "The location .* is unknown.\r\n.* return value .* will be ignored.\r\n" {
  	# This happens, e.g., on s390x unless using the vector ABI.
  	set should_kfail 1
  	exp_continue
      }
      -re "Make add_some_intvecs return now. .y or n. $" {
  	send_gdb "y\n"
  	exp_continue
      }
  }

Notice that we just assume that any target that returns the "The
location ..." message is supposed to.

Ideally, we'd have a helper function that tells us, for each target,
whether we expect to see that message or not - can't think what the
function would be called right now - then, instead of setting
"should_kfail" we'd either set the flag, or fail directly, like:

  gdb_test_multiple "return (int4) \{4, 2, 7, 6\}" $test {
      -re "#0 .* main .*$gdb_prompt $" {
  	pass $test
      }
      -re "The location .* is unknown.\r\n.* return value .* will be ignored.\r\n" {

        if { [current_target_preserves_address_or_something] } {
          fail $test
        } else {
          # This happens, e.g., on s390x unless using the vector ABI.
          set should_kfail 1
  	  exp_continue
        }
      }
      -re "Make add_some_intvecs return now. .y or n. $" {
  	send_gdb "y\n"
  	exp_continue
      }
  }

But that's probably out of scope for your patch.

> 

> Under this setup, the 64-bit RISC-V port passes this test, but the

> 32-bit port fails.


I suspect the 64-bit target is passing the vector entirely in
registers, the test returns a vector of 4 32-bit ints, and IIRC RISC-V
will use up to 2 registers for a return value, which would line up
with what you saw.

> 

> Andrew> The change itself is fine, but I'd like to see a test for this added

> Andrew> if the gnu_vector test isn't good enough.

> 

> I think it ought to be, but I just don't know how to try it.  I'm happy

> to give it a shot if you have a qemu board file or the like to try out.


I'm happy that this functionality is already covered.  Could you just
update the commit message to say that this fixes a test failure on
RV32 when running gdb.base/gnu_vector.exp, rather than implying this
change is untested - then I'm happy :)

Thanks,
Andrew
Simon Marchi via Gdb-patches Jan. 5, 2022, 7:33 p.m. | #4
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:


Andrew> I'm happy that this functionality is already covered.  Could you just
Andrew> update the commit message to say that this fixes a test failure on
Andrew> RV32 when running gdb.base/gnu_vector.exp, rather than implying this
Andrew> change is untested - then I'm happy :)

Sure thing, and thank you.

Tom

Patch

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 5a6269887e7..886996ce5b7 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -3301,7 +3301,7 @@  riscv_return_value (struct gdbarch  *gdbarch,
     case riscv_arg_info::location::in_reg:
       return RETURN_VALUE_REGISTER_CONVENTION;
     case riscv_arg_info::location::by_ref:
-      return RETURN_VALUE_ABI_RETURNS_ADDRESS;
+      return RETURN_VALUE_ABI_PRESERVES_ADDRESS;
     case riscv_arg_info::location::on_stack:
     default:
       error (_("invalid argument location"));