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

Youling Tang
2022-04-07T02:20:20Z

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