Document ReaderStack class (in reader.h)

This change documents the ReaderStack class in reader.h.

In addition to providing this documentation I've eliminated
unnecessary complexity in the ReaderStack iteration
interface. Previously they were using a pointer to an incomplete C++
class, ReaderStackIterator, as their iterator state but the client was
just using the space of such pointers to hold an index into the stack
(hence requiring a lot of casting and making the assumption that a
size_t will fit in a pointer--which is likely to always be true but is
nevertheless somewhat of a hack).

Instead I've made ReaderStackIterator a typedef to size_t and pass
such values as references in the API. I believe the result is a lot
cleaner.

Closes #143.

Documentation/index.md

@@ -1,18 +1,15 @@
-/**
-\mainpage LLILC
+# LLILC {#mainpage}
 
-\section main_intro Introduction
+## Introduction
 Welcome to LLILC.
 
-This documentation describes the \b internals of
-LLILC, not the \b external use of  LLILC. There are no instructions
-here on how to use LLILC, only the APIs that make up the software. For usage 
+This documentation describes the **internals** of
+LLILC, not the **external** use of  LLILC. There are no instructions
+here on how to use LLILC, only the APIs that make up the software. For usage
 instructions, please see the programmer's guide or reference manual.
 
-\section main_caveat Caveat 
-This documentation is generated directly from the source code with doxygen. 
+## Caveat
+This documentation is generated directly from the source code with doxygen.
 Since LLILC is under active development, what you're about to
 read is likely out of date! However, it may still be useful since certain portions
-of LLILC are very stable. 
-
-*/
+of LLILC are very stable.

include/Reader/reader.h

@@ -299,45 +299,112 @@ class FlowGraphNodeOffsetList;
 struct EHRegion;
 struct EHRegionList;
 struct FgData;
+class ReaderBase; // Forward declaration
 
-//
-// Reader Stack
-//
+#pragma region Reader Operand Stack
 
-typedef struct IRNodeListElement *IRNodeList;
+/// \brief An integral type used to index into the operand stack and to 
+/// iterate over the elements of the stack.
+///
+/// It is just a logical index into underlying array used to represent the stack.
+typedef size_t ReaderStackIterator;
 
-// A node for the push down list we call the reader stack. The
-// intent is that the stack is only accessed via push and pop.
-class ReaderStackNode;
-class ReaderStackIterator;
-class ReaderBase;
+/// \brief A stack of IRNode pointers representing the MSIL operand stack.
+///
+/// The MSIL instruction set operates on a stack machine. Instructions
+/// with operands may take them from the stack (if not some kind of
+/// immediate) and the results of instruction are pushed on the operand stack.
+/// The MSIL operands are translated by the reader into IRNodes. 
+/// The operand stack is represented by a stack of pointers to the 
+/// IRNodes for the operands. 
+///
+/// ReaderStack is an abstract class with no C++ declared state and all
+/// abstract methods. However we can describe the effects of the methods
+/// in terms of the stack that the implementing class will provide.
+/// 
+/// For convenience in the method descriptions below, we use
+/// stack[k] to denote the stack element that is k elements from the top,
+/// so that stack[0] is the top element. 
 
 class ReaderStack {
 public:
+  /// \brief Pop the top element off the operand stack.
+  /// \return The top element of the stack.
+  /// \pre The stack is not empty
+  /// \post The top element of the stack has been removed.
   virtual IRNode *pop(void) = 0;
+
+  /// \brief Push \p NewVal onto the operand stack.
+  /// \pre NewVal != NULL
   virtual void push(IRNode *NewVal, IRNode **NewIR) = 0;
+
+  /// \brief Make the operand stack empty.
+  /// \post The stack is empty.
   virtual void clearStack(void) = 0;
+
+  /// \brief Test whether the stack is empty.
+  /// \return True if the stack is empty
   virtual bool empty(void) = 0;
+
+  /// \brief If the stack is not empty, cause an assertion failure.
   virtual void assertEmpty(void) = 0;
+
+  /// \brief get the number of operands on the operand stack.
+  /// \return The number of elements in the stack.
   virtual uint32_t depth() = 0;
 
-  // For iteration, implement/use as needed by the client
-  virtual IRNode *getIterator(ReaderStackIterator **) = 0;
-  virtual IRNode *iteratorGetNext(ReaderStackIterator **) = 0;
-  virtual void iteratorReplace(ReaderStackIterator **, IRNode *) = 0;
-  virtual IRNode *getReverseIterator(ReaderStackIterator **) = 0;
-  virtual IRNode *getReverseIteratorFromDepth(ReaderStackIterator **,
-                                              uint32_t Depth) = 0;
-  virtual IRNode *reverseIteratorGetNext(ReaderStackIterator **) = 0;
+  /// \brief Initialize iteration over stack from top to bottom.
+  ///
+  /// If the stack is non-empty, return value of top element of the stack
+  /// and set \a Iterator to index the top element.
+  /// \return (empty() ? NULL : stack[0])
+  virtual IRNode *getIterator(ReaderStackIterator& Iterator) = 0;
+
+  /// \brief Return the next stack value in top to bottom iteration and 
+  /// advance the iteration position, but returns NULL is the bottom
+  /// element has already been reached.
+  virtual IRNode *iteratorGetNext(ReaderStackIterator& Iterator) = 0;
+
+  /// \brief Replace the stack location referenced by \a Iterator
+  /// with the \a NewValue.
+  virtual void iteratorReplace(ReaderStackIterator& Iterator, IRNode *NewValue) 
+    = 0;
+
+  /// \brief Initialize iteration over stack from bottom to top.
+  ///
+  /// If the stack is non-empty, return value of bottom element of the stack
+  /// and set \a Iterator to index the bottom element. If the stack
+  /// is empty just return NULL>
+  virtual IRNode *getReverseIterator(ReaderStackIterator& Iterator) = 0;
+
+  /// \brief Initialize iteration over stack from bottom to top but only
+  /// iterating over the top \a Depth elements.
+  ///
+  /// If the stack is non-empty, return value of element (\a Depth - 1) from the 
+  /// top of the stack and set \a Iterator to index that element.
+  /// Besides starting an iteration this can also be used to randomly access
+  /// elements of the stack. For example calling this with \a Depth == 2
+  /// would return the element just below the top element on the stack.
+  virtual IRNode *getReverseIteratorFromDepth(ReaderStackIterator& Iterator,
+    uint32_t Depth) = 0;
+
+
+  /// \brief Return the next stack value in bottom to top iteration and 
+  /// advance the iteration position. But return NULL if the top has already
+  /// been reached.
+  virtual IRNode *reverseIteratorGetNext(ReaderStackIterator& Iterator) = 0;
 
 #if defined(_DEBUG)
+  /// \brief Print the contents of the operand stack onto the debug output.
   virtual void print() = 0;
 #endif
 
-  // Creates stack copy, used in non-empty stacks across flow, etc.
+  /// \brief Returns a copy of this operand stack.
   virtual ReaderStack *copy() = 0;
 };
 
+#pragma endregion
+
 class ReaderCallTargetData {
   friend class ReaderBase;
 
@@ -1769,8 +1836,6 @@ public:
   virtual void assignToSuccessorStackNode(FlowGraphNode *, IRNode *Destination,
                                           IRNode *Source, IRNode **NewIR,
                                           bool *) = 0;
-  //    virtual ReaderStackNode* copyStackList(ReaderStackNode* stack) =
-  //    0;
   virtual bool typesCompatible(IRNode *Src1, IRNode *Src2) = 0;
 
   virtual void removeStackInterference(IRNode **NewIR) = 0;

include/Reader/readerir.h

@@ -184,13 +184,13 @@ public:
   uint32_t depth() override;
 
   // For iteration
-  IRNode *getIterator(ReaderStackIterator **) override;
-  IRNode *iteratorGetNext(ReaderStackIterator **) override;
-  void iteratorReplace(ReaderStackIterator **, IRNode *) override;
-  IRNode *getReverseIterator(ReaderStackIterator **) override;
-  IRNode *getReverseIteratorFromDepth(ReaderStackIterator **,
-                                      uint32_t Depth) override;
-  IRNode *reverseIteratorGetNext(ReaderStackIterator **) override;
+  IRNode *getIterator(ReaderStackIterator& Iterator) override;
+  IRNode *iteratorGetNext(ReaderStackIterator& Iterator) override;
+  void iteratorReplace(ReaderStackIterator& Iterator, IRNode *) override;
+  IRNode *getReverseIterator(ReaderStackIterator& Iterator) override;
+  IRNode *getReverseIteratorFromDepth(ReaderStackIterator& Iterator,
+    uint32_t Depth) override;
+  IRNode *reverseIteratorGetNext(ReaderStackIterator& Iterator) override;
 
 #if !defined(NODEBUG)
   void print() override;
@@ -522,8 +522,6 @@ public:
                                   bool *IsMultiByteAssign) override {
     throw NotYetImplementedException("assignToSuccessorStackNode");
   };
-  //    ReaderStackNode* CopyStackList(ReaderStackNode* stack) =
-  //    0;
   bool typesCompatible(IRNode *Src1, IRNode *Src2) override {
     throw NotYetImplementedException("typesCompatible");
   };

lib/Reader/reader.cpp

@@ -5896,7 +5896,7 @@ void ReaderBase::handleNonEmptyStack(FlowGraphNode *Fg, IRNode **NewIR,
                                      bool *FmbAssign) {
   FlowGraphEdgeList *SuccessorList;
   FlowGraphNode *SuccessorBlock;
-  ReaderStackIterator *Iterator;
+  ReaderStackIterator Iterator;
   IRNode *CurrentNode;
 
 #ifndef NODEBUG
@@ -5935,19 +5935,19 @@ void ReaderBase::handleNonEmptyStack(FlowGraphNode *Fg, IRNode **NewIR,
   // unpopulated) then we must construct an operand stack using the
   // CurrentNoderent reader stack.
   if (SuccessorStack == NULL) {
-    ReaderStackIterator *TargetIterator;
+    ReaderStackIterator TargetIterator;
 
     // 2. Create stack typed stack of tmpvars.
     SuccessorStack = ReaderOperandStack->copy();
 
     // Push new elements onto temp stack.
-    CurrentNode = ReaderOperandStack->getIterator(&Iterator);
-    SuccessorStack->getIterator(&TargetIterator);
+    CurrentNode = ReaderOperandStack->getIterator(Iterator);
+    SuccessorStack->getIterator(TargetIterator);
     while (CurrentNode != NULL) {
-      SuccessorStack->iteratorReplace(&TargetIterator,
+      SuccessorStack->iteratorReplace(TargetIterator,
                                       makeStackTypeNode(CurrentNode));
-      CurrentNode = ReaderOperandStack->iteratorGetNext(&Iterator);
-      SuccessorStack->iteratorGetNext(&TargetIterator);
+      CurrentNode = ReaderOperandStack->iteratorGetNext(Iterator);
+      SuccessorStack->iteratorGetNext(TargetIterator);
     }
 
     // Copy operand stack onto all edges in the CurrentNoderent web.
@@ -6065,15 +6065,15 @@ void ReaderBase::handleNonEmptyStack(FlowGraphNode *Fg, IRNode **NewIR,
 
   // Assign CurrentNoderent stack elements to tmpvars on successor operand
   // stack.
-  ReaderStackIterator *TargetIterator;
+  ReaderStackIterator TargetIterator;
   IRNode *Target;
 
-  CurrentNode = ReaderOperandStack->getIterator(&Iterator);
-  Target = SuccessorStack->getIterator(&TargetIterator);
+  CurrentNode = ReaderOperandStack->getIterator(Iterator);
+  Target = SuccessorStack->getIterator(TargetIterator);
   while ((CurrentNode != NULL) && (Target != NULL)) {
     assignToSuccessorStackNode(Fg, Target, CurrentNode, NewIR, FmbAssign);
-    CurrentNode = ReaderOperandStack->iteratorGetNext(&Iterator);
-    Target = SuccessorStack->iteratorGetNext(&TargetIterator);
+    CurrentNode = ReaderOperandStack->iteratorGetNext(Iterator);
+    Target = SuccessorStack->iteratorGetNext(TargetIterator);
   }
 
 #if !defined(CC_PEVERIFY)

lib/Reader/readerir.cpp

@@ -42,6 +42,8 @@ GenStack::GenStack(uint32_t MaxStack, ReaderBase *Rdr) {
 }
 
 void GenStack::push(IRNode *NewVal, IRNode **NewIR) {
+  ASSERT(NewVal != NULL);
+  ASSERT(GenIR::isValidStackType(NewVal));
 
   // Note that in this client, due to inlining and jmp, MaxStack
   // is not a hard and fast value we can count on.  It is possible
@@ -58,11 +60,7 @@ void GenStack::push(IRNode *NewVal, IRNode **NewIR) {
     Max = NewSize - 1;
   }
 
-  IRNode *Result = NewVal;
-
-  ASSERT(GenIR::isValidStackType(Result));
-
-  Stack[++Top] = Result;
+  Stack[++Top] = NewVal;
 }
 
 IRNode *GenStack::pop() {
@@ -81,57 +79,55 @@ void GenStack::assertEmpty() { ASSERT(empty()); }
 
 uint32_t GenStack::depth() { return (uint32_t)(Top + 1); }
 
-IRNode *GenStack::getIterator(ReaderStackIterator **Iterator) {
-  *((size_t *)Iterator) = Top;
+IRNode *GenStack::getIterator(ReaderStackIterator& Iterator) {
+  Iterator = Top;
   if (empty())
     return NULL;
   else
-    return (IRNode *)Stack[Top];
+    return Stack[Top];
 }
 
-IRNode *GenStack::iteratorGetNext(ReaderStackIterator **Iterator) {
-  size_t *I = (size_t *)Iterator;
-  if (*I == 0)
+IRNode *GenStack::iteratorGetNext(ReaderStackIterator& Iterator) {
+  if (Iterator == 0)
     return NULL;
-  return Stack[--(*I)];
+  return Stack[--Iterator];
 }
 
-void GenStack::iteratorReplace(ReaderStackIterator **Iterator, IRNode *Node) {
-  Stack[*((size_t *)Iterator)] = Node;
+void GenStack::iteratorReplace(ReaderStackIterator& Iterator, IRNode *Node) {
+  Stack[Iterator] = Node;
 }
 
-IRNode *GenStack::getReverseIterator(ReaderStackIterator **Iterator) {
+IRNode *GenStack::getReverseIterator(ReaderStackIterator& Iterator) {
   return getReverseIteratorFromDepth(Iterator, depth());
 }
 
-IRNode *GenStack::getReverseIteratorFromDepth(ReaderStackIterator **Iterator,
-                                              uint32_t Depth) {
+IRNode *GenStack::getReverseIteratorFromDepth(ReaderStackIterator& Iterator,
+  uint32_t Depth) {
   size_t Idx = (size_t)(Top - Depth + 1);
   ASSERT(Idx >= 0);
-  *((size_t *)Iterator) = Idx;
+  Iterator = Idx;
   if (empty())
     return NULL;
   else
-    return (IRNode *)Stack[Idx];
+    return Stack[Idx];
 }
 
-IRNode *GenStack::reverseIteratorGetNext(ReaderStackIterator **Iterator) {
-  size_t *I = (size_t *)Iterator;
-  if (*I == Top)
+IRNode *GenStack::reverseIteratorGetNext(ReaderStackIterator& Iterator) {
+  if (Iterator == Top)
     return NULL;
-  return Stack[++(*I)];
+  return Stack[++Iterator];
 }
 
 #if !defined(NODEBUG)
 void GenStack::print() {
   dbgs() << "{GenStack dump, Top first, depth = " << depth() << '\n';
-  ReaderStackIterator *Iterator;
-  IRNode *N = getIterator(&Iterator);
+  ReaderStackIterator Iterator;
+  IRNode *N = getIterator(Iterator);
   int32_t I = 0;
   while (N) {
     dbgs() << "[" << I++ << "]: ";
     Reader->dbPrintIRNode(N);
-    N = iteratorGetNext(&Iterator);
+    N = iteratorGetNext(Iterator);
   }
   dbgs() << "}\n";
 }
@@ -141,7 +137,7 @@ ReaderStack *GenStack::copy() {
   GenStack *Copy;
 
   void *Buffer = Reader->getTempMemory(sizeof(GenStack));
-  Copy = new (Buffer) GenStack(Max + 1, Reader);
+  Copy = new (Buffer)GenStack(Max + 1, Reader);
   Copy->Top = Top;
   for (int32_t I = 0; I <= Top; I++)
     Copy->Stack[I] = Stack[I];
@@ -151,7 +147,7 @@ ReaderStack *GenStack::copy() {
 ReaderStack *GenIR::createStack(uint32_t MaxStack, ReaderBase *Reader) {
   void *Buffer = Reader->getTempMemory(sizeof(GenStack));
   // extra 16 should reduce frequency of reallocation when inlining / jmp
-  return new (Buffer) GenStack(MaxStack + 16, Reader);
+  return new (Buffer)GenStack(MaxStack + 16, Reader);
 }
 
 #pragma endregion
@@ -308,6 +304,7 @@ void rgnSetCatchClassToken(EHRegion *CatchRegion, mdToken Token) { return; }
 //===----------------------------------------------------------------------===//
 
 // Get memory that will be freed at end of reader
+
 void *GenIR::getTempMemory(size_t NumBytes) { return calloc(1, NumBytes); }
 
 // Get memory that will persist after the reader
@@ -4309,8 +4306,8 @@ void GenIR::maintainOperandStack(IRNode **Opr1, IRNode **Opr2,
         CreatePHIs = true;
       }
 
-      ReaderStackIterator *Iterator;
-      IRNode *Current = ReaderOperandStack->getReverseIterator(&Iterator);
+      ReaderStackIterator Iterator;
+      IRNode *Current = ReaderOperandStack->getReverseIterator(Iterator);
       Instruction *CurrentInst = SuccessorBlock->begin();
       PHINode *Phi = nullptr;
       while (Current != nullptr) {
@@ -4336,7 +4333,7 @@ void GenIR::maintainOperandStack(IRNode **Opr1, IRNode **Opr2,
         }
         Phi->addIncoming(CurrentValue, (BasicBlock *)CurrentBlock);
         SuccessorStack->push((IRNode *)Phi, NewIR);
-        Current = ReaderOperandStack->reverseIteratorGetNext(&Iterator);
+        Current = ReaderOperandStack->reverseIteratorGetNext(Iterator);
       }
 
       // The number if PHI instructions should match the number of values on the