Closed Bug 767759 Opened 7 years ago Closed 6 years ago

Add support for new x32 abi for nspr

Categories

(NSPR :: NSPR, defect, P2)

4.9.1
x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
4.10.4

People

(Reporter: anarchy, Assigned: wtc)

Details

Attachments

(4 files, 6 obsolete files)

Attached patch nspr x32 support (obsolete) — Splinter Review
NSPR uses preprocessor definitions like __x86_64__ to guess implementation details like type sizes, and to know which version of assembly code to compile. This makes it guess the wrong type sizes on x32, and compile the 64-bit version of assembly code. This prevents programs from being able to link at compile time.
Attachment #636129 - Flags: review?(wtc)
Assignee: nobody → wtc
Component: Build Config → NSPR
Product: Core → NSPR
QA Contact: build-config → nspr
Target Milestone: --- → 4.9.2
Version: unspecified → 4.6.2
Version: 4.6.2 → 4.9.1
Comment on attachment 636129 [details] [diff] [review]
nspr x32 support

Jory: thank you for the patch.

I have never heard of this meaning of __x86_64__.  In my experience,
the __x86_64__ macro always implies the LP64 data model.  I believe
a lot of code makes this assumption.

Is this not the case for Gentoo Linux?  Is this a recent change?
Attachment #636129 - Flags: superreview?(ted.mielczarek)
(In reply to Wan-Teh Chang from comment #1)
> Comment on attachment 636129 [details] [diff] [review]
> nspr x32 support
> 
> Jory: thank you for the patch.
> 
> I have never heard of this meaning of __x86_64__.  In my experience,
> the __x86_64__ macro always implies the LP64 data model.  I believe
> a lot of code makes this assumption.
> 
> Is this not the case for Gentoo Linux?  Is this a recent change?

This is a recent change in glibc/gcc glibc-2.16 and gcc-4.7 are the first to offer x32 support. You can read more about the x32 abi at https://sites.google.com/site/x32abi/
> +#elif defined(__x86_64__) && defined(__LP64)
__LP64__ ?
(In reply to Masatoshi Kimura [:emk] from comment #3)
> > +#elif defined(__x86_64__) && defined(__LP64)
> __LP64__ ?

That would be correct, Damn typo creeped in during my port to mozilla-inbound for patch generation for upstream.
Comment on attachment 636129 [details] [diff] [review]
nspr x32 support

Review of attachment 636129 [details] [diff] [review]:
-----------------------------------------------------------------

::: nsprpub/pr/include/md/_linux.cfg
@@ +210,5 @@
>  #define PR_BYTES_PER_WORD_LOG2  3
>  #define PR_BYTES_PER_DWORD_LOG2 3
>  
>  #elif defined(__x86_64__)
> +#ifdef __LP64__

The changes to this file are fine.  I recommend testing
the __ILP32__ macro instead.  I will take care of this.

::: nsprpub/pr/include/md/_linux.h
@@ +39,1 @@
>  #define _PR_SI_ARCHITECTURE "x86"

I think we should define _PR_SI_ARCHITECTURE as "x32" for
the "defined(__x86_64__) && !defined(__LP64__)" case.

x32 binaries are different from x86 binaries, right?

@@ +103,5 @@
>  extern PRInt32 _PR_ia64_AtomicSet(PRInt32 *val, PRInt32 newval);
>  #define _MD_ATOMIC_SET                _PR_ia64_AtomicSet
>  #endif
>  
> +#if defined(__x86_64__) && defined(__LP64__)

I suspect the two changes related to atomic ops are not necessary.

The x32 build should be able to use the x86_64 assembly code in
mozilla/nsprpub/pr/src/md/unix/os_Linux_x86_64.s, right?  This is
because none of the functions defined in os_Linux_x86_64.s use
the 'long' or 'pointer' types.

If you are worried that this assumption will no longer hold when
we add new functions to os_Linux_x86_64.s, we can fork the file
and create os_Linux_x32.s.
Test __ILP32__ instead of __LP64__, just in case some compiler
does not define the __LP64__ macro for the x86_64 ABI.  (This is
extremely unlikely though.)
Attachment #644058 - Flags: review?(anarchy)
(In reply to Wan-Teh Chang from comment #5)
> Comment on attachment 636129 [details] [diff] [review]
> I suspect the two changes related to atomic ops are not necessary.
> 
> The x32 build should be able to use the x86_64 assembly code in
> mozilla/nsprpub/pr/src/md/unix/os_Linux_x86_64.s, right?  This is
> because none of the functions defined in os_Linux_x86_64.s use
> the 'long' or 'pointer' types.
> 
> If you are worried that this assumption will no longer hold when
> we add new functions to os_Linux_x86_64.s, we can fork the file
> and create os_Linux_x32.s.

# nm -D /usr/lib/libnspr4.so.9 |grep -i atomic
0000000000016720 T PR_AtomicAdd
0000000000016700 T PR_AtomicDecrement
00000000000166f0 T PR_AtomicIncrement
0000000000016710 T PR_AtomicSet
000000000002c6b8 T _PR_x86_64_AtomicAdd
000000000002c6a4 T _PR_x86_64_AtomicDecrement
000000000002c698 T _PR_x86_64_AtomicIncrement
000000000002c6b0 T _PR_x86_64_AtomicSet

On x32 with your patch:

# nm -D /usr/libx32/libnspr4.so.9 |grep -i atomic
00014cf0 T PR_AtomicAdd
00014cd0 T PR_AtomicDecrement
00014cc0 T PR_AtomicIncrement
00014ce0 T PR_AtomicSet
         U _PR_x86_64_AtomicAdd
         U _PR_x86_64_AtomicDecrement
         U _PR_x86_64_AtomicIncrement
         U _PR_x86_64_AtomicSet
0002b3c0 T _PR_x86_AtomicAdd
0002b39c T _PR_x86_AtomicDecrement
0002b388 T _PR_x86_AtomicIncrement
0002b3b0 T _PR_x86_AtomicSet

I am a bit concerned later new functions could break this again, it might be best to create the os_linux_x32.s and use it rather then depending on either.
Jory: thank you for the nm command output.  This shows that
we need to change mozilla/nsprpub/configure.in so that
os_Linux_x86_64.s (or the new os_Linux_x32.s file) is used
for the x32 build:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.in&rev=1.334&mark=1822,1824,1826-1829#1822

Could you attach the NSPR build log?  I'd like to take a look
to see if -Di386 and -m32 are on the gcc command line.  Thanks.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: 4.9.2 → 4.9.3
(In reply to Wan-Teh Chang from comment #8)
> Jory: thank you for the nm command output.  This shows that
> we need to change mozilla/nsprpub/configure.in so that
> os_Linux_x86_64.s (or the new os_Linux_x32.s file) is used
> for the x32 build:
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/configure.
> in&rev=1.334&mark=1822,1824,1826-1829#1822
> 
> Could you attach the NSPR build log?  I'd like to take a look
> to see if -Di386 and -m32 are on the gcc command line.  Thanks.

Sure I can attach it in a few, It actually passes -mx32 with Di386=1
Status: ASSIGNED → NEW
Priority: P2 → --
Target Milestone: 4.9.3 → 4.9.2
Attached file build.log (obsolete) —
Target Milestone: 4.9.2 → 4.9.3
Comment on attachment 644068 [details]
build.log

Thank you for the NSPR build log.

>checking whether gcc -m32 needs -traditional... no
...
>checking whether gcc -m32 accepts -pthread... yes

These are evidences that mozilla/nsprpub/configure.in adds the -m32
flag to the CC variable.

>make -j5 CC=x86_64-pc-linux-gnu-gcc CXX=x86_64-pc-linux-gnu-g++ 

Here you override the CC variable used by makefiles.  If you simply say

  make -j5

I believe you will see -m32 on the compiler command line.

It is better to pass your desired CC to the configure script in its
environment, as follows:

  CC=x86_64-pc-linux-gnu-gcc CXX=x86_64-pc-linux-gnu-g++ mozilla/nsprpub/configure <...options...>

and then simply say "make".
Comment on attachment 644058 [details] [diff] [review]
Patch for mozilla/nsprpub/pr/include/md/_linux.cfg (checked in)

As shown earlier we will end up with undefined symbols being included.
Attachment #644058 - Flags: review?(anarchy) → review-
Attached patch update with __ILP32__ (obsolete) — Splinter Review
As you suggested I have changed __LP64__ to __ILP32__, I do not believe we will need to create anything else, using the x86 asm should be fine. If at a later time it is determined to be insufficient we can always modify to create an x32 specific asm file.
Attachment #636129 - Attachment is obsolete: true
Attachment #636129 - Flags: superreview?(ted.mielczarek)
Attachment #636129 - Flags: review?(wtc)
Attachment #644114 - Flags: review?(wtc)
Comment on attachment 644058 [details] [diff] [review]
Patch for mozilla/nsprpub/pr/include/md/_linux.cfg (checked in)

Jory: I'm sorry I wasn't clear.  This patch is only part of the
complete solution.  This patch is meant to replace the changes
to _linux.cfg in your patch.  We still need to patch _linux.h
and possibly mozilla/nsprpub/configure.in.  I created this separate
patch so that we can do the x32 porting incrementally.
Comment on attachment 644114 [details] [diff] [review]
update with __ILP32__

Review of attachment 644114 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the new patch.

::: nsprpub/pr/include/md/_linux.h
@@ +34,5 @@
>  #elif defined(__sparc__) && defined(__arch64__)
>  #define _PR_SI_ARCHITECTURE "sparc64"
>  #elif defined(__sparc__)
>  #define _PR_SI_ARCHITECTURE "sparc"
> +#elif defined(__i386__) || (defined(__x86_64__) && defined(__ILP32__))

For the "defined(__x86_64__) && defined(__ILP32__)" case,
_PR_SI_ARCHITECTURE should be defined as "x32", right?

I think the x32 ABI is different from the x86 ABI, even though
both are 32-bit.  Correct?

@@ +77,5 @@
>  #ifdef __FreeBSD_kernel__
>  #define _PR_HAVE_SOCKADDR_LEN
>  #endif
>  
> +#if defined(__i386__) || (defined(__x86_64__) && defined(__ILP32__))

Although the x32 build can use the i386 assembly code, it won't take
advantage of the x32 ABI, such as the additional registers.  This is
why I suggested that we figure out how to change mozilla/nsprpub/configure.in
so that the x32 build will use os_Linux_x86_64.s.

In any case, we will need to change mozilla/nsprpub/configure.in
so that -Di386=1 won't appear in the compiler command line.
Comment on attachment 644058 [details] [diff] [review]
Patch for mozilla/nsprpub/pr/include/md/_linux.cfg (checked in)

As Wan-Teh pointed out he wants to handle this in increments the patch is fine.
Attachment #644058 - Flags: review- → review+
(In reply to Wan-Teh Chang from comment #15)
> Comment on attachment 644114 [details] [diff] [review]
> update with __ILP32__
> 
> Review of attachment 644114 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the new patch.
> 
> ::: nsprpub/pr/include/md/_linux.h
> @@ +34,5 @@
> >  #elif defined(__sparc__) && defined(__arch64__)
> >  #define _PR_SI_ARCHITECTURE "sparc64"
> >  #elif defined(__sparc__)
> >  #define _PR_SI_ARCHITECTURE "sparc"
> > +#elif defined(__i386__) || (defined(__x86_64__) && defined(__ILP32__))
> 
> For the "defined(__x86_64__) && defined(__ILP32__)" case,
> _PR_SI_ARCHITECTURE should be defined as "x32", right?
> 
> I think the x32 ABI is different from the x86 ABI, even though
> both are 32-bit.  Correct?
> 
It is different as it can utilizes 64bit pointers while still being a 32bit lib so yes I would agree the define should be x32.
> @@ +77,5 @@
> >  #ifdef __FreeBSD_kernel__
> >  #define _PR_HAVE_SOCKADDR_LEN
> >  #endif
> >  
> > +#if defined(__i386__) || (defined(__x86_64__) && defined(__ILP32__))
> 
> Although the x32 build can use the i386 assembly code, it won't take
> advantage of the x32 ABI, such as the additional registers.  This is
> why I suggested that we figure out how to change mozilla/nsprpub/configure.in
> so that the x32 build will use os_Linux_x86_64.s.
> 
> In any case, we will need to change mozilla/nsprpub/configure.in
> so that -Di386=1 won't appear in the compiler command line.

I will look at doing up a patch for configure, are we wanting to utilize os_Linux_X86_64.s or create os_Linux_X32.s? I do not see a problem fixing the configure so we can remove the -Di386=1 being passed to the compiler.
Jory: for configure, the most important goal is to get rid of the
AC_DEFINE(i386).  A nice-to-have goal is to use os_Linux_x86_64.s.
(We don't need to create the os_Linux_x32.s fork.)  Thanks.
(In reply to Wan-Teh Chang from comment #18)
> Jory: for configure, the most important goal is to get rid of the
> AC_DEFINE(i386).  A nice-to-have goal is to use os_Linux_x86_64.s.
> (We don't need to create the os_Linux_x32.s fork.)  Thanks.

Okay this is not gonna be part of mozilla issues, it will be mine in gentoo as we try to decide how to handle the --enable-64bit, if I pass --enable-64bit on x32 all works as expected with just the minor changes to _linux.cfg
Sorry was meaning to include the updates nm -D /usr/libx32/libnspr4.so.9 output so here it is. 

00015510 T PR_AtomicAdd
000154b0 T PR_AtomicDecrement
00015480 T PR_AtomicIncrement
000154e0 T PR_AtomicSet
00030180 T _PR_x86_64_AtomicAdd
0003016c T _PR_x86_64_AtomicDecrement
00030160 T _PR_x86_64_AtomicIncrement
00030178 T _PR_x86_64_AtomicSet
(In reply to Jory A. Pratt from comment #19)
>
> Okay this is not gonna be part of mozilla issues, it will be mine in gentoo
> as we try to decide how to handle the --enable-64bit, if I pass
> --enable-64bit on x32 all works as expected with just the minor changes to
> _linux.cfg

I see.  We may need to add a --enable-x32 option to configure.
(In reply to Wan-Teh Chang from comment #21)
> (In reply to Jory A. Pratt from comment #19)
> >
> > Okay this is not gonna be part of mozilla issues, it will be mine in gentoo
> > as we try to decide how to handle the --enable-64bit, if I pass
> > --enable-64bit on x32 all works as expected with just the minor changes to
> > _linux.cfg
> 
> I see.  We may need to add a --enable-x32 option to configure.

I do think it is needed, the problem is in gentoo we try to support to many archs and cross compilers.

	echo > "${T}"/test.c
	$(tc-getCC) -c "${T}"/test.c -o "${T}"/test.o
	case $(scanelf -BF'%M' "${T}"/test.o)$(scanmacho -BF'%M' "${T}"/test.o) in
		ELFCLASS64*|POWERPC64*|X86_64*) myconf="${myconf} --enable-64bit";;
		ELFCLASS32*|POWERPC*|I386*|ARM*) ;;
		*) die "Failed to detect whether your arch is 64bits or 32bits, disable distcc if you're using it, please";;
	esac

As you can see we compile a test and check its ELFCLASS to determine weather we will enable--64bit or leave it disabled
Yes, let's add a --enable-x32 option to configure.
So, when building NSPR on x86_64, we will do
- an i386 (x86) build by default,
- an x32 build if --enable-x32 is specified, or
- an x86_64 (x64) build if --enable-64bit is specified.
Comment on attachment 644114 [details] [diff] [review]
update with __ILP32__

Jory: thank you for the patch. Sorry I forgot to review this.

The _linux.cfg changes are good. I will check them in first.

In nsprpub/pr/include/md/_linux.h:

>-#elif defined(__i386__)
>+#elif defined(__i386__) || (defined(__x86_64__) && defined(__ILP32__))
> #define _PR_SI_ARCHITECTURE "x86"

The architecture should be "x32" for defined(__x86_64__) && defined(__ILP32__).
Attachment #644114 - Flags: review?(wtc) → review-
Comment on attachment 644058 [details] [diff] [review]
Patch for mozilla/nsprpub/pr/include/md/_linux.cfg (checked in)

Patch checked in on the NSPR trunk.

Checking in _linux.cfg;
/cvsroot/mozilla/nsprpub/pr/include/md/_linux.cfg,v  <--  _linux.cfg
new revision: 3.29; previous revision: 3.28
done
Attachment #644058 - Attachment description: Patch for mozilla/nsprpub/pr/include/md/_linux.cfg → Patch for mozilla/nsprpub/pr/include/md/_linux.cfg (checked in)
Priority: -- → P2
Target Milestone: 4.9.3 → 4.9.4
Target Milestone: 4.9.4 → 4.9.5
Target Milestone: 4.9.5 → 4.9.6
Target Milestone: 4.9.6 → 4.10
Target Milestone: 4.10 → 4.10.1
Target Milestone: 4.10.1 → 4.10.2
Attached patch add --enable-x32 to configure (obsolete) — Splinter Review
What has been checked in is not enough, you need to drop -m32/-m64 as well.  Other than for weird multilib systems, there should be no need to specify -mx32 explicitly, but there is no harm in doing so.

I attached Daniel Schepler's patch to do so (from http://bugs.debian.org/699185) as-is; it needs mozilla/nsprpub -> nspr, but appears to work for me.
Attached patch Patch by wtc (obsolete) — Splinter Review
I wrote this patch without looking at the other patches.
I will compare it with the other patches tomorrow.
Attachment #644114 - Attachment is obsolete: true
Attachment #644068 - Attachment is obsolete: true
My patch is essentially the same as Daniel Schepler's patch,
so I checked in his patch, after making a small change (to
initialize USE_X32 to empty at the beginning).

https://hg.mozilla.org/projects/nspr/rev/439404406e4b
Attachment #8368936 - Attachment is obsolete: true
Attachment #8378473 - Flags: checked-in+
I compiled nspr/pr/include/gencfg.c with -mx32 and generated the
correct values of the alignments of int64 (long long) and double
for x32.
Attachment #8378475 - Flags: review?(kaie)
This patch contains two independent changes. I believe both are optional.

1. Expose the USE_X32 variable to makefiles. Right now no makefile needs
to know if we are targeting x32, so this is for future use.

2. Set COMPILER_TAG to _x32. COMPILER_TAG is a component of NSPR's
OBJDIR name. I am not sure if it is still being used.
Attachment #8378076 - Attachment is obsolete: true
Attachment #8378477 - Flags: review?(kaie)
I searched for "amd64", "x86_64", and "x86-64" in the NSPR source tree
and inspected all the relevant code. I believe we don't need any other
changes.
Status: NEW → ASSIGNED
Target Milestone: 4.10.2 → 4.10.4
Comment on attachment 644114 [details] [diff] [review]
update with __ILP32__

Review of attachment 644114 [details] [diff] [review]:
-----------------------------------------------------------------

::: nsprpub/pr/include/md/_linux.h
@@ +31,1 @@
>  #define _PR_SI_ARCHITECTURE "x86-64"

Note that we still define _PR_SI_ARCHITECTURE as "x86-64" for the x32 ABI.
I think this is reasonable and there is precedence in doing something
similar: in _solaris.h, _PR_SI_ARCHITECTURE is defined as "sparc" for both
32-bit and 64-bit SPARC.

We can change this in the future if this causes confusion.
Comment on attachment 8378475 [details] [diff] [review]
Fix the alignments of int64 (long long) and double for x32

Review of attachment 8378475 [details] [diff] [review]:
-----------------------------------------------------------------

Kai, could you please review this patch? The other patch is
not needed. Thanks.
Attachment #8378475 - Flags: review?(kaie) → review+
Attachment #8378477 - Flags: review?(kaie) → review+
Comment on attachment 8378475 [details] [diff] [review]
Fix the alignments of int64 (long long) and double for x32

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/45b34c9e0627
Attachment #8378475 - Flags: checked-in+
I found that the RELEASE_OBJDIR_NAME variable in NSPR's makefiles
isn't being used, and it has diverged from NSS's OBJDIR_NAME. So
I removed the COMPILER_TAG change from the patch.

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/fcc324c913da
Attachment #8378477 - Attachment is obsolete: true
Attachment #8386429 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.