1128452 - Assertion failure: !mEntered, at dist/include/mozilla/ReentrancyGuard.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

./js-dbg-opt-32-dm-nsprBuild-armSim-linux-940118b1adcd --fuzzing-safe --arm-sim-icache-checks --ion-eager 17.js

assert js debug 32-bit ARM-simulator shell on m-c rev 940118b1adcd at Assertion failure: !mEntered, at dist/include/mozilla/ReentrancyGuard.h

(17.js is an empty file)

Debug configuration options:

CXX="g++ -m32 -msse2 -mfpmath=sse" CC="gcc -m32 -msse2 -mfpmath=sse" AR=ar PKG_CONFIG_LIBDIR=/usr/lib/pkgconfig sh /home/gkwong/trees/mozilla-central/js/src/configure --target=i686-pc-linux --enable-arm-simulator --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

===

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/b0e9b9113cb0
user:        Lars T Hansen
date:        Fri Jan 30 09:15:16 2015 +0100
summary:     Bug 1125837 - smush SimulatorRuntime into Simulator.  r=jandem

Lars, is bug 1125837 a likely regressor? This blocks fuzzing 32-bit ARM simulator builds.

Comment 1

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

Attachment: stack

(gdb) bt 5
#0  0x08599d6d in ReentrancyGuard<js::detail::HashTable<js::HashMapEntry<void*, js::jit::CachePage*>, js::HashMap<void*, js::jit::CachePage*, js::jit::Simulator::ICacheHasher, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy> > (_notifier=..., aObj=..., this=<optimized out>) at ../../dist/include/mozilla/ReentrancyGuard.h:39
#1  add<js::HashMapEntry<void*, js::jit::CachePage*> > (u=<optimized out>, p=..., this=<optimized out>) at ../../dist/include/js/HashTable.h:1569
#2  add<void*&, js::jit::CachePage*&> (k=<optimized out>, v=<optimized out>, p=..., this=<optimized out>) at ../../dist/include/js/HashTable.h:140
#3  js::jit::GetCachePage (i_cache=..., page=0xf4e0d000) at /home/gkwong/trees/mozilla-central/js/src/jit/arm/Simulator-arm.cpp:934
#4  0x08599eb2 in js::jit::FlushOnePage (i_cache=..., start=-186592412, size=4) at /home/gkwong/trees/mozilla-central/js/src/jit/arm/Simulator-arm.cpp:950
(More stack frames follow...)
(gdb)

Comment 2

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

This can be intermittent, but is fairly reliable. It seems to go away with either --no-threads or --ion-offthread-compile=off.

Comment 3

[Lars T Hansen [:lth]]

This is almost certainly a missing lock in access to the icache hash table.  I removed a lock (AutoLockSimulatorRuntime, or something like that) when I refactored the simulator on the assumption that it was guarding against concurrent access to the runtyme by multiple executor threads, a case that disappeared with PJS.  But it appears it was also guarding against concurrent access by the off-thread compiler.  The lock probably needs to protect more than the icache: it probably needs to protect the redirect list too.  (For the redirect list the race is relatively benign - we'll just have duplicates in the list, I don't think anything worse will happen.  But it should still be fixed.)

Comment 4

[Lars T Hansen [:lth]]

Indeed I can observe in gdb that Redirection::Get is reached from HelperThread::handleIonWorkload on a background thread as well as from Simulator::RedirectNativeFunction on the main thread.  So variables accessed by the Redirection functionality definitely need to be in a critical section.

Comment 5

[Lars T Hansen [:lth]]

Attachment: Reintroduce the lock in the ARM simulator

A little more elaborate than strictly needed: I renamed some functions to make it clear that they are running within the critical section.

Comment 6

[Lars T Hansen [:lth]]

A similar patch is needed for MIPS but I'll create a separate bug for that.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8557905 [details] [diff] [review]
Reintroduce the lock in the ARM simulator

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

Looks good, sorry for the delay.

Comment 8

[Lars T Hansen [:lth]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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