[llvm] [ADT] Add a missing call to a unique_function destructor after move (PR #98747)
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 19 11:57:52 PDT 2024
https://github.com/dwblaikie updated https://github.com/llvm/llvm-project/pull/98747
>From 9ed5469feed836b3666732207fd903b12f492a27 Mon Sep 17 00:00:00 2001
From: Dmitry Yanovsky <kerambyte at gmail.com>
Date: Sat, 13 Jul 2024 15:45:18 +0100
Subject: [PATCH 1/7] [ADT] Add a missing call to a unique_function destructor
after move
Right now immediately after moving the captured state, we `disarm` the moved-from object's destructor by resetting the `RHS.CallbackAndInlineFlag`. This means that we never properly destroy the RHS object's captured state.
---
llvm/include/llvm/ADT/FunctionExtras.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/llvm/include/llvm/ADT/FunctionExtras.h b/llvm/include/llvm/ADT/FunctionExtras.h
index 49e0e8ab0db400..38a3829fb51376 100644
--- a/llvm/include/llvm/ADT/FunctionExtras.h
+++ b/llvm/include/llvm/ADT/FunctionExtras.h
@@ -312,6 +312,7 @@ template <typename ReturnT, typename... ParamTs> class UniqueFunctionBase {
// Non-trivial move, so dispatch to a type-erased implementation.
getNonTrivialCallbacks()->MovePtr(getInlineStorage(),
RHS.getInlineStorage());
+ getNonTrivialCallbacks()->DestroyPtr(RHS.getInlineStorage());
}
// Clear the old callback and inline flag to get back to as-if-null.
>From 72c19829da13bb0bca25ea617a56ac5960c6fb9e Mon Sep 17 00:00:00 2001
From: kerambyte <kerambyte at gmail.com>
Date: Mon, 15 Jul 2024 13:03:51 +0100
Subject: [PATCH 2/7] add a unit test
---
llvm/unittests/ADT/FunctionExtrasTest.cpp | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/llvm/unittests/ADT/FunctionExtrasTest.cpp b/llvm/unittests/ADT/FunctionExtrasTest.cpp
index fc856a976946bf..80f7f3cf55665e 100644
--- a/llvm/unittests/ADT/FunctionExtrasTest.cpp
+++ b/llvm/unittests/ADT/FunctionExtrasTest.cpp
@@ -310,4 +310,22 @@ class Incomplete {};
Incomplete incompleteFunction() { return {}; }
const Incomplete incompleteFunctionConst() { return {}; }
+// Check that the moved-from captured state is properly destroyed during
+// move construction/assignment.
+TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
+ static int NumOfDestructorsCalled = 0;
+ struct State {
+ State() = default;
+ State(State &&) = default;
+ State &operator=(State &&) = default;
+ ~State() { ++NumOfDestructorsCalled; }
+ };
+ {
+ unique_function<void()> CapturingFunction{[state = State{}] {}};
+ unique_function<void()> CapturingFunctionMoved{
+ std::move(CapturingFunction)};
+ }
+ EXPECT_EQ(NumOfDestructorsCalled, 4);
+}
+
} // anonymous namespace
>From 0442dbd1a5ddb29c6c1c204181b84d98384c39fd Mon Sep 17 00:00:00 2001
From: kerambyte <kerambyte at gmail.com>
Date: Mon, 15 Jul 2024 17:13:54 +0100
Subject: [PATCH 3/7] refactor the test
---
llvm/unittests/ADT/FunctionExtrasTest.cpp | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/llvm/unittests/ADT/FunctionExtrasTest.cpp b/llvm/unittests/ADT/FunctionExtrasTest.cpp
index 80f7f3cf55665e..01cdf499ac100c 100644
--- a/llvm/unittests/ADT/FunctionExtrasTest.cpp
+++ b/llvm/unittests/ADT/FunctionExtrasTest.cpp
@@ -313,11 +313,14 @@ const Incomplete incompleteFunctionConst() { return {}; }
// Check that the moved-from captured state is properly destroyed during
// move construction/assignment.
TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
+ static int NumOfMovesCalled = 0;
static int NumOfDestructorsCalled = 0;
struct State {
State() = default;
- State(State &&) = default;
- State &operator=(State &&) = default;
+ State(const State &) = delete;
+ State(State &&) { ++NumOfMovesCalled; }
+ State &operator=(const State &) = delete;
+ State &operator=(State &&Rhs) { return *new (this) State{std::move(Rhs)}; }
~State() { ++NumOfDestructorsCalled; }
};
{
@@ -325,7 +328,7 @@ TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
unique_function<void()> CapturingFunctionMoved{
std::move(CapturingFunction)};
}
- EXPECT_EQ(NumOfDestructorsCalled, 4);
+ EXPECT_EQ(NumOfDestructorsCalled, 1 + NumOfMovesCalled);
}
} // anonymous namespace
>From 8348a98aeb4b2687d98d1a07d190c3d154db5e7c Mon Sep 17 00:00:00 2001
From: kerambyte <kerambyte at gmail.com>
Date: Mon, 15 Jul 2024 17:26:49 +0100
Subject: [PATCH 4/7] cleanup
---
llvm/unittests/ADT/FunctionExtrasTest.cpp | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/llvm/unittests/ADT/FunctionExtrasTest.cpp b/llvm/unittests/ADT/FunctionExtrasTest.cpp
index 01cdf499ac100c..23be6be037aa9d 100644
--- a/llvm/unittests/ADT/FunctionExtrasTest.cpp
+++ b/llvm/unittests/ADT/FunctionExtrasTest.cpp
@@ -317,10 +317,7 @@ TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
static int NumOfDestructorsCalled = 0;
struct State {
State() = default;
- State(const State &) = delete;
State(State &&) { ++NumOfMovesCalled; }
- State &operator=(const State &) = delete;
- State &operator=(State &&Rhs) { return *new (this) State{std::move(Rhs)}; }
~State() { ++NumOfDestructorsCalled; }
};
{
@@ -328,6 +325,7 @@ TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
unique_function<void()> CapturingFunctionMoved{
std::move(CapturingFunction)};
}
+ printf("%i, %i\n", NumOfMovesCalled, NumOfDestructorsCalled);
EXPECT_EQ(NumOfDestructorsCalled, 1 + NumOfMovesCalled);
}
>From 56f4cee7852c9d2d027308fb58846f662514007a Mon Sep 17 00:00:00 2001
From: kerambyte <kerambyte at gmail.com>
Date: Tue, 16 Jul 2024 10:00:55 +0100
Subject: [PATCH 5/7] refactor the test and fix alignment issue
---
llvm/include/llvm/ADT/FunctionExtras.h | 6 ++++--
llvm/unittests/ADT/CountCopyAndMove.cpp | 1 +
llvm/unittests/ADT/CountCopyAndMove.h | 10 ++++++++--
llvm/unittests/ADT/FunctionExtrasTest.cpp | 16 ++++++----------
4 files changed, 19 insertions(+), 14 deletions(-)
diff --git a/llvm/include/llvm/ADT/FunctionExtras.h b/llvm/include/llvm/ADT/FunctionExtras.h
index 38a3829fb51376..d92868e3715f40 100644
--- a/llvm/include/llvm/ADT/FunctionExtras.h
+++ b/llvm/include/llvm/ADT/FunctionExtras.h
@@ -80,6 +80,7 @@ using EnableIfCallable = std::enable_if_t<std::disjunction<
template <typename ReturnT, typename... ParamTs> class UniqueFunctionBase {
protected:
static constexpr size_t InlineStorageSize = sizeof(void *) * 3;
+ static constexpr size_t InlineStorageAlign = alignof(void *);
template <typename T, class = void>
struct IsSizeLessThanThresholdT : std::false_type {};
@@ -161,7 +162,8 @@ template <typename ReturnT, typename... ParamTs> class UniqueFunctionBase {
// provide three pointers worth of storage here.
// This is mutable as an inlined `const unique_function<void() const>` may
// still modify its own mutable members.
- alignas(void *) mutable std::byte InlineStorage[InlineStorageSize];
+ alignas(InlineStorageAlign) mutable std::byte
+ InlineStorage[InlineStorageSize];
} StorageUnion;
// A compressed pointer to either our dispatching callback or our table of
@@ -262,7 +264,7 @@ template <typename ReturnT, typename... ParamTs> class UniqueFunctionBase {
bool IsInlineStorage = true;
void *CallableAddr = getInlineStorage();
if (sizeof(CallableT) > InlineStorageSize ||
- alignof(CallableT) > alignof(decltype(StorageUnion.InlineStorage))) {
+ alignof(CallableT) > InlineStorageAlign) {
IsInlineStorage = false;
// Allocate out-of-line storage. FIXME: Use an explicit alignment
// parameter in C++17 mode.
diff --git a/llvm/unittests/ADT/CountCopyAndMove.cpp b/llvm/unittests/ADT/CountCopyAndMove.cpp
index fe1e2f4a5b89fd..f6f657388048d3 100644
--- a/llvm/unittests/ADT/CountCopyAndMove.cpp
+++ b/llvm/unittests/ADT/CountCopyAndMove.cpp
@@ -10,6 +10,7 @@
using namespace llvm;
+int CountCopyAndMove::Constructions = 0;
int CountCopyAndMove::CopyConstructions = 0;
int CountCopyAndMove::CopyAssignments = 0;
int CountCopyAndMove::MoveConstructions = 0;
diff --git a/llvm/unittests/ADT/CountCopyAndMove.h b/llvm/unittests/ADT/CountCopyAndMove.h
index 126054427b81a3..390a59a2e3f146 100644
--- a/llvm/unittests/ADT/CountCopyAndMove.h
+++ b/llvm/unittests/ADT/CountCopyAndMove.h
@@ -12,6 +12,7 @@
namespace llvm {
struct CountCopyAndMove {
+ static int Constructions;
static int CopyConstructions;
static int CopyAssignments;
static int MoveConstructions;
@@ -19,8 +20,8 @@ struct CountCopyAndMove {
static int Destructions;
int val;
- CountCopyAndMove() = default;
- explicit CountCopyAndMove(int val) : val(val) {}
+ CountCopyAndMove() { ++Constructions; }
+ explicit CountCopyAndMove(int val) : val(val) { ++Constructions; }
CountCopyAndMove(const CountCopyAndMove &other) : val(other.val) {
++CopyConstructions;
}
@@ -40,6 +41,7 @@ struct CountCopyAndMove {
~CountCopyAndMove() { ++Destructions; }
static void ResetCounts() {
+ Constructions = 0;
CopyConstructions = 0;
CopyAssignments = 0;
MoveConstructions = 0;
@@ -47,6 +49,10 @@ struct CountCopyAndMove {
Destructions = 0;
}
+ static int TotalConstructions() {
+ return Constructions + MoveConstructions + CopyConstructions;
+ }
+
static int TotalCopies() { return CopyConstructions + CopyAssignments; }
static int TotalMoves() { return MoveConstructions + MoveAssignments; }
diff --git a/llvm/unittests/ADT/FunctionExtrasTest.cpp b/llvm/unittests/ADT/FunctionExtrasTest.cpp
index 23be6be037aa9d..880a18b4481f96 100644
--- a/llvm/unittests/ADT/FunctionExtrasTest.cpp
+++ b/llvm/unittests/ADT/FunctionExtrasTest.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "llvm/ADT/FunctionExtras.h"
+#include "CountCopyAndMove.h"
#include "gtest/gtest.h"
#include <memory>
@@ -313,20 +314,15 @@ const Incomplete incompleteFunctionConst() { return {}; }
// Check that the moved-from captured state is properly destroyed during
// move construction/assignment.
TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
- static int NumOfMovesCalled = 0;
- static int NumOfDestructorsCalled = 0;
- struct State {
- State() = default;
- State(State &&) { ++NumOfMovesCalled; }
- ~State() { ++NumOfDestructorsCalled; }
- };
+ CountCopyAndMove::ResetCounts();
{
- unique_function<void()> CapturingFunction{[state = State{}] {}};
+ unique_function<void()> CapturingFunction{
+ [Counter = CountCopyAndMove{}] {}};
unique_function<void()> CapturingFunctionMoved{
std::move(CapturingFunction)};
}
- printf("%i, %i\n", NumOfMovesCalled, NumOfDestructorsCalled);
- EXPECT_EQ(NumOfDestructorsCalled, 1 + NumOfMovesCalled);
+ EXPECT_EQ(CountCopyAndMove::TotalConstructions(),
+ CountCopyAndMove::Destructions);
}
} // anonymous namespace
>From b54974e7ee1b27ac05550189c6c7015be21fcf2c Mon Sep 17 00:00:00 2001
From: kerambyte <kerambyte at gmail.com>
Date: Thu, 18 Jul 2024 00:10:01 +0100
Subject: [PATCH 6/7] remove alignment fix
---
llvm/include/llvm/ADT/FunctionExtras.h | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/llvm/include/llvm/ADT/FunctionExtras.h b/llvm/include/llvm/ADT/FunctionExtras.h
index d92868e3715f40..38a3829fb51376 100644
--- a/llvm/include/llvm/ADT/FunctionExtras.h
+++ b/llvm/include/llvm/ADT/FunctionExtras.h
@@ -80,7 +80,6 @@ using EnableIfCallable = std::enable_if_t<std::disjunction<
template <typename ReturnT, typename... ParamTs> class UniqueFunctionBase {
protected:
static constexpr size_t InlineStorageSize = sizeof(void *) * 3;
- static constexpr size_t InlineStorageAlign = alignof(void *);
template <typename T, class = void>
struct IsSizeLessThanThresholdT : std::false_type {};
@@ -162,8 +161,7 @@ template <typename ReturnT, typename... ParamTs> class UniqueFunctionBase {
// provide three pointers worth of storage here.
// This is mutable as an inlined `const unique_function<void() const>` may
// still modify its own mutable members.
- alignas(InlineStorageAlign) mutable std::byte
- InlineStorage[InlineStorageSize];
+ alignas(void *) mutable std::byte InlineStorage[InlineStorageSize];
} StorageUnion;
// A compressed pointer to either our dispatching callback or our table of
@@ -264,7 +262,7 @@ template <typename ReturnT, typename... ParamTs> class UniqueFunctionBase {
bool IsInlineStorage = true;
void *CallableAddr = getInlineStorage();
if (sizeof(CallableT) > InlineStorageSize ||
- alignof(CallableT) > InlineStorageAlign) {
+ alignof(CallableT) > alignof(decltype(StorageUnion.InlineStorage))) {
IsInlineStorage = false;
// Allocate out-of-line storage. FIXME: Use an explicit alignment
// parameter in C++17 mode.
>From 04ce60f668470d95ff8c85fe548d9585e249d53c Mon Sep 17 00:00:00 2001
From: kerambyte <kerambyte at gmail.com>
Date: Sun, 11 Aug 2024 10:55:58 +0100
Subject: [PATCH 7/7] update names
---
llvm/unittests/ADT/CountCopyAndMove.cpp | 3 ++-
llvm/unittests/ADT/CountCopyAndMove.h | 12 +++++++-----
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/llvm/unittests/ADT/CountCopyAndMove.cpp b/llvm/unittests/ADT/CountCopyAndMove.cpp
index f6f657388048d3..022dbee40dc6f1 100644
--- a/llvm/unittests/ADT/CountCopyAndMove.cpp
+++ b/llvm/unittests/ADT/CountCopyAndMove.cpp
@@ -10,7 +10,8 @@
using namespace llvm;
-int CountCopyAndMove::Constructions = 0;
+int CountCopyAndMove::DefaultConstructions = 0;
+int CountCopyAndMove::ValueConstructions = 0;
int CountCopyAndMove::CopyConstructions = 0;
int CountCopyAndMove::CopyAssignments = 0;
int CountCopyAndMove::MoveConstructions = 0;
diff --git a/llvm/unittests/ADT/CountCopyAndMove.h b/llvm/unittests/ADT/CountCopyAndMove.h
index 390a59a2e3f146..9f5048cc6107d9 100644
--- a/llvm/unittests/ADT/CountCopyAndMove.h
+++ b/llvm/unittests/ADT/CountCopyAndMove.h
@@ -12,7 +12,8 @@
namespace llvm {
struct CountCopyAndMove {
- static int Constructions;
+ static int DefaultConstructions;
+ static int ValueConstructions;
static int CopyConstructions;
static int CopyAssignments;
static int MoveConstructions;
@@ -20,8 +21,8 @@ struct CountCopyAndMove {
static int Destructions;
int val;
- CountCopyAndMove() { ++Constructions; }
- explicit CountCopyAndMove(int val) : val(val) { ++Constructions; }
+ CountCopyAndMove() { ++DefaultConstructions; }
+ explicit CountCopyAndMove(int val) : val(val) { ++ValueConstructions; }
CountCopyAndMove(const CountCopyAndMove &other) : val(other.val) {
++CopyConstructions;
}
@@ -41,7 +42,8 @@ struct CountCopyAndMove {
~CountCopyAndMove() { ++Destructions; }
static void ResetCounts() {
- Constructions = 0;
+ DefaultConstructions = 0;
+ ValueConstructions = 0;
CopyConstructions = 0;
CopyAssignments = 0;
MoveConstructions = 0;
@@ -50,7 +52,7 @@ struct CountCopyAndMove {
}
static int TotalConstructions() {
- return Constructions + MoveConstructions + CopyConstructions;
+ return DefaultConstructions + ValueConstructions + MoveConstructions + CopyConstructions;
}
static int TotalCopies() { return CopyConstructions + CopyAssignments; }
More information about the llvm-commits
mailing list