Re: mips n64 porting review

Re: mips n64 porting review

[Rich Felker]

On Wed, Feb 03, 2016 at 03:41:13PM +0000, Mahesh Bodapati wrote:
> Hi Rich,
> I have attached the patch which has all the MIPS n64 porting work. I
> have created mips64port remote branch on GitHub and the repository
> is https://github.com/MaheshBodapati/musl/tree/mips64port which has
> the broken down patches and the base revision on which I have
> prepared patch is v1.1.12-41-g3abb094.

Some preliminary review:

The relocation support commit still needs a lot of work:

https://github.com/MaheshBodapati/musl/commit/55147891bc0caf757bb0e00a81d550aadc71750c

The good news is, I think it's mostly -'s that it needs. I was puzzled
at first by all the Elf64_Mips_Rel[a] stuff, and found it seems to be
have been copied verbatim from uClibc source (and possibly indirectly
from glibc) which would be a big problem, but I don't think it's
needed anyway. Here's the relevant comment:

https://git.uclibc.org/uClibc/tree/ldso/ldso/mips/dl-sysdep.h?id=966adfe8ed0364ae32cc80d375d736298972d031#n13

In short, somebody's mental model of how MIPS64 relocations work (and
this might actually have relevance for relocations used in .o files)
was that each relocation slot stores up to 3 actual relocations. In
reality, however, the only place where "more than one relocation"
appears in the same lot for dynamic relocations is R_MIPS_REL32
followed by R_MIPS_64, which you're actually treating as one
relocation:

#define REL_SYM_OR_REL  4611		/* (R_MIPS_64 << 8) | R_MIPS_REL32 */

and this latter treatment makes a whole lot more sense. In short, I
think you could remove all of the Elf64_Mips_Rel[a] stuff and the
associated #ifdef __mips64 and just use the standard ELF reloc
processing code paths, and everything should work. If not, it should
only be minor details keeping it from working and it should be easy to
fix them in a cleaner way.

Second thing I noticed was the invasive way you're dealing with the
broken kernel stat structure for mips. This should be done the same
way it's done on 32-bit mips, with the fixup function in
syscall_arch.h.

Third, I suspect socket.h needs to be checked. The kernel usually has
the wrong types for 64-bit archs, and some endian-specific padding is
required to work around it. See how x86_64 and aarch64 do it.

Fourth, I suspect ksigaction.h is mildly wrong ([4] should be [2]) and
this is why the #ifdefs to use _NSIG/8 instead of sizeof were needed.
Fixing that should eliminate the need to change non-arch-specific
files.

Fifth, I don't see why there's a mips64-specific definition for
RLIM_INFINITY; it's the same value as the generic definition.

There are a couple other things that you've got #ifdefs for that are
probably a matter of code that's changed upstream:

pthread_cancel.c should be using the MC_PC macro, which pthread_arch.h
should define, to access the member of mcontext_t containing the
program counter. The definition from 32-bit mips should work.

A recent commit also replaced the DYNAMIC_IS_RO macro with
DT_DEBUG_INDIRECT which was part of making gdb work right on mips. I
suspect the same solution works on mips64.

The bits deduplication patch removed the need for a lot of the bits
headers. The ones that are no longer needed should be left out when we
merge.

The .sub system for subarchs (soft float) was removed and replaced by
support for .S and .c files in arch dirs, so this also needs to be
updated when merging.

I haven't gotten a chance to review everything in full detail yet but
that should be the bulk of what needs to be changed. I'll follow up
with anything else I find.

Thanks for all your work on this!

Rich

Re: mips n64 porting review

[Szabolcs Nagy]

* Mahesh Bodapati <Mahesh.Bodapati@imgtec.com> [2016-02-03 15:41:13 +0000]:
> We have modified 3 test cases in libc test bench and I have attached that patch too.

thanks, fixed.

> We have tested MIPS n64 [mips64r2 to mips64r5] libraries on little endian and here are the FAILS.
> 
> FAIL ./src/api/main.exe [status 1]
ok

> FAIL ./src/functional/pthread_cancel-points-static.exe [timed out]
> FAIL ./src/functional/pthread_cancel-points.exe [timed out]
> FAIL ./src/functional/pthread_cond.exe [timed out]

these should pass

> FAIL ./src/functional/snprintf-static.exe [status 1]
> FAIL ./src/functional/snprintf.exe [status 1]
> FAIL ./src/functional/strtold-static.exe [status 1]
> FAIL ./src/functional/strtold.exe [status 1]
> FAIL ./src/functional/swprintf-static.exe [status 1]
> FAIL ./src/functional/swprintf.exe [status 1]

bits/float.h is wrong

if mips64 uses ieee binary128 then copy aarch64 float.h
otherwise use arm float.h

> FAIL ./src/functional/wcstol-static.exe [status 1]
> FAIL ./src/functional/wcstol.exe [status 1]
ok

> FAIL ./src/math/acosh.exe [status 1]
> FAIL ./src/math/asinh.exe [status 1]
ok

> FAIL ./src/math/fenv.exe [status 1]
> FAIL ./src/math/isless.exe [status 1]
these should pass

> FAIL ./src/math/j0.exe [status 1]
> FAIL ./src/math/jn.exe [status 1]
> FAIL ./src/math/jnf.exe [status 1]
> FAIL ./src/math/lgamma.exe [status 1]
> FAIL ./src/math/lgamma_r.exe [status 1]
> FAIL ./src/math/lgammaf.exe [status 1]
> FAIL ./src/math/lgammaf_r.exe [status 1]
> FAIL ./src/math/sinh.exe [status 1]
> FAIL ./src/math/tgamma.exe [status 1]
> FAIL ./src/math/y0.exe [status 1]
> FAIL ./src/math/y0f.exe [status 1]
> FAIL ./src/math/ynf.exe [status 1]
ok

> FAIL ./src/regression/printf-fmt-g-round-static.exe [status 1]
> FAIL ./src/regression/printf-fmt-g-round.exe [status 1]
bits/float.h

> FAIL ./src/regression/pthread_cond-smasher.exe [status 1]
> FAIL ./src/regression/pthread_condattr_setclock.exe [status 1]

should work.

> 
> We haven't test on big endian boards and we are doing it now.
> 
> Thanks,
> Mahesh
> 
> 

Re: Re: mips n64 porting review

[Rich Felker]

On Wed, Feb 03, 2016 at 06:36:57PM -0500, Rich Felker wrote:
> On Wed, Feb 03, 2016 at 03:41:13PM +0000, Mahesh Bodapati wrote:
> > Hi Rich,
> > I have attached the patch which has all the MIPS n64 porting work. I
> > have created mips64port remote branch on GitHub and the repository
> > is https://github.com/MaheshBodapati/musl/tree/mips64port which has
> > the broken down patches and the base revision on which I have
> > prepared patch is v1.1.12-41-g3abb094.
> 
> Some preliminary review:

One more thing that came up in reviewing syscall_cp.s was actually a
bug copied from existing code in musl, which is fixed by this commit:

http://git.musl-libc.org/cgit/musl/commit/?id=756c8af8589265e99e454fe3adcda1d0bc5e1963

In practice the code seemed to work but it was wrong with respect to
ABI requirements.

I think the way you're saving $gp on the stack in sigsetjmp.s is also
invalid since that part of the stack will have been clobbered by the
time setjmp returns a second time. You could save it inside an unused
part of the jump buffer, but it might be better to just avoid the $gp
register and instead use temp registers and possibly some pc-relative
address computations.

Rich

Re: Re: mips n64 porting review

[Rich Felker]

On Thu, Feb 04, 2016 at 06:52:47PM -0500, Rich Felker wrote:
> On Wed, Feb 03, 2016 at 06:36:57PM -0500, Rich Felker wrote:
> > On Wed, Feb 03, 2016 at 03:41:13PM +0000, Mahesh Bodapati wrote:
> > > Hi Rich,
> > > I have attached the patch which has all the MIPS n64 porting work. I
> > > have created mips64port remote branch on GitHub and the repository
> > > is https://github.com/MaheshBodapati/musl/tree/mips64port which has
> > > the broken down patches and the base revision on which I have
> > > prepared patch is v1.1.12-41-g3abb094.
> > 
> > Some preliminary review:
> 
> One more thing that came up in reviewing syscall_cp.s was actually a
> bug copied from existing code in musl, which is fixed by this commit:
> 
> http://git.musl-libc.org/cgit/musl/commit/?id=756c8af8589265e99e454fe3adcda1d0bc5e1963
> 
> In practice the code seemed to work but it was wrong with respect to
> ABI requirements.
> 
> I think the way you're saving $gp on the stack in sigsetjmp.s is also
> invalid since that part of the stack will have been clobbered by the
> time setjmp returns a second time. You could save it inside an unused
> part of the jump buffer, but it might be better to just avoid the $gp
> register and instead use temp registers and possibly some pc-relative
> address computations.

Likewise in pipe.s, it's overly complicated by the fact that it's
saving $gp on the stack and thereby can't make a tail call to
__syscall_ret like it's intended to. If you just use a different
register instead of $gp that's call-clobbered you can tail-call just
like the 32-bit mips version. Alternatively it's possible now to write
src/unistd/mips64/pipe.c instead using inline asm and let the compiler
handle the nastiness of the calling convention. I'd be happy with
either approach; the C version is probably mildly nicer, especially if
the compiler still does the tail-call right and doesn't generate come
bloated monstrosity. (Note that you can't use C for sigsetjmp though
because it can't use the stack.)

At this point I think I've looked through all the mips64 files at
least once and caught all the important issues.

Rich

Re: Re: mips n64 porting review

[Rich Felker]

On Thu, Feb 04, 2016 at 11:27:34PM -0500, Rich Felker wrote:
> On Thu, Feb 04, 2016 at 06:52:47PM -0500, Rich Felker wrote:
> > On Wed, Feb 03, 2016 at 06:36:57PM -0500, Rich Felker wrote:
> > > On Wed, Feb 03, 2016 at 03:41:13PM +0000, Mahesh Bodapati wrote:
> > > > Hi Rich,
> > > > I have attached the patch which has all the MIPS n64 porting work. I
> > > > have created mips64port remote branch on GitHub and the repository
> > > > is https://github.com/MaheshBodapati/musl/tree/mips64port which has
> > > > the broken down patches and the base revision on which I have
> > > > prepared patch is v1.1.12-41-g3abb094.
> > > 
> > > Some preliminary review:
> > 
> > One more thing that came up in reviewing syscall_cp.s was actually a
> > bug copied from existing code in musl, which is fixed by this commit:
> > 
> > http://git.musl-libc.org/cgit/musl/commit/?id=756c8af8589265e99e454fe3adcda1d0bc5e1963
> > 
> > In practice the code seemed to work but it was wrong with respect to
> > ABI requirements.
> > 
> > I think the way you're saving $gp on the stack in sigsetjmp.s is also
> > invalid since that part of the stack will have been clobbered by the
> > time setjmp returns a second time. You could save it inside an unused
> > part of the jump buffer, but it might be better to just avoid the $gp
> > register and instead use temp registers and possibly some pc-relative
> > address computations.
> 
> Likewise in pipe.s, it's overly complicated by the fact that it's
> saving $gp on the stack and thereby can't make a tail call to
> __syscall_ret like it's intended to. If you just use a different
> register instead of $gp that's call-clobbered you can tail-call just
> like the 32-bit mips version. Alternatively it's possible now to write
> src/unistd/mips64/pipe.c instead using inline asm and let the compiler
> handle the nastiness of the calling convention. I'd be happy with
> either approach; the C version is probably mildly nicer, especially if
> the compiler still does the tail-call right and doesn't generate come
> bloated monstrosity. (Note that you can't use C for sigsetjmp though
> because it can't use the stack.)
> 
> At this point I think I've looked through all the mips64 files at
> least once and caught all the important issues.

Ping. Any questions or updates on mips64?

Rich

RE: Re: mips n64 porting review

[Jaydeep Patil]

Hi Rich,

Thanks for your comments. We are working on rebasing the patch. 

Regards,
Jaydeep

-----Original Message-----
From: Rich Felker [mailto:dalias@aerifal.cx] On Behalf Of Rich Felker
Sent: 13 February 2016 AM 12:21
To: Mahesh Bodapati
Cc: Szabolcs Nagy; Jaydeep Patil; Anand Takale; musl@lists.openwall.com
Subject: Re: [musl] Re: mips n64 porting review

On Thu, Feb 04, 2016 at 11:27:34PM -0500, Rich Felker wrote:
> On Thu, Feb 04, 2016 at 06:52:47PM -0500, Rich Felker wrote:
> > On Wed, Feb 03, 2016 at 06:36:57PM -0500, Rich Felker wrote:
> > > On Wed, Feb 03, 2016 at 03:41:13PM +0000, Mahesh Bodapati wrote:
> > > > Hi Rich,
> > > > I have attached the patch which has all the MIPS n64 porting 
> > > > work. I have created mips64port remote branch on GitHub and the 
> > > > repository is 
> > > > https://github.com/MaheshBodapati/musl/tree/mips64port which has 
> > > > the broken down patches and the base revision on which I have prepared patch is v1.1.12-41-g3abb094.
> > > 
> > > Some preliminary review:
> > 
> > One more thing that came up in reviewing syscall_cp.s was actually a 
> > bug copied from existing code in musl, which is fixed by this commit:
> > 
> > http://git.musl-libc.org/cgit/musl/commit/?id=756c8af8589265e99e454f
> > e3adcda1d0bc5e1963
> > 
> > In practice the code seemed to work but it was wrong with respect to 
> > ABI requirements.
> > 
> > I think the way you're saving $gp on the stack in sigsetjmp.s is 
> > also invalid since that part of the stack will have been clobbered 
> > by the time setjmp returns a second time. You could save it inside 
> > an unused part of the jump buffer, but it might be better to just 
> > avoid the $gp register and instead use temp registers and possibly 
> > some pc-relative address computations.
> 
> Likewise in pipe.s, it's overly complicated by the fact that it's 
> saving $gp on the stack and thereby can't make a tail call to 
> __syscall_ret like it's intended to. If you just use a different 
> register instead of $gp that's call-clobbered you can tail-call just 
> like the 32-bit mips version. Alternatively it's possible now to write 
> src/unistd/mips64/pipe.c instead using inline asm and let the compiler 
> handle the nastiness of the calling convention. I'd be happy with 
> either approach; the C version is probably mildly nicer, especially if 
> the compiler still does the tail-call right and doesn't generate come 
> bloated monstrosity. (Note that you can't use C for sigsetjmp though 
> because it can't use the stack.)
> 
> At this point I think I've looked through all the mips64 files at 
> least once and caught all the important issues.

Ping. Any questions or updates on mips64?

Rich