Bug 186722 – [CMake] Automatically disable JIT and enable USE_SYSTEM_MALLOC on unfamiliar architectures

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

We know that the JSC JIT and bmalloc both work on only a limited set of architectures. In Fedora, we have to manually disable these when building for s390x, ppc64, and ppc64le. But it's really easy to do the right thing automatically, so we might as well.

[Michael Catanzaro]

Created attachment 342881 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 342881 [details]
Patch

Clearing flags on attachment: 342881

Committed r232918: <https://trac.webkit.org/changeset/232918>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tomas Popela]

Nice one Michael!

[Michael Catanzaro]

*** Bug 175767 has been marked as a duplicate of this bug. ***

[Alberto Garcia]

This is broken, the Debian ppc64el build (among others) fails now
because it defaults to ENABLE_JIT=ON. See full build log here:

   https://buildd.debian.org/status/package.php?p=webkit2gtk&suite=experimental

I tried to debug this a bit and I see that ENABLE_JIT_DEFAULT is set
correctly (to OFF in this case) but later on the "Enabled features"
list you can see that ENABLE_JIT is ON.

With my zero CMake knowledge it seems that whatever ENABLE_JIT value
is set in WebKitFeatures.cmake is later overridden in OptionsGTK.cmake
by this line:

   WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ON)

Setting it to ${ENABLE_JIT_DEFAULT} instead of ON appears to solve the
problem, although I haven't run the full build yet.

Also I notice that ENABLE_JIT is a PRIVATE option in
WebKitFeatures.cmake but PUBLIC in OptionsGTK.cmake. Does that make
sense?

[Alberto Garcia]

(In reply to Alberto Garcia from comment #6)
> With my zero CMake knowledge it seems that whatever ENABLE_JIT value
> is set in WebKitFeatures.cmake is later overridden in OptionsGTK.cmake

Same problem with USE_SYSTEM_MALLOC by the way.

[Alberto Garcia]

(In reply to Alberto Garcia from comment #6)
>    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ON)
>
> Setting it to ${ENABLE_JIT_DEFAULT} instead of ON appears to solve the
> problem, although I haven't run the full build yet.

I confirm that this fixes the build.

[Michael Catanzaro]

(In reply to Alberto Garcia from comment #6)
> With my zero CMake knowledge it seems that whatever ENABLE_JIT value
> is set in WebKitFeatures.cmake is later overridden in OptionsGTK.cmake
> by this line:
> 
>    WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ON)

Oops, yes indeed, good catch Berto. OptionsWPE.cmake has the same problem. Other ports are fine.

I'm not really happy with depending on a variable that ought to be private to WebKitFeatures.cmake, so let me think about how to rework this check.

> Setting it to ${ENABLE_JIT_DEFAULT} instead of ON appears to solve the
> problem, although I haven't run the full build yet.
> 
> Also I notice that ENABLE_JIT is a PRIVATE option in
> WebKitFeatures.cmake but PUBLIC in OptionsGTK.cmake. Does that make
> sense?

Yes, all options are PRIVATE in WebKitFeatures.cmake to ensure ports control which options are exposed. Actually the only reason to have it in OptionsGTK.cmake is to change it to public.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #9) 
> I'm not really happy with depending on a variable that ought to be private
> to WebKitFeatures.cmake, so let me think about how to rework this check.

I can't think of another way, so let's do this.

[Michael Catanzaro]

Actually no, let's just make the options private. They don't need to be public anymore as there is one right answer determined by architecture. Developers will know how to toggle them for testing regardless.

[Michael Catanzaro]

Created attachment 347672 [details]
Patch

[Alberto Garcia]

On a related note, the current code enables JIT by default on MIPS64, and that makes the build fail.

   Source/JavaScriptCore/assembler/MacroAssembler.h:69:2: error: #error "The MacroAssembler is not supported on this platform"

I think this should be enough to fix the problem (I'm currently testing it):

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3fb3dd58a2b..062dcd02d09 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -87,6 +87,8 @@ if (LOWERCASE_CMAKE_SYSTEM_PROCESSOR MATCHES "^arm")
     set(WTF_CPU_ARM 1)
 elseif (LOWERCASE_CMAKE_SYSTEM_PROCESSOR MATCHES "^aarch64")
     set(WTF_CPU_ARM64 1)
+elseif (LOWERCASE_CMAKE_SYSTEM_PROCESSOR MATCHES "^mips64")
+    set(WTF_CPU_MIPS64 1)
 elseif (LOWERCASE_CMAKE_SYSTEM_PROCESSOR MATCHES "^mips")
     set(WTF_CPU_MIPS 1)
 elseif (LOWERCASE_CMAKE_SYSTEM_PROCESSOR MATCHES "(x64|x86_64|amd64)")

[Michael Catanzaro]

(In reply to Alberto Garcia from comment #13)
> On a related note, the current code enables JIT by default on MIPS64, and
> that makes the build fail.
> 
>    Source/JavaScriptCore/assembler/MacroAssembler.h:69:2: error: #error "The
> MacroAssembler is not supported on this platform"
> 
> I think this should be enough to fix the problem (I'm currently testing it):

r=me for this, but please land it in a separate bug.

[EWS Watchlist]

Comment on attachment 347672 [details]
Patch

Attachment 347672 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8947672

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html

[EWS Watchlist]

Created attachment 347834 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Michael Catanzaro]

Ping any reviewers.

[Alberto Garcia]

Related: bug 191941

[Michael Catanzaro]

*** Bug 191941 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

Created attachment 355563 [details]
Patch

[Alberto Garcia]

One problem with this approach is that the "Enabled features" list that cmake prints no longer shows whether the JIT, the sampling profiler or the system malloc are enabled or not.

[Michael Catanzaro]

(In reply to Alberto Garcia from comment #21)
> One problem with this approach is that the "Enabled features" list that
> cmake prints no longer shows whether the JIT, the sampling profiler or the
> system malloc are enabled or not.

That's desired, since WebKit now sets these and users no longer need to deal with them.

[Alberto Garcia]

Fine with me then.

[WebKit Commit Bot]

Comment on attachment 355563 [details]
Patch

Clearing flags on attachment: 355563

Committed r238619: <https://trac.webkit.org/changeset/238619>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.