Add error handling to the DataExtractor class

Summary:
This is motivated by D63591, where we realized that there isn't a really
good way of telling whether a DataExtractor is reading actual data, or
is it just returning default values because it reached the end of the
buffer.

This patch resolves that by providing a new "Cursor" class. A Cursor
object encapsulates two things:
- the current position/offset in the DataExtractor
- an error object

Storing the error object inside the Cursor enables one to use the same
pattern as the std::{io}stream API, where one can blindly perform a
sequence of reads and only check for errors once at the end of the
operation. Similarly to the stream API, as soon as we encounter one
error, all of the subsequent operations are skipped (return default
values) too, even if the would suceed with clear error state. Unlike the
std::stream API (but in line with other llvm APIs), we force the error
state to be checked through usage of llvm::Error.

Reviewers: probinson, dblaikie, JDevlieghere, aprantl, echristo

Subscribers: kristina, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D63713

llvm-svn: 370042

1 files changed, 143 insertions, 0 deletions

diff --git a/llvm/unittests/Support/DataExtractorTest.cpp b/llvm/unittests/Support/DataExtractorTest.cpp
index d1c23cd15e5..d182715d199 100644
--- a/llvm/unittests/Support/DataExtractorTest.cpp
+++ b/llvm/unittests/Support/DataExtractorTest.cpp
@@ -7,6 +7,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/DataExtractor.h"
+#include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 using namespace llvm;
 
@@ -126,4 +127,146 @@ TEST(DataExtractorTest, LEB128_error) {
   EXPECT_EQ(0U, DE.getSLEB128(&Offset));
   EXPECT_EQ(0U, Offset);
 }
+
+TEST(DataExtractorTest, Cursor_tell) {
+  DataExtractor DE(StringRef("AB"), false, 8);
+  DataExtractor::Cursor C(0);
+  // A successful read operation advances the cursor
+  EXPECT_EQ('A', DE.getU8(C));
+  EXPECT_EQ(1u, C.tell());
+
+  // An unsuccessful one doesn't.
+  EXPECT_EQ(0u, DE.getU16(C));
+  EXPECT_EQ(1u, C.tell());
+
+  // And neither do any subsequent operations.
+  EXPECT_EQ(0, DE.getU8(C));
+  EXPECT_EQ(1u, C.tell());
+
+  consumeError(C.takeError());
+}
+
+TEST(DataExtractorTest, Cursor_takeError) {
+  DataExtractor DE(StringRef("AB"), false, 8);
+  DataExtractor::Cursor C(0);
+  // Initially, the cursor is in the "success" state.
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+
+  // It remains "success" after a successful read.
+  EXPECT_EQ('A', DE.getU8(C));
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+
+  // An unsuccessful read sets the error state.
+  EXPECT_EQ(0u, DE.getU32(C));
+  EXPECT_THAT_ERROR(C.takeError(), Failed());
+
+  // Once set the error sticks until explicitly cleared.
+  EXPECT_EQ(0u, DE.getU32(C));
+  EXPECT_EQ(0, DE.getU8(C));
+  EXPECT_THAT_ERROR(C.takeError(), Failed());
+
+  // At which point reads can be succeed again.
+  EXPECT_EQ('B', DE.getU8(C));
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+}
+
+TEST(DataExtractorTest, Cursor_chaining) {
+  DataExtractor DE(StringRef("ABCD"), false, 8);
+  DataExtractor::Cursor C(0);
+
+  // Multiple reads can be chained without trigerring any assertions.
+  EXPECT_EQ('A', DE.getU8(C));
+  EXPECT_EQ('B', DE.getU8(C));
+  EXPECT_EQ('C', DE.getU8(C));
+  EXPECT_EQ('D', DE.getU8(C));
+  // And the error checked at the end.
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+}
+
+#if defined(GTEST_HAS_DEATH_TEST) && defined(_DEBUG)
+TEST(DataExtractorDeathTest, Cursor) {
+  DataExtractor DE(StringRef("AB"), false, 8);
+
+  // Even an unused cursor must be checked for errors:
+  EXPECT_DEATH(DataExtractor::Cursor(0),
+               "Success values must still be checked prior to being destroyed");
+
+  {
+    auto C = std::make_unique<DataExtractor::Cursor>(0);
+    EXPECT_EQ(0u, DE.getU32(*C));
+    // It must also be checked after an unsuccessful operation.
+    // destruction.
+    EXPECT_DEATH(C.reset(), "unexpected end of data");
+    EXPECT_THAT_ERROR(C->takeError(), Failed());
+  }
+  {
+    auto C = std::make_unique<DataExtractor::Cursor>(0);
+    EXPECT_EQ('A', DE.getU8(*C));
+    // Same goes for a successful one.
+    EXPECT_DEATH(
+        C.reset(),
+        "Success values must still be checked prior to being destroyed");
+    EXPECT_THAT_ERROR(C->takeError(), Succeeded());
+  }
+  {
+    auto C = std::make_unique<DataExtractor::Cursor>(0);
+    EXPECT_EQ('A', DE.getU8(*C));
+    EXPECT_EQ(0u, DE.getU32(*C));
+    // Even if a successful operation is followed by an unsuccessful one.
+    EXPECT_DEATH(C.reset(), "unexpected end of data");
+    EXPECT_THAT_ERROR(C->takeError(), Failed());
+  }
+  {
+    auto C = std::make_unique<DataExtractor::Cursor>(0);
+    EXPECT_EQ(0u, DE.getU32(*C));
+    EXPECT_EQ(0, DE.getU8(*C));
+    // Even if an unsuccessful operation is followed by one that would normally
+    // succeed.
+    EXPECT_DEATH(C.reset(), "unexpected end of data");
+    EXPECT_THAT_ERROR(C->takeError(), Failed());
+  }
+}
+#endif
+
+TEST(DataExtractorTest, getU8_vector) {
+  DataExtractor DE(StringRef("AB"), false, 8);
+  DataExtractor::Cursor C(0);
+  SmallVector<uint8_t, 2> S;
+
+  DE.getU8(C, S, 4);
+  EXPECT_THAT_ERROR(C.takeError(), Failed());
+  EXPECT_EQ("", toStringRef(S));
+
+  DE.getU8(C, S, 2);
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+  EXPECT_EQ("AB", toStringRef(S));
+}
+
+TEST(DataExtractorTest, skip) {
+  DataExtractor DE(StringRef("AB"), false, 8);
+  DataExtractor::Cursor C(0);
+
+  DE.skip(C, 4);
+  EXPECT_THAT_ERROR(C.takeError(), Failed());
+  EXPECT_EQ(0u, C.tell());
+
+  DE.skip(C, 2);
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+  EXPECT_EQ(2u, C.tell());
+}
+
+TEST(DataExtractorTest, eof) {
+  DataExtractor DE(StringRef("A"), false, 8);
+  DataExtractor::Cursor C(0);
+
+  EXPECT_FALSE(DE.eof(C));
+
+  EXPECT_EQ(0, DE.getU16(C));
+  EXPECT_FALSE(DE.eof(C));
+  EXPECT_THAT_ERROR(C.takeError(), Failed());
+
+  EXPECT_EQ('A', DE.getU8(C));
+  EXPECT_TRUE(DE.eof(C));
+  EXPECT_THAT_ERROR(C.takeError(), Succeeded());
+}
 }