From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 519DB3858D28 for ; Fri, 8 Apr 2022 17:05:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 519DB3858D28 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0C6911042; Fri, 8 Apr 2022 10:05:22 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1AE473F5A1; Fri, 8 Apr 2022 10:05:20 -0700 (PDT) From: Richard Sandiford To: "Maciej W. Rozycki" Mail-Followup-To: "Maciej W. Rozycki" , Youling Tang , Andrew Burgess , Lancelot SIX , gdb-patches@sourceware.org, richard.sandiford@arm.com Cc: Youling Tang , Andrew Burgess , Lancelot SIX , gdb-patches@sourceware.org Subject: Re: [COMMITTED PATCH v2] gdb: mips: Fix the handling of complex type of function return value References: <1647406106-25723-1-git-send-email-tangyouling@loongson.cn> <20220316084248.m5m2et3njtngeoge@Plymouth> <877d8uuop8.fsf@redhat.com> Date: Fri, 08 Apr 2022 18:05:19 +0100 In-Reply-To: (Maciej W. Rozycki's message of "Wed, 6 Apr 2022 23:46:23 +0100 (BST)") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-5.9 required=5.0 tests=BAYES_00, KAM_ASCII_DIVIDERS, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Apr 2022 17:05:24 -0000 "Maciej W. Rozycki" writes: > From: Youling Tang > > $ objdump -d outputs/gdb.base/varargs/varargs > 00000001200012e8 : > ... > 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 <=3D 2 * MIPS64_REGSIZE, the return va= lue > will be passed through $f0 and $f2, so fix the corresponding processing > in mips_n32n64_return_value(). > > $ make check RUNTESTFLAGS=3D'GDB=3D../gdb gdb.base/varargs.exp --outdir= =3Dtest' > > 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 f= loat _Complex > > Signed-off-by: Youling Tang > Co-Authored-By: Maciej W. Rozycki > --- > Hi Youling, > >> > This has to be double-checked, because as I recall we have an ABI bu= g 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). >> >=20 >> > 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 thoroug= hly >> > 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=20 > this area (more on this later), your change is conceptually a move in the= =20 > right direction except for these issues: > > - $f0/$f2 may only be used if we have an FPU, so the `tdep->mips_fpu_type= '=20 > 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= =20 > fix: > > FAIL: gdb.base/varargs.exp: print find_max_float_real(4, fc1, fc2, fc3, f= c4) > > in my environment because of the latter issue. > > I went ahead and fixed these issues (in addition to code formatting), an= d=20 > added explanatory notes including real/imaginary part usage in particular= .=20 > I have verified the updated change with the `mips64-linux-gnu' big-endian= =20 > target and the n64 ABI using remote `gdbserver' with my Malta 5Kc MIPS64r= 1=20 > system. I have mentioned the gdb.base/callfuncs.exp progression in the=20 > change description too. > > I cannot see you listed in MAINTAINERS, but your original change only ha= d=20 > a legally insignificant amount of changes, so I have now committed this=20 > updated version. However for the sake of any future submissions you may= =20 > 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=20 > 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 =3D d }; > } > > we get this assembly: > > .file 1 "ldouble.c" > .section .mdebug.abi64 > .previous > .nan legacy > .module fp=3D64 > .module oddspreg > .module arch=3Dmips3 > .abicalls > .text > .align 2 > .align 3 > .globl v > .set nomips16 > .set nomicromips > .ent v > .type v, @function > v: > .frame $sp,0,$31 # vars=3D 0, regs=3D 0/0, args=3D 0, gp=3D 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=3D 0, regs=3D 0/0, args=3D 0, gp=3D 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=20 > $f0/$f1 instead. I'll see if I can discuss this with the GCC community=20 > 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.=20= =20 > Considering this program: > > #include > > 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 =3D c }; > } > > double complex > v (double complex c) > { > return c; > } > > complex_t > s (double complex c) > { > return (complex_t) { .c =3D c }; > } > > we get this assembly: > > .file 1 "complex.c" > .section .mdebug.abi64 > .previous > .nan legacy > .module fp=3D64 > .module oddspreg > .module arch=3Dmips3 > .abicalls > .text > .align 2 > .align 3 > .globl vf > .set nomips16 > .set nomicromips > .ent vf > .type vf, @function > vf: > .frame $sp,0,$31 # vars=3D 0, regs=3D 0/0, args=3D 0, gp=3D 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=3D 16, regs=3D 0/0, args=3D 0, gp=3D 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=3D 0, regs=3D 0/0, args=3D 0, gp=3D 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=3D 0, regs=3D 0/0, args=3D 0, gp=3D 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= =20 > it in $2/$3 and especially `sf' goes through the hoops to squeeze it into= =20 > $3 (and the caller then has to unsqueeze it). My understanding of MIPSpr= o=20 > documentation is in both cases the result shall go into $f0/$f2. I think= =20 > this is something to take with GCC as well. > > Richard, would you by any chance know/remember what IRIX/MIPSpro did=20 > 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 =E2=80=9Cwrong=E2=80= =9D). 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 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- 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 fir= st > 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.) */ >=20=20 > if (TYPE_LENGTH (type) > 2 * MIPS64_REGSIZE) > return RETURN_VALUE_STRUCT_CONVENTION; > - else if (type->code () =3D=3D TYPE_CODE_FLT > - && TYPE_LENGTH (type) =3D=3D 16 > + else if ((type->code () =3D=3D TYPE_CODE_COMPLEX > + || (type->code () =3D=3D TYPE_CODE_FLT && TYPE_LENGTH (type) =3D=3D= 16)) > && tdep->mips_fpu_type !=3D 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 () =3D=3D TYPE_CODE_FLT