llvm/unittests/ADT/ilistTest.cpp
Duncan P. N. Exon Smith 3cef1f9cbf Reapply "ADT: Remove UB in ilist (and use a circular linked list)"
This reverts commit r279053, reapplying r278974 after fixing PR29035
with r279104.

Note that r279312 has been committed in the meantime, and this has been
rebased on top of that.  Otherwise it's identical to r278974.

Note for maintainers of out-of-tree code (that I missed in the original
message): if the new isKnownSentinel() assertion is firing from
ilist_iterator<>::operator*(), this patch has identified a bug in your
code.  There are a few common patterns:
- Some IR-related APIs htake an IRUnit* that might be nullptr, and pass
  in an incremented iterator as an insertion point.  Some old code was
  using "&*++I", which in the case of end() only worked by fluke.  If
  the IRUnit in question inherits from ilist_node_with_parent<>, you can
  use "I->getNextNode()".  Otherwise, use "List.getNextNode(*I)".
- In most other cases, crashes on &*I just need to check for I==end()
  before dereferencing.
- There's also occasional code that sends iterators into a function, and
  then starts calling I->getOperand() (or other API).  Either check for
  end() before the entering the function, or early exit.

Note for if the static_assert with HasObsoleteCustomization is firing
for you:
- r278513 has examples of how to stop using custom sentinel traits.
- r278532 removed ilist_nextprev_traits since no one was using it.  See
  lld's r278469 for the only migration I needed to do.

Original commit message follows.

----

This removes the undefined behaviour (UB) in ilist/ilist_node/etc.,
mainly by removing (gutting) the ilist_sentinel_traits customization
point and canonicalizing on a single, efficient memory layout.  This
fixes PR26753.

The new ilist is a doubly-linked circular list.
- ilist_node_base has two ilist_node_base*: Next and Prev.  Size-of: two
  pointers.
- ilist_node<T> (size-of: two pointers) is a type-safe wrapper around
  ilist_node_base.
- ilist_iterator<T> (size-of: two pointers) operates on an
  ilist_node<T>*, and downcasts to T* on dereference.
- ilist_sentinel<T> (size-of: two pointers) is a wrapper around
  ilist_node<T> that has some extra API for list management.
- ilist<T> (size-of: two pointers) has an ilist_sentinel<T>, whose
  address is returned for end().

The new memory layout matches ilist_half_embedded_sentinel_traits<T>
exactly.  The Head pointer that previously lived in ilist<T> is
effectively glued to the ilist_half_node<T> that lived in
ilist_half_embedded_sentinel_traits<T>, becoming the Next and Prev in
the ilist_sentinel_node<T>, respectively.  sizeof(ilist<T>) is now the
size of two pointers, and there is never any additional storage for a
sentinel.

This is a much simpler design for a doubly-linked list, removing most of
the corner cases of list manipulation (add, remove, etc.).  In follow-up
commits, I intend to move as many algorithms as possible into a
non-templated base class (ilist_base) to reduce code size.

Moreover, this fixes the UB in ilist_iterator/getNext/getPrev
operations.  Previously, ilist_iterator<T> operated on a T*, even when
the sentinel was not of type T (i.e., ilist_embedded_sentinel_traits and
ilist_half_embedded_sentinel_traits).  This added UB to all operations
involving end().   Now, ilist_iterator<T> operates on an ilist_node<T>*,
and only downcasts when the full type is guaranteed to be T*.

What did we lose?  There used to be a crash (in some configurations) on
++end().  Curiously (via UB), ++end() would return begin() for users of
ilist_half_embedded_sentinel_traits<T>, but otherwise ++end() would
cause a nice dependable nullptr dereference, crashing instead of a
possible infinite loop.  Options:
 1. Lose that behaviour.
 2. Keep it, by stealing a bit from Prev in asserts builds.
 3. Crash on dereference instead, using the same technique.

Hans convinced me (because of the number of problems this and r278532
exposed on Windows) that we really need some assertion here, at least in
the short term.  I've opted for #3 since I think it catches more bugs.

I added only a couple of unit tests to root out specific bugs I hit
during bring-up, but otherwise this is tested implicitly via the
extensive usage throughout LLVM.

Planned follow-ups:
- Remove ilist_*sentinel_traits<T>.  Here I've just gutted them to
  prevent build failures in sub-projects.  Once I stop referring to them
  in sub-projects, I'll come back and delete them.
- Add ilist_base and move algorithms there.
- Check and fix move construction and assignment.

Eventually, there are other interesting directions:
- Rewrite reverse iterators, so that rbegin().getNodePtr()==&*rbegin().
  This allows much simpler logic when erasing elements during a reverse
  traversal.
- Remove ilist_traits::createNode, by deleting the remaining API that
  creates nodes.  Intrusive lists shouldn't be creating nodes
  themselves.
- Remove ilist_traits::deleteNode, by (1) asserting that lists are empty
  on destruction and (2) changing API that calls it to take a Deleter
  functor (intrusive lists shouldn't be in the memory management
  business).
- Reconfigure the remaining callback traits (addNodeToList, etc.) to be
  higher-level, pulling out a simple_ilist<T> that is much easier to
  read and understand.
- Allow tags (e.g., ilist_node<T,tag1> and ilist_node<T,tag2>) so that T
  can be a member of multiple intrusive lists.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@279314 91177308-0d34-0410-b5e6-96231b3b80d8
2016-08-19 20:40:12 +00:00

230 lines
5.9 KiB
C++

//===- llvm/unittest/ADT/APInt.cpp - APInt unit tests ---------------------===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
#include "llvm/ADT/ilist.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/ilist_node.h"
#include "gtest/gtest.h"
#include <ostream>
using namespace llvm;
namespace {
struct Node : ilist_node<Node> {
int Value;
Node() {}
Node(int Value) : Value(Value) {}
Node(const Node&) = default;
~Node() { Value = -1; }
};
TEST(ilistTest, Basic) {
ilist<Node> List;
List.push_back(Node(1));
EXPECT_EQ(1, List.back().Value);
EXPECT_EQ(nullptr, List.getPrevNode(List.back()));
EXPECT_EQ(nullptr, List.getNextNode(List.back()));
List.push_back(Node(2));
EXPECT_EQ(2, List.back().Value);
EXPECT_EQ(2, List.getNextNode(List.front())->Value);
EXPECT_EQ(1, List.getPrevNode(List.back())->Value);
const ilist<Node> &ConstList = List;
EXPECT_EQ(2, ConstList.back().Value);
EXPECT_EQ(2, ConstList.getNextNode(ConstList.front())->Value);
EXPECT_EQ(1, ConstList.getPrevNode(ConstList.back())->Value);
}
TEST(ilistTest, SpliceOne) {
ilist<Node> List;
List.push_back(1);
// The single-element splice operation supports noops.
List.splice(List.begin(), List, List.begin());
EXPECT_EQ(1u, List.size());
EXPECT_EQ(1, List.front().Value);
EXPECT_TRUE(std::next(List.begin()) == List.end());
// Altenative noop. Move the first element behind itself.
List.push_back(2);
List.push_back(3);
List.splice(std::next(List.begin()), List, List.begin());
EXPECT_EQ(3u, List.size());
EXPECT_EQ(1, List.front().Value);
EXPECT_EQ(2, std::next(List.begin())->Value);
EXPECT_EQ(3, List.back().Value);
}
TEST(ilistTest, SpliceSwap) {
ilist<Node> L;
Node N0(0);
Node N1(1);
L.insert(L.end(), &N0);
L.insert(L.end(), &N1);
EXPECT_EQ(0, L.front().Value);
EXPECT_EQ(1, L.back().Value);
L.splice(L.begin(), L, ++L.begin());
EXPECT_EQ(1, L.front().Value);
EXPECT_EQ(0, L.back().Value);
L.clearAndLeakNodesUnsafely();
}
TEST(ilistTest, SpliceSwapOtherWay) {
ilist<Node> L;
Node N0(0);
Node N1(1);
L.insert(L.end(), &N0);
L.insert(L.end(), &N1);
EXPECT_EQ(0, L.front().Value);
EXPECT_EQ(1, L.back().Value);
L.splice(L.end(), L, L.begin());
EXPECT_EQ(1, L.front().Value);
EXPECT_EQ(0, L.back().Value);
L.clearAndLeakNodesUnsafely();
}
TEST(ilistTest, UnsafeClear) {
ilist<Node> List;
// Before even allocating a sentinel.
List.clearAndLeakNodesUnsafely();
EXPECT_EQ(0u, List.size());
// Empty list with sentinel.
ilist<Node>::iterator E = List.end();
List.clearAndLeakNodesUnsafely();
EXPECT_EQ(0u, List.size());
// The sentinel shouldn't change.
EXPECT_TRUE(E == List.end());
// List with contents.
List.push_back(1);
ASSERT_EQ(1u, List.size());
Node *N = &*List.begin();
EXPECT_EQ(1, N->Value);
List.clearAndLeakNodesUnsafely();
EXPECT_EQ(0u, List.size());
ASSERT_EQ(1, N->Value);
delete N;
// List is still functional.
List.push_back(5);
List.push_back(6);
ASSERT_EQ(2u, List.size());
EXPECT_EQ(5, List.front().Value);
EXPECT_EQ(6, List.back().Value);
}
struct Empty {};
TEST(ilistTest, HasObsoleteCustomizationTrait) {
// Negative test for HasObsoleteCustomization.
static_assert(!ilist_detail::HasObsoleteCustomization<Empty, Node>::value,
"Empty has no customizations");
}
struct GetNext {
Node *getNext(Node *);
};
TEST(ilistTest, HasGetNextTrait) {
static_assert(ilist_detail::HasGetNext<GetNext, Node>::value,
"GetNext has a getNext(Node*)");
static_assert(ilist_detail::HasObsoleteCustomization<GetNext, Node>::value,
"Empty should be obsolete because of getNext()");
// Negative test for HasGetNext.
static_assert(!ilist_detail::HasGetNext<Empty, Node>::value,
"Empty does not have a getNext(Node*)");
}
struct CreateSentinel {
Node *createSentinel();
};
TEST(ilistTest, HasCreateSentinelTrait) {
static_assert(ilist_detail::HasCreateSentinel<CreateSentinel>::value,
"CreateSentinel has a getNext(Node*)");
static_assert(
ilist_detail::HasObsoleteCustomization<CreateSentinel, Node>::value,
"Empty should be obsolete because of createSentinel()");
// Negative test for HasCreateSentinel.
static_assert(!ilist_detail::HasCreateSentinel<Empty>::value,
"Empty does not have a createSentinel()");
}
struct NodeWithCallback : ilist_node<NodeWithCallback> {
int Value = 0;
bool IsInList = false;
NodeWithCallback() = default;
NodeWithCallback(int Value) : Value(Value) {}
NodeWithCallback(const NodeWithCallback &) = delete;
};
} // end namespace
namespace llvm {
template <>
struct ilist_traits<NodeWithCallback>
: public ilist_node_traits<NodeWithCallback> {
void addNodeToList(NodeWithCallback *N) { N->IsInList = true; }
void removeNodeFromList(NodeWithCallback *N) { N->IsInList = false; }
};
} // end namespace llvm
namespace {
TEST(ilistTest, addNodeToList) {
ilist<NodeWithCallback> L;
NodeWithCallback N(7);
ASSERT_FALSE(N.IsInList);
L.insert(L.begin(), &N);
ASSERT_EQ(1u, L.size());
ASSERT_EQ(&N, &*L.begin());
ASSERT_TRUE(N.IsInList);
L.remove(&N);
ASSERT_EQ(0u, L.size());
ASSERT_FALSE(N.IsInList);
}
struct PrivateNode : private ilist_node<PrivateNode> {
friend struct llvm::ilist_node_access;
int Value = 0;
PrivateNode() = default;
PrivateNode(int Value) : Value(Value) {}
PrivateNode(const PrivateNode &) = delete;
};
TEST(ilistTest, privateNode) {
// Instantiate various APIs to be sure they're callable when ilist_node is
// inherited privately.
ilist<NodeWithCallback> L;
NodeWithCallback N(7);
L.insert(L.begin(), &N);
++L.begin();
(void)*L.begin();
(void)(L.begin() == L.end());
ilist<NodeWithCallback> L2;
L2.splice(L2.end(), L);
L2.remove(&N);
}
} // end namespace