Add (unsupported experimental) feature allowing byte aligned bitcode.

include/llvm/Bitcode/NaCl/NaClBitstreamReader.h

 //===- NaClBitstreamReader.h -----------------------------------*- C++ -*-===//
 //     Low-level bitstream reader interface
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This header defines the BitstreamReader class.  This class can be used to
 // read an arbitrary bitstream, regardless of its contents.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_BITCODE_NACL_NACLBITSTREAMREADER_H
 #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"
 #include <climits>
 #include <vector>
 
 namespace llvm {
 
 class Deserializer;
 
@@ skipping 24 matching lines @@
 /// the NaClBitstreamCursor class.
 class NaClBitstreamReader {
 public:
   /// This contains information emitted to BLOCKINFO_BLOCK blocks. These
   /// describe abbreviations that all blocks of the specified ID inherit.
   struct BlockInfo {
     unsigned BlockID;
     std::vector<NaClBitCodeAbbrev*> Abbrevs;
   };
 private:
+  friend class NaClBitstreamCursor;
+
   std::unique_ptr<MemoryObject> BitcodeBytes;
 
   std::vector<BlockInfo> BlockInfoRecords;
 
   /// \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)
       : InitialAddress(MyInitialAddress) {
     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; }
 
   ~NaClBitstreamReader() {
     // Free the BlockInfoRecords.
     while (!BlockInfoRecords.empty()) {
       BlockInfo &Info = BlockInfoRecords.back();
       // Free blockinfo abbrev info.
       for (unsigned i = 0, e = static_cast<unsigned>(Info.Abbrevs.size());
@@ skipping 409 matching lines @@
 
       if ((Piece & (1U << (NumBits-1))) == 0)
         return Result;
 
       NextBit += NumBits-1;
       Piece = Read(NumBits);
     }
   }
 
 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.
     if (sizeof(word_t) > 4 &&
         BitsInCurWord >= 32) {
       CurWord >>= BitsInCurWord-32;
       BitsInCurWord = 32;
       return;
     }
 
@@ skipping 122 matching lines @@
   // Skips over an abbreviation record. Duplicates code of ReadAbbrevRecord,
   // except that no abbreviation is built.
   void SkipAbbrevRecord();
   
   bool ReadBlockInfoBlock(NaClAbbrevListener *Listener);
 };
 
 } // End llvm namespace
 
 #endif