Bug 153422 – [B3] REGRESSION(r195395): testComplex(64, 128) asserts on Linux with GCC

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Csaba Osztrogonác]

testComplex(64, 128) asserts at least on X86_64 Linux.
I don't know if it is a regression or not and if it fails on Mac or not.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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. )

[Csaba Osztrogonác]

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

[Filip Pizlo]

Seems bad, I'll look today.

[Filip Pizlo]

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.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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.

[Filip Pizlo]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 270152 [details]
Patch

[Filip Pizlo]

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.

[Yusuke Suzuki]

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!

[Yusuke Suzuki]

Created attachment 270155 [details]
Patch

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Thanks for reviewing!

[Yusuke Suzuki]

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