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

[Pavel Labath via Phabricator via llvm-commits]

labath marked an inline comment as done.
labath 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:
> 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"... )
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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78113