From b1f29cec251188a594148f7a53d063281d4ef155 Mon Sep 17 00:00:00 2001 From: Pavel Labath Date: Tue, 27 Aug 2019 11:24:08 +0000 Subject: 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 --- llvm/unittests/Support/DataExtractorTest.cpp | 143 +++++++++++++++++++++++++++ 1 file changed, 143 insertions(+) (limited to 'llvm/unittests/Support/DataExtractorTest.cpp') 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(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(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(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(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 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()); +} } -- cgit v1.2.3