1277619 - fix system header wrappers to work with clang/libc++

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

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.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: hide libc++ visibility defines on Android when compiling with clang

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

Comment 3

[Nathan Froyd [:froydnj]]

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

Comment 4

[Mike Hommey [:glandium]]

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?

Comment 5

[Nathan Froyd [:froydnj]]

(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.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: hide libc++ visibility defines on Android when compiling with clang

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 7

[Mike Hommey [:glandium]]

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'

Comment 8

[Mike Hommey [:glandium]]

(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.

Comment 9

[Nathan Froyd [:froydnj]]

(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?

Comment 10

[Nathan Froyd [:froydnj]]

ni? glandium and jorendorff for comment 9.

Comment 11

[Nathan Froyd [:froydnj]]

(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.

Comment 12

[Jason Orendorff [:jorendorff]]

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".

Comment 13

[Mike Hommey [:glandium]]

(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)

Comment 14

[Mike Hommey [:glandium]]

(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).

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: hide libc++ visibility defines on Android when compiling with clang

OK, doing this in moz.configure...

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8169a53af11c

Comment 18

[Nicolas B. Pierron [:nbp]]

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