Re: [PATCH] gdb/amdgpu: provide dummy implementation of gdbarch_return_value_as_value

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

On 3/7/23 05:45, Lancelot SIX wrote:
> Hi Simon,
> 
>> +/* Dummy implementation of gdbarch_return_value_as_value.  */
>> +
>> +static return_value_convention
>> +amdgpu_return_value_as_value (gdbarch *arch, value *function, type *valtype,
>> +			      regcache *regcache, value **read_value,
>> +			      const gdb_byte *writebuf)
>> +{
>> +  gdb_assert_not_reached ("not implemented");
> 
> Isn't "error" more appropriate here?  We just need to indicate that the
> current hook failed.  GDB is not in an inconsistent state.

In my original patch, I made these hooks optional, and added some
predicate checks:

  if (!gdbarch_return_value_as_value_p (gdbarch))
    error_arch_no_return_value (gdbarch);

The feedback was that throwing errors at some of these places (like
during event handling) would probably put GDB in a bad state.  Erroring
out of amdgpu_return_value_as_value would be the same.

> 
> Maybe another approach could be to add an elem to the
> return_value_convention like RETURN_VALUE_UNKNOWN which could be a
> reasonable default if the gdbarch does not know how to handle a given
> type.

I think that Pedro hinted that we would need this anyway at some point,
for functions that don't follow a defined ABI.  So, I think it would
make sense, but we need to update the core of GDB to handle that
response.  And I'm not too familiar with this area, so I don't know how
much work this represents.  But if we know we're going to need this
anyway, I might as well give it a shot.

> Anyway, I do not think that you can easily reach this point with the
> current port of amdgcn.  The `return`, `finish` and `call` commands will
> produce errors before reaching this point.

Yes, that's my experience.  The AMD GPU port upstream is too barebones
to use these commands.  And it's just temporary.

Simon