Re: [PATCH] xtensa: initialize call_abi in xtensa_tdep

[Max Filippov <jcmvbkbc@gmail.com>]

On Thu, Aug 20, 2015 at 3:09 PM, Joel Brobecker <brobecker@adacore.com> wrote:
> On Sun, Jun 07, 2015 at 01:05:36AM +0300, Max Filippov wrote:
>> Use XSHAL_ABI value provided by xtensa-config.h to correctly initialize
>> xtensa_tdep.call_abi
>> This fixes calls to functions from GDB that otherwise fail with the
>> following assertion in call0 configuration:
>>
>>   gdb/regcache.c:602: internal-error: regcache_raw_read: Assertion
>>   `regnum >= 0 && regnum < regcache->descr->nr_raw_registers' failed.
>>
>> gdb/
>>       * xtensa-tdep.h (XTENSA_GDBARCH_TDEP_INSTANTIATE): Initialize
>>       call_abi using XSHAL_ABI macro.
>
> I am not sure I understand the patch.
>
> The first thing I should mention is that, unless what your patch
> suggests, the code as I see it on master currently initializes call_abi
> to CallAbiDefault:
>
>           .call_abi = CallAbiDefault,                           \
>
> And looking at the definitions of XSHAL_ABI and XTHAL_ABI_CALL0
> in include/xtensa-config.h, those definitions seem to be entirely
> static...
>
>     #undef XSHAL_ABI
>     #undef XTHAL_ABI_WINDOWED
>     #undef XTHAL_ABI_CALL0
>     #define XSHAL_ABI                       XTHAL_ABI_WINDOWED
>     #define XTHAL_ABI_WINDOWED              0
>     #define XTHAL_ABI_CALL0                 1
>
> ... and resolving to distinct values. So the condition would always
> be false, resulting in call_abi always being set to CallAbiDefault.
>
> Am I missing something?

The file include/xtensa-config.h is meant to be replaced by processor-specific
version where XSHAL_ABI definition may be different. That's why I mention
'call0 configuration' in the description: when xtensa core is configured without
windowed registers it will necessarily have call0 ABI.

>>  gdb/xtensa-tdep.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/xtensa-tdep.h b/gdb/xtensa-tdep.h
>> index adacaf8..3b6ea66 100644
>> --- a/gdb/xtensa-tdep.h
>> +++ b/gdb/xtensa-tdep.h
>> @@ -246,7 +246,8 @@ struct gdbarch_tdep
>>         .spill_location = -1,                                 \
>>         .spill_size = (spillsz),                              \
>>         .unused = 0,                                          \
>> -       .call_abi = 0,                                        \
>> +       .call_abi = (XSHAL_ABI == XTHAL_ABI_CALL0) ?          \
>> +             CallAbiCall0Only : CallAbiDefault,              \
>
> Small style issue: binary operators should be at the start of
> the next line, rather than the end of the current line.

I'll send fixed v2.

-- 
Thanks.
-- Max