Bug 198446 - [PlayStation] Build a shared JavaScriptCore
Summary: [PlayStation] Build a shared JavaScriptCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CMake (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Don Olmstead
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-31 19:28 PDT by Don Olmstead
Modified: 2020-02-05 21:47 PST (History)
18 users (show)

See Also:


Attachments
WIP Patch (3.17 KB, patch)
2019-06-03 09:07 PDT, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (5.91 KB, patch)
2020-02-04 16:39 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (5.91 KB, patch)
2020-02-04 16:42 PST, Don Olmstead
Hironori.Fujii: review-
Details | Formatted Diff | Diff
Patch (6.06 KB, patch)
2020-02-05 15:03 PST, Don Olmstead
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Don Olmstead 2019-05-31 19:28:18 PDT
JSC should be built as a SHARED library.
Comment 1 Don Olmstead 2019-06-03 09:07:38 PDT
Created attachment 371191 [details]
WIP Patch
Comment 2 Michael Catanzaro 2019-06-03 09:10:21 PDT
Comment on attachment 371191 [details]
WIP Patch

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

So this crack works?

> Source/JavaScriptCore/PlatformPlayStation.cmake:41
> +# Both bmalloc and WTF are built as object libraries. Link them into JavaScriptCore
> +# privately so the object files do not propagate.

Otherwise we would wind up with two separate copies of both bmalloc and WTF. Right? Might be worth explaining in the comment?
Comment 3 Don Olmstead 2019-06-03 09:12:26 PDT
(In reply to Michael Catanzaro from comment #2)
> Comment on attachment 371191 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371191&action=review
> 
> So this crack works?
> 
> > Source/JavaScriptCore/PlatformPlayStation.cmake:41
> > +# Both bmalloc and WTF are built as object libraries. Link them into JavaScriptCore
> > +# privately so the object files do not propagate.
> 
> Otherwise we would wind up with two separate copies of both bmalloc and WTF.
> Right? Might be worth explaining in the comment?

This hack does work yes. Chris is trying something similar with GTK. Posting this now to just give you an idea on what is going on. We'll land this eventually.
Comment 4 Don Olmstead 2020-02-04 16:39:54 PST
Created attachment 389741 [details]
Patch
Comment 5 Don Olmstead 2020-02-04 16:42:05 PST
Created attachment 389742 [details]
Patch
Comment 6 Stephan Szabo 2020-02-04 18:30:17 PST
Comment on attachment 389742 [details]
Patch

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

> Source/JavaScriptCore/PlatformPlayStation.cmake:60
> +    STATICALLY_LINKED_WITH_WTF

It seems like we probably should have STATICALLY_LINKED_WITH_bmalloc as well since Heap.cpp can include bmalloc headers depending on USE() settings.
Comment 7 Don Olmstead 2020-02-04 18:35:40 PST
Comment on attachment 389742 [details]
Patch

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

>> Source/JavaScriptCore/PlatformPlayStation.cmake:60
>> +    STATICALLY_LINKED_WITH_WTF
> 
> It seems like we probably should have STATICALLY_LINKED_WITH_bmalloc as well since Heap.cpp can include bmalloc headers depending on USE() settings.

This actually gets propagated from the WTF_PRIVATE_DEFINITIONS.

Once we get a handle on PUBLIC/PRIVATE with WebKit library dependencies I'd push this STATICALLY_LINKED_WITH_${target} into the WebKit framework macros.
Comment 8 Fujii Hironori 2020-02-04 20:53:44 PST
Comment on attachment 389742 [details]
Patch

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

>>> Source/JavaScriptCore/PlatformPlayStation.cmake:60
>>> +    STATICALLY_LINKED_WITH_WTF
>> 
>> It seems like we probably should have STATICALLY_LINKED_WITH_bmalloc as well since Heap.cpp can include bmalloc headers depending on USE() settings.
> 
> This actually gets propagated from the WTF_PRIVATE_DEFINITIONS.
> 
> Once we get a handle on PUBLIC/PRIVATE with WebKit library dependencies I'd push this STATICALLY_LINKED_WITH_${target} into the WebKit framework macros.

Weird. WTF_PRIVATE_DEFINITIONS shouldn't be propagated because it is PRIVATE.

> Tools/TestWebKitAPI/PlatformPlayStation.cmake:19
> +)

You don't need to use TARGET_OBJECTS.
See the second example of the document.
https://cmake.org/cmake/help/v3.16/manual/cmake-buildsystem.7.html#object-libraries

Actually, WinCairo port is using OBJECT library for WebCore, but not using TARGET_OBJECTS for it.
Comment 9 Don Olmstead 2020-02-05 14:57:01 PST
Comment on attachment 389742 [details]
Patch

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

>>>> Source/JavaScriptCore/PlatformPlayStation.cmake:60
>>>> +    STATICALLY_LINKED_WITH_WTF
>>> 
>>> It seems like we probably should have STATICALLY_LINKED_WITH_bmalloc as well since Heap.cpp can include bmalloc headers depending on USE() settings.
>> 
>> This actually gets propagated from the WTF_PRIVATE_DEFINITIONS.
>> 
>> Once we get a handle on PUBLIC/PRIVATE with WebKit library dependencies I'd push this STATICALLY_LINKED_WITH_${target} into the WebKit framework macros.
> 
> Weird. WTF_PRIVATE_DEFINITIONS shouldn't be propagated because it is PRIVATE.

I'll add it its not like it hurts anything. CMake can be weird.

>> Tools/TestWebKitAPI/PlatformPlayStation.cmake:19
>> +)
> 
> You don't need to use TARGET_OBJECTS.
> See the second example of the document.
> https://cmake.org/cmake/help/v3.16/manual/cmake-buildsystem.7.html#object-libraries
> 
> Actually, WinCairo port is using OBJECT library for WebCore, but not using TARGET_OBJECTS for it.

There is not currently a WebKit::WebCore interface target. WebCore is used directly. That works fine which as WinCairo shows. In this case there are WebKit::WTF and WebKit::bmalloc interface targets that depend on the OBJECT library and the TARGET_OBJECTS does not propagate to targets that depend on the interface target. This can be fixed by using $<TARGET_OBJECTS> directly when defining ${target}_INTERFACE_LIBRARIES if an OBJECT library is being used.

However the CMake is not in a great state when it comes to properly using PUBLIC and PRIVATE. There's also a lot of difference between the ports for how they present different libraries within the project. Windows does everything shared except WebCore and PAL, Mac does JavaScriptCore WebCore and WebKit as shared, WPE only has WebKit as shared. This really needs to be tamed before any of the logic for OBJECT libraries get wrapped into WEBKIT_FRAMEWORK. So setting the $<TARGET_OBJECTS> in the JavaScriptCore Platform file makes it so we don't have any problems with a SHARED JavaScriptCore with multiply defined symbols. I agree its not ideal but this is a workaround for the current state of the build system.
Comment 10 Don Olmstead 2020-02-05 15:03:21 PST
Created attachment 389877 [details]
Patch

Address feedback
Comment 11 Michael Catanzaro 2020-02-05 15:27:33 PST
The port-specific differences regarding which libraries are shared and which are static are not really "tamable" in the sense that we don't want to change them. GTK needs JSC to be shared because it's a separate platform library. WPE doesn't expose it, so no need for that. macOS still supports WebKit1 so WebCore has to be sharde there, but we wouldn't that to be shared on Linux. Generally I'd recommend building as much as possible as a static lib, to minimize code size and maximize performance.

If PlayStation doesn't have a really good reason to use shared JSC, I wouldn't. It makes linking a lot harder.
Comment 12 Don Olmstead 2020-02-05 15:30:34 PST
(In reply to Michael Catanzaro from comment #11)
> The port-specific differences regarding which libraries are shared and which
> are static are not really "tamable" in the sense that we don't want to
> change them. GTK needs JSC to be shared because it's a separate platform
> library. WPE doesn't expose it, so no need for that. macOS still supports
> WebKit1 so WebCore has to be sharde there, but we wouldn't that to be shared
> on Linux. Generally I'd recommend building as much as possible as a static
> lib, to minimize code size and maximize performance.
> 
> If PlayStation doesn't have a really good reason to use shared JSC, I
> wouldn't. It makes linking a lot harder.

We have a good reason for wanting a shared JavaScriptCore 🤐
Comment 13 Fujii Hironori 2020-02-05 20:49:43 PST
Comment on attachment 389877 [details]
Patch

LGTM
Comment 14 WebKit Commit Bot 2020-02-05 21:47:00 PST
Comment on attachment 389877 [details]
Patch

Clearing flags on attachment: 389877

Committed r255905: <https://trac.webkit.org/changeset/255905>
Comment 15 WebKit Commit Bot 2020-02-05 21:47:02 PST
All reviewed patches have been landed.  Closing bug.