[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
Tue Jul 16 02:02:15 PDT 2024
https://github.com/kerambyte updated https://github.com/llvm/llvm-project/pull/98747
>From 2f4a7e7ec52d60c0f9cd57c0a9da5a5ccf9f2757 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/5] [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 49e0e8ab0db40..38a3829fb5137 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 d3c36176002505dff00b013d26c572b934a995a1 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/5] 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 fc856a976946b..80f7f3cf55665 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 1f59df8bee75577b50e45f9aca1033cc25707ae4 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/5] 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 80f7f3cf55665..01cdf499ac100 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 e1c35030f6f0407f6668ba8175fc7bcb3cd0375d 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/5] 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 01cdf499ac100..23be6be037aa9 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 926bd52f8eb9d7cf0e0be41f5376480da4f282d4 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/5] 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 38a3829fb5137..d92868e3715f4 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 fe1e2f4a5b89f..f6f657388048d 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 126054427b81a..390a59a2e3f14 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 23be6be037aa9..880a18b4481f9 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
More information about the llvm-commits
mailing list