Skip to content

A
abseil-cpp
Commit d368d3d6 by Evan Brown Committed by Copybara-Service
Options

Add iterator invalidation checking for when the hashtable is moved. 

Motivation: once we enable small object optimization in swisstable, iterators can be invalidated when the table is moved.
PiperOrigin-RevId: 573884505
Change-Id: I4278129829143d3747dfd0ef0ff92f321c2633dc
parent 99bbd7c4
Showing with 45 additions and 15 deletions
absl/container/internal/raw_hash_set.h
...@@ -846,9 +846,10 @@ class CommonFieldsGenerationInfoEnabled { ...@@ -846,9 +846,10 @@ class CommonFieldsGenerationInfoEnabled {
if (reserved_growth_ > 0) { if (reserved_growth_ > 0) {
if (--reserved_growth_ == 0) reserved_growth_ = kReservedGrowthJustRanOut; if (--reserved_growth_ == 0) reserved_growth_ = kReservedGrowthJustRanOut;
} else { } else {
*generation_ = NextGeneration(*generation_); increment_generation();
} }
} }
void increment_generation() { *generation_ = NextGeneration(*generation_); }
void reset_reserved_growth(size_t reservation, size_t size) { void reset_reserved_growth(size_t reservation, size_t size) {
reserved_growth_ = reservation - size; reserved_growth_ = reservation - size;
} }
...@@ -895,6 +896,7 @@ class CommonFieldsGenerationInfoDisabled { ...@@ -895,6 +896,7 @@ class CommonFieldsGenerationInfoDisabled {
return false; return false;
} }
void maybe_increment_generation_on_insert() {} void maybe_increment_generation_on_insert() {}
void increment_generation() {}
void reset_reserved_growth(size_t, size_t) {} void reset_reserved_growth(size_t, size_t) {}
size_t reserved_growth() const { return 0; } size_t reserved_growth() const { return 0; }
void set_reserved_growth(size_t) {} void set_reserved_growth(size_t) {}
...@@ -1066,6 +1068,10 @@ class CommonFields : public CommonFieldsGenerationInfo { ...@@ -1066,6 +1068,10 @@ class CommonFields : public CommonFieldsGenerationInfo {
return CommonFieldsGenerationInfo:: return CommonFieldsGenerationInfo::
should_rehash_for_bug_detection_on_insert(control(), capacity()); should_rehash_for_bug_detection_on_insert(control(), capacity());
} }
void maybe_increment_generation_on_move() {
if (capacity() == 0) return;
increment_generation();
}
void reset_reserved_growth(size_t reservation) { void reset_reserved_growth(size_t reservation) {
CommonFieldsGenerationInfo::reset_reserved_growth(reservation, size()); CommonFieldsGenerationInfo::reset_reserved_growth(reservation, size());
} }
...@@ -1219,7 +1225,7 @@ inline void AssertIsFull(const ctrl_t* ctrl, GenerationType generation, ...@@ -1219,7 +1225,7 @@ inline void AssertIsFull(const ctrl_t* ctrl, GenerationType generation,
if (ABSL_PREDICT_FALSE(generation != *generation_ptr)) { if (ABSL_PREDICT_FALSE(generation != *generation_ptr)) {
ABSL_RAW_LOG(FATAL, ABSL_RAW_LOG(FATAL,
"%s called on invalid iterator. The table could have " "%s called on invalid iterator. The table could have "
"rehashed since this iterator was initialized.", "rehashed or moved since this iterator was initialized.",
operation); operation);
} }
if (ABSL_PREDICT_FALSE(!IsFull(*ctrl))) { if (ABSL_PREDICT_FALSE(!IsFull(*ctrl))) {
...@@ -1250,8 +1256,8 @@ inline void AssertIsValidForComparison(const ctrl_t* ctrl, ...@@ -1250,8 +1256,8 @@ inline void AssertIsValidForComparison(const ctrl_t* ctrl,
if (SwisstableGenerationsEnabled()) { if (SwisstableGenerationsEnabled()) {
if (ABSL_PREDICT_FALSE(generation != *generation_ptr)) { if (ABSL_PREDICT_FALSE(generation != *generation_ptr)) {
ABSL_RAW_LOG(FATAL, ABSL_RAW_LOG(FATAL,
"Invalid iterator comparison. The table could have " "Invalid iterator comparison. The table could have rehashed "
"rehashed since this iterator was initialized."); "or moved since this iterator was initialized.");
} }
if (ABSL_PREDICT_FALSE(!ctrl_is_valid_for_comparison)) { if (ABSL_PREDICT_FALSE(!ctrl_is_valid_for_comparison)) {
ABSL_RAW_LOG( ABSL_RAW_LOG(
...@@ -1338,8 +1344,8 @@ inline void AssertSameContainer(const ctrl_t* ctrl_a, const ctrl_t* ctrl_b, ...@@ -1338,8 +1344,8 @@ inline void AssertSameContainer(const ctrl_t* ctrl_a, const ctrl_t* ctrl_b,
ABSL_HARDENING_ASSERT( ABSL_HARDENING_ASSERT(
AreItersFromSameContainer(ctrl_a, ctrl_b, slot_a, slot_b) && AreItersFromSameContainer(ctrl_a, ctrl_b, slot_a, slot_b) &&
"Invalid iterator comparison. The iterators may be from different " "Invalid iterator comparison. The iterators may be from different "
"containers or the container might have rehashed. Consider running " "containers or the container might have rehashed or moved. Consider "
"with --config=asan to diagnose rehashing issues."); "running with --config=asan to diagnose issues.");
} }
} }
...@@ -1926,6 +1932,7 @@ class raw_hash_set { ...@@ -1926,6 +1932,7 @@ class raw_hash_set {
settings_(std::move(that.common()), that.hash_ref(), that.eq_ref(), settings_(std::move(that.common()), that.hash_ref(), that.eq_ref(),
that.alloc_ref()) { that.alloc_ref()) {
that.common() = CommonFields{}; that.common() = CommonFields{};
common().maybe_increment_generation_on_move();
} }
raw_hash_set(raw_hash_set&& that, const allocator_type& a) raw_hash_set(raw_hash_set&& that, const allocator_type& a)
...@@ -1935,6 +1942,7 @@ class raw_hash_set { ...@@ -1935,6 +1942,7 @@ class raw_hash_set {
} else { } else {
move_elements_allocs_unequal(std::move(that)); move_elements_allocs_unequal(std::move(that));
} }
common().maybe_increment_generation_on_move();
} }
raw_hash_set& operator=(const raw_hash_set& that) { raw_hash_set& operator=(const raw_hash_set& that) {
...@@ -2707,6 +2715,7 @@ class raw_hash_set { ...@@ -2707,6 +2715,7 @@ class raw_hash_set {
CopyAlloc(alloc_ref(), that.alloc_ref(), CopyAlloc(alloc_ref(), that.alloc_ref(),
std::integral_constant<bool, propagate_alloc>()); std::integral_constant<bool, propagate_alloc>());
that.common() = CommonFields{}; that.common() = CommonFields{};
common().maybe_increment_generation_on_move();
return *this; return *this;
} }
...@@ -2741,6 +2750,7 @@ class raw_hash_set { ...@@ -2741,6 +2750,7 @@ class raw_hash_set {
// TODO(b/296061262): move instead of copying hash/eq. // TODO(b/296061262): move instead of copying hash/eq.
hash_ref() = that.hash_ref(); hash_ref() = that.hash_ref();
eq_ref() = that.eq_ref(); eq_ref() = that.eq_ref();
common().maybe_increment_generation_on_move();
return move_elements_allocs_unequal(std::move(that)); return move_elements_allocs_unequal(std::move(that));
} }
......
absl/container/internal/raw_hash_set_allocator_test.cc
...@@ -284,32 +284,36 @@ TEST_F(NoPropagateOnCopy, CopyConstructorWithDifferentAlloc) { ...@@ -284,32 +284,36 @@ TEST_F(NoPropagateOnCopy, CopyConstructorWithDifferentAlloc) {
} }
TEST_F(PropagateOnAll, MoveConstructor) { TEST_F(PropagateOnAll, MoveConstructor) {
auto it = t1.insert(0).first; t1.insert(0);
Table u(std::move(t1)); Table u(std::move(t1));
auto it = u.begin();
EXPECT_EQ(1, a1.num_allocs()); EXPECT_EQ(1, a1.num_allocs());
EXPECT_EQ(0, it->num_moves()); EXPECT_EQ(0, it->num_moves());
EXPECT_EQ(0, it->num_copies()); EXPECT_EQ(0, it->num_copies());
} }
TEST_F(NoPropagateOnMove, MoveConstructor) { TEST_F(NoPropagateOnMove, MoveConstructor) {
auto it = t1.insert(0).first; t1.insert(0);
Table u(std::move(t1)); Table u(std::move(t1));
auto it = u.begin();
EXPECT_EQ(1, a1.num_allocs()); EXPECT_EQ(1, a1.num_allocs());
EXPECT_EQ(0, it->num_moves()); EXPECT_EQ(0, it->num_moves());
EXPECT_EQ(0, it->num_copies()); EXPECT_EQ(0, it->num_copies());
} }
TEST_F(PropagateOnAll, MoveConstructorWithSameAlloc) { TEST_F(PropagateOnAll, MoveConstructorWithSameAlloc) {
auto it = t1.insert(0).first; t1.insert(0);
Table u(std::move(t1), a1); Table u(std::move(t1), a1);
auto it = u.begin();
EXPECT_EQ(1, a1.num_allocs()); EXPECT_EQ(1, a1.num_allocs());
EXPECT_EQ(0, it->num_moves()); EXPECT_EQ(0, it->num_moves());
EXPECT_EQ(0, it->num_copies()); EXPECT_EQ(0, it->num_copies());
} }
TEST_F(NoPropagateOnMove, MoveConstructorWithSameAlloc) { TEST_F(NoPropagateOnMove, MoveConstructorWithSameAlloc) {
auto it = t1.insert(0).first; t1.insert(0);
Table u(std::move(t1), a1); Table u(std::move(t1), a1);
auto it = u.begin();
EXPECT_EQ(1, a1.num_allocs()); EXPECT_EQ(1, a1.num_allocs());
EXPECT_EQ(0, it->num_moves()); EXPECT_EQ(0, it->num_moves());
EXPECT_EQ(0, it->num_copies()); EXPECT_EQ(0, it->num_copies());
...@@ -378,9 +382,10 @@ TEST_F(NoPropagateOnCopy, CopyAssignmentWithDifferentAlloc) { ...@@ -378,9 +382,10 @@ TEST_F(NoPropagateOnCopy, CopyAssignmentWithDifferentAlloc) {
} }
TEST_F(PropagateOnAll, MoveAssignmentWithSameAlloc) { TEST_F(PropagateOnAll, MoveAssignmentWithSameAlloc) {
auto it = t1.insert(0).first; t1.insert(0);
Table u(0, a1); Table u(0, a1);
u = std::move(t1); u = std::move(t1);
auto it = u.begin();
EXPECT_EQ(a1, u.get_allocator()); EXPECT_EQ(a1, u.get_allocator());
EXPECT_EQ(1, a1.num_allocs()); EXPECT_EQ(1, a1.num_allocs());
EXPECT_EQ(0, it->num_moves()); EXPECT_EQ(0, it->num_moves());
...@@ -388,9 +393,10 @@ TEST_F(PropagateOnAll, MoveAssignmentWithSameAlloc) { ...@@ -388,9 +393,10 @@ TEST_F(PropagateOnAll, MoveAssignmentWithSameAlloc) {
} }
TEST_F(NoPropagateOnMove, MoveAssignmentWithSameAlloc) { TEST_F(NoPropagateOnMove, MoveAssignmentWithSameAlloc) {
auto it = t1.insert(0).first; t1.insert(0);
Table u(0, a1); Table u(0, a1);
u = std::move(t1); u = std::move(t1);
auto it = u.begin();
EXPECT_EQ(a1, u.get_allocator()); EXPECT_EQ(a1, u.get_allocator());
EXPECT_EQ(1, a1.num_allocs()); EXPECT_EQ(1, a1.num_allocs());
EXPECT_EQ(0, it->num_moves()); EXPECT_EQ(0, it->num_moves());
...@@ -398,9 +404,10 @@ TEST_F(NoPropagateOnMove, MoveAssignmentWithSameAlloc) { ...@@ -398,9 +404,10 @@ TEST_F(NoPropagateOnMove, MoveAssignmentWithSameAlloc) {
} }
TEST_F(PropagateOnAll, MoveAssignmentWithDifferentAlloc) { TEST_F(PropagateOnAll, MoveAssignmentWithDifferentAlloc) {
auto it = t1.insert(0).first; t1.insert(0);
Table u(0, a2); Table u(0, a2);
u = std::move(t1); u = std::move(t1);
auto it = u.begin();
EXPECT_EQ(a1, u.get_allocator()); EXPECT_EQ(a1, u.get_allocator());
EXPECT_EQ(1, a1.num_allocs()); EXPECT_EQ(1, a1.num_allocs());
EXPECT_EQ(0, a2.num_allocs()); EXPECT_EQ(0, a2.num_allocs());
...@@ -409,10 +416,10 @@ TEST_F(PropagateOnAll, MoveAssignmentWithDifferentAlloc) { ...@@ -409,10 +416,10 @@ TEST_F(PropagateOnAll, MoveAssignmentWithDifferentAlloc) {
} }
TEST_F(NoPropagateOnMove, MoveAssignmentWithDifferentAlloc) { TEST_F(NoPropagateOnMove, MoveAssignmentWithDifferentAlloc) {
auto it = t1.insert(0).first; t1.insert(0);
Table u(0, a2); Table u(0, a2);
u = std::move(t1); u = std::move(t1);
it = u.find(0); auto it = u.find(0);
EXPECT_EQ(a2, u.get_allocator()); EXPECT_EQ(a2, u.get_allocator());
EXPECT_EQ(1, a1.num_allocs()); EXPECT_EQ(1, a1.num_allocs());
EXPECT_EQ(1, a2.num_allocs()); EXPECT_EQ(1, a2.num_allocs());
......
absl/container/internal/raw_hash_set_test.cc
...@@ -2370,6 +2370,19 @@ TEST(Iterator, InvalidUseWithReserveCrashesWithSanitizers) { ...@@ -2370,6 +2370,19 @@ TEST(Iterator, InvalidUseWithReserveCrashesWithSanitizers) {
#endif #endif
} }
TEST(Iterator, InvalidUseWithMoveCrashesWithSanitizers) {
if (!SwisstableGenerationsEnabled()) GTEST_SKIP() << "Generations disabled.";
if (kMsvc) GTEST_SKIP() << "MSVC doesn't support | in regexp.";
IntTable t1, t2;
t1.insert(1);
auto it = t1.begin();
t2 = std::move(t1);
EXPECT_DEATH_IF_SUPPORTED(*it, kInvalidIteratorDeathMessage);
EXPECT_DEATH_IF_SUPPORTED(void(it == t2.begin()),
kInvalidIteratorDeathMessage);
}
TEST(Table, ReservedGrowthUpdatesWhenTableDoesntGrow) { TEST(Table, ReservedGrowthUpdatesWhenTableDoesntGrow) {
IntTable t; IntTable t;
for (int i = 0; i < 8; ++i) t.insert(i); for (int i = 0; i < 8; ++i) t.insert(i);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment