[DebugInfo] Prevent crash when .debug_line line_range is zero

[jhenderson]

Summary

This change adds a check for a .debug_line line_range value of 0 to prevent the program crashing when a special opcode or DW_LNS_const_add_pc is detected. The diagnostic is only issued once per table, and for this case the opcodes do not adjust the address or line.

Depends on D74819.

[aprantl]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

309

Can you also make it so that llvm-dwarfdump --verify reports this as an error?

[aprantl]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

309

Nevermind. That's what you did!

[aprantl]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

302

Bonus points for fixing this one, too :-)

309

Can you use DWARFVerifier::warn() here?

[jhenderson]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

290

And presumably this one too @aprantl?

309

I'll take a look for this and the other usages here.

llvm-dwarfdump --verify reports an error automatically if false is returned here, so that's how we get an error, as demonstrated in the test.

[aprantl]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

290

That would be great. Can be a separate commit, and you don't need to open a review for that one.

[jhenderson]

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

309

Can you use DWARFVerifier::warn() here?

Just quickly looking at this before I leave the office. This function is currently private, and relies on an instance of DWARFVerifier, which I don't think is available. Do you mean to make it do the same thing as what DWARFVerifier::warn() does?

[JDevlieghere]

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

[jhenderson]

In D43470#1012578, @JDevlieghere wrote:

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

Having thought about this a bit and played around, I'm inclined to agree that this is the correct approach. It'll be a bigger change, but it will make the code more amenable to being used as a library, where the consumers can decide what to do with the errors/warnings on a case-by-case basis. I'll experiment with it a bit more, and then upload an updated patch.

[JDevlieghere]

In D43470#1013034, @jhenderson wrote:

In D43470#1012578, @JDevlieghere wrote:

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

Having thought about this a bit and played around, I'm inclined to agree that this is the correct approach. It'll be a bigger change, but it will make the code more amenable to being used as a library, where the consumers can decide what to do with the errors/warnings on a case-by-case basis. I'll experiment with it a bit more, and then upload an updated patch.

Great, thanks!

[jhenderson]

Resurrecting this change, by rewriting the fix on the latest HEAD. In this version, the check is done when a special opcode or DW_LNS_const_add_pc is encountered. An Error is reported if the line_range is 0 for the first such instance of these. Testing is now done via unit tests.

[jhenderson]

Please could this be re-reviewed, given the significant changes in the area since the last review.

[jhenderson]

I missed a case that needs checking. Will revisit.

[jhenderson]

Rewrite based on D75188 and D74819.

[probinson]

It looks like getOperationAdvance() and advanceAddr() now share a lot of parameters. Is it feasible to combine them? I see that they're not *always* called together, but it seems like it would improve clarity not to repeat 3 lines of parameters.

[jhenderson]

Rebase, to adopt interface change as suggested by @probinson. Also fix a warning in the test.

[probinson]

I see a pre-merge clang-format alert in DWARFDebugLine.h that you should take care of.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

668 ↗

(On Diff #247604)

This seems a bit much for a ternary operator.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp

981 ↗

(On Diff #247604)

While you _can_ write this in one line, I think it's clearer to write

if (LineRange == 0)
  return Base + AdvanceIncr;
return AdjustAddressFixtureBase::getAdjustedAddr( ... );

[jhenderson]

Fix formatting. Simplify ternaries.

[probinson]

LGTM.

[jhenderson]

Rebase to take account of opcode name handling changes.

[probinson]

One more small bit of cleanup.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

606 ↗

(On Diff #248467)

This could go under the if as you don't need the name unless you are reporting the warning.

615 ↗

(On Diff #248467)

Setting the flag to false can go under the if to help distinguish error handling from functional code.

[jhenderson]

Rebase, and put bits inside the if.

[jhenderson]

Remove unnecessary public.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

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, HasMD5, IncludeDirectories,
                               FileNames)) {
       fprintf(stderr,
               "warning: parsing line table prologue at 0x%8.8" PRIx64
               " found an invalid directory or file table description at"
               " 0x%8.8" PRIx64 "\n", PrologueOffset, (uint64_t)*OffsetPtr);
       return false;
     }
   } else
     parseV2DirFileTables(DebugLineData, OffsetPtr, EndPrologueOffset,
                          IncludeDirectories, FileNames);
 
   if (*OffsetPtr != EndPrologueOffset) {
     fprintf(stderr,
             "warning: parsing line table prologue at 0x%8.8" PRIx64
             " should have ended at 0x%8.8" PRIx64
             " but it ended at 0x%8.8" PRIx64 "\n",
             PrologueOffset, EndPrologueOffset, (uint64_t)*OffsetPtr);
     return false;
   }
+
+  if (LineRange == 0) {
+    fprintf(stderr, "warning: parsing line table prologue at 0x%8.8" PRIx64
+                    " found a line_range of value 0\n",
+            PrologueOffset);
+    return false;
+  }
+
   return true;
 }
 
 DWARFDebugLine::Row::Row(bool DefaultIsStmt) { reset(DefaultIsStmt); }
 
 void DWARFDebugLine::Row::postAppend() {
   BasicBlock = false;
   PrologueEnd = false;

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

+# RUN: llvm-mc %s -filetype obj -triple x86_64-pc-linux -o %t.o
+# RUN: llvm-dwarfdump -debug-line %t.o | FileCheck %s -check-prefixes=CHECK
+# RUN: FileCheck %s -check-prefix=WARN -input-file=%t.err
+# RUN: not llvm-dwarfdump -debug-line -verify %t.o 2> %t2.err | FileCheck %s -check-prefix=ERR
+# RUN: FileCheck %s -check-prefix=WARN -input-file=%t2.err
+
+# CHECK-NOT: error:
+# CHECK-NOT: warning:
+# CHECK: line_range: 0
+# CHECK-NOT: error:
+# CHECK-NOT: warning:
+
+# WARN: warning: parsing line table prologue at 0x00000000 found a line_range of value 0
+# ERR: error: .debug_line[0x00000000] was not able to be parsed for CU:
+
+.section .debug_line,"",@progbits
+
+.Lline_table_start0:
+.long .Ldebug_line_end0 - .Ldebug_line_start0 # unit_length
+.Ldebug_line_start0:
+.short 4                                      # version
+.long .Lheader_end - .Lheader_start           # header_length
+.Lheader_start:
+.byte 1                                       # minimum_instruction_length
+.byte 1                                       # maximum_operations_per_instruction
+.byte 1                                       # default_is_stmt
+.byte -5                                      # line_base
+.byte 0                                       # line_range
+.byte 13                                      # opcode_base
+.byte 0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1      # standard_opcode_lengths
+.byte 0                                       # include directories
+.byte 0                                       # file_names
+.Lheader_end:
+.byte 25 # special opcode. In order to calculate its meaning, a division by line_range is required.
+.Ldebug_line_end0:
+
+.section    .debug_info,"",@progbits
+.Lsection_info:
+.long   .Lunit_end - .Lunit_start # Length of Unit
+.Lunit_start:
+.short  4                       # DWARF version number
+.long   .Lsection_abbrev        # Offset Into Abbrev. Section
+.byte   8                       # Address Size (in bytes)
+.byte   1                       # Abbrev [1] 0xb:0x59 DW_TAG_compile_unit
+.long   .Lline_table_start0     # DW_AT_stmt_list
+.byte   0                       # End Of Children Mark
+.Lunit_end:
+
+.section    .debug_abbrev,"",@progbits
+.Lsection_abbrev:
+.byte   1                       # Abbreviation Code
+.byte   17                      # DW_TAG_compile_unit
+.byte   0                       # DW_CHILDREN_no
+.byte   16                      # DW_AT_stmt_list
+.byte   23                      # DW_FORM_sec_offset
+.byte   0                       # EOM(1)
+.byte   0                       # EOM(2)
+.byte   0                       # EOM(3)