Bug 65470

Summary: [Qt] libwebcore.a source is compiled without -fvisibility=hidden -fvisibility-inlines-hidden
Product: WebKit Reporter: Holger Freyther <zecke>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, hausmann, laszlo.gombos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 65826, 27551    
Bug Blocks:    
Attachments:
Description Flags
Example to re-enable visibility
none
Reduce symbols (force enabling of export macros)
none
Reduce symbols (force enabling of export macros) [ascii version]
none
Patch for qtwebkit-2.2 none

Description Holger Freyther 2011-08-01 08:04:03 PDT
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
Comment 1 Holger Freyther 2011-08-06 23:56:21 PDT
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
Comment 2 Holger Freyther 2011-08-07 00:37:19 PDT
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.
Comment 3 Holger Freyther 2011-08-07 07:41:14 PDT
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.
Comment 4 Ademar Reis 2011-08-08 11:11:16 PDT
(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.
Comment 5 Ademar Reis 2011-08-08 12:26:15 PDT
(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.
Comment 6 Holger Freyther 2011-08-09 19:41:03 PDT
(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.
Comment 7 Ademar Reis 2011-08-18 06:55:29 PDT
(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?
Comment 8 Holger Freyther 2011-08-18 07:09:21 PDT
(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.
Comment 9 Ademar Reis 2011-08-22 07:36:19 PDT
(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)
Comment 10 Holger Freyther 2011-08-22 10:33:36 PDT
(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.
Comment 11 Ademar Reis 2011-08-26 06:01:43 PDT
Created attachment 105353 [details]
Reduce symbols (force enabling of export macros) [ascii version]

The same patch as before, but without the ansi escape chars.
Comment 12 Ademar Reis 2011-08-26 06:21:21 PDT
Created attachment 105357 [details]
Patch for qtwebkit-2.2
Comment 13 Ademar Reis 2011-08-26 06:22:22 PDT
This appears to be related to this QT bug: https://bugreports.qt.nokia.com/browse/QTBUG-20556
Comment 14 Andreas Kling 2011-08-26 06:31:05 PDT
Comment on attachment 105357 [details]
Patch for qtwebkit-2.2

rs=me for 2.2
Comment 15 Ademar Reis 2011-08-26 08:48:38 PDT
Patch added to qtwebkit-2.2 with commit 16cbdf3 <http://gitorious.org/webkit/qtwebkit/commit/16cbdf3>
Comment 16 Ademar Reis 2011-08-31 07:26:50 PDT
(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).
Comment 17 Ademar Reis 2011-08-31 07:55:23 PDT
(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.
Comment 18 Ademar Reis 2011-08-31 11:43:43 PDT
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.
Comment 19 Holger Freyther 2011-08-31 16:25:09 PDT
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)
Comment 20 Ademar Reis 2011-09-12 08:01:43 PDT
(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).
Comment 21 Ademar Reis 2011-10-03 11:39:34 PDT
Not part of 2.2, so no need to block bug 32653.
Comment 22 Simon Hausmann 2012-01-12 23:51:40 PST
This is fixed in trunk actually with bug #60951 and it's still the case after man y build system changes :)