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

Author: labath

llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h

@@ -36,13 +36,18 @@ class DWARFDataExtractor : public DataExtractor {
   /// Extracts a value and applies a relocation to the result if
   /// one exists for the given offset.
   uint64_t getRelocatedValue(uint32_t Size, uint64_t *Off,
-                             uint64_t *SectionIndex = nullptr) const;
+                             uint64_t *SectionIndex = nullptr,
+                             Error *Err = nullptr) const;
 
   /// Extracts an address-sized value and applies a relocation to the result if
   /// one exists for the given offset.
   uint64_t getRelocatedAddress(uint64_t *Off, uint64_t *SecIx = nullptr) const {
     return getRelocatedValue(getAddressSize(), Off, SecIx);
   }
+  uint64_t getRelocatedAddress(Cursor &C, uint64_t *SecIx = nullptr) const {
+    return getRelocatedValue(getAddressSize(), &getOffset(C), SecIx,
+                             &getError(C));
+  }
 
   /// Extracts a DWARF-encoded pointer in \p Offset using \p Encoding.
   /// There is a DWARF encoding that uses a PC-relative adjustment.

llvm/include/llvm/Support/DataExtractor.h

@@ -11,6 +11,7 @@
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/DataTypes.h"
+#include "llvm/Support/Error.h"
 
 namespace llvm {
 
@@ -42,6 +43,38 @@ class DataExtractor {
   uint8_t IsLittleEndian;
   uint8_t AddressSize;
 public:
+  /// A class representing a position in a DataExtractor, as well as any error
+  /// encountered during extraction. It enables one to extract a sequence of
+  /// values without error-checking and then checking for errors in bulk at the
+  /// end. The class holds an Error object, so failing to check the result of
+  /// the parse will result in a runtime error. The error flag is sticky and
+  /// will cause all subsequent extraction functions to fail without even
+  /// attempting to parse and without updating the Cursor offset. After clearing
+  /// the error flag, one can again use the Cursor object for parsing.
+  class Cursor {
+    uint64_t Offset;
+    Error Err;
+
+    friend class DataExtractor;
+
+  public:
+    /// Construct a cursor for extraction from the given offset.
+    explicit Cursor(uint64_t Offset) : Offset(Offset), Err(Error::success()) {}
+
+    /// Checks whether the cursor is valid (i.e. no errors were encountered). In
+    /// case of errors, this does not clear the error flag -- one must call
+    /// takeError() instead.
+    explicit operator bool() { return !Err; }
+
+    /// Return the current position of this Cursor. In the error state this is
+    /// the position of the Cursor before the first error was encountered.
+    uint64_t tell() const { return Offset; }
+
+    /// Return error contained inside this Cursor, if any. Clears the internal
+    /// Cursor state.
+    Error takeError() { return std::move(Err); }
+  };
+
   /// Construct with a buffer that is owned by the caller.
   ///
   /// This constructor allows us to use data that is owned by the
@@ -124,10 +157,24 @@ class DataExtractor {
   /// @param[in] byte_size
   ///     The size in byte of the integer to extract.
   ///
+  /// @param[in,out] Err
+  ///     A pointer to an Error object. Upon return the Error object is set to
+  ///     indicate the result (success/failure) of the function. If the Error
+  ///     object is already set when calling this function, no extraction is
+  ///     performed.
+  ///
   /// @return
   ///     The unsigned integer value that was extracted, or zero on
   ///     failure.
-  uint64_t getUnsigned(uint64_t *offset_ptr, uint32_t byte_size) const;
+  uint64_t getUnsigned(uint64_t *offset_ptr, uint32_t byte_size,
+                       Error *Err = nullptr) const;
+
+  /// Extract an unsigned integer of the given size from the location given by
+  /// the cursor. In case of an extraction error, or if the cursor is already in
+  /// an error state, zero is returned.
+  uint64_t getUnsigned(Cursor &C, uint32_t Size) const {
+    return getUnsigned(&C.Offset, Size, &C.Err);
+  }
 
   /// Extract an signed integer of size \a byte_size from \a *offset_ptr.
   ///
@@ -175,6 +222,11 @@ class DataExtractor {
     return getUnsigned(offset_ptr, AddressSize);
   }
 
+  /// Extract a pointer-sized unsigned integer from the location given by the
+  /// cursor. In case of an extraction error, or if the cursor is already in
+  /// an error state, zero is returned.
+  uint64_t getAddress(Cursor &C) const { return getUnsigned(C, AddressSize); }
+
   /// Extract a uint8_t value from \a *offset_ptr.
   ///
   /// Extract a single uint8_t from the binary data at the offset
@@ -187,9 +239,20 @@ class DataExtractor {
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
+  /// @param[in,out] Err
+  ///     A pointer to an Error object. Upon return the Error object is set to
+  ///     indicate the result (success/failure) of the function. If the Error
+  ///     object is already set when calling this function, no extraction is
+  ///     performed.
+  ///
   /// @return
   ///     The extracted uint8_t value.
-  uint8_t getU8(uint64_t *offset_ptr) const;
+  uint8_t getU8(uint64_t *offset_ptr, Error *Err = nullptr) const;
+
+  /// Extract a single uint8_t value from the location given by the cursor. In
+  /// case of an extraction error, or if the cursor is already in an error
+  /// state, zero is returned.
+  uint8_t getU8(Cursor &C) const { return getU8(&C.Offset, &C.Err); }
 
   /// Extract \a count uint8_t values from \a *offset_ptr.
   ///
@@ -216,6 +279,26 @@ class DataExtractor {
   ///     NULL otherise.
   uint8_t *getU8(uint64_t *offset_ptr, uint8_t *dst, uint32_t count) const;
 
+  /// Extract \a Count uint8_t values from the location given by the cursor and
+  /// store them into the destination buffer. In case of an extraction error, or
+  /// if the cursor is already in an error state, a nullptr is returned and the
+  /// destination buffer is left unchanged.
+  uint8_t *getU8(Cursor &C, uint8_t *Dst, uint32_t Count) const;
+
+  /// Extract \a Count uint8_t values from the location given by the cursor and
+  /// store them into the destination vector. The vector is resized to fit the
+  /// extracted data. In case of an extraction error, or if the cursor is
+  /// already in an error state, the destination vector is left unchanged and
+  /// cursor is placed into an error state.
+  void getU8(Cursor &C, SmallVectorImpl<uint8_t> &Dst, uint32_t Count) const {
+    if (isValidOffsetForDataOfSize(C.Offset, Count))
+      Dst.resize(Count);
+
+    // This relies on the fact that getU8 will not attempt to write to the
+    // buffer if isValidOffsetForDataOfSize(C.Offset, Count) is false.
+    getU8(C, Dst.data(), Count);
+  }
+
   //------------------------------------------------------------------
   /// Extract a uint16_t value from \a *offset_ptr.
   ///
@@ -229,10 +312,21 @@ class DataExtractor {
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
+  /// @param[in,out] Err
+  ///     A pointer to an Error object. Upon return the Error object is set to
+  ///     indicate the result (success/failure) of the function. If the Error
+  ///     object is already set when calling this function, no extraction is
+  ///     performed.
+  ///
   /// @return
   ///     The extracted uint16_t value.
   //------------------------------------------------------------------
-  uint16_t getU16(uint64_t *offset_ptr) const;
+  uint16_t getU16(uint64_t *offset_ptr, Error *Err = nullptr) const;
+
+  /// Extract a single uint16_t value from the location given by the cursor. In
+  /// case of an extraction error, or if the cursor is already in an error
+  /// state, zero is returned.
+  uint16_t getU16(Cursor &C) const { return getU16(&C.Offset, &C.Err); }
 
   /// Extract \a count uint16_t values from \a *offset_ptr.
   ///
@@ -288,9 +382,20 @@ class DataExtractor {
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
+  /// @param[in,out] Err
+  ///     A pointer to an Error object. Upon return the Error object is set to
+  ///     indicate the result (success/failure) of the function. If the Error
+  ///     object is already set when calling this function, no extraction is
+  ///     performed.
+  ///
   /// @return
   ///     The extracted uint32_t value.
-  uint32_t getU32(uint64_t *offset_ptr) const;
+  uint32_t getU32(uint64_t *offset_ptr, Error *Err = nullptr) const;
+
+  /// Extract a single uint32_t value from the location given by the cursor. In
+  /// case of an extraction error, or if the cursor is already in an error
+  /// state, zero is returned.
+  uint32_t getU32(Cursor &C) const { return getU32(&C.Offset, &C.Err); }
 
   /// Extract \a count uint32_t values from \a *offset_ptr.
   ///
@@ -329,9 +434,20 @@ class DataExtractor {
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
+  /// @param[in,out] Err
+  ///     A pointer to an Error object. Upon return the Error object is set to
+  ///     indicate the result (success/failure) of the function. If the Error
+  ///     object is already set when calling this function, no extraction is
+  ///     performed.
+  ///
   /// @return
   ///     The extracted uint64_t value.
-  uint64_t getU64(uint64_t *offset_ptr) const;
+  uint64_t getU64(uint64_t *offset_ptr, Error *Err = nullptr) const;
+
+  /// Extract a single uint64_t value from the location given by the cursor. In
+  /// case of an extraction error, or if the cursor is already in an error
+  /// state, zero is returned.
+  uint64_t getU64(Cursor &C) const { return getU64(&C.Offset, &C.Err); }
 
   /// Extract \a count uint64_t values from \a *offset_ptr.
   ///
@@ -390,9 +506,30 @@ class DataExtractor {
   ///     enough bytes to extract this value, the offset will be left
   ///     unmodified.
   ///
+  /// @param[in,out] Err
+  ///     A pointer to an Error object. Upon return the Error object is set to
+  ///     indicate the result (success/failure) of the function. If the Error
+  ///     object is already set when calling this function, no extraction is
+  ///     performed.
+  ///
   /// @return
   ///     The extracted unsigned integer value.
-  uint64_t getULEB128(uint64_t *offset_ptr) const;
+  uint64_t getULEB128(uint64_t *offset_ptr, llvm::Error *Err = nullptr) const;
+
+  /// Extract an unsigned ULEB128 value from the location given by the cursor.
+  /// In case of an extraction error, or if the cursor is already in an error
+  /// state, zero is returned.
+  uint64_t getULEB128(Cursor &C) const { return getULEB128(&C.Offset, &C.Err); }
+
+  /// Advance the Cursor position by the given number of bytes. No-op if the
+  /// cursor is in an error state.
+  void skip(Cursor &C, uint64_t Length) const;
+
+  /// Return true iff the cursor is at the end of the buffer, regardless of the
+  /// error state of the cursor. The only way both eof and error states can be
+  /// true is if one attempts a read while the cursor is at the very end of the
+  /// data buffer.
+  bool eof(const Cursor &C) const { return Data.size() == C.Offset; }
 
   /// Test the validity of \a offset.
   ///
@@ -420,6 +557,12 @@ class DataExtractor {
   bool isValidOffsetForAddress(uint64_t offset) const {
     return isValidOffsetForDataOfSize(offset, AddressSize);
   }
+
+protected:
+  // Make it possible for subclasses to access these fields without making them
+  // public.
+  static uint64_t &getOffset(Cursor &C) { return C.Offset; }
+  static Error &getError(Cursor &C) { return C.Err; }
 };
 
 } // namespace llvm

llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp

@@ -13,13 +13,14 @@
 using namespace llvm;
 
 uint64_t DWARFDataExtractor::getRelocatedValue(uint32_t Size, uint64_t *Off,
-                                               uint64_t *SecNdx) const {
+                                               uint64_t *SecNdx,
+                                               Error *Err) const {
   if (SecNdx)
     *SecNdx = object::SectionedAddress::UndefSection;
   if (!Section)
-    return getUnsigned(Off, Size);
+    return getUnsigned(Off, Size, Err);
   Optional<RelocAddrEntry> E = Obj->find(*Section, *Off);
-  uint64_t A = getUnsigned(Off, Size);
+  uint64_t A = getUnsigned(Off, Size, Err);
   if (!E)
     return A;
   if (SecNdx)