Fix PNaCl bitcode reader to release global variables to emitter.

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.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the PNaCl bitcode file to Ice, to machine code
@@ skipping 145 matching lines @@
   TopLevelParser(const TopLevelParser &) = delete;
   TopLevelParser &operator=(const TopLevelParser &) = delete;
 
 public:
   typedef std::vector<Ice::FunctionDeclaration *> FunctionDeclarationListType;
 
   TopLevelParser(Ice::Translator &Translator, NaClBitcodeHeader &Header,
                  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 {}
 
   Ice::Translator &getTranslator() { return Translator; }
 
   void setBlockParser(BlockParserBaseClass *NewBlockParser) {
     BlockParser = NewBlockParser;
   }
 
@@ skipping 68 matching lines @@
     return NextDefiningFunctionID++;
   }
 
   /// Returns the function associated with ID.
   Ice::FunctionDeclaration *getFunctionByID(unsigned ID) {
     if (ID < FunctionDeclarationList.size())
       return FunctionDeclarationList[ID];
     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
   /// called constant addresses. Returns the corresponding operand
   /// to replace the calling address with. Reports an error if
   /// a stub could not be found, returning the CallValue.
   Ice::Operand *getStubbedConstCallValue(Ice::Operand *CallValue) {
     if (StubbedConstCallValue)
       return StubbedConstCallValue;
     for (unsigned i = 0; i < getNumFunctionIDs(); ++i) {
       Ice::FunctionDeclaration *Func = getFunctionByID(i);
       if (!Func->isProto()) {
-        StubbedConstCallValue = getOrCreateGlobalConstantByID(i);
+        StubbedConstCallValue = getGlobalConstantByID(i);
         return StubbedConstCallValue;
       }
     }
     Error("Unable to find function definition to stub constant calls with");
     return CallValue;
   }
 
   /// Returns the number of function declarations in the bitcode file.
   unsigned getNumFunctionIDs() const { return FunctionDeclarationList.size(); }
 
   /// 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);
   }
 
   /// Returns the global declaration (variable or function) with the
   /// given Index.
   Ice::GlobalDeclaration *getGlobalDeclarationByID(size_t Index) {
     size_t NumFunctionIds = FunctionDeclarationList.size();
     if (Index < NumFunctionIds)
       return getFunctionByID(Index);
     else
       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:
   // The translator associated with the parser.
   Ice::Translator &Translator;
   // The bitcode header.
   NaClBitcodeHeader &Header;
   // The exit status that should be set to true if an error occurs.
   Ice::ErrorCode &ErrorStatus;
   // The number of errors reported.
   unsigned NumErrors;
   // The types associated with each type ID.
   std::vector<ExtendedType> TypeIDValues;
   // The set of functions (prototype and defined).
   FunctionDeclarationListType FunctionDeclarationList;
   // The ID of the next possible defined function ID in
   // FunctionDeclarationList.  FunctionDeclarationList is filled
   // first. It's the set of functions (either defined or isproto). Then
   // function definitions are encountered/parsed and
   // NextDefiningFunctionID is incremented to track the next
   // 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.
   Ice::FuncSigType UndefinedFuncSigType;
   // The block parser currently being applied. Used for error
   // reporting.
   BlockParserBaseClass *BlockParser;
   // Value to use to stub constant calls.
   Ice::Operand *StubbedConstCallValue;
 
   bool ParseBlock(unsigned BlockID) override;
 
   // Gets extended type associated with the given index, assuming the
   // extended type is of the WantedKind. Generates error message if
   // corresponding extended type of WantedKind can't be found, and
   // returns nullptr.
   ExtendedType *getTypeByIDAsKind(unsigned ID,
                                   ExtendedType::TypeKind WantedKind) {
     ExtendedType *Ty = nullptr;
     if (ID < TypeIDValues.size()) {
       Ty = &TypeIDValues[ID];
       if (Ty->getKind() == WantedKind)
         return Ty;
     }
     // Generate an error message and set ErrorStatus.
     this->reportBadTypeIDAs(ID, Ty, WantedKind);
     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);
 
   // Reports that there is no function declaration for ID. Returns an
   // error recovery value to use.
   Ice::FunctionDeclaration *reportGetFunctionByIDError(unsigned ID);
 
   // Reports that there is not global variable declaration for
   // ID. Returns an error recovery value to use.
@@ skipping 42 matching lines @@
     return FunctionDeclarationList[0];
   report_fatal_error("Unable to continue");
 }
 
 Ice::VariableDeclaration *
 TopLevelParser::reportGetGlobalVariableByIDError(unsigned Index) {
   std::string Buffer;
   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");
 }
 
 Ice::Type TopLevelParser::convertToIceTypeError(Type *LLVMTy) {
   std::string Buffer;
   raw_string_ostream StrBuf(Buffer);
   StrBuf << "Invalid LLVM type: " << *LLVMTy;
   Error(StrBuf.str());
   return Ice::IceType_void;
 }
@@ skipping 641 matching lines @@
   void setNextLocalInstIndex(Ice::Operand *Op) {
     setOperand(NextLocalInstIndex++, Op);
   }
 
   // Set the next constant ID to the given constant C.
   void setNextConstantID(Ice::Constant *C) { setNextLocalInstIndex(C); }
 
   // 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()) {
       std::string Buffer;
       raw_string_ostream StrBuf(Buffer);
       StrBuf << "Value index " << Index << " not defined!";
       Error(StrBuf.str());
       report_fatal_error("Unable to continue");
     }
     Ice::Operand *Op = LocalOperands[LocalIndex];
@@ skipping 1627 matching lines @@
   // True if we have already installed names for unnamed global declarations,
   // and have generated global constant initializers.
   bool GlobalDeclarationNamesAndInitializersInstalled;
 
   // Generates names for unnamed global addresses (i.e. functions and
   // global variables). Then lowers global variable declaration
   // initializers to the target. May be called multiple times. Only
   // 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(); }
 
   void ProcessRecord() override;
 };
 
 class ModuleValuesymtabParser : public ValuesymtabParser {
   ModuleValuesymtabParser(const ModuleValuesymtabParser &) = delete;
   void operator=(const ModuleValuesymtabParser &) = delete;
@@ skipping 164 matching lines @@
 
   if (TopLevelBlocks != 1) {
     errs() << IRFilename
            << ": Contains more than one module. Found: " << TopLevelBlocks
            << "\n";
     ErrorStatus.assign(EC_Bitcode);
   }
 }
 
 } // end of namespace Ice