Currently if one wants to enable WebKit logging support it has to use Debug builds or pass manually the flag with something like CXXFLAGS="-DLOG_DISABLED=0" Its actually documented here: https://trac.webkit.org/wiki/WebKitGTK/Debugging#Loggingsupport For debug builds, logging is enabled at build-time but disabled by default at run-time. To enable it at run-time the environment variable WEBKIT_DEBUG should be set to the right value And for release builds I think it makes sense to do the same when ENABLE(DEVELOPER_MODE) is set (enable it at build-time). Its bit of a pain not being able to have logging support in release development builds.
It should also enable asserts (but we're not quite ready for that yet)
Created attachment 331491 [details] Patch
This may result in different performance of developer and non-developer release builds. Using -DRELEASE_LOG_DISABLED=0 should be safer
(In reply to Konstantin Tokarev from comment #3) > This may result in different performance of developer and non-developer > release builds. I don't think this will impact performance in any sensible way. The feature is disabled at runtime by default, you have to set the environment variable WEBKIT_DEBUG to enable it. > Using -DRELEASE_LOG_DISABLED=0 should be safer RELEASE_LOG_DISABLED is another complete different thing not supported in the CMake ports (GTK and WPE currently). It uses os_log() which is only supported on Mac. Check https://developer.apple.com/documentation/os/os_log and bug 160969 that introduced it
Comment on attachment 331491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=331491&action=review > Source/cmake/OptionsCommon.cmake:99 > +if (DEVELOPER_MODE) > + # Explicit enable logging support (ensures its also enabled on release development builds). > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DLOG_DISABLED=0") > +endif () Can you find a better way to do this? What is setting LOG_DISABLED in the first place?
// wtf/Assertions.h #ifndef LOG_DISABLED #define LOG_DISABLED ASSERTIONS_DISABLED_DEFAULT #endif
(In reply to Carlos Alberto Lopez Perez from comment #4) > RELEASE_LOG_DISABLED is another complete different thing not supported in > the CMake ports (GTK and WPE currently). It uses os_log() which is only > supported on Mac. Check https://developer.apple.com/documentation/os/os_log > and bug 160969 that introduced it We could define RELEASE_LOG macros to use "normal" logging. Though for me personally it would be really nice to see ltt-ng's tracelog() support built-in :)
(In reply to Michael Catanzaro from comment #1) > It should also enable asserts (but we're not quite ready for that yet) Are you really ready to fix all WebCore assertion as they pop up when sites change their content?
(In reply to Michael Catanzaro from comment #5) > Comment on attachment 331491 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331491&action=review > > > Source/cmake/OptionsCommon.cmake:99 > > +if (DEVELOPER_MODE) > > + # Explicit enable logging support (ensures its also enabled on release development builds). > > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DLOG_DISABLED=0") > > +endif () > > Can you find a better way to do this? What is setting LOG_DISABLED in the > first place? I don't think there is a better way to do this. On debug mode DLOG_DISABLED=0 gets set on the header because cmake defines -DNDEBUG=1 Can you please remove the r-?
(In reply to Konstantin Tokarev from comment #7) > (In reply to Carlos Alberto Lopez Perez from comment #4) > > RELEASE_LOG_DISABLED is another complete different thing not supported in > > the CMake ports (GTK and WPE currently). It uses os_log() which is only > > supported on Mac. Check https://developer.apple.com/documentation/os/os_log > > and bug 160969 that introduced it > > We could define RELEASE_LOG macros to use "normal" logging. Though for me > personally it would be really nice to see ltt-ng's tracelog() support > built-in :) That would be a different thing that I want to do here. Please open a new bug for that if you wish.
(In reply to Carlos Alberto Lopez Perez from comment #10) > (In reply to Konstantin Tokarev from comment #7) > > (In reply to Carlos Alberto Lopez Perez from comment #4) > > > RELEASE_LOG_DISABLED is another complete different thing not supported in > > > the CMake ports (GTK and WPE currently). It uses os_log() which is only > > > supported on Mac. Check https://developer.apple.com/documentation/os/os_log > > > and bug 160969 that introduced it > > > > We could define RELEASE_LOG macros to use "normal" logging. Though for me > > personally it would be really nice to see ltt-ng's tracelog() support > > built-in :) > > That would be a different thing that I want to do here. > Please open a new bug for that if you wish. I mean "That would be a different thing than what I want to do here."
(In reply to Carlos Alberto Lopez Perez from comment #9) > (In reply to Michael Catanzaro from comment #5) > > Comment on attachment 331491 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=331491&action=review > > > > > Source/cmake/OptionsCommon.cmake:99 > > > +if (DEVELOPER_MODE) > > > + # Explicit enable logging support (ensures its also enabled on release development builds). > > > + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DLOG_DISABLED=0") > > > +endif () > > > > Can you find a better way to do this? What is setting LOG_DISABLED in the > > first place? > > I don't think there is a better way to do this. > On debug mode DLOG_DISABLED=0 gets set on the header because cmake defines > -DNDEBUG=1 > > Can you please remove the r-? Well, it is possible to set it on wtf/Assertions.h if DEVELOPER_MODE is set. I can do that if you prefer.
Created attachment 331516 [details] Patch Patch v2, set logging on wtf/Assertions.h
Comment on attachment 331516 [details] Patch The are more than LOG_DISABLED that should be set to 0. Will upload new patch
(In reply to Carlos Alberto Lopez Perez from comment #12) > Well, it is possible to set it on wtf/Assertions.h if DEVELOPER_MODE is set. > I can do that if you prefer. That sounds better! (In reply to Konstantin Tokarev from comment #8) > Are you really ready to fix all WebCore assertion as they pop up when sites > change their content? Nope, but this is a proposal for DEVELOPER_MODE. Problem is, some of our developers like to use release builds, and don't notice when their code is broken because they aren't running assertions. Anyway, this a topic for another bug, another day....
Created attachment 331534 [details] Patch Patch v3, enable also error and fatal logs on developer builds
Comment on attachment 331534 [details] Patch Will try to do this in Assertions.h rather than in the cmake file.
(In reply to Michael Catanzaro from comment #15) > Problem is, some of our developers like to use release builds, and don't notice when their code is broken because they aren't running assertions. Same problem here. I think the only working solution (if WebCore's assert rate didn't decrease significantly between my branch and trunk) is having non-fatal assertions, just print message + backtrace and proceed.
Also, I can name yet another problem: JSC in general and Inspector in particular are too slow in debug, so everyone tries to avoid debug builds. I didn't research it yet, but I guess there may be unneeded assertions on the hot paths(which could be removed or put under special ifdef guards), and there may be hot paths which badly lack compiler optimizations (maybe we need to build JSC with enabled optimization in all build types, in old times Qt port was using this trick)
(In reply to Konstantin Tokarev from comment #19) > Also, I can name yet another problem: JSC in general and Inspector in > particular are too slow in debug, so everyone tries to avoid debug builds. I > didn't research it yet, but I guess there may be unneeded assertions on the > hot paths(which could be removed or put under special ifdef guards), and > there may be hot paths which badly lack compiler optimizations (maybe we > need to build JSC with enabled optimization in all build types, in old times > Qt port was using this trick) That is because Debug gets compiled with -O0. WebKit/JSC is painful slow without compiler optimizations.