This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Fix QoI bug with construction of std::tuple involving std::any
ClosedPublic

Authored by ldionne on May 3 2021, 10:35 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Commits
rG17f2d1cb9b93: [libc++] Fix QoI bug with construction of std::tuple involving std::any
Summary

In std::tuple, we should try to avoid calling std::is_copy_constructible
whenever we can to avoid surprising interactions with (I believe) compiler
builtins. This bug was reported in https://reviews.llvm.org/D96523#2730953.

The issue was that when tuple<_Up...> was the same as tuple<_Tp...>, we
would short-circuit the _Or (because sizeof...(_Tp) != 1) and go evaluate
the following is_constructible<_Tp, const _Up&>....

Instead, after this patch, we check whether the constructed-from tuple
is the same as the current tuple regardless of the number of elements,
since we should always prefer the normal copy constructor in that case
anyway.

Diff Detail

Event Timeline

ldionne requested review of this revision.May 3 2021, 10:35 AM
ldionne created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 10:35 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone added inline comments.
libcxx/test/std/utilities/tuple/tuple.tuple/tuple.cnstr/copy.with_any.compile.pass.cpp
11–14

I'd like to see this test additionally test with std::any itself — since this is kind of a subtle interaction between the metaprogramming in both libc++ tuple and libc++ any, right? Could go in this same file, just under TEST_STD_VER >= 17; or could go into copy.with_std_any.compile.pass.cpp or something.

ldionne updated this revision to Diff 342735.May 4 2021, 7:47 AM
ldionne marked an inline comment as done.
ldionne retitled this revision from [libc++] Fix bug with construction of std::tuple involving std::any to [libc++] Fix QoI bug with construction of std::tuple involving std::any.
ldionne edited the summary of this revision. (Show Details)

Properly fix the bug.

ldionne updated this revision to Diff 342736.May 4 2021, 7:49 AM

Test buildkite job cancellation!

@ldionne: At first I didn't see why builtins were related, but I'm coming around to your point of view. Consider: https://www.godbolt.org/z/Y45bTadaq (which oddly fails with libstdc++ but not with libc++!)

Here, the compiler is trying to figure out whether B is copy-constructible. So one of the things it asks is whether the constructor B(any) is usable as a copy-constructor. So it asks whether any is convertible-from const B&. But the compiler shouldn't ask that, because the answer is obviously irrelevant in this context! Even if any were convertible-from const B&, that would be a user-defined conversion, which means we wouldn't be able to stack it with a second user-defined conversion B(any) in order to copy-construct a B object. So indeed, I think the compiler is wrong to ask whether any is convertible-from const B&. Clang's and GCC's builtins have identical behavior, though.

And there's a weird dependence on whether I use is_copy_constructible<T> versus is_constructible<T, const T&> in various places. I guess that's because we need to be in the middle of instantiating is_constructible<T, const T&> when we hit a call to is_copy_constructible<T> — the symptom is "is_copy_constructible has an incomplete base class."

And that implies that this would be a sufficient (although obviously not intellectually satisfying) fix for the original bug report:

diff --git a/libcxx/include/any b/libcxx/include/any
index aaeaab6c8f74..997f1656e157 100644
--- a/libcxx/include/any
+++ b/libcxx/include/any
@@ -204,7 +204,7 @@ public:
     , class = enable_if_t<
         !is_same<_Tp, any>::value &&
         !__is_inplace_type<_ValueType>::value &&
-        is_copy_constructible<_Tp>::value>
+        is_constructible<_Tp, const _Tp&>::value>
     >
   _LIBCPP_INLINE_VISIBILITY
   any(_ValueType && __value);

I've tested it with your test case and it does make the test case work (but of course only the std::any test case, not your original hand-rolled ::any).

ldionne accepted this revision as: Restricted Project.May 4 2021, 1:41 PM

@ldionne: At first I didn't see why builtins were related, but I'm coming around to your point of view. Consider: https://www.godbolt.org/z/Y45bTadaq (which oddly fails with libstdc++ but not with libc++!)

Here, the compiler is trying to figure out whether B is copy-constructible. So one of the things it asks is whether the constructor B(any) is usable as a copy-constructor. So it asks whether any is convertible-from const B&. But the compiler shouldn't ask that, because the answer is obviously irrelevant in this context! Even if any were convertible-from const B&, that would be a user-defined conversion, which means we wouldn't be able to stack it with a second user-defined conversion B(any) in order to copy-construct a B object. So indeed, I think the compiler is wrong to ask whether any is convertible-from const B&. Clang's and GCC's builtins have identical behavior, though.

And there's a weird dependence on whether I use is_copy_constructible<T> versus is_constructible<T, const T&> in various places. I guess that's because we need to be in the middle of instantiating is_constructible<T, const T&> when we hit a call to is_copy_constructible<T> — the symptom is "is_copy_constructible has an incomplete base class."

And that implies that this would be a sufficient (although obviously not intellectually satisfying) fix for the original bug report:

diff --git a/libcxx/include/any b/libcxx/include/any
index aaeaab6c8f74..997f1656e157 100644
--- a/libcxx/include/any
+++ b/libcxx/include/any
@@ -204,7 +204,7 @@ public:
     , class = enable_if_t<
         !is_same<_Tp, any>::value &&
         !__is_inplace_type<_ValueType>::value &&
-        is_copy_constructible<_Tp>::value>
+        is_constructible<_Tp, const _Tp&>::value>
     >
   _LIBCPP_INLINE_VISIBILITY
   any(_ValueType && __value);

I've tested it with your test case and it does make the test case work (but of course only the std::any test case, not your original hand-rolled ::any).

Very interesting, thanks for the analysis. I thought it was an issue with the builtins, but I didn't do the work of figuring out why exactly. I think we should open a bug report against Clang - do you want to do it or should I?

This revision was not accepted when it landed; it landed in state Needs Review.May 4 2021, 1:43 PM
This revision was automatically updated to reflect the committed changes.