[PATCH] D80797: [DebugInfo] Check for errors when reading data for extended opcode

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 2 inline comments as done.
jhenderson added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:856
           FileNameEntry FileEntry;
-          const char *Name = TableData.getCStr(OffsetPtr);
+          const char *Name = TableData.getCStr(OffsetPtr, &Err);
           FileEntry.Name =
----------------
MaskRay wrote:
> Does it help clarity using `DataExtractor::Cursor` and `StringRef getCStrRef(Cursor &C) const`?
Maybe. I'm not particularly familiar with the `Cursor` usage yet, so I'll spend some time experimenting with that to see if it makes sense here.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:877
       default:
-        if (OS)
+        if (!Err && OS)
           *OS << format("Unrecognized extended op 0x%02.02" PRIx8, SubOpcode)
----------------
labath wrote:
> aprantl wrote:
> > This seems oddly stricter than before. Is the `!Err` condition necessary?
> I'm guessing this is because  in the erroneous state the `SubOpcode` and `Len` won't contain real data, but only zeroes, and so a better course of action is to print the error about reaching the end-of-file (contribution) message.
> 
> Though if that's the case, then it does bring up the question of why is this the only statement guarded by `!Err` -- this argument should apply to other print statements  too...
@labath is right in that if there's an error, the "extended opcode" won't be a known value (it'll be another zero), so printing its value doesn't make much sense. Although I guess printing the length might not be a terrible idea. I'll give that some thought.

Re. consistency: I ran out of time to finish uploading a series of patches. One of my later patches does the check for the othe opcodes to stop them printing their arguments. I'll get back to that in the next day or two, and it should make everything consistent. Although now you point that out, maybe I should move this bit into that patch. I'll take a look to see if it makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80797