Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 11 Apr 2016 03:40:03 +0000
From: Jaydeep Patil <Jaydeep.Patil@...tec.com>
To: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: RE: MIPS64 N32 port

>-----Original Message-----
>From: Rich Felker [mailto:dalias@...ifal.cx] On Behalf Of Rich Felker
>Sent: 10 April 2016 AM 06:18
>To: musl@...ts.openwall.com
>Subject: Re: [musl] [MUSL] MIPS64 N32 port
>
>On Tue, Apr 05, 2016 at 05:57:26AM +0000, Jaydeep Patil wrote:
>> Please refer to
>> https://github.com/JaydeepIMG/musl-1/tree/mipsn32port_v1 for rebased
>> MIPS64 N32 port (also attached).
>
>I tested this on my flight back and it seems to be mostly working,
>except for configure failing to detect n32. Some comments inline:
>
>> +#define a_barrier a_barrier
>> +static inline void a_barrier()
>> +{
>> +#if __mips < 2
>> +	/* mips2 sync, but using too many directives causes
>> +	 * gcc not to inline it, so encode with .long instead. */
>> +	__asm__ __volatile__ (".long 0xf" : : : "memory");
>> +#else
>> +	__asm__ __volatile__ ("sync" : : : "memory");
>> +#endif
>> +}
>
>I think __mips<2 is mutually exclusive with 64-bit so it should be
>okay to omit this.
>
>> diff --git a/configure b/configure
>> index 969671d..dbc0198 100755
>> --- a/configure
>> +++ b/configure
>> @@ -307,6 +307,7 @@ i?86*) ARCH=i386 ;;
>>  x86_64-x32*|x32*|x86_64*x32) ARCH=x32 ;;
>>  x86_64-nt64*) ARCH=nt64 ;;
>>  x86_64*) ARCH=x86_64 ;;
>> +mipsn32) ARCH=mipsn32 ;;
>>  mips64*) ARCH=mips64 ;;
>>  mips*) ARCH=mips ;;
>>  microblaze*) ARCH=microblaze ;;
>
>This pattern does not seem to match any tuple actually used; it
>doesn't even have a glob * in it. On my first try, I got a heap of
>warnings and a broken build because musl detected the n32 compiler as
>mips64.
>
>It doesn't seem to be possible to detect from the tuple that a
>compiler is n32, so I think the right fix is something like:
>
>@@ -618,6 +619,7 @@ trycppif __mips_soft_float "$t" &&
>SUBARCH=${SUBARCH}-sf
> fi
>
> if test "$ARCH" = "mips64" ; then
>+trycppif "_MIPS_SIM != _ABI64" "$t" && ARCH=mipsn32
> trycppif "__mips_isa_rev >= 6" "$t" && SUBARCH=${SUBARCH}r6
> trycppif "_MIPSEL || __MIPSEL || __MIPSEL__" "$t" &&
>SUBARCH=${SUBARCH}el
> trycppif __mips_soft_float "$t" && SUBARCH=${SUBARCH}-sf

Ok

>Alternatively, instead of depending on the predefined macros we could
>compile a test program that errors out depending on sizeof(void*)
>being 4 vs 8. If we take the latter option, a new trystaticassert
>function should probably be added to configure to make it easy to
>reuse the logic. But I think the preprocessor version is okay for now.
>
>I'd really like to transition the configure script to only using the
>-dumpmachine tuple as a hint, and only when it's available, and
>checking predefined macros and such to verify the arch.
>
>> @@ -611,6 +612,12 @@ if test "$ARCH" = "aarch64" ; then
>>  trycppif __AARCH64EB__ "$t" && SUBARCH=${SUBARCH}_be
>>  fi
>>
>> +if test "$ARCH" = "mipsn32" ; then
>> +trycppif "__mips_isa_rev >= 6" "$t" && SUBARCH=${SUBARCH}r6
>> +trycppif "_MIPSEL || __MIPSEL || __MIPSEL__" "$t" &&
>SUBARCH=${SUBARCH}el
>> +trycppif __mips_soft_float "$t" && SUBARCH=${SUBARCH}-sf
>> +fi
>> +
>
>This hunk would become unnecessary then.

Yes

>> diff --git a/src/thread/mipsn32/__unmapself.s
>b/src/thread/mipsn32/__unmapself.s
>> new file mode 100644
>> index 0000000..771f8b3
>> --- /dev/null
>> +++ b/src/thread/mipsn32/__unmapself.s
>> @@ -0,0 +1,10 @@
>> +.set	noreorder
>> +.global	__unmapself
>> +.type	__unmapself,@function
>> +__unmapself:
>> +	move	$sp, $25
>> +	li	$2, 6011
>> +	syscall
>> +	li	$4, 0
>> +	li	$2, 6058
>> +	syscall
>
>I don't think the move $sp, $25 is needed here. If you read the git
>history for why it was added, it way to work around an o32 kernel bug
>that does not affect other ABIs. The git log message references the
>relevant kernel commit and I checked the kernel git history and
>verified that the affected code is o32-only.

Ok.

>Otherwise, it looks very good. Thanks for working on this and for your
>patience. If the above suggested changes look okay with you I can make
>them and commit it.

Thanks for the review.
Could you please commit it?

>Rich

Thanks,
Jaydeep


Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.