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

src/PNaClTranslator.cpp

 //===- subzero/src/PNaClTranslator.cpp - ICE from bitcode -----------------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 288 matching lines @@
   /// Returns the number of global declarations (i.e. IDs) defined in
   /// the bitcode file.
   size_t getNumGlobalIDs() const {
     if (VariableDeclarations) {
       return FunctionDeclarationList.size() + VariableDeclarations->size();
     } else {
       return ValueIDConstants.size();
     }
   }
 
-  /// 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.
   Ice::VariableDeclaration *getGlobalVariableByID(NaClBcIndexSize_t Index) {
     assert(VariableDeclarations);
     if (Index < VariableDeclarations->size())
       return VariableDeclarations->at(Index);
     return reportGetGlobalVariableByIDError(Index);
   }
 
@@ skipping 613 matching lines @@
   GlobalsParser() = delete;
   GlobalsParser(const GlobalsParser &) = delete;
   GlobalsParser &operator=(const GlobalsParser &) = delete;
 
 public:
   GlobalsParser(unsigned BlockID, BlockParserBaseClass *EnclosingParser)
       : BlockParserBaseClass(BlockID, EnclosingParser),
         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;
 
   // The index of the next global variable declaration.
   NaClBcIndexSize_t NextGlobalID = 0;
 
   // Dummy global variable declaration to guarantee CurGlobalVar is
   // always defined (allowing code to not need to check if
   // CurGlobalVar is nullptr).
   Ice::VariableDeclaration *DummyGlobalVar;
 
   // Holds the current global variable declaration being built.
   Ice::VariableDeclaration *CurGlobalVar;
 
+  // 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 " << 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();
-    unsigned NumIDs = Context->getNumGlobalVariables();
-    if (NextGlobalID < NumIDs) {
-      std::string Buffer;
-      raw_string_ostream StrBuf(Buffer);
-      StrBuf << getBlockName() << " block expects " << NumIDs
-             << " global variable declarations. Found: " << NextGlobalID;
-      Error(StrBuf.str());
-    }
+    installGlobalVariables();
     BlockParserBaseClass::ExitBlock();
   }
 
   void ProcessRecord() override;
 
   // Checks if the number of initializers for the CurGlobalVar is the same as
   // the number found in the bitcode file. If different, and error message is
   // generated, and the internal state of the parser is fixed so this condition
   // is no longer violated.
   void verifyNoMissingInitializers() {
@@ skipping 12 matching lines @@
   }
 };
 
 void GlobalsParser::ProcessRecord() {
   const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
   switch (Record.GetCode()) {
   case naclbitc::GLOBALVAR_COUNT:
     // 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);
     }
     ++NextGlobalID;
     return;
   }
   case naclbitc::GLOBALVAR_COMPOUND:
     // COMPOUND: [size]
     if (!isValidRecordSize(1, "compound"))
       return;
@@ skipping 32 matching lines @@
     CurGlobalVar->addInitializer(
         Ice::VariableDeclaration::DataInitializer::create(Values));
     return;
   }
   case naclbitc::GLOBALVAR_RELOC: {
     // RELOC: [val, [addend]]
     if (!isValidRecordSizeInRange(1, 2, "reloc"))
       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];
       if (Offset > std::numeric_limits<uint32_t>::max()) {
         std::string Buffer;
         raw_string_ostream StrBuf(Buffer);
         StrBuf << "Addend of global reloc record too big: " << Offset;
         Error(StrBuf.str());
       }
     }
     CurGlobalVar->addInitializer(
         Ice::VariableDeclaration::RelocInitializer::create(
-            Context->getGlobalDeclarationByID(Index), Offset));
+            getGlobalDeclByID(Index), Offset));
     return;
   }
   default:
     BlockParserBaseClass::ProcessRecord();
     return;
   }
 }
 
 /// Base class for parsing a valuesymtab block in the bitcode file.
 class ValuesymtabParser : public BlockParserBaseClass {
@@ skipping 2025 matching lines @@
   }
   if (InputStreamFile.getBitcodeBytes().getExtent() % 4 != 0) {
     ErrStream
         << IRFilename
         << ": Bitcode stream should be a multiple of 4 bytes in length.\n";
     llvm::report_fatal_error("Bitcode stream should be a multiple of 4 bytes");
   }
 }
 
 } // end of namespace Ice