It seems that with embedded build systems like Yocto or Buildroot is relatively easier to shoot on your foot and compile without optimizations. That happened just to me playing with the value of CXXFLAGS on a Yocto recipe. I believe that nobody wants to build release without compiler optimizations. So we can generate an error at build time that will alert the developer. I believe that a build time error is much better than an unexpected "oh, WebKit is really slow ..." situation.
Created attachment 322197 [details] Patch
Comment on attachment 322197 [details] Patch Mmmmm... this was unexpected!! Why are the Mac/JSC EWS bots building release without compiling optimizations???? If you check the command line of the bots that failed they first pass -O3 and then -O0 (conflicting compiler switches, the last one to be specified wins.. so the EWS are effectively building release with -O0 )
In the Build Phases section of testair, there's an extra -O0 in the per-file CFLAGS, but the testair project is -O3 in release normally. I think you'll need to ask pizlo which is expected.
(In reply to Tim Horton from comment #3) > In the Build Phases section of testair, there's an extra -O0 in the per-file > CFLAGS, but the testair project is -O3 in release normally. I think you'll > need to ask pizlo which is expected. Interesting. I see this was done in bug 153520 for compilation speed.
Created attachment 322215 [details] Patch
Comment on attachment 322215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322215&action=review I think this is a good idea, but I have one minor complaint and one major complaint. The major complaint is that this breaks the standard 'cmake -DPORT=GTK && make' build. And that's fine, because that's really never what you want to do, but we should error out earlier, at cmake time, rather than when compiling. I think it's fine to keep the check in Compiler.h, because it has to be there to catch weird issues with CFLAGS or CXXFLAGS. But I would add an additional check in the CMake build somewhere to error out even earlier if CMAKE_BUILD_TYPE is unset. Most users will hit that error first rather than the Compiler.h error, and then only people who have accidentally messed up CFLAGS or CXXFLAGS will see the error in Compiler.h. > Source/WTF/wtf/Compiler.h:108 > +#error "Building release without compiler optimizations: WebKit will be slow. Set -DRELEASE_WITHOUT_OPTIMIZATIONS if this is intended." Minor complaint: I think it would be better for the error message to tell the user how to enable optimizations using -DCMAKE_BUILD_TYPE=Release or -DCMAKE_BUILD_TYPE=RelWithDebInfo, rather than how to workaround the lack of optimization. Because as you note, generally nobody wants to do that. That means you'd need to make the error message dependent on the value of BUILDING_WITH_CMAKE as well. In that case, you could decide whether or not you want an error out on Mac XCode at all. I would probably leave it be.
(In reply to Michael Catanzaro from comment #6) > Comment on attachment 322215 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322215&action=review > > I think this is a good idea, but I have one minor complaint and one major > complaint. The major complaint is that this breaks the standard 'cmake > -DPORT=GTK && make' build. It doesn't break that, because before error'ing it checks if NDEBUG is defined or not. And NDEBUG will be only set by CMake if you specify a build type Release. > But I would add an additional check in the CMake build somewhere to error out even earlier if CMAKE_BUILD_TYPE is unset. That looks a good idea. I will add it.
Created attachment 322229 [details] Patch Add check to ensure CMAKE_BUILD_TYPE is valid
Comment on attachment 322229 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322229&action=review > CMakeLists.txt:10 > +if (CMAKE_BUILD_TYPE) > + string(TOUPPER ${CMAKE_BUILD_TYPE} uppercase_CMAKE_BUILD_TYPE) > + if (NOT uppercase_CMAKE_BUILD_TYPE MATCHES "^(DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL)$") > + message(FATAL_ERROR "Invalid value for CMAKE_BUILD_TYPE: ${CMAKE_BUILD_TYPE}") > + endif () > +else () Ehh, now I'm having second thoughts. We don't want to police the possible values of CMAKE_BUILD_TYPE. If CMake doesn't do this itself, that's CMake's problem. I'd hate to block use of a future build type. Perhaps we should just fail the build if it is completely unset. Honestly I'm having second thoughts about that too, since distros that like to have full control over compiler flags might intentionally not set any build type. Maybe the Compiler.h check is enough. Dunno.
(In reply to Michael Catanzaro from comment #9) > Comment on attachment 322229 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322229&action=review > > > CMakeLists.txt:10 > > +if (CMAKE_BUILD_TYPE) > > + string(TOUPPER ${CMAKE_BUILD_TYPE} uppercase_CMAKE_BUILD_TYPE) > > + if (NOT uppercase_CMAKE_BUILD_TYPE MATCHES "^(DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL)$") > > + message(FATAL_ERROR "Invalid value for CMAKE_BUILD_TYPE: ${CMAKE_BUILD_TYPE}") > > + endif () > > +else () > > Ehh, now I'm having second thoughts. We don't want to police the possible > values of CMAKE_BUILD_TYPE. If CMake doesn't do this itself, that's CMake's > problem. I'd hate to block use of a future build type. Perhaps we should > just fail the build if it is completely unset. > > Honestly I'm having second thoughts about that too, since distros that like > to have full control over compiler flags might intentionally not set any > build type. Maybe the Compiler.h check is enough. Dunno. I think we should ensure the default values are sane. I'm more worried about an user just running "cmake -DPORT=GTK .. && make" without realizing what she is really doing, than worried about annoying some distro that likes to do clever things with the compiler flags. In this last case, this distro can simply pass the path to a custom -DCMAKE_TOOLCHAIN_FILE= where they re-define the FLAGS for the build type, they can patch this out, or they can simply define build type Debug and pass in the CXXFLAGS/CFLAGS="-DNDEBUG -DRELEASE_WITHOUT_OPTIMIZATIONS [extraflags]". Its not a big deal IMHO. BTW.. I borrowed the check from LLVM: https://github.com/llvm-mirror/llvm/blob/master/CMakeLists.txt#L270 They think is fine to do this.
(In reply to Carlos Alberto Lopez Perez from comment #10) > (In reply to Michael Catanzaro from comment #9) > > Comment on attachment 322229 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=322229&action=review > > > > > CMakeLists.txt:10 > > > +if (CMAKE_BUILD_TYPE) > > > + string(TOUPPER ${CMAKE_BUILD_TYPE} uppercase_CMAKE_BUILD_TYPE) > > > + if (NOT uppercase_CMAKE_BUILD_TYPE MATCHES "^(DEBUG|RELEASE|RELWITHDEBINFO|MINSIZEREL)$") > > > + message(FATAL_ERROR "Invalid value for CMAKE_BUILD_TYPE: ${CMAKE_BUILD_TYPE}") > > > + endif () > > > +else () > > > > Ehh, now I'm having second thoughts. We don't want to police the possible > > values of CMAKE_BUILD_TYPE. If CMake doesn't do this itself, that's CMake's > > problem. I'd hate to block use of a future build type. Perhaps we should > > just fail the build if it is completely unset. > > > > Honestly I'm having second thoughts about that too, since distros that like > > to have full control over compiler flags might intentionally not set any > > build type. Maybe the Compiler.h check is enough. Dunno. > > I think we should ensure the default values are sane. > > I'm more worried about an user just running "cmake -DPORT=GTK .. && make" > without realizing what she is really doing, than worried about annoying some > distro that likes to do clever things with the compiler flags. > > In this last case, this distro can simply pass the path to a custom > -DCMAKE_TOOLCHAIN_FILE= where they re-define the FLAGS for the build type, > they can patch this out, or they can simply define build type Debug and pass > in the CXXFLAGS/CFLAGS="-DNDEBUG -DRELEASE_WITHOUT_OPTIMIZATIONS > [extraflags]". Its not a big deal IMHO. > > BTW.. I borrowed the check from LLVM: > https://github.com/llvm-mirror/llvm/blob/master/CMakeLists.txt#L270 > They think is fine to do this. And another issue that comes to mind with distros setting CCFLAGS/CXXFLAGS to some optimization value manually but not setting CMAKE_BUILD_TYPE is that they are really building something like DebugWithOptimizations, because they are also likely not setting -DNDEBUG .... therefore they are building all codepaths that we intended to be enabled only on Debug builds :(. Another option if we don't want to cause a build time error is setting a default value of Release. But I'm unsure if this is better than causing a failure and letting the developer review the setting
(In reply to Carlos Alberto Lopez Perez from comment #11) > And another issue that comes to mind with distros setting CCFLAGS/CXXFLAGS > to some optimization value manually but not setting CMAKE_BUILD_TYPE is that > they are really building something like DebugWithOptimizations, because they > are also likely not setting -DNDEBUG .... therefore they are building all > codepaths that we intended to be enabled only on Debug builds :(. True, that should probably be discouraged.
Created attachment 322546 [details] Patch Dont cause an error if CMAKE_BUILD_TYPE is not set, just print a warning and default to RelWithDebInfo
Comment on attachment 322546 [details] Patch Attachment 322546 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4745199 New failing tests: workers/wasm-long-compile.html
Created attachment 322591 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 322546 [details] Patch Clearing flags on attachment: 322546 Committed r222840: <http://trac.webkit.org/changeset/222840>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34810030>
(In reply to WebKit Commit Bot from comment #17) > All reviewed patches have been landed. Closing bug. \o/
Reverted r222840 for reason: This change breaks internal builds. Committed r222870: <http://trac.webkit.org/changeset/222870>
(In reply to Ryan Haddad from comment #20) > Reverted r222840 for reason: > > This change breaks internal builds. > > Committed r222870: <http://trac.webkit.org/changeset/222870> Sorry about the rollout, but this breaks the build for some of our internal configurations. Tim, I added a link to <rdar://problem/34810030>, can you take a look to see if this is something we can resolve before attempting to re-land this patch?
I think we need Brian Burg.
Comment on attachment 322546 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322546&action=review > Source/WTF/ChangeLog:18 > + The intention of this patch is to ensure that nobody builds Release > + without enabling compiler optimization by mistake. Does this happen often?
(In reply to Filip Pizlo from comment #23) > Does this happen often? Sadly, I would be pretty unsurprised if many companies are shipping unoptimized builds of WebKit by accident. The technical competence and quality expectations of many embedded device manufacturers are very low. Anyway, it's trivial to avoid breaking the internal build by adding an && !PLATFORM(COCOA) there, but let's wait and see what Brian wants to do.
(In reply to Filip Pizlo from comment #23) > Comment on attachment 322546 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=322546&action=review > > > Source/WTF/ChangeLog:18 > > + The intention of this patch is to ensure that nobody builds Release > > + without enabling compiler optimization by mistake. > > Does this happen often? Not often, but its relatively easy to happen. And its a major pain, because many developer hours will go to the trash bin until someone notices this simple (but dangerous) mistake. For example, with Yocto (which is currently the most popular solution for cross-building Linux-based embedded solutions) simply setting CXXFLAGS="something" on the WebKit recipe will cause that optimizations are disabled. That is because Yocto patches the CMake definitions of Release, RelWithDebugInfo, etc to remove the optimization flags from there (they keep the -DNDEBUG flag but remove the -O3/-O2). And then they pass the -O2 as part of a global CFLAGS/CXXFLAGS environment value. So any user that wants to pass any custom CXXFLAGS to their build can easily end bitten by this. As its not obvious that by setting some new value to the variable (instead of appending to it) optimizations can be disabled. And I have heard histories of this also happening with Buildroot (which is also another popular solution for cross-building). So with this simple patch that I proposed here we ensure things like this won't happen anymore by mistake.
(In reply to Tim Horton from comment #22) > I think we need Brian Burg. I have identified a fix. I'll set this to cq+ once it's ready.
Comment on attachment 322546 [details] Patch r=me, let's try again.
Comment on attachment 322546 [details] Patch Clearing flags on attachment: 322546 Committed r222930: <http://trac.webkit.org/changeset/222930>