[PATCH] D78558: [Support] Make DataExtractor error messages more clear

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/DataExtractor.h:696
+  /// error object to indicate an error.
+  bool prepareRead(uint64_t Offset, uint64_t Size, Error *E) const;
+
----------------
labath wrote:
> jhenderson wrote:
> > Any particular reason this is `protected`, not `private`?
> There is, though it's fairly arbitrary -- it seemed like it could be useful to subclasses like DWARFDataExtractor.
Fair enough. I'll leave it up to you to decide what would be best.


================
Comment at: llvm/lib/Support/DataExtractor.cpp:27
+        ", 0x%" PRIx64 ")",
+        Data.size(), Offset, Offset + Size);
+  return false;
----------------
labath wrote:
> jhenderson wrote:
> > My instinct says that this needs to be Data.size() + Offset. I'm reading some of the changed messages in the tests and getting confused by how they line up with the range Example: "offset 0x48 while reading [0xbadbeef, 0xbadbef7)" - what is the offset 0x48 referring to? I think "offset 0xbadbef6 while reading..." (or whatever the value actually should be) might be clearer.
> Yeah, that one is funny. What happens there is that the very first read starts at a completely bogus offset. In this case that happens because the value of DW_AT_location is (deliberately) corrupted, but it could happen when following other kinds of indirect references (e.g. `DW_FORM_strp`), depending on how exactly is that implemented.
> 
> What would you say to checking for `Data.size() < Offset` and emitting a different error message in that case ("reading offset 0xdeadbeef beyond the end of data at 0x48") ?
That seems like a reasonable improvement. It might mean I'm less likely to get completely thrown by the error message in the future!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78558