[PATCH] D77555: [DWARFDebugLine] Check for errors when parsing v2 file/dir lists

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Looks good - some of the suggestions I made don't hold or hold less with one of your other changes applied (truncated data extractors) - but might still be worth considering even combined with that.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:173-192
+  Error Err = Error::success();
+  while (true) {
+    StringRef S = DebugLineData.getCStrRef(OffsetPtr, &Err);
+    if (Err || *OffsetPtr > EndPrologueOffset)
+      break;
     if (S.empty()) {
       Terminated = true;
----------------
I realize this is the way the existing code is - so feel free to ignore this, do it as a follow-up or precommit, or roll it is as you see fit: The "Terminated" flag here seems to complicate this code compared to doing the error handling and returning immediately in the one way out of the loop above where Terminated would be false (this would also allow you to narrow the scope of the the "Error" variable to inside the loop)


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:197-199
+    if (Err || *OffsetPtr > EndPrologueOffset)
+      break;
     if (Name.empty()) {
----------------
If you changed this code to be:
```
if (!Err && Name.empty()) {
  Terminated = true;
  break;
}
```

Would that let the later error handling catch handle both errors - then you could do the same as above, and roll the error handling in tighter without needing the "Terminated" flag, etc?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:427-429
+    // Reset the offset to the end of the prologue to prevent the length check
+    // below from executing.
+    *OffsetPtr = EndPrologueOffset;
----------------
Rather than doing this, could we return early in these cases (eg: have ReportInvalidDirFileTable return Error::Success and have the two calls be "return ReportInvalid..." perhaps? or just two separate statements, (ReportInvalid + return Success)) so the follow-on check is never run, rather than having to create a fake state to avoid it reporting?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:441
 
   if (*OffsetPtr != EndPrologueOffset) {
     RecoverableErrorHandler(createStringError(
----------------
Does this only catch cases where the offset is < the end prologue offset now? If so, it might be nice to assert that it's not > and write the test as < and rephrase the error message to be more specific about the mismatch being too short, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77555