[PATCH] D44560: [DWARF] Rework debug line parsing to use llvm::Error and callbacks

Fri Mar 16 06:44:55 PDT 2018

jhenderson created this revision.
jhenderson added reviewers: probinson, dblaikie, JDevlieghere, aprantl, labath, espindola.

This is a variation of https://reviews.llvm.org/D44382, taking account of the suggestions to use a callback. I think I prefer this solution, but I want to leave the other review open until an approach has been agreed.

The .debug_line parser previously reported errors by printing to stderr and return false. This is not particularly helpful for clients of the library code, as it prevents them from handling the errors in a manner based on the calling context. This change switches to using llvm::Error and callbacks to indicate what problems were detected during parsing, and has updated clients to handle the errors in a location-specific manner. In general, this means that they continue to do the same thing to external users. Below, I have outlined what the known behaviour changes are, relating to this change.

There are three levels of "errors" in the new error mechanism, to broadly distinguish between different fail states of the parser, since not every failure will prevent parsing of the unit, or of subsequent unit. Fatal errors represent errors that prevent reading the unit length field. If this happens, it will be impossible to know where the next unit starts, so this will prevent further parsing of both the current unit and any subsequent ones. Non-fatal errors represent errors that prevent reading the remainder of the table, e.g. because
the version is unsupported. Minor issues represent problems with parsing that do not prevent attempting to continue reading the table, and are reported by calling a specified callback funciton. The only example of this currently is when the last sequence of a unit is unterminated. However, I think it would be good to change the handling of unrecognised opcodes to report as minor issues as well, rather than just printing to the stream if --verbose is used (this would be a subsequent change however).

I have substantially extended the DwarfGenerator to be able to handle custom-crafted .debug_line sections, allowing for comprehensive unit-testing of the parser code. For now, I am just adding unit tests to cover the basic error reporting, and positive cases, and do not currently intend to test every part of the parser, although the framework should be sufficient to do so at a later point.

Known behaviour changes:

- The dump function in DWARFContext now does not attempt to read subsequent tables when searching for a specific offset, if there was a fatal error.
- The dump function now uses the severity of the error to determine if subsequent units should be read or not. If a major error is detected, the table is skipped, but subsequent ones are read, making the assumption that the unit length field is valid.
- getOrParseLineTable now returns a useful Error if an invalid offset is encountered, rather than simply a nullptr.
- The parse functions no longer use fprintf(stderr,...) to report errors, meaning that LLD will now correctly print errors, rather than them sometimes not being flushed, or being interleaved with other errors (see also the forthcoming LLD review).
- The existing parse error messages have been updated to not specifically include "warning" in their message, allowing consumers to determine what severity the problem is.
- If the line table version field appears to have a value less than 2, an informative error is returned, instead of just false.
- If the line table unit length field uses a reserved value, an informative error is returned, instead of just false.

As a helper for the generator code, I have re-added EmitInt64 to the AsmPrinter code. This previously existed, but was removed way back in r100296, presumably because it was dead at the time. A quick review of LLVM code suggests that there are several places that could do with this function, instead of using EmitIntValue(..., 8).

This change also requires a change to LLD. I will post a separate review for this shortly, since I don't know how to reliably create a review for both repositories simultaneously.

I'm conscious that this is quite a large change, so if anybody has any suggestions on how to usefully break it up, please let me know.

Repository:
  rL LLVM

https://reviews.llvm.org/D44560

Files:
  include/llvm/CodeGen/AsmPrinter.h
  include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
  lib/CodeGen/AsmPrinter/AsmPrinter.cpp
  lib/DebugInfo/DWARF/DWARFContext.cpp
  lib/DebugInfo/DWARF/DWARFDebugLine.cpp
  test/DebugInfo/X86/dwarfdump-bogus-LNE.s
  test/DebugInfo/X86/dwarfdump-line-dwo.s
  test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
  test/tools/llvm-dwarfdump/X86/Inputs/debug_line_reserved_length.s
  test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
  tools/dsymutil/DwarfLinker.cpp
  unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
  unittests/DebugInfo/DWARF/DwarfGenerator.cpp
  unittests/DebugInfo/DWARF/DwarfGenerator.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D44560.138686.patch
Type: text/x-patch
Size: 61694 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180316/6b0c0ea3/attachment-0001.bin>