Fix testing for end of stream in bitstream reader.

[kschimpf]

Summary

This fixes a bug found while working on the bitcode reader. In
particular, the method BitstreamReader::AtEndOfStream doesn't always
behave correctly when processing a data streamer. The method
fillCurWord doesn't properly set CurWord/BitsInCurWord if the data
streamer was already at eof, but GetBytes had not yet set the
ObjectSize field of the streaming memory object.

This patch fixes this problem, and provides a test to show that
this problem has been fixed.

[kschimpf]

Will remove unnecessary include in StreamingMemoryObject.h

include/llvm/Support/StreamingMemoryObject.h

20 ↗

(On Diff #30268)

Delete this!

[• rafael]

include/llvm/Support/StreamingMemoryObject.h

56 ↗

(On Diff #30268)

I don't think constexpr is supported everywhere. Why do you need it?

[filcab]

unittests/Bitcode/BitReaderTest.cpp

109 ↗

(On Diff #30268)

Why std::move() a StringRef?

[kschimpf]

• Fix issues raised.
• Fix invalid-abbrev.bc to include 0's at end to close module block.

[kschimpf]

Note that I had to add a few zero-valued bytes to the end of (binary) test case test/Bitcode/Input/invalid-abbrev.bc. These zeros are needed to correctly make the bitcode reader terminate (it wasn't reading the last byte correctly due to eof bug fixed by this CL).

include/llvm/Support/StreamingMemoryObject.h

56 ↗

(On Diff #30268)

It isn't necessary. removing.

unittests/Bitcode/BitReaderTest.cpp

109 ↗

(On Diff #30268)

This is a leftover from when I created the memory buffer using a different constructor of a memory buffer. Removing.

[dschuff]

It looks like this test is leaking memory: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/6024/steps/check-llvm%20asan/logs/stdio

[kschimpf]

Looking into cause of test failure under compilation with ASAN. Also creating patch to remove constexpr from unittests/Bitcode/BitReaderTest.cpp.

llvm/trunk/include/llvm/Bitcode/BitstreamReader.h

void fillCurWord() {
     // Read the next word from the stream.
     uint8_t Array[sizeof(word_t)] = {0};
 
     uint64_t BytesRead =
         BitStream->getBitcodeBytes().readBytes(Array, sizeof(Array), NextChar);
 
     // If we run out of data, stop at the end of the stream.
     if (BytesRead == 0) {
+      CurWord = 0;
+      BitsInCurWord = 0;
       Size = NextChar;
       return;
     }
 
     CurWord =
         support::endian::read<word_t, support::little, support::unaligned>(
             Array);
     NextChar += BytesRead;

llvm/trunk/include/llvm/Support/StreamingMemoryObject.h

public:
   /// back into the stream once we've read them.
   bool dropLeadingBytes(size_t s);
 
   /// If the data object size is known in advance, many of the operations can
   /// be made more efficient, so this method should be called before reading
   /// starts (although it can be called anytime).
   void setKnownObjectSize(size_t size);
 
+  /// The number of bytes read at a time from the data streamer.
+  static const uint32_t kChunkSize = 4096 * 4;
+
 private:
-  const static uint32_t kChunkSize = 4096 * 4;
   mutable std::vector<unsigned char> Bytes;
   std::unique_ptr<DataStreamer> Streamer;
   mutable size_t BytesRead;   // Bytes read from stream
   size_t BytesSkipped;// Bytes skipped at start of stream (e.g. wrapper/header)
   mutable size_t ObjectSize; // 0 if unknown, set if wrapper seen or EOF reached
   mutable bool EOFReached;
 
   // Fetch enough bytes such that Pos can be read (i.e. BytesRead >

llvm/trunk/unittests/Bitcode/BitReaderTest.cpp

 //===- llvm/unittest/Bitcode/BitReaderTest.cpp - Tests for BitReader ------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/AsmParser/Parser.h"
+#include "llvm/Bitcode/BitstreamReader.h"
 #include "llvm/Bitcode/BitstreamWriter.h"
 #include "llvm/Bitcode/ReaderWriter.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Support/DataStream.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SourceMgr.h"
+#include "llvm/Support/StreamingMemoryObject.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
 std::unique_ptr<Module> parseAssembly(const char *Assembly) {
   SMDiagnostic Error;
getStreamedModuleFromAssembly(LLVMContext &Context, SmallString<1024> &Mem,
   std::unique_ptr<MemoryBuffer> Buffer =
       MemoryBuffer::getMemBuffer(Mem.str(), "test", false);
   auto Streamer = llvm::make_unique<BufferDataStreamer>(std::move(Buffer));
   ErrorOr<std::unique_ptr<Module>> ModuleOrErr =
       getStreamedBitcodeModule("test", std::move(Streamer), Context);
   return std::move(ModuleOrErr.get());
 }
 
+// Checks if we correctly detect eof if we try to read N bits when there are not
+// enough bits left on the input stream to read N bits, and we are using a data
+// streamer. In particular, it checks if we properly set the object size when
+// the eof is reached under such conditions.
+TEST(BitReaderTest, TestForEofAfterReadFailureOnDataStreamer) {
+  // Note: Because StreamingMemoryObject does a call to method GetBytes in it's
+  // constructor, using internal constant kChunkSize, we must fill the input
+  // with more characters than that amount.
+  static size_t InputSize = StreamingMemoryObject::kChunkSize + 5;
+  char *Text = new char[InputSize];
+  std::memset(Text, 'a', InputSize);
+  Text[InputSize - 1] = '\0';
+  StringRef Input(Text);
+
+  // Build bitsteam reader using data streamer.
+  auto MemoryBuf = MemoryBuffer::getMemBuffer(Input);
+  std::unique_ptr<DataStreamer> Streamer(
+      new BufferDataStreamer(std::move(MemoryBuf)));
+  auto OwnedBytes =
+      llvm::make_unique<StreamingMemoryObject>(std::move(Streamer));
+  auto Reader = llvm::make_unique<BitstreamReader>(std::move(OwnedBytes));
+  BitstreamCursor Cursor;
+  Cursor.init(Reader.get());
+
+  // Jump to two bytes before end of stream.
+  Cursor.JumpToBit((InputSize - 4) * CHAR_BIT);
+  // Try to read 4 bytes when only 2 are present, resulting in error value 0.
+  constexpr size_t ReadErrorValue = 0;
+  EXPECT_EQ(ReadErrorValue, Cursor.Read(32));
+  // Should be at eof now.
+  EXPECT_TRUE(Cursor.AtEndOfStream());
+}
+
 TEST(BitReaderTest, MateralizeForwardRefWithStream) {
   SmallString<1024> Mem;
 
   LLVMContext Context;
   std::unique_ptr<Module> M = getStreamedModuleFromAssembly(
       Context, Mem, "@table = constant i8* blockaddress(@func, %bb)\n"
                     "define void @func() {\n"
                     "  unreachable\n"