366844 - [configure] visibility pragma support broken

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Karsten Bräckelmann]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.4) Gecko/20060607 Firefox/1.5.0.4
Build Identifier: 

The configure test for the visibility pragma seems to be broken. In my case it claims the pragma to be supported, which breaks the build. Explicitely disabling it makes the build happy again.

Firefox 2.0.0.1

gcc (GCC) 3.4.3 (Mandrakelinux 10.2 3.4.3-7mdk)
GNU ld version 2.15.92.0.2 20040927

$ uname -s -m -p -i
Linux i686 Intel(R) Pentium(R) M processor 1.73GHz unknown

Note: I had no such issue building FF 1.5.x.x on the very same machine and system. I first encountered this issue since FF 2.0.


Reproducible: Always

Steps to Reproduce:
Build FF... ;)

Actual Results:  
The relevant configure report for the *broken* build, will attach the build error to not break lines:

 checking for visibility(hidden) attribute... yes
 checking for visibility(default) attribute... yes
 checking for visibility pragma support... yes
 checking For x86_64 gcc visibility bug with builtins (GCC bug 20297)... no

Configure output after explicitely *disabling* visibility pragma support by adding "ac_cv_visibility_pragma=no" to mozconfig-firefox:

 checking for visibility(hidden) attribute... yes
 checking for visibility(default) attribute... yes
 checking for visibility pragma support... (cached) no



If you need any more details, please feel free to ask.

Comment 1

[Karsten Bräckelmann]

Attachment: build error

Comment 2

[Karsten Bräckelmann]

The User-Agent: reported above of course is not the one I am talking about, but the currently used one. ;)

I found the workaround here:
http://benjamin.smedbergs.us/blog/2005-10-27/gcc-40-workaround
http://benjamin.smedbergs.us/blog/2006-03-28/gcc-and-visibility-one-step-forward-hit-a-brick-wall/

This seems not to be limited to GCC 4 only. The "unresolvable relocation" is common to my issue and the one in these links, although for different symbols (and FF versions).

Comment 3

[Benjamin Smedberg]

Does the trunk build properly? I think that there are trunk configure patches which were not backported to the branches.

Comment 4

[Karsten Bräckelmann]

Some details of a *different* user, who got a similar relocation error:

$ uname -a
Linux domo-debian 2.6.19.1.2007-01-04 #1 PREEMPT Thu Jan 4 11:18:32 EET 2007 x86_64 GNU/Linux

$ gcc -v
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --enable-languages=c,c++,fortran,objc,obj-c++,treelang --prefix=/usr --enable-shared --with-system-zlib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --enable-nls --program-suffix=-4.1 --enable-__cxa_atexit --enable-clocale=gnu --enable-libstdcxx-debug --enable-mpfr --enable-checking=release x86_64-linux-gnu
Thread model: posix
gcc version 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)

$ ld --version
GNU ld version 2.17 Debian GNU/Linux


The build error (straight from my IRC logs):

g++ -I/home/domas/garnome/include  -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -I/home/domas/garnome/include -L/home/domas/garnome/lib -O2 -pipe -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O -fPIC -shared -Wl,-z,defs -Wl,-h,libxpcom_core.so -o libxpcom_core.so  pldhash.o nsCOMPtr.o nsComponentManagerUtils.o nsDebug.o nsID.o nsIInterfaceRequestorUtils.o nsINIParser.o nsMemory.o nsTraceRefcnt.o nsWeakReference.o nsGREGlue.o nsVersionComparator.o nsTHashtable.o nsTArray.o nsGenericFactory.o nsXPComInit.o nsStringAPI.o    -Wl,--export-dynamic -L/home/domas/garnome/lib   -Wl,--whole-archive ../../dist/lib/libxpcomds_s.a ../../dist/lib/libxpcomio_s.a ../../dist/lib/libxpcomcomponents_s.a ../../dist/lib/libxpcomthreads_s.a ../../dist/lib/libxpcomproxy_s.a ../../dist/lib/libxpcombase_s.a ../../dist/lib/libxptcall.a ../../dist/lib/libxptinfo.a ../../dist/lib/libxpt.a ../../dist/lib/libxptcmd.a ../../dist/lib/libstring_s.a  -Wl,--no-whole-archive  -L../../dist/lib -lplds4 -lplc4 -lnspr4 -L/home/domas/garnome/lib -lpthread -ldl -ldl -lm
/usr/bin/ld: nsCOMPtr.o: relocation R_X86_64_PC32 against `nsGetServiceByContractIDWithError::operator()(nsID const&, void**) const' can not be used when making a shared object; recompile with -fPIC
/usr/bin/ld: final link failed: Bad value


Adding "ac_cv_visibility_pragma=no" to mozconfig-firefox fixed the issue for him, too.

Comment 5

[Karsten Bräckelmann]

(In reply to comment #3)
> Does the trunk build properly? I think that there are trunk configure patches
> which were not backported to the branches.

Ah, the one who provided the workaround himself. :)

I did not try this with trunk, just 2.0 and 2.0.0.1. I will try to give trunk a test the next days.


Btw, are there any downsides of disabling this pragma? Any impact?

Comment 6

[Benjamin Smedberg]

The pragma, when it works correctly, reduces the number of dynamic relocations in mozilla code and improves startup time significantly... somewhere between 5-15% in some tests.

Comment 7

[Karsten Bräckelmann]

I see, thanks Benjamin. :)

What we (the GARNOME Team) did, is this:  Get the latest Firefox sources from CVS HEAD, identify the visibility pragma test in configure.in and substitute the one from 2.0.0.1 with the latest code. Find an old autoconf 2.13 ;) and regenerate configure. Results from running ./configure:

 checking for visibility(hidden) attribute... yes
 checking for visibility(default) attribute... yes
 checking for visibility pragma support... yes
 checking For gcc visibility bug with class-level attributes (GCC bug 26905)...
  yes
 checking For x86_64 gcc visibility bug with builtins (GCC bug 20297)... no

Unlike the current 2.0.0.1 test, latest code identifies a gcc visibility bug to be present.


[Currently running the build to verify... Will report back later.]

So, given this upstream code works fine, what are the odds for this to be backported soon, like... 2.0.0.2? :)


Btw, I can provide the patch I used for this on request. ;)

Comment 8

[Karsten Bräckelmann]

Confirmed. Build finished successfully with the visibility pragma test from HEAD.

(Again, this still is Firefix 2.0.0.1, I just changed some short part of configure to use the latest code from HEAD.)

Comment 9

[Benjamin Smedberg]

If you can provide a backport patch then yes, I will consider it. But please note that when I made the configure changes there were code changes required as well. I don't think a patch that touches just configure will be sufficient.

Just a note, when attaching patches here please don't diff configure, only configure.in

Comment 10

[Karsten Bräckelmann]

Great. :)  I will test the patch with the other user (see comment 4) first. If it works as expected, I'll attach the proposed patch for review here.

Thanks, Benjamin.

Comment 11

[Karsten Bräckelmann]

Confirmed, the configure.in patch works for the other user (see comment 4, totally different system) as well.

The original configure results for the relevant visibility pragma tests are identical to the ones in comment 0. With the patch in place and regenerating configure, the results are identical to comment 7. Build finished successfully, without the need to edit mozconfig.

Comment 12

[Karsten Bräckelmann]

Attachment: proposed fix, backported GCC visibility bug tests

As mentoined in comment 7, this is a straightforward backport of the latest configure tests for the GCC visibility bugs. I believe the patch to be isolated and not require further code changes, since it simply extends the tests to identify GCC visibility bugs and requires both to not be present.

Comment 13

[Karsten Bräckelmann]

Benjamin, did you have a chance to review the patch yet?

(FWIW, a hand crafted configure patch based on this one just has been added to GARNOME trunk.)

Comment 14

[Benjamin Smedberg]

Comment on attachment 251463 [details] [diff] [review]
proposed fix, backported GCC visibility bug tests

Karsten, in generally you should set the review? flag to me to request review. I'm setting the flag now to add it to my queue.

Comment 15

[Karsten Bräckelmann]

Oops. Thanks for the info. I'm not that used to the Mozilla bugzilla but the GNOME way of handling bugs... I will remember this bit. :)

Comment 16

[Benjamin Smedberg]

Comment on attachment 251463 [details] [diff] [review]
proposed fix, backported GCC visibility bug tests

Yeah, I had already ported the other bits. I really mean to be requesting approval1.8.1.3?, but that flag isn't available so I'm setting 1.8.1.2 and I'll let the drivers set the correct flags.

Comment 17

[Jay Patel [:jay]]

Comment on attachment 251463 [details] [diff] [review]
proposed fix, backported GCC visibility bug tests

bsmedberg:  Clearing the blocking flag for 1.8.1.2 to get this off our radar for the upcoming release.

If you want this in 1.8.1.3, please renominate when those flags are available. Thanks.

Comment 18

[Karsten Bräckelmann]

Comment on attachment 251463 [details] [diff] [review]
proposed fix, backported GCC visibility bug tests

Requesting approval1.8.1.3? as per comment 16.

Comment 19

[Karsten Bräckelmann]

Also requesting blocking1.8.1.3?, since I have been told on IRC that this is the way to go. :)

Comment 20

[Daniel Veditz [:dveditz]]

Karsten: assigning to you to push this in. Please get someone to check this in on the trunk for sanity-checking before we approve the patch. Not blocking any release, but we will approve this build change once it's been vetted for unexpected bustage on the trunk.

Comment 21

[Karsten Bräckelmann]

Daniel, I'm not sure if you get you. Maybe it's just the subtle lingo difference between GNOME and Mozilla bugzilla. ;)

Please note that I do not have commit permissions. Which the assigning is about, no? I'd need someone to do this on my behalf, please. I can provide the missing ChangeLog bit, if you want me to.  Benjamin? :)

Also, this patch is a straight backport from trunk (aka HEAD). See comment 7 and comment 12. This code is already part of trunk, and thus committing and sanity- checking this particular patch on trunk won't really work.

As I understand it, Benjamnin already reviewed the patch, and even meant to request approval1.8.1.3? himself, which unfortunately hasn't been available at that stage. I'm kind of confused how to go on and finally get this patch backported.


Regarding "not blocking": Sorry, if this was wrong. I just set it according to the advice in #firefox. I'm not used to the Mozilla way of processing patches.

Comment 22

[Benjamin Smedberg]

Dan, this code is already landed on trunk, and should be safe for the branch.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 251463 [details] [diff] [review]
proposed fix, backported GCC visibility bug tests

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 24

[Benjamin Smedberg]

Fixed on MOZILLA_1_8_BRANCH

Comment 25

[Robert Kaiser]

This might have caused a build problem on SeaMonkey Mac, see my post in m.d.builds

Comment 26

[Karsten Bräckelmann]

Robert, does trunk have the same issue for a long time already?

Please note, that this patch is a straight backport from trunk of some configure-time detection magic, which has been used in trunk before.

Comment 27

[Robert Kaiser]

Still, the same library apparently links fine on trunk but doesn't on branch. Something clearly _is_ different, and it started happening the first time configure was run by that box after the checkin for this bug.
See the newsgroup for the details.