931151 - Offer an alternative to NSPR when building the shell on POSIX platforms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: nspr-emulation

This patch is intended to address the following situation:

1. It's not a good idea to build shells with --disable-threadsafe since it's not a very good approximation of actual Firefox behavior.

2. Building with --enable-threadsafe is pretty painful since it requires NSPR. I've never been able to build a 32-bit version of NSPR, and we very often need 32-bit shells for debugging fuzzbugs. I'm guessing that building for ARM is also difficult.

3. On most platforms that developers actually use, NSPR is (or should be) a very minimal wrapper over pthreads.

This patch provides a replacement for NSPR on POSIX platforms. You can enable this code by compiling with --enable-posix-nspr-emulation. If you do this, and you're running on a POSIX platform, you won't need NSPR anymore.

My expectation is that JS engine developers would use this feature when building the shell. I don't think you would ever want to build a browser this way.

I've only tested with gcc and Linux so far, but I don't think there should be problems getting it running on MacOS and clang.

Comment 1

[Brian Hackett [Laid off!]]

I've been able to build/install a 32 bit nspr using the instructions at https://developer.mozilla.org/en-US/docs/NSPR_build_instructions.  The only problem I've had here when working with shells is that if I build/install a 64 bit nspr as well the two will overwrite each other.

Comment 2

[Luke Wagner [:luke]]

Thanks Bill!  In the best case, I was thinking, if you built with --enable-threadsafe (or, now that it's the default, failed to disable it) and you *didn't* specify --with-system-nspr or --with-nspr-* AND you were on a Unix, that --enable-posix-nspr-emulation would be on by default.  That way, './configure' on Unices would just work.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 822483 [details] [diff] [review]
nspr-emulation

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

Bill++; I wish I had thought of this. r=me, fwiw.

::: js/src/vm/PosixNSPR.cpp
@@ +104,5 @@
> +
> +void
> +PR_SetCurrentThreadName(const char *name)
> +{
> +    /* Ignored. */

I looked this up before and it would be handy information to have in gdb now that we have umpteen threads:

    pthread_setname_np(
#ifdef XP_UNIX
                       gSelfThread,
#endif
                       name);

Comment 4

[Nicholas Nethercote [inactive]]

awesome

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 822483 [details] [diff] [review]
nspr-emulation

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

Good grief, why are we still carrying NSPR around?

Thanks again. r=me with some stupid comments.

::: js/src/configure.in
@@ +4098,5 @@
> +    JS_POSIX_NSPR=1,
> +    JS_POSIX_NSPR= )
> +if test -n "$JS_POSIX_NSPR"; then
> +    AC_DEFINE(JS_POSIX_NSPR)
> +fi

Please make it so you can't configure with both --enable-posix-nspr-emulation and any of the other nspr-related configure flags.

::: js/src/vm/PosixNSPR.cpp
@@ +180,5 @@
> +    if (!lock)
> +        return nullptr;
> +
> +    if (pthread_mutex_init(&lock->mutex(), nullptr))
> +        return nullptr;

`js_delete(lock);` in the error path.

@@ +223,5 @@
> +    if (!cvar)
> +        return nullptr;
> +
> +    if (pthread_cond_init(&cvar->cond(), nullptr))
> +        return nullptr;

Same here.

@@ +254,5 @@
> +
> +uint32_t
> +PR_MillisecondsToInterval(uint32_t milli)
> +{
> +    MOZ_ASSUME_UNREACHABLE("PR_MillisecondsToInterval unimplemented");

return milli;

@@ +264,5 @@
> +
> +uint32_t
> +PR_TicksPerSecond()
> +{
> +    return 1000;

return TicksPerSecond;

::: js/src/vm/PosixNSPR.h
@@ +70,5 @@
> +PRThread *
> +PR_GetCurrentThread();
> +
> +void
> +PR_SetCurrentThreadName(const char *name);

The NSPR function returns PRStatus.

I can't imagine it would ever matter, but might as well match it.

@@ +109,5 @@
> +void
> +PR_Lock(PRLock *lock);
> +
> +void
> +PR_Unlock(PRLock *lock);

Also returns PRStatus.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Build peer review is probably appropriate here.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: nspr-emulation v2

This contains the main changes. Carrying over Jason's r+.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: nspr-config-changes

This patch does as Luke suggested in comment 2. If you don't specify any NSPR options, and you're on Linux, it enables --enable-posix-nspr-emulation by default.

Hopefully someone can test this on Mac soon and we can add that to the list of platforms where it's on by default.

Comment 9

[Luke Wagner [:luke]]

\o/

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 824231 [details] [diff] [review]
nspr-emulation v2

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

::: js/src/vm/PosixNSPR.cpp
@@ +71,5 @@
> +    if (pthread_create(&t->pthread(), &attr, &nspr::Thread::ThreadRoutine, t))
> +        return nullptr;
> +
> +    if (state == PR_UNJOINABLE_THREAD) {
> +        if (pthread_detach(t->pthread()))

Do you need to js_delete(t) if pthread_create() or pthread_detach() fail?

@@ +75,5 @@
> +        if (pthread_detach(t->pthread()))
> +            return nullptr;
> +    }
> +
> +    pthread_attr_destroy(&attr);

pthread_attr_destroy() is typically an empty implementation, but you still might want to call pthread_attr_destroy() on PR_CreateThread()'s error return paths.

@@ +117,5 @@
> +}
> +
> +static const size_t MaxTLSKeyCount = 32;
> +size_t gTLSKeyCount;
> +pthread_key_t gTLSKeys[MaxTLSKeyCount];

Should gTLSKeyCount and gTLSKeys be static? Otherwise they will be initialized to garbage values instead of zeros. gTLSKeyCount is used below without other initialization.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> Do you need to js_delete(t) if pthread_create() or pthread_detach() fail?

I was pretty lazy with all the failure cases in this patch, but I can fix.

> Should gTLSKeyCount and gTLSKeys be static? Otherwise they will be initialized to garbage
> values instead of zeros. gTLSKeyCount is used below without other initialization.

You're right that they should be static. However, all globals are initialized in C++ regardless of whether they're static.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 824235 [details] [diff] [review]
nspr-config-changes

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

Is there an existing check that js-threadsafe has a NSPR present? (I didn't see one.)

::: js/src/configure.in
@@ +2929,5 @@
>      NSPR_LIBS=$withval)
>  AC_SUBST(NSPR_CFLAGS)
>  AC_SUBST(NSPR_LIBS)
>  
> +if test "$_USE_SYSTEM_NSPR" || (test "$NSPR_CFLAGS" -o "$NSPR_LIBS"); then

There's probably a better way to express this, but I don't remember shell syntax well enough.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81175b9cddcf

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/81175b9cddcf

Comment 15

[Till Schneidereit [:till]]

I updated the build documentation[1] and added a short note to the 31 release notes[2].

[1]: https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation
[2]: https://developer.mozilla.org/en-US/docs/SpiderMonkey/31