[PATCH] D78113: Fix DWARFDataExtractor::getRelocatedValue near EOF

Wed Apr 15 09:50:46 PDT 2020

dblaikie added inline comments.

================
Comment at: llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp:65
+  EXPECT_THAT_ERROR(C.takeError(),
+                    FailedWithMessage("unexpected end of data at offset 0x4"));
+}
----------------
dblaikie wrote:
> labath wrote:
> > jhenderson wrote:
> > > 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.
> > 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.
> 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)
(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"... )

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78113/new/

https://reviews.llvm.org/D78113