Bug 65470 – [Qt] libwebcore.a source is compiled without -fvisibility=hidden -fvisibility-inlines-hidden

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Holger Freyther]

This was done due some linking issues with, the support for this was removed in r79320. The reduction of exports should be enabled again.


ARM:
23M 2011-08-01 12:19 QtWebKit.arm.full
19M 2011-08-01 12:55 QtWebKit.arm.full.visibility


MIPS(o32):
17M 2011-08-01 07:00 QtWebKit.mips.minimal.nosvg.Os
13M 2011-08-01 07:00 QtWebKit.mips.minimal.nosvg.Os.visibility

[Holger Freyther]

Created attachment 103173 [details]
Example to re-enable visibility

This patch depends on #27551 introducing USE(EXPORT_MACROS) which would fix this but would normally also export more symbols than we want/need for Qt.

X86 build (using -2 on the build)
35499072 2011-08-07 04:38 libQtWebKit.so.4.10.0.before
29595673 2011-08-07 06:52 lib/libQtWebKit.so.4.10.0.after

[Holger Freyther]

I think this applies to QtWebKit2.2 as well and I wonder if this is a show stopper for the release. I'm not sure which parts of WebKit2 (Testing utilities) link against libQtWebKit.so (instead of libWebCore directly).

I am not really sure how to move forward for QtWebKit2.2, for trunk it is probably waiting for bug #27551.

[Holger Freyther]

Created attachment 103181 [details]
Reduce symbols (force enabling of export macros)

X86:
29616930 2011-08-07 07:48 lib/libQtWebKit.so.4.10.0

this patch requires the patch from bug #65826.

[Ademar Reis]

(In reply to comment #3)
> Created an attachment (id=103181) [details]
> Reduce symbols (force enabling of export macros)
> 
> X86:
> 29616930 2011-08-07 07:48 lib/libQtWebKit.so.4.10.0
> 
> this patch requires the patch from bug #65826.

There's something wrong with this patch, it's not ASCII/cleartext, it contains lots of escape sequences.

[Ademar Reis]

(In reply to comment #2)
> I think this applies to QtWebKit2.2 as well and I wonder if this is a show stopper for the release. I'm not sure which parts of WebKit2 (Testing utilities) link against libQtWebKit.so (instead of libWebCore directly).
> 

Regarding QtWebKit-2.2, your last patch appears to work and indeed reduces the size of the library (webkit1 only, linux, x86, release):

27M     libQtWebKit.so.4.9.0 (no-visibility)
31M     libQtWebKit.so.4.9.0 (full)

> I am not really sure how to move forward for QtWebKit2.2, for trunk it is probably waiting for bug #27551.

Since WebKit2 is not supported in QtWebKit-2.2, I think just the patch from this bug should suffice, but I may be missing something. I'm adding it to the "nice-to have" meta bug for now.

[Holger Freyther]

(In reply to comment #4)

> There's something wrong with this patch, it's not ASCII/cleartext, it contains lots of escape sequences.

Oh sorry this is coming from git diff > /tmp/diff. I can create a patch with the escape sequences removed.

[Ademar Reis]

(In reply to comment #1)
> Created an attachment (id=103173) [details]
> Example to re-enable visibility
> 
> This patch depends on #27551 introducing USE(EXPORT_MACROS) which would fix this but would normally also export more symbols than we want/need for Qt.

Holger: The latest patch attached to bug 27551 doesn't contain any references to EXPORT_MACROS. Are you sure it's still needed? Regarding WebKit1 (QtWebKit-2.2), would it make sense to land your patch AS IS?

[Holger Freyther]

(In reply to comment #7)
> (In reply to comment #1)
> > Created an attachment (id=103173) [details] [details]
> > Example to re-enable visibility
> > 
> > This patch depends on #27551 introducing USE(EXPORT_MACROS) which would fix this but would normally also export more symbols than we want/need for Qt.
> 
> Holger: The latest patch attached to bug 27551 doesn't contain any references to EXPORT_MACROS. Are you sure it's still needed? Regarding WebKit1 (QtWebKit-2.2), would it make sense to land your patch AS IS?

Export macros landed as part of r81135 but are turned off on the transition phase.

[Ademar Reis]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #1)
> > > Created an attachment (id=103173) [details] [details] [details]
> > > Example to re-enable visibility
> > > 
> > > This patch depends on #27551 introducing USE(EXPORT_MACROS) which would fix this but would normally also export more symbols than we want/need for Qt.
> > 
> > Holger: The latest patch attached to bug 27551 doesn't contain any references to EXPORT_MACROS. Are you sure it's still needed? Regarding WebKit1 (QtWebKit-2.2), would it make sense to land your patch AS IS?
> 
> Export macros landed as part of r81135 but are turned off on the transition phase.

So my understanding is that the only thing currently blocking this for trunk is bug 65826 (which is WK2 related). In other words, it could be applied to 2.2 AS IS, right?

(I'm not saying I want to apply it, just asking if there's anything missing)

[Holger Freyther]

(In reply to comment #9)

> So my understanding is that the only thing currently blocking this for trunk is bug 65826 (which is WK2 related). In other words, it could be applied to 2.2 AS IS, right?
> 
> (I'm not saying I want to apply it, just asking if there's anything missing)

Yes, it could be applied as is.

[Ademar Reis]

Created attachment 105353 [details]
Reduce symbols (force enabling of export macros) [ascii version]

The same patch as before, but without the ansi escape chars.

[Ademar Reis]

Created attachment 105357 [details]
Patch for qtwebkit-2.2

[Ademar Reis]

This appears to be related to this QT bug: https://bugreports.qt.nokia.com/browse/QTBUG-20556

[Andreas Kling]

Comment on attachment 105357 [details]
Patch for qtwebkit-2.2

rs=me for 2.2

[Ademar Reis]

Patch added to qtwebkit-2.2 with commit 16cbdf3 <http://gitorious.org/webkit/qtwebkit/commit/16cbdf3>

[Ademar Reis]

(In reply to comment #15)
> Patch added to qtwebkit-2.2 with commit 16cbdf3 <http://gitorious.org/webkit/qtwebkit/commit/16cbdf3>

Unfortunately the patch broke most plugin layout tests:
http://build.webkit.sed.hu/results/QtWebKit2.2-branch%20x86-32%20Linux%20Release%20Qt%204.8.x/rfb3b4700aa5abbb2db471b4f0eb7946ce4850cc4%20%28184%29/results.html

After a few tests, I figured this is the culprit:

WebKit.pro:
contains(QT_CONFIG, reduce_exports):CONFIG += hide_symbols

Any objections to the removal of this line but keeping the rest of the patch?

(BTW, I couldn't find documentation about this CONFIG option... any pointers are welcome).

[Ademar Reis]

(In reply to comment #16)
> (In reply to comment #15)
> > Patch added to qtwebkit-2.2 with commit 16cbdf3 <http://gitorious.org/webkit/qtwebkit/commit/16cbdf3>
> 
> Unfortunately the patch broke most plugin layout tests:
> http://build.webkit.sed.hu/results/QtWebKit2.2-branch%20x86-32%20Linux%20Release%20Qt%204.8.x/rfb3b4700aa5abbb2db471b4f0eb7946ce4850cc4%20%28184%29/results.html
> 
> After a few tests, I figured this is the culprit:
> 
> WebKit.pro:
> contains(QT_CONFIG, reduce_exports):CONFIG += hide_symbols
> 
> Any objections to the removal of this line but keeping the rest of the patch?

BTW, as far as library size is concerned, there's no advantage in keeping the other changes. Withoug hide_symbols, the library size ends up the same as before the patch was applied.

[Ademar Reis]

Since the original change broke several tests and after removing CONFIG += hide_symbols (that fixes the tests) we're back to the original library size, I'm reverting the whole patch in 2.2 (I need the bots green).

If it makes sense to add a partial patch, please let me know.

[Holger Freyther]

Okay, I am sadly on vacation with a netbook. I will try to propose another patch for webkit 2.2.x.


doc:
It is complicated. It starts with the Qt config tests for the visibility that add reduce symbols to the IIRC features.pri (just grep inside the QMAKESPEC directory). The other part is the magic of mapping the Qt config option to a NAME.prf (e.g common/features/unix/hide_symbols.prf)

[Ademar Reis]

(In reply to comment #19)
> Okay, I am sadly on vacation with a netbook. I will try to propose another patch for webkit 2.2.x.
> 

Just a heads up: QtWebKit-2.2-RC1 is planned for this week (today or tomorrow).

[Ademar Reis]

Not part of 2.2, so no need to block bug 32653.

[Simon Hausmann]

This is fixed in trunk actually with bug #60951 and it's still the case after man y build system changes :)