Update Subzero to start parsing PNaCl bitcode files.

src/PNaClTranslator.cpp

+//===- subzero/src/PNaClTranslator.cpp - Builds ICE from PNaCl 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 translator.
+//
+//===----------------------------------------------------------------------===//
+
+#include "PNaClTranslator.h"
+#include "llvm/Bitcode/NaCl/NaClBitcodeDecoders.h"
+#include "llvm/Bitcode/NaCl/NaClBitcodeHeader.h"
+#include "llvm/Bitcode/NaCl/NaClBitcodeParser.h"
+#include "llvm/Bitcode/NaCl/NaClReaderWriter.h"
+#include "llvm/IR/Constants.h"
+#include "llvm/IR/LLVMContext.h"
+#include "llvm/IR/Module.h"
+#include "llvm/Support/Format.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/ValueHandle.h"
+
+#include <vector>
+#include <cassert>
+
+using namespace llvm;
+
+namespace {
+
+// Top-level class to read PNaCl bitcode files, and translate to ICE.
+class TopLevelParser : public NaClBitcodeParser {
+  TopLevelParser(const TopLevelParser&) LLVM_DELETED_FUNCTION;
+  void operator=(const TopLevelParser&) LLVM_DELETED_FUNCTION;

[jvoung (off chromium), 2014/07/01 17:32:52]

I think for Subzero, Jim has been using:

T &operator=(const T&) LLVM_DELETED_FUNCTION
instead of void operator=

though yes, about 2/3 of llvm uses void and the other 1/3 uses T &

[Karl, 2014/07/01 21:31:07]

Done.

+
+public:
+  TopLevelParser(Module *Mod,

[jvoung (off chromium), 2014/07/01 17:32:52]

It would be good to clarify what state the incoming Module is in. It seems like
it should be mostly empty, but it's hard to tell from just looking.

See below -- should this parser just allocate the module on its own, and the
fact that it even has to allocate a module is private implementation detail?

[Karl, 2014/07/01 21:31:07]

I hadn't really worried about this because these classes are in an unnamed
namespace, and hence, are not visible outside this file. I thought the
construction (down in PNaClTranslator::translate) was
sufficient.

However, I will move it.

+                 NaClBitcodeHeader &Header,
+                 NaClBitstreamCursor &Cursor)
+      : NaClBitcodeParser(Cursor),
+        Mod(Mod),
+        Header(Header),
+        NumErrors(),

[jvoung (off chromium), 2014/07/01 17:32:53]

initialize to 0?

[Karl, 2014/07/01 21:31:06]

Definitely!

[jvoung (off chromium), 2014/07/02 17:00:15]

Okay, using () does initialize to zero, but it seems inconsistent that NumErrors
uses () while the other fields use (0).

[Karl, 2014/07/02 18:09:54]

Adding zero to be more clear.

+        NumFunctionIds(0),
+        GlobalVarPlaceHolderType(0) {}
+
+  virtual ~TopLevelParser() {} LLVM_OVERRIDE;
+
+  virtual bool Error(const std::string &Message) LLVM_OVERRIDE {
+    ++NumErrors;
+    return NaClBitcodeParser::Error(Message);
+  }
+
+  /// Returns the number of errors found while parsing the bitcode
+  /// file.
+  unsigned getNumErrors() const {
+    return NumErrors;
+  }
+
+  /// Returns the LLVM module associated with the translation.
+  Module *getModule() {
+    return Mod;
+  }
+
+  /// Returns the number of bytes in the bitcode header.
+  size_t getHeaderSize() {

[jvoung (off chromium), 2014/07/01 17:32:53]

Some of these other methods are const too?

[Karl, 2014/07/01 21:31:06]

Done.

+    return Header.getHeaderSize();
+  }
+
+  /// Returns the llvm context to use.
+  LLVMContext &getLLVMContext() {
+    return Mod->getContext();
+  }
+
+  /// Changes the size of the type list to the given size.
+  void resizeTypeIDValues(unsigned NewSize) {
+    TypeIDValues.resize(NewSize);
+  }
+
+  /// Returns the type associated with the given index.
+  Type *getTypeByID(unsigned ID) {
+    Type *Ty = ID < TypeIDValues.size() ? TypeIDValues[ID] : 0;

[jvoung (off chromium), 2014/07/01 17:32:53]

Clarify that the array could really end up storing 0 as some of its in-bound
entries? Otherwise this check could have been simpler.

[Karl, 2014/07/01 21:31:08]

Done.

+    if (Ty) return Ty;
+    return reportTypeIDAsUndefined(ID);
+  }
+
+  /// Defines type for ID.
+  void setTypeID(unsigned ID, Type *Ty) {
+    if (ID < TypeIDValues.size() && TypeIDValues[ID] == 0) {
+      TypeIDValues[ID] = Ty;
+      return;
+    }
+    reportBadSetTypeID(ID, Ty);
+  }
+
+  /// Sets the next function ID to the given LLVM function.
+  void setNextFunctionID(Function *Fcn) {
+    ++NumFunctionIds;
+    ValueIDValues.push_back(Fcn);
+  }
+
+  /// Defines the next function ID as one that has an implementation
+  /// (i.e a corresponding function block in the bitcode).
+  void setNextValueIDAsImplementedFunction() {
+    DefiningFunctionsList.push_back(ValueIDValues.size());
+  }
+
+  /// Returns the LLVM IR value associatd with the global value ID.
+  Value *getGlobalValueByID(unsigned ID) {
+    if (ID >= ValueIDValues.size()) return 0;
+    return ValueIDValues[ID];
+  }
+
+  /// Returns the number of function addresses (i.e. ID's) defined in
+  /// the bitcode file.
+  unsigned getNumFunctionIDs() {

[jvoung (off chromium), 2014/07/01 17:32:52]

const

[Karl, 2014/07/01 21:31:08]

Done.

+    return NumFunctionIds;
+  }
+
+  /// Returns the number of global values defined in the bitcode
+  /// file.
+  unsigned getNumGlobalValueIDs() {
+    return ValueIDValues.size();
+  }
+
+  /// Resizes the list of of value IDs to include Count global
+  /// variable IDs.
+  void resizeValueIDsForGlobalVarCount(unsigned Count) {
+    ValueIDValues.resize(ValueIDValues.size() + Count);
+  }
+
+  /// Returns the global variable address associated with the given
+  /// value ID. If the ID refers to a global variable address not yet
+  /// defined, a placeholder is created so that we can fix it up
+  /// later.
+  Constant *getOrCreateGlobalVarRef(unsigned ID) {
+    if (ID >= ValueIDValues.size()) return 0;
+    if (Value *C = ValueIDValues[ID])
+      return dyn_cast<Constant>(C);
+
+    if (GlobalVarPlaceHolderType == 0)

[jvoung (off chromium), 2014/07/01 17:32:53]

Why not just eagerly initialize it in the class's ctor?

[Karl, 2014/07/01 21:31:06]

Done.

+      GlobalVarPlaceHolderType = Type::getInt8Ty(getLLVMContext());
+    Constant *C =
+        new GlobalVariable(*Mod, GlobalVarPlaceHolderType, false,
+                           GlobalValue::ExternalLinkage, 0);
+    ValueIDValues[ID] = C;
+    return C;
+  }
+
+  /// Assigns the given global variable (address) to the given value
+  /// ID.  Returns true if ID is a valid global variable ID. Otherwise
+  /// returns false.
+  bool assignGlobalVariable(GlobalVariable *GV, unsigned ID) {
+    if (ID < NumFunctionIds || ID >= ValueIDValues.size()) return false;
+    WeakVH &OldV = ValueIDValues[ID];
+    if (OldV == 0) {
+      ValueIDValues[ID] = GV;
+      return true;
+    }
+
+    // If reached, there was a forward reference to this value. Replace it.
+    Value *PrevVal = OldV;
+    GlobalVariable *Placeholder = cast<GlobalVariable>(PrevVal);
+    Placeholder->replaceAllUsesWith(
+        ConstantExpr::getBitCast(GV, Placeholder->getType()));
+    Placeholder->eraseFromParent();
+    ValueIDValues[ID] = GV;
+    return true;
+  }
+
+private:
+  // The parsed module.
+  Module *Mod;
+  // The bitcode header.
+  NaClBitcodeHeader &Header;
+  // The number of errors reported.
+  unsigned NumErrors;
+  // The types associated with each type ID.
+  std::vector<Type*> TypeIDValues;
+  // The (global) value IDs.
+  std::vector<WeakVH> ValueIDValues;
+  // The number of function IDs.
+  unsigned NumFunctionIds;
+  // The list of value IDs (in the order found) of defining function
+  // addresses.
+  std::vector<unsigned> DefiningFunctionsList;
+  // Cached global variable placeholder type. Used for all forward
+  // references to global variable addresses.
+  Type *GlobalVarPlaceHolderType;
+
+  virtual bool ParseBlock(unsigned BlockID) LLVM_OVERRIDE;
+
+  /// Reports that type ID is undefined, and then returns
+  /// the void type.
+  Type *reportTypeIDAsUndefined(unsigned ID);
+
+  /// Reports error about bad call to setTypeID.
+  void reportBadSetTypeID(unsigned ID, Type *Ty);
+};
+
+Type *TopLevelParser::reportTypeIDAsUndefined(unsigned ID) {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);

[jvoung (off chromium), 2014/07/01 17:32:52]

This probably doesn't count as "Subzero core" since it still uses a bunch of
LLVM bits, but Jim was trying to avoid using raw_string_ostream in "Subzero
core":
https://codereview.chromium.org/353553004/diff/20001/src/IceTargetLoweringX86...

Perhaps it was code-size concerns? Will have to ask to clarify.

I do know that the plain C++ stringstream pulls in a hefty chunk of code (I
think adding 500KB to 1MB to statically linked nexes).

[Karl, 2014/07/01 21:31:06]

First off, Jim's comment about not using it is incorrect. He uses it a lot in
IceConverter.cpp. Hence, it will be pulled into llvm2ice.

BTW, raw_string_ostream is really relatively small (if you look at the code),
and is way smaller than the C++ stringstreams.

Also, errs() and outs() define much smaller footprints than std::cout and
std::cerr, but Jim is using those as well.

+  StrBuf << "Can't find type for type id: " << ID;
+  Error(StrBuf.str());
+  Type *Ty =  Type::getVoidTy(getLLVMContext());
+  // To reduce error messages, update type list if possible.
+  if (ID < TypeIDValues.size()) TypeIDValues[ID] = Ty;
+  return Ty;
+}
+
+void TopLevelParser::reportBadSetTypeID(unsigned ID, Type *Ty) {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  if (ID >= TypeIDValues.size()) {
+    StrBuf << "Type index " << ID << " out of range: can't install.";
+  } else {
+    // Must be case that index already defined.
+    StrBuf << "Type index " << ID << " defined as " << *TypeIDValues[ID]
+           << " and " << *Ty << ".";
+  }
+  Error(StrBuf.str());
+}
+
+// class for parsing blocks within the TopLevelParser.
+class BlockParser : public NaClBitcodeParser {
+protected:
+
+  // Constructor for nested block parsers.
+  BlockParser(unsigned BlockID, BlockParser *EnclosingParser)
+      : NaClBitcodeParser(BlockID, EnclosingParser),
+        Context(EnclosingParser->Context) {}
+
+  // Returns a string describing the bit address of the current record
+  // being processed by the block parser.
+  std::string getRecordAddress() const {

[jvoung (off chromium), 2014/07/01 17:32:52]

could inline this into Error(), then you don't need a separate buffer and
raw_string_ostream

[Karl, 2014/07/01 21:31:06]

Ok. the other uses I had for it has since been removed.

+    uint64_t Bit =  Record.GetStartBit() + Context->getHeaderSize() * 8;
+    std::string Buffer;
+    raw_string_ostream StrBuf(Buffer);
+    StrBuf << format("%"PRIu64":%u",
+                     (Bit / 8),
+                     static_cast<unsigned>(Bit % 8));
+    return StrBuf.str();
+  }
+
+  virtual bool Error(const std::string &Message) LLVM_OVERRIDE {
+    std::string Buffer;
+    raw_string_ostream StrBuf(Buffer);
+    StrBuf << "(" << getRecordAddress() << ") " << Message;
+    return Context->Error(StrBuf.str());
+  }
+
+public:
+  // Constructor for the top-level module block parser.
+  BlockParser(unsigned BlockID, TopLevelParser *Context)

[jvoung (off chromium), 2014/07/01 17:32:52]

Why not make this first, before the nested block ctor, then it will be a
contiguous section of protected: code, rather than have a small island of
public: in the middle?

[Karl, 2014/07/01 21:31:07]

Done.

+      : NaClBitcodeParser(BlockID, Context),
+        Context(Context) {}
+
+  virtual ~BlockParser() LLVM_OVERRIDE {}
+
+protected:
+  // Default implementation. Reports that block is unknown and skips
+  // its contents.
+  virtual bool ParseBlock(unsigned BlockID) LLVM_OVERRIDE;
+
+  // Default implementation. Reports that the record is not
+  // understood.
+  virtual void ProcessRecord() LLVM_OVERRIDE;
+
+  // The context parser that contains the decoded state.

[jvoung (off chromium), 2014/07/01 17:32:53]

Can we collect the fields in one place, and the methods in another instead of
interleaving them?

[Karl, 2014/07/01 21:31:06]

Done.

+  TopLevelParser *Context;
+
+  /// Checks if the size of the record is Size. If not, an error is
+  /// produced using the given RecordName. Return true if error was
+  /// reported. Otherwise false.

[jvoung (off chromium), 2014/07/01 17:32:53]

I feel like some of these comments about the error case can be shared between
the next few methods.

[Karl, 2014/07/01 21:31:08]

Done.

+  bool checkRecordSize(unsigned Size, const char *RecordName) {
+    const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+    if (Values.size() != Size) {
+      return RecordSizeError(Size, RecordName, 0);
+    }
+    return false;
+  }
+
+  /// Checks if the size of the record is at least as large as the
+  /// LowerLimit. If not, an error is produced using the given
+  /// RecordName. Return true if error was reported. Otherwise false.
+  bool checkRecordSizeAtLeast(unsigned LowerLimit, const char *RecordName) {
+    const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+    if (Values.size() < LowerLimit) {
+      return RecordSizeError(LowerLimit, RecordName, "at least");
+    }
+    return false;
+  }
+
+  /// Checks if the size of the record is no larger than the
+  /// UpperLimit.  If not, an error is produced using the given
+  /// RecordName. Return true if error was reported. Otherwise false.
+  bool checkRecordSizeNoMoreThan(unsigned UpperLimit, const char *RecordName) {
+    const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+    if (Values.size() > UpperLimit) {
+      return RecordSizeError(UpperLimit, RecordName, "no more than");
+    }
+    return false;
+  }
+
+  /// Checks if the size of the record is at least as large as the
+  /// LowerLimit, and no larger than the UpperLimit.  If not, an error
+  /// is produced using the given RecordName. Return true if error was
+  /// reported. Otherwise false.
+  bool checkRecordSizeInRange(unsigned LowerLimit, unsigned UpperLimit,
+                              const char *RecordName) {
+    return checkRecordSizeAtLeast(LowerLimit, RecordName)
+        || checkRecordSizeNoMoreThan(UpperLimit, RecordName);
+  }
+
+private:
+  /// Generates a record size error. ExpectedSize is the number
+  /// of elements expected. RecordName is the name of the kind of
+  /// record that has incorrect size. ContextMessage (if not 0)
+  /// is appended to "record expects" to describe how ExpectedSize
+  /// should be interpreted.
+  bool RecordSizeError(unsigned ExpectedSize,
+                       const char *RecordName,
+                       const char *ContextMessage) {
+    std::string Buffer;
+    raw_string_ostream StrBuf(Buffer);
+    StrBuf << RecordName << " record expects";
+    if (ContextMessage) StrBuf << " " << ContextMessage;
+    StrBuf << " " << ExpectedSize << " argument";
+    if(ExpectedSize > 1) StrBuf << "s";
+    StrBuf << ". Found: " << Record.GetValues().size();
+    return Error(StrBuf.str());
+  }
+};
+
+bool BlockParser::ParseBlock(unsigned BlockID) {

[jvoung (off chromium), 2014/07/01 17:32:53]

I wonder if this class should be called something that more clearly indicates
that it's a base class and doesn't actually handle the details.

Otherwise it seems unintuitive that a "BlockParser::ParseBlock" doesn't actually
parse blocks =)

[Karl, 2014/07/01 21:31:07]

Done.

+  // If called, derived class doesn't know how to handle block.
+  // Report error and skip.
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Don't know how to parse block id: " << BlockID;
+  Error(StrBuf.str());
+  SkipBlock();
+  return false;
+}
+
+void BlockParser::ProcessRecord() {
+  // If called, derived class doesn't know how to handle.
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << "Don't know how to process record: " << Record;
+  Error(StrBuf.str());
+}
+
+// Class to parse a types block.
+class TypesParser : public BlockParser {
+public:
+  TypesParser(unsigned BlockID, BlockParser *EnclosingParser)
+      : BlockParser(BlockID, EnclosingParser), NextTypeId(0) {}
+
+  ~TypesParser() LLVM_OVERRIDE {}
+
+protected:
+  virtual void ProcessRecord() LLVM_OVERRIDE;
+  // The type ID that will be associated with the next type defining
+  // record in the types block.
+  unsigned NextTypeId;
+};
+
+void TypesParser::ProcessRecord() {
+  Type *Ty = 0;
+  const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+  switch (Record.GetCode()) {
+  case naclbitc::TYPE_CODE_NUMENTRY:
+    // NUMENTRY: [numentries]
+    if (checkRecordSize(1, "Type count")) return;
+    Context->resizeTypeIDValues(Values[0]);
+    return;
+  case naclbitc::TYPE_CODE_VOID:
+    // VOID
+    if (checkRecordSize(0, "Type void")) break;
+    Ty = Type::getVoidTy(Context->getLLVMContext());
+    break;
+  case naclbitc::TYPE_CODE_FLOAT:
+    // FLOAT
+    if (checkRecordSize(0, "Type float")) break;
+    Ty = Type::getFloatTy(Context->getLLVMContext());
+    break;
+  case naclbitc::TYPE_CODE_DOUBLE:
+    // DOUBLE
+    if (checkRecordSize(0, "Type double")) break;
+    Ty = Type::getDoubleTy(Context->getLLVMContext());
+    break;
+  case naclbitc::TYPE_CODE_INTEGER:
+    // INTEGER: [width]
+    if (checkRecordSize(1, "Type integer")) break;
+    Ty = IntegerType::get(Context->getLLVMContext(), Values[0]);
+    // TODO(kschimpf) Check if size is legal.
+    break;
+  case naclbitc::TYPE_CODE_VECTOR:
+    // VECTOR: [numelts, eltty]
+    if (checkRecordSize(2, "Type vector")) break;
+    Ty = VectorType::get(Context->getTypeByID(Values[1]), Values[0]);
+    break;
+  case naclbitc::TYPE_CODE_FUNCTION: {
+    // FUNCTION: [vararg, retty, paramty x N]
+    if (checkRecordSizeAtLeast(2, "Type signature")) break;
+    SmallVector<Type *, 8> ArgTys;
+    for (unsigned i = 2, e = Values.size(); i != e; ++i) {
+      ArgTys.push_back(Context->getTypeByID(Values[i]));
+    }
+    Ty = FunctionType::get(Context->getTypeByID(Values[1]),
+                           ArgTys, Values[0]);
+    break;
+  }
+  default:
+    BlockParser::ProcessRecord();
+    break;
+  }
+  // If Ty not defined, assume error. Use void as filler.
+  if (Ty == 0)
+    Ty = Type::getVoidTy(Context->getLLVMContext());
+  Context->setTypeID(NextTypeId++, Ty);
+}
+
+/// Parses the globals block (i.e. global variables).
+class GlobalsParser : public BlockParser {
+public:
+  GlobalsParser(unsigned BlockID, BlockParser *EnclosingParser)
+      : BlockParser(BlockID, EnclosingParser),
+        InitializersNeeded(0),
+        Alignment(1),
+        IsConstant(false) {
+    NextGlobalID = Context->getNumFunctionIDs();
+  }
+
+  virtual ~GlobalsParser() LLVM_OVERRIDE {}
+
+protected:
+  virtual void ExitBlock() LLVM_OVERRIDE {
+    verifyNoMissingInitializers();
+    unsigned NumIDs = Context->getNumGlobalValueIDs();
+    if (NextGlobalID < NumIDs) {
+      unsigned NumFcnIDs = Context->getNumFunctionIDs();
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Globals block expects "
+             << (NumIDs - NumFcnIDs)
+             << " global definitions. Found: "
+             << (NextGlobalID - NumFcnIDs);
+      Error(StrBuf.str());
+    }
+    BlockParser::ExitBlock();
+  }
+
+  virtual void ProcessRecord() LLVM_OVERRIDE;
+
+  // Holds the sequence of initializers for the global.
+  SmallVector<Constant *, 10> Initializers;

[jvoung (off chromium), 2014/07/01 17:32:53]

Similar, can we put all the fields in one block, and methods in another?

Helps eyeball which fields exist + their types, and therefore which fields need
to be initialized by the ctor.

[Karl, 2014/07/01 21:31:07]

Done.

+
+  // Keeps track of how many initializers are expected for
+  // the global variable being built.
+  unsigned InitializersNeeded;
+
+  // The alignment assumed for the global variable being built.
+  unsigned Alignment;
+
+  // True if the global variable being built is a constant.
+  bool IsConstant;
+
+  // The index of the next global variable.
+  unsigned NextGlobalID;
+
+  // Checks if the number of initializers needed 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() {
+    if (InitializersNeeded != Initializers.size()) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Global variable @g"
+             << (NextGlobalID - Context->getNumFunctionIDs())
+             << " expected " << InitializersNeeded << " initializer";
+      if (InitializersNeeded > 1) StrBuf << "s";
+      StrBuf << ". Found: " << Initializers.size();
+      Error(StrBuf.str());
+      // Fix up state so that we can continue.
+      InitializersNeeded = Initializers.size();
+      installGlobalVar();
+    }
+  }
+
+  // Reserves a slot in the list of initializers being built. If there
+  // isn't room for the slot, an error message is generated.
+  void reserveInitializer(const char *RecordName) {
+    if (InitializersNeeded == Initializers.size()) {

[jvoung (off chromium), 2014/07/01 17:32:52]

Would it be safer to check if >= ? Otherwise, it looks like it's just about
right and odd that it would be an error.

[Karl, 2014/07/01 21:31:07]

I did this so that we wouldn't get cascading errors if multiple extra
initializers were provided. However, cascading errors might be appropriate as
well.

+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << RecordName << " record: Too many initializers, ignoring.";

[jvoung (off chromium), 2014/07/01 17:32:52]

Probably could have just std::string() + ... for this.

[Karl, 2014/07/01 21:31:07]

Done.

+      Error(StrBuf.str());
+    }
+  }
+
+  // Takes the initializers (and other parser state values) and
+  // installs a global variable (with the initializers) into the list
+  // of ValueIDs.
+  void installGlobalVar() {
+    Constant *Init = 0;
+    switch (Initializers.size()) {
+    case 0:
+      Error("No initializer for global variable in global vars block");
+      return;
+    case 1:
+      Init = Initializers[0];
+      break;
+    default:
+      Init = ConstantStruct::getAnon(Context->getLLVMContext(),
+                                     Initializers, true);
+      break;
+    }
+    GlobalVariable *GV = new GlobalVariable(
+        *Context->getModule(), Init->getType(), IsConstant,
+        GlobalValue::InternalLinkage, Init, "");
+    GV->setAlignment(Alignment);
+    if (!Context->assignGlobalVariable(GV, NextGlobalID)) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Defining global V[" << NextGlobalID
+             << "] not allowed. Out of range.";
+      Error(StrBuf.str());
+    }
+    ++NextGlobalID;
+    Initializers.clear();
+    InitializersNeeded = 0;
+    Alignment = 1;
+    IsConstant = false;
+  }
+};
+
+void GlobalsParser::ProcessRecord() {
+  const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+  switch (Record.GetCode()) {
+  case naclbitc::GLOBALVAR_COUNT:
+    // COUNT: [n]
+    if (checkRecordSize(1, "Globals count")) return;
+    if (NextGlobalID > Context->getNumFunctionIDs()) {

[jvoung (off chromium), 2014/07/01 17:32:52]

I can't put my finger on it, but this checks seems unintuitive.

NextGlobalID is initialized to getNumFunctionIDs, so perhaps the check should be
for exact equality?

[Karl, 2014/07/01 21:31:07]

The test is to verify it appears before any globals are defined. Since it is an
error test, I modified it to inequality.

+      Error("Globals count record not first in block.");
+      return;
+    }
+    verifyNoMissingInitializers();
+    Context->resizeValueIDsForGlobalVarCount(Values[0]);
+    return;
+  case naclbitc::GLOBALVAR_VAR: {
+    // VAR: [align, isconst]
+    if (checkRecordSize(2, "Globals variable")) return;
+    verifyNoMissingInitializers();
+    InitializersNeeded = 1;
+    Initializers.clear();
+    Alignment = (1 << Values[0]) >> 1;
+    IsConstant = Values[0] != 0;

[jvoung (off chromium), 2014/07/01 17:32:52]

Values[1] != 0

Otherwise, the alignment could make you think it's constant =)

[Karl, 2014/07/01 21:31:08]

Done.

+    return;
+  }
+  case naclbitc::GLOBALVAR_COMPOUND:
+    // COMPOUND: [size]
+    if (checkRecordSize(1, "globals compound")) return;
+    if (Initializers.size() > 0 || InitializersNeeded != 1) {
+      Error("Globals compound record not first initializer");
+      return;
+    }
+    if (Values[0] < 2) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Globals compound record size invalid. Found: "
+             << Values[0];
+      Error(StrBuf.str());
+      return;
+    }
+    InitializersNeeded = Values[0];
+    return;
+  case naclbitc::GLOBALVAR_ZEROFILL: {
+    // ZEROFILL: [size]
+    if (checkRecordSize(1, "Globals zerofill")) return;
+    reserveInitializer("Globals zerofill");
+    Type *Ty = ArrayType::get(Type::getInt8Ty(Context->getLLVMContext()),
+                              Values[0]);
+    Constant *Zero = ConstantAggregateZero::get(Ty);
+    Initializers.push_back(Zero);
+    break;
+  }
+  case naclbitc::GLOBALVAR_DATA: {
+    // DATA: [b0, b1, ...]
+    if (checkRecordSizeAtLeast(1, "Globals data")) return;
+    reserveInitializer("Globals data");
+    unsigned Size = Values.size();
+    uint8_t *Buf = new uint8_t[Size];
+    assert(Buf);
+    for (unsigned i = 0; i < Size; ++i)

[jvoung (off chromium), 2014/07/01 17:32:53]

could memcpy this?

[Karl, 2014/07/01 21:31:07]

Not really. We are doing a cast from uint64_t to uint8_t on each element.
However, its clear this isn't obvious. Adding cast to clarify.

+      Buf[i] = Values[i];
+    Constant *Init = ConstantDataArray::get(
+        Context->getLLVMContext(),
+        ArrayRef<uint8_t>(Buf, Buf + Size));
+    Initializers.push_back(Init);
+    delete[] Buf;
+    break;
+  }
+  case naclbitc::GLOBALVAR_RELOC: {
+    // RELOC: [val, [addend]]
+    if (checkRecordSizeInRange(1, 2, "Globals reloc")) return;
+    Constant *BaseVal =
+        Context->getOrCreateGlobalVarRef(Values[0]);
+    if (BaseVal == 0) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Can't find global relocation value: " << Values[0];
+      Error(StrBuf.str());
+      return;
+    }
+    Type *IntPtrType = IntegerType::get(Context->getLLVMContext(), 32);
+    Constant *Val = ConstantExpr::getPtrToInt(BaseVal, IntPtrType);
+    if (Values.size() == 2) {
+      Val = ConstantExpr::getAdd(Val, ConstantInt::get(IntPtrType, Values[1]));
+    }
+    Initializers.push_back(Val);
+    break;
+  }
+  default:
+    BlockParser::ProcessRecord();
+    return;
+  }
+  // If reached, just processed another intializer. See if time
+  // to install global.
+  if (InitializersNeeded == Initializers.size()) installGlobalVar();
+}
+
+// Parses a valuesymtab block in the bitcode file.
+class ValuesymtabParser : public BlockParser {
+  typedef SmallString<128> StringType;
+public:
+  ValuesymtabParser(unsigned BlockID,
+                   BlockParser *EnclosingParser,

[jvoung (off chromium), 2014/07/01 17:32:53]

indent to line up to after the ( ?

There is "make -f Makefile.standalone format" to run clang-format over the code.
Beware of it modifying unrelated files though.

[Karl, 2014/07/01 21:31:06]

Done.

[Jim Stichnoth, 2014/07/07 20:50:23]

This (modifying unrelated files) is usually because different versions of
clang-format may make slightly different formatting decisions.  It would be nice
if there were a practical way to force a specific clang-format version.

+                   bool AllowBbEntries)
+      : BlockParser(BlockID, EnclosingParser),
+        AllowBbEntries(AllowBbEntries) {}
+
+  virtual ~ValuesymtabParser() LLVM_OVERRIDE {}
+
+protected:
+  // True if entries to name basic blocks allowed.
+  bool AllowBbEntries;
+  // The last name converted to a string using convertToString.
+  StringType ConvertedName;
+
+  virtual void ProcessRecord() LLVM_OVERRIDE;
+
+  void ConvertToString() {
+    ConvertedName.clear();
+    const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+    for (size_t i = 1, e = Values.size(); i != e; ++i) {
+      ConvertedName += static_cast<char>(Values[i]);
+    }
+  }
+};
+
+void ValuesymtabParser::ProcessRecord() {
+  const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+  switch (Record.GetCode()) {
+  case naclbitc::VST_CODE_ENTRY: {
+    // VST_ENTRY: [valid, namechar x N]

[jvoung (off chromium), 2014/07/01 17:32:53]

valueID, or something instead of valid

[Karl, 2014/07/01 21:31:07]

Done.

+    if (checkRecordSizeAtLeast(2, "Valuesymtab value entry")) return;
+    ConvertToString();

[jvoung (off chromium), 2014/07/01 17:32:52]

Feels a bit roundabout that ConvertedName is a field within the
ValuesymtabParser, and needs to be .clear()'ed each round. It's "parser state"
but pretty limited in scope. It's really only live for one record, so really
only live during ProcessRecord?

[Karl, 2014/07/01 21:31:07]

I mainly did this to avoid allocations between calls. However, since we
are using SmallString, this is unlikely. Fixing.

+    Value *V = Context->getGlobalValueByID(Values[0]);
+    if (V == 0) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Invalid global address ID in valuesymtab: " << Values[0];
+      Error(StrBuf.str());
+      return;
+    }
+    V->setName(StringRef(ConvertedName.data(), ConvertedName.size()));
+    return;
+  }
+  case naclbitc::VST_CODE_BBENTRY: {
+    // VST_BBENTRY: [bbid, namechar x N]
+    // For now, since we aren't processing function blocks, don't handle.
+    if (AllowBbEntries) {
+      Error("Valuesymtab bb entry not implemented");
+      return;
+    }
+    break;
+  }
+  default:
+    break;
+  }
+  // If reached, don't know how to handle record.
+  BlockParser::ProcessRecord();
+  return;
+}
+
+/// Parses the module block in the bitcode file.
+class ModuleParser : public BlockParser {
+public:
+  ModuleParser(unsigned BlockID, TopLevelParser *Context)
+      : BlockParser(BlockID, Context) {}
+
+  virtual ~ModuleParser() LLVM_OVERRIDE {}
+
+protected:
+  virtual bool ParseBlock(unsigned BlockID) LLVM_OVERRIDE;
+
+  virtual void ProcessRecord() LLVM_OVERRIDE;
+};
+
+bool ModuleParser::ParseBlock(unsigned BlockID) LLVM_OVERRIDE {
+  switch (BlockID) {
+  case naclbitc::BLOCKINFO_BLOCK_ID:
+    return NaClBitcodeParser::ParseBlock(BlockID);
+  case naclbitc::TYPE_BLOCK_ID_NEW: {
+    TypesParser Parser(BlockID, this);
+    return Parser.ParseThisBlock();
+  }
+  case naclbitc::GLOBALVAR_BLOCK_ID: {
+    GlobalsParser Parser(BlockID, this);
+    return Parser.ParseThisBlock();
+  }
+  case naclbitc::VALUE_SYMTAB_BLOCK_ID: {
+    ValuesymtabParser Parser(BlockID, this, false);
+    return Parser.ParseThisBlock();
+  }
+  case naclbitc::FUNCTION_BLOCK_ID: {
+    Error("Function block parser not yet implemented, skipping");
+    SkipBlock();
+    return false;
+  }
+  default:
+    return BlockParser::ParseBlock(BlockID);
+  }
+}
+
+void ModuleParser::ProcessRecord() {
+  const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
+  switch (Record.GetCode()) {
+  case naclbitc::MODULE_CODE_VERSION: {
+    // VERSION: [version#]
+    if (checkRecordSize(1, "Module version")) return;
+    unsigned Version = Values[0];
+    if (Version != 1) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Unknown bitstream version: " << Version;
+      Error(StrBuf.str());
+    }
+    return;
+  }
+  case naclbitc::MODULE_CODE_FUNCTION: {
+    // FUNCTION:  [type, callingconv, isproto, linkage]
+    if (checkRecordSize(4, "Function heading")) return;
+    Type *Ty = Context->getTypeByID(Values[0]);
+    FunctionType *FTy = dyn_cast<FunctionType>(Ty);
+    if (FTy == 0) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Function heading expects function type. Found: "
+             << Ty;
+      Error(StrBuf.str());
+      return;
+    }
+    CallingConv::ID CallingConv;
+    if (!naclbitc::DecodeCallingConv(Values[1], CallingConv)) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Function heading has unknown calling convention: "
+             << Values[1];
+      Error(StrBuf.str());
+      return;
+    }
+    GlobalValue::LinkageTypes Linkage;
+    if (!naclbitc::DecodeLinkage(Values[3], Linkage)) {
+      std::string Buffer;
+      raw_string_ostream StrBuf(Buffer);
+      StrBuf << "Function heading has unknown linkage. Found "
+             << Values[3];
+      Error(StrBuf.str());
+      return;
+    }
+    Function *Func = Function::Create(FTy, Linkage, "", Context->getModule());
+    Func->setCallingConv(CallingConv);
+    if (Values[2] == 0) Context->setNextValueIDAsImplementedFunction();
+    Context->setNextFunctionID(Func);
+    // TODO(kschimpf) verify if Func matches PNaCl ABI.
+    return;
+  }
+  default:
+    BlockParser::ProcessRecord();
+    return;
+  }
+}
+
+bool TopLevelParser::ParseBlock(unsigned BlockID) {
+  if (BlockID == naclbitc::MODULE_BLOCK_ID) {
+    ModuleParser Parser(BlockID, this);
+    bool Results = Parser.ParseThisBlock();

[jvoung (off chromium), 2014/07/01 17:32:53]

Singular Result? Otherwise it sounds like a collection of results (e.g., at
"return Results").

a "bool" can only count to 1 =)

[Karl, 2014/07/01 21:31:06]

Done.

+    // TODO(kschimpf): Remove once translating function blocks.
+    errs() << "Global addresses:\n";
+    for (size_t i = 0; i < ValueIDValues.size(); ++i) {
+      errs() << "[" << i << "]: " << *ValueIDValues[i] << "\n";

[jvoung (off chromium), 2014/07/01 17:32:53]

How will this eventually transition? This starts to call translateFcn()
directly, with some amount of construction of llvm::Types and some amount of
conversion from llvm::Type to Ice::Type for globals and function signatures, and
then function bodies are constructed as Ice::Cfg and Ice::Inst "directly" (but
if the Ice::Inst refers to a global it'll also convert the LLVM type to Ice
Type?

[Karl, 2014/07/01 21:31:08]

I need to implement a FunctionParser. It will have a local field containing the
Cfg being built. The method FunctionParser::ProcessRecord() will add
instructions to the Cfg as it
processes each record (rather than building LLVM IR first). On block
exit (i.e. in FunctionParser::ExitBlock()) it will cal translateFcn().

For global addresses, we will look up the corresponding LLVM value (in
ValueIDValues) and do the conversion in place.

I still haven't thought of the exact transition of LLVM types to Ice types. If
it is simple enough, I will probably have the TypesParser build the types as
they are parsed in (and store them in a vector).
Otherwise, I will do it in FunctionParser.

Note: I may also lift convertType and convertIntegerType (from IceConverter.cpp)
to the base class (or as separate functions) so
that we only have one set of conversion routines.

+    }
+    return Results;
+  }
+  // Generate error message by using default block implementation.
+  BlockParser Parser(BlockID, this);
+  return Parser.ParseThisBlock();
+}
+
+}
+
+namespace Ice {
+
+int PNaClTranslator::translate(std::string IRFilename) {
+  OwningPtr<MemoryBuffer> MemBuf;
+  if (error_code ec =
+      MemoryBuffer::getFileOrSTDIN(IRFilename.c_str(), MemBuf)) {
+    errs() << "Error reading '" << IRFilename << "': "
+           << ec.message() << "\n";
+    return ExitStatus = 1;
+  }
+
+  if (MemBuf->getBufferSize() % 4 != 0) {
+    errs() << IRFilename
+           << ": Bitcode stream should be a multiple of 4 bytes in length.\n";
+    return ExitStatus = 1;
+  }
+
+  const unsigned char *BufPtr = (const unsigned char *)MemBuf->getBufferStart();
+  const unsigned char *EndBufPtr = BufPtr+MemBuf->getBufferSize();
+
+  // Read header and verify it is good.
+  NaClBitcodeHeader Header;
+  if (Header.Read(BufPtr, EndBufPtr) || !Header.IsSupported()) {
+    errs() << "Invalid PNaCl bitcode header.\n";
+    return ExitStatus = 1;
+  }
+
+  // Create a bitstream reader to read the bitcode file.
+  NaClBitstreamReader InputStreamFile(BufPtr, EndBufPtr);
+  NaClBitstreamCursor InputStream(InputStreamFile);
+
+  OwningPtr<Module> Mod(
+      new Module(MemBuf->getBufferIdentifier(), getGlobalContext()));
+
+  Mod->setDataLayout(PNaClDataLayout);
+
+  TopLevelParser Parser(&*Mod, Header, InputStream);

[jvoung (off chromium), 2014/07/01 17:32:52]

Mod.get(), if you just want the raw pointer.

Or maybe it should just pass the ownership over.

Or maybe the Parser should be the thing constructing and owning the module the
whole time, not get it passed in. Then, when we are able to avoid LLVM types
there won't be a module to create at all.

[Karl, 2014/07/01 21:31:07]

Moved into Parser constructor.

+  int TopLevelBlocks = 0;
+  while (!InputStream.AtEndOfStream()) {
+    if (Parser.Parse()) return 1;
+    ++TopLevelBlocks;
+  }
+
+  if (TopLevelBlocks != 1) {
+    errs() << IRFilename << ": Contains more than one module. Found: "
+           << TopLevelBlocks << "\n";
+    return ExitStatus = 1;
+  }
+
+  return ExitStatus = (Parser.getNumErrors() > 0);
+}
+
+}