[Support] Make DataExtractor string functions error-aware

[labath]

Summary

This patch adds an optional Error argument to DataExtractor functions
for string extraction, and makes them behave like other DataExtractor
functions (set the error if extraction fails, don't do anything if the
error is already set).

I have merged the StringRef and c string versions of the functions to
reduce code duplication.

[probinson]

One nit and LGTM.

llvm/lib/Support/DataExtractor.cpp

159

This is the only use of Result; eliminate the var and return StringRef() here.

[MaskRay]

Aside from the StringRef Result nit, in the description, c string versions -> C string versions.

[dblaikie]

Looks good - thanks!

llvm/include/llvm/Support/DataExtractor.h

public:
   ///
   /// @param[in,out] offset_ptr
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     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
   ///     A pointer to the C string value in the data. If the offset
   ///     pointed to by \a offset_ptr is out of bounds, or if the
   ///     offset plus the length of the C string is out of bounds,
   ///     NULL will be returned.
-  const char *getCStr(uint64_t *offset_ptr) const;
+  const char *getCStr(uint64_t *OffsetPtr, Error *Err = nullptr) const {
+    return getCStrRef(OffsetPtr, Err).data();
+  }
+
+  /// Extract a C string from the location given by the cursor. In case of an
+  /// extraction error, or if the cursor is already in an error state, a
+  /// nullptr is returned.
+  const char *getCStr(Cursor &C) const { return getCStrRef(C).data(); }
 
   /// Extract a C string from \a *offset_ptr.
   ///
   /// Returns a StringRef for the C String from the data at the offset
   /// pointed to by \a offset_ptr. A variable length NULL terminated C
   /// string will be extracted and the \a offset_ptr will be
   /// updated with the offset of the byte that follows the NULL
   /// terminator byte.
   ///
   /// \param[in,out] offset_ptr
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     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
   ///     A StringRef for the C string value in the data. If the offset
   ///     pointed to by \a offset_ptr is out of bounds, or if the
   ///     offset plus the length of the C string is out of bounds,
   ///     a default-initialized StringRef will be returned.
-  StringRef getCStrRef(uint64_t *offset_ptr) const;
+  StringRef getCStrRef(uint64_t *OffsetPtr, Error *Err = nullptr) const;
+
+  /// Extract a C string (as a StringRef) from the location given by the cursor.
+  /// In case of an extraction error, or if the cursor is already in an error
+  /// state, a default-initialized StringRef is returned.
+  StringRef getCStrRef(Cursor &C) const {
+    return getCStrRef(&C.Offset, &C.Err);
+  }
 
   /// Extract a fixed length string from \a *OffsetPtr and consume \a Length
   /// bytes.
   ///
   /// Returns a StringRef for the string from the data at the offset
   /// pointed to by \a OffsetPtr. A fixed length C string will be extracted
   /// and the \a OffsetPtr will be advanced by \a Length bytes.
   ///
public:
 
   /// Extract a fixed number of bytes from the location given by the cursor. In
   /// case of an extraction error, or if the cursor is already in an error
   /// state, a default-initialized StringRef is returned.
   StringRef getBytes(Cursor &C, uint64_t Length) {
     return getBytes(&C.Offset, Length, &C.Err);
   }
 
-  StringRef getCStrRef(Cursor &C) const { return getCStrRef(&C.Offset); }
-
   /// Extract an unsigned integer of size \a byte_size from \a
   /// *offset_ptr.
   ///
   /// Extract a single unsigned integer value and update the offset
   /// pointed to by \a offset_ptr. The size of the extracted integer
   /// is specified by the \a byte_size argument. \a byte_size should
   /// have a value greater than or equal to one and less than or equal
   /// to eight since the return value is 64 bits wide. Any

llvm/lib/Support/DataExtractor.cpp

DataExtractor::getSigned(uint64_t *offset_ptr, uint32_t byte_size) const {
   case 4:
     return (int32_t)getU32(offset_ptr);
   case 8:
     return (int64_t)getU64(offset_ptr);
   }
   llvm_unreachable("getSigned unhandled case!");
 }
 
-const char *DataExtractor::getCStr(uint64_t *offset_ptr) const {
-  uint64_t offset = *offset_ptr;
-  StringRef::size_type pos = Data.find('\0', offset);
-  if (pos != StringRef::npos) {
-    *offset_ptr = pos + 1;
-    return Data.data() + offset;
-  }
-  return nullptr;
-}
+StringRef DataExtractor::getCStrRef(uint64_t *OffsetPtr, Error *Err) const {
+  ErrorAsOutParameter ErrAsOut(Err);
+  StringRef Result;
+  if (isError(Err))
+    return Result;
 
-StringRef DataExtractor::getCStrRef(uint64_t *offset_ptr) const {
-  uint64_t Start = *offset_ptr;
+  uint64_t Start = *OffsetPtr;
   StringRef::size_type Pos = Data.find('\0', Start);
   if (Pos != StringRef::npos) {
-    *offset_ptr = Pos + 1;
+    *OffsetPtr = Pos + 1;
     return StringRef(Data.data() + Start, Pos - Start);
   }
+  unexpectedEndReached(Err, Start);
   return StringRef();
 }
 
 StringRef DataExtractor::getFixedLengthString(uint64_t *OffsetPtr,
                                               uint64_t Length,
                                               StringRef TrimChars) const {
   StringRef Bytes(getBytes(OffsetPtr, Length));
   return Bytes.trim(TrimChars);

llvm/unittests/Support/DataExtractorTest.cpp

 //===- llvm/unittest/Support/DataExtractorTest.cpp - DataExtractor tests --===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 using namespace llvm;
 
 namespace {
 
 const char numberData[] = "\x80\x90\xFF\xFF\x80\x00\x00\x00";
-const char stringData[] = "hellohello\0hello";
 const char leb128data[] = "\xA6\x49";
 const char bigleb128data[] = "\xAA\xA9\xFF\xAA\xFF\xAA\xFF\x4A";
 
 TEST(DataExtractorTest, OffsetOverflow) {
   DataExtractor DE(StringRef(numberData, sizeof(numberData)-1), false, 8);
   EXPECT_FALSE(DE.isValidOffsetForDataOfSize(-2U, 5));
 }
 
TEST(DataExtractorTest, SignedNumbers) {
   EXPECT_EQ(-2137980929, DE.getSigned(&offset, 4));
   EXPECT_EQ(4U, offset);
   offset = 0;
   EXPECT_EQ(-9182558167379214336LL, DE.getSigned(&offset, 8));
   EXPECT_EQ(8U, offset);
 }
 
 TEST(DataExtractorTest, Strings) {
+  const char stringData[] = "hellohello\0hello";
   DataExtractor DE(StringRef(stringData, sizeof(stringData)-1), false, 8);
   uint64_t offset = 0;
 
   EXPECT_EQ(stringData, DE.getCStr(&offset));
   EXPECT_EQ(11U, offset);
   EXPECT_EQ(nullptr, DE.getCStr(&offset));
   EXPECT_EQ(11U, offset);
+
+  DataExtractor::Cursor C(0);
+  EXPECT_EQ(stringData, DE.getCStr(C));
+  EXPECT_EQ(11U, C.tell());
+  EXPECT_EQ(nullptr, DE.getCStr(C));
+  EXPECT_EQ(11U, C.tell());
+  EXPECT_THAT_ERROR(C.takeError(),
+                    FailedWithMessage("unexpected end of data at offset 0xb"));
 }
 
 TEST(DataExtractorTest, LEB128) {
   DataExtractor DE(StringRef(leb128data, sizeof(leb128data)-1), false, 8);
   uint64_t offset = 0;
 
   EXPECT_EQ(9382ULL, DE.getULEB128(&offset));
   EXPECT_EQ(2U, offset);