146440 2015-06-29 16:40:54 -0700 Crash on xLarge memory allocation using bmalloc on 32bit systems 2015-07-03 13:40:15 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED https://bugzilla.redhat.com/show_bug.cgi?id=1225733 P2 Normal --- 1 mario webkit-unassigned cgarcia clopez cosimoc ggaren mark.lam mcatanzaro mrobinson oldest_to_newest 1105559 0 mario 2015-06-29 16:40:54 -0700 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 1105692 1 mario 2015-06-30 09:31:07 -0700 (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. 1106256 2 mario 2015-07-01 17:19:58 -0700 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 1106273 3 mario 2015-07-01 17:59:01 -0700 (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}") 1106280 4 clopez 2015-07-01 18:45:19 -0700 (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? 1106281 5 mcatanzaro 2015-07-01 18:58:51 -0700 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. :( 1106348 6 mario 2015-07-02 02:10:49 -0700 (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 :) 1106358 7 clopez 2015-07-02 03:31:11 -0700 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 1106360 8 mario 2015-07-02 04:16:52 -0700 (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. 1106361 9 mario 2015-07-02 04:18:46 -0700 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 1106384 10 mario 2015-07-02 07:13:39 -0700 (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... 1106402 11 mario 2015-07-02 10:23:34 -0700 (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? 1106403 12 mcatanzaro 2015-07-02 10:28:33 -0700 OK, I agree it's worth taking that patch. 1106485 13 256038 mario 2015-07-02 15:07:39 -0700 Created attachment 256038 Patch proposal Here comes the patch, ready for review. 1106587 14 256038 clopez 2015-07-02 18:50:23 -0700 Comment on attachment 256038 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>) 1106588 15 clopez 2015-07-02 19:12:23 -0700 (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. 1106624 16 256038 cgarcia 2015-07-02 23:00:40 -0700 Comment on attachment 256038 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. 1106652 17 mario 2015-07-03 01:24:11 -0700 (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. 1106685 18 256097 mario 2015-07-03 05:57:35 -0700 Created attachment 256097 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! 1106754 19 256097 gustavo 2015-07-03 12:47:24 -0700 Comment on attachment 256097 Patch proposal LGTM, win seems to be broken by something unrelated to this 1106766 20 mario 2015-07-03 13:33:39 -0700 (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 1106768 21 mario 2015-07-03 13:40:15 -0700 Committed r186263: <http://trac.webkit.org/changeset/186263> 256038 2015-07-02 15:07:39 -0700 2015-07-03 05:57:35 -0700 Patch proposal 0001-2015-07-02-Mario-Sanchez-Prada-mario-endlessm.com.patch text/plain 2811 mario RnJvbSA5OGRkYWViZTA5MTY1ODVmZDgzMTE4OWU5MjRiNDdjYmZlY2VhZTc1IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJpbyBTYW5jaGV6IFByYWRhIDxtYXJpb0B3ZWJraXQub3Jn PgpEYXRlOiBUaHUsIDIgSnVsIDIwMTUgMjM6MDQ6MzEgKzAxMDAKU3ViamVjdDogW1BBVENIXSAy MDE1LTA3LTAyICBNYXJpbyBTYW5jaGV6IFByYWRhICA8bWFyaW9AZW5kbGVzc20uY29tPgoKICAg ICAgICBDcmFzaCBvbiB4TGFyZ2UgbWVtb3J5IGFsbG9jYXRpb24gdXNpbmcgYm1hbGxvYyBvbiAz MmJpdCBzeXN0ZW1zCiAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE0NjQ0MAoKICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KCiAgICAgICAg RGlzYWJsZSB0aGUgZ2NjJ3MgLWZ0cmVlLXNyYSBvcHRpbWl6YXRpb24gKGF1dG9tYXRpY2FsbHkg ZW5hYmxlZAogICAgICAgIHdpdGggLU8xIGFuZCBoaWdoZXIgbGV2ZWxzKSBmb3IgV2ViQ29yZSBh bmQgMzJiaXQgSW50ZWwgYXJjaGl0ZWN0dXJlcywKICAgICAgICBhcyB0aGF0IGNhdXNlcyB0aGUg Y3Jhc2ggaW4gYm1hbGxvYyB3aGVuIGFsbG9jYXRpbmcgbGFyZ2UgYW1vdW50cyBvZgogICAgICAg IG1lbW9yeSBmcm9tIHRoZSB0ZXh0dXJlIG1hcHBlcidzIHRpbGVkIGJhY2tpbmcgc3RvcmUgaW1w bGVtZW50YXRpb24uCgogICAgICAgICogQ01ha2VMaXN0cy50eHQ6IFBhc3MgLWZuby1mcmVlLXNy YSB0byBnY2Mgb24gMzJiaXQgSW50ZWwgYXJjaGl0ZWN0dXJlcy4KLS0tCiBTb3VyY2UvV2ViQ29y ZS9DTWFrZUxpc3RzLnR4dCB8ICA4ICsrKysrKysrCiBTb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cg ICAgICB8IDE0ICsrKysrKysrKysrKysrCiAyIGZpbGVzIGNoYW5nZWQsIDIyIGluc2VydGlvbnMo KykKCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9DTWFrZUxpc3RzLnR4dCBiL1NvdXJjZS9X ZWJDb3JlL0NNYWtlTGlzdHMudHh0CmluZGV4IDU2NGEyMzkuLjc5NGNhZWIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL0NNYWtlTGlzdHMudHh0CisrKyBiL1NvdXJjZS9XZWJDb3JlL0NNYWtl TGlzdHMudHh0CkBAIC0zNTc0LDYgKzM1NzQsMTQgQEAgYWRkX2xpYnJhcnkoV2ViQ29yZSAke1dl YkNvcmVfTElCUkFSWV9UWVBFfSAke1dlYkNvcmVfU09VUkNFU30pCiBzZXRfdGFyZ2V0X3Byb3Bl cnRpZXMoV2ViQ29yZSBQUk9QRVJUSUVTIENPTVBJTEVfREVGSU5JVElPTlMgIkJVSUxESU5HX1dl YkNvcmUiKQogc2V0X3RhcmdldF9wcm9wZXJ0aWVzKFdlYkNvcmUgUFJPUEVSVElFUyBGT0xERVIg IldlYkNvcmUiKQogCisjIFRoZSAtZnRyZWUtc3JhIG9wdGltaXphdGlvbiAoaW1wbGljaXQgd2l0 aCAtTzIpIGNhdXNlcyBjcmFzaGVzIHdoZW4KKyMgYWxsb2NhdGluZyBsYXJnZSBjaHVua3Mgb2Yg bWVtb3J5IHVzaW5nIGJtYWxsb2Mgb24gSW50ZWwgMzJiaXQuCisjIFNlZSBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ2NDQwCitzdHJpbmcoVE9MT1dFUiAke0NNQUtF X0hPU1RfU1lTVEVNX1BST0NFU1NPUn0gTE9XRVJDQVNFX0NNQUtFX0hPU1RfU1lTVEVNX1BST0NF U1NPUikKK2lmIChDTUFLRV9DT01QSUxFUl9JU19HTlVDWFggQU5EICIke0xPV0VSQ0FTRV9DTUFL RV9IT1NUX1NZU1RFTV9QUk9DRVNTT1J9IiBNQVRDSEVTICIoaVszLTZdODZ8eDg2KSIpCisgICAg QUREX1RBUkdFVF9QUk9QRVJUSUVTKFdlYkNvcmUgQ09NUElMRV9GTEFHUyAiLWZuby10cmVlLXNy YSIpCitlbmRpZiAoKQorCiBpZiAoV2ViQ29yZV9PVVRQVVRfTkFNRSkKICAgICBzZXRfdGFyZ2V0 X3Byb3BlcnRpZXMoV2ViQ29yZSBQUk9QRVJUSUVTIE9VVFBVVF9OQU1FICR7V2ViQ29yZV9PVVRQ VVRfTkFNRX0pCiBlbmRpZiAoKQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9n IGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IGE5ZjRlMzIuLmIyMDM4MDIgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViQ29yZS9DaGFu Z2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAxNS0wNy0wMiAgTWFyaW8gU2FuY2hleiBQcmFkYSAg PG1hcmlvQGVuZGxlc3NtLmNvbT4KKworICAgICAgICBDcmFzaCBvbiB4TGFyZ2UgbWVtb3J5IGFs bG9jYXRpb24gdXNpbmcgYm1hbGxvYyBvbiAzMmJpdCBzeXN0ZW1zCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDY0NDAKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBEaXNhYmxlIHRoZSBnY2MncyAtZnRyZWUt c3JhIG9wdGltaXphdGlvbiAoYXV0b21hdGljYWxseSBlbmFibGVkCisgICAgICAgIHdpdGggLU8x IGFuZCBoaWdoZXIgbGV2ZWxzKSBmb3IgV2ViQ29yZSBhbmQgMzJiaXQgSW50ZWwgYXJjaGl0ZWN0 dXJlcywKKyAgICAgICAgYXMgdGhhdCBjYXVzZXMgdGhlIGNyYXNoIGluIGJtYWxsb2Mgd2hlbiBh bGxvY2F0aW5nIGxhcmdlIGFtb3VudHMgb2YKKyAgICAgICAgbWVtb3J5IGZyb20gdGhlIHRleHR1 cmUgbWFwcGVyJ3MgdGlsZWQgYmFja2luZyBzdG9yZSBpbXBsZW1lbnRhdGlvbi4KKworICAgICAg ICAqIENNYWtlTGlzdHMudHh0OiBQYXNzIC1mbm8tZnJlZS1zcmEgdG8gZ2NjIG9uIDMyYml0IElu dGVsIGFyY2hpdGVjdHVyZXMuCisKIDIwMTUtMDctMDIgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25A YXBwbGUuY29tPgogCiAgICAgICAgIEFkZCBwcmVmZXJlbmNlIHRvIGRpc2FibGUgYWxsIGh0dHAt ZXF1aXYuCi0tIAoyLjQuMwoK 256097 2015-07-03 05:57:35 -0700 2015-07-03 12:47:24 -0700 Patch proposal 0001-2015-07-02-Mario-Sanchez-Prada-mario-endlessm.com.patch text/plain 2812 mario RnJvbSBmZTc3OGNkZjE3Y2UwYjY4MjkzMGNkYTQ5NjA0NGEzMjUzMGU0MmI3IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJpbyBTYW5jaGV6IFByYWRhIDxtYXJpb0B3ZWJraXQub3Jn PgpEYXRlOiBUaHUsIDIgSnVsIDIwMTUgMjM6MDQ6MzEgKzAxMDAKU3ViamVjdDogW1BBVENIXSAy MDE1LTA3LTAyICBNYXJpbyBTYW5jaGV6IFByYWRhICA8bWFyaW9AZW5kbGVzc20uY29tPgoKICAg ICAgICBDcmFzaCBvbiB4TGFyZ2UgbWVtb3J5IGFsbG9jYXRpb24gdXNpbmcgYm1hbGxvYyBvbiAz MmJpdCBzeXN0ZW1zCiAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE0NjQ0MAoKICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KCiAgICAgICAg RGlzYWJsZSB0aGUgZ2NjJ3MgLWZ0cmVlLXNyYSBvcHRpbWl6YXRpb24gKGF1dG9tYXRpY2FsbHkg ZW5hYmxlZAogICAgICAgIHdpdGggLU8xIGFuZCBoaWdoZXIgbGV2ZWxzKSBmb3IgV2ViQ29yZSBh bmQgMzJiaXQgSW50ZWwgYXJjaGl0ZWN0dXJlcywKICAgICAgICBhcyB0aGF0IGNhdXNlcyB0aGUg Y3Jhc2ggaW4gYm1hbGxvYyB3aGVuIGFsbG9jYXRpbmcgbGFyZ2UgYW1vdW50cyBvZgogICAgICAg IG1lbW9yeSBmcm9tIHRoZSB0ZXh0dXJlIG1hcHBlcidzIHRpbGVkIGJhY2tpbmcgc3RvcmUgaW1w bGVtZW50YXRpb24uCgogICAgICAgICogQ01ha2VMaXN0cy50eHQ6IFBhc3MgLWZuby1mcmVlLXNy YSB0byBnY2Mgb24gMzJiaXQgSW50ZWwgYXJjaGl0ZWN0dXJlcy4KLS0tCiBTb3VyY2UvV2ViQ29y ZS9DTWFrZUxpc3RzLnR4dCB8ICA4ICsrKysrKysrCiBTb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cg ICAgICB8IDE0ICsrKysrKysrKysrKysrCiAyIGZpbGVzIGNoYW5nZWQsIDIyIGluc2VydGlvbnMo KykKCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9DTWFrZUxpc3RzLnR4dCBiL1NvdXJjZS9X ZWJDb3JlL0NNYWtlTGlzdHMudHh0CmluZGV4IDU2NGEyMzkuLjZkMTkyYzQgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL0NNYWtlTGlzdHMudHh0CisrKyBiL1NvdXJjZS9XZWJDb3JlL0NNYWtl TGlzdHMudHh0CkBAIC0zNTc0LDYgKzM1NzQsMTQgQEAgYWRkX2xpYnJhcnkoV2ViQ29yZSAke1dl YkNvcmVfTElCUkFSWV9UWVBFfSAke1dlYkNvcmVfU09VUkNFU30pCiBzZXRfdGFyZ2V0X3Byb3Bl cnRpZXMoV2ViQ29yZSBQUk9QRVJUSUVTIENPTVBJTEVfREVGSU5JVElPTlMgIkJVSUxESU5HX1dl YkNvcmUiKQogc2V0X3RhcmdldF9wcm9wZXJ0aWVzKFdlYkNvcmUgUFJPUEVSVElFUyBGT0xERVIg IldlYkNvcmUiKQogCisjIFRoZSAtZnRyZWUtc3JhIG9wdGltaXphdGlvbiAoaW1wbGljaXQgd2l0 aCAtTzIpIGNhdXNlcyBjcmFzaGVzIHdoZW4KKyMgYWxsb2NhdGluZyBsYXJnZSBjaHVua3Mgb2Yg bWVtb3J5IHVzaW5nIGJtYWxsb2Mgb24gSW50ZWwgMzJiaXQuCisjIFNlZSBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ2NDQwCitzdHJpbmcoVE9MT1dFUiAke0NNQUtF X0hPU1RfU1lTVEVNX1BST0NFU1NPUn0gTE9XRVJDQVNFX0NNQUtFX0hPU1RfU1lTVEVNX1BST0NF U1NPUikKK2lmIChDTUFLRV9DT01QSUxFUl9JU19HTlVDWFggQU5EICIke0xPV0VSQ0FTRV9DTUFL RV9IT1NUX1NZU1RFTV9QUk9DRVNTT1J9IiBNQVRDSEVTICIoaVszLTZdODZ8eDg2KSQiKQorICAg IEFERF9UQVJHRVRfUFJPUEVSVElFUyhXZWJDb3JlIENPTVBJTEVfRkxBR1MgIi1mbm8tdHJlZS1z cmEiKQorZW5kaWYgKCkKKwogaWYgKFdlYkNvcmVfT1VUUFVUX05BTUUpCiAgICAgc2V0X3Rhcmdl dF9wcm9wZXJ0aWVzKFdlYkNvcmUgUFJPUEVSVElFUyBPVVRQVVRfTkFNRSAke1dlYkNvcmVfT1VU UFVUX05BTUV9KQogZW5kaWYgKCkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxv ZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBhOWY0ZTMyLi5iMjAzODAyIDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTUtMDctMDIgIE1hcmlvIFNhbmNoZXogUHJhZGEg IDxtYXJpb0BlbmRsZXNzbS5jb20+CisKKyAgICAgICAgQ3Jhc2ggb24geExhcmdlIG1lbW9yeSBh bGxvY2F0aW9uIHVzaW5nIGJtYWxsb2Mgb24gMzJiaXQgc3lzdGVtcworICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ2NDQwCisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRGlzYWJsZSB0aGUgZ2NjJ3MgLWZ0cmVl LXNyYSBvcHRpbWl6YXRpb24gKGF1dG9tYXRpY2FsbHkgZW5hYmxlZAorICAgICAgICB3aXRoIC1P MSBhbmQgaGlnaGVyIGxldmVscykgZm9yIFdlYkNvcmUgYW5kIDMyYml0IEludGVsIGFyY2hpdGVj dHVyZXMsCisgICAgICAgIGFzIHRoYXQgY2F1c2VzIHRoZSBjcmFzaCBpbiBibWFsbG9jIHdoZW4g YWxsb2NhdGluZyBsYXJnZSBhbW91bnRzIG9mCisgICAgICAgIG1lbW9yeSBmcm9tIHRoZSB0ZXh0 dXJlIG1hcHBlcidzIHRpbGVkIGJhY2tpbmcgc3RvcmUgaW1wbGVtZW50YXRpb24uCisKKyAgICAg ICAgKiBDTWFrZUxpc3RzLnR4dDogUGFzcyAtZm5vLWZyZWUtc3JhIHRvIGdjYyBvbiAzMmJpdCBJ bnRlbCBhcmNoaXRlY3R1cmVzLgorCiAyMDE1LTA3LTAyICBCcmFkeSBFaWRzb24gIDxiZWlkc29u QGFwcGxlLmNvbT4KIAogICAgICAgICBBZGQgcHJlZmVyZW5jZSB0byBkaXNhYmxlIGFsbCBodHRw LWVxdWl2LgotLSAKMi40LjMKCg==