Bug 137973 – Disable Linux-specific code in a Windows build

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Milan Crha]

The used functions are not available under windows, but it can be built otherwise, thus, in a price of a lighter functionality, disable the code under Windows. Please let me know, if I missed anything.

[Milan Crha]

Created attachment 240283 [details]
proposed wk patch

[Benjamin Poulain]

Comment on attachment 240283 [details]
proposed wk patch

I don't really see the point. We optimize binary size for ARM, it does not really matter for Windows.

If there is no negative side effect, we can leave the code as it is. Adding #ifdefs is adding complexity to the codebase.

[Milan Crha]

The functions being "commented out" are Linux specific, they are not available under windows (mine mingw/msys build environment). Having them available/enabled is not about binary size, it's about being able to build the source file at all, for a price of a less functionality being provided in the windows builds (no stacktraces).

[Joseph Pecoraro]

Comment on attachment 240283 [details]
proposed wk patch

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

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:47
> +#if !PLATFORM(WIN)

In wtf/Assertions.cpp they use:

#if OS(DARWIN) || (OS(LINUX) && !defined(__UCLIBC__))
...
#endif

I would be fine with doing that here.

[Milan Crha]

Created attachment 240788 [details]
proposed wk patch ][

You are right, that works too.

[Joseph Pecoraro]

Comment on attachment 240788 [details]
proposed wk patch ][

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

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:172
> +#endif

Do you also need:

    #else
    UNUSED_PARAM(callStack);
    #endif

To avoid other warnings?

[Milan Crha]

Created attachment 240906 [details]
proposed wk patch ]I[

Nice, I didn't know you've a macro for it. I noticed the warning, but there are shown more other from various places when I compile the code on Windows, thus I didn't pay attention to it. My fault, I'm sorry for that.

[Brent Fulgham]

Joe, it looks like the changes you requested are in the patch. Is this okay to take now?

[Joseph Pecoraro]

Comment on attachment 240906 [details]
proposed wk patch ]I[

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

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:152
> +#if OS(DARWIN) || (OS(LINUX) && !defined(__UCLIBC__))

Based on wtf/Assertions.cpp, which has:

    #if OS(DARWIN) || OS(LINUX)
    #  if PLATFORM(GTK)
    #    if defined(__GLIBC__) && !defined(__UCLIBC__)
    #      define WTF_USE_BACKTRACE_SYMBOLS 1
    #    endif
    #  else
    #    define WTF_USE_DLADDR 1
    #  endif
    #endif

This should probably be:

    #if OS(DARWIN) || (OS(LINUX) && !PLATFORM(GTK))

Same up above in the includes. Do you agree?

> Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:177
> +        UNUSED_PARAM(callStack);

Nit: Too much indentation.

[Milan Crha]

Created attachment 250497 [details]
proposed wk patch IV

Updated webkit patch with the suggested changes applied. I agree with both.

[Milan Crha]

(In reply to comment #9)
> This should probably be:
> 
>     #if OS(DARWIN) || (OS(LINUX) && !PLATFORM(GTK))
> 
> Same up above in the includes. Do you agree?

Hmm, thinking of it, this will disable backtraces for any GTK platform build, not only for those which do not support it, like those not having dlfcn.h and execinfo.h.

[Joseph Pecoraro]

(In reply to comment #10)
> Created attachment 250497 [details]
> proposed wk patch IV
> 
> Updated webkit patch with the suggested changes applied. I agree with both.

(In reply to comment #11)
> (In reply to comment #9)
> > This should probably be:
> > 
> >     #if OS(DARWIN) || (OS(LINUX) && !PLATFORM(GTK))
> > 
> > Same up above in the includes. Do you agree?
> 
> Hmm, thinking of it, this will disable backtraces for any GTK platform
> build, not only for those which do not support it, like those not having
> dlfcn.h and execinfo.h.

I believe this code is only reachable through ENABLE_REMOTE_INSPECTOR so it really doesn't matter ;)

[Joseph Pecoraro]

Comment on attachment 250497 [details]
proposed wk patch IV

r=me

[WebKit Commit Bot]

Comment on attachment 250497 [details]
proposed wk patch IV

Clearing flags on attachment: 250497

Committed r182636: <http://trac.webkit.org/changeset/182636>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.