1019744 - mark all the functions ICU exports as local to libxul

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Trevor Saunders (:tbsaunde)]

doing this shrinks libxul.so by ~1100k on x86_64.

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: mark all of the functions ICU exports as local to libxul

The ICU bit is certainly a hack, but I'm not sure its worth the time to do better.

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8433467 [details] [diff] [review]
mark all of the functions ICU exports as local to libxul

Review of attachment 8433467 [details] [diff] [review]:
-----------------------------------------------------------------

We're defining U_STATIC_IMPLEMENTATION, which should make all symbols not exported... except if the default visibility is not set to hidden. You's rather look at that.

::: toolkit/library/symverscript.in
@@ +2,5 @@
>  @VERSION@ {
>      global: *;
> +local: *_MOZ_ICU_IMPLEMENTATION*;
> + extern "C++" {
> +*std::*;

That's dangerous. There are std symbols that are meant to be common across all DSOs. They are also unrelated to ICU.

Comment 3

[Mike Hommey [:glandium]]

That is, we should probably just pass VISIBILITY_FLAGS down to ICU. (which can be tricky because it uses make variables, so the VISIBILITY_FLAGS definition might need a change)

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #3)
> That is, we should probably just pass VISIBILITY_FLAGS down to ICU. (which
> can be tricky because it uses make variables, so the VISIBILITY_FLAGS
> definition might need a change)

why can't we just add it to CXXFLAGS in icu.m4?

> We're defining U_STATIC_IMPLEMENTATION, which should make all symbols not
> exported... except if the default visibility is not set to hidden. You's
> rather look at that.

lets see how that goes.

> ::: toolkit/library/symverscript.in
> @@ +2,5 @@
> >  @VERSION@ {
> >      global: *;
> > +local: *_MOZ_ICU_IMPLEMENTATION*;
> > + extern "C++" {
> > +*std::*;
> 
> That's dangerous. There are std symbols that are meant to be common across
> all DSOs. They are also unrelated to ICU.

ah, I wasn't aware of that, so it seemed like something worth doing while there.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to Mike Hommey [:glandium] from comment #3)
> > That is, we should probably just pass VISIBILITY_FLAGS down to ICU. (which
> > can be tricky because it uses make variables, so the VISIBILITY_FLAGS
> > definition might need a change)
> 
> why can't we just add it to CXXFLAGS in icu.m4?

ah, actually trying that makes it clear

Comment 6

[Trevor Saunders (:tbsaunde)]

> We're defining U_STATIC_IMPLEMENTATION, which should make all symbols not
> exported... except if the default visibility is not set to hidden. You's
> rather look at that.

actually it looks like ICU is still marking some classes as having default visibility see for example TimeZoneFormat in intl/icu/source/i18n/unicode/tzfmt.h

Comment 7

[Mike Hommey [:glandium]]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> actually it looks like ICU is still marking some classes as having default
> visibility see for example TimeZoneFormat in
> intl/icu/source/i18n/unicode/tzfmt.h

That one is marked U_I18N_API, which is defined to expand to nothing with U_STATIC_IMPLEMENTATION.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > actually it looks like ICU is still marking some classes as having default
> > visibility see for example TimeZoneFormat in
> > intl/icu/source/i18n/unicode/tzfmt.h
> 
> That one is marked U_I18N_API, which is defined to expand to nothing with
> U_STATIC_IMPLEMENTATION.

yeah, aparently we aren't defining U_STATIC_IMPLEMENTATION when we compile ICU itself.  Fixing that does make such classes not have __attribute__((visibility("default"))), but its not enough to make them become hidden.

Comment 9

[Mike Hommey [:glandium]]

Which is why I said we need to pass VISIBILITY_FLAGS down to icu :)

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #9)
> Which is why I said we need to pass VISIBILITY_FLAGS down to icu :)

and I was already doing that :)

Comment 11

[Trevor Saunders (:tbsaunde)]

It also looks like this isn't a system wrapper issue of some sort, I reduced the visibility pragmas in astro.ii where CalendarAstronomer::getJulianCentury() is not hidden to just the #pragma GCC visibility push(hidden) from gcc-hidden.h, so clearly something is marking it default visibility but I can't guess what.

Comment 12

[Trevor Saunders (:tbsaunde)]

actually, apparently I'm just bad and should have clobbered harder.

However there is another issue with adding VISIBILITY_FLAGS to CXXFLAGS when configuring ICU.  At that point we haven't made export in config/ so the system wrappers don't yet exist and ICU's configure's tests end up including gcc-hidden.h  and the actual system headers directly which ends up with link errors because functions in libc are hidden.  Its not immediately obvious that there's a safe point in configure where we can build the system wrappers before configuring ICU because making the system headers depends on variables configure defines.

Comment 13

[Trevor Saunders (:tbsaunde)]

Attachment: make ICU symbols hidden when compiling ICU as static libraries

with this libxul.so is 66,316 k for me on x86_64-linux

Comment 14

[Mike Hommey [:glandium]]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> with this libxul.so is 66,316 k for me on x86_64-linux

Down from?

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8440907 [details] [diff] [review]
make ICU symbols hidden when compiling ICU as static libraries

Review of attachment 8440907 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, let me change that r+ to an f+ with the following request:

::: build/autoconf/icu.m4
@@ +300,5 @@
>          fi
>  
> +        if test "$GNU_CC" -a -z "$MOZ_SHARED_ICU"; then
> +          ICU_CXXFLAGS="$ICU_CXXFLAGS -fvisibility=hidden -DU_STATIC_IMPLEMENTATION"
> +          ICU_CFLAGS="$ICU_CFLAGS -fvisibility=hidden -DU_STATIC_IMPLEMENTATION"

Please make -DU_STATIC_IMPLEMENTATION only depend on -z $MOZ_SHARED_ICU, and -fvisibility=hidden on $GCC_CC. That'll make it work on windows whenever we stop building ICU as a shared library (which may or may not happen, but it's better not to have to care about the extra exported symbols when/if it does).

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Mike Hommey [:glandium] from comment #15)
> Comment on attachment 8440907 [details] [diff] [review]
> make ICU symbols hidden when compiling ICU as static libraries
> 
> Review of attachment 8440907 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Actually, let me change that r+ to an f+ with the following request:
> 
> ::: build/autoconf/icu.m4
> @@ +300,5 @@
> >          fi
> >  
> > +        if test "$GNU_CC" -a -z "$MOZ_SHARED_ICU"; then
> > +          ICU_CXXFLAGS="$ICU_CXXFLAGS -fvisibility=hidden -DU_STATIC_IMPLEMENTATION"
> > +          ICU_CFLAGS="$ICU_CFLAGS -fvisibility=hidden -DU_STATIC_IMPLEMENTATION"
> 
> Please make -DU_STATIC_IMPLEMENTATION only depend on -z $MOZ_SHARED_ICU, and
> -fvisibility=hidden on $GCC_CC. That'll make it work on windows whenever we
> stop building ICU as a shared library (which may or may not happen, but it's
> better not to have to care about the extra exported symbols when/if it does).

sure, though I wonder what mingw does with -fvisibility.


(In reply to Mike Hommey [:glandium] from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #13)
> > with this libxul.so is 66,316 k for me on x86_64-linux
> 
> Down from?

67292k

Comment 17

[Trevor Saunders (:tbsaunde)]

Attachment: make ICU symbols hidden when compiling ICU as static libraries

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8441688 [details] [diff] [review]
make ICU symbols hidden when compiling ICU as static libraries

Review of attachment 8441688 [details] [diff] [review]:
-----------------------------------------------------------------

Jacek, what does mingw gcc do with -fvisibility=hidden?

Comment 19

[Jacek Caban]

(In reply to Mike Hommey [:glandium] from comment #18)
> Jacek, what does mingw gcc do with -fvisibility=hidden?

I quickly tested it and it doesn't seem to have any effect. visibility=hidden is kind of default for win32 builds. I remember problems with dllexport combined with hidden visibility (bug 1008192), but maybe that's only when #pragma is used.

Anyway, it's currently impossible to build ICU with mingw anyway (I looked at this a bit in the past and crosscompilation of ICU is impossible because it uses built tools to build itself, so we'd need two builds to make it work AFAIR), so this patch can't hurt.

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to Jacek Caban from comment #19)
> (In reply to Mike Hommey [:glandium] from comment #18)
> > Jacek, what does mingw gcc do with -fvisibility=hidden?
> 
> I quickly tested it and it doesn't seem to have any effect.
> visibility=hidden is kind of default for win32 builds. I remember problems

yeah, I was mostly wondering if its an error because its an unknown option.

> with dllexport combined with hidden visibility (bug 1008192), but maybe
> that's only when #pragma is used.
> 
> Anyway, it's currently impossible to build ICU with mingw anyway (I looked
> at this a bit in the past and crosscompilation of ICU is impossible because
> it uses built tools to build itself, so we'd need two builds to make it work
> AFAIR), so this patch can't hurt.

I'm not sure if we have cross compilation for android working yet or not, but presumably mingw can use the same setup.

Comment 21

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80a5c49abbc6

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/80a5c49abbc6