Bug 177459 – [CMake] ENABLE(DEVELOPER_MODE) should enable logging support for Release builds.

• Component: Tools / Tests
• Status: NEW

[Carlos Alberto Lopez Perez]

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.

[Michael Catanzaro]

It should also enable asserts (but we're not quite ready for that yet)

[Carlos Alberto Lopez Perez]

Created attachment 331491 [details]
Patch

[Konstantin Tokarev]

This may result in different performance of developer and non-developer release builds. Using -DRELEASE_LOG_DISABLED=0 should be safer

[Carlos Alberto Lopez Perez]

(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

[Michael Catanzaro]

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?

[Konstantin Tokarev]

// wtf/Assertions.h

#ifndef LOG_DISABLED
#define LOG_DISABLED ASSERTIONS_DISABLED_DEFAULT
#endif

[Konstantin Tokarev]

(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 :)

[Konstantin Tokarev]

(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?

[Carlos Alberto Lopez Perez]

(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-?

[Carlos Alberto Lopez Perez]

(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.

[Carlos Alberto Lopez Perez]

(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."

[Carlos Alberto Lopez Perez]

(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.

[Carlos Alberto Lopez Perez]

Created attachment 331516 [details]
Patch

Patch v2, set logging on wtf/Assertions.h

[Carlos Alberto Lopez Perez]

Comment on attachment 331516 [details]
Patch

The are more than LOG_DISABLED that should be set to 0.
Will upload new patch

[Michael Catanzaro]

(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....

[Carlos Alberto Lopez Perez]

Created attachment 331534 [details]
Patch

Patch v3, enable also error and fatal logs on developer builds

[Carlos Alberto Lopez Perez]

Comment on attachment 331534 [details]
Patch

Will try to do this in Assertions.h rather than in the cmake file.

[Konstantin Tokarev]

(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.

[Konstantin Tokarev]

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)

[Carlos Alberto Lopez Perez]

(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.