Bug 161308 – REGRESSION (r205107): ASSERTION FAILED: !(reinterpret_cast<char*>(this)[i])

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

[Carlos Alberto Lopez Perez]

Revision r205107 <http://trac.webkit.org/r205107> has caused lot of assertions on the Debug build of GTK+: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/10835

Backtrace:

ASSERTION FAILED: !(reinterpret_cast<char*>(this)[i])
../../Source/JavaScriptCore/dfg/DFGAbstractValue.h(66) : JSC::DFG::AbstractValue::AbstractValue()
1   0x7f5e53a30411 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x1e) [0x7f5e53a30411]
2   0x7f5e530e5262 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::AbstractValue::AbstractValue()+0x96) [0x7f5e530e5262]
3   0x7f5e53115412 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::AbstractValue::fullTop()+0x19) [0x7f5e53115412]
4   0x7f5e53114b74 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::BasicBlock::BasicBlock(unsigned int, unsigned int, unsigned int, float)+0x12e) [0x7f5e53114b74]
5   0x7f5e5312f9e0 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::ByteCodeParser::parseCodeBlock()+0x60e) [0x7f5e5312f9e0]
6   0x7f5e5312ffb6 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::ByteCodeParser::parse()+0x1c8) [0x7f5e5312ffb6]
7   0x7f5e531302aa /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::parse(JSC::DFG::Graph&)+0x3b) [0x7f5e531302aa]
8   0x7f5e533155c4 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)+0xe2) [0x7f5e533155c4]
9   0x7f5e53314fcf /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)+0x173) [0x7f5e53314fcf]
10  0x7f5e53423754 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::Worklist::runThread(JSC::DFG::ThreadData*)+0x316) [0x7f5e53423754]
11  0x7f5e53423a7a /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(JSC::DFG::Worklist::threadFunction(void*)+0x2a) [0x7f5e53423a7a]
12  0x7f5e53a4d6be /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x228c6be) [0x7f5e53a4d6be]
13  0x7f5e53a4d874 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x228c874) [0x7f5e53a4d874]
14  0x7f5e5a04c7ce /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(std::function<void ()>::operator()() const+0x32) [0x7f5e5a04c7ce]
15  0x7f5e53a4d5a0 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x228c5a0) [0x7f5e53a4d5a0]
16  0x7f5e53a887e1 /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x22c77e1) [0x7f5e53a887e1]
17  0x7f5e4f88f0a4 /lib/x86_64-linux-gnu/libpthread.so.0(+0x80a4) [0x7f5e4f88f0a4]
18  0x7f5e4afc587d /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d) [0x7f5e4afc587d]

[Carlos Garcia Campos]

All layout tests are crashing for the same assert in the Debug bot.

[Carlos Garcia Campos]

I don't think this is GTK specific, Carlos confirmed he could reproduce the assert with jsconly but only when building with GCC, not with clang.

[Saam Barati]

(In reply to comment #2)
> I don't think this is GTK specific, Carlos confirmed he could reproduce the
> assert with jsconly but only when building with GCC, not with clang.

Interesting. Any idea on why this may be?

[Saam Barati]

I wonder if GCC is emitting a 32-bit store instead of a 64-bit store?

[Saam Barati]

Maybe the bug is not changing the "1u" to "1ull" inside SpeculatedType.h?

[Carlos Alberto Lopez Perez]

(In reply to comment #5)
> Maybe the bug is not changing the "1u" to "1ull" inside SpeculatedType.h?

I just tested to replace all occurrences of "1u" with "1ull" in SpeculatedType.h, and did a debug build of JSCOnly with GCC 4.9.

It is still crashing with the same assert.

[Saam Barati]

Ok.
Third guess, there is now 4 bytes of padding after the ArrayModes m_arrayModes
field, maybe those aren't getting zeroed out? And for some reason, clang zeroes
them out.

[Zan Dobersek]

(In reply to comment #7)
> Ok.
> Third guess, there is now 4 bytes of padding after the ArrayModes
> m_arrayModes
> field, maybe those aren't getting zeroed out? And for some reason, clang
> zeroes
> them out.

That's the reason.

[Carlos Garcia Campos]

// The WTF Traits for AbstractValue allow the initialization of values with bzero().
// We verify the correctness of this assumption here.

So, that's assuming that AbstractValue is always created as a Vector or Hash value and then initialized by them with memset. But AbstractValue::heapTop(), AbstractValue::bytecodeTop() and AbstractValue::fullTop() are creating a stack allocated AbstractValue that is not zero initialized.

[Carlos Alberto Lopez Perez]

(In reply to comment #7)
> Ok.
> Third guess, there is now 4 bytes of padding after the ArrayModes
> m_arrayModes
> field, maybe those aren't getting zeroed out? And for some reason, clang
> zeroes
> them out.

It seems that the standard only guarantees that the padding in structures is zero-initialized on some very specific cases [1]. In the rest of cases it is undefined behaviour. And this seems to be one of this cases. That explains why clang is zero-initializing the padding of the structure, but GCC does not.

This fixes the issue with GCC:

--- a/Source/JavaScriptCore/dfg/DFGAbstractValue.h
+++ b/Source/JavaScriptCore/dfg/DFGAbstractValue.h
@@ -191,21 +191,21 @@ struct AbstractValue {
     
     static AbstractValue heapTop()
     {
-        AbstractValue result;
+        static AbstractValue result;
         result.makeHeapTop();
         return result;
     }
     
     static AbstractValue bytecodeTop()
     {
-        AbstractValue result;
+        static AbstractValue result;
         result.makeBytecodeTop();
         return result;
     }
     
     static AbstractValue fullTop()
     {
-        AbstractValue result;
+        static AbstractValue result;
         result.makeFullTop();
         return result;
     }



Now I wonder if this is the best approach (declaring it as static), or it will be better to directly bzero the structure on the constructor, or perhaps it will be better to modify the assert to check only the values of the members of the structure.


[1] https://gustedt.wordpress.com/2012/10/24/c11-defects-initialization-of-padding/

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #7)
> > Ok.
> > Third guess, there is now 4 bytes of padding after the ArrayModes
> > m_arrayModes
> > field, maybe those aren't getting zeroed out? And for some reason, clang
> > zeroes
> > them out.
> 
> It seems that the standard only guarantees that the padding in structures is
> zero-initialized on some very specific cases [1]. In the rest of cases it is
> undefined behaviour. And this seems to be one of this cases. That explains
> why clang is zero-initializing the padding of the structure, but GCC does
> not.

Stack allocated objects are never zero-initialized by the compiler, I think, their members are initialized on construction, so it's expected that the padding is not initialized. AbstractValue adds vector and hash traits to ensure that they are initialized with memset when allocated by a vector or hash objects. In such cases the passing is always 0 because memset fills from the base pointer to the sizeof().

> This fixes the issue with GCC:
> 
> --- a/Source/JavaScriptCore/dfg/DFGAbstractValue.h
> +++ b/Source/JavaScriptCore/dfg/DFGAbstractValue.h
> @@ -191,21 +191,21 @@ struct AbstractValue {
>      
>      static AbstractValue heapTop()
>      {
> -        AbstractValue result;
> +        static AbstractValue result;
>          result.makeHeapTop();
>          return result;
>      }
>      
>      static AbstractValue bytecodeTop()
>      {
> -        AbstractValue result;
> +        static AbstractValue result;
>          result.makeBytecodeTop();
>          return result;
>      }
>      
>      static AbstractValue fullTop()
>      {
> -        AbstractValue result;
> +        static AbstractValue result;
>          result.makeFullTop();
>          return result;
>      }
> 
> 
> 
> Now I wonder if this is the best approach (declaring it as static), or it
> will be better to directly bzero the structure on the constructor, or
> perhaps it will be better to modify the assert to check only the values of
> the members of the structure.

I guess we really want a new AbstractValue object every time those methods are called. I think the check is what is wrong, not the callers.

Could we remove that check while we find a better solution? The GTK+ Debug bot has exited early for two days

> 
> [1]
> https://gustedt.wordpress.com/2012/10/24/c11-defects-initialization-of-
> padding/

[Yusuke Suzuki]

Fixed in https://trac.webkit.org/changeset/205254.