Closed Bug 1019744 Opened 5 years ago Closed 5 years ago

mark all the functions ICU exports as local to libxul

Categories

(Firefox Build System :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla33

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(1 file, 2 obsolete files)

doing this shrinks libxul.so by ~1100k on x86_64.
The ICU bit is certainly a hack, but I'm not sure its worth the time to do better.
Attachment #8433467 - Flags: review?(mh+mozilla)
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.
Attachment #8433467 - Flags: review?(mh+mozilla) → review-
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)
(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.
(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
> 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
(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.
(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.
Which is why I said we need to pass VISIBILITY_FLAGS down to icu :)
(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 :)
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.
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.
with this libxul.so is 66,316 k for me on x86_64-linux
Attachment #8433467 - Attachment is obsolete: true
Attachment #8440907 - Flags: review?(mh+mozilla)
Attachment #8440907 - Flags: review?(mh+mozilla) → review+
(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 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).
Attachment #8440907 - Flags: review+ → feedback+
(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
Attachment #8440907 - Attachment is obsolete: true
Attachment #8441688 - Flags: review?(mh+mozilla)
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?
Attachment #8441688 - Flags: review?(mh+mozilla)
Attachment #8441688 - Flags: review+
Attachment #8441688 - Flags: feedback?(jacek)
(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.
Attachment #8441688 - Flags: feedback?(jacek) → feedback+
(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.
https://hg.mozilla.org/mozilla-central/rev/80a5c49abbc6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.