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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
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;
----------------
dblaikie wrote:
> 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)
It was a little bit of time ago since I added the `Terminated` flag, but IIRC the reason it needed to exist was because a missing null terminator versus running off the section end would be undetectable without it. I think it becomes redundant once the other planned changes are made, since we no longer need to check the offset when reading, and can keep going until the EOF error or a null terminator is hit.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:207-209
+    FileEntry.DirIdx = DebugLineData.getULEB128(OffsetPtr, &Err);
+    FileEntry.ModTime = DebugLineData.getULEB128(OffsetPtr, &Err);
+    FileEntry.Length = DebugLineData.getULEB128(OffsetPtr, &Err);
----------------
Are there separate test cases for each of these being truncated?


================
Comment at: llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test:80-84
+# NONFATAL:      file_names[  1]:
+# NONFATAL-NEXT:            name: "file1"
+# NONFATAL-NEXT:       dir_index: 0
+# NONFATAL-NEXT:        mod_time: 0x00000000
+# NONFATAL-NEXT:          length: 0x00000000
----------------
Why did this change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77555