common: RefBase fixes and improvements
Fix missing Release() on RefBase::operator=(const T&).
Always acquire the new reference before dropping the existing reference.
Consider:
* Object A holds the only reference to object B.
* Ref of A is assigned B.
If A were released before referencing B, then B would be a dead pointer.
Remove RefBase::kNullValue, just use Traits::kNullValue. Avoids an unnecessary constexpr copy.
Add even more tests.
Change-Id: I111079d56cbf8c09162aa6e493078bead9ae97fa
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/55882
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>
diff --git a/src/common/RefBase.h b/src/common/RefBase.h
index 495a24f..6ba4f50 100644
--- a/src/common/RefBase.h
+++ b/src/common/RefBase.h
@@ -33,17 +33,13 @@
// RefBase supports
template <typename T, typename Traits>
class RefBase {
- private:
- static constexpr T kNullValue = Traits::kNullValue;
-
public:
// Default constructor and destructor.
- RefBase() : mValue(kNullValue) {
+ RefBase() : mValue(Traits::kNullValue) {
}
~RefBase() {
- Release();
- mValue = kNullValue;
+ Release(mValue);
}
// Constructors from nullptr.
@@ -51,49 +47,39 @@
}
RefBase<T, Traits>& operator=(std::nullptr_t) {
- Release();
- mValue = kNullValue;
+ Set(Traits::kNullValue);
return *this;
}
// Constructors from a value T.
RefBase(T value) : mValue(value) {
- Reference();
+ Reference(value);
}
RefBase<T, Traits>& operator=(const T& value) {
- mValue = value;
- Reference();
+ Set(value);
return *this;
}
// Constructors from a RefBase<T>
RefBase(const RefBase<T, Traits>& other) : mValue(other.mValue) {
- Reference();
+ Reference(other.mValue);
}
RefBase<T, Traits>& operator=(const RefBase<T, Traits>& other) {
- if (&other != this) {
- other.Reference();
- Release();
- mValue = other.mValue;
- }
-
+ Set(other.mValue);
return *this;
}
RefBase(RefBase<T, Traits>&& other) {
- mValue = other.mValue;
- other.mValue = kNullValue;
+ mValue = other.Detach();
}
RefBase<T, Traits>& operator=(RefBase<T, Traits>&& other) {
if (&other != this) {
- Release();
- mValue = other.mValue;
- other.mValue = kNullValue;
+ Release(mValue);
+ mValue = other.Detach();
}
-
return *this;
}
@@ -102,14 +88,12 @@
// operators defined with `other` == RefBase<T, Traits>.
template <typename U, typename UTraits, typename = typename std::is_convertible<U, T>::type>
RefBase(const RefBase<U, UTraits>& other) : mValue(other.mValue) {
- Reference();
+ Reference(other.mValue);
}
template <typename U, typename UTraits, typename = typename std::is_convertible<U, T>::type>
RefBase<T, Traits>& operator=(const RefBase<U, UTraits>& other) {
- other.Reference();
- Release();
- mValue = other.mValue;
+ Set(other.mValue);
return *this;
}
@@ -120,7 +104,7 @@
template <typename U, typename UTraits, typename = typename std::is_convertible<U, T>::type>
RefBase<T, Traits>& operator=(RefBase<U, UTraits>&& other) {
- Release();
+ Release(mValue);
mValue = other.Detach();
return *this;
}
@@ -150,18 +134,18 @@
}
T Detach() DAWN_NO_DISCARD {
- T value = mValue;
- mValue = kNullValue;
+ T value{std::move(mValue)};
+ mValue = Traits::kNullValue;
return value;
}
void Acquire(T value) {
- Release();
+ Release(mValue);
mValue = value;
}
T* InitializeInto() DAWN_NO_DISCARD {
- ASSERT(mValue == kNullValue);
+ ASSERT(mValue == Traits::kNullValue);
return &mValue;
}
@@ -171,14 +155,24 @@
template <typename U, typename UTraits>
friend class RefBase;
- void Reference() const {
- if (mValue != kNullValue) {
- Traits::Reference(mValue);
+ static void Reference(T value) {
+ if (value != Traits::kNullValue) {
+ Traits::Reference(value);
}
}
- void Release() const {
- if (mValue != kNullValue) {
- Traits::Release(mValue);
+ static void Release(T value) {
+ if (value != Traits::kNullValue) {
+ Traits::Release(value);
+ }
+ }
+
+ void Set(T value) {
+ if (mValue != value) {
+ // Ensure that the new value is referenced before the old is released to prevent any
+ // transitive frees that may affect the new value.
+ Reference(value);
+ Release(mValue);
+ mValue = value;
}
}
diff --git a/src/tests/BUILD.gn b/src/tests/BUILD.gn
index a0c6e89..9a26592 100644
--- a/src/tests/BUILD.gn
+++ b/src/tests/BUILD.gn
@@ -174,6 +174,7 @@
"unittests/PerStageTests.cpp",
"unittests/PerThreadProcTests.cpp",
"unittests/PlacementAllocatedTests.cpp",
+ "unittests/RefBaseTests.cpp",
"unittests/RefCountedTests.cpp",
"unittests/ResultTests.cpp",
"unittests/RingBufferAllocatorTests.cpp",
diff --git a/src/tests/unittests/ObjectBaseTests.cpp b/src/tests/unittests/ObjectBaseTests.cpp
index ab141d3..51244c7 100644
--- a/src/tests/unittests/ObjectBaseTests.cpp
+++ b/src/tests/unittests/ObjectBaseTests.cpp
@@ -120,6 +120,24 @@
ASSERT_EQ(refcount, 2);
}
+// Test the repeated copy assignment of C++ objects
+TEST(ObjectBase, RepeatedCopyAssignment) {
+ int refcount = 1;
+ Object source(&refcount);
+
+ Object destination;
+ for (int i = 0; i < 10; i++) {
+ destination = source;
+ }
+
+ ASSERT_EQ(source.Get(), &refcount);
+ ASSERT_EQ(destination.Get(), &refcount);
+ ASSERT_EQ(3, refcount);
+
+ destination = Object();
+ ASSERT_EQ(refcount, 2);
+}
+
// Test the copy assignment of C++ objects onto themselves
TEST(ObjectBase, CopyAssignmentSelf) {
int refcount = 1;
diff --git a/src/tests/unittests/RefBaseTests.cpp b/src/tests/unittests/RefBaseTests.cpp
new file mode 100644
index 0000000..27fd922
--- /dev/null
+++ b/src/tests/unittests/RefBaseTests.cpp
@@ -0,0 +1,319 @@
+// Copyright 2021 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <gmock/gmock.h>
+
+#include "common/RefBase.h"
+
+namespace {
+ using Id = uint32_t;
+
+ enum class Action {
+ kReference,
+ kRelease,
+ kAssign,
+ kMarker,
+ };
+
+ struct Event {
+ Action action;
+ Id thisId = 0;
+ Id otherId = 0;
+ };
+
+ std::ostream& operator<<(std::ostream& os, const Event& event) {
+ switch (event.action) {
+ case Action::kReference:
+ os << "Reference " << event.thisId;
+ break;
+ case Action::kRelease:
+ os << "Release " << event.thisId;
+ break;
+ case Action::kAssign:
+ os << "Assign " << event.thisId << " <- " << event.otherId;
+ break;
+ case Action::kMarker:
+ os << "Marker " << event.thisId;
+ break;
+ }
+ return os;
+ }
+
+ bool operator==(const Event& a, const Event& b) {
+ return a.action == b.action && a.thisId == b.thisId && a.otherId == b.otherId;
+ }
+
+ using Events = std::vector<Event>;
+
+ struct RefTracker {
+ explicit constexpr RefTracker(nullptr_t) : mId(0), mEvents(nullptr) {
+ }
+
+ constexpr RefTracker(const RefTracker& other) = default;
+
+ RefTracker(Id id, Events* events) : mId(id), mEvents(events) {
+ }
+
+ void Reference() const {
+ mEvents->emplace_back(Event{Action::kReference, mId});
+ }
+
+ void Release() const {
+ mEvents->emplace_back(Event{Action::kRelease, mId});
+ }
+
+ RefTracker& operator=(const RefTracker& other) {
+ if (mEvents || other.mEvents) {
+ Events* events = mEvents ? mEvents : other.mEvents;
+ events->emplace_back(Event{Action::kAssign, mId, other.mId});
+ }
+ mId = other.mId;
+ mEvents = other.mEvents;
+ return *this;
+ }
+
+ bool operator==(const RefTracker& other) const {
+ return mId == other.mId;
+ }
+
+ bool operator!=(const RefTracker& other) const {
+ return mId != other.mId;
+ }
+
+ Id mId;
+ Events* mEvents;
+ };
+
+ struct RefTrackerTraits {
+ static constexpr RefTracker kNullValue{nullptr};
+
+ static void Reference(const RefTracker& handle) {
+ handle.Reference();
+ }
+
+ static void Release(const RefTracker& handle) {
+ handle.Release();
+ }
+ };
+
+ constexpr RefTracker RefTrackerTraits::kNullValue;
+
+ using Ref = RefBase<RefTracker, RefTrackerTraits>;
+} // namespace
+
+TEST(RefBase, Acquire) {
+ Events events;
+ RefTracker tracker1(1, &events);
+ RefTracker tracker2(2, &events);
+ Ref ref(tracker1);
+
+ events.clear();
+ { ref.Acquire(tracker2); }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kRelease, 1}, // release ref
+ Event{Action::kAssign, 1, 2} // acquire tracker2
+ ));
+}
+
+TEST(RefBase, Detach) {
+ Events events;
+ RefTracker tracker(1, &events);
+ Ref ref(tracker);
+
+ events.clear();
+ { DAWN_UNUSED(ref.Detach()); }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kAssign, 1, 0} // nullify ref
+ ));
+}
+
+TEST(RefBase, Constructor) {
+ Ref ref;
+ EXPECT_EQ(ref.Get(), RefTrackerTraits::kNullValue);
+}
+
+TEST(RefBase, ConstructDestruct) {
+ Events events;
+ RefTracker tracker(1, &events);
+
+ events.clear();
+ {
+ Ref ref(tracker);
+ events.emplace_back(Event{Action::kMarker, 10});
+ }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1}, // reference tracker
+ Event{Action::kMarker, 10}, //
+ Event{Action::kRelease, 1} // destruct ref
+ ));
+}
+
+TEST(RefBase, CopyConstruct) {
+ Events events;
+ RefTracker tracker(1, &events);
+ Ref refA(tracker);
+
+ events.clear();
+ {
+ Ref refB(refA);
+ events.emplace_back(Event{Action::kMarker, 10});
+ }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1}, // reference tracker
+ Event{Action::kMarker, 10}, //
+ Event{Action::kRelease, 1} // destruct ref
+ ));
+}
+
+TEST(RefBase, RefCopyAssignment) {
+ Events events;
+ RefTracker tracker1(1, &events);
+ RefTracker tracker2(2, &events);
+ Ref refA(tracker1);
+ Ref refB(tracker2);
+
+ events.clear();
+ {
+ Ref ref;
+ events.emplace_back(Event{Action::kMarker, 10});
+ ref = refA;
+ events.emplace_back(Event{Action::kMarker, 20});
+ ref = refB;
+ events.emplace_back(Event{Action::kMarker, 30});
+ ref = refA;
+ events.emplace_back(Event{Action::kMarker, 40});
+ }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kMarker, 10}, //
+ Event{Action::kReference, 1}, // reference tracker1
+ Event{Action::kAssign, 0, 1}, // copy tracker1
+ Event{Action::kMarker, 20}, //
+ Event{Action::kReference, 2}, // reference tracker2
+ Event{Action::kRelease, 1}, // release tracker1
+ Event{Action::kAssign, 1, 2}, // copy tracker2
+ Event{Action::kMarker, 30}, //
+ Event{Action::kReference, 1}, // reference tracker1
+ Event{Action::kRelease, 2}, // release tracker2
+ Event{Action::kAssign, 2, 1}, // copy tracker1
+ Event{Action::kMarker, 40}, //
+ Event{Action::kRelease, 1} // destruct ref
+ ));
+}
+
+TEST(RefBase, RefMoveAssignment) {
+ Events events;
+ RefTracker tracker1(1, &events);
+ RefTracker tracker2(2, &events);
+ Ref refA(tracker1);
+ Ref refB(tracker2);
+
+ events.clear();
+ {
+ Ref ref;
+ events.emplace_back(Event{Action::kMarker, 10});
+ ref = std::move(refA);
+ events.emplace_back(Event{Action::kMarker, 20});
+ ref = std::move(refB);
+ events.emplace_back(Event{Action::kMarker, 30});
+ }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kMarker, 10}, //
+ Event{Action::kAssign, 1, 0}, // nullify refA
+ Event{Action::kAssign, 0, 1}, // move into ref
+ Event{Action::kMarker, 20}, //
+ Event{Action::kRelease, 1}, // release tracker1
+ Event{Action::kAssign, 2, 0}, // nullify refB
+ Event{Action::kAssign, 1, 2}, // move into ref
+ Event{Action::kMarker, 30}, //
+ Event{Action::kRelease, 2} // destruct ref
+ ));
+}
+
+TEST(RefBase, RefCopyAssignmentSelf) {
+ Events events;
+ RefTracker tracker(1, &events);
+ Ref ref(tracker);
+ Ref& self = ref;
+
+ events.clear();
+ {
+ ref = self;
+ ref = self;
+ ref = self;
+ }
+ EXPECT_THAT(events, testing::ElementsAre());
+}
+
+TEST(RefBase, RefMoveAssignmentSelf) {
+ Events events;
+ RefTracker tracker(1, &events);
+ Ref ref(tracker);
+ Ref& self = ref;
+
+ events.clear();
+ {
+ ref = std::move(self);
+ ref = std::move(self);
+ ref = std::move(self);
+ }
+ EXPECT_THAT(events, testing::ElementsAre());
+}
+
+TEST(RefBase, TCopyAssignment) {
+ Events events;
+ RefTracker tracker(1, &events);
+ Ref ref;
+
+ events.clear();
+ {
+ ref = tracker;
+ ref = tracker;
+ ref = tracker;
+ }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1}, //
+ Event{Action::kAssign, 0, 1}));
+}
+
+TEST(RefBase, TMoveAssignment) {
+ Events events;
+ RefTracker tracker(1, &events);
+ Ref ref;
+
+ events.clear();
+ { ref = std::move(tracker); }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1}, //
+ Event{Action::kAssign, 0, 1}));
+}
+
+TEST(RefBase, TCopyAssignmentAlternate) {
+ Events events;
+ RefTracker tracker1(1, &events);
+ RefTracker tracker2(2, &events);
+ Ref ref;
+
+ events.clear();
+ {
+ ref = tracker1;
+ events.emplace_back(Event{Action::kMarker, 10});
+ ref = tracker2;
+ events.emplace_back(Event{Action::kMarker, 20});
+ ref = tracker1;
+ events.emplace_back(Event{Action::kMarker, 30});
+ }
+ EXPECT_THAT(events, testing::ElementsAre(Event{Action::kReference, 1}, // reference tracker1
+ Event{Action::kAssign, 0, 1}, // copy tracker1
+ Event{Action::kMarker, 10}, //
+ Event{Action::kReference, 2}, // reference tracker2
+ Event{Action::kRelease, 1}, // release tracker1
+ Event{Action::kAssign, 1, 2}, // copy tracker2
+ Event{Action::kMarker, 20}, //
+ Event{Action::kReference, 1}, // reference tracker1
+ Event{Action::kRelease, 2}, // release tracker2
+ Event{Action::kAssign, 2, 1}, // copy tracker1
+ Event{Action::kMarker, 30}));
+}
