[libc++] Better handling for zero-sized types

[• Quuxplusone]

Summary

Zero-sized types are a GCC extension, also supported by Clang.
It's already invalid to delete a void pointer or a pointer-to-incomplete,
so in theory we don't need any special code to catch those cases; but
for some reason Clang accepts both constructs with nothing more than
a warning (which is suppressed in system headers), so that's why the
static_assert isn't entirely removed.

In ranges::begin/end, check sizeof >= 0 instead of sizeof != 0,
so as to permit zero-sized types while still disallowing incomplete
types.

Fixes https://github.com/llvm/llvm-project/issues/54100

[philnik]

The tests should be in libcxx/, since it's an extension, right? Is there a reason you only check with make_unique? If you don't use make_unique the UNSUPPORTED: c++11 could be removed from the test.

libcxx/include/__memory/unique_ptr.h

50–53

Could you check the compiler diagnostics before and after?

[• Quuxplusone]

In D120633#3347916, @philnik wrote:

The tests should be in libcxx/, since it's an extension, right?

I'd say no, because it's a compiler extension (in multiple compilers), not a libc++ extension. I do expect the new tests to need UNSUPPORTED: msvc, but decided I could let buildkite tell me that instead of guessing ahead.

Is there a reason you only check with make_unique? If you don't use make_unique the UNSUPPORTED: c++11 could be removed from the test.

make_unique has its own overload set that we want to make sure works as expected. However, I suppose I should also check the behavior of std::default_delete<A> directly, in a new test, and that one can be enabled in C++11. Will fix.

libcxx/include/__memory/unique_ptr.h

50–53

Hmm, I guess the diagnostic for deleting an incomplete or void pointer is somehow just a warning! I wonder what the compiler thinks it means. (How could it possibly know how much memory to free, without a size?)

So I suppose it would be reasonable to keep the assert on lines 50-51, and just change its condition to sizeof(_Tp) >= 0. That'll still be false for void, so we still don't need the assert on lines 52-53.

Code:

#include <memory>

struct I;
void f(std::default_delete<I> d, I *p) { d(p); }
void f(std::default_delete<void> d, void *p) { d(p); }

void f(I *p) { delete p; }
void f(void *p) { delete p; }

Before (i.e. in main):

x.cpp:7:16: warning: deleting pointer to incomplete type 'I' may cause undefined behavior [-Wdelete-incomplete]
void f(I *p) { delete p; }
               ^      ~
x.cpp:3:8: note: forward declaration of 'I'
struct I;
       ^
x.cpp:8:19: warning: cannot delete expression with pointer-to-'void' type 'void *' [-Wdelete-incomplete]
void f(void *p) { delete p; }
                  ^      ~
In file included from x.cpp:1:
In file included from /Users/aodwyer/llvm-project/build2/bin/../include/c++/v1/memory:820:
In file included from /Users/aodwyer/llvm-project/build2/bin/../include/c++/v1/__memory/shared_ptr.h:24:
/Users/aodwyer/llvm-project/build2/bin/../include/c++/v1/__memory/unique_ptr.h:50:19: error: invalid application of 'sizeof' to an incomplete type 'I'
    static_assert(sizeof(_Tp) > 0,
                  ^~~~~~~~~~~
x.cpp:4:43: note: in instantiation of member function 'std::default_delete<I>::operator()' requested here
void f(std::default_delete<I> d, I *p) { d(p); }
                                          ^
x.cpp:3:8: note: forward declaration of 'I'
struct I;
       ^
In file included from x.cpp:1:
In file included from /Users/aodwyer/llvm-project/build2/bin/../include/c++/v1/memory:820:
In file included from /Users/aodwyer/llvm-project/build2/bin/../include/c++/v1/__memory/shared_ptr.h:24:
/Users/aodwyer/llvm-project/build2/bin/../include/c++/v1/__memory/unique_ptr.h:50:19: error: invalid application of 'sizeof' to an incomplete type 'void'
    static_assert(sizeof(_Tp) > 0,
                  ^~~~~~~~~~~
x.cpp:5:49: note: in instantiation of member function 'std::default_delete<void>::operator()' requested here
void f(std::default_delete<void> d, void *p) { d(p); }
                                                ^

After (i.e. with the current revision of this PR):

x.cpp:7:16: warning: deleting pointer to incomplete type 'I' may cause undefined behavior [-Wdelete-incomplete]
void f(I *p) { delete p; }
               ^      ~
x.cpp:3:8: note: forward declaration of 'I'
struct I;
       ^
x.cpp:8:19: warning: cannot delete expression with pointer-to-'void' type 'void *' [-Wdelete-incomplete]
void f(void *p) { delete p; }
                  ^      ~
2 warnings generated.

[• Quuxplusone]

Update per review comments.

[• Quuxplusone]

libcxx/include/__memory/unique_ptr.h

76–77

My substitution of _Up for _Tp here was accidental, but it turns out to be a bugfix: https://eel.is/c++draft/smartptr#unique.ptr.dltr.dflt1-4
I'm not sure the difference is detectable, though; I can't think of any situation where U(*)[] is convertible to T(*)[] and yet T's completeness is different from U's.

[philnik]

LGTM, assuming CI passes.

[Mordante]

libcxx/include/__memory/unique_ptr.h

49–50

I would like a comment explaining why sizeof(_Tp) == 0 is possible.

Looking at the number of sizeof(_Tp) >= 0 in the code, would it make sense to add a type trait __is_complete and add the explanation there?

libcxx/test/std/ranges/range.access/begin.sizezero.pass.cpp

24

I think it would be better to move this comment to line 11 to describe what the test is about.

Let's make sure the test also works on MSVC, where the size is 4. That will probably make @CaseyCarter happy.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique.sizezero.pass.cpp

12

Should there be a similar test for std::shared_ptr?

[• Quuxplusone]

libcxx/include/__memory/unique_ptr.h

49–50

I would like a comment explaining why sizeof(_Tp) == 0 is possible.

I'm not sure what such a comment would say. Would it just point to the regression test, like

// This is correct. See test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique.sizezero.pass.cpp

I mean, removing or changing the condition will end up pointing to that test anyway. For me, the important thing here is consistency: we want every instance of a completeness check across the whole codebase to use the same phrasing (sizeof(_Tp) >= 0), so that as long as we get it right once we get it right for every case.

Re concept __is_complete, see https://quuxplusone.github.io/blog/2021/12/27/libstdcxx-what-are-you-doing/ and D108645 and probably a few other passing mentions in different reviews. :)

libcxx/test/std/ranges/range.access/begin.sizezero.pass.cpp

24

Sure, I can say something like

// std::ranges::begin
// std::ranges::cbegin
//   Test an element type that is complete, yet has size zero.

I definitely don't want this test to silently compile when sizeof(A) > 0, because then someone might accidentally change m's declaration so that sizeof(A) > 0 on GCC and Clang, and then it would be silently nerfed. I don't object to adding XFAIL: msvc, though. And if @CaseyCarter knows a way to achieve sizeof(A) == 0 on MSVC, that'd be even better!

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique.sizezero.pass.cpp

12

shared_ptr doesn't have any similar completeness check, does it?

[CaseyCarter]

libcxx/test/std/ranges/range.access/begin.sizezero.pass.cpp

24

Arrays of size-zero elements are anathema. Feel free to XFAIL: msvc or UNSUPPORTED: msvc this test as you like =)

[• Quuxplusone]

Address review comments. UNSUPPORTED: msvc.

Restore the check for is_void too. Geez, these super-helpful compilers just let us delete anything, in a system header! I wonder what the generated code actually does at runtime...

https://reviews.llvm.org/harbormaster/unit/view/2823801/ is due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104568 so UNSUPPORTED: gcc-11 for this test (it's not super important to test, and we can reenable the test when GCC fixes their ICE)

[philnik]

In D120633#3350002, @Quuxplusone wrote:

Address review comments. UNSUPPORTED: msvc.

Restore the check for is_void too. Geez, these super-helpful compilers just let us delete anything, in a system header! I wonder what the generated code actually does at runtime...

https://reviews.llvm.org/harbormaster/unit/view/2823801/ is due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104568 so UNSUPPORTED: gcc-11 for this test (it's not super important to test, and we can reenable the test when GCC fixes their ICE)

You are also allowed to do this in user code. Good to know I should just add -Werror=delete-incomplete to any project. https://godbolt.org/z/vv194v6rs looks like the destructor just doesn't get called. Creepy.

[Mordante]

libcxx/include/__memory/unique_ptr.h

49–50

The problem I have with the test is that types with size 0 don't exist according to C++. So it warrants an explanation why we need to test for 0.

I'm not sure why you don't want a __is_complete concept, but use the incomplete in the wording of the assert. So it seems we need to be able to test for completeness in libc++ at some places. (I get the point of the blog post that we should be careful about whether we need to test for completeness everywhere.)

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique.sizezero.pass.cpp

12

No. I just wondered about it since it's the other smart pointer type.

[• Quuxplusone]

libcxx/include/__memory/unique_ptr.h

49–50

I'm not sure why you don't want a __is_complete concept [...] it seems we need to be able to test for completeness in libc++ at some places

We want to test for completeness right here, but we must scrupulously avoid caching/memoizing that computed result anywhere that might become stale later on in the TU (including in a concept, type trait, or variable template).
Godbolt link copied from D108645: https://godbolt.org/z/nr3dPY7va

[• Quuxplusone]

Rebase and poke CI.
I don't know what happened in https://buildkite.com/llvm-project/libcxx-ci/builds/9156 but it doesn't look like my fault. Let's see if this run is greener.

[Mordante]

libcxx/include/__memory/unique_ptr.h

49–50

Ah thanks, I forgot about the memoizing. I still think it would be good to add a comment why sizeof == 0 is intended.

libcxx/include/__memory/unique_ptr.h

#else
   _LIBCPP_INLINE_VISIBILITY default_delete() {}
 #endif
   template <class _Up>
   _LIBCPP_INLINE_VISIBILITY
   default_delete(const default_delete<_Up>&,
                  typename enable_if<is_convertible<_Up*, _Tp*>::value>::type* =
                      0) _NOEXCEPT {}
 
   _LIBCPP_INLINE_VISIBILITY void operator()(_Tp* __ptr) const _NOEXCEPT {
-    static_assert(sizeof(_Tp) > 0,
-                  "default_delete can not delete incomplete type");
-    static_assert(!is_void<_Tp>::value,
-                  "default_delete can not delete incomplete type");
     delete __ptr;
   }
 };
 
 template <class _Tp>
 struct _LIBCPP_TEMPLATE_VIS default_delete<_Tp[]> {
 private:
   template <class _Up>
   struct _EnableIfConvertible
#endif
   template <class _Up>
   _LIBCPP_INLINE_VISIBILITY
   default_delete(const default_delete<_Up[]>&,
                  typename _EnableIfConvertible<_Up>::type* = 0) _NOEXCEPT {}
 
   template <class _Up>
   _LIBCPP_INLINE_VISIBILITY
   typename _EnableIfConvertible<_Up>::type
   operator()(_Up* __ptr) const _NOEXCEPT {
-    static_assert(sizeof(_Tp) > 0,
-                  "default_delete can not delete incomplete type");
-    static_assert(!is_void<_Tp>::value,
-                  "default_delete can not delete void type");
     delete[] __ptr;
   }
 };
 
 template <class _Deleter>
 struct __unique_ptr_deleter_sfinae {
   static_assert(!is_reference<_Deleter>::value, "incorrect specialization");
   typedef const _Deleter& __lval_ref_type;
   typedef _Deleter&& __good_rval_ref_type;

libcxx/include/__ranges/access.h

concept __unqualified_begin =
     __class_or_enum<remove_cvref_t<_Tp>> &&
     requires(_Tp && __t) {
       { _LIBCPP_AUTO_CAST(begin(__t)) } -> input_or_output_iterator;
     };
 
   struct __fn {
     template <class _Tp>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[]) const noexcept
-      requires (sizeof(_Tp) != 0)  // Disallow incomplete element types.
+      requires (sizeof(_Tp) >= 0)  // Disallow incomplete element types.
     {
       return __t + 0;
     }
 
     template <class _Tp, size_t _Np>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[_Np]) const noexcept
-      requires (sizeof(_Tp) != 0)  // Disallow incomplete element types.
+      requires (sizeof(_Tp) >= 0)  // Disallow incomplete element types.
     {
       return __t + 0;
     }
 
     template <class _Tp>
       requires __member_begin<_Tp>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(_LIBCPP_AUTO_CAST(__t.begin())))
requires(_Tp && __t) {
       typename iterator_t<_Tp>;
       { _LIBCPP_AUTO_CAST(end(__t)) } -> sentinel_for<iterator_t<_Tp>>;
     };
 
   class __fn {
   public:
     template <class _Tp, size_t _Np>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp (&__t)[_Np]) const noexcept
-      requires (sizeof(_Tp) != 0)  // Disallow incomplete element types.
+      requires (sizeof(_Tp) >= 0)  // Disallow incomplete element types.
     {
       return __t + _Np;
     }
 
     template <class _Tp>
       requires __member_end<_Tp>
     [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Tp&& __t) const
       noexcept(noexcept(_LIBCPP_AUTO_CAST(__t.end())))

libcxx/test/std/ranges/range.access/begin.sizezero.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// std::ranges::begin
+// std::ranges::cbegin
+
+#include <ranges>
+#include <cassert>
+
+#include "test_macros.h"
+
+struct A {
+  alignas(int[0]) int ptr[0];
+};
+static_assert(sizeof(A) == 0);
+
+int main(int, char**)
+{
+  A a[10];
+  std::same_as<A*> auto p = std::ranges::begin(a);
+  assert(p == a);
+  std::same_as<const A*> auto cp = std::ranges::cbegin(a);
+  assert(cp == a);
+
+  return 0;
+}

libcxx/test/std/ranges/range.access/end.sizezero.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+
+// std::ranges::end
+// std::ranges::cend
+
+#include <ranges>
+#include <cassert>
+
+#include "test_macros.h"
+
+struct A {
+  alignas(int[0]) int ptr[0];
+};
+static_assert(sizeof(A) == 0);
+
+int main(int, char**)
+{
+  A a[10];
+  std::same_as<A*> auto p = std::ranges::end(a);
+  assert(p == a + 10);
+  std::same_as<const A*> auto cp = std::ranges::cend(a);
+  assert(cp == a + 10);
+
+  return 0;
+}

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique.sizezero.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11
+
+// Test the fix for https://llvm.org/PR54100
+
+#include <memory>
+#include <cassert>
+
+#include "test_macros.h"
+
+struct A {
+  alignas(int[0]) int ptr[0];
+};
+
+int main(int, char**)
+{
+  {
+    std::unique_ptr<A> p = std::make_unique<A>();
+    assert(p != nullptr);
+  }
+  {
+    std::unique_ptr<A[]> p = std::make_unique<A[]>(1);
+    assert(p != nullptr);
+  }
+  return 0;
+}