676584 - fix NSPR compilation for Darwin/ARM

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Doug Turner (:dougt)]

Attachment: fix nspr complication

NSPR should build on iOS

Comment 1

[Doug Turner (:dougt)]

Attachment: allow forcing cross-compile with CROSS_COMPILE=1, to make targeting the simulator a cross-compile

Comment 2

[Doug Turner (:dougt)]

patches from ted.

Comment 3

[Wan-Teh Chang]

Comment on attachment 550745 [details] [diff] [review]
fix nspr complication

Thank you for the patch.  I suggest some changes.

In nsprpub/config/autoconf.mk.in, you added three lines:

>+OS_FRAMEWORKS   = @OS_FRAMEWORKS@
...
>+BUNDLE_DSO_LDOPTS	= @BUNDLE_DSO_LDOPTS@
>+LDOPTS		= @LDOPTS@

But these three variables are not used by any makefile.
Can they be removed?

In nsprpub/configure.in:

>-        _SAVE_CXXFLAGS=$CXXLAGS
>+        _SAVE_CXXFLAGS=$CXXFLAGS

Good typo fix!  I guess it hasn't been noticed because NSPR doesn't
have C++ code.

>     case "$build:$target" in
>-      powerpc-apple-darwin8*:i?86-apple-darwin*)
>+      powerpc-apple-darwin8*:i?86-apple-darwin*|*-apple-darwin*:arm*-apple-darwin*)

What is *-apple-darwin* for?  What does '|' mean here?

>     case "${target_cpu}" in
>+        arm*)
>+            AC_DEFINE(arm)
>+            AC_DEFINE(ARM)
>+            CPU_ARCH=ARM
>+            ;;

Remove the two AC_DEFINE's.

Set CPU_ARCH to arm, lowercase.

>+AC_SUBST(BUNDLE_DSO_LDOPTS)
>+AC_SUBST(LDOPTS)
...
>+AC_SUBST(OS_FRAMEWORKS)

I don't see any new code in nsprpub/configure.in that sets these
three variables.  Where do they come from?

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

> #ifdef XP_MACOSX
> #include <AvailabilityMacros.h>
> #endif
> 
>+#include <TargetConditionals.h>

Can you move this inside #ifdef XP_MACOSX?

>-#ifdef __x86_64__
>+#if defined(__x86_64__) || defined(TARGET_OS_IPHONE)

This should be simply TARGET_OS_IPHONE, because TARGET_OS_IPHONE
may be defined as 0.

In nsprpub/pr/src/Makefile.in:

> ifeq ($(OS_TARGET),MacOSX)
>-OS_LIBS		= -framework CoreServices -framework CoreFoundation
>+OS_LIBS                = -framework CoreServices -framework CoreFoundation
> endif

If this is just a whitespace change, please undo it.

In nsprpub/pr/src/md/unix/uxproces.c:

> #if defined(DARWIN)
>+#ifdef HAVE_CRT_EXTERNS_H
> #include <crt_externs.h>
> #else
>+PR_IMPORT(char ***) _NSGetEnviron(void);
>+#endif
>+#else
> PR_IMPORT_DATA(char **) environ;
> #endif

Are you sure _NSGetEnviron() is defined when the <crt_externs.h>
header doesn't exist?

If the <crt_externs.h> header doesn't exist, does it mean the
function is considered undocumented and should not be used?

Comment 4

[Wan-Teh Chang]

Comment on attachment 550746 [details] [diff] [review]
allow forcing cross-compile with CROSS_COMPILE=1, to make targeting the simulator a cross-compile

r=wtc.  Please add a comment to explain that "this makes targeting
the iOS simulator a cross-compile".

Comment 5

[(not currently active) Ted Mielczarek]

It's possible that some of this patch is unnecessary. I wrote it while experimenting, so I may have iterated but forgotten to remove some things.(In reply to comment #3)

> Comment on attachment 550745 [details] [diff] [review] [diff] [details] [review]
> Are you sure _NSGetEnviron() is defined when the <crt_externs.h>
> header doesn't exist?

It does exist on the device, it works.

> If the <crt_externs.h> header doesn't exist, does it mean the
> function is considered undocumented and should not be used?

...but this is also true. I actually looked around recently and found some people saying that their apps were rejected by Apple for using _NSGetEnviron().

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Wan-Teh Chang from comment #3)
> >     case "$build:$target" in
> >-      powerpc-apple-darwin8*:i?86-apple-darwin*)
> >+      powerpc-apple-darwin8*:i?86-apple-darwin*|*-apple-darwin*:arm*-apple-darwin*)
> 
> What is *-apple-darwin* for?  What does '|' mean here?

The | is just the separator for multiple patterns in a bash case statement:
http://www.gnu.org/software/bash/manual/bashref.html#Conditional-Constructs

*-apple-darwin is there because the case is "$build:$target", so we want to match "compiling on any host Darwin system to arm-darwin". I guess we could be even more liberal there and just make it "*:arm*-apple-darwin*", to handle cross-compiling from any OS to arm-darwin.


> >+AC_SUBST(BUNDLE_DSO_LDOPTS)
> >+AC_SUBST(LDOPTS)
> ...
> >+AC_SUBST(OS_FRAMEWORKS)
> 
> I don't see any new code in nsprpub/configure.in that sets these
> three variables.  Where do they come from?

I think they're leftovers from a previous version of the patch. I'll remove them.
 
> In nsprpub/pr/src/Makefile.in:
> 
> > ifeq ($(OS_TARGET),MacOSX)
> >-OS_LIBS		= -framework CoreServices -framework CoreFoundation
> >+OS_LIBS                = -framework CoreServices -framework CoreFoundation
> > endif
> 
> If this is just a whitespace change, please undo it.

It looks like I replaced tabs with spaces. I don't know if that was intentional or not. I'll revert it.

I'll have to fix the uxproces.c code to not use _NSGetEnviron. I think maybe we can just leave childEnvp == NULL so the code will use the existing execv path instead of execve.

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Wan-Teh Chang from comment #3)
> In nsprpub/pr/include/md/_darwin.h:
> 
> > #ifdef XP_MACOSX
> > #include <AvailabilityMacros.h>
> > #endif
> > 
> >+#include <TargetConditionals.h>
> 
> Can you move this inside #ifdef XP_MACOSX?

This doesn't currently work, because the AC_DEFINE(XP_MACOSX) is inside a check for Carbon, which fails when targeting iOS:
http://mxr.mozilla.org/nspr/source/nsprpub/configure.in#1362

I can put this inside an #ifdef __APPLE__ if you'd like.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: fix NSPR compilation for arm-darwin

This addresses all the review comments from above, except where I've explicitly replied to your comments. NSPR compiles successfully with this patch, and I ran some of the unittests on a device successfully. I tested pr/tests/parent.c on-device to verify that my uxproces.c changes worked, it ran correctly.

Comment 9

[Wan-Teh Chang]

Comment on attachment 553545 [details] [diff] [review]
fix NSPR compilation for arm-darwin

Ted: thank you for the patch.  I suggest some changes.

Please omit nsprpub/configure in patches.

In nsprpub/configure.in:

>     case "$build:$target" in 
>       powerpc-apple-darwin8*:i?86-apple-darwin*)
>         dnl The Darwin cross compiler doesn't necessarily point itself at a
>         dnl root that has libraries for the proper architecture, it defaults
>         dnl to the system root.  The libraries in the system root on current
>         dnl versions of PPC OS X 10.4 aren't fat, so these target compiler
>         dnl checks will fail.  Fake a working SDK in that case.
>         _SAVE_CFLAGS=$CFLAGS 
>-        _SAVE_CXXFLAGS=$CXXLAGS
>+        _SAVE_CXXFLAGS=$CXXFLAGS
>         CFLAGS="-isysroot /Developer/SDKs/MacOSX10.4u.sdk $CFLAGS"
>         CXXFLAGS="-isysroot /Developer/SDKs/MacOSX10.4u.sdk $CXXFLAGS"

Question: should we update the /Developer/SDKs/MacOSX10.4u.sdk pathname?

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

> #ifdef XP_MACOSX
> #include <AvailabilityMacros.h>
> #endif
> 
>+#include <TargetConditionals.h>

This looks strange because <AvailabilityMacros.h> and <TargetConditionals.h>
are the same kind of headers.  Let's either remove #ifdef XP_MACOSX, or
change it to #ifdef __APPLE__.

Perhaps we should stop pretending there are non-Mac OS X variants of Darwin
and remove the use of XP_MACOSX.

In nsprpub/pr/src/md/unix/uxproces.c:

> #if defined(DARWIN)
>+#ifdef HAVE_CRT_EXTERNS_H
> #include <crt_externs.h>
>+#endif
> #else
> PR_IMPORT_DATA(char **) environ;
> #endif

This doesn't look nice.  Both should use #ifdef or
#if defined.

>+        if (NULL != childEnvp) {
              ...

The code that you moved inside the if (NULL != childEnvp)
block adds the environment variable NSPR_INHERIT_FDS.
Without that environment variable in the environment,
the feature (NSPR file descriptor inheritance) won't
work.  So I suggest we simply fail for iOS:

    if (NULL == childEnvp) {
#ifdef DARWIN
#ifdef HAVE_CRT_EXTERNS_H
            childEnvp = *(_NSGetEnviron());
#else
            PR_DELETE(process);
            PR_SetError(PR_NOT_IMPLEMENTED_ERROR, 0);
            return NULL;
#endif
#else
            childEnvp = environ;
#endif

You may be able to reformat this to avoid the nested ifdefs.

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to Wan-Teh Chang from comment #9)
 \> Please omit nsprpub/configure in patches.

Sorry, I don't use CVS much nowadays, so I forgot about that.

> Question: should we update the /Developer/SDKs/MacOSX10.4u.sdk pathname?

This condition is only for cross-compiling an i386 build from a PPC machine. I'm not sure you can get an XCode that doesn't have the 10.4u SDK on a PPC machine, so it's probably not worth worrying about.
 
> Perhaps we should stop pretending there are non-Mac OS X variants of Darwin
> and remove the use of XP_MACOSX.

That seems reasonable.
 
> The code that you moved inside the if (NULL != childEnvp)
> block adds the environment variable NSPR_INHERIT_FDS.
> Without that environment variable in the environment,
> the feature (NSPR file descriptor inheritance) won't
> work.  So I suggest we simply fail for iOS:

This would mean that PR_CreateProcess would fail if you tried to pass envp=NULL. That's sort of unfortunate, but I suppose it's a tradeoff.

Comment 11

[(not currently active) Ted Mielczarek]

Oh, I misread the code. We'd only fail if you pass envp=NULL *and* the fdInheritBuffer attribute is set. That seems okay.

Comment 12

[(not currently active) Ted Mielczarek]

Attachment: fix NSPR compilation for arm-darwin

This should address all of your comments.

Comment 13

[Wan-Teh Chang]

Comment on attachment 553741 [details] [diff] [review]
fix NSPR compilation for arm-darwin

r=wtc.  Thanks!

Comment 14

[Wan-Teh Chang]

Ted: is __APPLE__ defined on a non-Mac OS X variant of Darwin?
Just curious.

Comment 15

[(not currently active) Ted Mielczarek]

I don't actually know how to answer that question. I believe __APPLE__ will always be defined when using an Apple toolchain, but I'm not sure what non-OS X Darwin uses.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 553741 [details] [diff] [review]
fix NSPR compilation for arm-darwin

Checked in:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.311; previous revision: 1.310
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.313; previous revision: 1.312
done
Checking in pr/include/md/_darwin.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_darwin.h,v  <--  _darwin.h
new revision: 3.23; previous revision: 3.22
done
Checking in pr/src/md/unix/uxproces.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/uxproces.c,v  <--  uxproces.c
new revision: 3.25; previous revision: 3.24
done

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 550746 [details] [diff] [review]
allow forcing cross-compile with CROSS_COMPILE=1, to make targeting the simulator a cross-compile

Checked in:
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.312; previous revision: 1.311
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.314; previous revision: 1.313
done

Comment 18

[Wan-Teh Chang]

Attachment: Use atomic instructions

Ted, could you review and test this code?  I think it
should work, but I don't know how to build and test
this.  Thanks.

Comment 19

[(not currently active) Ted Mielczarek]

Is there any particular test I should run with this patch?

Compiling for iOS is not terribly difficult, you just need a version of XCode installed with the iOS SDK, and then you have to manually pass the compiler path. I configure like so:

CC="/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin/gcc-4.2 -arch armv7 -miphoneos-version-min=4.0 -isysroot /Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS4.0.sdk" ~/build/mozilla/nsprpub/configure --target=arm-apple-darwin --with-macos-sdk=/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS4.0.sdk

Testing the resulting binaries is more difficult. You could link the static NSPR library into an XCode project if you have an iOS developer license, or you can run the tests from pr/tests directly on a jailbroken device, which is how I've been testing.

Comment 20

[(not currently active) Ted Mielczarek]

Ah, right, pr/tests/atomic.c. Your patch compiles fine, and that test program passes on a device.

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 554224 [details] [diff] [review]
Use atomic instructions

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

This looks good. Thanks!

Comment 22

[Wan-Teh Chang]

Comment on attachment 554224 [details] [diff] [review]
Use atomic instructions

Ted: thanks a lot for reviewing and testing my patch.

Patch checked in on the NSPR trunk (NSPR 4.9).

Checking in pr/include/md/_darwin.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_darwin.h,v  <--  _darwin.h
new revision: 3.24; previous revision: 3.23
done
Checking in pr/src/md/unix/uxproces.c;
/cvsroot/mozilla/nsprpub/pr/src/md/unix/uxproces.c,v  <--  uxproces.c
new revision: 3.26; previous revision: 3.25
done

Comment 23

[Wan-Teh Chang]

Attachment: Change one more XP_MACOSX to __APPLE__ in _darwin.h

I'm sorry I forgot to do one more cleanup in the
previous patch.

The use of the XP_MACOSX in _darwin.h was introduced in rev. 3.18:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/nsprpub/pr/include/md&command=DIFF_FRAMESET&file=_darwin.h&rev2=3.18&rev1=3.17

XP_MACOSX was used in two places in _darwin.h.  We should change
both of them to __APPLE__ for consistency.

Comment 24

[Wan-Teh Chang]

Comment on attachment 554255 [details] [diff] [review]
Change one more XP_MACOSX to __APPLE__ in _darwin.h

Patch checked in on the NSPR trunk (NSPR 4.9).

Checking in _darwin.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_darwin.h,v  <--  _darwin.h
new revision: 3.25; previous revision: 3.24
done