Hidden visibility is needed for CFI. Clang will only emit CFI checks for a class if it can infer hidden visibility. Michael mentioned that there were some issues hit with GTK and hidden visibility where some static object instances can be duplicated in multiple shared objects https://bugs.webkit.org/show_bug.cgi?id=197920.
Created attachment 370355 [details] WIP Patch Currently compiles and runs gtk layout tests and minibrowser. Only 25% of layout tests are passing with these changes.
I think this should only be a problem for GTK though, because only GTK installs multiple shared libraries (libjavascriptcoregtk for JSC/WTF/bmalloc, and libwebkit2gtk for WebCore/WebKit). Ports that have only a single shared library (like WPE) should be OK with -fvisibility=hidden.
You're going to want to review: https://bugs.webkit.org/show_bug.cgi?id=179914#c49 And bug #182496.
Also https://bugs.webkit.org/show_bug.cgi?id=181438#c40 and r226945. Note, in particular, from bug #179914: """ Adding _ZN7bmalloc10PerProcessINS_12IsoTLSLayoutEE8s_objectE to the global section in the filter file fixes the crash """ To use -fvisibility=hidden, every global template is going to need to be listed in the linker version script. You will need to add a script to the build process to ensure the build fails if any symbols are missed. In bug #179914, Carlos indicates that he had a script that gets you partway there, at least at the time. You might ask him about that. Note there is an extern "C++" syntax to avoid the need to use mangled names in the linker script.
Oh, the script is Tools/Scripts/check-for-global-bss-symbols-in-webkigtk-libs, it already exists. Lucky.
Thanks for those references, they have been helpful. I edited the bss script to show duplicated bss symbols that are marked local in both shared objects. One case of that was g_gigacageBasePtrs was being duplicated in each module which was causing failures in the cases I was looking at (and probably a large chunk of the other failures). I didn't realize bmalloc has its own export macros switch, g_gigacageBasePtrs was supposed to be export but wasn't because BUSE_EXPORT_MACROS was off. I'll upload a new patch with that on. The cases I was looking are now passing in minibrowser but WKTR is now deadlocking. > std::lock_guard<Mutex> lock(Heap::mutex()); > for (unsigned i = numHeaps; i--;) { > if (!isActiveHeapKind(static_cast<HeapKind>(i))) > continue; > PerProcess<PerHeapKind<Heap>>::get()->at(i).scavenge(lock, decommitter, deferredDecommits); The heap mutex is acquired in the lock_guard then acquired again in the PerProcess<>::getSlowCase. It seems similar to the other PerProcess issues that were linked. > $ objdump -t -j .bss -C libjavascriptcoregtk-4.0.so | grep s_object > 0000000002649098 l O .bss 0000000000000008 .hidden bmalloc::PerProcess<bmalloc::PerHeapKind<bmalloc::Heap> >::s_object > $ objdump -t -j .bss -C ../bin/WebKitTestRunner | grep s_object > 0000000000651070 l O .bss 0000000000000008 .hidden bmalloc::PerProcess<bmalloc::PerHeapKind<bmalloc::Heap> >::s_object Interestingly there only seems to be a duplicate instance of it in WKTR. It's not in the minibrowser binary or libwebkit2gtk.so. The other s_object instances that are in both the jsc and wk shared objects are all marked as global with objdump.
Created attachment 370782 [details] WIP Patch Turned on bmalloc exports.
It looks like most of these cases are sorted out if we link WTF and bmalloc static libraries as whole archives when building libjavascriptcoregtk.so then stop linking to those static libraries and just link to libjavascriptcoregtk.so later on. The WKTR PerProcess deadlock was because WKTR was directly linking to libWTFGTK.a in its _LIBRARIES and it was preceding libjavascriptcoregtk.so in the link command so functions were getting linked from there before libjavascriptcoregtk.so. If we stop linking to bmalloc and WTF after building libjavascriptcoregtk.so then most of these issues will become build errors because of missing exports instead of subtle runtime issues. The only instances of duplicated local .bss symbols after doing that is with statics defined in headers. Specifically these `cached` statics are the only duplicated symbols I'm seeing now https://github.com/WebKit/webkit/blob/master/Source/bmalloc/bmalloc/VMAllocate.h#L57. I'll upload my WIP patch with cmake hacked together to stop linking to bmalloc and WTF static libs after libjavascriptcoregtk.so is built. Don was looking at ways to achieve that properly in cmake. I haven't been able to run layout tests on it yet since GTK layout tests are red at the moment.
Created attachment 371357 [details] WIP Patch hacking cmake to stop linking to libWTFGTK.a and libbmalloc.a after building libjavascriptcoregtk.so Note: currently only builds in debug, release might need some more exports
Comment on attachment 371357 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371357&action=review > Source/JavaScriptCore/CMakeLists.txt:125 > + -Wl,--no-whole-archive OBJECT library is better way to do it in CMake. See Bug 196866.
(In reply to Fujii Hironori from comment #10) > Comment on attachment 371357 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371357&action=review > > > Source/JavaScriptCore/CMakeLists.txt:125 > > + -Wl,--no-whole-archive > > OBJECT library is better way to do it in CMake. See Bug 196866. Chris is hitting https://gitlab.kitware.com/cmake/cmake/issues/18010 when using object libraries. Ubuntu comes with 3.10 but the fix came in 3.12
Wow... nice!
Comment on attachment 371357 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371357&action=review > Source/JavaScriptCore/CMakeLists.txt:121 > + -lpthread No doubt this is perfectly idiomatic CMake. Yup. :P > Source/JavaScriptCore/CMakeLists.txt:1317 > +get_target_property(link_props JavaScriptCore INTERFACE_LINK_LIBRARIES) > +message("${link_props}") > +list(REMOVE_ITEM link_props WTF bmalloc -Wl,--whole-archive -Wl,--no-whole-archive) > +message("${link_props}") > +set_target_properties(JavaScriptCore PROPERTIES INTERFACE_LINK_LIBRARIES "${link_props}") Heh, well, awful as this is, I'm quite impressed that you succeeded. I'm excited for -fvisibility=hidden!
(In reply to Michael Catanzaro from comment #13) > Comment on attachment 371357 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371357&action=review > > > Source/JavaScriptCore/CMakeLists.txt:121 > > + -lpthread > > No doubt this is perfectly idiomatic CMake. Yup. :P 🥺 WIP Well get this cleaned up before landing but this should hopefully solve the issue.
The adventure continues in bug #210366.