Bug 196866

Summary: [WinCairo] Non-unified build fails to link Tools
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: Tools / TestsAssignee: Ross Kirsling <ross.kirsling>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, don.olmstead, Hironori.Fujii, lforschler, stephan.szabo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=200820
Attachments:
Description Flags
Patch none

Description Ross Kirsling 2019-04-12 10:24:24 PDT
As mentioned in bug 196845 comment 2, building WinCairo with clang-cl / lld and -DENABLE_UNIFIED_BUILDS=OFF, we fail to link DumpRenderTree and TestRunnerInjectedBundle.

> lld-link.exe: error: undefined symbol: __imp_?toJS@WebCore@@YA?AVJSValue@JSC@@PEAVExecState@3@PEAVJSDOMGlobalObject@1@AEAVTextTrackCueGeneric@1@@Z
> >>> referenced by C:\GitHub\neko\WebKitBuild\Release\DerivedSources\WebCore\JSInternals.cpp:6803
> >>>               WebCoreTestSupport.lib(JSInternals.cpp.obj):(?jsInternalsPrototypeFunctionCreateGenericCue@WebCore@@YA_JPEAVExecState@JSC@@@Z)
> 
> 
> lld-link.exe: error: undefined symbol: ?notifyNetworkStateChange@ServiceWorkerThreadProxy@WebCore@@QEAAX_N@Z
> >>> referenced by C:\GitHub\neko\WebKitBuild\Release\WTF\Headers\wtf\Function.h:102
> >>>               WebCoreTestSupport.lib(ServiceWorkerInternals.cpp.obj):(?call@?$CallableWrapper@V<lambda_0>@?0??setOnline@ServiceWorkerInternals@WebCore@@QEAAX_N@Z@@?$Function@$$A6AXXZ@WTF@@UEAAXXZ)
> 
> 
> lld-link.exe: error: undefined symbol: __imp_?parseHEVCCodecParameters@WebCore@@YA?AV?$Optional@UHEVCParameterSet@WebCore@@@WTF@@AEBVString@3@@Z
> >>> referenced by C:\GitHub\neko\Source\WebCore\testing\Internals.cpp:4962
> >>>               WebCoreTestSupport.lib(Internals.cpp.obj):(?parseHEVCCodecParameters@Internals@WebCore@@QEAA?AV?$Optional@UHEVCParameterSet@WebCore@@@WTF@@AEBVString@4@@Z)
> 
> 
> lld-link.exe: error: undefined symbol: __imp_?doDispatchMessageOnFrontendPage@InspectorClient@WebCore@@SAXPEAVPage@2@AEBVString@WTF@@@Z
> >>> referenced by C:\GitHub\neko\Source\WebCore\testing\Internals.cpp:372
> >>>               WebCoreTestSupport.lib(Internals.cpp.obj):(?sendMessageToFrontend@InspectorStubFrontend@WebCore@@EEAAXAEBVString@WTF@@@Z)
Comment 1 Don Olmstead 2019-04-12 10:27:42 PDT
(In reply to Ross Kirsling from comment #0)
> As mentioned in bug 196845 comment 2, building WinCairo with clang-cl / lld
> and -DENABLE_UNIFIED_BUILDS=OFF, we fail to link DumpRenderTree and
> TestRunnerInjectedBundle.
> 
> > lld-link.exe: error: undefined symbol: __imp_?toJS@WebCore@@YA?AVJSValue@JSC@@PEAVExecState@3@PEAVJSDOMGlobalObject@1@AEAVTextTrackCueGeneric@1@@Z
> > >>> referenced by C:\GitHub\neko\WebKitBuild\Release\DerivedSources\WebCore\JSInternals.cpp:6803
> > >>>               WebCoreTestSupport.lib(JSInternals.cpp.obj):(?jsInternalsPrototypeFunctionCreateGenericCue@WebCore@@YA_JPEAVExecState@JSC@@@Z)
> > 
> > 
> > lld-link.exe: error: undefined symbol: ?notifyNetworkStateChange@ServiceWorkerThreadProxy@WebCore@@QEAAX_N@Z
> > >>> referenced by C:\GitHub\neko\WebKitBuild\Release\WTF\Headers\wtf\Function.h:102
> > >>>               WebCoreTestSupport.lib(ServiceWorkerInternals.cpp.obj):(?call@?$CallableWrapper@V<lambda_0>@?0??setOnline@ServiceWorkerInternals@WebCore@@QEAAX_N@Z@@?$Function@$$A6AXXZ@WTF@@UEAAXXZ)
> > 
> > 
> > lld-link.exe: error: undefined symbol: __imp_?parseHEVCCodecParameters@WebCore@@YA?AV?$Optional@UHEVCParameterSet@WebCore@@@WTF@@AEBVString@3@@Z
> > >>> referenced by C:\GitHub\neko\Source\WebCore\testing\Internals.cpp:4962
> > >>>               WebCoreTestSupport.lib(Internals.cpp.obj):(?parseHEVCCodecParameters@Internals@WebCore@@QEAA?AV?$Optional@UHEVCParameterSet@WebCore@@@WTF@@AEBVString@4@@Z)
> > 
> > 
> > lld-link.exe: error: undefined symbol: __imp_?doDispatchMessageOnFrontendPage@InspectorClient@WebCore@@SAXPEAVPage@2@AEBVString@WTF@@@Z
> > >>> referenced by C:\GitHub\neko\Source\WebCore\testing\Internals.cpp:372
> > >>>               WebCoreTestSupport.lib(Internals.cpp.obj):(?sendMessageToFrontend@InspectorStubFrontend@WebCore@@EEAAXAEBVString@WTF@@@Z)

This is clang-cl specific right? If so update the description
Comment 2 Ross Kirsling 2019-04-12 10:37:08 PDT
(In reply to Don Olmstead from comment #1)
> This is clang-cl specific right? If so update the description

Not sure, I can try combining regular cl with lld.

The MSVC linker is unable to handle the size of WebCore.lib as mentioned in bug 196762 comment 2.
Comment 3 Ross Kirsling 2019-04-12 11:25:33 PDT
(In reply to Ross Kirsling from comment #2)
> (In reply to Don Olmstead from comment #1)
> > This is clang-cl specific right? If so update the description
> 
> Not sure, I can try combining regular cl with lld.
> 
> The MSVC linker is unable to handle the size of WebCore.lib as mentioned in
> bug 196762 comment 2.

Confirmed that this does not require clang-cl.
Comment 4 Ross Kirsling 2019-04-18 13:57:16 PDT
Evidently the issue here is that certain object files are unused when linking WK[L] and thus their symbols don't get exported.

One fix for this is to add the following line to WebKit[Legacy]/PlatformWin.cmake:
> set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} /WHOLEARCHIVE:WebCore.lib")

But Fujii-san pointed out that we can alternatively tell Cmake to build WebCore as an "object library", and it turns out this makes the non-unified build work with not only lld but also with the MSVC linker!
Comment 5 Ross Kirsling 2019-04-18 14:24:51 PDT
Created attachment 367756 [details]
Patch
Comment 6 Fujii Hironori 2019-04-18 19:01:10 PDT
Comment on attachment 367756 [details]
Patch

I prefer the Object library approach to /WHOLEARCHIVE and --whole-archive switches.
Looks good to me.

I have some questions.

Do you know the reason why GTK port doesn't have the same issue?

According to the document, use need to use $<TARGET_OBJECTS:WebCore> in order to use Object Library. Do you know the reason why you don't need it actually?
https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries
Comment 7 Ross Kirsling 2019-04-18 20:01:25 PDT
(In reply to Fujii Hironori from comment #6)
> Comment on attachment 367756 [details]
> Patch
> 
> I prefer the Object library approach to /WHOLEARCHIVE and --whole-archive
> switches.
> Looks good to me.
> 
> I have some questions.
> 
> Do you know the reason why GTK port doesn't have the same issue?

I'm not 100% certain. Steph suggested it could be a behavioral difference between declspec on Windows and visibility on Unix, but then --whole-archive is originally a GCC option...

In fact, a whole-archived WebCore gets added to WebKitPluginProcess2, though I don't see how this would help TestRunnerInjectedBundle:
https://github.com/WebKit/webkit/blob/master/Source/WebKit/PlatformGTK.cmake#L682

It's interesting that GTK is the only platform on which WebCore is added to WebKitTestRunner_LIBRARIES, but it's not added to WebKitTestRunnerInjectedBundle_LIBRARIES, so that might not matter:
https://github.com/WebKit/webkit/blob/master/Tools/WebKitTestRunner/PlatformGTK.cmake#L27-L35

I now wonder whether WPE has any issues here separate from GTK...

> According to the document, use need to use $<TARGET_OBJECTS:WebCore> in
> order to use Object Library. Do you know the reason why you don't need it
> actually?
> https://cmake.org/cmake/help/latest/command/add_library.html#object-libraries

I believe that syntax is only required when interpolating with the explicit list of items? Notice the second example here:
https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#object-libraries
Comment 8 WebKit Commit Bot 2019-04-18 20:28:40 PDT
Comment on attachment 367756 [details]
Patch

Clearing flags on attachment: 367756

Committed r244448: <https://trac.webkit.org/changeset/244448>
Comment 9 WebKit Commit Bot 2019-04-18 20:28:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-04-18 20:29:17 PDT
<rdar://problem/50038638>
Comment 11 Fujii Hironori 2019-04-18 20:57:46 PDT
Oh. GTK port stopped using -fvisibility=hidden in Bug 181438. All symbols are exported.
Comment 12 Fujii Hironori 2019-04-18 21:16:07 PDT
(In reply to Fujii Hironori from comment #11)
> Oh. GTK port stopped using -fvisibility=hidden in Bug 181438. All symbols
> are exported.

Err. Irrelevant. Ignore my comment.