[DWARFDebugLine] Check for (EOF) errors when parsing v5 content descriptors

[labath]

Summary

Without that we could be silently reading zeroes, as that's the default
DataExtractor behavior. The entire parse would still most likely fail,
but it would do that with a seemingly unrelated/nonsensical error
message.

[dblaikie]

Looks good - thanks!

[jhenderson]

I feel like this is slightly undertested: at the moment, you only have one new test case, but there are two other places where a warning can be detected, one of which causes a different code path too (the early out from the loop). I think all three need testing.

llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s

2

This is probably fine as is, but did you consider extending debug_line_invalid.s (and its corresponding input file) or the gtest unit tests to test this?

37

Could you add a comment please, saying why this entry is considered too short?

[labath]

• add more test cases

llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s

2

debug_line_invalid.s won't cut it because this needs to be at the end of the section (at least until the truncation patch lands -- after that I was planning on deleting it as it will be equivalent to one of the tests there -- however, given the new test cases I've added now, it can probably stay).

The dwarf generator in the unittests would suffer from a similar problem -- I could get it to emit a shorter prologue length (though I would have to manually compute what that length would be), but it would still try to emit the prologue in entirety.

37

I've done something (maybe?) even better. One of the new cases I've added shows how the prologue would look like if it was complete.

[jhenderson]

LGTM, with one request.

llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s

1–2

Something I'm trying to encourage is using "##" for comment markers in lit tests using assembly/YAML etc, to distinguish from the RUN and CHECK lines. I find it makes them stand out better. Would you mind changing this here?

37

Thanks. I didn't know that logic was even possible in assembly!

[labath]

llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s

1–2

Yeah, I think that's a good convention -- I just keep forgetting about that.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

 }
 
 // Parse v5 directory/file entry content descriptions.
 // 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, uint64_t *OffsetPtr,
                    DWARFDebugLine::ContentTypeTracker *ContentTypes) {
+  Error Err = Error::success();
   ContentDescriptors Descriptors;
-  int FormatCount = DebugLineData.getU8(OffsetPtr);
+  int FormatCount = DebugLineData.getU8(OffsetPtr, &Err);
   bool HasPath = false;
-  for (int I = 0; I != FormatCount; ++I) {
+  for (int I = 0; I != FormatCount && !Err; ++I) {
     ContentDescriptor Descriptor;
     Descriptor.Type =
-      dwarf::LineNumberEntryFormat(DebugLineData.getULEB128(OffsetPtr));
-    Descriptor.Form = dwarf::Form(DebugLineData.getULEB128(OffsetPtr));
+        dwarf::LineNumberEntryFormat(DebugLineData.getULEB128(OffsetPtr, &Err));
+    Descriptor.Form = dwarf::Form(DebugLineData.getULEB128(OffsetPtr, &Err));
     if (Descriptor.Type == dwarf::DW_LNCT_path)
       HasPath = true;
     if (ContentTypes)
       ContentTypes->trackContentType(Descriptor.Type);
     Descriptors.push_back(Descriptor);
   }
 
+  if (Err)
+    return createStringError(errc::invalid_argument,
+                             "failed to parse entry content descriptors: %s",
+                             toString(std::move(Err)).c_str());
+
   if (!HasPath)
     return createStringError(errc::invalid_argument,
                              "failed to parse entry content descriptions"
                              " because no path was found");
   return Descriptors;
 }
 
 static Error

llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s

+# Test cases when we run into the end of section while parsing a line table
+# prologue.
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj --defsym CASE=0 -o %t0
+# RUN: llvm-dwarfdump -debug-line %t0 2>&1 | FileCheck %s --check-prefixes=ALL,C0
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj --defsym CASE=1 -o %t1
+# RUN: llvm-dwarfdump -debug-line %t1 2>&1 | FileCheck %s --check-prefixes=ALL,C1
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj --defsym CASE=2 -o %t2
+# RUN: llvm-dwarfdump -debug-line %t2 2>&1 | FileCheck %s --check-prefixes=ALL,C2
+
+# RUN: llvm-mc -triple x86_64-pc-linux %s -filetype=obj --defsym CASE=3 -o %t3
+# RUN: llvm-dwarfdump -debug-line %t3 2>&1 | FileCheck %s --check-prefixes=ALL,OK
+
+# ALL:      debug_line[0x00000000]
+# C0-NEXT:  warning: parsing line table prologue at 0x00000000 found an invalid directory or file table description at 0x00000027
+# C0-NEXT:  warning: failed to parse entry content descriptors: unexpected end of data at offset 0x27
+# C1-NEXT:  warning: parsing line table prologue at 0x00000000 found an invalid directory or file table description at 0x0000002a
+# C1-NEXT:  warning: failed to parse entry content descriptors: malformed uleb128, extends past end
+# C2-NEXT:  warning: parsing line table prologue at 0x00000000 found an invalid directory or file table description at 0x0000002b
+# C2-NEXT:  warning: failed to parse entry content descriptors: malformed uleb128, extends past end
+# ALL:      include_directories[  0] = "/tmp"
+# OK:       file_names[  0]:
+# OK-NEXT:             name: "foo"
+# OK-NEXT:        dir_index: 0
+
+.section .debug_line,"",@progbits
+.long   .Lend-.Lstart   # Length of Unit
+.Lstart:
+.short  5               # DWARF version number
+.byte   8               # Address Size
+.byte   0               # Segment Selector Size
+.long   .Lprologue_end-.Lprologue_start  # Length of Prologue
+.Lprologue_start:
+.byte   1               # Minimum Instruction Length
+.byte   1               # Maximum Operations per Instruction
+.byte   1               # Default is_stmt
+.byte   -5              # Line Base
+.byte   14              # Line Range
+.byte   13              # Opcode Base
+.byte   0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 # Standard Opcode Lengths
+# Directory table format
+.byte   1               # One element per directory entry
+.byte   1               # DW_LNCT_path
+.byte   0x08            # DW_FORM_string
+# Directory table entries
+.byte   1               # 1 directory
+.asciz  "/tmp"
+# File table format
+.if CASE >= 1
+.byte   2               # 2 elements per file entry
+.byte   1               # DW_LNCT_path
+.byte   8               # DW_FORM_string
+.if CASE >= 2
+.byte   2               # DW_LNCT_directory_index
+.if CASE >= 3
+.byte   11              # DW_FORM_data1
+# File table entries
+.byte   1
+.asciz  "foo"
+.byte   0
+.endif
+.endif
+.endif
+
+.Lprologue_end:
+.Lend: