Add (unsupported experimental) feature allowing byte aligned bitcode.

include/llvm/Bitcode/NaCl/NaClBitstreamReader.h

Index: include/llvm/Bitcode/NaCl/NaClBitstreamReader.h
diff --git a/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h b/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h
index db6eb6080b730e283e70a16615ff0fa1497d0f81..94f1715faec070c16b7a410a6cad8d6fa55b8bdb 100644
--- a/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h
+++ b/include/llvm/Bitcode/NaCl/NaClBitstreamReader.h
@@ -17,6 +17,7 @@
 #define LLVM_BITCODE_NACL_NACLBITSTREAMREADER_H
 
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Bitcode/NaCl/NaClBitcodeHeader.h"
 #include "llvm/Bitcode/NaCl/NaClLLVMBitCodes.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/StreamingMemoryObject.h"
@@ -61,6 +62,8 @@ public:
     std::vector<NaClBitCodeAbbrev*> Abbrevs;
   };
 private:
+  friend class NaClBitstreamCursor;
+
   std::unique_ptr<MemoryObject> BitcodeBytes;
 
   std::vector<BlockInfo> BlockInfoRecords;
@@ -68,15 +71,28 @@ private:
   /// \brief Holds the offset of the first byte after the header.
   size_t InitialAddress;
 
+  // True if filler should be added to byte align records.
+  bool AlignBitcodeRecords = false;
+
   NaClBitstreamReader(const NaClBitstreamReader&) LLVM_DELETED_FUNCTION;
   void operator=(const NaClBitstreamReader&) LLVM_DELETED_FUNCTION;
+
+  void initFromHeader(const NaClBitcodeHeader &Header) {
+    AlignBitcodeRecords = Header.getAlignBitcodeRecords();
+  }
+
 public:
   NaClBitstreamReader() : InitialAddress(0) {}
 
   NaClBitstreamReader(const unsigned char *Start, const unsigned char *End,
                       size_t MyInitialAddress=0) {
     InitialAddress = MyInitialAddress;

[jvoung (off chromium), 2015/05/07 18:11:17]

This variant no longer calls init(Start, End) so
BitcodeBytes.reset(getNonStreamedMemoryObject(Start, End)); no longer happens?

[Karl, 2015/05/07 22:18:54]

Bad refactoring on my part. Fixing to have both forms.

-    init(Start, End);
+  }
+
+  NaClBitstreamReader(const unsigned char *Start, const unsigned char *End,
+                      NaClBitcodeHeader &Header) {

[jvoung (off chromium), 2015/05/07 18:11:17]

Not a huge fan of having some many variants of the constructor, since you have
to make sure they all initialize enough of the fields.

Could you have just one of the "MyInitialAddress" vs "Header" variants instead
of both? It's not clear what the versions that don't call initFromHeader() are
missing out on.

Otherwise, could you make the InitialAddress initialization stylistically
consistent? One of ctors does "InitialAddress = Header.getHeaderSize();", and
the other does ": InitialAddress(Header.getHeaderSize())"?

[Karl, 2015/05/07 22:18:54]

There are two reasons for multiple headers. The first is that Subzero uses the
old constructor, and hence it can't be removed (or not yet). Some unit tests run
without adding a header, and hence it can't be removed.

However, many cases use a header, and it is annoying to have to repeat this
pattern on those cases. Once we fix subzero, I think we can
simplify these two constructors to:

NaClBitstreamReader(Start, End) and NaClBitstreamReader(Start, End, Header).
Note that we already have both forms for the MemoryObject version.

Decided to use MemoryObject on all (will submit fix to subzero separately).

[jvoung (off chromium), 2015/05/08 17:55:01]

Yeah it is definitely annoying to have to either set up a header or set up a
getNonStreamedMemoryObject in more places...

I'd consider the form with the explicit InitialAddress (no header) less useful
compared to the (Start, End) variant. The InitialAddress is used by passing in
Header.getHeaderSize(), but that fails to pass along the other header fields.

I brought this up because it seemed easy to start drifting on the amount of
initialization in the various ctor combos and wasn't clear which variants were
tested where. There were really (2 x 3 + 1) variants:

{Start/End vs MemObj} x {Header vs InitialAddress=X vs InitialAddress
unspecified) + null-ctor.

So considering those priorities, something like the following seemed reasonable
to me:

(1) NaClBitstreamReader(Start, End, Header)      // For convenience of
non-streaming, non-lazy tools.
(2) NaClBitstreamReader(MemObj, Header)          // For streaming/lazy tools.
(3) NaClBitstreamReader(MemObj, InitialAddress)  // Limit this use... Force
unittest to just pick a very specific form w/out the header and w/out a default
parameter.
(4) NaClBitstreamReader() // For ???

WDYT?

If (4) can be removed that would be nice too.

[Karl, 2015/05/08 21:08:59]

Looks reasonable, changing.

+    InitialAddress = Header.getHeaderSize();
+    init(Start, End, Header);
   }
 
   NaClBitstreamReader(MemoryObject *Bytes, size_t MyInitialAddress=0)
@@ -84,9 +100,17 @@ public:
     BitcodeBytes.reset(Bytes);
   }
 
-  void init(const unsigned char *Start, const unsigned char *End) {
+  NaClBitstreamReader(MemoryObject *Bytes, NaClBitcodeHeader &Header)
+      : InitialAddress(Header.getHeaderSize()) {
+    BitcodeBytes.reset(Bytes);
+    initFromHeader(Header);
+  }
+
+  void init(const unsigned char *Start, const unsigned char *End,
+            NaClBitcodeHeader &Header) {
     assert(((End-Start) & 3) == 0 &&"Bitcode stream not a multiple of 4 bytes");
     BitcodeBytes.reset(getNonStreamedMemoryObject(Start, End));
+    initFromHeader(Header);
   }
 
   MemoryObject &getBitcodeBytes() { return *BitcodeBytes; }
@@ -516,6 +540,19 @@ public:
   }
 
 private:
+  void SkipToByteBoundary() {
+    unsigned BitsToSkip = BitsInCurWord % CHAR_BIT;
+    if (BitsToSkip) {
+      CurWord >>= BitsToSkip;
+      BitsInCurWord -= BitsToSkip;
+    }
+  }
+
+  void SkipToByteBoundaryIfAligned() {
+    if (BitStream->AlignBitcodeRecords)
+      SkipToByteBoundary();
+  }
+
   void SkipToFourByteBoundary() {
     // If word_t is 64-bits and if we've read less than 32 bits, just dump
     // the bits we have up to the next 32-bit boundary.