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..a3cc45e62b13cdb70a304a51116be870a15a7d34 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.get());

[Jim Stichnoth, 2015/02/10 04:01:04]

You should be able to just assert(VariableDeclarations) because of operator
bool().

It's good to minimize the uses of unique_ptr<>::get() so it's easier to reason
about whether there are any stray copies of the pointer.

[Karl, 2015/02/10 17:19:26]

Good point. Didn't realize it had a bool type conversion operator.

+    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.get());

[Jim Stichnoth, 2015/02/10 04:01:05]

assert(VariableDeclarations);

[Karl, 2015/02/10 17:19:26]

Done.

+    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 (Ice::VariableDeclarationList *Decls = VariableDeclarations.get()) {
+      return FunctionDeclarationList.size() + Decls->size();

[Jim Stichnoth, 2015/02/10 04:01:05]

if (VariableDeclarations) {
  return FunctionDeclarationList.size() + VariableDeclarations->size();

[Karl, 2015/02/10 17:19:26]

Done.

+    } else {
+      return ValueIDConstants.size();
+    }
   }
 
   /// Creates Count global variable declarations.
   void CreateGlobalVariables(size_t Count) {
-    assert(VariableDeclarations.empty());
+    assert(VariableDeclarations.get());

[Jim Stichnoth, 2015/02/10 04:01:05]

assert(VariableDeclarations);

[Karl, 2015/02/10 17:19:26]

Done.

+    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 (Ice::VariableDeclarationList *Decls = VariableDeclarations.get()) {
+      return Decls->size();

[Jim Stichnoth, 2015/02/10 04:01:04]

if (VariableDeclarations) {
  return VariableDeclarations->size();

[Karl, 2015/02/10 17:19:26]

Done.

+    } 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.get());

[Jim Stichnoth, 2015/02/10 04:01:05]

assert(VariableDeclarations);

[Karl, 2015/02/10 17:19:26]

Done.

+    if (Index < VariableDeclarations->size())
+      return VariableDeclarations->at(Index);
     return reportGetGlobalVariableByIDError(Index);
   }
 
@@ -376,9 +343,15 @@ 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.
+  Ice::VariableDeclarationList *getGlobalVariables() {

[Jim Stichnoth, 2015/02/10 04:01:04]

This would be better returning a unique_ptr.  The last statement would be:

return std::move(VariableDeclarations);

[Karl, 2015/02/10 17:19:26]

I didn't know how to return a unique_ptr, which was why I returned a pointer.
Fixing.

+    // Before returning, check that ValidIDConstants has already been
+    // build.
+    if (Ice::VariableDeclarationList *Decls = VariableDeclarations.get()) {
+      assert(Decls->size() <= ValueIDConstants.size());

[Jim Stichnoth, 2015/02/10 04:01:04]

if (VariableDeclarations) {
  assert(VariableDeclarations->size() <= ...);

or:

assert(!VariableDeclarations || VariableDeclarations->size() <= ...);

[Karl, 2015/02/10 17:19:26]

Done.

+    }
+    return VariableDeclarations.release();
   }
 
 private:
@@ -402,7 +375,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 +405,91 @@ 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
+  // automatically incremented if a new new is created.  Context is
+  // literal text describing the type of name being created.
+  void installDeclarationName(// Ice::Translator &Trans,

[Jim Stichnoth, 2015/02/10 04:01:05]

Remove comment in arglist?  Same for the two calls below to this function?

[Karl, 2015/02/10 17:19:26]

Done.

+                              Ice::GlobalDeclaration *Decl,
+                              const Ice::IceString &Prefix, const char *Context,

[Jim Stichnoth, 2015/02/10 04:01:04]

Bikeshedding.  Maybe rename the Context arg, since there are already a couple of
overloaded uses of this term?

[Karl, 2015/02/10 17:19:26]

Done.

+                              uint32_t &NameIndex) {
+    if (!Decl->hasName()) {
+      Decl->setName(Translator.createUnnamedName(Prefix, NameIndex));
+      ++NameIndex;
+    } else {
+      Translator.checkIfUnnamedNameSafe(Decl->getName(), Context, Prefix);
+    }
+  }
+
+  // Installs names for global variables without names.
+  void installGlobalVarNames() {
+    assert(VariableDeclarations.get());

[Jim Stichnoth, 2015/02/10 04:01:04]

assert(VariableDeclarations);

[Karl, 2015/02/10 17:19:26]

Done.

+    const Ice::IceString &GlobalPrefix =
+        getTranslator().getFlags().getDefaultGlobalPrefix();
+    if (!GlobalPrefix.empty()) {
+      // Ice::Translator &Trans = getTranslator();

[Jim Stichnoth, 2015/02/10 04:01:05]

remove?

[Karl, 2015/02/10 17:19:26]

Done.

+      uint32_t NameIndex = 0;
+      for (Ice::VariableDeclaration *Var : *VariableDeclarations) {
+        installDeclarationName(/*Trans,*/ Var, GlobalPrefix, "global", NameIndex);
+      }
+    }
+  }
+
+  // Installs names for functions without names.
+  void installFunctionNames() {
+    const Ice::IceString &FunctionPrefix =
+        getTranslator().getFlags().getDefaultFunctionPrefix();
+    if (!FunctionPrefix.empty()) {
+      // Ice::Translator &Trans = getTranslator();

[Jim Stichnoth, 2015/02/10 04:01:04]

remove?

[Karl, 2015/02/10 17:19:26]

Done.

+      uint32_t NameIndex = 0;
+      for (Ice::FunctionDeclaration *Func : FunctionDeclarationList) {
+        installDeclarationName(/*Trans,*/ 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() {
+    size_t NumFunctions = FunctionDeclarationList.size();
+    for (size_t i = 0; i < NumFunctions; ++i) {

[Jim Stichnoth, 2015/02/10 04:01:05]

Can you use a range-based for loop?

[Karl, 2015/02/10 17:19:26]

I didn't use a range-based loop, since I needed in an earlier version. However,
I later realized I needed to set the value using push_back (rather that
assignment to array element). Fixing.

+      Ice::Constant *C = nullptr;
+      if (!isIRGenerationDisabled()) {
+        Ice::FunctionDeclaration *Func = FunctionDeclarationList[i];
+        C = getConstantSym(Func->getName(), Func->getSuppressMangling(),
+                           Func->isProto());

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

Thanks for simplifying the IsExternal a bit.

+      }
+      ValueIDConstants.push_back(C);
+    }
+  }
+
+  // Converts global variable declarations into constant value IDs.
+  void createValueIDsForGlobalVars() {
+    size_t NumGlobalVars = VariableDeclarations->size();
+    for (size_t i = 0; i < NumGlobalVars; ++i) {

[Jim Stichnoth, 2015/02/10 04:01:05]

Range-based for loop?

[Karl, 2015/02/10 17:19:26]

Done.

+      Ice::Constant *C = nullptr;
+      if (!isIRGenerationDisabled()) {
+        Ice::VariableDeclaration *Decl = VariableDeclarations->at(i);
+        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 +552,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 +1218,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 +2866,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());

[Jim Stichnoth, 2015/02/10 04:01:04]

With the suggested change to the getGlobalVariable() return type, this would be
something like:

std::unique_ptr<Ice::VariableDeclarationList> DeclsPtr =
Context->getGlobalVariables();

[Karl, 2015/02/10 17:19:26]

Done.

+      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(); }