Re: [COMMITTED PATCH v2] gdb: mips: Fix the handling of complex type of function return value

[Youling Tang <tangyouling@loongson.cn>]

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