Fix processing of global variable indices in the global vars block.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index 6ff9d97929ba5bc5d48ac61738786cd3e150bbc2..5494c62946305ac773fe1bea988a2d1d9c99409d 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -306,24 +306,11 @@ public:
     }
   }
 
-  /// Creates Count global variable declarations.
-  void createGlobalVariables(NaClBcIndexSize_t Count) {
+  /// Adds the given global declaration to the end of the list of global
+  /// declarations.
+  void addGlobalDeclaration(Ice::VariableDeclaration *Decl) {
     assert(VariableDeclarations);
-    assert(VariableDeclarations->empty());
-    for (size_t i = 0; i < Count; ++i) {
-      VariableDeclarations->push_back(
-          Ice::VariableDeclaration::create(getTranslator().getContext()));
-    }
-  }
-
-  /// Returns the number of global variable declarations in the
-  /// bitcode file.
-  size_t getNumGlobalVariables() const {
-    if (VariableDeclarations) {
-      return VariableDeclarations->size();
-    } else {
-      return ValueIDConstants.size() - FunctionDeclarationList.size();
-    }
+    VariableDeclarations->push_back(Decl);
   }
 
   /// Returns the global variable declaration with the given index.
@@ -957,14 +944,30 @@ public:
         Timer(Ice::TimerStack::TT_parseGlobals, getTranslator().getContext()),
         DummyGlobalVar(
             Ice::VariableDeclaration::create(getTranslator().getContext())),
-        CurGlobalVar(DummyGlobalVar) {}
+        CurGlobalVar(DummyGlobalVar) {
+    NumFunctionIDs = Context->getNumFunctionIDs();

[Jim Stichnoth, 2015/08/06 22:09:12]

Can this be added to the member initializer list?  Or is Context not stabilized
at that point?

[Karl, 2015/08/07 16:29:22]

I originally thought it had to wait. However, since it gets initialized after
the base class, it can be initialized as a member. Changing.

+  }
 
   ~GlobalsParser() final = default;
 
   const char *getBlockName() const override { return "globals"; }
 
 private:
+  typedef std::unordered_map<NaClBcIndexSize_t, Ice::VariableDeclaration *>
+      GlobalVarsMapType;
+
   Ice::TimerMarker Timer;
+
+  // Holds global variables generated/referenced in the global variables block.
+  GlobalVarsMapType GlobalVarsMap;
+
+  // Holds the number of defined function IDs.
+  NaClBcIndexSize_t NumFunctionIDs;
+
+  // Holds the specified number of global variables by the count record in
+  // the global variables block.
+  NaClBcIndexSize_t SpecifiedNumberVars = 0;
+
   // Keeps track of how many initializers are expected for the global variable
   // declaration being built.
   NaClBcIndexSize_t InitializersNeeded = 0;
@@ -980,16 +983,43 @@ private:
   // Holds the current global variable declaration being built.
   Ice::VariableDeclaration *CurGlobalVar;
 
-  void ExitBlock() override {
-    verifyNoMissingInitializers();
-    unsigned NumIDs = Context->getNumGlobalVariables();
-    if (NextGlobalID < NumIDs) {
+  // Returns the global variable associated with the given Index.
+  Ice::VariableDeclaration *getGlobalVarByID(NaClBcIndexSize_t Index) {
+    Ice::VariableDeclaration *&Decl = GlobalVarsMap[Index];
+    if (Decl == nullptr)
+      Decl = Ice::VariableDeclaration::create(getTranslator().getContext());
+    return Decl;
+  }
+
+  // Returns the global declaratoin associated with the given index.

[Jim Stichnoth, 2015/08/06 22:09:12]

declaration

[Karl, 2015/08/07 16:29:22]

Done.

+  Ice::GlobalDeclaration *getGlobalDeclByID(NaClBcIndexSize_t Index) {
+    if (Index < NumFunctionIDs)
+      return Context->getFunctionByID(Index);
+    return getGlobalVarByID(Index - NumFunctionIDs);
+  }
+
+  // If global variables parsed correctly, install them into the top-level
+  // context.
+  void installGlobalVariables() {
+    // Verify specified number of globals matches number found.
+    size_t NumGlobals = GlobalVarsMap.size();
+    if (SpecifiedNumberVars != NumGlobals ||
+        SpecifiedNumberVars != NextGlobalID) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
-      StrBuf << getBlockName() << " block expects " << NumIDs
-             << " global variable declarations. Found: " << NextGlobalID;
+      StrBuf << getBlockName() << " block expects " << SpecifiedNumberVars
+             << " global variables. Found: " << GlobalVarsMap.size();
       Error(StrBuf.str());
+      return;
     }
+    // Install global variables into top-level context.
+    for (size_t I = 0; I < NumGlobals; ++I)
+      Context->addGlobalDeclaration(GlobalVarsMap[I]);
+  }
+
+  void ExitBlock() override {
+    verifyNoMissingInitializers();
+    installGlobalVariables();
     BlockParserBaseClass::ExitBlock();
   }
 
@@ -1022,20 +1052,23 @@ void GlobalsParser::ProcessRecord() {
     // COUNT: [n]
     if (!isValidRecordSize(1, "count"))
       return;
-    if (NextGlobalID != Context->getNumGlobalVariables()) {
+    if (SpecifiedNumberVars || NextGlobalID) {
       Error("Globals count record not first in block.");
       return;
     }
-    Context->createGlobalVariables(Values[0]);
+    SpecifiedNumberVars = Values[0];
     return;
   case naclbitc::GLOBALVAR_VAR: {
     // VAR: [align, isconst]
     if (!isValidRecordSize(2, "variable"))
       return;
     verifyNoMissingInitializers();
+    // Always build the global variable, even if IR generation is turned off.
+    // This is needed because we need a placeholder in the top-level context
+    // when no IR is generated.
+    CurGlobalVar = getGlobalVarByID(NextGlobalID);
     if (!isIRGenerationDisabled()) {
       InitializersNeeded = 1;
-      CurGlobalVar = Context->getGlobalVariableByID(NextGlobalID);
       CurGlobalVar->setAlignment((1 << Values[0]) >> 1);
       CurGlobalVar->setIsConstant(Values[1] != 0);
     }
@@ -1088,7 +1121,15 @@ void GlobalsParser::ProcessRecord() {
       return;
     if (isIRGenerationDisabled())
       return;
-    unsigned Index = Values[0];

[Jim Stichnoth, 2015/08/06 22:09:12]

Nice.  Any other "unsigned" types that you can change to something like
NaClBcIndexSize_t, would also be welcome.  (I generally try to avoid "int" and
"unsigned" in favor of int32_t, uint32_t, size_t, etc.)

[Karl, 2015/08/07 16:29:22]

Acknowledged.

+    NaClBcIndexSize_t Index = Values[0];
+    NaClBcIndexSize_t IndexLimit = SpecifiedNumberVars + NumFunctionIDs;
+    if (Index >= IndexLimit) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Relocation index " << Index << " to big. Expect index < "
+             << IndexLimit;
+      Error(StrBuf.str());
+    }
     uint64_t Offset = 0;
     if (Values.size() == 2) {
       Offset = Values[1];
@@ -1101,7 +1142,7 @@ void GlobalsParser::ProcessRecord() {
     }
     CurGlobalVar->addInitializer(
         Ice::VariableDeclaration::RelocInitializer::create(
-            Context->getGlobalDeclarationByID(Index), Offset));
+            getGlobalDeclByID(Index), Offset));
     return;
   }
   default: