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

RESOLVED FIXED in Firefox 47

Status

()

Core
Graphics
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jan Beich, Assigned: lsalzman)

Tracking

Trunk
mozilla47
Unspecified
FreeBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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)
   ^
(Assignee)

Comment 1

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

Updated

2 years ago
Flags: needinfo?(jbeich)
(Reporter)

Comment 2

2 years ago
(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:
Flags: needinfo?(jbeich)
(Assignee)

Comment 3

2 years ago
Created attachment 8719930 [details] [diff] [review]
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?
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Flags: needinfo?(jbeich)
Attachment #8719930 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

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

Fix oops with MSVC detection.
Attachment #8719930 - Attachment is obsolete: true
Attachment #8719930 - Flags: review?(jmuizelaar)
Attachment #8719936 - Flags: review?(jmuizelaar)
(Assignee)

Comment 5

2 years ago
Created attachment 8719975 [details] [diff] [review]
add __throw_bad_function_call stub for libstdc++ compat

Workaround to avoid check_stdcxx symbol nagging.
Attachment #8719975 - Flags: review?(nfroyd)
(Reporter)

Comment 6

2 years ago
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++.
Flags: needinfo?(jbeich)
Attachment #8719936 - Flags: feedback+
(Reporter)

Comment 7

2 years ago
Err, I'm talking about FreeBSD 10+ where /usr/bin/cc is clang.
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.
Attachment #8719975 - Flags: review?(nfroyd)
(Assignee)

Updated

2 years ago
Depends on: 1249167
(Assignee)

Comment 9

2 years ago
Created attachment 8720936 [details] [diff] [review]
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.
Attachment #8719975 - Attachment is obsolete: true
Attachment #8720936 - Flags: review?(nfroyd)
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.
Attachment #8720936 - Flags: review?(nfroyd) → review-
(Assignee)

Updated

2 years ago
See Also: → bug 1250196
(Assignee)

Comment 11

2 years ago
Created attachment 8722072 [details] [diff] [review]
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.
Attachment #8719936 - Attachment is obsolete: true
Attachment #8720936 - Attachment is obsolete: true
Attachment #8719936 - Flags: review?(jmuizelaar)
Attachment #8722072 - Flags: review?(nfroyd)
(Assignee)

Comment 12

2 years ago
Created attachment 8722081 [details] [diff] [review]
add symbols for bad_function_call exception for C++ runtimes

Forgot to add functional to the header manifests. Fixed.
Attachment #8722072 - Attachment is obsolete: true
Attachment #8722072 - Flags: review?(nfroyd)
Attachment #8722081 - Flags: review?(nfroyd)
(Assignee)

Comment 13

2 years ago
Since bug 1250196 landed, the original symptoms should be temporarily resolved, at least. Jan, can you confirm with latest central code?
Flags: needinfo?(jbeich)
(Reporter)

Comment 14

2 years ago
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
Flags: needinfo?(jbeich)
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.
Attachment #8722081 - Flags: review?(nfroyd) → review+

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6041201c9e88
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.