[DWARF] Detect extraction errors in DWARFFormValue::extractValue

[labath]

Summary

Although the function had a bool return value, it was always returning
true. Presumably this is because the main type of errors one can
encounter here is running off the end of the stream, and until very
recently, the DataExtractor class made it very difficult to detect that.

The situation has changed now, and we can easily detect errors here,
which this patch does.

[probinson]

Code change LGTM and the tidy-up of block handling in particular is a nice consequence.
I'm not comfortable enough with the test infrastructure used by this patch to click Accept, though.

Re the bool return value, there might have been actual false returns before I worked it over as part of the v5 implementation. But I don't remember. I'm moderately sure i wouldn't have introduced an always-true return value on my own.

[dblaikie]

This change adds a bunch more error handling, if I understand correctly (previously extractValue would've silently succeeded (it could only return true) where now it will fail) - I'd expect some negative test cases demonstrating the new failure modes/error cases being handled? Is there a reason they're not tested?

[aprantl]

I guess this might be unit-testable by supplying too-short DataExtractors?

[labath]

In D77308#1958553, @aprantl wrote:

I guess this might be unit-testable by supplying too-short DataExtractors?

That is what the code DWARFFormValueTest is doing. Maybe it's somewhat over-engineered (I would have just written a sequence of EXPECTS, instead of instantiating a separate test for each form), but I tried to follow the style of the other tests in that file.

@dblaikie: Is that what you had in mind, or were you expecting to see some higher-level tests?

[dblaikie]

In D77308#1963790, @labath wrote:

In D77308#1958553, @aprantl wrote:

I guess this might be unit-testable by supplying too-short DataExtractors?

That is what the code DWARFFormValueTest is doing. Maybe it's somewhat over-engineered (I would have just written a sequence of EXPECTS, instead of instantiating a separate test for each form), but I tried to follow the style of the other tests in that file.

@dblaikie: Is that what you had in mind, or were you expecting to see some higher-level tests?

Nah, that looks good to me - I think I was probably just going change blind with all the reviews.

llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp

if (!Ctx && CU)
     Ctx = &CU->getContext();
   C = Ctx;
   U = CU;
   bool Indirect = false;
   bool IsBlock = false;
   Value.data = nullptr;
   // Read the value for the form into value and follow and DW_FORM_indirect
   // instances we run into
+  Error Err = Error::success();
   do {
     Indirect = false;
     switch (Form) {
     case DW_FORM_addr:
     case DW_FORM_ref_addr: {
       uint16_t Size =
           (Form == DW_FORM_addr) ? FP.AddrSize : FP.getRefAddrByteSize();
-      Value.uval = Data.getRelocatedValue(Size, OffsetPtr, &Value.SectionIndex);
+      Value.uval =
+          Data.getRelocatedValue(Size, OffsetPtr, &Value.SectionIndex, &Err);
       break;
     }
     case DW_FORM_exprloc:
     case DW_FORM_block:
-      Value.uval = Data.getULEB128(OffsetPtr);
+      Value.uval = Data.getULEB128(OffsetPtr, &Err);
       IsBlock = true;
       break;
     case DW_FORM_block1:
-      Value.uval = Data.getU8(OffsetPtr);
+      Value.uval = Data.getU8(OffsetPtr, &Err);
       IsBlock = true;
       break;
     case DW_FORM_block2:
-      Value.uval = Data.getU16(OffsetPtr);
+      Value.uval = Data.getU16(OffsetPtr, &Err);
       IsBlock = true;
       break;
     case DW_FORM_block4:
-      Value.uval = Data.getU32(OffsetPtr);
+      Value.uval = Data.getU32(OffsetPtr, &Err);
       IsBlock = true;
       break;
     case DW_FORM_data1:
     case DW_FORM_ref1:
     case DW_FORM_flag:
     case DW_FORM_strx1:
     case DW_FORM_addrx1:
-      Value.uval = Data.getU8(OffsetPtr);
+      Value.uval = Data.getU8(OffsetPtr, &Err);
       break;
     case DW_FORM_data2:
     case DW_FORM_ref2:
     case DW_FORM_strx2:
     case DW_FORM_addrx2:
-      Value.uval = Data.getU16(OffsetPtr);
+      Value.uval = Data.getU16(OffsetPtr, &Err);
       break;
     case DW_FORM_strx3:
-      Value.uval = Data.getU24(OffsetPtr);
+      Value.uval = Data.getU24(OffsetPtr, &Err);
       break;
     case DW_FORM_data4:
     case DW_FORM_ref4:
     case DW_FORM_ref_sup4:
     case DW_FORM_strx4:
     case DW_FORM_addrx4:
-      Value.uval = Data.getRelocatedValue(4, OffsetPtr);
+      Value.uval = Data.getRelocatedValue(4, OffsetPtr, nullptr, &Err);
       break;
     case DW_FORM_data8:
     case DW_FORM_ref8:
     case DW_FORM_ref_sup8:
-      Value.uval = Data.getRelocatedValue(8, OffsetPtr);
+      Value.uval = Data.getRelocatedValue(8, OffsetPtr, nullptr, &Err);
       break;
     case DW_FORM_data16:
       // Treat this like a 16-byte block.
       Value.uval = 16;
       IsBlock = true;
       break;
     case DW_FORM_sdata:
-      Value.sval = Data.getSLEB128(OffsetPtr);
+      Value.sval = Data.getSLEB128(OffsetPtr, &Err);
       break;
     case DW_FORM_udata:
     case DW_FORM_ref_udata:
     case DW_FORM_rnglistx:
     case DW_FORM_loclistx:
-      Value.uval = Data.getULEB128(OffsetPtr);
+    case DW_FORM_GNU_addr_index:
+    case DW_FORM_GNU_str_index:
+    case DW_FORM_addrx:
+    case DW_FORM_strx:
+      Value.uval = Data.getULEB128(OffsetPtr, &Err);
       break;
     case DW_FORM_string:
-      Value.cstr = Data.getCStr(OffsetPtr);
+      Value.cstr = Data.getCStr(OffsetPtr, &Err);
       break;
     case DW_FORM_indirect:
-      Form = static_cast<dwarf::Form>(Data.getULEB128(OffsetPtr));
+      Form = static_cast<dwarf::Form>(Data.getULEB128(OffsetPtr, &Err));
       Indirect = true;
       break;
     case DW_FORM_strp:
     case DW_FORM_sec_offset:
     case DW_FORM_GNU_ref_alt:
     case DW_FORM_GNU_strp_alt:
     case DW_FORM_line_strp:
     case DW_FORM_strp_sup: {
-      Value.uval =
-          Data.getRelocatedValue(FP.getDwarfOffsetByteSize(), OffsetPtr);
+      Value.uval = Data.getRelocatedValue(FP.getDwarfOffsetByteSize(),
+                                          OffsetPtr, nullptr, &Err);
       break;
     }
     case DW_FORM_flag_present:
       Value.uval = 1;
       break;
     case DW_FORM_ref_sig8:
-      Value.uval = Data.getU64(OffsetPtr);
-      break;
-    case DW_FORM_GNU_addr_index:
-    case DW_FORM_GNU_str_index:
-    case DW_FORM_addrx:
-    case DW_FORM_strx:
-      Value.uval = Data.getULEB128(OffsetPtr);
+      Value.uval = Data.getU64(OffsetPtr, &Err);
       break;
     default:
       // DWARFFormValue::skipValue() will have caught this and caused all
       // DWARF DIEs to fail to be parsed, so this code is not be reachable.
       llvm_unreachable("unsupported form");
     }
-  } while (Indirect);
+  } while (Indirect && !Err);
 
-  if (IsBlock) {
-    StringRef Str = Data.getData().substr(*OffsetPtr, Value.uval);
-    Value.data = nullptr;
-    if (!Str.empty()) {
-      Value.data = Str.bytes_begin();
-      *OffsetPtr += Value.uval;
-    }
-  }
+  if (IsBlock)
+    Value.data = Data.getBytes(OffsetPtr, Value.uval, &Err).bytes_begin();
 
-  return true;
+  return !errorToBool(std::move(Err));
 }
 
 void DWARFFormValue::dumpSectionedAddress(raw_ostream &OS,
                                           DIDumpOptions DumpOpts,
                                           object::SectionedAddress SA) const {
   OS << format("0x%016" PRIx64, SA.Address);
   dumpAddressSection(U->getContext().getDWARFObj(), OS, DumpOpts,
                      SA.SectionIndex);

llvm/unittests/DebugInfo/DWARF/DWARFFormValueTest.cpp

 //===- llvm/unittest/DebugInfo/DWARFFormValueTest.cpp ---------------------===//
 //
 // 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/DebugInfo/DWARF/DWARFFormValue.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/LEB128.h"
 #include "gtest/gtest.h"
 #include <climits>
 using namespace llvm;
 using namespace dwarf;
 
 namespace {
testing::Values(
                   3, true),
         ParamType(DW_FORM_indirect, 4, 4, DWARF32,
                   ArrayRef<uint8_t>(IndirectIndirectEnd,
                                     sizeof(IndirectIndirectEnd)),
                   2, false),
         ParamType(/*Unknown=*/Form(0xff), 4, 4, DWARF32, SampleU32, 0,
                   false)), );
 
+using ErrorParams = std::tuple<Form, std::vector<uint8_t>>;
+struct ExtractValueErrorFixture : public testing::TestWithParam<ErrorParams> {
+  void SetUp() override { std::tie(Fm, InitialData) = GetParam(); }
+
+  Form Fm;
+  ArrayRef<uint8_t> InitialData;
+};
+
+TEST_P(ExtractValueErrorFixture, Test) {
+  SCOPED_TRACE(formatv("Fm = {0}, InitialData = {1}", Fm,
+                       make_range(InitialData.begin(), InitialData.end()))
+                   .str());
+
+  DWARFDataExtractor Data(InitialData, sys::IsLittleEndianHost, 4);
+  DWARFFormValue Form(Fm);
+  uint64_t Offset = 0;
+  EXPECT_FALSE(Form.extractValue(Data, &Offset, {0, 0, DWARF32}));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    ExtractValueErrorParams, ExtractValueErrorFixture,
+    testing::Values(
+        ErrorParams{DW_FORM_ref_addr, {}}, ErrorParams{DW_FORM_block, {}},
+        ErrorParams{DW_FORM_block, {1}}, ErrorParams{DW_FORM_block, {2, 0}},
+        ErrorParams{DW_FORM_block1, {}}, ErrorParams{DW_FORM_block2, {}},
+        ErrorParams{DW_FORM_block4, {}}, ErrorParams{DW_FORM_data1, {}},
+        ErrorParams{DW_FORM_data2, {}}, ErrorParams{DW_FORM_strx3, {}},
+        ErrorParams{DW_FORM_data4, {}}, ErrorParams{DW_FORM_data8, {}},
+        ErrorParams{DW_FORM_data16, {}}, ErrorParams{DW_FORM_sdata, {}},
+        ErrorParams{DW_FORM_udata, {}}, ErrorParams{DW_FORM_string, {}},
+        ErrorParams{DW_FORM_indirect, {}},
+        ErrorParams{DW_FORM_indirect, {DW_FORM_data1}},
+        ErrorParams{DW_FORM_strp_sup, {}},
+        ErrorParams{DW_FORM_ref_sig8, {}}), );
+
 } // end anonymous namespace