Bug 153422

Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffae1ff700 (LWP 13525)] 0x00007ffff6efbb99 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321 321 *(int *)(uintptr_t)0xbbadbeef = 0; (gdb) bt #0 0x00007ffff6efbb99 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321 #1 0x00000000006316a8 in WTF::CrashOnOverflow::crash () at ../../Source/WTF/wtf/CheckedArithmetic.h:85 #2 0x000000000063169f in WTF::CrashOnOverflow::overflowed () at ../../Source/WTF/wtf/CheckedArithmetic.h:78 #3 0x00007ffff645266d in WTF::Vector<std::unique_ptr<JSC::B3::Value, std::default_delete<JSC::B3::Value> >, 0ul, WTF::CrashOnOverflow, 16ul>::at (this=0x7fffaeb908a0, i=164) at ../../Source/WTF/wtf/Vector.h:665 #4 0x00007ffff6451451 in WTF::Vector<std::unique_ptr<JSC::B3::Value, std::default_delete<JSC::B3::Value> >, 0ul, WTF::CrashOnOverflow, 16ul>::operator[] (this=0x7fffaeb908a0, i=164) at ../../Source/WTF/wtf/Vector.h:680 #5 0x00007ffff645100e in JSC::B3::Procedure::ValuesCollection::at (this=0x7fffae1fe490, index=164) at ../../Source/JavaScriptCore/b3/B3Procedure.h:222 #6 0x00007ffff64516b1 in JSC::B3::IndexSet<JSC::B3::Value>::Iterable<JSC::B3::Procedure::ValuesCollection>::iterator::operator* ( this=0x7fffae1fe3c0) at ../../Source/JavaScriptCore/b3/B3IndexSet.h:96 #7 0x00007ffff644eb07 in JSC::B3::demoteValues (proc=..., values=...) at ../../Source/JavaScriptCore/b3/B3FixSSA.cpp:59 #8 0x00007ffff6438993 in JSC::B3::(anonymous namespace)::DuplicateTails::run (this=0x7fffae1fe770) at ../../Source/JavaScriptCore/b3/B3DuplicateTails.cpp:87 #9 0x00007ffff6438db7 in JSC::B3::duplicateTails (proc=...) at ../../Source/JavaScriptCore/b3/B3DuplicateTails.cpp:142 #10 0x00007ffff646221a in JSC::B3::generateToAir (procedure=..., optLevel=1) at ../../Source/JavaScriptCore/b3/B3Generate.cpp:84 #11 0x00007ffff64620c6 in JSC::B3::prepareForGeneration (procedure=..., optLevel=1) at ../../Source/JavaScriptCore/b3/B3Generate.cpp:56 #12 0x00007ffff643053d in JSC::B3::Compilation::Compilation (this=0x7fffaeb99b40, vm=..., proc=..., optLevel=1) at ../../Source/JavaScriptCore/b3/B3Compilation.cpp:45 #13 0x0000000000636e9a in std::make_unique<JSC::B3::Compilation, JSC::VM&, JSC::B3::Procedure&, unsigned int&> () at ../../Source/WTF/wtf/StdLibExtras.h:311 #14 0x0000000000454162 in (anonymous namespace)::compile (procedure=..., optLevel=1) at ../../Source/JavaScriptCore/b3/testb3.cpp:89 #15 0x000000000048b44c in (anonymous namespace)::testComplex (numVars=64, numConstructs=128) at ../../Source/JavaScriptCore/b3/testb3.cpp:5945 #16 0x00000000004bca19 in (anonymous namespace)::<lambda()>::operator()(void) const (__closure=0x7fffaef81d4c) at ../../Source/JavaScriptCore/b3/testb3.cpp:10337 #17 0x000000000062cb56 in WTF::SharedTaskFunctor<void(), (anonymous namespace)::run(char const*)::<lambda()> >::run(void) ( this=0x7fffaef81d40) at ../../Source/WTF/wtf/SharedTask.h:90 #18 0x00000000004c475e in (anonymous namespace)::<lambda()>::operator()(void) const (__closure=0x7fffae1fee90) at ../../Source/JavaScriptCore/b3/testb3.cpp:10939 #19 0x00000000005b7190 in std::_Function_handler<void(), (anonymous namespace)::run(char const*)::<lambda()> >::_M_invoke(const std::_Any_data &) (__functor=...) at /usr/include/c++/5/functional:1871 #20 0x00007ffff69480a4 in std::function<void ()>::operator()() const (this=0x7fffae1fee90) at /usr/include/c++/5/functional:2271 #21 0x00007ffff6f18b43 in WTF::threadEntryPoint (contextData=0x7fffae7d0050) at ../../Source/WTF/wtf/Threading.cpp:58 #22 0x00007ffff6f5016a in WTF::wtfThreadEntryPoint (param=0x7fffae3c6db0) at ../../Source/WTF/wtf/ThreadingPthreads.cpp:164 #23 0x00007ffff347e6aa in start_thread (arg=0x7fffae1ff700) at pthread_create.c:333 #24 0x00007ffff43d0eed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109

I bisected this bug, it is a regression caused by https://trac.webkit.org/changeset/195395 . - on r195394 all testb3 tests passed (debug and release too) - on r195395 testComplex(64, 128) asserts in debug mode and testDiamond() crashes in release mode. ( I disabled parallel test running. )

Could you possibly check if this regression is valid on Mac too?

Seems bad, I'll look today.

I investigated this on r195553. I cannot reproduce any version of this crash on Mac. On trunk on Mac, I see: - testb3 passes in debug without asserting or crashing, both in parallel and sequential mode, and both with JSC_validateGraphAtEachPhase=true and =false (note that we usually run these tests with that flag set to true). - testb3 passes in release without asserting or crashing, both in parallel and sequential mode, both with and without validateGraphBlahBlah. Assigning back to Ossy sine this is probably a platform-specific issue.

(In reply to comment #5) > I investigated this on r195553. I cannot reproduce any version of this > crash on Mac. On trunk on Mac, I see: > > - testb3 passes in debug without asserting or crashing, both in parallel and > sequential mode, and both with JSC_validateGraphAtEachPhase=true and =false > (note that we usually run these tests with that flag set to true). > > - testb3 passes in release without asserting or crashing, both in parallel > and sequential mode, both with and without validateGraphBlahBlah. Thanks for checking it. It seems it is a Linux specific issue. Unfortunately there are zillion JSC stress tests crashes too. > Assigning back to Ossy sine this is probably a platform-specific issue. Set to unassigned, probably I won't have time to investigate it in the near future.

Additionally it caused 3624 JSC test crashes (from 30762 tests)

I found an interesting thing here. This bug isn't valid if I build JSC with Clang (3.6). But crash happens with GCC 4.9 and 5.2 too. It might be a compiler bug or maybe the new code introduced in r195395 relies some non standard Clang behaviour. By the way, it's not "good" that we try to get the 164th element from a null-sized vector. The question is why is it null-sized.

(In reply to comment #8) > I found an interesting thing here. This bug isn't valid if I build > JSC with Clang (3.6). But crash happens with GCC 4.9 and 5.2 too. > > It might be a compiler bug or maybe the new code introduced > in r195395 relies some non standard Clang behaviour. > > By the way, it's not "good" that we try to get the 164th element > from a null-sized vector. The question is why is it null-sized. Fascinating. We'll need to get a bot to run testb3 and testair so that we can catch these things. It would be great to have such a bot on Linux, compiling with GCC. I'm really curious what causes this bug.

(In reply to comment #9) > (In reply to comment #8) > > I found an interesting thing here. This bug isn't valid if I build > > JSC with Clang (3.6). But crash happens with GCC 4.9 and 5.2 too. > > > > It might be a compiler bug or maybe the new code introduced > > in r195395 relies some non standard Clang behaviour. > > > > By the way, it's not "good" that we try to get the 164th element > > from a null-sized vector. The question is why is it null-sized. > > Fascinating. We'll need to get a bot to run testb3 and testair so that we > can catch these things. It would be great to have such a bot on Linux, > compiling with GCC. > > I'm really curious what causes this bug. I've found the issue. Seeing B3FixSSA.cpp's `for (Value* value : values.values(proc.values())) {` proc.values() returns ValuesCollection (Not reference!). But values.values takes const ValueCollection&. And later it produces IndexSet<Value>::Iterable<Procedure::ValuesCollection>, it holds const ValueCollection& as its member. But IndexSet<Value>::Iterable<Procedure::ValuesCollection> is just an instance. So after creating this, the lifetime of the ValueCollection const reference finished! Easy example, class A { A(const std::string& value) : m_string(value) { } const std::string& m_string; }; A a(std::string("Value")); // Here, the lifetime of the const reference to the std::string is ended. So I think the behavior of the GCC is correct.

Created attachment 270152 [details] Patch

Comment on attachment 270152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270152&action=review > Source/JavaScriptCore/b3/B3IndexSet.h:137 > + // Procedure::ValuesCollection valuesCollection = procedure.values(); > + // indexSet.values(valuesCollection); > + // > + // Don't call it like indexSet.values(proceduce.values()). collection will be touched by const reference. > + // Since procedure.values() creates ValuesCollection (not reference), this lifetime will be finished immediately > + // after Iterable<ValuesCollection> is constructed. I really don't like this idiom. Can we change it so that procedure.values() returns a long-lived value? For example it can have a ValuesCollection instance sitting around inside Procedure.

Comment on attachment 270152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270152&action=review >> Source/JavaScriptCore/b3/B3IndexSet.h:137 >> + // after Iterable<ValuesCollection> is constructed. > > I really don't like this idiom. Can we change it so that procedure.values() returns a long-lived value? For example it can have a ValuesCollection instance sitting around inside Procedure. OK, I'll change it soon :) Thanks!

Created attachment 270155 [details] Patch

After this fix, testb3 and testair pass on GTK environment.

Thanks for reviewing!

Committed r195800: <http://trac.webkit.org/changeset/195800>