[GTK] Reenable -fvisibility=hidden

[Michael Catanzaro]

We should reenable -fvisibility=hidden and -fvisibility-hidden-inlines

This is currently broken for GTK; it needs more work to ensure WTF does not get static linked into libwebkit2gtk first. I've so far fixed three different places where this is happening, but I guess there is somewhere else missing.

[Michael Catanzaro]

Created attachment 331882 [details]
Patch

[Michael Catanzaro]

FYI, the backtrace:

#0  0x00007fcc17c76f92 in WTF::RunLoop::TimerBase::TimerBase(WTF::RunLoop&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#1  0x00007fcc17c7640b in WTF::scheduleDispatchFunctionsOnMainThread() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#2  0x00007fcc1a06b885 in WebCore::Document::postTask(WebCore::ScriptExecutionContext::Task&&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#3  0x00007fcc1a3b1cc1 in WebCore::ApplicationCacheGroup::postListenerTask(WTF::AtomicString const&, int, int, WebCore::DocumentLoader&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00007fcc1a3b27dc in WebCore::ApplicationCacheGroup::selectCacheWithoutManifestURL(WebCore::Frame&) [clone .part.528] ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#5  0x00007fcc1a1faaf9 in WebCore::HTMLHtmlElement::insertedByParser() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#6  0x00007fcc1a28bebf in WebCore::HTMLConstructionSite::insertHTMLHtmlStartTagBeforeHTML(WebCore::AtomicHTMLToken&&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#7  0x00007fcc1a29d3da in WebCore::HTMLTreeBuilder::defaultForBeforeHTML() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#8  0x00007fcc1a2adbdb in WebCore::HTMLTreeBuilder::processEndOfFile(WebCore::AtomicHTMLToken&&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#9  0x00007fcc1a2aef0b in WebCore::HTMLTreeBuilder::constructTree(WebCore::AtomicHTMLToken&&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#10 0x00007fcc1a285ade in WebCore::HTMLDocumentParser::constructTreeFromHTMLToken(WebCore::HTMLTokenizer::TokenPtr&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#11 0x00007fcc1a286963 in WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#12 0x00007fcc1a286c9e in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#13 0x00007fcc1a28729b in WebCore::HTMLDocumentParser::prepareToStopParsing()
    ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#14 0x00007fcc1a2873a0 in WebCore::HTMLDocumentParser::finish() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#15 0x00007fcc1a349a3a in WebCore::DocumentWriter::end() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#16 0x00007fcc1a354703 in WebCore::DocumentLoader::finishedLoading() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#17 0x00007fcc1a356a39 in WebCore::DocumentLoader::maybeLoadEmpty() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#18 0x00007fcc1a35837c in WebCore::DocumentLoader::startLoadingMainResource()
    ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#19 0x00007fcc1a3665c3 in WebCore::FrameLoader::init() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#20 0x00007fcc196534cc in WebKit::WebFrame::createWithCoreMainFrame(WebKit::WebPage*, WebCore::Frame*) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#21 0x00007fcc19669d21 in WebKit::WebPage::WebPage(unsigned long, WebKit::WebPageCreationParameters&&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#22 0x00007fcc1966a6be in WebKit::WebPage::create(unsigned long, WebKit::WebPageCreationParameters&&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#23 0x00007fcc195b4808 in WebKit::WebProcess::createWebPage(unsigned long, WebKit::WebPageCreationParameters&&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#24 0x00007fcc1985f225 in void IPC::handleMessage<Messages::WebProcess::CreateWebPage, WebKit::WebProcess, void (WebKit::WebProcess::*)(unsigned long, WebKit::WebPageCreationParameters&&)>(IPC::Decoder&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(unsigned long, WebKit::WebPageCreationParameters&&)) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#25 0x00007fcc1985aa23 in WebKit::WebProcess::didReceiveWebProcessMessage(IPC::Connection&, IPC::Decoder&) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#26 0x00007fcc194329d0 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#27 0x00007fcc19433298 in IPC::Connection::dispatchOneMessage() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#28 0x00007fcc1ac8afb3 in WTF::RunLoop::performWork() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#29 0x00007fcc1ac893b9 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#30 0x00007fcc10b167b5 in g_main_dispatch ()
    at /home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148
#31 g_main_context_dispatch ()
    at /home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813
#32 0x00007fcc10b16b58 in g_main_context_iterate ()
    at /home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3886
#33 0x00007fcc10b16e62 in g_main_loop_run ()
    at /home/mcatanzaro/Projects/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:4082
#34 0x00007fcc1ac89cf8 in WTF::RunLoop::run() ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#35 0x00007fcc197f40e8 in int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) ()
   from /home/mcatanzaro/Projects/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#36 0x00007fcc0c82d00a in __libc_start_main (main=0x400bc0 <main>, argc=3, 
    argv=0x7ffcecda1b18, init=<optimized out>, fini=<optimized out>, 
    rtld_fini=<optimized out>, stack_end=0x7ffcecda1b08)
    at ../csu/libc-start.c:308
#37 0x0000000000400c4a in _start ()

[Michael Catanzaro]

To test it, you need to remove all the ENABLE(DEVELOPER_MODE) conditions from ProcessExecutablePathGtk.cpp, then build with DEVELOPER_MODE disabled and MiniBrowser enabled:

build-webkit --gtk --cmakeargs="-DDEVELOPER_MODE=OFF -DENABLE_MINIBROWSER=ON"

[Don Olmstead]

Created attachment 416033 [details]
WIP Patch

[Don Olmstead]

Created attachment 416040 [details]
WIP Patch

[Don Olmstead]

Comment on attachment 416040 [details]
WIP Patch

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

Here's a patch that builds to let you all experiment with turning hidden visibility back on.

> Source/cmake/OptionsGTK.cmake:36
> +# Hidden visibility requires OBJECT libraries which are not
> +# well supported before 3.12
> +if (${CMAKE_VERSION} VERSION_LESS "3.12.0")
> +    message(WARNING "Using static libraries instead of object libraries. Consider upgrading your CMake version")
> +    set(bmalloc_LIBRARY_TYPE STATIC)
> +    set(WTF_LIBRARY_TYPE STATIC)
> +else ()
> +    SET_AND_EXPOSE_TO_BUILD(BUSE_EXPORT_MACROS ON)
> +    SET_AND_EXPOSE_TO_BUILD(USE_EXPORT_MACROS ON)
> +    set(CMAKE_C_VISIBILITY_PRESET hidden)
> +    set(CMAKE_CXX_VISIBILITY_PRESET hidden)
> +    set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)
> +
> +    set(bmalloc_LIBRARY_TYPE OBJECT)
> +    set(WTF_LIBRARY_TYPE OBJECT)
> +endif ()

For context for this change.

Using OBJECT libraries is the proper way in CMake to accomplish --whole-archive. There was a rather large limitation on OBJECT libraries prior to 3.12 when https://gitlab.kitware.com/cmake/cmake/-/issues/18010 was fixed. Since we support CMake 3.10 the use of OBJECT libraries and turning on hidden visibility is contingent on having CMake 3.12+.

There's probably a way to make this work with 3.10 but I haven't put in the research for it. I'm happy to help someone pull that off if that's required. Its my understanding that 3.12 can be used after April 2021 when more recent Linux releases can be used. So if it proves too much of a pain then it might make sense to just leave the if statement until then.

> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
> +    // I think this needs to be in a WK API
> +    // WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);

I recall there's an issue with exporting this when building the Apple port. Adrian had an idea in https://bugs.webkit.org/show_bug.cgi?id=210366#c73 on how to tackle it.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> We should reenable -fvisibility=hidden and -fvisibility-hidden-inlines

Hm, I think WPE should use -fvisibility-inlines-hidden, but GTK cannot do so safely because it's split into two shared objects. The ports will just have to be a little different.

[Don Olmstead]

Created attachment 419641 [details]
WIP Patch

[Don Olmstead]

Created attachment 419717 [details]
WIP Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Michael Catanzaro from comment #0)
> > We should reenable -fvisibility=hidden and -fvisibility-hidden-inlines
> 
> Hm, I think WPE should use -fvisibility-inlines-hidden, but GTK cannot do so
> safely because it's split into two shared objects. The ports will just have
> to be a little different.

There is another consideration here. Building with -fvisibility=hidden will break Tools/Scripts/check-for-global-bss-symbols-in-webkitgtk-libs. This script is needed to ensure we don't wind up reintroducing bug #179914 again. We should definitely use -fvisibility=hidden in release builds, because it will result in better code generation, but to do this safely, we should find another way to run Tools/Scripts/check-for-global-bss-symbols-in-webkitgtk-libs. Currently it runs during dist. That won't work anymore: it would need to move to an EWS that builds without -fvisibility=hidden. (The script necessarily cannot work properly if WebKit is built with -fvisibility=hidden.)

Again, this only matters for GTK. I think WPE can safely enable -fvisibility=hidden and -fvisibility-inlines-hidden as soon as we're able to make it build successfully.

[Michael Catanzaro]

Comment on attachment 419717 [details]
WIP Patch

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

> Source/cmake/OptionsGTK.cmake:32
> +    set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)

gcc(1) says:

       -fvisibility-inlines-hidden
           This switch declares that the user does not attempt to compare pointers to inline functions or methods
           where the addresses of the two functions are taken in different shared objects.

GTK port, PlayStation port, and other ports that install multiple shared libraries cannot guarantee this, which is why I don't think we should use it. (In contrast, WPE only installs a single shared library, so it should be safe to use in OptionsWPE.cmake.)

The full documentation:

       -fvisibility-inlines-hidden
           This switch declares that the user does not attempt to compare pointers to inline functions or methods
           where the addresses of the two functions are taken in different shared objects.

           The effect of this is that GCC may, effectively, mark inline methods with "__attribute__ ((visibility
           ("hidden")))" so that they do not appear in the export table of a DSO and do not require a PLT
           indirection when used within the DSO.  Enabling this option can have a dramatic effect on load and link
           times of a DSO as it massively reduces the size of the dynamic export table when the library makes heavy
           use of templates.

           The behavior of this switch is not quite the same as marking the methods as hidden directly, because it
           does not affect static variables local to the function or cause the compiler to deduce that the function
           is defined in only one shared object.

           You may mark a method as having a visibility explicitly to negate the effect of the switch for that
           method.  For example, if you do want to compare pointers to a particular inline method, you might mark
           it as having default visibility.  Marking the enclosing class with explicit visibility has no effect.

           Explicitly instantiated inline methods are unaffected by this option as their linkage might otherwise
           cross a shared library boundary.

[Michael Catanzaro]

Created attachment 419741 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #12)
> Created attachment 419741 [details]
> Patch

I fixed the linker error by adding WEBCORE_EXPORT to ~EventTarget and defining it as an empty function in EventTarget.cpp. It cannot be inlined or default or it doesn't link. Yay C++.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #11)
> > Source/cmake/OptionsGTK.cmake:32
> > +    set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)

Oh, but I haven't removed that... I suggest we try removing that next.

[Michael Catanzaro]

Comment on attachment 419741 [details]
Patch

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

> Source/WebCore/dom/EventTarget.cpp:71
> +EventTarget::~EventTarget()
> +{
> +}

Well somehow this causes the Apple EWS to fail to link:

Undefined symbols for architecture x86_64:
  "WebCore::EventTarget::~EventTarget()", referenced from:
      WTF::RefCounted<WebCore::AbortSignal, std::__1::default_delete<WebCore::AbortSignal> >::deref() const in Internals-227951BA24B9AE61.o
      WebCore::AbortSignal::~AbortSignal() in Internals-227951BA24B9AE61.o
      non-virtual thunk to WebCore::AbortSignal::~AbortSignal() in Internals-227951BA24B9AE61.o
      WebCore::AbortSignal::~AbortSignal() in Internals-227951BA24B9AE61.o
      non-virtual thunk to WebCore::AbortSignal::~AbortSignal() in Internals-227951BA24B9AE61.o
ld: symbol(s) not found for architecture x86_64

Yay.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> There is another consideration here. Building with -fvisibility=hidden will
> break Tools/Scripts/check-for-global-bss-symbols-in-webkitgtk-libs. This
> script is needed to ensure we don't wind up reintroducing bug #179914 again.

No, I'm wrong. It won't break anything because symbols that are needed in both libraries would have to be manually exported, otherwise the build would immediately, since they won't be visible anymore.

I'm not sure why I got so confused by this. It should be obviously safe. I think.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #15)
> Well somehow this causes the Apple EWS to fail to link:

I see the GTK EWS fails the same way. Trying to build it locally now, it also fails.

Now when I developed this patch, I tried a bunch of changes until it finally didn't fail to build. Apparently I must have messed up somehow.

[Michael Catanzaro]

Comment on attachment 419741 [details]
Patch

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

> Source/cmake/OptionsGTK.cmake:32
> +    set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)

As mentioned above, I think only WPE can safely use CMAKE_VISIBILITY_INLINES_HIDDEN.

> Tools/WebKitTestRunner/gtk/UIScriptControllerGtk.cpp:81
> +    // I think this needs to be in a WK API
> +    //WebKit::TextChecker::setContinuousSpellCheckingEnabled(enabled);

Yes indeed.

[Michael Catanzaro]

It seems the destructor needs to be exported. I must have somehow convinced myself that it built without the destructor.

[Michael Catanzaro]

Created attachment 422389 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #20)
> Created attachment 422389 [details]
> Patch

This adds WPE port. I didn't test that locally, so we'll see what EWS thinks.

UIScriptControllerGtk::setContinuousSpellCheckingEnabled is going to need a separate patch as that is a bit of a mess.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #21)
> UIScriptControllerGtk::setContinuousSpellCheckingEnabled is going to need a
> separate patch as that is a bit of a mess.

Bug #222818

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #21)
> This adds WPE port. I didn't test that locally, so we'll see what EWS thinks.

Well the WPE EWS is dead currently. Anyway, there should at least be fewer red EWS than with previous iterations.

[Don Olmstead]

Comment on attachment 422389 [details]
Patch

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

> Source/WebCore/dom/EventTarget.cpp:72
> +EventTarget::~EventTarget()
> +{
> +}
> +

You can just do `EventTarget::~EventTarget() = default;` here.

[Michael Catanzaro]

Uhh I found this in Tools/TestWebKitAPI/PlatformGTK.cmake:

# FIXME: Remove when turning on hidden visibility https://bugs.webkit.org/show_bug.cgi?id=181916
list(APPEND TestJavaScriptCore_LIBRARIES WTF)

So we should try dropping that too....

(In reply to Don Olmstead from comment #24)
> You can just do `EventTarget::~EventTarget() = default;` here.

Didn't know that worked.

[Lauro Moura]

(In reply to Michael Catanzaro from comment #23)
> (In reply to Michael Catanzaro from comment #21)
> > This adds WPE port. I didn't test that locally, so we'll see what EWS thinks.
> 
> Well the WPE EWS is dead currently. Anyway, there should at least be fewer
> red EWS than with previous iterations.

There was an issue with the container running it. It's back online now.

[Michael Catanzaro]

Created attachment 422424 [details]
Patch

[Michael Catanzaro]

Created attachment 422426 [details]
Patch

[Michael Catanzaro]

Created attachment 422434 [details]
Patch

[Michael Catanzaro]

Created attachment 422437 [details]
Patch

[Michael Catanzaro]

Created attachment 422441 [details]
Patch

[Michael Catanzaro]

Hmm, there seem to be API test regressions:

Unexpected failures (2)
    /WebKit2Gtk/TestInputMethodContext
        /webkit/WebKitInputMethodContext/surrounding
    /TestJSC
        /jsc/weak-value

[Michael Catanzaro]

Comment on attachment 422441 [details]
Patch

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

> Source/cmake/OptionsWPE.cmake:11
> +set(CMAKE_C_VISIBILITY_PRESET hidden)
> +set(CMAKE_CXX_VISIBILITY_PRESET hidden)
> +set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)

Let's split the WPE changes out to bug #222860. Looks like it's going to require some more work.

[Michael Catanzaro]

Created attachment 422491 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #32)
> Hmm, there seem to be API test regressions:
> 
> Unexpected failures (2)
>     /WebKit2Gtk/TestInputMethodContext
>         /webkit/WebKitInputMethodContext/surrounding

Looks like this test is just flaky. Not our fault.

>     /TestJSC
>         /jsc/weak-value

Seems this one is really somehow broken by hidden visibility. Needs investigation.

[Michael Catanzaro]

This test is flaky. It usually passes for me locally, but sometimes fails. I guess the garbage collection is failing on line 3310 of TestJSC.cpp. The failure case looks like this:

(gdb) bt full
#0  0x00007f31a2a2c9d5 in raise () at /lib64/libc.so.6
#1  0x00007f31a2a158a4 in abort () at /lib64/libc.so.6
#2  0x00007f31a6723b6c in g_assertion_message_expr.cold () at /lib64/libglib-2.0.so.0
#3  0x0000000000412d8d in testJSCWeakValue() ()
    at ../../Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3311
        checker = 
              {m_watchedObjects = {m_impl = {static smallMaxLoadNumerator = <optimized out>, static smallMaxLoadDenominator = <optimized out>, static largeMaxLoadNumerator = <optimized out>, static largeMaxLoadDenominator = <optimized out>, static maxSmallTableCapacity = <optimized out>, static minLoad = <optimized out>, static tableSizeOffset = <optimized out>, static tableSizeMaskOffset = <optimized out>, static keyCountOffset = <optimized out>, static deletedCountOffset = <optimized out>, static metadataSize = <optimized out>, m_table = 0x7f31a1ce94c0}}}
        context = {m_ptr = 0x22b2830}
        weak = {m_ptr = 0x22b7020}
        exceptionHandler = {m_context = 0x22b2830}
        object = {m_ptr = 0x0}
        weakValueCleared = false
        foo = {m_ptr = 0x0}
        weakFoo = {m_ptr = 0x0}
        undefinedValue = {m_ptr = 0x22b0d40}
        __PRETTY_FUNCTION__ = "void testJSCWeakValue()"
#4  0x00007f31a678079e in g_test_run_suite_internal () at /lib64/libglib-2.0.so.0
#5  0x00007f31a678059b in g_test_run_suite_internal () at /lib64/libglib-2.0.so.0
#6  0x00007f31a6780c4b in g_test_run_suite () at /lib64/libglib-2.0.so.0
#7  0x00007f31a6780ca5 in g_test_run () at /lib64/libglib-2.0.so.0
#8  0x0000000000404865 in main(int, char**) (argc=<optimized out>, argv=<optimized out>)
    at ../../Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3904

I'm going to check to see if I can reproduce without hidden visibility, since I can't imagine how this could have broken garbage collection.

[Michael Catanzaro]

OK I don't know how to explain this, but the test always passes without this patch. With this patch, it's flaky. So somehow hidden visibility must have some sort of effect on garbage collection. Weird.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #37)
> OK I don't know how to explain this, but the test always passes without this
> patch. With this patch, it's flaky. So somehow hidden visibility must have
> some sort of effect on garbage collection. Weird.

Well I saw this test fail four times locally this morning after running it maybe 10 times total, but now I have run it over 500 times without a single failure. This is unfortunate. One difference is that this morning I would have been running trunk from a day or two ago with this patch applied, whereas now I have since updated trunk after running 'webkit-patch apply-from-bug 181916'.

Mark Lam had a theory on Slack as to what might be happening with this test:

"""
My theory is that the -fvisibility=hidden now makes it such that some functions get inlined, thereby changing the stack values, which in turn potentially keeps a pointer to a dead object on the stack (though the value is not used).  With a conservative GC, that keeps the object alive.

...

The test is making an assumption that there is no temp on the stack / in a register storing the value of weakFoo

try this: 1. move this code into a function and call it:
        weakFoo = adoptGRef(jsc_weak_value_get_value(weak.get()));
        checker.watch(weakFoo.get());
        g_assert_true(jsc_value_is_object(weakFoo.get()));
        weakFoo = nullptr;

2. call sanitizeStackForVM() after that before you do the GC.

See if that makes the test pass

Actually, move all the code that uses weakFoo into the out of line function

If this makes the test pass, then it confirms my suspicion that the test is just badly written and made assumptions about the GC that it shouldn’t have

Alternatively, you can also do this:

1. Set a breakpoint at 3305 and inspect the value of weakFoo
2. Set a breakpoint at 3310, and check if the value of weakFoo is in any registers or in any stack slots
"""

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #38)
> One difference is that this morning I would
> have been running trunk from a day or two ago with this patch applied,
> whereas now I have since updated trunk after running 'webkit-patch
> apply-from-bug 181916'.

EWS is still consistently failing, so we'll just have to use EWS for this. :S

[Michael Catanzaro]

Created attachment 422617 [details]
Patch

[Mark Lam]

Comment on attachment 422617 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:3278
> +static void useWeakFooInASeparateFunctionToForceGarbageCollection(GRefPtr<JSCValue>&& object, GRefPtr<JSCValue>&& foo, JSCWeakValue* weak, LeakChecker& checker, JSCContext* context, bool& weakValueCleared)

Let's add NEVER_INLINE here.

[Michael Catanzaro]

Created attachment 422618 [details]
Patch

[Michael Catanzaro]

Well Mark, it seems your suggestion actually worked. Wow, thanks.

TODO: I need to rewrite the change to the test to be less ugly, and land it in a separate bug report since it doesn't belong in this one.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #43)
> TODO: I need to rewrite the change to the test to be less ugly, and land it
> in a separate bug report since it doesn't belong in this one.

Let's handle this in bug #222972.

[Michael Catanzaro]

Created attachment 422723 [details]
Patch

[EWS]

Committed r274166: <https://commits.webkit.org/r274166>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422723 [details].

[Tyler Wilcock]

Hi!

I ran a `git bisect` and found this patch broke my local debug build (release is fine).  Can anyone running the GTK port also reproduce this?

Tools/Scripts/build-webkit --gtk --debug

Building flatpak based environment
+  cmake -DPORT="GTK" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Debug -G Ninja -DDEVELOPER_MODE=ON -DENABLE_EXPERIMENTAL_FEATURES=ON "/app/webkit"

...snip...

[3515/3716] Linking CXX executable bin/TestWebKitAPI/TestWebCore
FAILED: bin/TestWebKitAPI/TestWebCore 
: && /usr/bin/c++ -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare  -fno-strict-aliasing -fno-exceptions -fno-rtti -gsplit-dwarf -g -fuse-ld=gold -Wl,--disable-new-dtags -Wl,--gdb-index Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/TestsController.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/WTFStringUtilities.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/AffineTransform.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/CSSParser.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/CalculationValue.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/ColorTests.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/ComplexTextController.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/ContextMenuAction.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/FileMonitor.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/FloatPointTests.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/FloatRectTests.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/FloatSizeTests.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/GridPosition.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/HTMLParserIdioms.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/HTTPParsers.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/IntPointTests.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/IntRectTests.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/IntSizeTests.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/KeyedCoding.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/LayoutUnitTests.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/MIMETypeRegistry.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/ParsedContentRange.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/PublicSuffix.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/SecurityOrigin.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/SharedBuffer.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/SharedBufferTest.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/TimeRanges.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/TransformationMatrix.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/URLParserTextEncoding.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/gtk/main.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/UserAgentQuirks.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/gstreamer/GStreamerTest.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/Tests/WebCore/gstreamer/GstMappedBuffer.cpp.o Tools/TestWebKitAPI/CMakeFiles/TestWebCore.dir/glib/UtilitiesGLib.cpp.o -o bin/TestWebKitAPI/TestWebCore  -Wl,-rpath,/app/webkit/WebKitBuild/Debug/lib  lib/libgtest.so  lib/libWebCoreGTK.a  lib/libPAL.a  /usr/lib/x86_64-linux-gnu/libgtk-3.so  /usr/lib/x86_64-linux-gnu/libgdk-3.so  /usr/lib/x86_64-linux-gnu/libz.so  /usr/lib/x86_64-linux-gnu/libpangocairo-1.0.so  /usr/lib/x86_64-linux-gnu/libpango-1.0.so  /usr/lib/x86_64-linux-gnu/libharfbuzz.so  /usr/lib/x86_64-linux-gnu/libcairo-gobject.so  /usr/lib/x86_64-linux-gnu/libcairo.so  /usr/lib/x86_64-linux-gnu/libgdk_pixbuf-2.0.so  lib/libjavascriptcoregtk-4.0.so.18.19.0  /usr/lib/x86_64-linux-gnu/libicudata.so  /usr/lib/x86_64-linux-gnu/libicui18n.so  /usr/lib/x86_64-linux-gnu/libicuuc.so  -lpthread  /usr/lib/x86_64-linux-gnu/libsystemd.so  /usr/lib/x86_64-linux-gnu/libxml2.so  /usr/lib/x86_64-linux-gnu/libsqlite3.so  /usr/lib/x86_64-linux-gnu/libxslt.so  lib/libANGLE.a  -ldl  /usr/lib/x86_64-linux-gnu/libGL.so  /usr/lib/x86_64-linux-gnu/libEGL.so  /usr/lib/x86_64-linux-gnu/libwoff2dec.so  lib/libxdgmime.a  lib/libwebrtc.a  /usr/lib/x86_64-linux-gnu/libvpx.so  /usr/lib/x86_64-linux-gnu/libevent.so  /usr/lib/x86_64-linux-gnu/libopus.so  /usr/lib/x86_64-linux-gnu/libcairo.so  /usr/lib/x86_64-linux-gnu/libfontconfig.so  /usr/lib/x86_64-linux-gnu/libfreetype.so  /usr/lib/x86_64-linux-gnu/libharfbuzz.so  /usr/lib/x86_64-linux-gnu/libharfbuzz-icu.so  /usr/lib/x86_64-linux-gnu/libgcrypt.so  /usr/lib/x86_64-linux-gnu/libgstapp-1.0.so  /usr/lib/x86_64-linux-gnu/libgstbase-1.0.so  /usr/lib/x86_64-linux-gnu/libgstreamer-1.0.so  /usr/lib/x86_64-linux-gnu/libgstpbutils-1.0.so  /usr/lib/x86_64-linux-gnu/libgstaudio-1.0.so  /usr/lib/x86_64-linux-gnu/libgsttag-1.0.so  /usr/lib/x86_64-linux-gnu/libgstvideo-1.0.so  /usr/lib/x86_64-linux-gnu/libgstgl-1.0.so  /usr/lib/x86_64-linux-gnu/libgstcodecparsers-1.0.so  /usr/lib/x86_64-linux-gnu/libgstfft-1.0.so  /usr/lib/x86_64-linux-gnu/libgcrypt.so  /usr/lib/x86_64-linux-gnu/libgstapp-1.0.so  /usr/lib/x86_64-linux-gnu/libgstbase-1.0.so  /usr/lib/x86_64-linux-gnu/libgstreamer-1.0.so  /usr/lib/x86_64-linux-gnu/libgstpbutils-1.0.so  /usr/lib/x86_64-linux-gnu/libgstaudio-1.0.so  /usr/lib/x86_64-linux-gnu/libgsttag-1.0.so  /usr/lib/x86_64-linux-gnu/libgstvideo-1.0.so  /usr/lib/x86_64-linux-gnu/libgstgl-1.0.so  /usr/lib/x86_64-linux-gnu/libgstcodecparsers-1.0.so  /usr/lib/x86_64-linux-gnu/libgstfft-1.0.so  -lgpg-error  /usr/lib/x86_64-linux-gnu/libjpeg.so  /usr/lib/x86_64-linux-gnu/libpng.so  /usr/lib/x86_64-linux-gnu/libz.so  /usr/lib/x86_64-linux-gnu/libopenjp2.so  /usr/lib/x86_64-linux-gnu/libwebpdemux.so  /usr/lib/x86_64-linux-gnu/libwebp.so  /usr/lib/x86_64-linux-gnu/libsoup-2.4.so  /usr/lib/x86_64-linux-gnu/libatk-1.0.so  /usr/lib/x86_64-linux-gnu/libenchant-2.so  /usr/lib/x86_64-linux-gnu/libgio-2.0.so  /usr/lib/x86_64-linux-gnu/libgmodule-2.0.so  /usr/lib/x86_64-linux-gnu/libgobject-2.0.so  /usr/lib/x86_64-linux-gnu/libglib-2.0.so  -lsecret-1  -lgio-2.0  -lgobject-2.0  -lglib-2.0  /usr/lib/x86_64-linux-gnu/libtasn1.so  /usr/lib/x86_64-linux-gnu/libhyphen.so  /usr/lib/x86_64-linux-gnu/libX11.so  /usr/lib/x86_64-linux-gnu/libXcomposite.so  /usr/lib/x86_64-linux-gnu/libXdamage.so  /usr/lib/x86_64-linux-gnu/libXrender.so  /usr/lib/x86_64-linux-gnu/libXt.so  /usr/lib/x86_64-linux-gnu/libwpe-1.0.so  -lwayland-server  -lwayland-egl  -lwayland-client  /usr/lib/x86_64-linux-gnu/libmanette-0.2.so && :
../../Source/WebCore/platform/glib/SharedBufferGlib.cpp:60: error: undefined reference to 'WTF::FileSystemImpl::filenameForDisplay(WTF::String const&)'
DerivedSources/ForwardingHeaders/JavaScriptCore/IsoHeapCellType.h:33: error: undefined reference to 'vtable for JSC::IsoHeapCellType'
/usr/lib/gcc/x86_64-unknown-linux-gnu/10.2.0/../../../../x86_64-unknown-linux-gnu/bin/ld.gold: the vtable symbol may be undefined because the class is missing its key function
collect2: error: ld returned 1 exit status
[3516/3716] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/WebProcess/WebPage/WebPage.cpp.o
ninja: build stopped: subcommand failed.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 223024

[Diego Pino]

(In reply to Tyler Wilcock from comment #47)
> Hi!
> 
> I ran a `git bisect` and found this patch broke my local debug build
> (release is fine).  Can anyone running the GTK port also reproduce this?
> 

That's correct. GTK-Debug build bot started failing compilation in the range r274162-r274169 (https://build.webkit.org/#/builders/43). I bisected the range and found the regression started happening in r274166 (this patch).

If the issue cannot be fixed soon, we may need to revert the patch.

[Michael Catanzaro]

You need an EWS for it. :P

[Michael Catanzaro]

Created attachment 422836 [details]
Patch

[EWS]

Committed r274216: <https://commits.webkit.org/r274216>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422836 [details].

[Carlos Garcia Campos]

I think this broke the build with ENABLE_TREE_DEBUGGING, but I don't understand why. I'm getting these linking errors:

lib/libWebCoreTestSupport.a(lib/../Source/WebCore/CMakeFiles/WebCoreTestSupport.dir/testing/Internals.cpp.o):Internals.cpp:vtable for WebCore::InlineBox: error: referencia sin definir al «WebCore::InlineBox::outputLineTreeAndMark(WTF::TextStream&, WebCore::InlineBox const*, int) const»
lib/libWebCoreTestSupport.a(lib/../Source/WebCore/CMakeFiles/WebCoreTestSupport.dir/testing/Internals.cpp.o):Internals.cpp:vtable for WebCore::InlineBox: error: referencia sin definir al «WebCore::InlineBox::outputLineBox(WTF::TextStream&, bool, int) const»
lib/libWebCoreTestSupport.a(lib/../Source/WebCore/CMakeFiles/WebCoreTestSupport.dir/testing/Internals.cpp.o):Internals.cpp:vtable for WebCore::InlineBox: error: referencia sin definir al «WebCore::InlineBox::boxName() const»
collect2: error: ld returned 1 exit status

They are fixed by marking them as exported in InlineBox.h, but I don't see where they are used from Internals.cpp.

[Michael Catanzaro]

Just export them and move on. They must be used from somewhere.

[Carlos Alberto Lopez Perez]

(In reply to EWS from comment #52)
> Committed r274216: <https://commits.webkit.org/r274216>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 422836 [details].

This has caused a major regression on GTK, all the javascript-core-tests are crashing due too much memory usage, see bug 229321

[Carlos Alberto Lopez Perez]

Reopening bug because the original commit was reverted on r281333

[Michael Catanzaro]

We need help from JSC experts to investigate this.

And if we can't fix it, then we should remove the split libjavascriptcoregtk.so from the GTK 4 API. This split has very little benefit and is causing too many problems. ;)

[Carlos Alberto Lopez Perez]

It seems enabling -fvisibility=hidden and -fvisibility-hidden-inlines it is an overall 20% performance improvement on the DOM related tests.

You can see here this graphs:

https://perf.webkit.org/v3/#/dashboard/DOM?numberOfDays=90
https://perf.webkit.org/v3/#/charts?since=1608728479871&paneList=((16-6-null-null-(5-2.5-500)

There is a performance improvement on the DOM perf tests when r274166 landed and a performance regression when r281333 landed (the revert of r274166)


To be double sure I built r281332 and r281333 and benchmarked the DOM tests:

https://people.igalia.com/clopez/wkbug/181916/PerformanceTestsResults.html

As you can see, r281333 (which reverts r274166) causes a noticeable performance regressions

[Michael Catanzaro]

Wow.

BTW, for best results, I think it's time for the performance bot to start building with -flto=auto. That could affect things considerably.

(In reply to Carlos Alberto Lopez Perez from comment #58)
> It seems enabling -fvisibility=hidden and -fvisibility-hidden-inlines it is
> an overall 20% performance improvement on the DOM related tests.

Would also be interesting to know the impact of getting rid of the split libjsc.so....

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #59)
> BTW, for best results, I think it's time for the performance bot to start
> building with -flto=auto. That could affect things considerably.

The performance bot runs the builds from the default release build bot (it doesn't build webkit directly, it just reuses the built-products from the release bot).

I think that enabling LTO by default (in CMake release build) can be a very good idea since that will benefit all users. What do you think? Any reason to not enable it by default? I can try to get some perf numbers comparing both builds in other bug.

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #60)
> I think that enabling LTO by default (in CMake release build) can be a very
> good idea since that will benefit all users. What do you think? Any reason
> to not enable it by default? I can try to get some perf numbers comparing
> both builds in other bug.

OK, tangent time!

LTO makes builds take twice as long: effectively, everything gets compiled again during the linking stage, or something like that, more or less. So we do not want LTO when DEVELOPER_MODE is enabled. I think it's OK to enable if build type is Release or RelWithDebInfo and DEVELOPER_MODE is not enabled.

I understand there are some problems with reproducible builds: https://gitlab.com/freedesktop-sdk/freedesktop-sdk/-/issues/1298#note_663774619. But I don't know more about that.

Also there is bug #229867 that I do not know how to fix.

That said, I don't think we necessarily need to do it at the CMake level. I would treat it the same as hardening flags, where we don't do any hardening by default because CMake just doesn't, but all distros know it's required to distribute secure packages and handle it at that level. Each distro is going to make a choice on whether to use LTO for all packages or not, whereas -fvisibility=hidden is the responsibility of individual packages and obviously cannot be set at the distro level. Similarly, the other flags we use are either (a) warning flags, or (b) flags required to the C++ language behavior we want: -fno-strict-aliasing, -fno-exceptions, -fno-rtti. LTO doesn't fit in line with these. So I think my vote would be "don't do it, let distros decide," or to do it at the build-webkit level instead of the CMake level, but that is a lightly-held opinion, definitely not a strong one. If it gets anywhere near LTO's performance benefit, then we should forget about build flag purity and just do it in CMake for those sweet performance wins. :P

[Carlos Garcia Campos]

So, for some reason when using the hidden visibility a lot more memory is used, for example just running jsc it uses around 2MB of memory when not using hidden visibility and 122MB when using it. I'm not sure if it's actually because of the symbol visibilkity or because we build WTF and bmalloc with OBJECT library type, which I don't even know what it is. It doesn't happen when building with hidden visibility and system malloc.

[Carlos Garcia Campos]

I can confirm the problem is not the symbols visibility but the OBJECT library type, using static libs fixes the memory usage issue. I don't know why, though.

[Carlos Garcia Campos]

Created attachment 440201 [details]
Patch

[Carlos Garcia Campos]

API test failure should be fixed by bug #222972

[Michael Catanzaro]

Comment on attachment 440201 [details]
Patch

I'd like Don to look at this.

OBJECT libraries are basically static libraries linked with --whole-archive: https://gitlab.kitware.com/cmake/community/-/wikis/doc/tutorials/Object-Library. So any symbols not needed directly by JSC ought to be discarded if using STATIC rather than OBJECT library type. The point of using OBJECT is to ensure that WTF and bmalloc symbols needed by WebCore and WebKit are not discarded, causing linking to fail. But obviously that is not happening after all. I'm not sure how to explain that. I guess they are getting an entirely separate copy of the WTF/bmalloc libraries? If so, that reminds me of bug #181438 (a consequence of my previous misguided attempt to fix bug #179914).

This all would be much simpler if we didn't have the separate shared libjsc.so. ;)

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #66)
> This all would be much simpler if we didn't have the separate shared
> libjsc.so. ;)

I think we should drop it from the GTK 4 API, and require that applications use the JSCOnly port if they only want JSC. That's what the JSCOnly port exists for, after all! Without split libjsc.so, we no longer have to worry about these weird linking issues that are so difficult to investigate and resolve. We'd also no longer be required to export internal JSC/WTF APIs in libjsc.so.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #66)
> OBJECT libraries are basically static libraries linked with --whole-archive:
> https://gitlab.kitware.com/cmake/community/-/wikis/doc/tutorials/Object-
> Library.

Well, I was just wrong. The very top of the wiki page says:

"CMake generates rules to compile the source files into object files but
does not link archive or link them into a library file. Instead the
object files may be included in other targets created by add_library
or add_executable by listing the object library as a source with
special syntax $<TARGET_OBJECTS:objlib>, where "objlib" is
the object library name."

So... that's clear enough. Using OBJECT causes us to get two separate copies of WTF and bmalloc. And that's a disaster, it's bug #181438 all over again, so we don't want that.

The only reason to use OBJECT was to avoid link failures caused by lack of --whole-archive. But your patch here successfully links without it, so we might as well not worry about it.

I do still want to wait for Don to comment before landing, though.

[Michael Catanzaro]

Comment on attachment 440201 [details]
Patch

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

> ChangeLog:8
> +        Set symbol visibility to hidden, but keep using staic library type for bmalloc and WTF.

static

[Michael Catanzaro]

Comment on attachment 440201 [details]
Patch

Actually I'm removing my r+ because I think this might break the check-for-invalid-symbols-in-version-script safety check that you added in bug #179914. If -fvisibility=hidden is used, then symbols that don't use BEXPORT or WTF_EXPORT_PRIVATE could wind up duplicated, right? That would reintroduce bug #179914?

[Don Olmstead]

Comment on attachment 440201 [details]
Patch

OBJECT libraries is the CMake way of doing --whole-archive. The _FRAMEWORKS macro relies on OBJECT libraries to properly set the libraries.

Why isn't OBJECT being used here?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #68)
> Using OBJECT causes us to get two separate copies
> of WTF and bmalloc. And that's a disaster, it's bug #181438 all over again,
> so we don't want that.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #72)
> (In reply to Michael Catanzaro from comment #68)
> > Using OBJECT causes us to get two separate copies
> > of WTF and bmalloc. And that's a disaster, it's bug #181438 all over again,
> > so we don't want that.

Don is telling me that I have this backwards, but I don't really understand why. Linking is hard.

[Don Olmstead]

(In reply to Michael Catanzaro from comment #73)
> (In reply to Michael Catanzaro from comment #72)
> > (In reply to Michael Catanzaro from comment #68)
> > > Using OBJECT causes us to get two separate copies
> > > of WTF and bmalloc. And that's a disaster, it's bug #181438 all over again,
> > > so we don't want that.
> 
> Don is telling me that I have this backwards, but I don't really understand
> why. Linking is hard.

This whole thing is hard to explain because each port has its own configuration of what is SHARED so regrettably there's magic involved to get things working properly.

The best way to think about it is who is presenting as the library. In this case JavaScriptCore is SHARED and bmalloc and WTF are not. We are packaging bmalloc and WTF into JavaScriptCore and so when WebKit wants to link WTF it needs to be pointed at JavaScriptCore rather than WTF (this prevents the multiple copies bug that mcatanzaro mentions).

The mechanics of the above is done through CMake macros and the setting of _FRAMEWORKS. I can go deeper on what exactly it does to compute it if that's warranted. Its possible there are bugs here so I'm happy to assist but I'm pretty adamant that this is the way forward to make hidden visibility work.

[Carlos Garcia Campos]

(In reply to Don Olmstead from comment #71)
> Comment on attachment 440201 [details]
> Patch
> 
> OBJECT libraries is the CMake way of doing --whole-archive. The _FRAMEWORKS
> macro relies on OBJECT libraries to properly set the libraries.
> 
> Why isn't OBJECT being used here?

Because for some reason it causes jsc to use a lot more memory, making all jsc tests to crash. I don't know what OBJECT exactly does, nor how CMake works, but hidden visibility seems to work with static libraries, and it seems to have an important impact in performance. So, what's the problem of enabling hidden visibility with static libs? I'm fine with moving to OBJECT thing in a follow up if the memory issue is fixed.

[Yusuke Suzuki]

Commented on slack. Using OBJECT library + hidden visibility is probably breaking bmalloc’s static variable uniqueness. As a result, thread local key and bmalloc::Cache is not appropriately instantiated and we are always taking a slow path. This basically means core of bmalloc is messed up and we are leaking all allocations. That’s why we are seeing memory growth: leaking allocations.
I have no idea how to fix it. But look like not using object library will fix the issue. So probably that’s the best approach.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #70)
> Actually I'm removing my r+ because I think this might break the
> check-for-invalid-symbols-in-version-script safety check that you added in
> bug #179914.

Sorry, I copy/pasted the wrong script name. I meant check-for-global-bss-symbols-in-webkigtk-libs. The point of that script is to ensure that we don't break the uniqueness of bmalloc's static global template variables (bug #179914). It's an important safety check, but it only works if we do not use -fvisibility=hidden. The script relies on being able to detect identical symbols that are public in libjsc.so but private in libwk.so, but when using -fvisibility=hidden they're all going to be private.

I actually predicted this problem in comment #10, but then in comment #16 I perhaps-incorrectly convinced myself it would not be a problem after all:

(In reply to Michael Catanzaro from comment #16)
> No, I'm wrong. It won't break anything because symbols that are needed in
> both libraries would have to be manually exported, otherwise the build would
> immediately, since they won't be visible anymore.

That's true when linking static libs (.a), but I suspect it is not true when linking object files (.o).

(In reply to Yusuke Suzuki from comment #76)
> Commented on slack. Using OBJECT library + hidden visibility is probably
> breaking bmalloc’s static variable uniqueness.

I doubt it's breaking *all* static variables, though, because if so it ought to crash immediately (bug #181438), right? Guess: perhaps only static/global templates are broken (bug #179914 redoux). That should *also* crash immediately in production builds, but next guess: we don't notice because our bots use build-webkit, which does DEVELOPER_MODE builds, those do not use the linker version script, sidestepping the problem. (Admittedly, I'm doing a *lot* of guessing there.)

[Michael Catanzaro]

Comment on attachment 440201 [details]
Patch

This patch is missing the change from Tools/TestWebKitAPI/PlatformGTK.cmake:

-# FIXME: Remove when turning on hidden visibility https://bugs.webkit.org/show_bug.cgi?id=181916
-list(APPEND TestJavaScriptCore_LIBRARIES WTF)

[Michael Catanzaro]

Comment on attachment 440201 [details]
Patch

It's also missing the same change from Source/WebCore/PlatformGTK.cmake.

[Carlos Garcia Campos]

I think those changes were required because of the use of OBJECT library type, not the symbols visibility.

[Michael Catanzaro]

Right, but we have to at least remove the FIXMEs so they don't become stale.

[Carlos Garcia Campos]

Or reword them, pointing to a new bug about using object library type if we really want to eventually switch to use it.

[Michael Catanzaro]

The sole purpose of using OBJECT is to facilitate use of -fvisibility=hidden, so if we manage to land this using STATIC instead of OBJECT, I don't think we would.

[Carlos Garcia Campos]

And the only problem of using static libs is the duplicated symbols in libjsc and libwk, right? So, we should check first if that's still a problem for production builds.

[Michael Catanzaro]

If we use:

 * static libs
 * -fvisibility=hidden
 * No -Wl,--whole-archive

as your patch does, then the only way for linking to succeed is if we have duplicate symbols. When libbmalloc and libwtf are linked into libjsc, all symbols not needed by libjsc will be discarded or hidden (because we don't use -Wl,--whole-archive anymore, and your patch does not add it). So without duplication of libbmalloc and libwtf, linking libwebkit would fail because libwebcore and libwebkit need symbols from libbmalloc and libwtf that should have been discarded.

However, I must be incorrect about something. If the above were really true, then we should also see the return of bug #181438. Yet we do not see that.

Also, adding -fvisibility=hidden alone should not be enough to cause duplication where there was none before, except for global templates, like PerProcess<Foo> or PerThread<Foo>. Those I would expect to become duplicated because the mechanism for deduplicating them across shared libs relies on the symbols being exported. But I think everything else would also need to be duplicated to allow linking to succeed. If we were not seeing that before, why does linking succeed now, with no changes to the linking strategy? I do not understand.

One more thing I do not understand: Yusuke has discovered that my patch causes PerThread<T>::getFastCase to *always* return nullptr, and I have just confirmed this. This makes no sense. If it was duplicated across two shared libs, then I would expect we have two PerThread<T>s per thread, but instead we seem to be getting one per call to PerThread<T>::get. I don't know why.

[Michael Catanzaro]

> When libbmalloc and libwtf are linked into libjsc, all symbols not needed by libjsc will be discarded or hidden (because we don't use -Wl,--whole-archive anymore, and your patch does not add it). So without duplication of libbmalloc and libwtf, linking libwebkit would fail because libwebcore and libwebkit need symbols from libbmalloc and libwtf that should have been discarded.

(Note this is not a problem without -fvisibility=hidden, because everything is exported by default and cannot be discarded.)

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #76)
> Commented on slack. Using OBJECT library + hidden visibility is probably
> breaking bmalloc’s static variable uniqueness. As a result, thread local key
> and bmalloc::Cache is not appropriately instantiated and we are always
> taking a slow path.

We have a long thread on Slack where I added a bunch of printfs to PerThread.h to see what it was doing. It actually seems to be working properly.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #87)
> (In reply to Yusuke Suzuki from comment #76)
> > Commented on slack. Using OBJECT library + hidden visibility is probably
> > breaking bmalloc’s static variable uniqueness. As a result, thread local key
> > and bmalloc::Cache is not appropriately instantiated and we are always
> > taking a slow path.
> 
> We have a long thread on Slack where I added a bunch of printfs to
> PerThread.h to see what it was doing. It actually seems to be working
> properly.

What are you using to test it? MiniBrowser? Could you try with the jsc shell? or running jsc tests? I suspect the problem might be the specific to jsc binary (or other binaries that depend on libjsc.so and WTF/bmalloc).

[Carlos Garcia Campos]

Note that when this landed, the layout and unit tests ran just fine, only jsc tests started to crash.

[Carlos Garcia Campos]

I've found the problem. There's another FIXME pointing to this bug in Source/JavaScriptCore/shell/PlatformGTK.cmake that was not addressed in the original commit. That was causing jsc to be linked twice with WTF and bmalloc.

[Carlos Garcia Campos]

Created attachment 440489 [details]
Patch

[Carlos Garcia Campos]

Created attachment 440493 [details]
Patch

Just remove the "no new tests" from the changelog to fix the style

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #88)
> What are you using to test it? MiniBrowser? Could you try with the jsc
> shell? or running jsc tests? I suspect the problem might be the specific to
> jsc binary (or other binaries that depend on libjsc.so and WTF/bmalloc).

Wow, good guess. Nice find.

Ideally we would find a way to assert that we only have a single copy of JSC. But I do not know how to do this. :/

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #77)
> Sorry, I copy/pasted the wrong script name. I meant
> check-for-global-bss-symbols-in-webkigtk-libs. The point of that script is
> to ensure that we don't break the uniqueness of bmalloc's static global
> template variables (bug #179914). It's an important safety check, but it
> only works if we do not use -fvisibility=hidden. The script relies on being
> able to detect identical symbols that are public in libjsc.so but private in
> libwk.so, but when using -fvisibility=hidden they're all going to be private.
> 
> I actually predicted this problem in comment #10, but then in comment #16 I
> perhaps-incorrectly convinced myself it would not be a problem after all:
> 
> (In reply to Michael Catanzaro from comment #16)
> > No, I'm wrong. It won't break anything because symbols that are needed in
> > both libraries would have to be manually exported, otherwise the build would
> > immediately, since they won't be visible anymore.
> 
> That's true when linking static libs (.a), but I suspect it is not true when
> linking object files (.o).

I wonder about this. I suspect non-exported symbols of WTF and bmalloc might now be available to JavaScriptCore. Not sure. Anyway, they will surely NOT be visible to WebKit or WebCore, and that is what matters for the purposes of check-for-global-bss-symbols-in-webkitgtk-libs. So I think we are fine.

[Carlos Garcia Campos]

Committed r283707 (242633@main): <https://commits.webkit.org/242633@main>

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #95)
> Committed r283707 (242633@main): <https://commits.webkit.org/242633@main>

I have discovered an unfortunate side effect of this patch.

backtrace() is not longer able to resolver pointer address to function names. So when you have a call to WTFCrash() or to WTFReportBacktrace() the backtrace it reports on stderr only contains address instead of function names, so the backtrace is pretty much useless :(

[Adrian Perez]

(In reply to Carlos Alberto Lopez Perez from comment #96)
> (In reply to Carlos Garcia Campos from comment #95)
> > Committed r283707 (242633@main): <https://commits.webkit.org/242633@main>
> 
> I have discovered an unfortunate side effect of this patch.
> 
> backtrace() is not longer able to resolver pointer address to function
> names. So when you have a call to WTFCrash() or to WTFReportBacktrace() the
> backtrace it reports on stderr only contains address instead of function
> names, so the backtrace is pretty much useless :(

Even with hidden visibility it is possible to get symbols in backtraces,
but it's indeed trickier and needs something that can actually parse ELF
symbol tables from the loaded files instead of relying on information
provided by the dynamic linker.

I took a quick look at https://github.com/ianlancetaylor/libbacktrace and
it indeed can generate meaningful backtraces when building with “-g”. To
save some space in the generated binaries, an option could be to use “-g1”
(or even “-gline-tables-only”) in combination with “-gz” to compress the
tables -- although in this case we still lose symbol names, but at least
source file names and line numbers are present in the backtraces.

I tried finding if there might be some way of keeping only symbol names
and/or source+line info when using “strip” but didn't manage so far.