Closed Bug 1001320 Opened 11 years ago Closed 11 years ago

Switch b2g and android to pragmas for visibility

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla32

People

(Reporter: glandium, Assigned: roc)

References

Details

Attachments

(6 files, 4 obsolete files)

Part 1: Add MOZ_EXPORT in various places
25.70 KB, patch
glandium
: review+
vchang
: review+
Details | Diff | Splinter Review
Part 2: Add NO_VISIBILITY_FLAGS to media/omx-plugin
7.51 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Part 4: Use better #include paths for libstagefright headers in a couple of places
1.81 KB, patch
glandium
: review+
Details | Diff | Splinter Review
part 3 v2
10.21 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Enable pragmas on non-B2G Android
2.65 KB, patch
glandium
: feedback+
Details | Diff | Splinter Review
Part 5 v3
9.22 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Attached patch roc's patch from bug 961264 (obsolete) — — Splinter Review
No description provided.
Comment on attachment 8412422 [details] [diff] [review] roc's patch from bug 961264 Review of attachment 8412422 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/stlport/src/dll_main.cpp @@ +222,5 @@ > + * would fix the original problem. On older NDKs, it is not a problem > + * either because the way __dso_handle was used was already broken (and > + * the custom linker works around it). > + */ > + NS_EXPORT __attribute__((weak)) void *__dso_handle; As mentioned on irc, I believe this would be placed better in gcc_hidden.h. Note that I don't think it really needs to be a weak reference, except where it currently is one (which may or may not be the cause of your problems on android 2.2 tests). Also, the comment is valid where it was, but it's unrelated here. ::: config/system-headers @@ +1148,5 @@ > +utils/Trace.h > +utils/TypeHelpers.h > +utils/Unicode.h > +utils/Vector.h > +utils/VectorImpl.h I think I'd prefer all those grouped and under a #if ANDROID or something ::: dom/system/gonk/android_audio/AudioSystem.h @@ +963,5 @@ > > }; // namespace android > > +#pragma GCC visibility pop > + If you need entire headers to be of visibility default, just add them to system_headers (we do have in-tree files in there) ::: netwerk/protocol/rtsp/rtsp/RTSPSource.h @@ +35,2 @@ > struct RtspConnectionHandler; > +struct MOZ_EXPORT AnotherPacketSource; I wonder why you're not doing that for the others instead of including all those headers. ::: widget/android/AndroidBridge.h @@ +37,5 @@ > class nsIDOMMozSmsMessage; > class nsIObserver; > > /* See the comment in AndroidBridge about this function before using it */ > +extern "C" __attribute__ ((visibility("default"))) include mozilla/Types.h and use MOZ_EXPORT?
BTW I don't have this working yet on try but I'd like to get some patches landed to reduce bitrot.
(In reply to Mike Hommey [:glandium] from comment #1) > ::: config/system-headers > @@ +1148,5 @@ > > +utils/Trace.h > > +utils/TypeHelpers.h > > +utils/Unicode.h > > +utils/Vector.h > > +utils/VectorImpl.h > > I think I'd prefer all those grouped and under a #if ANDROID or something Are you sure? We don't have any platform #ifdefs in system-headers right now.
Comment on attachment 8417808 [details] [diff] [review] Part 1: Add MOZ_EXPORT in various places Review of attachment 8417808 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but you should probably find someone to review the rtsp code changes because, aiui, it is imported from android, and i'm not sure it's fine to change like this.
Attachment #8417808 - Flags: review?(mh+mozilla) → review+
Attachment #8417809 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8417808 [details] [diff] [review] Part 1: Add MOZ_EXPORT in various places Review of attachment 8417808 [details] [diff] [review]: ----------------------------------------------------------------- Vincent, is it OK to change the RTSP header files like this?
Attachment #8417808 - Flags: review?(vchang)
Comment on attachment 8417812 [details] [diff] [review] Part 3: Add lots of system header wrappers for B2G/Android Review of attachment 8417812 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > Are you sure? We don't have any platform #ifdefs in system-headers right now. It's that way because, essentially, system-headers have been Linux-only so far.
Attachment #8417812 - Flags: review?(mh+mozilla) → review-
Attachment #8417813 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #11) > It's that way because, essentially, system-headers have been Linux-only so > far. There are tons of Mac headers in there.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12) > There are tons of Mac headers in there. Ah yes, from the old days when Mac builds were using gcc. But that doesn't change my opinion.
Attached patch part 3 v2 — — Splinter Review
Attachment #8417812 - Attachment is obsolete: true
Attachment #8417990 - Flags: review?(mh+mozilla)
Comment on attachment 8417990 [details] [diff] [review] part 3 v2 Review of attachment 8417990 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/system-headers @@ +648,5 @@ > lib$routines.h > limits > limits.h > link.h > +linux/android_alarm.h #ifdef ANDROID for this one and ashmem
Attachment #8417990 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8417808 [details] [diff] [review] Part 1: Add MOZ_EXPORT in various places Review of attachment 8417808 [details] [diff] [review]: ----------------------------------------------------------------- I don't know much about MOZ_EXPORT. But RTSP function still works well after applying the patches in B2G. So r+.
Attachment #8417808 - Flags: review?(vchang) → review+
Whiteboard: [leave open]
Tryserver run seems to show everything fine on Android. I'm separating this from B2G because I don't know how hard B2G is going to be to fix, and I don't want Android to regress while B2G gets fixed.
Attachment #8418551 - Flags: review?(mh+mozilla)
Comment on attachment 8418551 [details] [diff] [review] Enable pragmas on non-B2G Android Review of attachment 8418551 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +2549,5 @@ > dnl =============================================================== > if test "$GNU_CC"; then > AC_DEFINE(HAVE_VISIBILITY_HIDDEN_ATTRIBUTE) > AC_DEFINE(HAVE_VISIBILITY_ATTRIBUTE) > case "${OS_TARGET}" in I'd rather avoid the duplication and just do something like: case "${OS_TARGET}_${MOZ_WIDGET_TOOLKIT}" in Darwin_*|Android_gonk) ;; *) ;; esac
Attachment #8418551 - Flags: review?(mh+mozilla) → feedback+
We can't use MOZ_WIDGET_TOOLKIT here because it's not defined at this point in the script.
Flags: needinfo?(mh+mozilla)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #23) > We can't use MOZ_WIDGET_TOOLKIT here because it's not defined at this point > in the script. if test -n "$gonkdir"; then visibility_target=Gonk else visibility_target=$OS_TARGET fi case "$visibility_target" in Darwin|Gonk) ;; *) ;; esac
Flags: needinfo?(mh+mozilla)
Attached patch part 5 v2 (obsolete) — — Splinter Review
I don't think it's a big deal how we write this, since it's about the same amount of code and hopefully it will go away soon, but here you are.
Attachment #8419091 - Flags: review?(mh+mozilla)
Attachment #8419091 - Flags: review?(mh+mozilla) → review+
I think the remaining problem on B2G is that in https://tbpl.mozilla.org/php/getParsedLog.php?id=39180564&tree=Try&full=1#error0 AudioManager.cpp is somehow getting an unwrapped inclusion of AudioSystem.h. I can't see how this is happening.
For B2G, we're down to strzcmp16 being defined hidden in GonkPermission.o: https://tbpl.mozilla.org/?tree=Try&rev=5f09ad685378 The patch in that try push puts #include "utils/Unicode.h" as the first thing in GonkPermission.cpp, which (given utils/Unicode.h is in system-headers) should be enough to ensure that strzcmp16 is first declared with with default visibility. But it doesn't work. Locally, everything builds fine. Locally, I do see strzcmp16 being used by GonkPermission.cpp --- via binder/IPermissionController.h, which instantiates from IInterface.h this method: template<typename INTERFACE> inline sp<IInterface> BnInterface<INTERFACE>::queryLocalInterface( const String16& _descriptor) { if (_descriptor == INTERFACE::descriptor) return this; return NULL; } Maybe someone else could try building B2G locally with this patch? If we can reproduce the build error locally I'm pretty sure we can figure it out. (That try push shows a regression on Android but I'm pretty sure we can figure that out too once we have B2G working.)
Flags: needinfo?(mh+mozilla)
You could try to trick the build system into creating GonkPermission.i and add it to UPLOAD_EXTRA_FILES (could add GonPermission.o there too)
Flags: needinfo?(mh+mozilla)
The problem is that we pass -include gonk-misc/Unicode.h on the command line, which defines strzcmp16, and this is not wrapped: http://dxr.mozilla.org/mozilla-central/source/b2g/config/mozconfigs/ics_armv7a_gecko/debug#20
Bug 1011228 should fix the strzcmp16 problem.
Depends on: 1011228
The KK build keeps timing out. It doesn't seem related to my patches though --- it seems to be failing during an upload step.
It's failing during whatever step it happens to be in when the four hour total job time limit hits. There's a reason why the suggested bug is fast approaching 1000 tbplbot comments, and the reason certainly isn't "this is some rare and unusual thing caused by roc's patch". I'm tempted to say "just keep retriggering, you'll get green eventually" because it's true that you will, but... if you've gotten to make upload, you've gotten past the point where you have any stake in the result. Call it good enough, you've seen what you need to see, that it builds.
Comment on attachment 8424508 [details] [diff] [review] 1001320-b2g Review of attachment 8424508 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/configure.in @@ +2092,5 @@ > + 4.4*) > + VISIBILITY_FLAGS='-I$(DIST)/system_wrappers -include $(topsrcdir)/config/gcc_hidden_dso_handle.h' > + ;; > + *) > + VISIBILITY_FLAGS='-I$(DIST)/system_wrappers -include $(topsrcdir)/config/gcc_hidden.h' I assume this is what fixes the android issues?
Attachment #8424508 - Flags: review?(mh+mozilla) → review+
(In reply to Phil Ringnalda (:philor) from comment #35) > It's failing during whatever step it happens to be in when the four hour > total job time limit hits. There's a reason why the suggested bug is fast > approaching 1000 tbplbot comments, and the reason certainly isn't "this is > some rare and unusual thing caused by roc's patch". I'm tempted to say "just > keep retriggering, you'll get green eventually" because it's true that you > will, but... if you've gotten to make upload, you've gotten past the point > where you have any stake in the result. Call it good enough, you've seen > what you need to see, that it builds. Thanks! (In reply to Mike Hommey [:glandium] from comment #36) > ::: js/src/configure.in > @@ +2092,5 @@ > > + 4.4*) > > + VISIBILITY_FLAGS='-I$(DIST)/system_wrappers -include $(topsrcdir)/config/gcc_hidden_dso_handle.h' > > + ;; > > + *) > > + VISIBILITY_FLAGS='-I$(DIST)/system_wrappers -include $(topsrcdir)/config/gcc_hidden.h' > > I assume this is what fixes the android issues? Yes. It seems that on Android it's important we don't declare __dso_handle ourselves. Since the __dso_handle declaratoin only needed for 4.4 AFAWK, this seems like the best thing to do. We can just remove this check when we drop 4.4 support.
How come tryserver pushes don't show those builds? How can I reproduce those locally? What on earth is going on? :-)
Flags: needinfo?(mh+mozilla)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40) > How come tryserver pushes don't show those builds? According to their description, they are periodic builds. > How can I reproduce those locally? I don't know. Check the logs? For what they're doing? > What on earth is going on? :-) IIRC, it was the same error on android, so maybe there is something fishy with __dso_handle again for those builds. That being said, maybe there is also a bug in elfhack.
Flags: needinfo?(mh+mozilla)
Ah, elfhack was disabled locally in my B2G build for some reason. Hacking to force-enable it reproduces the issue locally.
Sure enough, it's __dso_handle. elfhack dies because the __dso_handle's section index is SHN_COMMON, which is not special-cased by ElfSymtab_Section::ElfSymtab_Section so we call getSection which treats it as out of range. Special-casing SHN_COMMON like SHN_ABS makes my build succeed, but I'm not sure what the right way is to fix this.
Flags: needinfo?(mh+mozilla)
Attached patch Part 5 v3 — — Splinter Review
Attachment #8424508 - Attachment is obsolete: true
Attachment #8426012 - Flags: review?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
For bug archaeologists: SHN_COMMON is a .o thing, so the elfhack problem was reading the injected code, not the library to elfhack. It's fine for elfhack not to support SHN_COMMON, what's not fine is ending up with SHN_COMMON symbols in the injected code.
Comment on attachment 8426012 [details] [diff] [review] Part 5 v3 Review of attachment 8426012 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/android_audio/AudioSystem.h @@ +16,5 @@ > > #ifndef ANDROID_AUDIOSYSTEM_H_ > #define ANDROID_AUDIOSYSTEM_H_ > > +#pragma GCC visibility push(default) Forgot to ask last time, doesn't wrapping this one work?
Attachment #8426012 - Flags: review?(mh+mozilla) → review+
Wrapping in-tree include files in system-headers doesn't generally work, because those files are included using "" instead of <>, so the compiler finds the in-tree header file before the wrapper.
Hmm, I guess it should because -I system_wrappers is the first thing on the command line, before any other include paths. But it did fail and I don't know why.
Ah, I think it was because the current directory is always searched first. And AudioManager.h #includes "android_audio/AudioSystem.h", which is found relative to the current directory, so that beats system_wrappers.
(In reply to Mike Hommey [:glandium] from comment #41) > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #40) > > How come tryserver pushes don't show those builds? > > According to their description, they are periodic builds. Other way around would be closer to the truth: they aren't run on try, so they're going to violate the tree rules, so why not violate them even more by not running them on-push? They aren't on try because device builds contain Seekrit Private Keep Out No Strangers Allowed code, and it requires virtually no effort to get access to push to try which would then enable you to steal Our Precious.
https://hg.mozilla.org/mozilla-central/rev/efa8579fe73a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
There are 2 products which are vixen & flatfish build error as the following log. Is this a toolchain problem? /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: /home/waynechen/projects/vixen/objdir-gecko/mozglue/build/Nuwa.o: requires unsupported dynamic reloc R_ARM_REL32; recompile with -fPIC /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: read-only segment has dynamic relocations /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '__dso_handle' is not defined locally /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '__dso_handle' is not defined locally /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '__dso_handle' is not defined locally /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '__dso_handle' is not defined locally /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '__dso_handle' is not defined locally /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '__dso_handle' is not defined locally /home/waynechen/projects/vixen/prebuilts/gcc/linux-x86/arm/arm-linux-androideabi-4.6/bin/../lib/gcc/arm-linux-androideabi/4.6.x-google/../../../../arm-linux-androideabi/bin/ld: error: hidden symbol '__dso_handle' is not defined locally collect2: ld returned 1 exit status make[6]: *** [libmozglue.so] Error 1 make[5]: *** [mozglue/build/libs] Error 2 make[4]: *** [libs] Error 2 make[3]: *** [default] Error 2 make[2]: *** [realbuild] Error 2 make[1]: *** [build] Error 2 make: *** [out/target/product/vixen/obj/DATA/gecko_intermediates/gecko] Error 2
Flags: needinfo?(roc)
In configure.in and js/src/configure.in there's case $GCC_VERSION in 4.4*) VISIBILITY_FLAGS='-I$(DIST)/system_wrappers -include $(topsrcdir)/config/gcc_hidden_dso_handle.h' Change that to include 4.6* I guess.
Flags: needinfo?(roc)
It works at vixen & flatfish product. But I wonder is it ok for other products?
There is another issue to tracking build error. bug 1014403
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: