Closed
Bug 961264
Opened 10 years ago
Closed 10 years ago
remove checks for visibility pragmas / attributes / options in gcc
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla32
People
(Reporter: tbsaunde, Assigned: glandium)
References
Details
Attachments
(1 file, 2 obsolete files)
12.33 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
the last bit of this became obsolete with gcc 4.1!
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8361949 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 2•10 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•10 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•10 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 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•10 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•10 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•10 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.
In case tryserver loses it and someone else wants to work on it
Attachment #8361949 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 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•10 years ago
|
Assignee: trev.saunders → mh+mozilla
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8412407 -
Flags: review?(ted)
Assignee | ||
Updated•10 years ago
|
Attachment #8412152 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 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•10 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•10 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•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=26ebaa983f24
Comment 30•10 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 31•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/35e7af3e86fd
Comment 33•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/35e7af3e86fd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Filed 1006248 to fix B2G/Android.
Comment 35•10 years ago
|
||
fwiw this breaks Linux ARM JS shell builds due to the symbols __aeabi_idivmod and __aeabi_uidivmod. However bug 1001320 fixes this.
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•