273336 - Make all symbols have hidden visibility by default

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Brian Ryner (not reading)]

The CVS version of gcc (to become 4.0) has some new features releating to symbol
visibility (also backported to Red Hat's 3.4 branch for FC3).  You can use
"#pragma GCC visibility push(foo)" to set the default visibility for all
declarations up to "#pragma GCC visibility pop".  By putting this in a file that
we give with -include, we can mark all of our symbols as hidden by default, and
use the existing NS_EXPORT declarations to note the exported functions / data. 
This is different from what the new -fvisibility=hidden switch does; that only
affects visibility for symbols that are defined in the same translation unit as
the caller.

To make this work, we have to do some special tricks relating to system headers
that we use.  Since Linux system headers do not explicitly say
"visibility(default)" on function declarations, they would be treated as hidden
and a relative relocation would be incorrectly generated.  One way to deal with
this, which isn't all that intrusive, is to wrap all of the system headers we
use by inserting our own header before it in the search path.  For example, for
<foo.h>, we would have our own foo.h that looks like this:

#pragma GCC visibility push(default)
#include_next <foo.h>
#pragma GCC visibility pop

(#include_next is a gcc extension to the language)

Using these techniques, we save about 150 KB of code size on a non-static build
of Firefox trunk (on Fedora Core 3) and we also get 4-5% faster on the page load
test.  Minimal changes are required to existing code, mostly just adding export
macros to unix-specific code that did not need it before.  All of our
cross-platform code is already marked up since we need dllexport on Windows.

One unfortunate bug I found with this is that if you do:

#pragma GCC visibility push(hidden)
class __attribute__((visibility ("default"))) Foo {
  static int bar;
};

Foo::bar will be marked with hidden visibility instead of exported.  We can get
around that by explicitly marking it as exported, though we have to make sure
_not_ to mark it dllexport on Windows if the class is already marked dllexport
(doing so is a compile error).

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Tested the following builds with this patch:

Linux (FC3):
firefox optimized gtk2+xft
seamonkey debug extensions=all gtk1

Windows (VC6):
firefox optimized

Comment 2

[Benjamin Smedberg]

Comment on attachment 167992 [details] [diff] [review]
patch

1) "mkdir -p" is not portable, use nsinstall -D unless we need these headers
before we build nsinstall, in which case perl should be taught to handle it.

2) Do we really need all the windows headers? Doesn't mingw use
dllimport/export?

3) Why the changes to nsMsgDBFolder.h ?

4) Does the NSPR build currently depend on perl?

5) Is nsComponentManagerLog exported for
xpcom/obsolete/component/nsRegistry.cpp ? If so, we should just fix nsRegistry
to use a different PRLog.

Comment 3

[Brian Ryner (not reading)]

(In reply to comment #2)
> (From update of attachment 167992 [details] [diff] [review])
> 1) "mkdir -p" is not portable, use nsinstall -D unless we need these headers
> before we build nsinstall, in which case perl should be taught to handle it.

We do, since we wrap some headers that nsinstall.c / pathsub.c include.  I
believe mkdir -p is portable enough for the platforms that will be using this
optimization.

> 
> 2) Do we really need all the windows headers? Doesn't mingw use
> dllimport/export?
> 

The list of headers was automatically generated by using perl to look for angle
bracket includes.  These extra headers don't really hurt anything (a mingw
windows build should not be using this optimization).

> 3) Why the changes to nsMsgDBFolder.h ?

See the part of the initial comment where I mentioned the bug with exporting
static members of a class.

> 
> 4) Does the NSPR build currently depend on perl?
> 

It appears to, yes.  Besides, who doesn't have perl?

> 5) Is nsComponentManagerLog exported for
> xpcom/obsolete/component/nsRegistry.cpp ? If so, we should just fix nsRegistry
> to use a different PRLog.
> 

Yes, that's why it's being exported.  I didn't want to break anyone's logging.

Comment 4

[Benjamin Smedberg]

> See the part of the initial comment where I mentioned the bug with exporting
> static members of a class.

Oh, I missed the fact that this was NS_MSG_BASE.

I'm willing to be r=bsmedberg on this.

Comment 5

[Brian Ryner (not reading)]

cc wan-teh in case he has comments

Comment 6

[Wan-Teh Chang]

Comment on attachment 167992 [details] [diff] [review]
patch

Brian, one thing in this patch that really bothers
me is the need to wrap system headers.	To me, it
shows that this compiler feature is not ready for
general consumption.

The system-headers files will be a maintenance
burden because they need to be updated when a new
system header is included by our source files.

I think we should wait until this feature can be
controlled by a compiler flag to use this feature.

Is it possible to exclude NSPR from this patch?

Comment 7

[Darin Fisher]

Comment on attachment 167992 [details] [diff] [review]
patch

netCore.h is not a system header.  it's part of necko.	nsAString.h is also not
a system header.  the list goes on...  there's lots of stuff in
config/system-headers that shouldn't be there.


NS_EXPORT_ seems like the wrong name for this macro since NS_EXPORT has
traditionally resolved to __declspec(dllexport) on windows.  it doesn't look
like you are changing that.  perhaps some other name would be better, given
that we have NSPR_API, NS_COM, NS_COM_GLUE, NS_COM_OBSOLETE, NS_MSG_BASE,
NS_STRINGAPI, etc. already for this sort of thing.


we should file a bug against GCC on the static member thing.


nspr's system-headers file also mistakenly references nspr's own headers.

Comment 8

[Brian Ryner (not reading)]

(In reply to comment #6)
> (From update of attachment 167992 [details] [diff] [review])
> Brian, one thing in this patch that really bothers
> me is the need to wrap system headers.	To me, it
> shows that this compiler feature is not ready for
> general consumption.
> 
> The system-headers files will be a maintenance
> burden because they need to be updated when a new
> system header is included by our source files.
> 
> I think we should wait until this feature can be
> controlled by a compiler flag to use this feature.
> 
> Is it possible to exclude NSPR from this patch?
> 

I'd rather get a win now than wait for something that may never happen and is
out of our control.  I don't believe the system headers file will be a huge
burden; new system headers aren't getting included all that often and it really
only matters for Linux.  I can exclude NSPR if you'd like but I really think
this is a valuable win.

(In reply to comment #7)
> (From update of attachment 167992 [details] [diff] [review])
> netCore.h is not a system header.  it's part of necko.	nsAString.h is also not
> a system header.  the list goes on...  there's lots of stuff in
> config/system-headers that shouldn't be there.
> 

Ok, as I said, I just generated that with a script (and then I added the NSS
headers by hand).  We can remove those files if you want, though if they only
declare exported functions it doesn't make any difference.

> 
> NS_EXPORT_ seems like the wrong name for this macro since NS_EXPORT has
> traditionally resolved to __declspec(dllexport) on windows.  it doesn't look
> like you are changing that.  perhaps some other name would be better, given
> that we have NSPR_API, NS_COM, NS_COM_GLUE, NS_COM_OBSOLETE, NS_MSG_BASE,
> NS_STRINGAPI, etc. already for this sort of thing.
> 

The list of macros you gave are all wired up to become NS_EXPORT when compiling
the library that defines the symbol, ans NS_IMPORT from outside the library.  We
could do that for some of the places that I used NS_EXPORT_ directly, but my
feeling is that it just amounts to extra cruft in the headers since export ==
import on Linux.

> 
> we should file a bug against GCC on the static member thing.
> 

Yep.

> 
> nspr's system-headers file also mistakenly references nspr's own headers.
> 

Same answer as above; we should remove these but it probably won't make any
difference.

Comment 9

[Darin Fisher]

> Ok, as I said, I just generated that with a script (and then I added the NSS
> headers by hand).  We can remove those files if you want, though if they only
> declare exported functions it doesn't make any difference.

Ok, yeah... makes sense.  It doesn't really matter then.


> could do that for some of the places that I used NS_EXPORT_ directly, but my
> feeling is that it just amounts to extra cruft in the headers since export ==
> import on Linux.

well... my point really was that export != import on windows, so it is a little
confusing to use NS_EXPORT in both the import and export cases (even if only on
code that is XP_UNIX).  anyways, gtkmozembed is built and used on windows w/ the
GTK windows port (despite the fact that it has bugs there), so it'd probably be
nice not to cause pain for those folks using it on windows.

i know its a bit more work to come up with a unique macro for each DSO that can
be used to expand to either NS_EXPORT or NS_IMPORT, but i think it's worth it in
the long run since it may make life easier in the future if we ever run into a
dllexport/dllimport-type build scenario that we don't anticipate now.

Comment 10

[Brian Ryner (not reading)]

Attachment: revised patch

This addresses Darin's comments. I removed internal headers from system-headers
for both NSPR and Mozilla, and got rid of hardcoded NS_EXPORT ustage.  I'm
leaving NSPR in this patch because I'm still hoping I can convince Wan-teh that
this is a good idea. :)

Comment 11

[Brian Ryner (not reading)]

Attachment: fix autoconf checks

The previous patches had a problem with the autoconf checks.  The check for the
result of AC_CACHE_VAL needs to be outside of the AC_CACHE_VAL, otherwise the
variables will not be set if the cached value is used.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 168342 [details] [diff] [review]
fix autoconf checks

>+sys/stat.h
>+sys\stat.h
...
>+sys/types.h
>+sys\types.h

The backslash versions should be removed.

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I wonder whether the autoconf test could actually work on any non-x86 platforms.
 If not, is there a chance it could give false positives?  Might it be worth
making it x86-only?

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 168342 [details] [diff] [review]
fix autoconf checks

I also wonder whether mozilla could use the NSPR copy of the perl script that
makes all the wrappers so you don't need two copies of it.

sr=dbaron

Comment 15

[Brian Ryner (not reading)]

(In reply to comment #13)
> I wonder whether the autoconf test could actually work on any non-x86 platforms.
>  If not, is there a chance it could give false positives?  Might it be worth
> making it x86-only?

I thought about this some more.  The .hidden directive is specific to ELF but
not specific to x86, i.e. I would expect this to work on Linux/PPC.

Comment 16

[Peter Weilbacher]

It seems that the line
   ! grep '\.hidden.*foo_default' conftest.s > /dev/null
broke the standard OS/2 compile and the BSD tinderbox. Both give the error message
   <path>/configure: <line-no.>: Syntax error: "!" unexpected
If I remove the check listed above or put the whole expression into brackets
like this "(! grep ... /dev/null)" configure runs through. Don't know what's
special about OS/2 again, perhaps it's the use of ash instead of bash...

Comment 17

[Brian Ryner (not reading)]

Attachment: fix extra tinderbox rebuilds

The patch as checked in has an unexpected problem on Tinderbox.  Since the
tinderbox script removes the entire dist directory for each cycle, the system
header wrappers are removed and regenerated, causing the whole tree to rebuild.
 This patch makes the actual headers be generated in config/, and then just
symlinked into dist.  To make things easier, I also just disabled the
visibility compile flags for the config directories.

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 168830 [details] [diff] [review]
fix extra tinderbox rebuilds

Does nsinstall need any special flags to install an entire directory (on
platforms without symlinks, e.g., mingw)?  Other than that, this seems fine,
esp. if nsinstall is the only file compiled in these directories.

Comment 19

[Brian Ryner (not reading)]

(In reply to comment #18)
> (From update of attachment 168830 [details] [diff] [review] [edit])
> Does nsinstall need any special flags to install an entire directory (on
> platforms without symlinks, e.g., mingw)?  Other than that, this seems fine,
> esp. if nsinstall is the only file compiled in these directories.
> 

Looks to me like it handles it.

Comment 20

[Brian Ryner (not reading)]

I'm keeping this bug open for investigation... Tp time increased on gruff with
this patch, I'm going to try to determine if it's real or not once it cycles
from checkin of attachment 168830 [details] [diff] [review].

Comment 21

[Brian Ryner (not reading)]

Attachment: fix problems with XPTCStubBase when using rtti

Comment 22

[Darin Fisher]

Comment on attachment 168891 [details] [diff] [review]
fix problems with XPTCStubBase when using rtti

r=darin

Comment 23

[Peter Weilbacher]

Attachment: fix configure problems on OS/2 (and BSD)

This is the kind of fix I was talking about in comment 16. Using test instead
of brackets seems more in line with the style in the rest of the configure.ins.
It works to get OS/2 through configure and I suspect it also gets the BSD
tinderbox working again.

Comment 24

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 168947 [details] [diff] [review]
fix configure problems on OS/2 (and BSD)

I'm surprised this doesn't give an error.  We want to test the return value of
grep, and that's not what 'test' does.

Comment 25

[Peter Weilbacher]

Attachment: 2nd try: fix configure problems on OS/2 (and BSD)

Right. How about this one, with brackets as I originally suggested. I have
tried it out this time with grep in both bash and ash and it seems to work the
way you want.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

> ( ! grep '\.hidden.*foo_default' conftest.s > /dev/null )

 ( grep -v ... ) ?

Comment 27

[Peter Weilbacher]

No, grep -v still gives OK because the assembler file contains lines like
".data" or ".align 4".

Comment 28

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: fix gtkmozembed_internal.h (checked in)

The patch from attachment 168342 [details] [diff] [review] didn't patch gtkmozembed_internal.h. That
header is essential for embedding apps (Epiphany); and without this patch
Epiphany doesn't link, since gtk_moz_embed_get_nsIWebBrowser isn't exported.

Comment 29

[Darin Fisher]

Comment on attachment 169123 [details] [diff] [review]
fix gtkmozembed_internal.h (checked in)

r=darin

Comment 30

[Yusuf Goolamabbas]

I'm not sure if it's due to this patch but when I am compiling Firefox trunk on
Fedora Core 3 (using an objdir) and with the following ~/.mozconfig. I get no
initial chrome window. ps cax shows firefox and firefox-bin processes
This is all I get when I start up firefox
*** loading the extensions datasource
*** loading the extensions datasource

I tried it with a clean profile and there was no difference. nightly builds are
okay (I think they are built on fc2). If somebody has got trunk firefox compiled
with fc3's gcc running correctly, let me know if you had to do any workaround.

. $topsrcdir/browser/config/mozconfig
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/objdir-fb-opt
ac_add_options --disable-tests
ac_add_options --disable-debug
ac_add_options --enable-strip
ac_add_options --enable-strip-libs
ac_add_options --enable-default-toolkit=gtk2
ac_add_options --enable-xft
ac_add_options --disable-freetype2
ac_add_options --disable-installer
ac_add_options --enable-static
ac_add_options --disable-shared
ac_add_options --enable-official-branding

my build script is
MOZILLA_OFFICIAL=1
export MOZILLA_OFFICIAL
BUILD_OFFICIAL=1
export BUILD_OFFICIAL
pushd /usr/local/src/firefox/mozilla
gmake -f client.mk build
popd

Comment 31

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Bustage fixed (see bug 275674).

Comment 32

[Mike Kaply [:mkaply]]

Did the nspr bustage fix make it in?

Comment 33

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Yes.

Comment 34

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 168948 [details] [diff] [review]
2nd try: fix configure problems on OS/2 (and BSD)

This is no longer needed.

Comment 35

[Samuel Sieb]

I am seeing the FC3 problem with my builds.  I disabled the visibility options
in config.cache and reran configure.  A clean build still showed exactly the
same problem so unless I've missed something, it's not caused by this.

Comment 36

[Brian Ryner (not reading)]

Attachment: fix unresolved symbols when building with svg

Since libart doesn't mark its exported symbols at all, the easiest fix is just
to export everything as we were doing before.

Comment 37

[Brian Ryner (not reading)]

(In reply to comment #30)
> I'm not sure if it's due to this patch but when I am compiling Firefox trunk on
> Fedora Core 3 (using an objdir) and with the following ~/.mozconfig. I get no
> initial chrome window. ps cax shows firefox and firefox-bin processes

Good news.  Using your mozconfig, I was able to reproduce this problem. 
Breaking on the hung process in gdb shows this stack (sorry no line info... I
didn't use -g):

#0  0x083cfb63 in nsAttrValue::ToString ()
#1  0x08481e1e in nsXULElement::GetAttr ()
#2  0x0834f0b5 in SelectorMatches ()
#3  0x0834f57f in ContentEnumFunc ()
#4  0x08348237 in RuleHash::EnumerateAllRules ()
#5  0x0834f614 in nsCSSRuleProcessor::RulesMatching ()
#6  0x0836bfb8 in EnumRulesMatching ()
#7  0x0836c157 in nsStyleSet::FileRules ()
#8  0x0836cb8c in nsStyleSet::ResolveStyleFor ()

and on down... nothing wrong with the stack except that it never returns from
ToString.  I think what's actually happening here is a compiler/optimizer issue.
 You were using the default optimization setting of -O.  I normally use -Os for
optimized builds.  As a test, I cleaned and recompiled just content/base/src
using -Os and relinked.  This made the problem disappear.  It would appear that
some code in that directory (probably nsAttrValue.cpp) has an odd problem that
only appears under FC3's gcc with -O.  Whether it's a compiler bug or our bug
remains to be seen, but it hasn't shown up on any other configuration.  Valgrind
does not show any warnings when the hang happens.

Comment 38

[Brian Ryner (not reading)]

Attachment: workaround for aforementioned compiler problem

This solves the problem for me, I have no idea why though.  I'd guess that the
compiler is choking on the combination of static_cast and reinterpret_cast,
perhaps not masking out the masked bits correctly or something.

So, I don't think this patch is to blame directly, but it may have exposed a
code generation bug that we'll need to deal with.  Yusuf, can you verify that
this fixes the problem for you as well?  We could check it in as a stopgap
since non-buggy compilers should be able to optimize away the temporaries.

Comment 39

[Yusuf Goolamabbas]

Bryner, applying your patch to trunk and then doing a clobber build. I get my
initial Firefox window. However, this build seems to hang at many pages. The
hangs seem to occur at pages which have flash content. Some URL's where the
browser hangs for me

http://news.com.com/
http://disney.go.com/
http://www.theinquirer.net/
http://www.theregister.co.uk/

I have no problems visiting Ali Ebrahim's blog or Asa's blog.
http://blog.ebrahim.org/
http://weblogs.mozillazine.org/asa/

I'm going to do another clobber build with the patched tree and having following
additional line in my mozconfig

ac_add_options --enable-optimize="-Os -fno-reorder-functions -freorder-blocks
-gstabs+"

I'll report back how that build turns out

Comment 40

[Yusuf Goolamabbas]

Bryner, The build (with your patch, id 170216) is extremely stable with the
optimised flags. I see no hangs even with extremely flash heavy websites. 

I took these flags from about:buildconfig from an official GTK2/XFT nightly build

-Os -fno-reorder-functions -freorder-blocks -gstabs+

Comment 41

[Brian Ryner (not reading)]

Ok, so something in FC3's gcc with -O1 doesn't get along with our code.

Comment 42

[Samuel Sieb]

I'm pretty sure I did my build with -Os.  I'll try it again with the extra flags.

Comment 43

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #38)
> Created an attachment (id=170216) [edit]
> workaround for aforementioned compiler problem

it only helps somewhat for me :( even with this patch and FC3, I sometimes get
hangs in nsViewManager::DisableRefresh:
0x0927e795 <_ZN13nsViewManager14DisableRefreshEv+11>:   sub    $0xc,%esp
0x0927e798 <_ZN13nsViewManager14DisableRefreshEv+14>:   jmp    0x927e795
<_ZN13nsViewManager14DisableRefreshEv+11>

(that's with the default optimization settings)

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 168891 [details] [diff] [review]
fix problems with XPTCStubBase when using rtti

(a slightly different version of this patch was checked in as part of Bug
281834)

Comment 45

[Mark Calderbank]

Using CVS gcc on Linux, I get a crash when using the left or right arrow keys in
any text field:

/home/mark/local/firefox4/firefox-bin: relocation error:
/home/mark/local/firefox4/components/libctl.so: undefined symbol: pangolite_find_map

I believe that the new visibility technique is the cause, as I can avoid the
crash by wrapping #pragma GCC visibility pop and push(hidden) around the
declaration of pangolite_find_map in intl/ctl/src/pangoLite/pango-modules.h.

Here's my mozconfig for completeness.

. $topsrcdir/browser/config/mozconfig
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/build-kermit
export CC=/home/mark/local/bin/gcc
export CXX=/home/mark/local/bin/g++
ac_add_options --disable-debug
ac_add_options --enable-crypto
ac_add_options --enable-reorder
ac_add_options --enable-strip
ac_add_options --enable-default-toolkit=gtk2
ac_add_options --enable-xft
ac_add_options --disable-freetype2
ac_add_options --disable-tests
ac_add_options --disable-mailnews
ac_add_options --disable-ldap
ac_add_options --disable-postscript
ac_add_options --disable-xprint
ac_add_options --disable-gnomevfs
ac_add_options --disable-gnomeui
ac_add_options --enable-ctl
ac_add_options --disable-accessibility
ac_add_options --disable-composer
ac_add_options --disable-installer
ac_add_options --with-system-jpeg=/usr
ac_add_options --with-system-png=/usr

Comment 46

[Julien Pierre]

The NSPR tip is also broken on all versions of Solaris (sparc 32 bit, 64 bit,
x86, AMD64) when using the gcc 3.4.3 compiler - with NS_USE_GCC set to 1 .
Builds with the Sun studio compiler are still OK .

The problem is autoconf related . The symptoms appear to be similar to the ones
reported for the OS/2 bustage. See below for the log . A fix for configure.in
was provided in this bug, but it does not apply to the NSPR tip. Also, a
configure fix was not provided. Both configure and configure.in fixes are
needed, since on Solaris, autoconf is not run as part of the build, even if
configure is deleted . This needs to get resolved ASAP on the NSPR tip.

[jp96085@monstre]/export/home/newtip/mozilla/security/nss 337 % gmake build_nspr
../coreconf/nsinstall/SunOS5.10_i86pc_gcc_64_OPT.OBJ/nsinstall -D
../../nsprpub/SunOS5.10_i86pc_gcc_64_OPT.OBJ
cd ../../nsprpub/SunOS5.10_i86pc_gcc_64_OPT.OBJ ; \
CC=gcc CXX=g++ sh ../configure \
--disable-debug --enable-optimize --enable-64bit \
--with-dist-prefix='/export/home/newtip/mozilla/security/nss/../../dist/SunOS5.10_i86pc_gcc_64_OPT.OBJ'
\
--with-dist-includedir='/export/home/newtip/mozilla/security/nss/../../dist/SunOS5.10_i86pc_gcc_64_OPT.OBJ/include'
creating cache ./config.cache
checking host system type... i386-pc-solaris2.10
checking target system type... i386-pc-solaris2.10
checking build system type... i386-pc-solaris2.10
checking for whoami... /usr/ucb/whoami
checking for c++... g++
checking whether the C++ compiler (g++  ) works... yes
checking whether the C++ compiler (g++  ) is a cross-compiler... no
checking whether we are using GNU C++... yes
checking whether g++ accepts -g... yes
checking for gcc... gcc
checking whether the C compiler (gcc  ) works... yes
checking whether the C compiler (gcc  ) is a cross-compiler... no
checking whether we are using GNU C... yes
checking whether gcc accepts -g... yes
checking how to run the C preprocessor... gcc -E
checking for ranlib... ranlib
checking for as... /opt/SUNWspro/prod/bin/as
checking for ar... /usr/ccs/bin/ar
checking for ld... /usr/ccs/bin/ld
checking for strip... /usr/ccs/bin/strip
checking for windres... no
checking for gcc -pipe support... no
checking for visibility(hidden) attribute... 
../configure: test: argument expected
gmake: *** [../../nsprpub/SunOS5.10_i86pc_gcc_64_OPT.OBJ/config.status] Error 1

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

> Both configure and configure.in fixes are
> needed, since on Solaris, autoconf is not run as part of the build

autoconf is run nowhere as part of the build...

Comment 48

[Wan-Teh Chang]

Julien,

Could you track down the line number and the cvs revision
of nsprpub/configure where this error message is emitted?

checking for visibility(hidden) attribute... 
../configure: test: argument expected

I looked at this and my guess is that we need to quote
the variables.  For example,

    if test $ac_cv_visibility_hidden = yes; then

should probably be

    if test "$ac_cv_visibility_hidden" = yes; then

or

    if test "$ac_cv_visibility_hidden" = "yes"; then

Similarly for

    if test $ac_cv_visibility_pragma = yes; then

Comment 49

[Wan-Teh Chang]

Attachment: NSPR patch (checked in): quote the arguments to the test command

Julien, please test this NSPR patch.

I'm surprised that no one else has built on
Solaris using gcc in the past two or three
months this code is in Mozilla.

mozilla/configure.in needs a similar patch.

Comment 50

[Julien Pierre]

Christian,

autoconf is run on OS/2 as part of the build if configure is missing. I have a
"del /s configure >& \DEV\NUL" at the end of my checkout script .

Wan-Teh,

The Solaris autoconf breakage happened in version 1.179 of
mozilla/nsprpub/configure , and release 1.181 of mozilla/nsprpub/configure.in .
These checkins happened on the NSPR tip on 4/29/2005, which is just 12 days ago.

Backing out to the release immediately before those files gets me past autoconf
. But the build still doesn't work, because other files were checked in at the
same time that depended on the changes .

I just verified that the patch you attached fixes the build problem . Please
check it in to the tip. Thanks !

Comment 51

[Wan-Teh Chang]

Comment on attachment 183229 [details] [diff] [review]
NSPR patch (checked in): quote the arguments to the test command

Requesting aviary1.1a1 and mozilla1.8b2 approvals.

This is a build fix for GCC builds on platforms where
Bourne shell rather than bash is used to interpret the
configure script.  Bash allows an argument to the test
command to be nonexistent, but Bourne shell requires
at least an empty string.  So the arguments to the test
command need to be quoted in most cases.

Comment 52

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183229 [details] [diff] [review]
NSPR patch (checked in): quote the arguments to the test command

a=shaver, anticipating julien's review.

Comment 53

[Wan-Teh Chang]

Attachment: mozilla configure patch (checked in): quote the arguments to the test command

The same fix for mozilla/configure and mozilla/configure.in.

I've verified that mozilla's and mozilla/nsprpub's configure
scripts are the only configure scripts that need this fix.

Comment 54

[Wan-Teh Chang]

Comment on attachment 183229 [details] [diff] [review]
NSPR patch (checked in): quote the arguments to the test command

I just checked in this patch on the NSPR trunk (NSPR 4.6)
and the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta 2).

Comment 55

[Wan-Teh Chang]

Comment on attachment 183304 [details] [diff] [review]
mozilla configure patch (checked in): quote the arguments to the test command

Requesting aviary1.1a1 and mozilla1.8b2 approvals.

This is the same as the NSPR patch that was just approved
but applied to Mozilla's configure script.

Comment 56

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183304 [details] [diff] [review]
mozilla configure patch (checked in): quote the arguments to the test command

a=shaver

Comment 57

[Wan-Teh Chang]

Comment on attachment 183304 [details] [diff] [review]
mozilla configure patch (checked in): quote the arguments to the test command

OK, this patch has also been checked in.

Comment 58

[Wan-Teh Chang]

Comment on attachment 183229 [details] [diff] [review]
NSPR patch (checked in): quote the arguments to the test command

Since our configure test for the GCC visibility
attribute was copied from glibc, I thought I'd
contribute this fix back to glibc.

When I looked at glibc's configure script, I found
that the arguments to the test command are not
quoted (as expected), but they don't need to be
quoted because they always have a value (either "no"
or "yes").  So I was confused.	Upon looking at
our configure script closer, I found that we made
a mistake when adapting glibc's configure test for
our use.  We renamed the variables, but missed one.
So we ended up with

    ...
    ac_cv_visibility_attribute=no  # not renamed!
    if blah blah; then
	if blah blab; then
	    ac_cv_visibility_hidden=yes
	fi
    fi
    ...
    if test $ac_cv_visibility_hidden = yes; then

So the root cause of the problem with Bourne Shell
is not that $ac_cv_visibility_hidden in the test
command is not quoted, but that we missed one
assignment statement when renaming that variable.
Our quoting fix takes care of this, so we can wait
until aviary1.1a2 to fix the variable name.

Comment 59

[Wan-Teh Chang]

Brian:

You wrote:

  [the new -fvisibility=hidden switch] only affects visibility
  for symbols that are defined in the same translation unit as
  the caller.

Are you sure this is the case?  I didn't see anything about this
in the GCC 4.0 manual.
http://gcc.gnu.org/onlinedocs/gcc-4.0.0/gcc/Code-Gen-Options.html#index-fvisibility-1562

Comment 60

[Simon Strandman]

Visibility support seems a bit broken on amd64. I get this error when trying to
build:

x86_64-pc-linux-gnu-gcc -DGENTOO_NSPLUGINS_DIR=\"/usr/lib64/nsplugins\"
-DGENTOO_NSBROWSER_PLUGINS_DIR=\"/usr/lib64/nsbrowser/plugins\"   -W -Wno-unused
-Wpointer-arith -Wcast-align -Wno-long-long -march=athlon64 -pipe -fPIC
-Wno-return-type -w -pthread -pipe  -DNDEBUG -DTRIMMED -ffunction-sections -Os
-fPIC -shared -Wl,-h -Wl,libmozjs.so -Wl,-R/usr/lib64/mozilla-firefox -o
libmozjs.so  jsapi.o jsarena.o jsarray.o jsatom.o jsbool.o jscntxt.o jsdate.o
jsdbgapi.o jsdhash.o jsdtoa.o jsemit.o jsexn.o jsfun.o jsgc.o jshash.o
jsinterp.o jslock.o jslog2.o jslong.o jsmath.o jsnum.o jsobj.o jsopcode.o
jsparse.o jsprf.o jsregexp.o jsscan.o jsscope.o jsscript.o jsstr.o jsutil.o
jsxdrapi.o jsxml.o prmjtime.o    -Wl,-O1  -Wl,-R/usr/lib64/mozilla-firefox   -lm
-ldl -L../../dist/lib -lplds4 -lplc4 -lnspr4 -lpthread -ldl -ldl -lm
/usr/lib/gcc/x86_64-pc-linux-gnu/4.0.2/../../../../x86_64-pc-linux-gnu/bin/ld:
warning: creating a DT_TEXTREL in object.
/usr/lib/gcc/x86_64-pc-linux-gnu/4.0.2/../../../../x86_64-pc-linux-gnu/bin/ld:
jsapi.o: relocation R_X86_64_PC32 against `memcpy@@GLIBC_2.2.5' can not be used
when making a shared object; recompile with -fPIC
/usr/lib/gcc/x86_64-pc-linux-gnu/4.0.2/../../../../x86_64-pc-linux-gnu/bin/ld:
final link failed: Bad value
collect2: ld returned 1 exit status
gmake[3]: *** [libmozjs.so] Error 1
gmake[3]: Leaving directory
`/var/tmp/portage/mozilla-firefox-1.5_rc2/work/mozilla/js/src'
gmake[2]: *** [libs] Error 2
gmake[2]: Leaving directory
`/var/tmp/portage/mozilla-firefox-1.5_rc2/work/mozilla/js'
gmake[1]: *** [tier_2] Error 2
gmake[1]: Leaving directory `/var/tmp/portage/mozilla-firefox-1.5_rc2/work/mozilla'
make: *** [default] Error

Disabling symbol visibility fixes it (I haven't actually completed the build but
it passes far beyond that point). My binutils is 2.16.1, glibc is 2.3.5 and gcc
is 4.0.2 with patches for gcc pr19664, pr20297 and pr19520.

Comment 61

[Wan-Teh Chang]

Simon: see bug 307168 for the Linux x86-64 build problem.

Brian: this bug can be resolved FIXED.  We should open new
bugs (such as bug 307168) to address the fallouts.

Comment 62

[Simon Strandman]

Ok, thanks!

A small question before this bug is forgotten. Shouldn't
-fvisibility-inlines-hidden also be added to the CXXFLAGS when visibility
support is enabled? It seems like a good idea according to the gcc manual.

Comment 63

[Boris Zbarsky [:bzbarsky]]

So it sounds like bug 283130 is comment 37 from this bug?  If so, that's happening with our trunk nightlies, no?  In which case we should probably check in that workaround...