[LLVM][Support] Delete non-const lvalue ref "copy" constructor of BumpPtrAllocatorImpl

[zixuw]

Summary

Though the implicit copy constructor of BumpPtrAllocatorImpl is deleted
because of the presence of a user-defined move constructor, the
perfect-forwarding constructor will still match for lvalue reference of
BumpPtrAllocatorImpl, silently "copying" a BumpPtrAllocatorImpl
(actually just copy-constructing an underlying AllocatorT and
blank-constructing a BumpPtrAllocatorImpl).
This patch explicitly deletes the problematic specialization of the
forwarding constructor.

[zixuw]

I'm having trouble to find an appropriate place and approach to test this. Any ideas?

[JDevlieghere]

In D119580#3315572, @zixuw wrote:

I'm having trouble to find an appropriate place and approach to test this. Any ideas?

Maybe you can use the std::is_copy_constructible trait in a unit test?

[zixuw]

Add unit test case with std::is_[copy]_constructible

[efriedma]

llvm/include/llvm/Support/Allocator.h

85

This isn't using the right template parameters. I'd suggest just BumpPtrAllocatorImpl(const BumpPtrAllocatorImpl &) = delete;. Or I guess you could do something like template <typename AllocatorT, size_t SlabSize, size_t SizeThreshold, size_t GrowthDelay> BumpPtrAllocatorImpl(const BumpPtrAllocatorImpl<AllocatorT, SlabSize, SizeThreshold, GrowthDelay> &) = delete;.

[JDevlieghere]

I was curious if the standard specified anything about allocators being copy-constructable. I didn't actually check the standard itself, but cppreference seems to say so: https://en.cppreference.com/w/cpp/named_req/Allocator

Every Allocator also satisfies CopyConstructible.

[zixuw]

llvm/include/llvm/Support/Allocator.h

85

Ahh makes sense.

just BumpPtrAllocatorImpl(const BumpPtrAllocatorImpl &) = delete;

IIUC this would just delete the case where BumpPtrAllocatorImpl with the same template arguments right? I guess I should go with the full thing.

Also I need to drop the const apparently because otherwise the perfect-forwarding will always be a better match.

[zixuw]

In D119580#3315835, @JDevlieghere wrote:

I was curious if the standard specified anything about allocators being copy-constructable. I didn't actually check the standard itself, but cppreference seems to say so: https://en.cppreference.com/w/cpp/named_req/Allocator

Every Allocator also satisfies CopyConstructible.

Huh. Then I guess the "fix" would be to actually provide correct copy-constructing behaviors? Currently in this case, the implicit copy-constructor (BumpAllocatorImpl(const BumpAllocatorImpl &)) is deleted, and the forwarding constructor provides a match for BumpAllocatorImpl(BumpAllocatorImpl &) but does nothing.

[zixuw]

Fix template parameters.

[dblaikie]

llvm/unittests/Support/AllocatorTest.cpp

49–58

Perhaps this could be done with a static_assert in the header - no need for a test case/runtime expect of the value?

llvm/include/llvm/Support/Allocator.h

static_assert(GrowthDelay > 0,
                 "slab size after each allocated slab.");
 
   BumpPtrAllocatorImpl() = default;
 
   template <typename T>
   BumpPtrAllocatorImpl(T &&Allocator)
       : AllocatorT(std::forward<T &&>(Allocator)) {}
 
+  // BumpPtrAllocator shouldn't be copyable. Explicitly delete this
+  // specialization of the perfect-forwarding constructor above.
+  template <typename AT, size_t SS, size_t ST, size_t GD>
+  BumpPtrAllocatorImpl(BumpPtrAllocatorImpl<AT, SS, ST, GD> &) = delete;
+
   // Manually implement a move constructor as we must clear the old allocator's
   // slabs as a matter of correctness.
   BumpPtrAllocatorImpl(BumpPtrAllocatorImpl &&Old)
       : AllocatorT(static_cast<AllocatorT &&>(Old)), CurPtr(Old.CurPtr),
         End(Old.End), Slabs(std::move(Old.Slabs)),
         CustomSizedSlabs(std::move(Old.CustomSizedSlabs)),
         BytesAllocated(Old.BytesAllocated), RedZoneSize(Old.RedZoneSize) {
     Old.CurPtr = Old.End = nullptr;

llvm/unittests/Support/AllocatorTest.cpp

 //===- llvm/unittest/Support/AllocatorTest.cpp - BumpPtrAllocator tests ---===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Allocator.h"
 #include "gtest/gtest.h"
 #include <cstdlib>
+#include <type_traits>
 
 using namespace llvm;
 
 namespace {
 
 TEST(AllocatorTest, Basics) {
   BumpPtrAllocator Alloc;
   int *a = (int*)Alloc.Allocate(sizeof(int), alignof(int));
TEST(AllocatorTest, Basics) {
   EXPECT_EQ(2, b[9]);
   EXPECT_EQ(3, *c);
 
   Alloc = std::move(Alloc2);
   EXPECT_EQ(0U, Alloc2.GetNumSlabs());
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
 }
 
+TEST(AllocatorTest, NoCopy) {
+  constexpr bool CopyConstructible =
+      std::is_copy_constructible<BumpPtrAllocator>::value;
+  EXPECT_FALSE(CopyConstructible);
+
+  constexpr bool NonConstLRefConstructible =
+      std::is_constructible<BumpPtrAllocator, BumpPtrAllocator &>::value;
+  EXPECT_FALSE(NonConstLRefConstructible);
+}
+
 // Allocate enough bytes to create three slabs.
 TEST(AllocatorTest, ThreeSlabs) {
   BumpPtrAllocator Alloc;
   Alloc.Allocate(3000, 1);
   EXPECT_EQ(1U, Alloc.GetNumSlabs());
   Alloc.Allocate(3000, 1);
   EXPECT_EQ(2U, Alloc.GetNumSlabs());
   Alloc.Allocate(3000, 1);