Fix processing of local variable indices in fuction blocks.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index 6ff9d97929ba5bc5d48ac61738786cd3e150bbc2..61f2a8999a21dbcd76746b92c6498d24c67c591a 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -1266,12 +1266,6 @@ public:
       return Context->getGlobalConstantByID(Index);
     }
     NaClBcIndexSize_t LocalIndex = Index - CachedNumGlobalValueIDs;
-    if (LocalIndex >= LocalOperands.size()) {
-      std::string Buffer;
-      raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Value index " << Index << " not defined!";
-      Fatal(StrBuf.str());
-    }
     Ice::Operand *Op = LocalOperands[LocalIndex];
     if (Op == nullptr) {
       if (isIRGenerationDisabled())
@@ -1294,6 +1288,7 @@ public:
   }
 
 private:
+  typedef std::unordered_map<NaClBcIndexSize_t, Ice::Operand *> OperandsMap;

[Jim Stichnoth, 2015/08/08 16:14:48]

For naming consistency, I would use "OperandMap" or "CfgNodesMap".  (I prefer
OperandMap/CfgNodeMap, fwiw.)

[Karl, 2015/08/10 18:13:09]

Done.

   typedef std::unordered_map<NaClBcIndexSize_t, Ice::CfgNode *> CfgNodeMap;
 
   Ice::TimerMarker Timer;
@@ -1316,7 +1311,7 @@ private:
   size_t CachedNumGlobalValueIDs;
   // Holds operands local to the function block, based on indices
   // defined in the bitcode file.
-  std::vector<Ice::Operand *> LocalOperands;
+  OperandsMap LocalOperands;
   // Holds the index within LocalOperands corresponding to the next
   // instruction that generates a value.
   NaClBcIndexSize_t NextLocalInstIndex;
@@ -1352,6 +1347,8 @@ private:
 
   bool verifyAndRenameBasicBlocks();
 
+  bool verifyAllForwardRefsDefined();
+
   // Returns the Index-th basic block in the list of basic blocks.
   // Assumes Index corresponds to a branch instruction. Hence, if
   // the branch references the entry block, it also generates a
@@ -1427,34 +1424,26 @@ private:
     assert(Op || isIRGenerationDisabled());
     // Check if simple push works.
     NaClBcIndexSize_t LocalIndex = Index - CachedNumGlobalValueIDs;
-    if (LocalIndex == LocalOperands.size()) {
-      LocalOperands.push_back(Op);
-      return;
-    }
-
-    // Must be forward reference, expand vector to accommodate.
-    if (LocalIndex >= LocalOperands.size())
-      LocalOperands.resize(LocalIndex + 1);
 
     // If element not defined, set it.
-    Ice::Operand *OldOp = LocalOperands[LocalIndex];
-    if (OldOp == nullptr) {
-      LocalOperands[LocalIndex] = Op;
+    Ice::Operand *&IndexedOp = LocalOperands[LocalIndex];
+    if (IndexedOp == nullptr) {
+      IndexedOp = Op;
       return;
     }
 
-    // See if forward reference matches.
-    if (OldOp == Op)
+    // See if forward reference matchers.
+    if (IndexedOp == Op)
       return;
 
     // Error has occurred.
     std::string Buffer;
     raw_string_ostream StrBuf(Buffer);
     StrBuf << "Multiple definitions for index " << Index << ": " << *Op
-           << " and " << *OldOp;
+           << " and " << *IndexedOp;
     Error(StrBuf.str());
     // TODO(kschimpf) Remove error recovery once implementation complete.

[Jim Stichnoth, 2015/08/08 16:14:48]

Trying to remember the context of these TODOs...  Isn't implementation complete
by now?

[Karl, 2015/08/10 18:13:09]

This was back when someone wasn't sure we should keep error recovery, once we
get things working (i.e. consider it only for debugging). I think that keeping
error recovery around is good, and will fix in a separate CL>

-    LocalOperands[LocalIndex] = Op;
+    IndexedOp = Op;
   }
 
   // Returns the relative operand (wrt to BaseIndex) referenced by
@@ -1958,6 +1947,26 @@ private:
   }
 };
 
+bool FunctionParser::verifyAllForwardRefsDefined() {
+  NaClBcIndexSize_t NumInstructions =
+      NextLocalInstIndex - CachedNumGlobalValueIDs;
+  if (NumInstructions == LocalOperands.size())
+    return true;
+  // Find undefined forward references and report.
+  std::vector<NaClBcIndexSize_t> UndefinedFwdRefs;
+  for (const std::pair<NaClBcIndexSize_t, Ice::Operand *> Elmt : LocalOperands)

[Jim Stichnoth, 2015/08/08 16:14:48]

I think this calls for auto:

  for (auto &Elmt : LocalOperands)

Or if not:

  for (const OperandsMap::value_type &Elmt : LocalOperands)

[Karl, 2015/08/10 18:13:09]

Good point. I didn't put the auto in because I didn't think the type was clear
enough. However, Using the second form is clear and simpler.

+    if (Elmt.first >= NextLocalInstIndex)
+      UndefinedFwdRefs.push_back(Elmt.first);
+  std::sort(UndefinedFwdRefs.begin(), UndefinedFwdRefs.end());
+  for (const NaClBcIndexSize_t Index : UndefinedFwdRefs) {
+    std::string Buffer;
+    raw_string_ostream StrBuf(Buffer);
+    StrBuf << "Instruction forward reference not defined: " << Index;
+    Error(StrBuf.str());
+  }
+  return false;
+}
+
 bool FunctionParser::verifyAndRenameBasicBlocks() {
   const size_t NumFoundBbs = BbMap.size();
   // Verify number of basic blocks found match amount specified in function.
@@ -2007,6 +2016,8 @@ void FunctionParser::ExitBlock() {
   }
   if (isIRGenerationDisabled())
     return;
+  if (!verifyAllForwardRefsDefined())
+    return;
   if (!verifyAndRenameBasicBlocks())
     return;
   // Before translating, check for blocks without instructions, and