If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

remove checks for visibility pragmas / attributes / options in gcc

RESOLVED FIXED in mozilla32

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: tbsaunde, Assigned: glandium)

Tracking

unspecified
mozilla32
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
the last bit of this became obsolete with gcc 4.1!
(Reporter)

Comment 1

4 years ago
Created attachment 8361949 [details] [diff] [review]
bug 961264 - remove obsolete checks for gcc visibility stuff
Attachment #8361949 - Flags: review?(mh+mozilla)
(Assignee)

Comment 2

4 years ago
Comment on attachment 8361949 [details] [diff] [review]
bug 961264 - remove obsolete checks for gcc visibility stuff

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

We're actually hitting one of those on mac:

   checking For gcc visibility bug with class-level attributes (GCC bug 26905)... yes

and that makes us use -fvisibility=hidden there.

Which, all things considered, I think we should try making work and see what different it makes nowadays (in symbol table sizes, etc.), because the headers wrapping thing is kind of a nightmare.
Attachment #8361949 - Flags: review?(mh+mozilla) → review-
(Reporter)

Comment 3

4 years ago
(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 8361949 [details] [diff] [review]
> bug 961264 - remove obsolete checks for gcc visibility stuff
> 
> Review of attachment 8361949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We're actually hitting one of those on mac:
> 
>    checking For gcc visibility bug with class-level attributes (GCC bug
> 26905)... yes

O.O I guess its worth seeing someones already filed a clang bug for that.

> and that makes us use -fvisibility=hidden there.
> 
> Which, all things considered, I think we should try making work and see what
> different it makes nowadays (in symbol table sizes, etc.), because the
> headers wrapping thing is kind of a nightmare.

not going to disagree with that!
(Assignee)

Comment 4

4 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> O.O I guess its worth seeing someones already filed a clang bug for that.

The problem is just that the test is wrong. I guess clang does the right thing, but I doubt "egrep -c '@PLT|\\$stub' conftest.S" is the right test on mac.

Comment 5

4 years ago
Comment on attachment 8361949 [details] [diff] [review]
bug 961264 - remove obsolete checks for gcc visibility stuff

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

js/src/configure.in and nsprpub/configure.in diff are missing

::: configure.in
@@ -2578,5 @@
> -                  fi
> -                  rm -f conftest.[cs]
> -                 ])
> -  if test "$ac_cv_visibility_hidden" = "yes"; then
> -    AC_DEFINE(HAVE_VISIBILITY_HIDDEN_ATTRIBUTE)

AC_DEFINE line shouldn't be removed

@@ -2594,5 @@
> -                    fi
> -                    rm -f conftest.[cs]
> -                   ])
> -    if test "$ac_cv_visibility_default" = "yes"; then
> -      AC_DEFINE(HAVE_VISIBILITY_ATTRIBUTE)

Ditto. Otherwise it'd break code that marks only certain symbols visible after pragma push.

  #pragma GCC visibility push(hidden)
  ...
  __attribute__((visibility("default"))) int foo();
  int bar();
dbaron points to https://bugzilla.mozilla.org/show_bug.cgi?id=273336#c0 for why #pragma visibility
er, for why #pragma GCC visibility is better than -fvisibility=hidden.
Seems to me that, per comment #4, we can take this patch with Jan's fixes, since the configure test failure on Mac is presumably spurious.
Ah, but we don't want to wrap all Mac system headers. OK.
I've updated the patch:
https://tbpl.mozilla.org/?tree=Try&rev=2219930e12f1
(Assignee)

Comment 11

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> I've updated the patch:
> https://tbpl.mozilla.org/?tree=Try&rev=2219930e12f1

# Not bothering to wrap system headers on Mac, since -fvisibility=hidden
# produces the same code on Mach-O

The above comment is actually not true, if I believe my linux's clang with -target x86_64-apple-darwin:

with pragma:
	movl	_a(%rip), %eax

with -fvisibility=hidden:
	movq	_a@GOTPCREL(%rip), %rax
	movl	(%rax), %eax

Comment 9 still applies, though.
(In reply to Mike Hommey [:glandium] from comment #11)
> The above comment is actually not true, if I believe my linux's clang with
> -target x86_64-apple-darwin:
> 
> with pragma:
> 	movl	_a(%rip), %eax
> 
> with -fvisibility=hidden:
> 	movq	_a@GOTPCREL(%rip), %rax
> 	movl	(%rax), %eax
> 
> Comment 9 still applies, though.

You should bring that up with bsmedberg on the dev-platform thread.
(Reporter)

Comment 13

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Seems to me that, per comment #4, we can take this patch with Jan's fixes,
> since the configure test failure on Mac is presumably spurious.

Well, I was intending to nuke all the stuff that used the defines instead of fixing them, but I guess if your in a hurry we can do that later.
Fixed lots of issues on Android, and got it building locally:
https://tbpl.mozilla.org/?tree=Try&rev=400d807ada83
(Assignee)

Comment 15

4 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Fixed lots of issues on Android, and got it building locally:
> https://tbpl.mozilla.org/?tree=Try&rev=400d807ada83

The change to build/stlport/stlport/stl/_cwchar.h is most probably unwanted (that's third party code).
All the omx-plugin stuff is built as separate shared libraries. You probably don't want hidden visibility applied to them at all, so setting NO_VISIBILITY_FLAGS = True in the corresponding moz.builds should be enough.
B2G and Android building locally: 
https://tbpl.mozilla.org/?tree=Try&rev=ca447c099a16
The changeset I based on was busted. The try push also uncovered a couple more B2G issues. new push:
https://tbpl.mozilla.org/?tree=Try&rev=e07dd708f34a
There's an Android crash in the middle of nowhere. That's a bit scary.
Revised for B2G issue: https://tbpl.mozilla.org/?tree=Try&rev=f3be73f627d2
More B2G fixage: https://tbpl.mozilla.org/?tree=Try&rev=20cb0237823f
Android's fine except xpcshell tests are failing on 2.2. They pass locally and I don't know how to get a crash-stack from the TBPL logs. Help?

Still working on B2G. https://tbpl.mozilla.org/?tree=Try&rev=a75acbdf1dc9
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Android's fine except xpcshell tests are failing on 2.2.

AFAICT the tests are actually passing and we're crashing on shutdown.
For B2G the latest pushed patch is down to just one error on ICS and JB: strzcmp16 is included hidden somehow in GonkPermission.cpp. I tried to fix this by making GonkPermission.cpp's first #include be GonkPermission.h and GonkPermission.h's first #include be <utils/String16.h>, but that didn't fix it.

On my local system I would debug this by simply running objdump -t on all .o files in the build directory and looking for lines that contain .hidden and strzcmp16, which will probably hit just GonkPermission.o. Then I'd grab the command line used to build the .o file and hack it to preprocess instead. Then I have a script that labels each line of the .i file with its current visibility state. That makes it easy to see whether/how there's a declaration of strzcmp16 with hidden visibility. But I can't do that without access to the partial build produced by the build machine.

KK has an additional issue where something in UnifiedProtocols0 is finding android::Fence in the hidden state. The same steps as above would work but I'd need build machine access.

And I'm going on PTO for a week.
Created attachment 8412152 [details] [diff] [review]
current patch

In case tryserver loses it and someone else wants to work on it
Attachment #8361949 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
Ok, let's just solve the immediate problem in this bug, that is that the tests are broken and we don't necessarily get the right flags currently, and leave switching android to pragmas to a followup bug.
(Assignee)

Updated

4 years ago
Assignee: trev.saunders → mh+mozilla
(Assignee)

Comment 25

4 years ago
Created attachment 8412407 [details] [diff] [review]
Remove checks for visibility pragmas and attributes in gcc
Attachment #8412407 - Flags: review?(ted)
(Assignee)

Updated

4 years ago
Attachment #8412152 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
(In reply to Mike Hommey [:glandium] from comment #25)
> Created attachment 8412407 [details] [diff] [review]
> Remove checks for visibility pragmas and attributes in gcc

This replaces a bunch of useless and wrong tests with a hardcoded list of what we currently end up doing. It also decouples the stl wrappers, which should be set whether visibility is set with pragmas or -fvisibility. It just happens that we are lucky that where we're meaning to use them, we're currently always using system wrappers. (Although, we should be using them on mac, and we currently aren't). But that's meant to change (cf. roc's work, which will move to a followup bug)
(Assignee)

Comment 27

4 years ago
We could do this in nspr too, but there are two concerns. The first is whether nspr wants to keep building with older versions of gcc (those that don't support visibility at all, which I think means < 4.0, and those that don't fuck up on x86_64, which I think is < 4.2 or 4.3). The second concern is that it means nspr is going to setup its own system wrappers on top of those setup by the gecko build system. We can't make nspr not do it because it needs to build separately, too, but I guess we could just override that when invoking make in nsprpub in config/nspr/Makefile.in

OTOH, maybe it doesn't make a significant difference and it's not worth caring...
Flags: needinfo?(wtc)
(Assignee)

Comment 28

4 years ago
On a nspr x86-64 standalone build with --enable-64bit:
with -fvisibility=hidden:
     text	   data	    bss	    dec	    hex	filename
   303811	   7800	  10624	 322235	  4eabb	dist/lib/libnspr4.so
    15812	   1072	     24	  16908	   420c	dist/lib/libplc4.so
    11472	   1008	     48	  12528	   30f0	dist/lib/libplds4.so
with system wrappers and pragma:
     text	   data	    bss	    dec	    hex	filename
   302759	   7760	  10624	 321143	  4e677	dist/lib/libnspr4.so
    15812	   1072	     24	  16908	   420c	dist/lib/libplc4.so
    11472	   1008	     48	  12528	   30f0	dist/lib/libplds4.so

Let's say we don't care.
Flags: needinfo?(wtc)
(Assignee)

Comment 29

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=26ebaa983f24
(Assignee)

Updated

4 years ago
Blocks: 1001320

Comment 30

4 years ago
(In reply to Mike Hommey [:glandium] from comment #27)

Mike: I don't want to create system header wrappers for NSPR.
If all the gcc/clang versions used by Mozilla support visibility
correctly, it is fine by me to remove the checks for visibility
in gcc from nspr/configure.in.
Comment on attachment 8412407 [details] [diff] [review]
Remove checks for visibility pragmas and attributes in gcc

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

It's...beautiful!
Attachment #8412407 - Flags: review?(ted) → review+
(Assignee)

Comment 32

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e7af3e86fd
https://hg.mozilla.org/mozilla-central/rev/35e7af3e86fd
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Filed 1006248 to fix B2G/Android.
fwiw this breaks Linux ARM JS shell builds due to the symbols __aeabi_idivmod and __aeabi_uidivmod.  However bug 1001320 fixes this.

Updated

3 years ago
Depends on: 1008192
You need to log in before you can comment on or make changes to this bug.