[DWARF] Add more error handling to debug line parser.

[JDevlieghere]

Summary

This patch exnteds the error handling in the debug line parser to get rid of the existing MD5 assertion. I want to reuse the debug line parser from LLVM in LLDB where we cannot crash on invalid input.

[JDevlieghere]

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

357

Looks like clang-format needs a little help here. I'll reflow this.

[JDevlieghere]

• Reflow string in unit test
• Update debug_line_invalid.test

[JDevlieghere]

Friendly ping

[dblaikie]

Looks like this code already had rudimentary error handling along this codepath where the assert is (the function returns empty, which returns false, which becomes an error further up, I think?) - and adding more descriptive/precise error handling along this path is probably good - but probably also means more testing is required to demonstrate all the new error results/messages/codepaths?

[JDevlieghere]

I'm not sure if there's a better way to test this without checking in a binary. I ended up modifying MCDwarf to emit a data8 instead of a data16 form for the MD5 hash to trigger this code path.

[dblaikie]

I was rather hoping for test coverage for all the new error messages this change introduced - is that unrealistic/impractical?

Yeah, the line table is especially tricky to hand-craft compared to checking in an object file. I think it technically can still be hand-crafted assembly (no line directives, etc, just a debug_line section with raw byte (etc) directives) - might be plausible & make it clearer what the input is? (checked in assembly, assembled with llvm-mc then run through llvm-dwarfdump to test the parsing)

[probinson]

In D64544#1589853, @dblaikie wrote:

I was rather hoping for test coverage for all the new error messages this change introduced - is that unrealistic/impractical?

Yeah, the line table is especially tricky to hand-craft compared to checking in an object file. I think it technically can still be hand-crafted assembly (no line directives, etc, just a debug_line section with raw byte (etc) directives) - might be plausible & make it clearer what the input is? (checked in assembly, assembled with llvm-mc then run through llvm-dwarfdump to test the parsing)

Hear hear. It looks like this patch is all about diagnosing issues in the v5 prologue/file-table, and it should be relatively easy to construct bogus examples in assembler. You don't need a line-number program at all. There ought to be examples of *valid* v5 line-table headers lying around, I know I did all the dumper work with assembler tests before I ever generated any v5 headers.

[JDevlieghere]

In D64544#1589861, @probinson wrote:

In D64544#1589853, @dblaikie wrote:

I was rather hoping for test coverage for all the new error messages this change introduced - is that unrealistic/impractical?

Oh, sure, I was focussed on the MD5 one because that's the only one that's "new". I'll make sure the other ones are covered too.

Yeah, the line table is especially tricky to hand-craft compared to checking in an object file. I think it technically can still be hand-crafted assembly (no line directives, etc, just a debug_line section with raw byte (etc) directives) - might be plausible & make it clearer what the input is? (checked in assembly, assembled with llvm-mc then run through llvm-dwarfdump to test the parsing)

Hear hear. It looks like this patch is all about diagnosing issues in the v5 prologue/file-table, and it should be relatively easy to construct bogus examples in assembler. You don't need a line-number program at all. There ought to be examples of *valid* v5 line-table headers lying around, I know I did all the dumper work with assembler tests before I ever generated any v5 headers.

How would you be able to emit a wrong form value for the MD5 hash from assembly? The directive (which I think you came up with?) is just md5 [data]? Deciding which form to emit is hard-coded in MCDwarf.

[probinson]

In D64544#1589926, @JDevlieghere wrote:

How would you be able to emit a wrong form value for the MD5 hash from assembly? The directive (which I think you came up with?) is just md5 [data]? Deciding which form to emit is hard-coded in MCDwarf.

Only if you use the .file directive to emit it. As David said, you can put whatever you want into a hand-coded .debug_line section.
See for example llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s

[dblaikie]

In D64544#1589926, @JDevlieghere wrote:

In D64544#1589861, @probinson wrote:

In D64544#1589853, @dblaikie wrote:

I was rather hoping for test coverage for all the new error messages this change introduced - is that unrealistic/impractical?

Oh, sure, I was focussed on the MD5 one because that's the only one that's "new". I'll make sure the other ones are covered too.

Yeah, the line table is especially tricky to hand-craft compared to checking in an object file. I think it technically can still be hand-crafted assembly (no line directives, etc, just a debug_line section with raw byte (etc) directives) - might be plausible & make it clearer what the input is? (checked in assembly, assembled with llvm-mc then run through llvm-dwarfdump to test the parsing)

Hear hear. It looks like this patch is all about diagnosing issues in the v5 prologue/file-table, and it should be relatively easy to construct bogus examples in assembler. You don't need a line-number program at all. There ought to be examples of *valid* v5 line-table headers lying around, I know I did all the dumper work with assembler tests before I ever generated any v5 headers.

How would you be able to emit a wrong form value for the MD5 hash from assembly? The directive (which I think you came up with?) is just md5 [data]? Deciding which form to emit is hard-coded in MCDwarf.

Not using line directives to create the line table - using normal/raw assembly (the same way we have assembly for debug_info sections for some tests) to create the bytes in the debug_line section. Does that make sense?

[JDevlieghere]

Got it, thanks! I'll give that a shot :-)

[JDevlieghere]

Add more tests!

[dblaikie]

Generally looks good (optional thoughT: maybe the .test file and the .s file could be merged (so the CHECK lines could sit near the assembly they're checking, would make it easier to eyeball whether the tests are testing the right thing, etc)

[JDevlieghere]

In D64544#1596538, @dblaikie wrote:

Generally looks good (optional thoughT: maybe the .test file and the .s file could be merged (so the CHECK lines could sit near the assembly they're checking, would make it easier to eyeball whether the tests are testing the right thing, etc)

I agree with the general idea, but given that this test is checking the same prefixes with two inputs (debug_line_reserved_length.s and debug_line_malformed.s) I think it would just increate the complexity within the test.

[jhenderson]

Hi @JDevlieghere, thanks for doing this! If you do any more new error handling for the debug line parser, could you subscibe me too, please? We've got a number of local patches to add additional error handling which I've been meaning to put up for review, but simply haven't had the time (and don't foresee having the time in the near future). If you subscribe me, I could probably provide you with bits that might be useful though, as there's a reasonable chance we already have the test case for example! It'll also help with our merging in of your change into our downstream repo.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

while (*OffsetPtr < EndPrologueOffset) {
     FileNames.push_back(FileEntry);
   }
 
   ContentTypes.HasModTime = true;
   ContentTypes.HasLength = true;
 }
 
 // Parse v5 directory/file entry content descriptions.
-// Returns the descriptors, or an empty vector if we did not find a path or
-// ran off the end of the prologue.
-static ContentDescriptors
-parseV5EntryFormat(const DWARFDataExtractor &DebugLineData, uint32_t
-    *OffsetPtr, uint64_t EndPrologueOffset, DWARFDebugLine::ContentTypeTracker
-    *ContentTypes) {
+// Returns the descriptors, or an error if we did not find a path or ran off
+// the end of the prologue.
+static llvm::Expected<ContentDescriptors>
+parseV5EntryFormat(const DWARFDataExtractor &DebugLineData, uint32_t *OffsetPtr,
+                   uint64_t EndPrologueOffset,
+                   DWARFDebugLine::ContentTypeTracker *ContentTypes) {
   ContentDescriptors Descriptors;
   int FormatCount = DebugLineData.getU8(OffsetPtr);
   bool HasPath = false;
   for (int I = 0; I != FormatCount; ++I) {
     if (*OffsetPtr >= EndPrologueOffset)
-      return ContentDescriptors();
+      return createStringError(
+          errc::invalid_argument,
+          "failed to parse entry content descriptions at offset "
+          "0x%8.8" PRIx64
+          " because offset extends beyond the prologue end at offset "
+          "0x%8.8" PRIx64,
+          OffsetPtr, EndPrologueOffset);
     ContentDescriptor Descriptor;
     Descriptor.Type =
       dwarf::LineNumberEntryFormat(DebugLineData.getULEB128(OffsetPtr));
     Descriptor.Form = dwarf::Form(DebugLineData.getULEB128(OffsetPtr));
     if (Descriptor.Type == dwarf::DW_LNCT_path)
       HasPath = true;
     if (ContentTypes)
       ContentTypes->trackContentType(Descriptor.Type);
     Descriptors.push_back(Descriptor);
   }
-  return HasPath ? Descriptors : ContentDescriptors();
+
+  if (!HasPath)
+    return createStringError(errc::invalid_argument,
+                             "failed to parse entry content descriptions"
+                             " because no path was found");
+  return Descriptors;
 }
 
-static bool
+static Error
 parseV5DirFileTables(const DWARFDataExtractor &DebugLineData,
                      uint32_t *OffsetPtr, uint64_t EndPrologueOffset,
                      const dwarf::FormParams &FormParams,
                      const DWARFContext &Ctx, const DWARFUnit *U,
                      DWARFDebugLine::ContentTypeTracker &ContentTypes,
                      std::vector<DWARFFormValue> &IncludeDirectories,
                      std::vector<DWARFDebugLine::FileNameEntry> &FileNames) {
   // Get the directory entry description.
-  ContentDescriptors DirDescriptors =
+  llvm::Expected<ContentDescriptors> DirDescriptors =
       parseV5EntryFormat(DebugLineData, OffsetPtr, EndPrologueOffset, nullptr);
-  if (DirDescriptors.empty())
-    return false;
+  if (!DirDescriptors)
+    return DirDescriptors.takeError();
 
   // Get the directory entries, according to the format described above.
   int DirEntryCount = DebugLineData.getU8(OffsetPtr);
   for (int I = 0; I != DirEntryCount; ++I) {
     if (*OffsetPtr >= EndPrologueOffset)
-      return false;
-    for (auto Descriptor : DirDescriptors) {
+      return createStringError(
+          errc::invalid_argument,
+          "failed to parse directory entry at offset "
+          "0x%8.8" PRIx64
+          " because offset extends beyond the prologue end at offset "
+          "0x%8.8" PRIx64,
+          OffsetPtr, EndPrologueOffset);
+    for (auto Descriptor : *DirDescriptors) {
       DWARFFormValue Value(Descriptor.Form);
       switch (Descriptor.Type) {
       case DW_LNCT_path:
         if (!Value.extractValue(DebugLineData, OffsetPtr, FormParams, &Ctx, U))
-          return false;
+          return createStringError(errc::invalid_argument,
+                                   "failed to parse directory entry because "
+                                   "extracting the form value failed.");
         IncludeDirectories.push_back(Value);
         break;
       default:
         if (!Value.skipValue(DebugLineData, OffsetPtr, FormParams))
-          return false;
+          return createStringError(errc::invalid_argument,
+                                   "failed to parse directory entry because "
+                                   "skipping the form value failed.");
       }
     }
   }
 
   // Get the file entry description.
-  ContentDescriptors FileDescriptors =
-      parseV5EntryFormat(DebugLineData, OffsetPtr, EndPrologueOffset,
-          &ContentTypes);
-  if (FileDescriptors.empty())
-    return false;
+  llvm::Expected<ContentDescriptors> FileDescriptors = parseV5EntryFormat(
+      DebugLineData, OffsetPtr, EndPrologueOffset, &ContentTypes);
+  if (!FileDescriptors)
+    return FileDescriptors.takeError();
 
   // Get the file entries, according to the format described above.
   int FileEntryCount = DebugLineData.getU8(OffsetPtr);
   for (int I = 0; I != FileEntryCount; ++I) {
     if (*OffsetPtr >= EndPrologueOffset)
-      return false;
+      return createStringError(
+          errc::invalid_argument,
+          "failed to parse file entry at offset "
+          "0x%8.8" PRIx64
+          " because offset extends beyond the prologue end at offset "
+          "0x%8.8" PRIx64,
+          OffsetPtr, EndPrologueOffset);
     DWARFDebugLine::FileNameEntry FileEntry;
-    for (auto Descriptor : FileDescriptors) {
+    for (auto Descriptor : *FileDescriptors) {
       DWARFFormValue Value(Descriptor.Form);
       if (!Value.extractValue(DebugLineData, OffsetPtr, FormParams, &Ctx, U))
-        return false;
+        return createStringError(errc::invalid_argument,
+                                 "failed to parse file entry because "
+                                 "extracting the form value failed.");
       switch (Descriptor.Type) {
       case DW_LNCT_path:
         FileEntry.Name = Value;
         break;
       case DW_LNCT_LLVM_source:
         FileEntry.Source = Value;
         break;
       case DW_LNCT_directory_index:
         FileEntry.DirIdx = Value.getAsUnsignedConstant().getValue();
         break;
       case DW_LNCT_timestamp:
         FileEntry.ModTime = Value.getAsUnsignedConstant().getValue();
         break;
       case DW_LNCT_size:
         FileEntry.Length = Value.getAsUnsignedConstant().getValue();
         break;
       case DW_LNCT_MD5:
-        assert(Value.getAsBlock().getValue().size() == 16);
+        return createStringError(
+            errc::invalid_argument,
+            "failed to parse file entry because the MD5 hash is invalid");
         std::uninitialized_copy_n(Value.getAsBlock().getValue().begin(), 16,
                                   FileEntry.Checksum.Bytes.begin());
         break;
       default:
         break;
       }
     }
     FileNames.push_back(FileEntry);
   }
-  return true;
+  return Error::success();
 }
 
 Error DWARFDebugLine::Prologue::parse(const DWARFDataExtractor &DebugLineData,
                                       uint32_t *OffsetPtr,
                                       const DWARFContext &Ctx,
                                       const DWARFUnit *U) {
   const uint64_t PrologueOffset = *OffsetPtr;
 
Error DWARFDebugLine::Prologue::parse(const DWARFDataExtractor &DebugLineData,
 
   StandardOpcodeLengths.reserve(OpcodeBase - 1);
   for (uint32_t I = 1; I < OpcodeBase; ++I) {
     uint8_t OpLen = DebugLineData.getU8(OffsetPtr);
     StandardOpcodeLengths.push_back(OpLen);
   }
 
   if (getVersion() >= 5) {
-    if (!parseV5DirFileTables(DebugLineData, OffsetPtr, EndPrologueOffset,
-                              FormParams, Ctx, U, ContentTypes,
-                              IncludeDirectories, FileNames)) {
-      return createStringError(errc::invalid_argument,
+    if (Error e = parseV5DirFileTables(
+            DebugLineData, OffsetPtr, EndPrologueOffset, FormParams, Ctx, U,
+            ContentTypes, IncludeDirectories, FileNames)) {
+      return joinErrors(
+          createStringError(
+              errc::invalid_argument,
               "parsing line table prologue at 0x%8.8" PRIx64
               " found an invalid directory or file table description at"
               " 0x%8.8" PRIx64,
-          PrologueOffset, (uint64_t)*OffsetPtr);
+              PrologueOffset, (uint64_t)*OffsetPtr),
+          std::move(e));
     }
   } else
     parseV2DirFileTables(DebugLineData, OffsetPtr, EndPrologueOffset,
                          ContentTypes, IncludeDirectories, FileNames);
 
   if (*OffsetPtr != EndPrologueOffset)
     return createStringError(errc::invalid_argument,
                        "parsing line table prologue at 0x%8.8" PRIx64

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

void checkGetOrParseLineTableEmitsError(StringRef ExpectedMsg,
     auto ExpectedLineTable = Line.getOrParseLineTable(
         LineData, Offset, *Context, nullptr, RecordRecoverable);
     EXPECT_FALSE(ExpectedLineTable);
     EXPECT_FALSE(Recoverable);
 
     checkError(ExpectedMsg, ExpectedLineTable.takeError());
   }
 
+  void checkGetOrParseLineTableEmitsError(ArrayRef<StringRef> ExpectedMsgs,
+                                          uint64_t Offset = 0) {
+    auto ExpectedLineTable = Line.getOrParseLineTable(
+        LineData, Offset, *Context, nullptr, RecordRecoverable);
+    EXPECT_FALSE(ExpectedLineTable);
+    EXPECT_FALSE(Recoverable);
+
+    checkError(ExpectedMsgs, ExpectedLineTable.takeError());
+  }
+
   std::unique_ptr<Generator> Gen;
   std::unique_ptr<DWARFContext> Context;
   DWARFDataExtractor LineData;
   DWARFDebugLine Line;
   Error Recoverable;
   std::function<void(Error)> RecordRecoverable;
   Error Unrecoverable;
   std::function<void(Error)> RecordUnrecoverable;
LT.setCustomPrologue({
       {0, LineTable::ULEB},  // directories count
       {0, LineTable::Byte},  // file name entry format count
       {0, LineTable::ULEB}   // file name entry count
   });
 
   generate();
 
   checkGetOrParseLineTableEmitsError(
-      "parsing line table prologue at 0x00000000 found an invalid directory or "
-      "file table description at 0x00000014");
+      {"parsing line table prologue at 0x00000000 found an invalid directory "
+       "or "
+       "file table description at 0x00000014",
+       "failed to parse entry content descriptions because no path was found"});
 }
 
 TEST_P(DebugLineParameterisedFixture, ErrorForTooLargePrologueLength) {
   if (!setupGenerator(Version))
     return;
 
   SCOPED_TRACE("Checking Version " + std::to_string(Version) + ", Format " +
                (Format == DWARF64 ? "DWARF64" : "DWARF32"));