Skip to content

Commit 1e8927d

Browse files
caspernorrbinjdksjolen
caspernorrbin
authored and
jdksjolen
committed
8354433: Assert in AbstractRBTree::visit_range_in_order(const K& from, const K& to, F f) is wrong
Reviewed-by: jsjolen, aboldtch
1 parent 7f3191a commit 1e8927d

File tree

3 files changed

+58
-41
lines changed

3 files changed

+58
-41
lines changed

‎src/hotspot/share/utilities/rbTree.hpp

Lines changed: 22 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,22 @@
3737
// - an int < 0 when a < b
3838
// - an int == 0 when a == b
3939
// - an int > 0 when a > b
40-
// A second static function `cmp(const IntrusiveRBNode* a, const IntrusiveRBNode* b)`
41-
// used for `verify_self` and other extra validation can optionally be provided. This should return:
42-
// - true if a < b
43-
// - false otherwise
40+
// Additional static functions used for extra validation can optionally be provided:
41+
// `cmp(K a, K b)` which returns:
42+
// - an int < 0 when a < b
43+
// - an int == 0 when a == b
44+
// - an int > 0 when a > b
45+
// `cmp(const IntrusiveRBNode* a, const IntrusiveRBNode* b)` which returns:
46+
// - true if a < b
47+
// - false otherwise
4448
// K needs to be of a type that is trivially destructible.
4549
// K needs to be stored by the user and is not stored inside the tree.
4650
// Nodes are address stable and will not change during its lifetime.
4751

4852
// A red-black tree is constructed with four template parameters:
4953
// K is the key type stored in the tree nodes.
5054
// V is the value type stored in the tree nodes.
51-
// COMPARATOR must have one of the static functions `cmp(K a, K b)` or `cmp(K a, const RBNode<K, V>* b)` which returns:
55+
// COMPARATOR must have a static function `cmp(K a, K b)` which returns:
5256
// - an int < 0 when a < b
5357
// - an int == 0 when a == b
5458
// - an int > 0 when a > b
@@ -198,20 +202,20 @@ class AbstractRBTree {
198202
struct has_cmp_type<CMP, RET, ARG1, ARG2, decltype(static_cast<RET(*)(ARG1, ARG2)>(CMP::cmp), void())> : std::true_type {};
199203

200204
template <typename CMP>
201-
static constexpr bool IsKeyComparator = has_cmp_type<CMP, int, K, K>::value;
205+
static constexpr bool HasKeyComparator = has_cmp_type<CMP, int, K, K>::value;
202206

203207
template <typename CMP>
204-
static constexpr bool IsNodeComparator = has_cmp_type<CMP, int, K, const NodeType*>::value;
208+
static constexpr bool HasNodeComparator = has_cmp_type<CMP, int, K, const NodeType*>::value;
205209

206210
template <typename CMP>
207211
static constexpr bool HasNodeVerifier = has_cmp_type<CMP, bool, const NodeType*, const NodeType*>::value;
208212

209-
template <typename CMP = COMPARATOR, ENABLE_IF(IsKeyComparator<CMP>)>
213+
template <typename CMP = COMPARATOR, ENABLE_IF(HasKeyComparator<CMP> && !HasNodeComparator<CMP>)>
210214
int cmp(const K& a, const NodeType* b) const {
211215
return COMPARATOR::cmp(a, b->key());
212216
}
213217

214-
template <typename CMP = COMPARATOR, ENABLE_IF(IsNodeComparator<CMP>)>
218+
template <typename CMP = COMPARATOR, ENABLE_IF(HasNodeComparator<CMP>)>
215219
int cmp(const K& a, const NodeType* b) const {
216220
return COMPARATOR::cmp(a, b);
217221
}
@@ -226,24 +230,13 @@ class AbstractRBTree {
226230
return COMPARATOR::cmp(a, b);
227231
}
228232

229-
template <typename CMP = COMPARATOR, ENABLE_IF(IsKeyComparator<CMP>)>
230-
void assert_leq(const K& a, const NodeType* b) const {
231-
assert(COMPARATOR::cmp(a, b->key()) <= 0, "key not <= node");
232-
}
233-
234-
template <typename CMP = COMPARATOR, ENABLE_IF(IsNodeComparator<CMP>)>
235-
void assert_leq(const K& a, const NodeType* b) const {
236-
assert(COMPARATOR::cmp(a, b) <= 0, "key not <= node");
237-
}
238-
239-
template <typename CMP = COMPARATOR, ENABLE_IF(IsKeyComparator<CMP>)>
240-
void assert_geq(const K& a, const NodeType* b) const {
241-
assert(COMPARATOR::cmp(a, b->key()) >= 0, "key not >= node");
242-
}
233+
// Cannot assert if no key comparator exist.
234+
template <typename CMP = COMPARATOR, ENABLE_IF(!HasKeyComparator<CMP>)>
235+
void assert_key_leq(K a, K b) const {}
243236

244-
template <typename CMP = COMPARATOR, ENABLE_IF(IsNodeComparator<CMP>)>
245-
void assert_geq(const K& a, const NodeType* b) const {
246-
assert(COMPARATOR::cmp(a, b) >= 0, "key not >= node");
237+
template <typename CMP = COMPARATOR, ENABLE_IF(HasKeyComparator<CMP>)>
238+
void assert_key_leq(K a, K b) const {
239+
assert(COMPARATOR::cmp(a, b) <= 0, "key a must be less or equal to key b");
247240
}
248241

249242
// True if node is black (nil nodes count as black)
@@ -272,7 +265,7 @@ class AbstractRBTree {
272265

273266
AbstractRBTree() : _num_nodes(0), _root(nullptr) DEBUG_ONLY(COMMA _expected_visited(false)) {
274267
static_assert(std::is_trivially_destructible<K>::value, "key type must be trivially destructable");
275-
static_assert(IsKeyComparator<COMPARATOR> || IsNodeComparator<COMPARATOR>,
268+
static_assert(HasKeyComparator<COMPARATOR> || HasNodeComparator<COMPARATOR>,
276269
"comparator must be of correct type");
277270
}
278271

@@ -425,12 +418,12 @@ class AbstractRBTree {
425418
verify_self([](const NodeType* a, const NodeType* b){ return COMPARATOR::cmp(a, b);});
426419
}
427420

428-
template <typename CMP = COMPARATOR, ENABLE_IF(IsKeyComparator<CMP> && !HasNodeVerifier<CMP>)>
421+
template <typename CMP = COMPARATOR, ENABLE_IF(HasKeyComparator<CMP> && !HasNodeVerifier<CMP>)>
429422
void verify_self() const {
430423
verify_self([](const NodeType* a, const NodeType* b){ return COMPARATOR::cmp(a->key(), b->key()) < 0; });
431424
}
432425

433-
template <typename CMP = COMPARATOR, ENABLE_IF(IsNodeComparator<CMP> && !HasNodeVerifier<CMP>)>
426+
template <typename CMP = COMPARATOR, ENABLE_IF(HasNodeComparator<CMP> && !HasKeyComparator<CMP> && !HasNodeVerifier<CMP>)>
434427
void verify_self() const {
435428
verify_self([](const NodeType*, const NodeType*){ return true;});
436429
}

‎src/hotspot/share/utilities/rbTree.inline.hpp

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -550,19 +550,19 @@ inline void AbstractRBTree<K, NodeType, COMPARATOR>::replace_at_cursor(NodeType*
550550
new_node->_parent = old_node->_parent;
551551

552552
if (new_node->is_left_child()) {
553-
assert(cmp((const NodeType*)new_node, (const NodeType*)new_node->_parent), "new node not < parent");
553+
assert(cmp(static_cast<const NodeType*>(new_node), static_cast<const NodeType*>(new_node->parent())), "new node not < parent");
554554
} else if (new_node->is_right_child()) {
555-
assert(cmp((const NodeType*)new_node->_parent, (const NodeType*)new_node->_right), "new node not > parent");
555+
assert(cmp(static_cast<const NodeType*>(new_node->parent()), static_cast<const NodeType*>(new_node)), "new node not > parent");
556556
}
557557

558558
new_node->_left = old_node->_left;
559559
new_node->_right = old_node->_right;
560560
if (new_node->_left != nullptr) {
561-
assert(cmp((const NodeType*)new_node->_left, (const NodeType*)new_node), "left child not < new node");
561+
assert(cmp(static_cast<const NodeType*>(new_node->_left), static_cast<const NodeType*>(new_node)), "left child not < new node");
562562
new_node->_left->set_parent(new_node);
563563
}
564564
if (new_node->_right != nullptr) {
565-
assert(cmp((const NodeType*)new_node, (const NodeType*)new_node->_right), "right child not > new node");
565+
assert(cmp(static_cast<const NodeType*>(new_node), static_cast<const NodeType*>(new_node->_right)), "right child not > new node");
566566
new_node->_right->set_parent(new_node);
567567
}
568568

@@ -606,6 +606,7 @@ inline void AbstractRBTree<K, NodeType, COMPARATOR>::visit_in_order(F f) const {
606606
template <typename K, typename NodeType, typename COMPARATOR>
607607
template <typename F>
608608
inline void AbstractRBTree<K, NodeType, COMPARATOR>::visit_range_in_order(const K& from, const K& to, F f) const {
609+
assert_key_leq(from, to);
609610
if (_root == nullptr) {
610611
return;
611612
}
@@ -615,13 +616,6 @@ inline void AbstractRBTree<K, NodeType, COMPARATOR>::visit_range_in_order(const
615616
const NodeType* start = cursor_start.found() ? cursor_start.node() : next(cursor_start).node();
616617
const NodeType* end = next(cursor_end).node();
617618

618-
if (start != nullptr) {
619-
assert_leq(from, start);
620-
assert_geq(to, start);
621-
} else {
622-
assert(end == nullptr, "end node found but not start node");
623-
}
624-
625619
while (start != end) {
626620
f(start);
627621
start = start->next();

‎test/hotspot/gtest/utilities/test_rbtree.cpp

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,16 @@
3333

3434
class RBTreeTest : public testing::Test {
3535
public:
36+
using RBTreeIntNode = RBNode<int, int>;
37+
3638
struct Cmp {
3739
static int cmp(int a, int b) {
3840
return a - b;
3941
}
42+
43+
static bool cmp(const RBTreeIntNode* a, const RBTreeIntNode* b) {
44+
return a->key() < b->key();
45+
}
4046
};
4147

4248
struct CmpInverse {
@@ -73,7 +79,6 @@ struct ArrayAllocator {
7379
};
7480

7581
using RBTreeInt = RBTreeCHeap<int, int, Cmp, mtTest>;
76-
using RBTreeIntNode = RBNode<int, int>;
7782
using IntrusiveTreeNode = IntrusiveRBNode;
7883

7984
struct IntrusiveHolder {
@@ -93,6 +98,10 @@ struct ArrayAllocator {
9398
return a - IntrusiveHolder::cast_to_self(b)->key;
9499
}
95100

101+
static int cmp(int a, int b) {
102+
return a - b;
103+
}
104+
96105
// true if a < b
97106
static bool cmp(const IntrusiveTreeNode* a, const IntrusiveTreeNode* b) {
98107
return (IntrusiveHolder::cast_to_self(a)->key -
@@ -300,6 +309,23 @@ struct ArrayAllocator {
300309
}
301310
}
302311

312+
void test_visit_outside_range() {
313+
RBTreeInt rbtree;
314+
using Node = RBTreeIntNode;
315+
316+
rbtree.upsert(2, 0);
317+
rbtree.upsert(5, 0);
318+
319+
constexpr int test_cases[9][2] = {{0, 0}, {0, 1}, {1, 1}, {3, 3}, {3, 4},
320+
{4, 4}, {6, 6}, {6, 7}, {7, 7}};
321+
322+
for (const int (&test_case)[2] : test_cases) {
323+
rbtree.visit_range_in_order(test_case[0], test_case[1], [&](const Node* x) {
324+
FAIL() << "Range should not visit nodes";
325+
});
326+
}
327+
}
328+
303329
void test_closest_leq() {
304330
using Node = RBTreeIntNode;
305331
{
@@ -802,6 +828,10 @@ TEST_VM_F(RBTreeTest, TestVisitors) {
802828
this->test_visitors();
803829
}
804830

831+
TEST_VM_F(RBTreeTest, TestVisitOutsideRange) {
832+
this->test_visit_outside_range();
833+
}
834+
805835
TEST_VM_F(RBTreeTest, TestClosestLeq) {
806836
this->test_closest_leq();
807837
}

0 commit comments

Comments
 (0)