[llvm] [ADT] Add a missing call to a unique_function destructor after move (PR #98747)
Dmitry Yanovsky via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 19 12:22:50 PDT 2024
https://github.com/kerambyte updated https://github.com/llvm/llvm-project/pull/98747
>From e545ecefabf87b1d5f2d56754872407129e10634 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/9] [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 ad5c89cbc8bda7..d92868e3715f40 100644
--- a/llvm/include/llvm/ADT/FunctionExtras.h
+++ b/llvm/include/llvm/ADT/FunctionExtras.h
@@ -314,6 +314,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 ac0b93b3290a4eb8653ce48f12a62670a1b393da 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/9] 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 7abdc8b77e0595..afeabc4c1e30d3 100644
--- a/llvm/unittests/ADT/FunctionExtrasTest.cpp
+++ b/llvm/unittests/ADT/FunctionExtrasTest.cpp
@@ -329,4 +329,22 @@ TEST(UniqueFunctionTest, InlineStorageWorks) {
UniqueFunctionWithInlineStorage(&UniqueFunctionWithInlineStorage);
}
+// 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 7d5d4a6f60a324713ca898332f564ff2f046c74b 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/9] 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 afeabc4c1e30d3..6e599982759e3e 100644
--- a/llvm/unittests/ADT/FunctionExtrasTest.cpp
+++ b/llvm/unittests/ADT/FunctionExtrasTest.cpp
@@ -332,11 +332,14 @@ TEST(UniqueFunctionTest, InlineStorageWorks) {
// 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; }
};
{
@@ -344,7 +347,7 @@ TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
unique_function<void()> CapturingFunctionMoved{
std::move(CapturingFunction)};
}
- EXPECT_EQ(NumOfDestructorsCalled, 4);
+ EXPECT_EQ(NumOfDestructorsCalled, 1 + NumOfMovesCalled);
}
} // anonymous namespace
>From e9f39e8e318177a89d72a4f2ad4441535e0e1182 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/9] 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 6e599982759e3e..b5407322914b17 100644
--- a/llvm/unittests/ADT/FunctionExtrasTest.cpp
+++ b/llvm/unittests/ADT/FunctionExtrasTest.cpp
@@ -336,10 +336,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; }
};
{
@@ -347,6 +344,7 @@ TEST(UniqueFunctionTest, MovedFromStateIsDestroyedCorrectly) {
unique_function<void()> CapturingFunctionMoved{
std::move(CapturingFunction)};
}
+ printf("%i, %i\n", NumOfMovesCalled, NumOfDestructorsCalled);
EXPECT_EQ(NumOfDestructorsCalled, 1 + NumOfMovesCalled);
}
>From 74ea9a82e5b5e1ee0dffbbfa464a09dacac57cd8 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/9] refactor the test and fix alignment issue
---
llvm/unittests/ADT/CountCopyAndMove.cpp | 1 +
llvm/unittests/ADT/CountCopyAndMove.h | 10 ++++++++--
llvm/unittests/ADT/FunctionExtrasTest.cpp | 16 ++++++----------
3 files changed, 15 insertions(+), 12 deletions(-)
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 b5407322914b17..f64b888dbe1591 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>
@@ -332,20 +333,15 @@ TEST(UniqueFunctionTest, InlineStorageWorks) {
// 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 832e1825f963f5989e8a2c5c8d98e57161b1dc4d 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/9] 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 a6682afdfacab4df03ed013bc92d907999aecb83 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/9] 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; }
>From f9e0630516f870ef9b4c4b7487c004b76dc07324 Mon Sep 17 00:00:00 2001
From: kerambyte <kerambyte at gmail.com>
Date: Mon, 19 Aug 2024 20:14:01 +0100
Subject: [PATCH 8/9] fix formatting
---
llvm/unittests/ADT/CountCopyAndMove.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/llvm/unittests/ADT/CountCopyAndMove.h b/llvm/unittests/ADT/CountCopyAndMove.h
index 9f5048cc6107d9..c91d34bd8ebb50 100644
--- a/llvm/unittests/ADT/CountCopyAndMove.h
+++ b/llvm/unittests/ADT/CountCopyAndMove.h
@@ -52,7 +52,8 @@ struct CountCopyAndMove {
}
static int TotalConstructions() {
- return DefaultConstructions + ValueConstructions + MoveConstructions + CopyConstructions;
+ return DefaultConstructions + ValueConstructions + MoveConstructions +
+ CopyConstructions;
}
static int TotalCopies() { return CopyConstructions + CopyAssignments; }
>From adce33709e7082d3f50106848659887486b0d973 Mon Sep 17 00:00:00 2001
From: kerambyte <kerambyte at gmail.com>
Date: Mon, 19 Aug 2024 20:22:25 +0100
Subject: [PATCH 9/9] fix merge issue
---
llvm/include/llvm/ADT/FunctionExtras.h | 6 ++++--
1 file changed, 4 insertions(+), 2 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.
More information about the llvm-commits
mailing list