libc++ has some hacks (some of which are Android-specific, but not all, apparently) in <cstdio> that look like this: #ifdef getchar inline _LIBCPP_INLINE_VISIBILITY int __libcpp_getchar(void) {return getchar();} #undef getchar inline _LIBCPP_INLINE_VISIBILITY int getchar(void) {return __libcpp_getchar();} #endif // getchar These exist because bionic's <stdio.h> defines getchar() as a function *and* a macro. This hack works OK for C code, but it doesn't work so well for C++ code when somebody writes: std::getchar() (Well, actually I think it works OK for *this* particular case, but it works less well when the macros refer to things that aren't being declared in std::) Our system wrappers interact with this in the following way: 1. <cstdio> is included. 2. <stdio.h> is included from <cstdio>. 2a. Our stdio.h wrapper is included, then the actual stdio.h. 3. Our wrapper does a: #pragma GCC visibility push(default) which gives things like getchar default visibility, as opposed to the hidden visibility they would get from the command line. 4. The compiler encounters the above definition of getchar, which has hidden visibility. GCC, AFAICT, happy munges the two together, keeping default visibility. clang, however, complains that we are changing the visibility of |getchar|, which it apparently does not allow. One might asks how this works without wrappers. I *think* that if one compiled: #include <cstdio> int f() { return ::getchar(); } normally (i.e. without our wrappers), clang doesn't complain, because the implicit default visibility that everything gets is treated differently than the "explicit" visibility push pragma above. This seems pedantic to me, but that's how this is. One way to get around this would be for the wrappers to undef the problematic macros in question. We would need to either modify the copy in NSPR or import it and modify it ourselves. I can't think of other solutions off the top of my head. I will try filing an NDK issue and seeing if they have suggestions.

Apparently GCC complains about this situation, too, but libc++ headers turn the _LIBCPP_INLINE_VISIBILITY annotation off for GCC. We can do: -D_LIBCPP_INLINE_VISIBILITY= and similar from the command line, even though that's screamingly ugly.

There's a very long comment that attempts to describe why we're doing this in the provided patch. Ideally that is good enough.

Bleh, this doesn't work when compiling js/ because we don't include mozilla-config.h there.

Comment on attachment 8759658 [details] [diff] [review] hide libc++ visibility defines on Android when compiling with clang Review of attachment 8759658 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla-config.h.in @@ +59,5 @@ > + * compiler as an explicit declaration of visibility on every function declared > + * in the header. Therefore, when the libc++ code above is encountered, it is > + * as though the compiler has effectively seen: > + * > + * int FUNC(...) __attribute__((__visibility__("default"))); I don't see what in the previous paragraphs would lead to this. Specifically, there is no description of anything declaring FUNC without _LIBCPP_INLINE_VISIBILITY, so where would the default visibility be coming from, if the declaration is always annotated with _LIBCPP_INLINE_VISIBILITY?

(In reply to Mike Hommey [:glandium] from comment #4) > Comment on attachment 8759658 [details] [diff] [review] > hide libc++ visibility defines on Android when compiling with clang > > Review of attachment 8759658 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mozilla-config.h.in > @@ +59,5 @@ > > + * compiler as an explicit declaration of visibility on every function declared > > + * in the header. Therefore, when the libc++ code above is encountered, it is > > + * as though the compiler has effectively seen: > > + * > > + * int FUNC(...) __attribute__((__visibility__("default"))); > > I don't see what in the previous paragraphs would lead to this. > Specifically, there is no description of anything declaring FUNC without > _LIBCPP_INLINE_VISIBILITY, so where would the default visibility be coming > from, if the declaration is always annotated with _LIBCPP_INLINE_VISIBILITY? If FUNC is getchar, the initial declaration comes from stdio.h (the visibility attribute is effectively added by the compiler). So <cstdio> includes <stdio.h>, and then tries to re-declare it with hidden visibility, and everything falls apart.

Hopefully my explanation in the previous comment was understandable. This is the same patch as before, but modified to work better with js/src/, as we don't include mozilla-config.h there.

Comment on attachment 8765016 [details] [diff] [review] hide libc++ visibility defines on Android when compiling with clang Review of attachment 8765016 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/RequiredDefines.h @@ +65,5 @@ > + * string (since we are properly managing visibility ourselves) and avoid this > + * whole mess. > + */ > +#if defined(__clang__) && defined(__ANDROID__) > +#define _LIBCPP_INLINE_VISIBILITY It seems to me this would be better as set_define()s in e.g. build/moz.configure/toolchain.configure, depending on target.os == 'Android' and c_compiler.type == 'clang'

(In reply to Mike Hommey [:glandium] from comment #7) > It seems to me this would be better as set_define()s in e.g. > build/moz.configure/toolchain.configure, depending on target.os == 'Android' > and c_compiler.type == 'clang' cxx_compiler, even.

(In reply to Mike Hommey [:glandium] from comment #7) > Comment on attachment 8765016 [details] [diff] [review] > hide libc++ visibility defines on Android when compiling with clang > > Review of attachment 8765016 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/public/RequiredDefines.h > @@ +65,5 @@ > > + * string (since we are properly managing visibility ourselves) and avoid this > > + * whole mess. > > + */ > > +#if defined(__clang__) && defined(__ANDROID__) > > +#define _LIBCPP_INLINE_VISIBILITY > > It seems to me this would be better as set_define()s in e.g. > build/moz.configure/toolchain.configure, depending on target.os == 'Android' > and c_compiler.type == 'clang' I guess doing it with defines limits the decision to just our code, whereas putting them in mozilla-config.h or similar makes it a policy decision for everything that might embed Gecko? Do we require embedders to use our system header wrappers and the like?

ni? glandium and jorendorff for comment 9.

(In reply to Mike Hommey [:glandium] from comment #7) > It seems to me this would be better as set_define()s in e.g. > build/moz.configure/toolchain.configure, depending on target.os == 'Android' > and c_compiler.type == 'clang' Also, how does this even work? You can't switch on c_compiler.type at the toplevel, and set_define isn't available inside a function.

I do not have the knowledge you seek (in comment 9), and I don't think anyone on the JS team knows that much about our build system. Tagging sfink just in case; he recently added a treeherder job that builds and links against SM "like an embedder would".

(In reply to Nathan Froyd [:froydnj] from comment #11) > (In reply to Mike Hommey [:glandium] from comment #7) > > It seems to me this would be better as set_define()s in e.g. > > build/moz.configure/toolchain.configure, depending on target.os == 'Android' > > and c_compiler.type == 'clang' > > Also, how does this even work? You can't switch on c_compiler.type at the > toplevel, and set_define isn't available inside a function. @depends(c_compiler, target) def libcpp_inline_visibility(c_compiler, target): if c_compiler.type == 'clang' and target.os == 'Android': return True set_define('_LIBCPP_INLINE_VISIBILITY', libcpp_inline_visibility)

(In reply to Nathan Froyd [:froydnj] from comment #9) > I guess doing it with defines limits the decision to just our code, whereas > putting them in mozilla-config.h or similar makes it a policy decision for > everything that might embed Gecko? Do we require embedders to use our > system header wrappers and the like? Embedders are forced to include RequiredDefines.h on js builds, but not mozilla-config.h on gecko builds. In neither builds are system header wrappers required (and most likely, they aren't used).

OK, doing this in moz.configure...

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8169a53af11c hide libc++ visibility defines on Android when compiling with clang; r=glandium

I can still reproduce the original issue on Linux, when using clang 3.7.1 with libc++ 3.7.1 [0]. I have to do that as otherwise, my system attempts to compile Firefox with the gcc 5.3 headers, which does not work because of compiler differences. The only working work-around [1] I found so-far comes from [2][3]. [0] https://github.com/nbp/firefox-build-env [1] https://github.com/nbp/firefox-build-env/blob/master/release.nix#L50 [2] https://llvm.org/bugs/show_bug.cgi?id=14435 [3] https://llvm.org/bugs/show_bug.cgi?id=12947