1248416 - std::forward (skia) vs. std::_LIBCPP_NAMESPACE::forward (libc++)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jan Beich]

gfx/skia/skia/include/private/SkTLogic.h contains |#define forward Forward| namespace pollution which breaks libc++ (C++ library from LLVM project). libc++ is default C++ library Bitrig, FreeBSD and OS X. VISIBILITY_FLAGS has no impact here, so I'm not sure why OS X isn't affected.

From |clang++ -stdlib=libc++|

  In file included from gfx/gl/SkiaGLGlue.cpp:8:
  In file included from obj/dist/include/mozilla/gfx/2D.h:10:
  In file included from obj/dist/include/mozilla/gfx/Point.h:13:
  In file included from obj/dist/include/mozilla/gfx/BasePoint.h:10:
  In file included from obj/dist/stl_wrappers/ostream:50:
  In file included from obj/dist/system_wrappers/ostream:3:
  In file included from /usr/include/c++/v1/ostream:138:
  In file included from obj/dist/stl_wrappers/ios:50:
  In file included from obj/dist/system_wrappers/ios:3:
  In file included from /usr/include/c++/v1/ios:216:
  In file included from /usr/include/c++/v1/__locale:15:
  In file included from obj/dist/stl_wrappers/string:50:
  In file included from obj/dist/system_wrappers/string:3:
  In file included from /usr/include/c++/v1/string:439:
  In file included from obj/dist/stl_wrappers/algorithm:50:
  In file included from obj/dist/system_wrappers/algorithm:3:
  In file included from /usr/include/c++/v1/algorithm:628:
  In file included from obj/dist/stl_wrappers/memory:50:
  In file included from obj/dist/system_wrappers/memory:3:
  In file included from /usr/include/c++/v1/memory:606:
  In file included from obj/dist/stl_wrappers/iterator:50:
  In file included from obj/dist/system_wrappers/iterator:3:
  In file included from /usr/include/c++/v1/iterator:343:
  /usr/include/c++/v1/__functional_base:370:18: error: no template named 'Forward' in namespace
	'std::__1'; did you mean simply 'Forward'?
      -> decltype((_VSTD::forward<_A0>(__a0).*__f)(_VSTD::forward<_Args>(__args)...))
		   ^~~~~~~
  /usr/include/c++/v1/__config:388:15: note: expanded from macro '_VSTD'
  #define _VSTD std::_LIBCPP_NAMESPACE
		^
  gfx/skia/skia/include/gpu/../private/SkTLogic.h:36:20: note: 'Forward' declared
	here
      using mozilla::Forward;
		     ^

From |g++ -nostdinc++ -isystem/usr/include/c++/v1|

  In file included from /usr/include/c++/v1/new:69:0,
		   from obj/dist/system_wrappers/new:3,
		   from obj/dist/stl_wrappers/new:28,
		   from gfx/skia/skia/include/core/SkPostConfig.h:104,
		   from gfx/skia/skia/include/core/SkTypes.h:14,
		   from gfx/skia/skia/include/gpu/GrTypes.h:11,
		   from gfx/skia/skia/include/gpu/GrCaps.h:11,
		   from gfx/skia/skia/include/gpu/GrContext.h:11,
		   from gfx/gl/SkiaGLGlue.cpp:6:
  /usr/include/c++/v1/__functional_base: At global scope:
  /usr/include/c++/v1/__functional_base:370:18: error: 'Forward' is not a member of 'std::__1'
       -> decltype((_VSTD::forward<_A0>(__a0).*__f)(_VSTD::forward<_Args>(__args)...))
		    ^
  /usr/include/c++/v1/__functional_base:370:18: note: suggested alternatives:
  In file included from gfx/skia/skia/include/core/../private/SkTLogic.h:28:0,
		   from gfx/skia/skia/include/core/../private/SkTemplates.h:14,
		   from gfx/skia/skia/include/core/SkTArray.h:11,
		   from gfx/skia/skia/include/gpu/GrTypesPriv.h:12,
		   from gfx/skia/skia/include/gpu/GrCaps.h:12,
		   from gfx/skia/skia/include/gpu/GrContext.h:11,
		   from gfx/gl/SkiaGLGlue.cpp:6:
  obj/dist/include/mozilla/Move.h:212:1: note:   'mozilla::Forward'
   Forward(typename RemoveReference<T>::Type& aX)
   ^
  obj/dist/include/mozilla/Move.h:219:1: note:   'mozilla::Forward'
   Forward(typename RemoveReference<T>::Type&& aX)
   ^

Comment 1

[Lee Salzman [:lsalzman]]

in gfx/skia/skia/include/private/SkTLogic.h, if you remove the #if SKIA_IMPLEMENTATION around the #include <algorithm>, does that work around it?

Comment 2

[Jan Beich]

(In reply to Lee Salzman [:lsalzman] from comment #1)
> remove the #if SKIA_IMPLEMENTATION

std::unique_ptr (skia) now conflicts with C++ library as well.

  In file included from gfx/gl/SkiaGLGlue.cpp:8:
  In file included from obj/dist/include/mozilla/gfx/2D.h:10:
  In file included from obj/dist/include/mozilla/gfx/Point.h:13:
  In file included from obj/dist/include/mozilla/gfx/BasePoint.h:10:
  In file included from obj/dist/stl_wrappers/ostream:50:
  In file included from obj/dist/system_wrappers/ostream:3:
  In file included from /usr/include/c++/v1/ostream:138:
  In file included from obj/dist/stl_wrappers/ios:50:
  In file included from obj/dist/system_wrappers/ios:3:
  In file included from /usr/include/c++/v1/ios:216:
  In file included from /usr/include/c++/v1/__locale:18:
  In file included from /usr/include/c++/v1/mutex:177:
  /usr/include/c++/v1/functional:1374:5: error: no template named 'UniquePtr'; did you mean 'mozilla::UniquePtr'?
      unique_ptr<__func, _Dp> __hold(__a.allocate(1), _Dp(__a, 1));
      ^
  gfx/skia/skia/include/core/../private/SkUniquePtr.h:22:24: note: expanded from macro 'unique_ptr'
      #define unique_ptr UniquePtr
			 ^
  obj/dist/include/mozilla/UniquePtr.h:154:7: note: 'mozilla::UniquePtr' declared here
  class UniquePtr
	^

libc++ bootlegs std::unique_ptr as follows

  In file included from gfx/gl/SkiaGLGlue.cpp:8:
  In file included from obj/dist/include/mozilla/gfx/2D.h:10:
  In file included from obj/dist/include/mozilla/gfx/Point.h:13:
  In file included from obj/dist/include/mozilla/gfx/BasePoint.h:10:
  In file included from obj/dist/stl_wrappers/ostream:50:
  In file included from obj/dist/system_wrappers/ostream:3:
  In file included from /usr/include/c++/v1/ostream:138:
  In file included from obj/dist/stl_wrappers/ios:50:
  In file included from obj/dist/system_wrappers/ios:3:
  In file included from /usr/include/c++/v1/ios:216:
  In file included from /usr/include/c++/v1/__locale:15:
  In file included from obj/dist/stl_wrappers/string:50:
  In file included from obj/dist/system_wrappers/string:3:
  In file included from /usr/include/c++/v1/string:439:
  In file included from obj/dist/stl_wrappers/algorithm:50:
  In file included from obj/dist/system_wrappers/algorithm:3:
  In file included from /usr/include/c++/v1/algorithm:628:
  In file included from obj/dist/stl_wrappers/memory:50:
  In file included from obj/dist/system_wrappers/memory:

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: Allow Skia to use C++11 features on platforms that have them

This patch should just avoid the whole mess by directly using the C++11 standard library where it is safe to do so. Consulted with Nathan Froyd on this, so should be okay.

Jan, can you try this and see if it works for you?

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: Allow Skia to use C++11 features on platforms that have them

Fix oops with MSVC detection.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: add __throw_bad_function_call stub for libstdc++ compat

Workaround to avoid check_stdcxx symbol nagging.

Comment 6

[Jan Beich]

Comment on attachment 8719936 [details] [diff] [review]
Allow Skia to use C++11 features on platforms that have them

It builds (and runs) fine with the patch. Only tested clang/libc++ but not gcc/libc++.

Comment 7

[Jan Beich]

Err, I'm talking about FreeBSD 10+ where /usr/bin/cc is clang.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8719975 [details] [diff] [review]
add __throw_bad_function_call stub for libstdc++ compat

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

This is fine as far as it goes, but in talking with Lee, this patch cannot land as-is due to peculiar errors in the clang plugin.

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: add __throw_bad_function_call stub for libstdc++ compat

This fixes up some missing headers so that we pull in symbols with the correct default/non-hidden visibility.

This is contingent upon the build fix for system header wrappers in bug 1249167 to work.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8720936 [details] [diff] [review]
add __throw_bad_function_call stub for libstdc++ compat

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

OK, so I went and looked at <functional> across libstdc++, libc++, and MSVC.  This is the right idea for the first one, but we have to handle MSVC too.

We have a header, memory/mozalloc/throw_gcc.h, where this stuff is handled for GCC, and we should be putting all new __throw_* stubs in there.  We may also want to drop a comment in stdc++compat.cpp pointing to the existence of that file for people who think they should be adding new __throw_* stubs in stdc++compat:

10:08 AM <froydnj> glandium: so we should use the mozalloc one for new __throw things?
10:08 AM <glandium> froydnj: definitely
10:08 AM <glandium> I completely forgot this existed

libc++ DTRT with regard to exceptions for throwing bad_function_call, so we can safely ignore that.

MSVC is a little more complicated.  <functional> defines _Xbad_function_call for hiding the details of |throw bad_function_call()|.  There's memory/mozalloc/msvc_raise_wrappers.{h,cpp} to handle these sorts of functions, so I think we want to follow the pattern of adding our own _Xbad_function_call wrapper there.  We'll want to add:

#ifdef _FUNCTIONAL_
#  error "Unable to wrap _Xbad_function_call(); CRT versions already declared"
#endif

at the top of the file, as _FUNCTIONAL_ is the include guard used.  What I'm not sure about is whether we should #include <functional> lower down, as is done for <xstddef> and <xutility>.  I *think* that since we're not dealing with an x-header (the x-headers are internal-only headers, AIUI), the check for _FUNCTIONAL_ is sufficient.  <xfunctional> exists, but doesn't define anything of this nature.

Comment 11

[Lee Salzman [:lsalzman]]

Attachment: add symbols for bad_function_call exception for C++ runtimes

This adds throw wrappers for libstdc++ and MSVC for bad_function_call.

Piggy-backing the missing istream header fix onto this change as well.

Scrubbing on the actual Skia incorporation for now since bug 1250196 handles that aspect a bit better temporarily.

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: add symbols for bad_function_call exception for C++ runtimes

Forgot to add functional to the header manifests. Fixed.

Comment 13

[Lee Salzman [:lsalzman]]

Since bug 1250196 landed, the original symptoms should be temporarily resolved, at least. Jan, can you confirm with latest central code?

Comment 14

[Jan Beich]

Yes, it builds fine now. Our buildbot passes SkiaGLGlue.o and stops for an unrelated reasons (bug 1239550 + bug 1250891).
http://buildbot.rhaalovely.net/builders/mozilla-central-freebsd-amd64/builds/759/steps/build/logs/stdio

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8722081 [details] [diff] [review]
add symbols for bad_function_call exception for C++ runtimes

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

::: memory/mozalloc/msvc_raise_wrappers.h
@@ +11,5 @@
>  #ifdef _XSTDDEF_
>  #  error "Unable to wrap _RAISE(); CRT _RAISE() already defined"
>  #endif
>  #ifdef _XUTILITY_
> +#  error "Unabled to wrap _X[exception](); CRT versions already declared"

Might as well fix the "Unabled..." bit while you're at it.

@@ +28,5 @@
>  #  define _Xlength_error      moz_Xlength_error
>  #  define _Xout_of_range      moz_Xout_of_range
>  #  define _Xoverflow_error    moz_Xoverflow_error
>  #  define _Xruntime_error     moz_Xruntime_error
> +#  define _Xbad_function_call moz_Xbad_function_call

Please add a comment prior to this that <functional> defines this one.

Comment 16

[Lee Salzman [:lsalzman]]

Try builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f29a5fada922

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6041201c9e88

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6041201c9e88