Page MenuHomeFreeBSD

<type_traits>: Avoid instantiating a pointer type in std::decay<>.
ClosedPublic

Authored by jhb on Oct 6 2022, 11:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 26, 7:23 AM
Unknown Object (File)
Wed, Nov 20, 9:01 AM
Unknown Object (File)
Oct 18 2024, 11:15 AM
Unknown Object (File)
Oct 4 2024, 4:05 AM
Unknown Object (File)
Oct 4 2024, 4:05 AM
Unknown Object (File)
Sep 16 2024, 1:08 PM
Unknown Object (File)
Sep 9 2024, 12:56 AM
Unknown Object (File)
Aug 19 2024, 5:09 AM
Subscribers
None

Details

Summary

GCC expands the pointer type in this conditional expression even for
template types _Up that are not arrays. This raises an error when
std::decay<> is used with reference types (as is done in LLVM's
sources). Using add_pointer<> causes GCC to only instantiate a
pointer type for array types.

Upstream LLVM has at least one commit aimed at addressing this GCC
behavior, but it is not directly back-portable as upstream has also
split out type_traits into several internal headers. The upstream
commit (5fab33af7f083a0043112742027172e9f297c07f) also does not
include this fix here. I attempted to backport the equivalent
upstream fix (just using __remove_extent_t<> instead of typename
remove_extent<>) but it did not fix the compile error with GCC.

In file included from /usr/obj/usr/src/amd64.amd64/tmp/usr/include/c++/v1/__compare/ordering.h:13,
                 from /usr/obj/usr/src/amd64.amd64/tmp/usr/include/c++/v1/__compare/common_comparison_category.h:12,
                 from /usr/obj/usr/src/amd64.amd64/tmp/usr/include/c++/v1/tuple:168,
                 from /usr/src/contrib/llvm-project/llvm/include/llvm/ADT/DenseMapInfo.h:20,
                 from /usr/src/contrib/llvm-project/llvm/include/llvm/ADT/DenseMap.h:17,
                 from /usr/src/contrib/llvm-project/llvm/lib/Transforms/Scalar/GVNHoist.cpp:36:
/usr/obj/usr/src/amd64.amd64/tmp/usr/include/c++/v1/type_traits: In instantiation of 'struct std::__1::__decay<llvm::CHIArg&, true>':
/usr/obj/usr/src/amd64.amd64/tmp/usr/include/c++/v1/type_traits:1591:89:   required from 'struct std::__1::decay<llvm::CHIArg&&>'
/usr/obj/usr/src/amd64.amd64/tmp/usr/include/c++/v1/__utility/pair.h:132:16:   required by substitution of 'template<class _Tuple, typename std::__1::enable_if<typename std::__1::conditional<(std::__1::__tuple_like_with_size<_Tuple, 2, typename std::__1::__uncvref<_Tp>::type>::value && (! std::__1::is_same<typename std::__1::decay<_Tp>::type, std::__1::pair<llvm::BasicBlock*, llvm::SmallVector<llvm::CHIArg, 2> > >::value)), std::__1::pair<llvm::BasicBlock*, llvm::SmallVector<llvm::CHIArg, 2> >::_CheckTupleLikeConstructor, std::__1::__check_tuple_constructor_fail>::type::__enable_implicit<_Tuple>(), void>::type* <anonymous> > constexpr std::__1::pair<llvm::BasicBlock*, llvm::SmallVector<llvm::CHIArg, 2> >::pair(_Tuple&&) [with _Tuple = llvm::CHIArg&&; typename std::__1::enable_if<typename std::__1::conditional<(std::__1::__tuple_like_with_size<_Tuple, 2, typename std::__1::__uncvref<_Tp>::type>::value && (! std::__1::is_same<typename std::__1::decay<_Tp>::type, std::__1::pair<llvm::BasicBlock*, llvm::SmallVector<llvm::CHIArg, 2> > >::value)), std::__1::pair<llvm::BasicBlock*, llvm::SmallVector<llvm::CHIArg, 2> >::_CheckTupleLikeConstructor, std::__1::__check_tuple_constructor_fail>::type::__enable_implicit<_Tuple>(), void>::type* <anonymous> = <missing>]'
/usr/src/contrib/llvm-project/llvm/lib/Transforms/Scalar/GVNHoist.cpp:892:51:   required from here
/usr/obj/usr/src/amd64.amd64/tmp/usr/include/c++/v1/type_traits:1582:30: error: forming pointer to reference type 'std::__1::remove_extent<llvm::CHIArg&>::type' {aka 'llvm::CHIArg&'}
 1582 |                      >::type type;
      |                              ^~~~

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb requested review of this revision.Oct 6 2022, 11:44 PM
jhb created this revision.

Hm, I'd rather have this upstream instead of taking yet another diff in libc++. We already have too many customizations.

In D36898#837997, @dim wrote:

Hm, I'd rather have this upstream instead of taking yet another diff in libc++. We already have too many customizations.

I can't easily test this against upstream. I can come up with a patch, but I can't test it. In upstream the code is reorganized into separate files. However, something like this is required if we are going to permit using GCC to compile FreeBSD.

In D36898#838134, @jhb wrote:

https://reviews.llvm.org/D135469 is the upstream change

The upstream commit was accepted and I merged it yesterday. I'm not sure if that commit can be merged into the LLVM 15.x release branch which would maybe help minimize future conflicts?

I suspect it will not be merged back to 14.x though so we will need this as a local diff for now.

In D36898#851784, @jhb wrote:
In D36898#838134, @jhb wrote:

https://reviews.llvm.org/D135469 is the upstream change

The upstream commit was accepted and I merged it yesterday. I'm not sure if that commit can be merged into the LLVM 15.x release branch which would maybe help minimize future conflicts?

The last 15.x tag just went out (15.0.5), but historically people have still committed stuff on the release/R.x branches even after they were semi-officially closed. (Btw it can be done via GitHub now, I'll have a look.)

I suspect it will not be merged back to 14.x though so we will need this as a local diff for now.

14.x is really dormant, though not officially read-only, but if your patch applies without fuzz I see no problem having it locally.

Ehm, so please go ahead!

This revision is now accepted and ready to land.Nov 22 2022, 6:36 PM

So in 14 at least it's not a direct backport as type_traits has been split up into a bunch of separate headers now and the upstream fix also uses __add_pointer_t<> which doesn't exist in 14. I see the cherry-pick failed for 15, I'll do a manual branch and see if I can pacify that.

In D36898#851846, @jhb wrote:

So in 14 at least it's not a direct backport as type_traits has been split up into a bunch of separate headers now and the upstream fix also uses __add_pointer_t<> which doesn't exist in 14. I see the cherry-pick failed for 15, I'll do a manual branch and see if I can pacify that.

Yep, I was looking at the same. Ideally it shouldn't have to require backporting the whole lot of changes upstream did here.

Are you still ok with me committing this directly for now?

Sure, let's commit this now.