Fix DWARFDataExtractor::getRelocatedValue near EOF

[labath]

Summary

If we have an (invalid) relocation which relocates bytes which partially
lie outside the range of the relocated section, the getRelocatedValue
would return confusing results. It would first read zero (because that's
what the underlying DataExtractor api does for out-of-bounds reads, and
then relocate that zero anyway.

A more appropriate behavior is to return zero straight away. This is
what this patch does.

[labath]

• add const & to Twine

[dblaikie]

Sounds OK (some slight concern about whether we should have any defined behavior for return values when an Error is produced, but I can see the value in it for dumping, etc)

[jhenderson]

LGTM, although there's a typo in your summary (a missing closing ')') which should be fixed if you are using it as the commit message.

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

65

Don't think it needs fixing in this commit, since it's not really related to the change, but an offset of 0x4 here is confusing, given that the end of data is at 0x6.

[labath]

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

65

Maybe it's because I'm aware of the implementation, but I don't find that confusing -- we were at offset 4, tried to read 4 bytes, and got an EOF. The fact that the read would have succeeded if we tried to read 2 bytes instead does not give me pause.

[dblaikie]

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

65

Yeah, I'd go with @jhenderson here that it's somewhat unintuitive - could potentially be useful to show all three values: read offset, length of read, offset that's the end of the data, or something like that. (maybe start offset, end offset would be better), eg: unexpected end of data at offset 0x6 while reading [0x4, 0x8)

[dblaikie]

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

65

(but I'm not sure how much value the [0x4, 0x8) is - given it's just a fact about the field size we happen to be reading - even though at a broader scope we might be trying to read many more bytes, so is knowing the length of this particular field especially valuable/important/actionable? (fixing the data to have length 0x8 isn't necessarily going to fix this error, for instance - because it'll just try to read the next field and fail there)

Might help diagnose run-away things, where, say, a length-prefixed string read where it would print [0xN, 0xHUGE) and you'd be like "well that's not reasonable"... )

[labath]

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

65

Fair enough, I've created D78558 to implement something like that. That version print both the EOF offset as well as the currently attempted read range.

Assuming the user does the usual (recommended) thing of reading a bunch of data in sequence (with the same offset+error / cursor objects) and then checking for errors in bulk, we could theoretically try to be fancy, detect this, and extend the range of the error to cover the data that would be read this way. But that could be too fancy...

[jhenderson]

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

65

But that could be too fancy...

Yeah, I think that might be going too far.

llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp

 
 uint64_t DWARFDataExtractor::getRelocatedValue(uint32_t Size, uint64_t *Off,
                                                uint64_t *SecNdx,
                                                Error *Err) const {
   if (SecNdx)
     *SecNdx = object::SectionedAddress::UndefSection;
   if (!Section)
     return getUnsigned(Off, Size, Err);
+
+  ErrorAsOutParameter ErrAsOut(Err);
   Optional<RelocAddrEntry> E = Obj->find(*Section, *Off);
   uint64_t A = getUnsigned(Off, Size, Err);
-  if (!E)
+  if (!E || (Err && *Err))
     return A;
   if (SecNdx)
     *SecNdx = E->SectionIndex;
   uint64_t R = E->Resolver(E->Reloc, E->SymbolValue, A);
   if (E->Reloc2)
     R = E->Resolver(*E->Reloc2, E->SymbolValue2, R);
   return R;
 }

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp

 //===- DWARFDataExtractorTest.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/DWARFDataExtractor.h"
+#include "llvm/DebugInfo/DWARF/DWARFContext.h"
+#include "llvm/Object/ObjectFile.h"
+#include "llvm/ObjectYAML/yaml2obj.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 namespace {
 
+TEST(DWARFDataExtractorTest, getRelocatedValue) {
+  StringRef Yaml = R"(
+!ELF
+FileHeader:
+  Class:    ELFCLASS32
+  Data:     ELFDATA2LSB
+  Type:     ET_REL
+  Machine:  EM_386
+Sections:
+  - Name:     .text
+    Type:     SHT_PROGBITS
+    Size:     0x80
+  - Name:     .debug_line
+    Type:     SHT_PROGBITS
+    Content:  '000000000000'
+  - Name:     .rel.debug_line
+    Type:     SHT_REL
+    Info:     .debug_line
+    Relocations:
+      - Offset:   0
+        Symbol:   f
+        Type:     R_386_32
+      - Offset:   4
+        Symbol:   f
+        Type:     R_386_32
+Symbols:
+  - Name:     f
+    Type:     STT_SECTION
+    Section:  .text
+    Value:    0x42
+)";
+  SmallString<0> Storage;
+  std::unique_ptr<object::ObjectFile> Obj = yaml::yaml2ObjectFile(
+      Storage, Yaml, [](const Twine &Err) { errs() << Err; });
+  ASSERT_TRUE(Obj);
+  std::unique_ptr<DWARFContext> Ctx = DWARFContext::create(*Obj);
+  const DWARFObject &DObj = Ctx->getDWARFObj();
+  ASSERT_EQ(6u, DObj.getLineSection().Data.size());
+
+  DWARFDataExtractor Data(DObj, DObj.getLineSection(), Obj->isLittleEndian(),
+                          Obj->getBytesInAddress());
+  DataExtractor::Cursor C(0);
+  EXPECT_EQ(0x42u, Data.getRelocatedAddress(C));
+  EXPECT_EQ(0u, Data.getRelocatedAddress(C));
+  EXPECT_THAT_ERROR(C.takeError(),
+                    FailedWithMessage("unexpected end of data at offset 0x4"));
+}
+
 TEST(DWARFDataExtractorTest, getInitialLength) {
   auto GetWithError = [](ArrayRef<uint8_t> Bytes)
       -> Expected<std::tuple<uint64_t, dwarf::DwarfFormat, uint64_t>> {
     DWARFDataExtractor Data(Bytes, /*IsLittleEndian=*/false, /*AddressSize=*/8);
     DWARFDataExtractor::Cursor C(0);
     uint64_t Length;
     dwarf::DwarfFormat Format;
     std::tie(Length, Format) = Data.getInitialLength(C);