ADT: Avoid relying on UB in ilist_node::getNextNode()

Re-implement `ilist_node::getNextNode()` and `getPrevNode()` without
relying on the sentinel having a "next" pointer.  Instead, get access to
the owning list and compare against the `begin()` and `end()` iterators.

This only works when the node *can* get access to the owning list.  The
new support is in `ilist_node_with_parent<>`, and any class `Ty`
inheriting from `ilist_node<NodeTy>` that wants `getNextNode()` and/or
`getPrevNode()` should inherit from
`ilist_node_with_parent<NodeTy, ParentTy>` instead.  The requirements:

  - `NodeTy` must have a `getParent()` function that returns the parent.
  - `ParentTy` must have a `getSublistAccess()` static that, given a(n
    ignored) `NodeTy*` (to determine which list), returns a member field
    pointer to the appropriate `ilist<>`.

This isn't the cleanest way to get access to the owning list, but it
leverages the API already used in the IR hierarchy (see, e.g.,
`Instruction::getSublistAccess()`).

If anyone feels like ripping out the calls to `getNextNode()` and
`getPrevNode()` and replacing with direct iterator logic, they can also
remove the access function, etc., but as an incremental step, I'm
maintaining the API where it's currently used in tree.

If these requirements are *not* met, call sites with access to the ilist
can call `iplist<NodeTy>::getNextNode(NodeTy*)` directly, as in
ilistTest.cpp.

Why rewrite this?

The old code was broken, calling `getNext()` on a sentinel that possibly
didn't have a "next" pointer at all!  The new code avoids that
particular flavour of UB (see the commit message for r252538 for more
details about the "lucky" memory layout that made this function so
interesting).

There's still some UB here: the end iterator gets downcast to `NodeTy*`,
even when it's a sentinel (which is typically
`ilist_half_node<NodeTy*>`).  I'll tackle that in follow-up commits.
See this llvm-dev thread for more details:
http://lists.llvm.org/pipermail/llvm-dev/2015-October/091115.html

What's the danger?

There might be some code that relies on `getNextNode()` or
`getPrevNode()` *never* returning `nullptr` -- i.e., that relies on them
being broken when the sentinel is an `ilist_half_node<NodeTy>`.  I tried
to root out those cases with the audits I did leading up to r252380, but
it's possible I missed one or two.  I hope not.

(If (1) you have out-of-tree code, (2) you've reverted r252380
temporarily, and (3) you get some weird crashes with this commit, then I
recommend un-reverting r252380 and auditing the compile errors looking
for "strange" implicit conversions.)

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@252694 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/ADT/ilist.h b/include/llvm/ADT/ilist.h
index c34434a1f76871036e72668d96549829b543f743..3044a6c435f104e64df191b7a2e8e74680a718c0 100644 (file)
--- a/include/llvm/ADT/ilist.h
+++ b/include/llvm/ADT/ilist.h
@@ -251,11 +251,11 @@ public:
   pointer operator->() const { return &operator*(); }
 
   // Comparison operators
-  bool operator==(const ilist_iterator &RHS) const {
-    return NodePtr == RHS.NodePtr;
+  template <class Y> bool operator==(const ilist_iterator<Y> &RHS) const {
+    return NodePtr == RHS.getNodePtrUnchecked();
   }
-  bool operator!=(const ilist_iterator &RHS) const {
-    return NodePtr != RHS.NodePtr;
+  template <class Y> bool operator!=(const ilist_iterator<Y> &RHS) const {
+    return NodePtr != RHS.getNodePtrUnchecked();
   }
 
   // Increment and decrement operators...
@@ -687,6 +687,30 @@ public:
     merge(RightHalf, comp);
   }
   void sort() { sort(op_less); }
+
+  /// \brief Get the previous node, or \c nullptr for the list head.
+  NodeTy *getPrevNode(NodeTy &N) const {
+    auto I = N.getIterator();
+    if (I == begin())
+      return nullptr;
+    return &*std::prev(I);
+  }
+  /// \brief Get the previous node, or \c nullptr for the list head.
+  const NodeTy *getPrevNode(const NodeTy &N) const {
+    return getPrevNode(const_cast<NodeTy &>(N));
+  }
+
+  /// \brief Get the next node, or \c nullptr for the list tail.
+  NodeTy *getNextNode(NodeTy &N) const {
+    auto Next = std::next(N.getIterator());
+    if (Next == end())
+      return nullptr;
+    return &*Next;
+  }
+  /// \brief Get the next node, or \c nullptr for the list tail.
+  const NodeTy *getNextNode(const NodeTy &N) const {
+    return getNextNode(const_cast<NodeTy &>(N));
+  }
 };
 
 

diff --git a/include/llvm/ADT/ilist_node.h b/include/llvm/ADT/ilist_node.h
index 8f0538bcf6e7e7ea5c0cd09e7061f4745a44346d..7e5a0e0e5ad81d5867ef6710e65d2649bb84cab4 100644 (file)
--- a/include/llvm/ADT/ilist_node.h
+++ b/include/llvm/ADT/ilist_node.h
@@ -66,54 +66,55 @@ public:
     // FIXME: Stop downcasting to create the iterator (potential UB).
     return ilist_iterator<const NodeTy>(static_cast<const NodeTy *>(this));
   }
+};
+
+/// An ilist node that can access its parent list.
+///
+/// Requires \c NodeTy to have \a getParent() to find the parent node, and the
+/// \c ParentTy to have \a getSublistAccess() to get a reference to the list.
+template <typename NodeTy, typename ParentTy>
+class ilist_node_with_parent : public ilist_node<NodeTy> {
+protected:
+  ilist_node_with_parent() = default;
+
+private:
+  /// Forward to NodeTy::getParent().
+  ///
+  /// Note: do not use the name "getParent()".  We want a compile error
+  /// (instead of recursion) when the subclass fails to implement \a
+  /// getParent().
+  const ParentTy *getNodeParent() const {
+    return static_cast<const NodeTy *>(this)->getParent();
+  }
 
+public:
   /// @name Adjacent Node Accessors
   /// @{
-
-  /// \brief Get the previous node, or 0 for the list head.
+  /// \brief Get the previous node, or \c nullptr for the list head.
   NodeTy *getPrevNode() {
-    NodeTy *Prev = this->getPrev();
-
-    // Check for sentinel.
-    if (!Prev->getNext())
-      return nullptr;
-
-    return Prev;
+    // Should be separated to a reused function, but then we couldn't use auto
+    // (and would need the type of the list).
+    const auto &List =
+        getNodeParent()->*(ParentTy::getSublistAccess((NodeTy *)nullptr));
+    return List.getPrevNode(*static_cast<NodeTy *>(this));
   }
-
-  /// \brief Get the previous node, or 0 for the list head.
+  /// \brief Get the previous node, or \c nullptr for the list head.
   const NodeTy *getPrevNode() const {
-    const NodeTy *Prev = this->getPrev();
-
-    // Check for sentinel.
-    if (!Prev->getNext())
-      return nullptr;
-
-    return Prev;
+    return const_cast<ilist_node_with_parent *>(this)->getPrevNode();
   }
 
-  /// \brief Get the next node, or 0 for the list tail.
+  /// \brief Get the next node, or \c nullptr for the list tail.
   NodeTy *getNextNode() {
-    NodeTy *Next = getNext();
-
-    // Check for sentinel.
-    if (!Next->getNext())
-      return nullptr;
-
-    return Next;
+    // Should be separated to a reused function, but then we couldn't use auto
+    // (and would need the type of the list).
+    const auto &List =
+        getNodeParent()->*(ParentTy::getSublistAccess((NodeTy *)nullptr));
+    return List.getNextNode(*static_cast<NodeTy *>(this));
   }
-
-  /// \brief Get the next node, or 0 for the list tail.
+  /// \brief Get the next node, or \c nullptr for the list tail.
   const NodeTy *getNextNode() const {
-    const NodeTy *Next = getNext();
-
-    // Check for sentinel.
-    if (!Next->getNext())
-      return nullptr;
-
-    return Next;
+    return const_cast<ilist_node_with_parent *>(this)->getNextNode();
   }
-
   /// @}
 };
 

diff --git a/include/llvm/CodeGen/MachineBasicBlock.h b/include/llvm/CodeGen/MachineBasicBlock.h
index db3b24e2e1669beab69e09c22491cf72c6b7e7e9..4f76b21fb00fca4529161e9426beb127b45bc0de 100644 (file)
--- a/include/llvm/CodeGen/MachineBasicBlock.h
+++ b/include/llvm/CodeGen/MachineBasicBlock.h
@@ -65,7 +65,8 @@ private:
   void createNode(const MachineInstr &);
 };
 
-class MachineBasicBlock : public ilist_node<MachineBasicBlock> {
+class MachineBasicBlock
+    : public ilist_node_with_parent<MachineBasicBlock, MachineFunction> {
 public:
   /// Pair of physical register and lane mask.
   /// This is not simply a std::pair typedef because the members should be named
@@ -272,6 +273,11 @@ public:
   reverse_iterator       rend  ()       { return instr_rend();   }
   const_reverse_iterator rend  () const { return instr_rend();   }
 
+  /// Support for MachineInstr::getNextNode().
+  static Instructions MachineBasicBlock::*getSublistAccess(MachineInstr *) {
+    return &MachineBasicBlock::Insts;
+  }
+
   inline iterator_range<iterator> terminators() {
     return iterator_range<iterator>(getFirstTerminator(), end());
   }

diff --git a/include/llvm/CodeGen/MachineFunction.h b/include/llvm/CodeGen/MachineFunction.h
index 82bbbcf545533764eedb52e9133ea31dc602c558..d163c971d4bb85897a06e28d20fa447c5c1fde89 100644 (file)
--- a/include/llvm/CodeGen/MachineFunction.h
+++ b/include/llvm/CodeGen/MachineFunction.h
@@ -331,6 +331,12 @@ public:
   typedef std::reverse_iterator<const_iterator> const_reverse_iterator;
   typedef std::reverse_iterator<iterator>             reverse_iterator;
 
+  /// Support for MachineBasicBlock::getNextNode().
+  static BasicBlockListType MachineFunction::*
+  getSublistAccess(MachineBasicBlock *) {
+    return &MachineFunction::BasicBlocks;
+  }
+
   /// addLiveIn - Add the specified physical register as a live-in value and
   /// create a corresponding virtual register for it.
   unsigned addLiveIn(unsigned PReg, const TargetRegisterClass *RC);

diff --git a/include/llvm/CodeGen/MachineInstr.h b/include/llvm/CodeGen/MachineInstr.h
index 34681ec44e95a87c2cab7352268b6b6a1f88723a..607e2781960fb0c0a507c45371a2e73ff5c92903 100644 (file)
--- a/include/llvm/CodeGen/MachineInstr.h
+++ b/include/llvm/CodeGen/MachineInstr.h
@@ -48,7 +48,8 @@ class MachineMemOperand;
 /// MachineFunction is deleted, all the contained MachineInstrs are deallocated
 /// without having their destructor called.
 ///
-class MachineInstr : public ilist_node<MachineInstr> {
+class MachineInstr
+    : public ilist_node_with_parent<MachineInstr, MachineBasicBlock> {
 public:
   typedef MachineMemOperand **mmo_iterator;
 

diff --git a/include/llvm/IR/BasicBlock.h b/include/llvm/IR/BasicBlock.h
index fe5dca6a50618190dfe6d258c3011e68a70dab8b..c6b54d308ce69f7d24c49491f4353cfd4b824011 100644 (file)
--- a/include/llvm/IR/BasicBlock.h
+++ b/include/llvm/IR/BasicBlock.h
@@ -50,7 +50,7 @@ struct SymbolTableListSentinelTraits<BasicBlock>
 /// modifying a program. However, the verifier will ensure that basic blocks
 /// are "well formed".
 class BasicBlock : public Value, // Basic blocks are data objects also
-                   public ilist_node<BasicBlock> {
+                   public ilist_node_with_parent<BasicBlock, Function> {
   friend class BlockAddress;
 public:
   typedef SymbolTableList<Instruction> InstListType;

diff --git a/include/llvm/IR/Instruction.h b/include/llvm/IR/Instruction.h
index 581ac09cf0c3d99d06b4652673f6ebdfd4fd5e8c..c7ba8721fe06b9deab25730520a20595ea30947e 100644 (file)
--- a/include/llvm/IR/Instruction.h
+++ b/include/llvm/IR/Instruction.h
@@ -33,7 +33,8 @@ template <>
 struct SymbolTableListSentinelTraits<Instruction>
     : public ilist_half_embedded_sentinel_traits<Instruction> {};
 
-class Instruction : public User, public ilist_node<Instruction> {
+class Instruction : public User,
+                    public ilist_node_with_parent<Instruction, BasicBlock> {
   void operator=(const Instruction &) = delete;
   Instruction(const Instruction &) = delete;
 

diff --git a/include/llvm/MC/MCAssembler.h b/include/llvm/MC/MCAssembler.h
index d69472bf2ec21e55d2ea316836f54349128840e6..aadfe1747f08792e7c1e11743f48d90cb0631c11 100644 (file)
--- a/include/llvm/MC/MCAssembler.h
+++ b/include/llvm/MC/MCAssembler.h
@@ -35,7 +35,7 @@ class MCSubtargetInfo;
 class MCValue;
 class MCAsmBackend;
 
-class MCFragment : public ilist_node<MCFragment> {
+class MCFragment : public ilist_node_with_parent<MCFragment, MCSection> {
   friend class MCAsmLayout;
 
   MCFragment(const MCFragment &) = delete;

diff --git a/include/llvm/MC/MCSection.h b/include/llvm/MC/MCSection.h
index c049ad5455bf31268b35209cb18d4d857e21e26b..08af5334bc1972cc55ae5ecb381a2c07dac0f122 100644 (file)
--- a/include/llvm/MC/MCSection.h
+++ b/include/llvm/MC/MCSection.h
@@ -154,6 +154,11 @@ public:
     return const_cast<MCSection *>(this)->getFragmentList();
   }
 
+  /// Support for MCFragment::getNextNode().
+  static FragmentListType MCSection::*getSublistAccess(MCFragment *) {
+    return &MCSection::Fragments;
+  }
+
   const MCDummyFragment &getDummyFragment() const { return DummyFragment; }
   MCDummyFragment &getDummyFragment() { return DummyFragment; }
 

diff --git a/unittests/ADT/ilistTest.cpp b/unittests/ADT/ilistTest.cpp
index 4e5235ba03beb1320cc410429a6ec80388bdb855..377dcc044ddb3db242cf7a84f19515f0aa697978 100644 (file)
--- a/unittests/ADT/ilistTest.cpp
+++ b/unittests/ADT/ilistTest.cpp
@@ -30,18 +30,18 @@ TEST(ilistTest, Basic) {
   ilist<Node> List;
   List.push_back(Node(1));
   EXPECT_EQ(1, List.back().Value);
-  EXPECT_EQ(nullptr, List.back().getPrevNode());
-  EXPECT_EQ(nullptr, List.back().getNextNode());
+  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.front().getNextNode()->Value);
-  EXPECT_EQ(1, List.back().getPrevNode()->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.front().getNextNode()->Value);
-  EXPECT_EQ(1, ConstList.back().getPrevNode()->Value);
+  EXPECT_EQ(2, ConstList.getNextNode(ConstList.front())->Value);
+  EXPECT_EQ(1, ConstList.getPrevNode(ConstList.back())->Value);
 }
 
 TEST(ilistTest, SpliceOne) {