Make abbreviations explicit in pnacl-freeze/thaw.

[1] Explicitly enumerate all abbreviation values, including the
maximum abbreviation for each type of block.

[2] Make "enter subblock" calculate number of bits needed by passing
in maximum abbreviation (associated with block) rather than requiring
the developer to compute this value every time a subblock is entered.

*NOTE* This code changes encoding sizes to be based on
the maximum allowed value, rather than requiring the
developer to calculate out the number of bits needed. This
change doesn't make  the PNaCL bitcode files
incompatable with LLVM bitcode files, since it does
not effect the bitcode reader.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3405
R=jvoung@chromium.org

Review URL: https://codereview.chromium.org/14813032

8 files changed, 333 insertions, 115 deletions

diff --git a/include/llvm/Bitcode/NaCl/NaClBitCodes.h b/include/llvm/Bitcode/NaCl/NaClBitCodes.h
index 4c0f754f7b..bb52d0e20e 100644
--- a/include/llvm/Bitcode/NaCl/NaClBitCodes.h
+++ b/include/llvm/Bitcode/NaCl/NaClBitCodes.h
@@ -21,6 +21,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/MathExtras.h"
 #include <cassert>
 
 namespace llvm {
@@ -49,7 +50,21 @@ namespace naclbitc {
     UNABBREV_RECORD = 3,
 
     // This is not a code, this is a marker for the first abbrev assignment.
-    FIRST_APPLICATION_ABBREV = 4
+    // In addition, we assume up to two additional enumerated constants are
+    // added for each extension. These constants are:
+    //
+    //   PREFIX_MAX_FIXED_ABBREV
+    //   PREFIX_MAX_ABBREV
+    //
+    // PREFIX_MAX_ABBREV defines the maximal enumeration value used for
+    // the code selector of a block. If Both PREFIX_MAX_FIXED_ABBREV
+    // and PREFIX_MAX_ABBREV is defined, then PREFIX_MAX_FIXED_ABBREV
+    // defines the last code selector of the block that must be read using
+    // a single read (i.e. a FIXED read, or the first chunk of a VBR read.
+    FIRST_APPLICATION_ABBREV = 4,
+    // Defines default values for code length, if no additional selectors
+    // are added.
+    DEFAULT_MAX_ABBREV = FIRST_APPLICATION_ABBREV-1
   };
 
   /// StandardBlockIDs - All bitcode files can optionally include a BLOCKINFO
@@ -184,6 +199,59 @@ public:
     OperandList.push_back(OpInfo);
   }
 };
+
+/// \brief Returns number of bits needed to encode
+/// value for dense FIXED encoding.
+inline unsigned NaClBitsNeededForValue(unsigned Value) {
+  // Note: Need to handle case where Value=0xFFFFFFFF as special case,
+  // since we can't add 1 to it.
+  if (Value >= 0x80000000) return 32;
+  return Log2_32_Ceil(Value+1);
+}
+
+/// \brief Encode a signed value by moving the sign to the LSB for dense
+/// VBR encoding.
+inline uint64_t NaClEncodeSignRotatedValue(int64_t V) {
+  return (V >= 0) ? (V << 1) : ((-V << 1) | 1);
+}
+
+/// \brief Decode a signed value stored with the sign bit in
+/// the LSB for dense VBR encoding.
+inline uint64_t NaClDecodeSignRotatedValue(uint64_t V) {
+  if ((V & 1) == 0)
+    return V >> 1;
+  if (V != 1)
+    return -(V >> 1);
+  // There is no such thing as -0 with integers.  "-0" really means MININT.
+  return 1ULL << 63;
+}
+
+/// \brief This class determines whether a FIXED or VBR
+/// abbreviation should be used for the selector, and the number of bits
+/// needed to capture such selectors.
+class NaClBitcodeSelectorAbbrev {
+
+public:
+  // If true, use a FIXED abbreviation. Otherwise, use a VBR abbreviation.
+  bool IsFixed;
+  // Number of bits needed for selector.
+  unsigned NumBits;
+
+  // Creates a selector range for the given values.
+  NaClBitcodeSelectorAbbrev(bool IF, unsigned NB)
+      : IsFixed(IF), NumBits(NB) {}
+
+  // Creates a selector range when no abbreviations are defined.
+  NaClBitcodeSelectorAbbrev()
+      : IsFixed(true),
+        NumBits(NaClBitsNeededForValue(naclbitc::DEFAULT_MAX_ABBREV)) {}
+
+  // Creates a selector range to handle fixed abbrevations up to
+  // the specified value.
+  explicit NaClBitcodeSelectorAbbrev(unsigned MaxAbbrev)
+      : IsFixed(true),
+        NumBits(NaClBitsNeededForValue(MaxAbbrev)) {}
+};
 } // End llvm namespace
 
 #endif
diff --git a/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h b/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h
index 58c0a5d7fa..4c532bbc37 100644
--- a/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h
+++ b/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h
@@ -171,7 +171,6 @@ class NaClBitstreamCursor {
   NaClBitstreamReader *BitStream;
   size_t NextChar;
 
-  
   /// CurWord/word_t - This is the current data we have pulled from the stream
   /// but have not returned to the client.  This is specifically and
   /// intentionally defined to follow the word size of the host machine for
@@ -186,21 +185,22 @@ class NaClBitstreamCursor {
 
   // CurCodeSize - This is the declared size of code values used for the current
   // block, in bits.
-  unsigned CurCodeSize;
+  NaClBitcodeSelectorAbbrev CurCodeSize;
 
   /// CurAbbrevs - Abbrevs installed at in this block.
   std::vector<NaClBitCodeAbbrev*> CurAbbrevs;
 
   struct Block {
-    unsigned PrevCodeSize;
+    NaClBitcodeSelectorAbbrev PrevCodeSize;
     std::vector<NaClBitCodeAbbrev*> PrevAbbrevs;
-    explicit Block(unsigned PCS) : PrevCodeSize(PCS) {}
+    explicit Block() : PrevCodeSize() {}
+    explicit Block(const NaClBitcodeSelectorAbbrev& PCS)
+        : PrevCodeSize(PCS) {}
   };
 
   /// BlockScope - This tracks the codesize of parent blocks.
   SmallVector<Block, 8> BlockScope;
 
-  
 public:
   NaClBitstreamCursor() : BitStream(0), NextChar(0) {
   }
@@ -213,7 +213,6 @@ public:
     NextChar = 0;
     CurWord = 0;
     BitsInCurWord = 0;
-    CurCodeSize = 2;
   }
 
   void init(NaClBitstreamReader &R) {
@@ -223,7 +222,6 @@ public:
     NextChar = 0;
     CurWord = 0;
     BitsInCurWord = 0;
-    CurCodeSize = 2;
   }
 
   ~NaClBitstreamCursor() {
@@ -255,7 +253,7 @@ public:
   }
 
   /// getAbbrevIDWidth - Return the number of bits used to encode an abbrev #.
-  unsigned getAbbrevIDWidth() const { return CurCodeSize; }
+  unsigned getAbbrevIDWidth() const { return CurCodeSize.NumBits; }
 
   /// GetCurrentBitNo - Return the bit # of the bit we are reading.
   uint64_t GetCurrentBitNo() const {
@@ -343,7 +341,6 @@ public:
     }
   }
 
-
   uint32_t Read(unsigned NumBits) {
     assert(NumBits && NumBits <= 32 &&
            "Cannot return zero or more than 32 bits!");
@@ -459,10 +456,11 @@ private:
 public:
 
   unsigned ReadCode() {
-    return Read(CurCodeSize);
+    return CurCodeSize.IsFixed
+        ? Read(CurCodeSize.NumBits)
+        : ReadVBR(CurCodeSize.NumBits);
   }
 
-
   // Block header:
   //    [ENTER_SUBBLOCK, blockid, newcodelen, <align4bytes>, blocklen]
 
diff --git a/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h b/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h
index 26d9cb6b24..b4123261a0 100644
--- a/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h
+++ b/include/llvm/Bitcode/NaCl/NaClBitstreamWriter.h
@@ -33,7 +33,7 @@ class NaClBitstreamWriter {
 
   /// CurCodeSize - This is the declared size of code values used for the
   /// current block, in bits.
-  unsigned CurCodeSize;
+  NaClBitcodeSelectorAbbrev CurCodeSize;
 
   /// BlockInfoCurBID - When emitting a BLOCKINFO_BLOCK, this is the currently
   /// selected BLOCK ID.
@@ -43,10 +43,11 @@ class NaClBitstreamWriter {
   std::vector<NaClBitCodeAbbrev*> CurAbbrevs;
 
   struct Block {
-    unsigned PrevCodeSize;
+    NaClBitcodeSelectorAbbrev PrevCodeSize;
     unsigned StartSizeWord;
     std::vector<NaClBitCodeAbbrev*> PrevAbbrevs;
-    Block(unsigned PCS, unsigned SSW) : PrevCodeSize(PCS), StartSizeWord(SSW) {}
+    Block(const NaClBitcodeSelectorAbbrev& PCS, unsigned SSW)
+        : PrevCodeSize(PCS), StartSizeWord(SSW) {}
   };
 
   /// BlockScope - This tracks the current blocks that we have entered.
@@ -94,7 +95,7 @@ class NaClBitstreamWriter {
 
 public:
   explicit NaClBitstreamWriter(SmallVectorImpl<char> &O)
-    : Out(O), CurBit(0), CurValue(0), CurCodeSize(2) {}
+      : Out(O), CurBit(0), CurValue(0), CurCodeSize() {}
 
   ~NaClBitstreamWriter() {
     assert(CurBit == 0 && "Unflused data remaining");
@@ -156,6 +157,7 @@ public:
 
   void EmitVBR(uint32_t Val, unsigned NumBits) {
     assert(NumBits <= 32 && "Too many bits to emit!");
+    assert(NumBits > 1 && "Too few bits to emit!");
     uint32_t Threshold = 1U << (NumBits-1);
 
     // Emit the bits with VBR encoding, NumBits-1 bits at a time.
@@ -169,6 +171,7 @@ public:
 
   void EmitVBR64(uint64_t Val, unsigned NumBits) {
     assert(NumBits <= 32 && "Too many bits to emit!");
+    assert(NumBits > 1 && "Too few bits to emit!");
     if ((uint32_t)Val == Val)
       return EmitVBR((uint32_t)Val, NumBits);
 
@@ -186,7 +189,10 @@ public:
 
   /// EmitCode - Emit the specified code.
   void EmitCode(unsigned Val) {
-    Emit(Val, CurCodeSize);
+    if (CurCodeSize.IsFixed)
+      Emit(Val, CurCodeSize.NumBits);
+    else
+      EmitVBR(Val, CurCodeSize.NumBits);
   }
 
   //===--------------------------------------------------------------------===//
@@ -207,16 +213,22 @@ public:
     return 0;
   }
 
-  void EnterSubblock(unsigned BlockID, unsigned CodeLen) {
+private:
+  // Enter block using CodeLen bits to read the size of the code
+  // selector associated with the block.
+  void EnterSubblock(unsigned BlockID,
+                     const NaClBitcodeSelectorAbbrev& CodeLen,
+                     BlockInfo *Info) {
     // Block header:
     //    [ENTER_SUBBLOCK, blockid, newcodelen, <align4bytes>, blocklen]
     EmitCode(naclbitc::ENTER_SUBBLOCK);
     EmitVBR(BlockID, naclbitc::BlockIDWidth);
-    EmitVBR(CodeLen, naclbitc::CodeLenWidth);
+    assert(CodeLen.IsFixed && "Block codelens must be fixed");
+    EmitVBR(CodeLen.NumBits, naclbitc::CodeLenWidth);
     FlushToWord();
 
     unsigned BlockSizeWordIndex = GetWordIndex();
-    unsigned OldCodeSize = CurCodeSize;
+    NaClBitcodeSelectorAbbrev OldCodeSize(CurCodeSize);
 
     // Emit a placeholder, which will be replaced when the block is popped.
     Emit(0, naclbitc::BlockSizeWidth);
@@ -230,7 +242,7 @@ public:
 
     // If there is a blockinfo for this BlockID, add all the predefined abbrevs
     // to the abbrev list.
-    if (BlockInfo *Info = getBlockInfo(BlockID)) {
+    if (Info) {
       for (unsigned i = 0, e = static_cast<unsigned>(Info->Abbrevs.size());
            i != e; ++i) {
         CurAbbrevs.push_back(Info->Abbrevs[i]);
@@ -239,6 +251,31 @@ public:
     }
   }
 
+public:
+  /// \brief Enter block using CodeLen bits to read the size of the code
+  /// selector associated with the block.
+  void EnterSubblock(unsigned BlockID,
+                     const NaClBitcodeSelectorAbbrev& CodeLen) {
+    EnterSubblock(BlockID, CodeLen, getBlockInfo(BlockID));
+  }
+
+  /// \brief Enter block, using a code length based on the number of
+  /// (global) BlockInfo entries defined for the block. Note: This
+  /// should be used only if the block doesn't define any local abbreviations.
+  void EnterSubblock(unsigned BlockID) {
+    BlockInfo *Info = getBlockInfo(BlockID);
+    size_t NumAbbrevs = Info ? Info->Abbrevs.size() : 0;
+    NaClBitcodeSelectorAbbrev DefaultCodeLen(
+        naclbitc::DEFAULT_MAX_ABBREV+NumAbbrevs);
+    EnterSubblock(BlockID, DefaultCodeLen, Info);
+  }
+
+  /// \brief Enter block with the given number of abbreviations.
+  void EnterSubblock(unsigned BlockID, unsigned NumAbbrev) {
+    NaClBitcodeSelectorAbbrev CodeLenAbbrev(NumAbbrev);
+    EnterSubblock(BlockID, CodeLenAbbrev);
+  }
+
   void ExitBlock() {
     assert(!BlockScope.empty() && "Block scope imbalance!");
 
@@ -501,8 +538,8 @@ public:
   //===--------------------------------------------------------------------===//
 
   /// EnterBlockInfoBlock - Start emitting the BLOCKINFO_BLOCK.
-  void EnterBlockInfoBlock(unsigned CodeWidth) {
-    EnterSubblock(naclbitc::BLOCKINFO_BLOCK_ID, CodeWidth);
+  void EnterBlockInfoBlock() {
+    EnterSubblock(naclbitc::BLOCKINFO_BLOCK_ID);
     BlockInfoCurBID = ~0U;
   }
 private:
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
index 5317e11a27..df93c1b4de 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
+++ b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.cpp
@@ -8,6 +8,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+#define DEBUG_TYPE "NaClBitcodeReader"
+
 #include "llvm/Bitcode/NaCl/NaClReaderWriter.h"
 #include "NaClBitcodeReader.h"
 #include "llvm/ADT/SmallString.h"
@@ -20,6 +22,7 @@
 #include "llvm/IR/Module.h"
 #include "llvm/IR/OperandTraits.h"
 #include "llvm/IR/Operator.h"
+#include "llvm/Support/Debug.h"
 #include "llvm/Support/DataStream.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -450,6 +453,7 @@ static void decodeLLVMAttributesForBitcode(AttrBuilder &B,
 }
 
 bool NaClBitcodeReader::ParseAttributeBlock() {
+  DEBUG(dbgs() << "-> ParseAttributeBlock\n");
   if (Stream.EnterSubBlock(naclbitc::PARAMATTR_BLOCK_ID))
     return Error("Malformed block record");
 
@@ -469,6 +473,7 @@ bool NaClBitcodeReader::ParseAttributeBlock() {
     case NaClBitstreamEntry::Error:
       return Error("Error at end of PARAMATTR block");
     case NaClBitstreamEntry::EndBlock:
+      DEBUG(dbgs() << "<- ParseAttributeBlock\n");
       return false;
     case NaClBitstreamEntry::Record:
       // The interesting case.
@@ -508,6 +513,7 @@ bool NaClBitcodeReader::ParseAttributeBlock() {
 }
 
 bool NaClBitcodeReader::ParseAttributeGroupBlock() {
+  DEBUG(dbgs() << "-> ParseAttributeGroupBlock\n");
   if (Stream.EnterSubBlock(naclbitc::PARAMATTR_GROUP_BLOCK_ID))
     return Error("Malformed block record");
 
@@ -525,6 +531,7 @@ bool NaClBitcodeReader::ParseAttributeGroupBlock() {
     case NaClBitstreamEntry::Error:
       return Error("Error at end of PARAMATTR_GROUP block");
     case NaClBitstreamEntry::EndBlock:
+      DEBUG(dbgs() << "<- ParseAttributeGroupBlock\n");
       return false;
     case NaClBitstreamEntry::Record:
       // The interesting case.
@@ -584,10 +591,14 @@ bool NaClBitcodeReader::ParseAttributeGroupBlock() {
 }
 
 bool NaClBitcodeReader::ParseTypeTable() {
+  DEBUG(dbgs() << "-> ParseTypeTable\n");
   if (Stream.EnterSubBlock(naclbitc::TYPE_BLOCK_ID_NEW))
     return Error("Malformed block record");
 
-  return ParseTypeTableBody();
+  bool result = ParseTypeTableBody();
+  if (!result)
+    DEBUG(dbgs() << "<- ParseTypeTable\n");
+  return result;
 }
 
 bool NaClBitcodeReader::ParseTypeTableBody() {
@@ -810,6 +821,7 @@ bool NaClBitcodeReader::ParseTypeTableBody() {
 }
 
 bool NaClBitcodeReader::ParseValueSymbolTable() {
+  DEBUG(dbgs() << "-> ParseValueSymbolTable\n");
   if (Stream.EnterSubBlock(naclbitc::VALUE_SYMTAB_BLOCK_ID))
     return Error("Malformed block record");
 
@@ -825,6 +837,7 @@ bool NaClBitcodeReader::ParseValueSymbolTable() {
     case NaClBitstreamEntry::Error:
       return Error("malformed value symbol table block");
     case NaClBitstreamEntry::EndBlock:
+      DEBUG(dbgs() << "<- ParseValueSymbolTable\n");
       return false;
     case NaClBitstreamEntry::Record:
       // The interesting case.
@@ -866,6 +879,7 @@ bool NaClBitcodeReader::ParseValueSymbolTable() {
 bool NaClBitcodeReader::ParseMetadata() {
   unsigned NextMDValueNo = MDValueList.size();
 
+  DEBUG(dbgs() << "-> ParseMetadata\n");
   if (Stream.EnterSubBlock(naclbitc::METADATA_BLOCK_ID))
     return Error("Malformed block record");
 
@@ -881,6 +895,7 @@ bool NaClBitcodeReader::ParseMetadata() {
       Error("malformed metadata block");
       return true;
     case NaClBitstreamEntry::EndBlock:
+      DEBUG(dbgs() << "<- ParseMetadata\n");
       return false;
     case NaClBitstreamEntry::Record:
       // The interesting case.
@@ -961,17 +976,6 @@ bool NaClBitcodeReader::ParseMetadata() {
   }
 }
 
-/// decodeSignRotatedValue - Decode a signed value stored with the sign bit in
-/// the LSB for dense VBR encoding.
-uint64_t NaClBitcodeReader::decodeSignRotatedValue(uint64_t V) {
-  if ((V & 1) == 0)
-    return V >> 1;
-  if (V != 1)
-    return -(V >> 1);
-  // There is no such thing as -0 with integers.  "-0" really means MININT.
-  return 1ULL << 63;
-}
-
 /// ResolveGlobalAndAliasInits - Resolve all of the initializers for global
 /// values and aliases that we can.
 bool NaClBitcodeReader::ResolveGlobalAndAliasInits() {
@@ -1013,12 +1017,13 @@ bool NaClBitcodeReader::ResolveGlobalAndAliasInits() {
 static APInt ReadWideAPInt(ArrayRef<uint64_t> Vals, unsigned TypeBits) {
   SmallVector<uint64_t, 8> Words(Vals.size());
   std::transform(Vals.begin(), Vals.end(), Words.begin(),
-                 NaClBitcodeReader::decodeSignRotatedValue);
+                 NaClDecodeSignRotatedValue);
 
   return APInt(TypeBits, Words);
 }
 
 bool NaClBitcodeReader::ParseConstants() {
+  DEBUG(dbgs() << "-> ParseConstants\n");
   if (Stream.EnterSubBlock(naclbitc::CONSTANTS_BLOCK_ID))
     return Error("Malformed block record");
 
@@ -1041,6 +1046,7 @@ bool NaClBitcodeReader::ParseConstants() {
       // Once all the constants have been read, go through and resolve forward
       // references.
       ValueList.ResolveConstantForwardRefs();
+      DEBUG(dbgs() << "<- ParseConstants\n");
       return false;
     case NaClBitstreamEntry::Record:
       // The interesting case.
@@ -1069,7 +1075,7 @@ bool NaClBitcodeReader::ParseConstants() {
     case naclbitc::CST_CODE_INTEGER:   // INTEGER: [intval]
       if (!CurTy->isIntegerTy() || Record.empty())
         return Error("Invalid CST_INTEGER record");
-      V = ConstantInt::get(CurTy, decodeSignRotatedValue(Record[0]));
+      V = ConstantInt::get(CurTy, NaClDecodeSignRotatedValue(Record[0]));
       break;
     case naclbitc::CST_CODE_WIDE_INTEGER: {// WIDE_INTEGER: [n x intval]
       if (!CurTy->isIntegerTy() || Record.empty())
@@ -1415,6 +1421,7 @@ bool NaClBitcodeReader::ParseConstants() {
 }
 
 bool NaClBitcodeReader::ParseUseLists() {
+  DEBUG(dbgs() << "-> ParseUseLists\n");
   if (Stream.EnterSubBlock(naclbitc::USELIST_BLOCK_ID))
     return Error("Malformed block record");
 
@@ -1429,6 +1436,7 @@ bool NaClBitcodeReader::ParseUseLists() {
     case NaClBitstreamEntry::Error:
       return Error("malformed use list block");
     case NaClBitstreamEntry::EndBlock:
+      DEBUG(dbgs() << "<- ParseUseLists\n");
       return false;
     case NaClBitstreamEntry::Record:
       // The interesting case.
@@ -1455,6 +1463,7 @@ bool NaClBitcodeReader::ParseUseLists() {
 /// remember where it is and then skip it.  This lets us lazily deserialize the
 /// functions.
 bool NaClBitcodeReader::RememberAndSkipFunctionBody() {
+  DEBUG(dbgs() << "-> RememberAndSkipFunctionBody\n");
   // Get the function we are talking about.
   if (FunctionsWithBodies.empty())
     return Error("Insufficient function protos");
@@ -1469,6 +1478,7 @@ bool NaClBitcodeReader::RememberAndSkipFunctionBody() {
   // Skip over the function block for now.
   if (Stream.SkipBlock())
     return Error("Malformed block record");
+  DEBUG(dbgs() << "<- RememberAndSkipFunctionBody\n");
   return false;
 }
 
@@ -1499,6 +1509,7 @@ bool NaClBitcodeReader::GlobalCleanup() {
 }
 
 bool NaClBitcodeReader::ParseModule(bool Resume) {
+  DEBUG(dbgs() << "-> ParseModule\n");
   if (Resume)
     Stream.JumpToBit(NextUnreadBit);
   else if (Stream.EnterSubBlock(naclbitc::MODULE_BLOCK_ID))
@@ -1517,11 +1528,13 @@ bool NaClBitcodeReader::ParseModule(bool Resume) {
       Error("malformed module block");
       return true;
     case NaClBitstreamEntry::EndBlock:
+      DEBUG(dbgs() << "<- ParseModule\n");
       return GlobalCleanup();
 
     case NaClBitstreamEntry::SubBlock:
       switch (Entry.ID) {
       default:  // Skip unknown content.
+        DEBUG(dbgs() << "Skip unknown context\n");
         if (Stream.SkipBlock())
           return Error("Malformed block record");
         break;
@@ -1566,6 +1579,7 @@ bool NaClBitcodeReader::ParseModule(bool Resume) {
 
         if (RememberAndSkipFunctionBody())
           return true;
+
         // For streaming bitcode, suspend parsing when we reach the function
         // bodies. Subsequent materialization calls will resume it when
         // necessary. For streaming, the function bodies must be at the end of
@@ -1574,6 +1588,7 @@ bool NaClBitcodeReader::ParseModule(bool Resume) {
         // just finish the parse now.
         if (LazyStreamer && SeenValueSymbolTable) {
           NextUnreadBit = Stream.GetCurrentBitNo();
+          DEBUG(dbgs() << "<- ParseModule\n");
           return false;
         }
         break;
@@ -1861,6 +1876,7 @@ bool NaClBitcodeReader::ParseBitcodeInto(Module *M) {
 }
 
 bool NaClBitcodeReader::ParseModuleTriple(std::string &Triple) {
+  DEBUG(dbgs() << "-> ParseModuleTriple\n");
   if (Stream.EnterSubBlock(naclbitc::MODULE_BLOCK_ID))
     return Error("Malformed block record");
 
@@ -1875,6 +1891,7 @@ bool NaClBitcodeReader::ParseModuleTriple(std::string &Triple) {
     case NaClBitstreamEntry::Error:
       return Error("malformed module block");
     case NaClBitstreamEntry::EndBlock:
+      DEBUG(dbgs() << "<- ParseModuleTriple\n");
       return false;
     case NaClBitstreamEntry::Record:
       // The interesting case.
@@ -1940,6 +1957,7 @@ bool NaClBitcodeReader::ParseTriple(std::string &Triple) {
 
 /// ParseMetadataAttachment - Parse metadata attachments.
 bool NaClBitcodeReader::ParseMetadataAttachment() {
+  DEBUG(dbgs() << "-> ParseMetadataAttachment\n");
   if (Stream.EnterSubBlock(naclbitc::METADATA_ATTACHMENT_ID))
     return Error("Malformed block record");
 
@@ -1952,6 +1970,7 @@ bool NaClBitcodeReader::ParseMetadataAttachment() {
     case NaClBitstreamEntry::Error:
       return Error("malformed metadata block");
     case NaClBitstreamEntry::EndBlock:
+      DEBUG(dbgs() << "<- ParseMetadataAttachment\n");
       return false;
     case NaClBitstreamEntry::Record:
       // The interesting case.
@@ -1985,6 +2004,7 @@ bool NaClBitcodeReader::ParseMetadataAttachment() {
 
 /// ParseFunctionBody - Lazily parse the specified function body block.
 bool NaClBitcodeReader::ParseFunctionBody(Function *F) {
+  DEBUG(dbgs() << "-> ParseFunctionBody\n");
   if (Stream.EnterSubBlock(naclbitc::FUNCTION_BLOCK_ID))
     return Error("Malformed block record");
 
@@ -2016,21 +2036,26 @@ bool NaClBitcodeReader::ParseFunctionBody(Function *F) {
     case NaClBitstreamEntry::SubBlock:
       switch (Entry.ID) {
       default:  // Skip unknown content.
+        dbgs() << "default skip block\n";
         if (Stream.SkipBlock())
           return Error("Malformed block record");
         break;
       case naclbitc::CONSTANTS_BLOCK_ID:
-        if (ParseConstants()) return true;
+        if (ParseConstants())
+          return true;
         NextValueNo = ValueList.size();
         break;
       case naclbitc::VALUE_SYMTAB_BLOCK_ID:
-        if (ParseValueSymbolTable()) return true;
+        if (ParseValueSymbolTable())
+          return true;
         break;
       case naclbitc::METADATA_ATTACHMENT_ID:
-        if (ParseMetadataAttachment()) return true;
+        if (ParseMetadataAttachment())
+          return true;
         break;
       case naclbitc::METADATA_BLOCK_ID:
-        if (ParseMetadata()) return true;
+        if (ParseMetadata())
+          return true;
         break;
       }
       continue;
@@ -2862,6 +2887,7 @@ OutOfRecordLoop:
   ValueList.shrinkTo(ModuleValueListSize);
   MDValueList.shrinkTo(ModuleMDValueListSize);
   std::vector<BasicBlock*>().swap(FunctionBBs);
+  DEBUG(dbgs() << "-> ParseFunctionBody\n");
   return false;
 }
 
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
index f6e3e94841..f3d36862c3 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
+++ b/lib/Bitcode/NaCl/Reader/NaClBitcodeReader.h
@@ -237,8 +237,6 @@ public:
   /// @returns true if an error occurred.
   bool ParseTriple(std::string &Triple);
 
-  static uint64_t decodeSignRotatedValue(uint64_t V);
-
 private:
   Type *getTypeByID(unsigned ID);
   Value *getFnValueByID(unsigned ID, Type *Ty) {
@@ -315,7 +313,7 @@ private:
   Value *getValueSigned(SmallVector<uint64_t, 64> &Record, unsigned Slot,
                         unsigned InstNum, Type *Ty) {
     if (Slot == Record.size()) return 0;
-    unsigned ValNo = (unsigned)decodeSignRotatedValue(Record[Slot]);
+    unsigned ValNo = (unsigned) NaClDecodeSignRotatedValue(Record[Slot]);
     // Adjust the ValNo, if it was encoded relative to the InstNum.
     if (UseRelativeIDs)
       ValNo = InstNum - ValNo;
diff --git a/lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp b/lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp
index 677539071b..d75c42ca9b 100644
--- a/lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp
+++ b/lib/Bitcode/NaCl/Reader/NaClBitstreamReader.cpp
@@ -71,13 +71,14 @@ bool NaClBitstreamCursor::EnterSubBlock(unsigned BlockID, unsigned *NumWordsP) {
   }
 
   // Get the codesize of this block.
-  CurCodeSize = ReadVBR(naclbitc::CodeLenWidth);
+  CurCodeSize.IsFixed = true;
+  CurCodeSize.NumBits = ReadVBR(naclbitc::CodeLenWidth);
   SkipToFourByteBoundary();
   unsigned NumWords = Read(naclbitc::BlockSizeWidth);
   if (NumWordsP) *NumWordsP = NumWords;
 
   // Validate that this block is sane.
-  if (CurCodeSize == 0 || AtEndOfStream())
+  if (CurCodeSize.NumBits == 0 || AtEndOfStream())
     return true;
 
   return false;
diff --git a/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp b/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
index 2e318020ad..d77a79a2c9 100644
--- a/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
+++ b/lib/Bitcode/NaCl/Writer/NaClBitcodeWriter.cpp
@@ -11,6 +11,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+#define DEBUG_TYPE "NaClBitcodeWriter"
+
 #include "llvm/Bitcode/NaCl/NaClReaderWriter.h"
 #include "NaClValueEnumerator.h"
 #include "llvm/ADT/Triple.h"
@@ -23,6 +25,7 @@
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/ValueSymbolTable.h"
+#include "llvm/Support/Debug.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
@@ -32,20 +35,39 @@
 #include <map>
 using namespace llvm;
 
-/// These are manifest constants used by the bitcode writer. They do not need to
-/// be kept in sync with the reader, but need to be consistent within this file.
+/// These are manifest constants used by the bitcode writer. They do
+/// not need to be kept in sync with the reader, but need to be
+/// consistent within this file.
+///
+/// Note that for each block type GROUP, the last entry should be of
+/// the form:
+///
+///    GROUP_MAX_ABBREV = GROUP_LAST_ABBREV,
+///
+/// where GROUP_LAST_ABBREV is the last defined abbreviation. See
+/// include file "llvm/Bitcode/NaCl/NaClBitCodes.h" for more
+/// information on how groups should be defined.
 enum {
   // VALUE_SYMTAB_BLOCK abbrev id's.
   VST_ENTRY_8_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV,
   VST_ENTRY_7_ABBREV,
   VST_ENTRY_6_ABBREV,
   VST_BBENTRY_6_ABBREV,
+  VST_MAX_ABBREV = VST_BBENTRY_6_ABBREV,
 
   // CONSTANTS_BLOCK abbrev id's.
   CONSTANTS_SETTYPE_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV,
   CONSTANTS_INTEGER_ABBREV,
   CONSTANTS_CE_CAST_Abbrev,
   CONSTANTS_NULL_Abbrev,
+  CONSTANTS_MAX_ABBREV = CONSTANTS_NULL_Abbrev,
+
+  // CONSTANTS_BLOCK abbrev id's when global (extends list above).
+  CST_CONSTANTS_AGGREGATE_ABBREV = CONSTANTS_MAX_ABBREV+1,
+  CST_CONSTANTS_STRING_ABBREV,
+  CST_CONSTANTS_CSTRING_7_ABBREV,
+  CST_CONSTANTS_CSTRING_6_ABBREV,
+  CST_CONSTANTS_MAX_ABBREV = CST_CONSTANTS_CSTRING_6_ABBREV,
 
   // FUNCTION_BLOCK abbrev id's.
   FUNCTION_INST_LOAD_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV,
@@ -55,6 +77,24 @@ enum {
   FUNCTION_INST_RET_VOID_ABBREV,
   FUNCTION_INST_RET_VAL_ABBREV,
   FUNCTION_INST_UNREACHABLE_ABBREV,
+  FUNCTION_INST_MAX_ABBREV = FUNCTION_INST_UNREACHABLE_ABBREV,
+
+  // TYPE_BLOCK_ID_NEW abbrev id's.
+  TYPE_POINTER_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV,
+  TYPE_FUNCTION_ABBREV,
+  TYPE_STRUCT_ANON_ABBREV,
+  TYPE_STRUCT_NAME_ABBREV,
+  TYPE_STRUCT_NAMED_ABBREV,
+  TYPE_ARRAY_ABBREV,
+  TYPE_MAX_ABBREV = TYPE_ARRAY_ABBREV,
+
+  // META_DATA_BLOCK abbrev id's.
+  METADATA_STRING_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV,
+  METADATA_MAX_ABBREV = METADATA_STRING_ABBREV,
+
+  // MODULE_BLOCK abbrev id's.
+  MODULE_GLOBALVAR_ABBREV = naclbitc::FIRST_APPLICATION_ABBREV,
+  MODULE_MAX_ABBREV = MODULE_GLOBALVAR_ABBREV,
 
   // SwitchInst Magic
   SWITCH_INST_MAGIC = 0x4B5 // May 2012 => 1205 => Hex
@@ -160,8 +200,9 @@ static void WriteAttributeGroupTable(const NaClValueEnumerator &VE,
                                      NaClBitstreamWriter &Stream) {
   const std::vector<AttributeSet> &AttrGrps = VE.getAttributeGroups();
   if (AttrGrps.empty()) return;
+  DEBUG(dbgs() << "-> WriteAbbributeGroupTable\n");
 
-  Stream.EnterSubblock(naclbitc::PARAMATTR_GROUP_BLOCK_ID, 3);
+  Stream.EnterSubblock(naclbitc::PARAMATTR_GROUP_BLOCK_ID);
 
   SmallVector<uint64_t, 64> Record;
   for (unsigned i = 0, e = AttrGrps.size(); i != e; ++i) {
@@ -202,14 +243,16 @@ static void WriteAttributeGroupTable(const NaClValueEnumerator &VE,
   }
 
   Stream.ExitBlock();
+  DEBUG(dbgs() << "<- WriteAbbributeGroupTable\n");
 }
 
 static void WriteAttributeTable(const NaClValueEnumerator &VE,
                                 NaClBitstreamWriter &Stream) {
   const std::vector<AttributeSet> &Attrs = VE.getAttributes();
   if (Attrs.empty()) return;
+  DEBUG(dbgs() << "-> WriteAttributeTable\n");
 
-  Stream.EnterSubblock(naclbitc::PARAMATTR_BLOCK_ID, 3);
+  Stream.EnterSubblock(naclbitc::PARAMATTR_BLOCK_ID);
 
   SmallVector<uint64_t, 64> Record;
   for (unsigned i = 0, e = Attrs.size(); i != e; ++i) {
@@ -222,23 +265,26 @@ static void WriteAttributeTable(const NaClValueEnumerator &VE,
   }
 
   Stream.ExitBlock();
+  DEBUG(dbgs() << "<- WriteAttributeTable\n");
 }
 
 /// WriteTypeTable - Write out the type table for a module.
 static void WriteTypeTable(const NaClValueEnumerator &VE,
                            NaClBitstreamWriter &Stream) {
+  DEBUG(dbgs() << "-> WriteTypeTable\n");
   const NaClValueEnumerator::TypeList &TypeList = VE.getTypes();
 
-  Stream.EnterSubblock(naclbitc::TYPE_BLOCK_ID_NEW,
-                       4 /*count from # abbrevs */);
+  Stream.EnterSubblock(naclbitc::TYPE_BLOCK_ID_NEW, TYPE_MAX_ABBREV);
+
   SmallVector<uint64_t, 64> TypeVals;
 
+
   // Note: modify to use maximum number of bits if under cutoff. Otherwise,
   // use VBR to take advantage that frequently referenced types have
   // small IDs.
   //
   // Note: Cutoff chosen based on experiments on pnacl-translate.pexe.
-  uint64_t NumBits = Log2_32_Ceil(VE.getTypes().size()+1);
+  uint64_t NumBits = NaClBitsNeededForValue(VE.getTypes().size());
   static const uint64_t TypeVBRCutoff = 6;
   uint64_t TypeIdNumBits = (NumBits <= TypeVBRCutoff ? NumBits : TypeVBRCutoff);
   NaClBitCodeAbbrevOp::Encoding TypeIdEncoding =
@@ -250,49 +296,51 @@ static void WriteTypeTable(const NaClValueEnumerator &VE,
   Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::TYPE_CODE_POINTER));
   Abbv->Add(NaClBitCodeAbbrevOp(TypeIdEncoding, TypeIdNumBits));
   Abbv->Add(NaClBitCodeAbbrevOp(0));  // Addrspace = 0
-  unsigned PtrAbbrev = Stream.EmitAbbrev(Abbv);
+  if (TYPE_POINTER_ABBREV != Stream.EmitAbbrev(Abbv))
+    llvm_unreachable("Unexpected abbrev ordering!");
 
   // Abbrev for TYPE_CODE_FUNCTION.
   Abbv = new NaClBitCodeAbbrev();
   Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::TYPE_CODE_FUNCTION));
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, 1));  // isvararg
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Array));
-  Abbv->Add(NaClBitCodeAbbrevOp(TypeIdEncoding, TypeIdNumBits));
-
-  unsigned FunctionAbbrev = Stream.EmitAbbrev(Abbv);
+  Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, NumBits));
+  if (TYPE_FUNCTION_ABBREV != Stream.EmitAbbrev(Abbv))
+    llvm_unreachable("Unexpected abbrev ordering!");
 
   // Abbrev for TYPE_CODE_STRUCT_ANON.
   Abbv = new NaClBitCodeAbbrev();
   Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::TYPE_CODE_STRUCT_ANON));
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, 1));  // ispacked
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Array));
-  Abbv->Add(NaClBitCodeAbbrevOp(TypeIdEncoding, TypeIdNumBits));
-
-  unsigned StructAnonAbbrev = Stream.EmitAbbrev(Abbv);
+  Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, NumBits));
+  if (TYPE_STRUCT_ANON_ABBREV != Stream.EmitAbbrev(Abbv))
+    llvm_unreachable("Unexpected abbrev ordering!");
 
   // Abbrev for TYPE_CODE_STRUCT_NAME.
   Abbv = new NaClBitCodeAbbrev();
   Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::TYPE_CODE_STRUCT_NAME));
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Array));
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Char6));
-  unsigned StructNameAbbrev = Stream.EmitAbbrev(Abbv);
+  if (TYPE_STRUCT_NAME_ABBREV != Stream.EmitAbbrev(Abbv))
+    llvm_unreachable("Unexpected abbrev ordering!");
 
   // Abbrev for TYPE_CODE_STRUCT_NAMED.
   Abbv = new NaClBitCodeAbbrev();
   Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::TYPE_CODE_STRUCT_NAMED));
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, 1));  // ispacked
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Array));
-  Abbv->Add(NaClBitCodeAbbrevOp(TypeIdEncoding, TypeIdNumBits));
-
-  unsigned StructNamedAbbrev = Stream.EmitAbbrev(Abbv);
+  Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, NumBits));
+  if (TYPE_STRUCT_NAMED_ABBREV != Stream.EmitAbbrev(Abbv))
+    llvm_unreachable("Unexpected abbrev ordering!");
 
   // Abbrev for TYPE_CODE_ARRAY.
   Abbv = new NaClBitCodeAbbrev();
   Abbv->Add(NaClBitCodeAbbrevOp(naclbitc::TYPE_CODE_ARRAY));
   Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::VBR, 8));   // size
-  Abbv->Add(NaClBitCodeAbbrevOp(TypeIdEncoding, TypeIdNumBits));
-
-  unsigned ArrayAbbrev = Stream.EmitAbbrev(Abbv);
+  Abbv->Add(NaClBitCodeAbbrevOp(NaClBitCodeAbbrevOp::Fixed, NumBits));
+  if (TYPE_ARRAY_ABBREV != Stream.EmitAbbrev(Abbv))
+    llvm_unreachable("Unexpected abbrev ordering!");
 
   // Emit an entry count so the reader can reserve space.
   TypeVals.push_back(TypeList.size());
@@ -329,7 +377,7 @@ static void WriteTypeTable(const NaClValueEnumerator &VE,
       TypeVals.push_back(VE.getTypeID(PTy->getElementType()));
       unsigned AddressSpace = PTy->getAddressSpace();
       TypeVals.push_back(AddressSpace);
-      if (AddressSpace == 0) AbbrevToUse = PtrAbbrev;
+      if (AddressSpace == 0) AbbrevToUse = TYPE_POINTER_ABBREV;
       break;
     }