947683 - Unable to compile non-threadsafe js shells using --disable-threadsafe

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Bug 915735 may have broken compiling js shells and bug 947299 may have partially fixed some issues, but shells compiled with --disable-threadsafe still don't seem to work.

This isn't the first failure involving --disable-threadsafe (see bug 942552), please test with this flag in the future!

Setting needinfo? from Ehsan as he wrote the patch in bug 915735.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Build failure with m-c rev edac8cba9f78

Configure parameters:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --with-ccache --disable-threadsafe

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Additionally, I also seem to be having trouble compiling with --enable-threadsafe on Windows (32-bit and 64-bit), though that might be related to bug 947299. (also on rev edac8cba9f78)

Comment 3

[(no longer active)]

As you could have probably guessed form the build failure, this has nothing to do with bug 915735.

$ git bisect good
3b2fc4c5d744113e6c677dd86a4df7806d02450a is the first bad commit
commit 3b2fc4c5d744113e6c677dd86a4df7806d02450a
Author: Jan de Mooij <[e-mail]>
Date:   Fri Dec 6 21:03:27 2013 +0100

    Bug 946883 - Use NSPR thread for AsmJSMachExceptionHandler on OS X, so that it works with PosixNSPR. r=luke

    --HG--
    extra : rebase_source : 34a82b93197c14ab237df23ceb8646499049cbf8

:040000 040000 c72d0fd18fb42d08fcf80299209f85e18e9bd8c9 42da86449ed59158aca1197433e8a116154443a0 M	js

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Sigh. Sorry for the mixup.

Turns out that bug 946883 landed in the middle of the landing of bug 915735 and bug 947299's changesets (these broke & fixed Mac shells in some configurations), plus the build failure being in the middle rather than the end of the logfile does not help.

Comment 5

[Jan de Mooij [:jandem]]

Luke, do you think it's ok to disable signal handlers (and therefore Odin) on OS X with --disable-threadsafe?

Comment 6

[Luke Wagner [:luke]]

Yeah, just have EnsureIsSignalHandlingBroken() return 'false' in that case and Odin will turn off.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

This fixes the build for me with --disable-threadsafe.

Comment 8

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d5a48271b6

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6d5a48271b6

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This will need to land on Aurora, I think. The regressor made the merge but not this fix.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8344675 [details] [diff] [review]
Patch

This patch is basically NPOTB. I'll request approval just to be sure, but we never build the browser with --disable-threadsafe so it has no effect on it.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 946883.
User impact if declined: Unable to build a --disable-threadsafe shell.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): None.
String or IDL/UUID changes made by this patch: None.

Comment 12

[(no longer active)]

NPOTB changes do not need approval.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8344675 [details] [diff] [review]
Patch

what Ehsan said, use a=NPTOB in your commit, thanks for checking though.

Comment 14

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/423edfffe821

Comment 15

[u279076]

If this needs extra QA testing please remove the [qa-] whiteboard tag and add the verifyme keyword.

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This is working.