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

RESOLVED FIXED in mozilla28

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks: 1 bug)

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 822483 [details] [diff] [review]
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.
Attachment #822483 - Flags: review?(jorendorff)
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

4 years ago
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.
See Also: → bug 927915
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);
awesome
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.
Attachment #822483 - Flags: review?(jorendorff) → review+
Build peer review is probably appropriate here.
(Assignee)

Comment 7

4 years ago
Created attachment 824231 [details] [diff] [review]
nspr-emulation v2

This contains the main changes. Carrying over Jason's r+.
Attachment #822483 - Attachment is obsolete: true
Attachment #824231 - Flags: review+
(Assignee)

Comment 8

4 years ago
Created attachment 824235 [details] [diff] [review]
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.
Attachment #824235 - Flags: review?(ted)

Comment 9

4 years ago
\o/
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.
(Assignee)

Comment 11

4 years ago
> 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 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.
Attachment #824235 - Flags: review?(ted) → review+
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/81175b9cddcf
https://hg.mozilla.org/mozilla-central/rev/81175b9cddcf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Assignee)

Updated

4 years ago
Blocks: 720018

Updated

4 years ago
Depends on: 942552
Blocks: 946841
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

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.