* [PATCH] gdb: mips: Fix the handling of complex type of function return value
@ 2022-03-16 4:48 Youling Tang
2022-03-16 8:42 ` Lancelot SIX
0 siblings, 1 reply; 12+ messages in thread
From: Youling Tang @ 2022-03-16 4:48 UTC (permalink / raw)
To: gdb-patches, Maciej W. Rozycki
$ objdump -d outputs/gdb.base/varargs/varargs
00000001200012e8 <find_max_float_real>:
...
1200013b8: c7c10000 lwc1 $f1,0(s8)
1200013bc: c7c00004 lwc1 $f0,4(s8)
1200013c0: 46000886 mov.s $f2,$f1
1200013c4: 46000046 mov.s $f1,$f0
1200013c8: 46001006 mov.s $f0,$f2
1200013cc: 46000886 mov.s $f2,$f1
1200013d0: 03c0e825 move sp,s8
1200013d4: dfbe0038 ld s8,56(sp)
1200013d8: 67bd0080 daddiu sp,sp,128
1200013dc: 03e00008 jr ra
1200013e0: 00000000 nop
From the above disassembly, we can see that when the return value of the
function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
will be passed through $f0 and $f2, so fix the corresponding processing
in mips_n32n64_return_value().
$ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
Before applying the patch:
FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
# of expected passes 9
# of unexpected failures 2
After applying the patch:
# of expected passes 11
Signed-off-by: Youling Tang <tangyouling@loongson.cn>
---
gdb/mips-tdep.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 7e37578..cddb8f8 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -5224,9 +5224,10 @@ mips_n32n64_return_value (struct gdbarch *gdbarch, struct value *function,
if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
return RETURN_VALUE_STRUCT_CONVENTION;
- else if (type->code () == TYPE_CODE_FLT
+ else if ((type->code () == TYPE_CODE_FLT
&& TYPE_LENGTH (type) == 16
&& tdep->mips_fpu_type != MIPS_FPU_NONE)
+ || (type->code () == TYPE_CODE_COMPLEX))
{
/* A 128-bit floating-point value fills both $f0 and $f2. The
two registers are used in the same as memory order, so the
--
2.1.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdb: mips: Fix the handling of complex type of function return value
2022-03-16 4:48 [PATCH] gdb: mips: Fix the handling of complex type of function return value Youling Tang
@ 2022-03-16 8:42 ` Lancelot SIX
2022-03-16 9:00 ` Youling Tang
0 siblings, 1 reply; 12+ messages in thread
From: Lancelot SIX @ 2022-03-16 8:42 UTC (permalink / raw)
To: Youling Tang; +Cc: gdb-patches, Maciej W. Rozycki
On Wed, Mar 16, 2022 at 12:48:26PM +0800, Youling Tang wrote:
> $ objdump -d outputs/gdb.base/varargs/varargs
> 00000001200012e8 <find_max_float_real>:
> ...
> 1200013b8: c7c10000 lwc1 $f1,0(s8)
> 1200013bc: c7c00004 lwc1 $f0,4(s8)
> 1200013c0: 46000886 mov.s $f2,$f1
> 1200013c4: 46000046 mov.s $f1,$f0
> 1200013c8: 46001006 mov.s $f0,$f2
> 1200013cc: 46000886 mov.s $f2,$f1
> 1200013d0: 03c0e825 move sp,s8
> 1200013d4: dfbe0038 ld s8,56(sp)
> 1200013d8: 67bd0080 daddiu sp,sp,128
> 1200013dc: 03e00008 jr ra
> 1200013e0: 00000000 nop
>
> From the above disassembly, we can see that when the return value of the
> function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
> will be passed through $f0 and $f2, so fix the corresponding processing
> in mips_n32n64_return_value().
>
> $ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
>
> Before applying the patch:
> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
> FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
>
> # of expected passes 9
> # of unexpected failures 2
>
> After applying the patch:
> # of expected passes 11
>
> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
> ---
> gdb/mips-tdep.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 7e37578..cddb8f8 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -5224,9 +5224,10 @@ mips_n32n64_return_value (struct gdbarch *gdbarch, struct value *function,
>
> if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
> return RETURN_VALUE_STRUCT_CONVENTION;
> - else if (type->code () == TYPE_CODE_FLT
> + else if ((type->code () == TYPE_CODE_FLT
> && TYPE_LENGTH (type) == 16
> && tdep->mips_fpu_type != MIPS_FPU_NONE)
Hi,
Just minor note, those 2 lines above should be indented 2 more space I
think (so the && operator continues to vertically align with "type->code
()").
> + || (type->code () == TYPE_CODE_COMPLEX))
I do not think the extra set of parens are requires (but they do no harm
either).
For the rest, it seems reasonable to me but I’ll let MIPS / global
maintainers comment.
Best,
Lancelot.
> {
> /* A 128-bit floating-point value fills both $f0 and $f2. The
> two registers are used in the same as memory order, so the
> --
> 2.1.0
>
--
Lancelot SIX
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdb: mips: Fix the handling of complex type of function return value
2022-03-16 8:42 ` Lancelot SIX
@ 2022-03-16 9:00 ` Youling Tang
2022-03-16 11:22 ` Andrew Burgess
0 siblings, 1 reply; 12+ messages in thread
From: Youling Tang @ 2022-03-16 9:00 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches, Maciej W. Rozycki
Hi, Lancelo
On 03/16/2022 04:42 PM, Lancelot SIX wrote:
> On Wed, Mar 16, 2022 at 12:48:26PM +0800, Youling Tang wrote:
>> $ objdump -d outputs/gdb.base/varargs/varargs
>> 00000001200012e8 <find_max_float_real>:
>> ...
>> 1200013b8: c7c10000 lwc1 $f1,0(s8)
>> 1200013bc: c7c00004 lwc1 $f0,4(s8)
>> 1200013c0: 46000886 mov.s $f2,$f1
>> 1200013c4: 46000046 mov.s $f1,$f0
>> 1200013c8: 46001006 mov.s $f0,$f2
>> 1200013cc: 46000886 mov.s $f2,$f1
>> 1200013d0: 03c0e825 move sp,s8
>> 1200013d4: dfbe0038 ld s8,56(sp)
>> 1200013d8: 67bd0080 daddiu sp,sp,128
>> 1200013dc: 03e00008 jr ra
>> 1200013e0: 00000000 nop
>>
>> From the above disassembly, we can see that when the return value of the
>> function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
>> will be passed through $f0 and $f2, so fix the corresponding processing
>> in mips_n32n64_return_value().
>>
>> $ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
>>
>> Before applying the patch:
>> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
>> FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
>>
>> # of expected passes 9
>> # of unexpected failures 2
>>
>> After applying the patch:
>> # of expected passes 11
>>
>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
>> ---
>> gdb/mips-tdep.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
>> index 7e37578..cddb8f8 100644
>> --- a/gdb/mips-tdep.c
>> +++ b/gdb/mips-tdep.c
>> @@ -5224,9 +5224,10 @@ mips_n32n64_return_value (struct gdbarch *gdbarch, struct value *function,
>>
>> if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
>> return RETURN_VALUE_STRUCT_CONVENTION;
>> - else if (type->code () == TYPE_CODE_FLT
>> + else if ((type->code () == TYPE_CODE_FLT
>> && TYPE_LENGTH (type) == 16
>> && tdep->mips_fpu_type != MIPS_FPU_NONE)
> Hi,
>
> Just minor note, those 2 lines above should be indented 2 more space I
> think (so the && operator continues to vertically align with "type->code
> ()").
>
>> + || (type->code () == TYPE_CODE_COMPLEX))
> I do not think the extra set of parens are requires (but they do no harm
> either).
Ok, I will modify this code style in the next version.
Thanks,
Youling.
> For the rest, it seems reasonable to me but I’ll let MIPS / global
> maintainers comment.
>
> Best,
> Lancelot.
>> {
>> /* A 128-bit floating-point value fills both $f0 and $f2. The
>> two registers are used in the same as memory order, so the
>> --
>> 2.1.0
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdb: mips: Fix the handling of complex type of function return value
2022-03-16 9:00 ` Youling Tang
@ 2022-03-16 11:22 ` Andrew Burgess
2022-03-16 18:26 ` Maciej W. Rozycki
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Burgess @ 2022-03-16 11:22 UTC (permalink / raw)
To: Youling Tang, Lancelot SIX; +Cc: gdb-patches, Maciej W. Rozycki
Youling Tang <tangyouling@loongson.cn> writes:
> Hi, Lancelo
>
> On 03/16/2022 04:42 PM, Lancelot SIX wrote:
>> On Wed, Mar 16, 2022 at 12:48:26PM +0800, Youling Tang wrote:
>>> $ objdump -d outputs/gdb.base/varargs/varargs
>>> 00000001200012e8 <find_max_float_real>:
>>> ...
>>> 1200013b8: c7c10000 lwc1 $f1,0(s8)
>>> 1200013bc: c7c00004 lwc1 $f0,4(s8)
>>> 1200013c0: 46000886 mov.s $f2,$f1
>>> 1200013c4: 46000046 mov.s $f1,$f0
>>> 1200013c8: 46001006 mov.s $f0,$f2
>>> 1200013cc: 46000886 mov.s $f2,$f1
>>> 1200013d0: 03c0e825 move sp,s8
>>> 1200013d4: dfbe0038 ld s8,56(sp)
>>> 1200013d8: 67bd0080 daddiu sp,sp,128
>>> 1200013dc: 03e00008 jr ra
>>> 1200013e0: 00000000 nop
>>>
>>> From the above disassembly, we can see that when the return value of the
>>> function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
>>> will be passed through $f0 and $f2, so fix the corresponding processing
>>> in mips_n32n64_return_value().
>>>
>>> $ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
>>>
>>> Before applying the patch:
>>> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
>>> FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
>>>
>>> # of expected passes 9
>>> # of unexpected failures 2
>>>
>>> After applying the patch:
>>> # of expected passes 11
>>>
>>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
>>> ---
>>> gdb/mips-tdep.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
>>> index 7e37578..cddb8f8 100644
>>> --- a/gdb/mips-tdep.c
>>> +++ b/gdb/mips-tdep.c
>>> @@ -5224,9 +5224,10 @@ mips_n32n64_return_value (struct gdbarch *gdbarch, struct value *function,
>>>
>>> if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
>>> return RETURN_VALUE_STRUCT_CONVENTION;
>>> - else if (type->code () == TYPE_CODE_FLT
>>> + else if ((type->code () == TYPE_CODE_FLT
>>> && TYPE_LENGTH (type) == 16
>>> && tdep->mips_fpu_type != MIPS_FPU_NONE)
>> Hi,
>>
>> Just minor note, those 2 lines above should be indented 2 more space I
>> think (so the && operator continues to vertically align with "type->code
>> ()").
>>
>>> + || (type->code () == TYPE_CODE_COMPLEX))
>> I do not think the extra set of parens are requires (but they do no harm
>> either).
> Ok, I will modify this code style in the next version.
This change looks good to me with the style adjusments that Lancelot
pointed out.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdb: mips: Fix the handling of complex type of function return value
2022-03-16 11:22 ` Andrew Burgess
@ 2022-03-16 18:26 ` Maciej W. Rozycki
2022-03-17 7:01 ` Youling Tang
0 siblings, 1 reply; 12+ messages in thread
From: Maciej W. Rozycki @ 2022-03-16 18:26 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Youling Tang, Lancelot SIX, gdb-patches
On Wed, 16 Mar 2022, Andrew Burgess wrote:
> >>> $ objdump -d outputs/gdb.base/varargs/varargs
> >>> 00000001200012e8 <find_max_float_real>:
> >>> ...
> >>> 1200013b8: c7c10000 lwc1 $f1,0(s8)
> >>> 1200013bc: c7c00004 lwc1 $f0,4(s8)
> >>> 1200013c0: 46000886 mov.s $f2,$f1
> >>> 1200013c4: 46000046 mov.s $f1,$f0
> >>> 1200013c8: 46001006 mov.s $f0,$f2
> >>> 1200013cc: 46000886 mov.s $f2,$f1
> >>> 1200013d0: 03c0e825 move sp,s8
> >>> 1200013d4: dfbe0038 ld s8,56(sp)
> >>> 1200013d8: 67bd0080 daddiu sp,sp,128
> >>> 1200013dc: 03e00008 jr ra
> >>> 1200013e0: 00000000 nop
> >>>
> >>> From the above disassembly, we can see that when the return value of the
> >>> function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
> >>> will be passed through $f0 and $f2, so fix the corresponding processing
> >>> in mips_n32n64_return_value().
> >>>
> >>> $ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
> >>>
> >>> Before applying the patch:
> >>> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
> >>> FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
> >>>
> >>> # of expected passes 9
> >>> # of unexpected failures 2
> >>>
> >>> After applying the patch:
> >>> # of expected passes 11
> >>>
> >>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
> >>> ---
> >>> gdb/mips-tdep.c | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> >>> index 7e37578..cddb8f8 100644
> >>> --- a/gdb/mips-tdep.c
> >>> +++ b/gdb/mips-tdep.c
> >>> @@ -5224,9 +5224,10 @@ mips_n32n64_return_value (struct gdbarch *gdbarch, struct value *function,
> >>>
> >>> if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
> >>> return RETURN_VALUE_STRUCT_CONVENTION;
> >>> - else if (type->code () == TYPE_CODE_FLT
> >>> + else if ((type->code () == TYPE_CODE_FLT
> >>> && TYPE_LENGTH (type) == 16
> >>> && tdep->mips_fpu_type != MIPS_FPU_NONE)
> >> Hi,
> >>
> >> Just minor note, those 2 lines above should be indented 2 more space I
> >> think (so the && operator continues to vertically align with "type->code
> >> ()").
> >>
> >>> + || (type->code () == TYPE_CODE_COMPLEX))
> >> I do not think the extra set of parens are requires (but they do no harm
> >> either).
> > Ok, I will modify this code style in the next version.
>
> This change looks good to me with the style adjusments that Lancelot
> pointed out.
This has to be double-checked, because as I recall we have an ABI bug in
GCC in this area. Which is also the reason why the relevant test cases
have not been fixed in 15+ years now (I've been aware of this issue).
OTOH, if things have been like this for so long, then I suppose they need
to stay as they are. In any case I think this does have to be thoroughly
understood and documented.
Maciej
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] gdb: mips: Fix the handling of complex type of function return value
2022-03-16 18:26 ` Maciej W. Rozycki
@ 2022-03-17 7:01 ` Youling Tang
2022-04-06 22:46 ` [COMMITTED PATCH v2] " Maciej W. Rozycki
0 siblings, 1 reply; 12+ messages in thread
From: Youling Tang @ 2022-03-17 7:01 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Andrew Burgess, Lancelot SIX, gdb-patches
Hi, Maciej
On 03/17/2022 02:26 AM, Maciej W. Rozycki wrote:
> On Wed, 16 Mar 2022, Andrew Burgess wrote:
>
>>>>> $ objdump -d outputs/gdb.base/varargs/varargs
>>>>> 00000001200012e8 <find_max_float_real>:
>>>>> ...
>>>>> 1200013b8: c7c10000 lwc1 $f1,0(s8)
>>>>> 1200013bc: c7c00004 lwc1 $f0,4(s8)
>>>>> 1200013c0: 46000886 mov.s $f2,$f1
>>>>> 1200013c4: 46000046 mov.s $f1,$f0
>>>>> 1200013c8: 46001006 mov.s $f0,$f2
>>>>> 1200013cc: 46000886 mov.s $f2,$f1
>>>>> 1200013d0: 03c0e825 move sp,s8
>>>>> 1200013d4: dfbe0038 ld s8,56(sp)
>>>>> 1200013d8: 67bd0080 daddiu sp,sp,128
>>>>> 1200013dc: 03e00008 jr ra
>>>>> 1200013e0: 00000000 nop
>>>>>
>>>>> From the above disassembly, we can see that when the return value of the
>>>>> function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
>>>>> will be passed through $f0 and $f2, so fix the corresponding processing
>>>>> in mips_n32n64_return_value().
>>>>>
>>>>> $ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
>>>>>
>>>>> Before applying the patch:
>>>>> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
>>>>> FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
>>>>>
>>>>> # of expected passes 9
>>>>> # of unexpected failures 2
>>>>>
>>>>> After applying the patch:
>>>>> # of expected passes 11
>>>>>
>>>>> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
>>>>> ---
>>>>> gdb/mips-tdep.c | 3 ++-
>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
>>>>> index 7e37578..cddb8f8 100644
>>>>> --- a/gdb/mips-tdep.c
>>>>> +++ b/gdb/mips-tdep.c
>>>>> @@ -5224,9 +5224,10 @@ mips_n32n64_return_value (struct gdbarch *gdbarch, struct value *function,
>>>>>
>>>>> if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
>>>>> return RETURN_VALUE_STRUCT_CONVENTION;
>>>>> - else if (type->code () == TYPE_CODE_FLT
>>>>> + else if ((type->code () == TYPE_CODE_FLT
>>>>> && TYPE_LENGTH (type) == 16
>>>>> && tdep->mips_fpu_type != MIPS_FPU_NONE)
>>>> Hi,
>>>>
>>>> Just minor note, those 2 lines above should be indented 2 more space I
>>>> think (so the && operator continues to vertically align with "type->code
>>>> ()").
>>>>
>>>>> + || (type->code () == TYPE_CODE_COMPLEX))
>>>> I do not think the extra set of parens are requires (but they do no harm
>>>> either).
>>> Ok, I will modify this code style in the next version.
>> This change looks good to me with the style adjusments that Lancelot
>> pointed out.
> This has to be double-checked, because as I recall we have an ABI bug in
> GCC in this area. Which is also the reason why the relevant test cases
> have not been fixed in 15+ years now (I've been aware of this issue).
>
> OTOH, if things have been like this for so long, then I suppose they need
> to stay as they are. In any case I think this does have to be thoroughly
> understood and documented.
Thanks for your pointing out.If GCC's processing does not follow the ABI
call parameter specification, then this will be a GCC bug.GDB will remain
as is, without relevant modifications.
Thanks,
Youling.
>
> Maciej
^ permalink raw reply [flat|nested] 12+ messages in thread
* [COMMITTED PATCH v2] gdb: mips: Fix the handling of complex type of function return value
2022-03-17 7:01 ` Youling Tang
@ 2022-04-06 22:46 ` Maciej W. Rozycki
2022-04-07 2:19 ` Youling Tang
2022-04-08 17:05 ` Richard Sandiford
0 siblings, 2 replies; 12+ messages in thread
From: Maciej W. Rozycki @ 2022-04-06 22:46 UTC (permalink / raw)
To: Youling Tang, Richard Sandiford; +Cc: Andrew Burgess, Lancelot SIX, gdb-patches
From: Youling Tang <tangyouling@loongson.cn>
$ objdump -d outputs/gdb.base/varargs/varargs
00000001200012e8 <find_max_float_real>:
...
1200013b8: c7c10000 lwc1 $f1,0(s8)
1200013bc: c7c00004 lwc1 $f0,4(s8)
1200013c0: 46000886 mov.s $f2,$f1
1200013c4: 46000046 mov.s $f1,$f0
1200013c8: 46001006 mov.s $f0,$f2
1200013cc: 46000886 mov.s $f2,$f1
1200013d0: 03c0e825 move sp,s8
1200013d4: dfbe0038 ld s8,56(sp)
1200013d8: 67bd0080 daddiu sp,sp,128
1200013dc: 03e00008 jr ra
1200013e0: 00000000 nop
From the above disassembly, we can see that when the return value of the
function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
will be passed through $f0 and $f2, so fix the corresponding processing
in mips_n32n64_return_value().
$ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
Before applying the patch:
FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
# of expected passes 9
# of unexpected failures 2
After applying the patch:
# of expected passes 11
This also fixes:
FAIL: gdb.base/callfuncs.exp: call inferior func with struct - returns float _Complex
Signed-off-by: Youling Tang <tangyouling@loongson.cn>
Co-Authored-By: Maciej W. Rozycki <macro@orcam.me.uk>
---
Hi Youling,
> > This has to be double-checked, because as I recall we have an ABI bug in
> > GCC in this area. Which is also the reason why the relevant test cases
> > have not been fixed in 15+ years now (I've been aware of this issue).
> >
> > OTOH, if things have been like this for so long, then I suppose they need
> > to stay as they are. In any case I think this does have to be thoroughly
> > understood and documented.
> Thanks for your pointing out.If GCC's processing does not follow the ABI
> call parameter specification, then this will be a GCC bug.GDB will remain
> as is, without relevant modifications.
I have now carefully reviewed this issue and while we do have a bug in
this area (more on this later), your change is conceptually a move in the
right direction except for these issues:
- $f0/$f2 may only be used if we have an FPU, so the `tdep->mips_fpu_type'
check has to qualify complex types just as it does floating-point types,
- single complex types occupy lower halves of $f0/$f2 only.
You haven't stated exactly how you verified your change, but it does not
fix:
FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
in my environment because of the latter issue.
I went ahead and fixed these issues (in addition to code formatting), and
added explanatory notes including real/imaginary part usage in particular.
I have verified the updated change with the `mips64-linux-gnu' big-endian
target and the n64 ABI using remote `gdbserver' with my Malta 5Kc MIPS64r1
system. I have mentioned the gdb.base/callfuncs.exp progression in the
change description too.
I cannot see you listed in MAINTAINERS, but your original change only had
a legally insignificant amount of changes, so I have now committed this
updated version. However for the sake of any future submissions you may
make can you please explain your current FSF copyright assignment status?
Now as to the bug mentioned earlier on it's with `long double' rather
than complex data types. Considering this program:
typedef struct
{
long double d;
}
ldouble_t;
long double
v (long double d)
{
return d;
}
ldouble_t
s (long double d)
{
return (ldouble_t) { .d = d };
}
we get this assembly:
.file 1 "ldouble.c"
.section .mdebug.abi64
.previous
.nan legacy
.module fp=64
.module oddspreg
.module arch=mips3
.abicalls
.text
.align 2
.align 3
.globl v
.set nomips16
.set nomicromips
.ent v
.type v, @function
v:
.frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
.mask 0x00000000,0
.fmask 0x00000000,0
.set noreorder
.set nomacro
dmfc1 $4,$f12
dmfc1 $5,$f13
dmtc1 $4,$f0
dmtc1 $5,$f2
jr $31
nop
.set macro
.set reorder
.end v
.size v, .-v
.align 2
.align 3
.globl s
.set nomips16
.set nomicromips
.ent s
.type s, @function
s:
.frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
.mask 0x00000000,0
.fmask 0x00000000,0
.set noreorder
.set nomacro
dmfc1 $2,$f12
dmfc1 $3,$f13
dmtc1 $2,$f0
nop
dmtc1 $3,$f1
jr $31
nop
.set macro
.set reorder
.end s
.size s, .-s
.ident "GCC: (GNU) 12.0.1 20220129 (experimental)"
.section .note.GNU-stack,"",@progbits
so while `v' correctly returns the result in $f0/$f2, `s' returns it in
$f0/$f1 instead. I'll see if I can discuss this with the GCC community
once Stage 1 has opened, likely in a couple of weeks' time.
Additionally I found another issue, which I think is an ABI bug too.
Considering this program:
#include <complex.h>
typedef struct
{
float complex c;
}
complexf_t;
typedef struct
{
double complex c;
}
complex_t;
float complex
vf (float complex c)
{
return c;
}
complexf_t
sf (float complex c)
{
return (complexf_t) { .c = c };
}
double complex
v (double complex c)
{
return c;
}
complex_t
s (double complex c)
{
return (complex_t) { .c = c };
}
we get this assembly:
.file 1 "complex.c"
.section .mdebug.abi64
.previous
.nan legacy
.module fp=64
.module oddspreg
.module arch=mips3
.abicalls
.text
.align 2
.align 3
.globl vf
.set nomips16
.set nomicromips
.ent vf
.type vf, @function
vf:
.frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
.mask 0x00000000,0
.fmask 0x00000000,0
.set noreorder
.set nomacro
mov.s $f2,$f13
jr $31
mov.s $f0,$f12
.set macro
.set reorder
.end vf
.size vf, .-vf
.align 2
.align 3
.globl sf
.set nomips16
.set nomicromips
.ent sf
.type sf, @function
sf:
.frame $sp,16,$31 # vars= 16, regs= 0/0, args= 0, gp= 0
.mask 0x00000000,0
.fmask 0x00000000,0
.set noreorder
.set nomacro
daddiu $sp,$sp,-16
swc1 $f12,0($sp)
lwu $4,0($sp)
mfc1 $3,$f13
daddiu $sp,$sp,16
dsll $3,$3,32
jr $31
or $2,$4,$3
.set macro
.set reorder
.end sf
.size sf, .-sf
.align 2
.align 3
.globl v
.set nomips16
.set nomicromips
.ent v
.type v, @function
v:
.frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
.mask 0x00000000,0
.fmask 0x00000000,0
.set noreorder
.set nomacro
mov.d $f2,$f13
jr $31
mov.d $f0,$f12
.set macro
.set reorder
.end v
.size v, .-v
.align 2
.align 3
.globl s
.set nomips16
.set nomicromips
.ent s
.type s, @function
s:
.frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
.mask 0x00000000,0
.fmask 0x00000000,0
.set noreorder
.set nomacro
dmfc1 $2,$f12
dmfc1 $3,$f13
jr $31
nop
.set macro
.set reorder
.end s
.size s, .-s
.ident "GCC: (GNU) 12.0.1 20220129 (experimental)"
.section .note.GNU-stack,"",@progbits
again `v' and `vf' correctly return the result in $f0/$f2, but `s' holds
it in $2/$3 and especially `sf' goes through the hoops to squeeze it into
$3 (and the caller then has to unsqueeze it). My understanding of MIPSpro
documentation is in both cases the result shall go into $f0/$f2. I think
this is something to take with GCC as well.
Richard, would you by any chance know/remember what IRIX/MIPSpro did
here?
Maciej
---
gdb/mips-tdep.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
gdb-tangyouling-mips-n64-complex-return.diff
Index: binutils-gdb/gdb/mips-tdep.c
===================================================================
--- binutils-gdb.orig/gdb/mips-tdep.c
+++ binutils-gdb/gdb/mips-tdep.c
@@ -5217,30 +5217,44 @@ mips_n32n64_return_value (struct gdbarch
that all composite results be handled by conversion to implicit first
parameters. The MIPS/SGI Fortran implementation has always made a
specific exception to return COMPLEX results in the floating point
- registers.] */
+ registers.]
+
+ From MIPSpro Assembly Language Programmer's Guide, Document Number:
+ 007-2418-004
+
+ Software
+ Register Name(from
+ Name fgregdef.h) Use and Linkage
+ -----------------------------------------------------------------
+ $f0, $f2 fv0, fv1 Hold results of floating-point type function
+ ($f0) and complex type function ($f0 has the
+ real part, $f2 has the imaginary part.) */
if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
return RETURN_VALUE_STRUCT_CONVENTION;
- else if (type->code () == TYPE_CODE_FLT
- && TYPE_LENGTH (type) == 16
+ else if ((type->code () == TYPE_CODE_COMPLEX
+ || (type->code () == TYPE_CODE_FLT && TYPE_LENGTH (type) == 16))
&& tdep->mips_fpu_type != MIPS_FPU_NONE)
{
- /* A 128-bit floating-point value fills both $f0 and $f2. The
- two registers are used in the same as memory order, so the
- eight bytes with the lower memory address are in $f0. */
+ /* A complex value of up to 128 bits in width as well as a 128-bit
+ floating-point value goes in both $f0 and $f2. A single complex
+ value is held in the lower halves only of the respective registers.
+ The two registers are used in the same as memory order, so the
+ bytes with the lower memory address are in $f0. */
if (mips_debug)
gdb_printf (gdb_stderr, "Return float in $f0 and $f2\n");
mips_xfer_register (gdbarch, regcache,
(gdbarch_num_regs (gdbarch)
+ mips_regnum (gdbarch)->fp0),
- 8, gdbarch_byte_order (gdbarch),
+ TYPE_LENGTH (type) / 2, gdbarch_byte_order (gdbarch),
readbuf, writebuf, 0);
mips_xfer_register (gdbarch, regcache,
(gdbarch_num_regs (gdbarch)
+ mips_regnum (gdbarch)->fp0 + 2),
- 8, gdbarch_byte_order (gdbarch),
- readbuf ? readbuf + 8 : readbuf,
- writebuf ? writebuf + 8 : writebuf, 0);
+ TYPE_LENGTH (type) / 2, gdbarch_byte_order (gdbarch),
+ readbuf ? readbuf + TYPE_LENGTH (type) / 2 : readbuf,
+ (writebuf
+ ? writebuf + TYPE_LENGTH (type) / 2 : writebuf), 0);
return RETURN_VALUE_REGISTER_CONVENTION;
}
else if (type->code () == TYPE_CODE_FLT
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [COMMITTED PATCH v2] gdb: mips: Fix the handling of complex type of function return value
2022-04-06 22:46 ` [COMMITTED PATCH v2] " Maciej W. Rozycki
@ 2022-04-07 2:19 ` Youling Tang
2022-04-11 15:32 ` Maciej W. Rozycki
2022-04-08 17:05 ` Richard Sandiford
1 sibling, 1 reply; 12+ messages in thread
From: Youling Tang @ 2022-04-07 2:19 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Richard Sandiford, Andrew Burgess, Lancelot SIX, gdb-patches
Hi, Maciej
On 04/07/2022 06:46 AM, Maciej W. Rozycki wrote:
> From: Youling Tang <tangyouling@loongson.cn>
>
> $ objdump -d outputs/gdb.base/varargs/varargs
> 00000001200012e8 <find_max_float_real>:
> ...
> 1200013b8: c7c10000 lwc1 $f1,0(s8)
> 1200013bc: c7c00004 lwc1 $f0,4(s8)
> 1200013c0: 46000886 mov.s $f2,$f1
> 1200013c4: 46000046 mov.s $f1,$f0
> 1200013c8: 46001006 mov.s $f0,$f2
> 1200013cc: 46000886 mov.s $f2,$f1
> 1200013d0: 03c0e825 move sp,s8
> 1200013d4: dfbe0038 ld s8,56(sp)
> 1200013d8: 67bd0080 daddiu sp,sp,128
> 1200013dc: 03e00008 jr ra
> 1200013e0: 00000000 nop
>
> From the above disassembly, we can see that when the return value of the
> function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
> will be passed through $f0 and $f2, so fix the corresponding processing
> in mips_n32n64_return_value().
>
> $ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
>
> Before applying the patch:
> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
> FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
>
> # of expected passes 9
> # of unexpected failures 2
>
> After applying the patch:
> # of expected passes 11
>
> This also fixes:
> FAIL: gdb.base/callfuncs.exp: call inferior func with struct - returns float _Complex
>
> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
> Co-Authored-By: Maciej W. Rozycki <macro@orcam.me.uk>
> ---
> Hi Youling,
>
>>> This has to be double-checked, because as I recall we have an ABI bug in
>>> GCC in this area. Which is also the reason why the relevant test cases
>>> have not been fixed in 15+ years now (I've been aware of this issue).
>>>
>>> OTOH, if things have been like this for so long, then I suppose they need
>>> to stay as they are. In any case I think this does have to be thoroughly
>>> understood and documented.
>> Thanks for your pointing out.If GCC's processing does not follow the ABI
>> call parameter specification, then this will be a GCC bug.GDB will remain
>> as is, without relevant modifications.
> I have now carefully reviewed this issue and while we do have a bug in
> this area (more on this later), your change is conceptually a move in the
> right direction except for these issues:
>
> - $f0/$f2 may only be used if we have an FPU, so the `tdep->mips_fpu_type'
> check has to qualify complex types just as it does floating-point types,
>
> - single complex types occupy lower halves of $f0/$f2 only.
Thank you very much for your detailed explanation and for further
pointing out the problem.
>
> You haven't stated exactly how you verified your change, but it does not
> fix:
>
> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
>
> in my environment because of the latter issue.
>
> I went ahead and fixed these issues (in addition to code formatting), and
> added explanatory notes including real/imaginary part usage in particular.
> I have verified the updated change with the `mips64-linux-gnu' big-endian
> target and the n64 ABI using remote `gdbserver' with my Malta 5Kc MIPS64r1
> system. I have mentioned the gdb.base/callfuncs.exp progression in the
> change description too.
>
> I cannot see you listed in MAINTAINERS, but your original change only had
> a legally insignificant amount of changes, so I have now committed this
> updated version. However for the sake of any future submissions you may
> make can you please explain your current FSF copyright assignment status?
I am working for Loongson Technology Corporation Limited.
Loongson has signed the copyright assignment.
Loongson GNU ANY is RT 1604586.
Can I add to the MAINTAINERS list for the convenience of submitting code
later?
>
> Now as to the bug mentioned earlier on it's with `long double' rather
> than complex data types. Considering this program:
>
> typedef struct
> {
> long double d;
> }
> ldouble_t;
>
> long double
> v (long double d)
> {
> return d;
> }
>
> ldouble_t
> s (long double d)
> {
> return (ldouble_t) { .d = d };
> }
>
> we get this assembly:
>
> .file 1 "ldouble.c"
> .section .mdebug.abi64
> .previous
> .nan legacy
> .module fp=64
> .module oddspreg
> .module arch=mips3
> .abicalls
> .text
> .align 2
> .align 3
> .globl v
> .set nomips16
> .set nomicromips
> .ent v
> .type v, @function
> v:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> dmfc1 $4,$f12
> dmfc1 $5,$f13
> dmtc1 $4,$f0
> dmtc1 $5,$f2
> jr $31
> nop
>
> .set macro
> .set reorder
> .end v
> .size v, .-v
> .align 2
> .align 3
> .globl s
> .set nomips16
> .set nomicromips
> .ent s
> .type s, @function
> s:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> dmfc1 $2,$f12
> dmfc1 $3,$f13
> dmtc1 $2,$f0
> nop
> dmtc1 $3,$f1
> jr $31
> nop
>
> .set macro
> .set reorder
> .end s
> .size s, .-s
> .ident "GCC: (GNU) 12.0.1 20220129 (experimental)"
> .section .note.GNU-stack,"",@progbits
>
> so while `v' correctly returns the result in $f0/$f2, `s' returns it in
> $f0/$f1 instead. I'll see if I can discuss this with the GCC community
> once Stage 1 has opened, likely in a couple of weeks' time.
>
> Additionally I found another issue, which I think is an ABI bug too.
> Considering this program:
>
> #include <complex.h>
>
> typedef struct
> {
> float complex c;
> }
> complexf_t;
>
> typedef struct
> {
> double complex c;
> }
> complex_t;
>
> float complex
> vf (float complex c)
> {
> return c;
> }
>
> complexf_t
> sf (float complex c)
> {
> return (complexf_t) { .c = c };
> }
>
> double complex
> v (double complex c)
> {
> return c;
> }
>
> complex_t
> s (double complex c)
> {
> return (complex_t) { .c = c };
> }
>
> we get this assembly:
>
> .file 1 "complex.c"
> .section .mdebug.abi64
> .previous
> .nan legacy
> .module fp=64
> .module oddspreg
> .module arch=mips3
> .abicalls
> .text
> .align 2
> .align 3
> .globl vf
> .set nomips16
> .set nomicromips
> .ent vf
> .type vf, @function
> vf:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> mov.s $f2,$f13
> jr $31
> mov.s $f0,$f12
>
> .set macro
> .set reorder
> .end vf
> .size vf, .-vf
> .align 2
> .align 3
> .globl sf
> .set nomips16
> .set nomicromips
> .ent sf
> .type sf, @function
> sf:
> .frame $sp,16,$31 # vars= 16, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> daddiu $sp,$sp,-16
> swc1 $f12,0($sp)
> lwu $4,0($sp)
> mfc1 $3,$f13
> daddiu $sp,$sp,16
> dsll $3,$3,32
> jr $31
> or $2,$4,$3
>
> .set macro
> .set reorder
> .end sf
> .size sf, .-sf
> .align 2
> .align 3
> .globl v
> .set nomips16
> .set nomicromips
> .ent v
> .type v, @function
> v:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> mov.d $f2,$f13
> jr $31
> mov.d $f0,$f12
>
> .set macro
> .set reorder
> .end v
> .size v, .-v
> .align 2
> .align 3
> .globl s
> .set nomips16
> .set nomicromips
> .ent s
> .type s, @function
> s:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> dmfc1 $2,$f12
> dmfc1 $3,$f13
> jr $31
> nop
>
> .set macro
> .set reorder
> .end s
> .size s, .-s
> .ident "GCC: (GNU) 12.0.1 20220129 (experimental)"
> .section .note.GNU-stack,"",@progbits
>
> again `v' and `vf' correctly return the result in $f0/$f2, but `s' holds
> it in $2/$3 and especially `sf' goes through the hoops to squeeze it into
> $3 (and the caller then has to unsqueeze it). My understanding of MIPSpro
> documentation is in both cases the result shall go into $f0/$f2. I think
> this is something to take with GCC as well.
The assembly result is the same with my Loongson 3A4000 machine.
.file 1 "complex_test.c"
.section .mdebug.abi64
.previous
.nan legacy
.module fp=64
.module oddspreg
.abicalls
.text
.align 2
.globl vf
.set nomips16
.set nomicromips
.ent vf
.type vf, @function
vf:
.frame $fp,32,$31 # vars= 16, regs= 1/0, args= 0, gp= 0
.mask 0x40000000,-8
.fmask 0x00000000,0
.set noreorder
.set nomacro
daddiu $sp,$sp,-32
sd $fp,24($sp)
move $fp,$sp
mov.s $f1,$f12
mov.s $f0,$f13
swc1 $f1,0($fp)
swc1 $f0,4($fp)
lwc1 $f1,0($fp)
lwc1 $f0,4($fp)
mov.s $f2,$f1
mov.s $f1,$f0
mov.s $f0,$f2
mov.s $f2,$f1
move $sp,$fp
ld $fp,24($sp)
daddiu $sp,$sp,32
jr $31
nop
.set macro
.set reorder
.end vf
.size vf, .-vf
.align 2
.globl sf
.set nomips16
.set nomicromips
.ent sf
.type sf, @function
sf:
.frame $fp,32,$31 # vars= 16, regs= 1/0, args= 0, gp= 0
.mask 0x40000000,-8
.fmask 0x00000000,0
.set noreorder
.set nomacro
daddiu $sp,$sp,-32
sd $fp,24($sp)
move $fp,$sp
mov.s $f1,$f12
mov.s $f0,$f13
swc1 $f1,0($fp)
swc1 $f0,4($fp)
lwc1 $f1,0($fp)
lwc1 $f0,4($fp)
mfc1 $3,$f1
dins $2,$3,0,32
mfc1 $3,$f0
dins $2,$3,32,32
move $sp,$fp
ld $fp,24($sp)
daddiu $sp,$sp,32
jr $31
nop
.set macro
.set reorder
.end sf
.size sf, .-sf
.align 2
.globl v
.set nomips16
.set nomicromips
.ent v
.type v, @function
v:
.frame $fp,32,$31 # vars= 16, regs= 1/0, args= 0, gp= 0
.mask 0x40000000,-8
.fmask 0x00000000,0
.set noreorder
.set nomacro
daddiu $sp,$sp,-32
sd $fp,24($sp)
move $fp,$sp
mov.d $f1,$f12
mov.d $f0,$f13
sdc1 $f1,0($fp)
sdc1 $f0,8($fp)
ldc1 $f1,0($fp)
ldc1 $f0,8($fp)
mov.d $f2,$f1
mov.d $f1,$f0
mov.d $f0,$f2
mov.d $f2,$f1
move $sp,$fp
ld $fp,24($sp)
daddiu $sp,$sp,32
jr $31
nop
.set macro
.set reorder
.end v
.size v, .-v
.align 2
.globl s
.set nomips16
.set nomicromips
.ent s
.type s, @function
s:
.frame $fp,32,$31 # vars= 16, regs= 1/0, args= 0, gp= 0
.mask 0x40000000,-8
.fmask 0x00000000,0
.set noreorder
.set nomacro
daddiu $sp,$sp,-32
sd $fp,24($sp)
move $fp,$sp
mov.d $f1,$f12
mov.d $f0,$f13
sdc1 $f1,0($fp)
sdc1 $f0,8($fp)
ldc1 $f1,0($fp)
ldc1 $f0,8($fp)
dmfc1 $2,$f1
dmfc1 $3,$f0
move $sp,$fp
ld $fp,24($sp)
daddiu $sp,$sp,32
jr $31
nop
.set macro
.set reorder
.end s
.size s, .-s
.ident "GCC: (GNU) 7.3.1 20180303 (Red Hat 7.3.1-6)"
Thanks,
Youling.
>
> Richard, would you by any chance know/remember what IRIX/MIPSpro did
> here?
>
> Maciej
> ---
> gdb/mips-tdep.c | 34 ++++++++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> gdb-tangyouling-mips-n64-complex-return.diff
> Index: binutils-gdb/gdb/mips-tdep.c
> ===================================================================
> --- binutils-gdb.orig/gdb/mips-tdep.c
> +++ binutils-gdb/gdb/mips-tdep.c
> @@ -5217,30 +5217,44 @@ mips_n32n64_return_value (struct gdbarch
> that all composite results be handled by conversion to implicit first
> parameters. The MIPS/SGI Fortran implementation has always made a
> specific exception to return COMPLEX results in the floating point
> - registers.] */
> + registers.]
> +
> + From MIPSpro Assembly Language Programmer's Guide, Document Number:
> + 007-2418-004
> +
> + Software
> + Register Name(from
> + Name fgregdef.h) Use and Linkage
> + -----------------------------------------------------------------
> + $f0, $f2 fv0, fv1 Hold results of floating-point type function
> + ($f0) and complex type function ($f0 has the
> + real part, $f2 has the imaginary part.) */
>
> if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
> return RETURN_VALUE_STRUCT_CONVENTION;
> - else if (type->code () == TYPE_CODE_FLT
> - && TYPE_LENGTH (type) == 16
> + else if ((type->code () == TYPE_CODE_COMPLEX
> + || (type->code () == TYPE_CODE_FLT && TYPE_LENGTH (type) == 16))
> && tdep->mips_fpu_type != MIPS_FPU_NONE)
> {
> - /* A 128-bit floating-point value fills both $f0 and $f2. The
> - two registers are used in the same as memory order, so the
> - eight bytes with the lower memory address are in $f0. */
> + /* A complex value of up to 128 bits in width as well as a 128-bit
> + floating-point value goes in both $f0 and $f2. A single complex
> + value is held in the lower halves only of the respective registers.
> + The two registers are used in the same as memory order, so the
> + bytes with the lower memory address are in $f0. */
> if (mips_debug)
> gdb_printf (gdb_stderr, "Return float in $f0 and $f2\n");
> mips_xfer_register (gdbarch, regcache,
> (gdbarch_num_regs (gdbarch)
> + mips_regnum (gdbarch)->fp0),
> - 8, gdbarch_byte_order (gdbarch),
> + TYPE_LENGTH (type) / 2, gdbarch_byte_order (gdbarch),
> readbuf, writebuf, 0);
> mips_xfer_register (gdbarch, regcache,
> (gdbarch_num_regs (gdbarch)
> + mips_regnum (gdbarch)->fp0 + 2),
> - 8, gdbarch_byte_order (gdbarch),
> - readbuf ? readbuf + 8 : readbuf,
> - writebuf ? writebuf + 8 : writebuf, 0);
> + TYPE_LENGTH (type) / 2, gdbarch_byte_order (gdbarch),
> + readbuf ? readbuf + TYPE_LENGTH (type) / 2 : readbuf,
> + (writebuf
> + ? writebuf + TYPE_LENGTH (type) / 2 : writebuf), 0);
> return RETURN_VALUE_REGISTER_CONVENTION;
> }
> else if (type->code () == TYPE_CODE_FLT
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [COMMITTED PATCH v2] gdb: mips: Fix the handling of complex type of function return value
2022-04-06 22:46 ` [COMMITTED PATCH v2] " Maciej W. Rozycki
2022-04-07 2:19 ` Youling Tang
@ 2022-04-08 17:05 ` Richard Sandiford
2022-04-11 15:37 ` Maciej W. Rozycki
1 sibling, 1 reply; 12+ messages in thread
From: Richard Sandiford @ 2022-04-08 17:05 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: Youling Tang, Andrew Burgess, Lancelot SIX, gdb-patches
"Maciej W. Rozycki" <macro@orcam.me.uk> writes:
> From: Youling Tang <tangyouling@loongson.cn>
>
> $ objdump -d outputs/gdb.base/varargs/varargs
> 00000001200012e8 <find_max_float_real>:
> ...
> 1200013b8: c7c10000 lwc1 $f1,0(s8)
> 1200013bc: c7c00004 lwc1 $f0,4(s8)
> 1200013c0: 46000886 mov.s $f2,$f1
> 1200013c4: 46000046 mov.s $f1,$f0
> 1200013c8: 46001006 mov.s $f0,$f2
> 1200013cc: 46000886 mov.s $f2,$f1
> 1200013d0: 03c0e825 move sp,s8
> 1200013d4: dfbe0038 ld s8,56(sp)
> 1200013d8: 67bd0080 daddiu sp,sp,128
> 1200013dc: 03e00008 jr ra
> 1200013e0: 00000000 nop
>
> From the above disassembly, we can see that when the return value of the
> function is a complex type and len <= 2 * MIPS64_REGSIZE, the return value
> will be passed through $f0 and $f2, so fix the corresponding processing
> in mips_n32n64_return_value().
>
> $ make check RUNTESTFLAGS='GDB=../gdb gdb.base/varargs.exp --outdir=test'
>
> Before applying the patch:
> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
> FAIL: gdb.base/varargs.exp: print find_max_double_real(4, dc1, dc2, dc3, dc4)
>
> # of expected passes 9
> # of unexpected failures 2
>
> After applying the patch:
> # of expected passes 11
>
> This also fixes:
> FAIL: gdb.base/callfuncs.exp: call inferior func with struct - returns float _Complex
>
> Signed-off-by: Youling Tang <tangyouling@loongson.cn>
> Co-Authored-By: Maciej W. Rozycki <macro@orcam.me.uk>
> ---
> Hi Youling,
>
>> > This has to be double-checked, because as I recall we have an ABI bug in
>> > GCC in this area. Which is also the reason why the relevant test cases
>> > have not been fixed in 15+ years now (I've been aware of this issue).
>> >
>> > OTOH, if things have been like this for so long, then I suppose they need
>> > to stay as they are. In any case I think this does have to be thoroughly
>> > understood and documented.
>> Thanks for your pointing out.If GCC's processing does not follow the ABI
>> call parameter specification, then this will be a GCC bug.GDB will remain
>> as is, without relevant modifications.
>
> I have now carefully reviewed this issue and while we do have a bug in
> this area (more on this later), your change is conceptually a move in the
> right direction except for these issues:
>
> - $f0/$f2 may only be used if we have an FPU, so the `tdep->mips_fpu_type'
> check has to qualify complex types just as it does floating-point types,
>
> - single complex types occupy lower halves of $f0/$f2 only.
>
> You haven't stated exactly how you verified your change, but it does not
> fix:
>
> FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, fc4)
>
> in my environment because of the latter issue.
>
> I went ahead and fixed these issues (in addition to code formatting), and
> added explanatory notes including real/imaginary part usage in particular.
> I have verified the updated change with the `mips64-linux-gnu' big-endian
> target and the n64 ABI using remote `gdbserver' with my Malta 5Kc MIPS64r1
> system. I have mentioned the gdb.base/callfuncs.exp progression in the
> change description too.
>
> I cannot see you listed in MAINTAINERS, but your original change only had
> a legally insignificant amount of changes, so I have now committed this
> updated version. However for the sake of any future submissions you may
> make can you please explain your current FSF copyright assignment status?
>
> Now as to the bug mentioned earlier on it's with `long double' rather
> than complex data types. Considering this program:
>
> typedef struct
> {
> long double d;
> }
> ldouble_t;
>
> long double
> v (long double d)
> {
> return d;
> }
>
> ldouble_t
> s (long double d)
> {
> return (ldouble_t) { .d = d };
> }
>
> we get this assembly:
>
> .file 1 "ldouble.c"
> .section .mdebug.abi64
> .previous
> .nan legacy
> .module fp=64
> .module oddspreg
> .module arch=mips3
> .abicalls
> .text
> .align 2
> .align 3
> .globl v
> .set nomips16
> .set nomicromips
> .ent v
> .type v, @function
> v:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> dmfc1 $4,$f12
> dmfc1 $5,$f13
> dmtc1 $4,$f0
> dmtc1 $5,$f2
> jr $31
> nop
>
> .set macro
> .set reorder
> .end v
> .size v, .-v
> .align 2
> .align 3
> .globl s
> .set nomips16
> .set nomicromips
> .ent s
> .type s, @function
> s:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> dmfc1 $2,$f12
> dmfc1 $3,$f13
> dmtc1 $2,$f0
> nop
> dmtc1 $3,$f1
> jr $31
> nop
>
> .set macro
> .set reorder
> .end s
> .size s, .-s
> .ident "GCC: (GNU) 12.0.1 20220129 (experimental)"
> .section .note.GNU-stack,"",@progbits
>
> so while `v' correctly returns the result in $f0/$f2, `s' returns it in
> $f0/$f1 instead. I'll see if I can discuss this with the GCC community
> once Stage 1 has opened, likely in a couple of weeks' time.
>
> Additionally I found another issue, which I think is an ABI bug too.
> Considering this program:
>
> #include <complex.h>
>
> typedef struct
> {
> float complex c;
> }
> complexf_t;
>
> typedef struct
> {
> double complex c;
> }
> complex_t;
>
> float complex
> vf (float complex c)
> {
> return c;
> }
>
> complexf_t
> sf (float complex c)
> {
> return (complexf_t) { .c = c };
> }
>
> double complex
> v (double complex c)
> {
> return c;
> }
>
> complex_t
> s (double complex c)
> {
> return (complex_t) { .c = c };
> }
>
> we get this assembly:
>
> .file 1 "complex.c"
> .section .mdebug.abi64
> .previous
> .nan legacy
> .module fp=64
> .module oddspreg
> .module arch=mips3
> .abicalls
> .text
> .align 2
> .align 3
> .globl vf
> .set nomips16
> .set nomicromips
> .ent vf
> .type vf, @function
> vf:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> mov.s $f2,$f13
> jr $31
> mov.s $f0,$f12
>
> .set macro
> .set reorder
> .end vf
> .size vf, .-vf
> .align 2
> .align 3
> .globl sf
> .set nomips16
> .set nomicromips
> .ent sf
> .type sf, @function
> sf:
> .frame $sp,16,$31 # vars= 16, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> daddiu $sp,$sp,-16
> swc1 $f12,0($sp)
> lwu $4,0($sp)
> mfc1 $3,$f13
> daddiu $sp,$sp,16
> dsll $3,$3,32
> jr $31
> or $2,$4,$3
>
> .set macro
> .set reorder
> .end sf
> .size sf, .-sf
> .align 2
> .align 3
> .globl v
> .set nomips16
> .set nomicromips
> .ent v
> .type v, @function
> v:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> mov.d $f2,$f13
> jr $31
> mov.d $f0,$f12
>
> .set macro
> .set reorder
> .end v
> .size v, .-v
> .align 2
> .align 3
> .globl s
> .set nomips16
> .set nomicromips
> .ent s
> .type s, @function
> s:
> .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0
> .mask 0x00000000,0
> .fmask 0x00000000,0
> .set noreorder
> .set nomacro
> dmfc1 $2,$f12
> dmfc1 $3,$f13
> jr $31
> nop
>
> .set macro
> .set reorder
> .end s
> .size s, .-s
> .ident "GCC: (GNU) 12.0.1 20220129 (experimental)"
> .section .note.GNU-stack,"",@progbits
>
> again `v' and `vf' correctly return the result in $f0/$f2, but `s' holds
> it in $2/$3 and especially `sf' goes through the hoops to squeeze it into
> $3 (and the caller then has to unsqueeze it). My understanding of MIPSpro
> documentation is in both cases the result shall go into $f0/$f2. I think
> this is something to take with GCC as well.
>
> Richard, would you by any chance know/remember what IRIX/MIPSpro did
> here?
No, sorry. I remember there were some cases where we had to choose
between compatibility with MIPSpro and backwards compatibility with
GCC (which did something different and thus presumably “wrong”).
Since GCC de facto the ABI for GNU systems, it wasn't always an
easy choice. But that might not overlap with this case.
Richard
>
> Maciej
> ---
> gdb/mips-tdep.c | 34 ++++++++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> gdb-tangyouling-mips-n64-complex-return.diff
> Index: binutils-gdb/gdb/mips-tdep.c
> ===================================================================
> --- binutils-gdb.orig/gdb/mips-tdep.c
> +++ binutils-gdb/gdb/mips-tdep.c
> @@ -5217,30 +5217,44 @@ mips_n32n64_return_value (struct gdbarch
> that all composite results be handled by conversion to implicit first
> parameters. The MIPS/SGI Fortran implementation has always made a
> specific exception to return COMPLEX results in the floating point
> - registers.] */
> + registers.]
> +
> + From MIPSpro Assembly Language Programmer's Guide, Document Number:
> + 007-2418-004
> +
> + Software
> + Register Name(from
> + Name fgregdef.h) Use and Linkage
> + -----------------------------------------------------------------
> + $f0, $f2 fv0, fv1 Hold results of floating-point type function
> + ($f0) and complex type function ($f0 has the
> + real part, $f2 has the imaginary part.) */
>
> if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE)
> return RETURN_VALUE_STRUCT_CONVENTION;
> - else if (type->code () == TYPE_CODE_FLT
> - && TYPE_LENGTH (type) == 16
> + else if ((type->code () == TYPE_CODE_COMPLEX
> + || (type->code () == TYPE_CODE_FLT && TYPE_LENGTH (type) == 16))
> && tdep->mips_fpu_type != MIPS_FPU_NONE)
> {
> - /* A 128-bit floating-point value fills both $f0 and $f2. The
> - two registers are used in the same as memory order, so the
> - eight bytes with the lower memory address are in $f0. */
> + /* A complex value of up to 128 bits in width as well as a 128-bit
> + floating-point value goes in both $f0 and $f2. A single complex
> + value is held in the lower halves only of the respective registers.
> + The two registers are used in the same as memory order, so the
> + bytes with the lower memory address are in $f0. */
> if (mips_debug)
> gdb_printf (gdb_stderr, "Return float in $f0 and $f2\n");
> mips_xfer_register (gdbarch, regcache,
> (gdbarch_num_regs (gdbarch)
> + mips_regnum (gdbarch)->fp0),
> - 8, gdbarch_byte_order (gdbarch),
> + TYPE_LENGTH (type) / 2, gdbarch_byte_order (gdbarch),
> readbuf, writebuf, 0);
> mips_xfer_register (gdbarch, regcache,
> (gdbarch_num_regs (gdbarch)
> + mips_regnum (gdbarch)->fp0 + 2),
> - 8, gdbarch_byte_order (gdbarch),
> - readbuf ? readbuf + 8 : readbuf,
> - writebuf ? writebuf + 8 : writebuf, 0);
> + TYPE_LENGTH (type) / 2, gdbarch_byte_order (gdbarch),
> + readbuf ? readbuf + TYPE_LENGTH (type) / 2 : readbuf,
> + (writebuf
> + ? writebuf + TYPE_LENGTH (type) / 2 : writebuf), 0);
> return RETURN_VALUE_REGISTER_CONVENTION;
> }
> else if (type->code () == TYPE_CODE_FLT
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [COMMITTED PATCH v2] gdb: mips: Fix the handling of complex type of function return value
2022-04-07 2:19 ` Youling Tang
@ 2022-04-11 15:32 ` Maciej W. Rozycki
2022-04-12 0:46 ` Youling Tang
0 siblings, 1 reply; 12+ messages in thread
From: Maciej W. Rozycki @ 2022-04-11 15:32 UTC (permalink / raw)
To: Youling Tang; +Cc: Richard Sandiford, Andrew Burgess, Lancelot SIX, gdb-patches
Hi Youling,
> > I cannot see you listed in MAINTAINERS, but your original change only had
> > a legally insignificant amount of changes, so I have now committed this
> > updated version. However for the sake of any future submissions you may
> > make can you please explain your current FSF copyright assignment status?
> I am working for Loongson Technology Corporation Limited.
> Loongson has signed the copyright assignment.
> Loongson GNU ANY is RT 1604586.
OK, this sounds good.
> Can I add to the MAINTAINERS list for the convenience of submitting code
> later?
You'll need to request repository write access first and have that
sponsored by a general maintainer. You'll be able to make such a request
once you have got one or a couple of good changes accepted. At that point
you'll add yourself to MAINTAINERS as your first change to the repository.
Maciej
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [COMMITTED PATCH v2] gdb: mips: Fix the handling of complex type of function return value
2022-04-08 17:05 ` Richard Sandiford
@ 2022-04-11 15:37 ` Maciej W. Rozycki
0 siblings, 0 replies; 12+ messages in thread
From: Maciej W. Rozycki @ 2022-04-11 15:37 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Youling Tang, Andrew Burgess, Lancelot SIX, gdb-patches
On Fri, 8 Apr 2022, Richard Sandiford wrote:
> > Richard, would you by any chance know/remember what IRIX/MIPSpro did
> > here?
>
> No, sorry. I remember there were some cases where we had to choose
> between compatibility with MIPSpro and backwards compatibility with
> GCC (which did something different and thus presumably “wrong”).
> Since GCC de facto the ABI for GNU systems, it wasn't always an
> easy choice. But that might not overlap with this case.
OK, thanks for your consideration. I'll try to find some time to get
back to these issues once Stage 1 has opened and then people can discuss
actual code changes proposed and see what to do here.
Maciej
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [COMMITTED PATCH v2] gdb: mips: Fix the handling of complex type of function return value
2022-04-11 15:32 ` Maciej W. Rozycki
@ 2022-04-12 0:46 ` Youling Tang
0 siblings, 0 replies; 12+ messages in thread
From: Youling Tang @ 2022-04-12 0:46 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gdb-patches
Hi, Maciej
On 04/11/2022 11:32 PM, Maciej W. Rozycki wrote:
> Hi Youling,
>
>>> I cannot see you listed in MAINTAINERS, but your original change only had
>>> a legally insignificant amount of changes, so I have now committed this
>>> updated version. However for the sake of any future submissions you may
>>> make can you please explain your current FSF copyright assignment status?
>> I am working for Loongson Technology Corporation Limited.
>> Loongson has signed the copyright assignment.
>> Loongson GNU ANY is RT 1604586.
> OK, this sounds good.
>
>> Can I add to the MAINTAINERS list for the convenience of submitting code
>> later?
> You'll need to request repository write access first and have that
> sponsored by a general maintainer. You'll be able to make such a request
> once you have got one or a couple of good changes accepted. At that point
> you'll add yourself to MAINTAINERS as your first change to the repository.
OK, thanks for your suggestion.
Thanks,
Youling.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-04-12 0:46 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-16 4:48 [PATCH] gdb: mips: Fix the handling of complex type of function return value Youling Tang
2022-03-16 8:42 ` Lancelot SIX
2022-03-16 9:00 ` Youling Tang
2022-03-16 11:22 ` Andrew Burgess
2022-03-16 18:26 ` Maciej W. Rozycki
2022-03-17 7:01 ` Youling Tang
2022-04-06 22:46 ` [COMMITTED PATCH v2] " Maciej W. Rozycki
2022-04-07 2:19 ` Youling Tang
2022-04-11 15:32 ` Maciej W. Rozycki
2022-04-12 0:46 ` Youling Tang
2022-04-08 17:05 ` Richard Sandiford
2022-04-11 15:37 ` Maciej W. Rozycki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).