Fix PNaCl bitcode reader to release global variables to emitter.

src/PNaClTranslator.cpp

Index: src/PNaClTranslator.cpp
diff --git a/src/PNaClTranslator.cpp b/src/PNaClTranslator.cpp
index 46bd26159e906f80af701e7e97807c723cd0c71f..453b3bf2400df4d0b493f38cf0765d4c5e198d97 100644
--- a/src/PNaClTranslator.cpp
+++ b/src/PNaClTranslator.cpp
@@ -163,6 +163,7 @@ public:
                  NaClBitstreamCursor &Cursor, Ice::ErrorCode &ErrorStatus)
       : NaClBitcodeParser(Cursor), Translator(Translator), Header(Header),
         ErrorStatus(ErrorStatus), NumErrors(0), NextDefiningFunctionID(0),
+        VariableDeclarations(new Ice::VariableDeclarationList()),
         BlockParser(nullptr), StubbedConstCallValue(nullptr) {}
 
   ~TopLevelParser() override {}
@@ -251,71 +252,27 @@ public:
     return reportGetFunctionByIDError(ID);
   }
 
-  /// Returns the list of function declarations.
-  const FunctionDeclarationListType &getFunctionDeclarationList() const {
-    return FunctionDeclarationList;
+  /// Returns the constant associated with the given global value ID.
+  Ice::Constant *getGlobalConstantByID(unsigned ID) {
+    assert(ID < ValueIDConstants.size());
+    return ValueIDConstants[ID];
   }
 
-  /// Returns the corresponding constant associated with a global declaration.
-  /// (i.e. relocatable).
-  Ice::Constant *getOrCreateGlobalConstantByID(unsigned ID) {
-    // TODO(kschimpf): Can this be built when creating global initializers?
-    Ice::Constant *C;
-    if (ID >= ValueIDConstants.size()) {
-      C = nullptr;
-      unsigned ExpectedSize =
-          FunctionDeclarationList.size() + VariableDeclarations.size();
-      if (ID >= ExpectedSize)
-        ExpectedSize = ID;
-      ValueIDConstants.resize(ExpectedSize);
-    } else {
-      C = ValueIDConstants[ID];
-    }
-    if (C != nullptr)
-      return C;
+  /// Install names for all global values without names. Called after
+  /// the global value symbol table is processed, but before any
+  /// function blocks are processed.
+  void installGlobalNames() {
+    assert(VariableDeclarations);
+    installGlobalVarNames();
+    installFunctionNames();
+  }
 
-    if (isIRGenerationDisabled()) {
-      ValueIDConstants[ID] = nullptr;
-      return nullptr;
-    }
-
-    // If reached, no such constant exists, create one.
-    // TODO(kschimpf) Don't get addresses of intrinsic function declarations.
-    Ice::GlobalDeclaration *Decl = nullptr;
-    unsigned FcnIDSize = FunctionDeclarationList.size();
-    bool IsUndefined = false;
-    if (ID < FcnIDSize) {
-      Decl = FunctionDeclarationList[ID];
-      const auto Func = llvm::cast<Ice::FunctionDeclaration>(Decl);
-      IsUndefined = Func->isProto();
-    } else if ((ID - FcnIDSize) < VariableDeclarations.size()) {
-      Decl = VariableDeclarations[ID - FcnIDSize];
-      const auto Var = llvm::cast<Ice::VariableDeclaration>(Decl);
-      IsUndefined = !Var->hasInitializer();
-    }
-    Ice::IceString Name;
-    bool SuppressMangling;
-    if (Decl) {
-      Name = Decl->getName();
-      SuppressMangling = Decl->getSuppressMangling();
-    } else {
-      std::string Buffer;
-      raw_string_ostream StrBuf(Buffer);
-      StrBuf << "Reference to global not defined: " << ID;
-      BlockError(StrBuf.str());
-      // TODO(kschimpf) Remove error recovery once implementation complete.
-      Name = "??";
-      SuppressMangling = false;
-    }
-    if (IsUndefined)
-      C = getTranslator().getContext()->getConstantExternSym(Name);
-    else {
-      const Ice::RelocOffsetT Offset = 0;
-      C = getTranslator().getContext()->getConstantSym(Offset, Name,
-                                                       SuppressMangling);
-    }
-    ValueIDConstants[ID] = C;
-    return C;
+  void createValueIDs() {
+    assert(VariableDeclarations);
+    ValueIDConstants.reserve(VariableDeclarations->size() +
+                             FunctionDeclarationList.size());
+    createValueIDsForFunctions();
+    createValueIDsForGlobalVars();
   }
 
   /// Returns a defined function reference to be used in place of
@@ -328,7 +285,7 @@ public:
     for (unsigned i = 0; i < getNumFunctionIDs(); ++i) {
       Ice::FunctionDeclaration *Func = getFunctionByID(i);
       if (!Func->isProto()) {
-        StubbedConstCallValue = getOrCreateGlobalConstantByID(i);
+        StubbedConstCallValue = getGlobalConstantByID(i);
         return StubbedConstCallValue;
       }
     }
@@ -342,27 +299,37 @@ public:
   /// Returns the number of global declarations (i.e. IDs) defined in
   /// the bitcode file.
   unsigned getNumGlobalIDs() const {
-    return FunctionDeclarationList.size() + VariableDeclarations.size();
+    if (VariableDeclarations) {
+      return FunctionDeclarationList.size() + VariableDeclarations->size();
+    } else {
+      return ValueIDConstants.size();
+    }
   }
 
   /// Creates Count global variable declarations.
   void CreateGlobalVariables(size_t Count) {
-    assert(VariableDeclarations.empty());
+    assert(VariableDeclarations);
+    assert(VariableDeclarations->empty());
     for (size_t i = 0; i < Count; ++i) {
-      VariableDeclarations.push_back(Ice::VariableDeclaration::create());
+      VariableDeclarations->push_back(Ice::VariableDeclaration::create());
     }
   }
 
   /// Returns the number of global variable declarations in the
   /// bitcode file.
   Ice::SizeT getNumGlobalVariables() const {
-    return VariableDeclarations.size();
+    if (VariableDeclarations) {
+      return VariableDeclarations->size();
+    } else {
+      return ValueIDConstants.size() - FunctionDeclarationList.size();
+    }
   }
 
   /// Returns the global variable declaration with the given index.
   Ice::VariableDeclaration *getGlobalVariableByID(unsigned Index) {
-    if (Index < VariableDeclarations.size())
-      return VariableDeclarations[Index];
+    assert(VariableDeclarations);
+    if (Index < VariableDeclarations->size())
+      return VariableDeclarations->at(Index);
     return reportGetGlobalVariableByIDError(Index);
   }
 
@@ -376,9 +343,14 @@ public:
       return getGlobalVariableByID(Index - NumFunctionIds);
   }
 
-  /// Returns the list of parsed global variable declarations.
-  const Ice::VariableDeclarationList &getGlobalVariables() {
-    return VariableDeclarations;
+  /// Returns the list of parsed global variable declarations. Releases
+  /// ownership of the current list of global variables.
+  std::unique_ptr<Ice::VariableDeclarationList> &&getGlobalVariables() {

[Jim Stichnoth, 2015/02/10 17:47:11]

Does this actually have to have "&&"?  I thought it would just be:

std::unique_ptr<Ice::VariableDeclarationList> getGlobalVariables() {

[Karl, 2015/02/10 19:30:23]

I thought that would be more efficient, but I'll remove it.

+    // Before returning, check that ValidIDConstants has already been
+    // build.

[Jim Stichnoth, 2015/02/10 17:47:11]

built

[Karl, 2015/02/10 19:30:23]

Done.

+    assert(!VariableDeclarations ||

[jvoung (off chromium), 2015/02/10 18:24:58]

Why is it okay for "!VariableDeclarations"?

I assume this will only be called once. Is there a use where it's not expected
to be initialized?

[Jim Stichnoth, 2015/02/10 18:29:01]

Hmm, I was under the impression that this could potentially be called multiple
times, even though the current implementation only happens to call it once. 
Whatever the case, I guess it should be documented, probably in IceTranslator.h
or .cpp.

[Karl, 2015/02/10 19:30:23]

The code assumes it is only built once. I wanted to make the code do something
reasonable if it (in the future) gets called more than once. I assumed that
returning nullptr on later calls would allow the user (if it needs to) to know
if this was the first call. I updated this comment to communicate this.

+           VariableDeclarations->size() <= ValueIDConstants.size());
+    return std::move(VariableDeclarations);
   }
 
 private:
@@ -402,7 +374,7 @@ private:
   // actually-defined function.
   size_t NextDefiningFunctionID;
   // The set of global variables.
-  Ice::VariableDeclarationList VariableDeclarations;
+  std::unique_ptr<Ice::VariableDeclarationList> VariableDeclarations;
   // Relocatable constants associated with global declarations.
   std::vector<Ice::Constant *> ValueIDConstants;
   // Error recovery value to use when getFuncSigTypeByID fails.
@@ -432,6 +404,84 @@ private:
     return nullptr;
   }
 
+  // Gives Decl a name if it doesn't already have one. Prefix and
+  // NameIndex is used to generate the name. NameIndex is

[Jim Stichnoth, 2015/02/10 17:47:11]

are used

[Karl, 2015/02/10 19:30:23]

Done.

+  // automatically incremented if a new new is created.

[Jim Stichnoth, 2015/02/10 17:47:11]

"new new" ==> ???

[Karl, 2015/02/10 19:30:23]

Fixed.

+  // DeclType is literal text describing the type of name being
+  // created.

[Jim Stichnoth, 2015/02/10 17:47:11]

Mention that it's used just for error reporting?  (assuming that's true)

[Karl, 2015/02/10 19:30:23]

This is not true. As stated in the comment, it creates a name if it doesn't have
one. However, updated comment to communicate that it adds warning if
appropriate.

+  void installDeclarationName(Ice::GlobalDeclaration *Decl,
+                              const Ice::IceString &Prefix,
+                              const char *DeclType, uint32_t &NameIndex) {
+    if (!Decl->hasName()) {

[Jim Stichnoth, 2015/02/10 17:47:11]

Rewrite as
if (Decl->hasName()) {
  Translator...;
} else {
  Decl->setName(...);
}

[Karl, 2015/02/10 19:30:22]

Done.

+      Decl->setName(Translator.createUnnamedName(Prefix, NameIndex));
+      ++NameIndex;
+    } else {
+      Translator.checkIfUnnamedNameSafe(Decl->getName(), DeclType, Prefix);
+    }
+  }
+
+  // Installs names for global variables without names.
+  void installGlobalVarNames() {
+    assert(VariableDeclarations);
+    const Ice::IceString &GlobalPrefix =
+        getTranslator().getFlags().getDefaultGlobalPrefix();
+    if (!GlobalPrefix.empty()) {
+      uint32_t NameIndex = 0;
+      for (Ice::VariableDeclaration *Var : *VariableDeclarations) {
+        installDeclarationName(Var, GlobalPrefix, "global", NameIndex);
+      }
+    }
+  }
+
+  // Installs names for functions without names.
+  void installFunctionNames() {
+    const Ice::IceString &FunctionPrefix =
+        getTranslator().getFlags().getDefaultFunctionPrefix();
+    if (!FunctionPrefix.empty()) {
+      uint32_t NameIndex = 0;
+      for (Ice::FunctionDeclaration *Func : FunctionDeclarationList) {
+        installDeclarationName(Func, FunctionPrefix, "function", NameIndex);
+      }
+    }
+  }
+
+  // Builds a constant symbol named Name, suppressing name mangling if
+  // SuppressMangling.  IsExternal is true iff the symbol is external.
+  Ice::Constant *getConstantSym(const Ice::IceString &Name,
+                                bool SuppressMangling, bool IsExternal) {
+    if (IsExternal) {
+      return getTranslator().getContext()->getConstantExternSym(Name);
+    } else {
+      const Ice::RelocOffsetT Offset = 0;
+      return getTranslator().getContext()->getConstantSym(Offset, Name,
+                                                          SuppressMangling);
+    }
+  }
+
+  // Converts function declarations into constant value IDs.
+  void createValueIDsForFunctions() {
+    for (Ice::FunctionDeclaration *Func : FunctionDeclarationList) {

[Jim Stichnoth, 2015/02/10 17:47:11]

Declare Func as const* if possible.

[Karl, 2015/02/10 19:30:23]

Done.

+      Ice::Constant *C = nullptr;
+      if (!isIRGenerationDisabled()) {
+        C = getConstantSym(Func->getName(), Func->getSuppressMangling(),
+                           Func->isProto());
+      }
+      ValueIDConstants.push_back(C);
+    }
+  }
+
+  // Converts global variable declarations into constant value IDs.
+  void createValueIDsForGlobalVars() {
+    for (Ice::VariableDeclaration *Decl : *VariableDeclarations) {

[Jim Stichnoth, 2015/02/10 17:47:11]

Also declare Decl as const* if possible.

[Karl, 2015/02/10 19:30:23]

Done.

+      Ice::Constant *C = nullptr;
+      if (!isIRGenerationDisabled()) {
+        C = getConstantSym(Decl->getName(), Decl->getSuppressMangling(),
+                           !Decl->hasInitializer());
+      }
+      ValueIDConstants.push_back(C);
+    }
+  }
+
   // Reports that type ID is undefined, or not of the WantedType.
   void reportBadTypeIDAs(unsigned ID, const ExtendedType *Ty,
                          ExtendedType::TypeKind WantedType);
@@ -494,11 +544,11 @@ TopLevelParser::reportGetGlobalVariableByIDError(unsigned Index) {
   raw_string_ostream StrBuf(Buffer);
   StrBuf << "Global index " << Index
          << " not allowed. Out of range. Must be less than "
-         << VariableDeclarations.size();
+         << VariableDeclarations->size();
   BlockError(StrBuf.str());
   // TODO(kschimpf) Remove error recovery once implementation complete.
-  if (!VariableDeclarations.empty())
-    return VariableDeclarations[0];
+  if (!VariableDeclarations->empty())
+    return VariableDeclarations->at(0);
   report_fatal_error("Unable to continue");
 }
 
@@ -1160,7 +1210,7 @@ public:
   // Returns the value referenced by the given value Index.
   Ice::Operand *getOperand(uint32_t Index) {
     if (Index < CachedNumGlobalValueIDs) {
-      return Context->getOrCreateGlobalConstantByID(Index);
+      return Context->getGlobalConstantByID(Index);
     }
     uint32_t LocalIndex = Index - CachedNumGlobalValueIDs;
     if (LocalIndex >= LocalOperands.size()) {
@@ -2808,41 +2858,15 @@ private:
   // the first call will do the installation.
   void InstallGlobalNamesAndGlobalVarInitializers() {
     if (!GlobalDeclarationNamesAndInitializersInstalled) {
-      Ice::Translator &Trans = getTranslator();
-      const Ice::IceString &GlobalPrefix = getFlags().getDefaultGlobalPrefix();
-      if (!GlobalPrefix.empty()) {
-        uint32_t NameIndex = 0;
-        for (Ice::VariableDeclaration *Var : Context->getGlobalVariables()) {
-          installDeclarationName(Trans, Var, GlobalPrefix, "global", NameIndex);
-        }
-      }
-      const Ice::IceString &FunctionPrefix =
-          getFlags().getDefaultFunctionPrefix();
-      if (!FunctionPrefix.empty()) {
-        uint32_t NameIndex = 0;
-        for (Ice::FunctionDeclaration *Func :
-             Context->getFunctionDeclarationList()) {
-          installDeclarationName(Trans, Func, FunctionPrefix, "function",
-                                 NameIndex);
-        }
-      }
-      getTranslator().lowerGlobals(Context->getGlobalVariables());
+      Context->installGlobalNames();
+      Context->createValueIDs();
+      std::unique_ptr<Ice::VariableDeclarationList> DeclsPtr =
+          Context->getGlobalVariables();
+      const Ice::VariableDeclarationList &Decls = *DeclsPtr;
+      getTranslator().lowerGlobals(Decls);
       GlobalDeclarationNamesAndInitializersInstalled = true;
     }
   }
-
-  void installDeclarationName(Ice::Translator &Trans,
-                              Ice::GlobalDeclaration *Decl,
-                              const Ice::IceString &Prefix, const char *Context,
-                              uint32_t &NameIndex) {
-    if (!Decl->hasName()) {
-      Decl->setName(Trans.createUnnamedName(Prefix, NameIndex));
-      ++NameIndex;
-    } else {
-      Trans.checkIfUnnamedNameSafe(Decl->getName(), Context, Prefix);
-    }
-  }
-
   bool ParseBlock(unsigned BlockID) override;
 
   void ExitBlock() override { InstallGlobalNamesAndGlobalVarInitializers(); }