Bug 146440 – Crash on xLarge memory allocation using bmalloc on 32bit systems

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mario Sanchez Prada]

As mentioned in the WebKitGTK+ mailing list[1], I've been seeing the following crash consistently after upgrading to 2.8.3, in a 32bit Linux system:

(gdb) bt
#0  allocateXLarge () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Heap.cpp:287
#1  0xb4fc94f5 in allocateXLarge () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Heap.cpp:293
#2  0xb4fc6ac4 in allocateXLarge () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Allocator.cpp:227
#3  0xb4fc6b2e in allocateSlowCase () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Allocator.cpp:245
#4  0xb4f95de2 in allocate () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Allocator.h:86
#5  allocate () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/Cache.h:79
#6  malloc () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/bmalloc/bmalloc/bmalloc.h:43
#7  fastMalloc () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/FastMalloc.cpp:270
#8  0xb5592815 in allocateBuffer () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:269
#9  reserveCapacity () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:1090
#10 expandCapacity () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:951
#11 0xb5f766e1 in expandCapacity () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:958
#12 appendSlowCase<WebCore::FloatRect&> () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:1219
#13 append<WebCore::FloatRect&> () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/Vector.h:1210
#14 createOrDestroyTilesIfNeeded () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:89
#15 0xb5f77001 in updateContents () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/TextureMapperTiledBackingStore.cpp:148
#16 0xb64aefb3 in updateBackingStoreIfNeeded () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:549
#17 0xb64af095 in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:519
#18 0xb64af0ed in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#19 0xb64af0ed in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#20 0xb64af0ed in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#21 0xb64af0ed in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#22 0xb64af0ed in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#23 0xb64af0ed in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#24 0xb64af0ed in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#25 0xb64af0ed in updateBackingStoreIncludingSubLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:526
#26 0xb57a3c06 in flushPendingLayerChanges () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:272
#27 0xb57a43f6 in flushAndRenderLayers () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:313
#28 0xb57a4507 in layerFlushTimerFired () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebKit2/WebProcess/WebPage/gtk/LayerTreeHostGtk.cpp:237
#29 0xb57a48d2 in operator()<, void> () at /usr/include/c++/4.9/functional:569
#30 __call<void, 0u> () at /usr/include/c++/4.9/functional:1264
#31 operator()<, void> () at /usr/include/c++/4.9/functional:1323
#32 _M_invoke () at /usr/include/c++/4.9/functional:2039
#33 0xb4f43141 in operator() () at /usr/include/c++/4.9/functional:2439
#34 0xb4fc608d in voidCallback () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/gobject/GMainLoopSource.cpp:365
#35 0xb4fc17fd in voidSourceCallback () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/gobject/GMainLoopSource.cpp:456
#36 0xb3b8a3e0 in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
#37 0xb3b8dca3 in g_main_context_dispatch () from /lib/i386-linux-gnu/libglib-2.0.so.0
#38 0xb3b8e0b9 in ?? () from /lib/i386-linux-gnu/libglib-2.0.so.0
#39 0xb3b8e469 in g_main_loop_run () from /lib/i386-linux-gnu/libglib-2.0.so.0
#40 0xb6aeb8d9 in run () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WTF/wtf/gtk/RunLoopGtk.cpp:63
#41 0xb57a22d8 in ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebKit2/Shared/unix/ChildProcessMain.h:61
#42 0xb57a202c in WebProcessMainUnix () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:77
#43 0x080485f2 in main () at /home/mario/webkit2gtk-2.8.3+dfsg1/Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:44

(Sorry for not having a better backtrace. Being a 32bit build makes it quite complicate to build a debug build with more than -g1)

According to https://bugs.webkit.org/show_bug.cgi?id=145385#c6, this very same bug has been observed in at least 105 occasions on Fedora too, always in 32bit systems as well: https://bugzilla.redhat.com/show_bug.cgi?id=1225733

Also, I've tried applying the patch for bug 145385 in case it was happening due to an integer overflow but that did no good either (and did not good either for the Fedora package either[2]), so the issue has to be caused by something else...

In the last week I've been debugging this quite thoroughly, comparing how the webkigtk package was being built in our environment before and after the upgrade to 2.8.3 and found that building with -O0 instead of -O2 seems to make the crash go away, so perhaps this is related to some compiler options? JFTR, we used to build the previous version of webkigtk+ we use (2.6.2) with gcc 4.8 and are now using 4.9, so perhaps some of the changes in GCC 4.9's could be affecting to this, not sure yet though.

[1] https://lists.webkit.org/pipermail/webkit-gtk/2015-June/002381.html
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1225733#c4

[Mario Sanchez Prada]

(In reply to comment #0)
> In the last week I've been debugging this quite thoroughly, comparing how
> the webkigtk package was being built in our environment before and after the
> upgrade to 2.8.3 and found that building with -O0 instead of -O2 seems to
> make the crash go away, so perhaps this is related to some compiler options?

JFTR, I confirmed this "theory" yesterday night and tomorrow morning:

  * With -O2: I get the crash 
  * With -O1: I get the crash
  * With -OO: I do NOT get the crash

So, the problem seems to happen when -O1 is enabled, which in my system translates to some of the following optimizations, enabled for that level:

  -fbranch-count-reg          		[enabled]
  -fcombine-stack-adjustments 		[enabled]
  -fcompare-elim              		[enabled]
  -fcprop-registers           		[enabled]
  -fdefer-pop                 		[enabled]
  -fforward-propagate         		[enabled]
  -fguess-branch-probability  		[enabled]
  -fif-conversion             		[enabled]
  -fif-conversion2            		[enabled]
  -finline-functions-called-once 	[enabled]
  -fipa-profile               		[enabled]
  -fipa-pure-const            		[enabled]
  -fipa-reference             		[enabled]
  -fmerge-constants           		[enabled]
  -fmove-loop-invariants      		[enabled]
  -fshrink-wrap               		[enabled]
  -fsplit-wide-types          		[enabled]
  -ftree-bit-ccp              		[enabled]
  -ftree-ccp                  		[enabled]
  -ftree-ch                   		[enabled]
  -ftree-copy-prop            		[enabled]
  -ftree-copyrename           		[enabled]
  -ftree-dce                  		[enabled]
  -ftree-dominator-opts       		[enabled]
  -ftree-dse                  		[enabled] *
  -ftree-fre                  		[enabled]
  -ftree-pta                  		[enabled]
  -ftree-sink                 		[enabled]
  -ftree-slsr                 		[enabled]
  -ftree-sra                  		[enabled]
  -ftree-ter                  		[enabled]


If anyone can spot anything in there that might ring a bell, please let me know, otherwise I will continue the investigation myself the best I can.

Last, according to the documentation, -O1 also enables -fomit-frame-pointer (no idea why it does not show up there), but I already tried passing -fno-omit-frame-pointer (as well as -fno-tree-dce) and that did not work, so it has to be something else.

[Mario Sanchez Prada]

I found the optimization that was messing up with bmalloc here: -ftree-sra

According to the documentation [1]

  """
  Perform scalar replacement of aggregates. This pass replaces
  structure references with scalars to prevent committing
  structures to memory too early.

  This flag is enabled by default at -O and higher. 
  """

We are normally building with -g1 (for the debug package only) and with -O2 for optimizations, so simply passing -fno-free-sra while building on top would avoid the crash from happen.

Geoffrey, any idea why this could be the case?

Also, should disabling this optimization could make sense as a reasonable workaround for 2.8.3 (similar to what it's done in bug 127777 with -fno-omit-frame-pointer and -fno-tree-dce), would it be ok to propose a patch for the CMake files for WebKitGTK+? (Adding Martin to CC)

[1] https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Optimize-Options.html

[Mario Sanchez Prada]

(In reply to comment #2)
> [...]
> Also, should disabling this optimization could make sense as a reasonable
> workaround for 2.8.3 (similar to what it's done in bug 127777 with
> -fno-omit-frame-pointer and -fno-tree-dce), would it be ok to propose a
> patch for the CMake files for WebKitGTK+? (Adding Martin to CC)

To be more precise, I was thinking perhaps of something like this:

diff --git a/Source/cmake/OptionsCommon.cmake b/Source/cmake/OptionsCommon.cmake
index 6691526..355d475 100644
--- a/Source/cmake/OptionsCommon.cmake
+++ b/Source/cmake/OptionsCommon.cmake
@@ -99,6 +99,12 @@ endif ()
 
 string(TOLOWER ${CMAKE_HOST_SYSTEM_PROCESSOR} LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR)
 if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)")
+    # The -ftree-sra optimization (implicit with -O2) causes crashes when
+    # allocating large chunks of memory using bmalloc on Intel 32bit.
+    # See https://bugs.webkit.org/show_bug.cgi?id=146440
+    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-tree-sra")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-tree-sra")
+
     # To avoid out of memory when building with debug option in 32bit system.
     # See https://bugs.webkit.org/show_bug.cgi?id=77327
     set(CMAKE_SHARED_LINKER_FLAGS_DEBUG "-Wl,--no-keep-memory ${CMAKE_SHARED_LINKER_FLAGS_DEBUG}")

[Carlos Alberto Lopez Perez]

(In reply to comment #3)
> (In reply to comment #2)
> > [...]
> > Also, should disabling this optimization could make sense as a reasonable
> > workaround for 2.8.3 (similar to what it's done in bug 127777 with
> > -fno-omit-frame-pointer and -fno-tree-dce), would it be ok to propose a
> > patch for the CMake files for WebKitGTK+? (Adding Martin to CC)
> 
> To be more precise, I was thinking perhaps of something like this:
> 
> diff --git a/Source/cmake/OptionsCommon.cmake
> b/Source/cmake/OptionsCommon.cmake
> index 6691526..355d475 100644
> --- a/Source/cmake/OptionsCommon.cmake
> +++ b/Source/cmake/OptionsCommon.cmake
> @@ -99,6 +99,12 @@ endif ()
>  
>  string(TOLOWER ${CMAKE_HOST_SYSTEM_PROCESSOR}
> LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR)
>  if ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" AND
> "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)")
> +    # The -ftree-sra optimization (implicit with -O2) causes crashes when
> +    # allocating large chunks of memory using bmalloc on Intel 32bit.
> +    # See https://bugs.webkit.org/show_bug.cgi?id=146440
> +    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-tree-sra")
> +    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-tree-sra")
> +
>      # To avoid out of memory when building with debug option in 32bit
> system.
>      # See https://bugs.webkit.org/show_bug.cgi?id=77327
>      set(CMAKE_SHARED_LINKER_FLAGS_DEBUG "-Wl,--no-keep-memory
> ${CMAKE_SHARED_LINKER_FLAGS_DEBUG}")

What about setting this flag only for the bmalloc source files at Source/bmalloc/CMakeLists.txt . Would it be enough?

[Michael Catanzaro]

I think it'd be best to patch this downstream, unless we start getting additional bug reports; since you're the only one to have needed it so far, I guess it has likely already been fixed in GCC.

Good job finding the exact optimization at fault; I don't want to think about how long you spent compiling. :(

[Mario Sanchez Prada]

(In reply to comment #4)
> [...]
> What about setting this flag only for the bmalloc source files at
> Source/bmalloc/CMakeLists.txt . Would it be enough?

Not sure, but I will try that out just in case, I'd rather keep compiler optimizations enabled whenever/wherever possible. Thanks for the suggestion.

(In reply to comment #5)
> I think it'd be best to patch this downstream, unless we start getting
> additional bug reports; since you're the only one to have needed it so far,
> I guess it has likely already been fixed in GCC.
 
Not sure about this. While I understand this is a workaround and not the proper solution, this issue is not present just in our platform (where we use gcc 4.9) but also in Fedora, as you pointed out before, where you're using gcc 5.1.

The main difference between Fedora and our platform so far seems to be how likely the crash would happen. In our case it was happening always and that's why, even being quite sneaky, I managed to found a solution for it after all. But in other systems this can be very tricky to investigate, so perhaps it would be better to include the patch upstream, so that every single user would benefit of it?

Not 100% sure, would like to hear more opinions on this.

> Good job finding the exact optimization at fault; I don't want to think
> about how long you spent compiling. :(

Thanks. In the end it was not that terrible, the hardest part was before I stopped trying to debug the code and started looking at the compiler's optimizations. Once I figure out that -O1 would "cause" the crash, it was all much more simple :)

[Carlos Alberto Lopez Perez]

Do we know if this also happens with clang?
Do we know if any version of gcc is safe from this bug?

Unfortunately I'm not able to use clang for building WebKit on my i686 test chroot due to https://llvm.org/bugs/show_bug.cgi?id=18311

[Mario Sanchez Prada]

(In reply to comment #7)
> Do we know if this also happens with clang?

I have no idea, we don't use clang in our platform yet.

> Do we know if any version of gcc is safe from this bug?

All I can tell is that I know this is an issue for sure with gcc 4.8 and gcc 4.9, since those are the compilers I could try myself so far.

Also, it was mentioned somewhere (can't remember if IRC or any bugzilla) that the crashes seen in Fedora related to webkit packages built using gcc 5.1, so I assume the problem still happens there.

> Unfortunately I'm not able to use clang for building WebKit on my i686 test
> chroot due to https://llvm.org/bugs/show_bug.cgi?id=18311

I can't use clang here either, and I'm afraid that testing with different versions of GCC might be a bit of a problem too, but I suppose I could try if it's really important.

[Mario Sanchez Prada]

Btw, thanks to a comment in Red Hat bugzilla [1] I now know of an URL that reliably crashes MiniBrowser for me, in case someone else wants to try it out:

http://crucial.tmall.com/category-988709636.htm?utm_source=baidu&utm_medium=ppc&utm_term=6.18&utm_content=general&utm_campaign=s_mx100

For the sake of completeness, I've tested the "fix" proposed here with this URL, and it reliably avoids the crash (which I could reproduce without the "fix").

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1225733#c8

[Mario Sanchez Prada]

(In reply to comment #6)
> (In reply to comment #4)
> > [...]
> > What about setting this flag only for the bmalloc source files at
> > Source/bmalloc/CMakeLists.txt . Would it be enough?
> 
> Not sure, but I will try that out just in case, I'd rather keep compiler
> optimizations enabled whenever/wherever possible. Thanks for the suggestion.

I did this test right now but, unfortunately, that did not seem to be enough to get rid of the crash. That suggests the optimization must be disabled from somewhere else, I think I will try disabling it in some other places...

[Mario Sanchez Prada]

(In reply to comment #10)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > [...]
> > > What about setting this flag only for the bmalloc source files at
> > > Source/bmalloc/CMakeLists.txt . Would it be enough?
> > 
> > Not sure, but I will try that out just in case, I'd rather keep compiler
> > optimizations enabled whenever/wherever possible. Thanks for the suggestion.
> 
> I did this test right now but, unfortunately, that did not seem to be enough
> to get rid of the crash. That suggests the optimization must be disabled
> from somewhere else, I think I will try disabling it in some other places...

Indeed, just disabling it in bmalloc did not work, but disabling it for WebCore **only** did get rid of the crash too, so I think that's the one I'm proposing.

More specifically, this patch fixed the issue as well as the previous one I proposed, but without removing the optimization from gcc when building bmalloc, WTF, JSC and the WebKit API layers: "only" from WebCore, which is better.

See the proposed diff below:

diff --git a/Source/WebCore/CMakeLists.txt b/Source/WebCore/CMakeLists.txt
index 564a239..794caeb 100644
--- a/Source/WebCore/CMakeLists.txt
+++ b/Source/WebCore/CMakeLists.txt
@@ -3574,6 +3574,14 @@ add_library(WebCore ${WebCore_LIBRARY_TYPE} ${WebCore_SOURCES})
 set_target_properties(WebCore PROPERTIES COMPILE_DEFINITIONS "BUILDING_WebCore")
 set_target_properties(WebCore PROPERTIES FOLDER "WebCore")
 
+# The -ftree-sra optimization (implicit with -O2) causes crashes when
+# allocating large chunks of memory using bmalloc on Intel 32bit.
+# See https://bugs.webkit.org/show_bug.cgi?id=146440
+string(TOLOWER ${CMAKE_HOST_SYSTEM_PROCESSOR} LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR)
+if (CMAKE_COMPILER_IS_GNUCXX AND "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)")
+    ADD_TARGET_PROPERTIES(WebCore COMPILE_FLAGS "-fno-tree-sra")
+endif ()
+
 if (WebCore_OUTPUT_NAME)
     set_target_properties(WebCore PROPERTIES OUTPUT_NAME ${WebCore_OUTPUT_NAME})
 endif ()

What do you think? Is this worth proposing upstream?

[Michael Catanzaro]

OK, I agree it's worth taking that patch.

[Mario Sanchez Prada]

Created attachment 256038 [details]
Patch proposal

Here comes the patch, ready for review.

[Carlos Alberto Lopez Perez]

Comment on attachment 256038 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=256038&action=review

> Source/WebCore/CMakeLists.txt:3581
> +if (CMAKE_COMPILER_IS_GNUCXX AND "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)")

I think this should be: ... MATCHES "(i[3-6]86|x86)$")

Otherwise this becomes true also for AMD64 with GCC (where LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR = x86_64 ). (yes, there is a bug on the previous code introduced on 128855 <http://trac.webkit.org/r128855>)

[Carlos Alberto Lopez Perez]

(In reply to comment #14)
> Comment on attachment 256038 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256038&action=review
> 
> > Source/WebCore/CMakeLists.txt:3581
> > +if (CMAKE_COMPILER_IS_GNUCXX AND "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)")
> 
> I think this should be: ... MATCHES "(i[3-6]86|x86)$")
> 
> Otherwise this becomes true also for AMD64 with GCC (where
> LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR = x86_64 ). (yes, there is a bug on
> the previous code introduced on 128855 <http://trac.webkit.org/r128855>)

Also I don't think this will work for ARM 32-bit machines or $RANDOMARCH 32-bit machines.

[Carlos Garcia Campos]

Comment on attachment 256038 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=256038&action=review

Do you know how important this optimization is? My main concern, like with all other patches that disable optimizations to work around compiler bugs, is that it's very difficult to know when we can stop disabling it, and we end up with the optimization disabled forever. Also, I think we should report these bugs to GCC as well.

>>> Source/WebCore/CMakeLists.txt:3581
>>> +if (CMAKE_COMPILER_IS_GNUCXX AND "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)")
>> 
>> I think this should be: ... MATCHES "(i[3-6]86|x86)$")
>> 
>> Otherwise this becomes true also for AMD64 with GCC (where LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR = x86_64 ). (yes, there is a bug on the previous code introduced on 128855 <http://trac.webkit.org/r128855>)
> 
> Also I don't think this will work for ARM 32-bit machines or $RANDOMARCH 32-bit machines.

Yes, we do a similar match to add the --no-keep-memory linker option in 32 bits. I think we could move it to a common place, detecting also non intel 32 bits processors and exposing a variable to CMake.

[Mario Sanchez Prada]

(In reply to comment #14)
> Comment on attachment 256038 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256038&action=review
> 
> > Source/WebCore/CMakeLists.txt:3581
> > +if (CMAKE_COMPILER_IS_GNUCXX AND "${LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR}" MATCHES "(i[3-6]86|x86)")
> 
> I think this should be: ... MATCHES "(i[3-6]86|x86)$")
> 
> Otherwise this becomes true also for AMD64 with GCC (where
> LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR = x86_64 ). (yes, there is a bug on
> the previous code introduced on 128855 <http://trac.webkit.org/r128855>)

Good catch, I'll change that.

(In reply to comment #15)
> [...]
> > Otherwise this becomes true also for AMD64 with GCC (where
> > LOWERCASE_CMAKE_HOST_SYSTEM_PROCESSOR = x86_64 ). (yes, there is a bug on
> > the previous code introduced on 128855 <http://trac.webkit.org/r128855>)
> 
> Also I don't think this will work for ARM 32-bit machines or $RANDOMARCH
> 32-bit machines.

No, it won't work with ARM or other 32bit machines, but that's intentional: in the tests we did we saw this crash happening in Intel 32bit archs only but not on ARM, at least not in the case we tried. Thus, I'd rather keep this change for Intel 32bit only, because that's the only case where I'm sure the crash is happening.

(In reply to comment #16)
> Comment on attachment 256038 [details]
> Patch proposal
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256038&action=review
> 
> Do you know how important this optimization is? My main concern, like with
> all other patches that disable optimizations to work around compiler bugs,
> is that it's very difficult to know when we can stop disabling it, and we
> end up with the optimization disabled forever. Also, I think we should
> report these bugs to GCC as well.

Other than the fact that it reliably gets WebKit to crash if enabled, I don't know how important this optimization exactly is, which why I always said this is not a proper fix, but a workaround. That said, I agree on that doing this poses the problems you mentioned and that GCC devs should be reported about that, which is what Michael did on https://bugzilla.redhat.com/show_bug.cgi?id=1225733 already.  

> [...]
> > Also I don't think this will work for ARM 32-bit machines or $RANDOMARCH 32-bit machines.
> 
> Yes, we do a similar match to add the --no-keep-memory linker option in 32
> bits. I think we could move it to a common place, detecting also non intel
> 32 bits processors and exposing a variable to CMake.

As I said above, doing this for Intel 32bit only was intentional, as that's the only configuration I'm sure the bug is happening, so I'm not sure about merging this with the --no-keep-memory flag.

[Mario Sanchez Prada]

Created attachment 256097 [details]
Patch proposal

I talked to Carlos (KaL) on IRC this morning and agreed on that no more changes than the one proposed by clopez (which I'm doing with this new patch) are necessary, since we don't have enough evidence to know whether the -fno-tree-sra should be pass for ARM and other 32bit architectures.

FWIW, I've tested loading the URL mentioned above with MiniBrowser on ARM 32bit this morning, as well as the original use case specific to our platform, and I could not reproduce the crash at all with the unpatched compiled version of WebKitGTK+ 2.8.3, which suggests that applying it for Intel 32bit only is the right thing to do.

Please review, thanks!

[Gustavo Noronha (kov)]

Comment on attachment 256097 [details]
Patch proposal

LGTM, win seems to be broken by something unrelated to this

[Mario Sanchez Prada]

(In reply to comment #19)
> Comment on attachment 256097 [details]
> Patch proposal
> 
> LGTM, win seems to be broken by something unrelated to this

Thanks, I also think the win failure is unrelated. Will land soon now

[Mario Sanchez Prada]

Committed r186263: <http://trac.webkit.org/changeset/186263>