[DebugInfo] Refactor how unrecoverable debug line parsing errors work

[jhenderson]

Summary

Previously, unrecoverable errors were passed back to the debug line parse() caller, who would then pass the error to a callback. This meant that such errors were inconsistently printed in the output between verbose and non-verbose output: for verbose output, they'd be printed after the output (which is printed at the end of the parse() function) whilst non-verbose output was printed later on, after the error callback was run. This patch moves the error callback usage to inside the parse() function.

Depends on D81102.

[dblaikie]

Redirecting conversation from D81102:

This is the sort of "let's try to curate where errors print out relative to output" changes/direction I think are pretty unfortunate - being able to return errors rather than passing in error handlers seems like a better general coding paradigm where possible (while the callback system for warnings is great, and callbacks for errors are sometimes necessary - when there is some ability to print an error and continue) - returning errors seems clearer/more obvious/simpler control flow (there's no ability to accidentally print an error and continue because you forgot the error handler doesn't terminate execution/need to separately return? and what value do you return if you need to communicate to the caller they need to stop too etc)...

The problem is that in the common case, most tools are doing the same thing with both recoverable and unrecoverable errors, it's just done at different times. This means that some error messages appear in the middle of parsing the table (before the terminating blank line), whilst others appear after the prologue has been printed, after the blank line, and seemingly with the next table. This is the incosistency and potential confusion I'm trying to avoid here.

I have a patch for this already, but essentially all that it does is change this function to return void, having passed the error to a separate UnrecoverableErrorHandler (which already exists in higher-level code). The change is NFC from the point of view of what gets parsed - the same handler will still get called allowing the client to stop or do whatever etc. The only difference is when the callback is called, and since it's a callback, the client can still handle the error as it did before. In practice, they both just get manifested as warnings most of the time, so the timing difference looks weird to most clients.

Whilst I get your point about simpler control flow, we are currently in a slightly weird hybrid situation where sometimes we use a callback and other times we return the error, so in some ways, I think this idea simplifies the interface.

I think we're going in a less elegant direction by supporting more dumping/printing while parsing, rather than less - making these sort of error handling V dumping complexities.

What about breaking up DWARFDebugLine::LineTable::parse's implementation - so a caller could explicitly parse the prologue & decide what to do with that error, rather than having parse responsible for dumping things - then call to parse the rest of the table (where I wouldn't mind revisiting the dumping+parsing mix there too but can save that for another day/refactor/review/etc).

I think my underlying thesis here is that mixing parsing and dumping is a bit problematic/difficult to maintain well (though I don't have a perfect answer for the verbose line table dumping, for instance - I imagine the realistic solution there is a callback-based API for the raw/verbose line table, and implementing the processed line table on top of that API - possibly inserting an adapter of sorts there when you want to retrieve the processed line table while at the same time dumping the verbose details) and that fatal errors are probably tidier handled via return values than via a callback since they should be stopping control flow anyway. (at some point they're not quite fatal - fatal within one line table contribution, but still allow the parser to continue reading the next contribution - at that API level you'd change to a callback/"non-fatal" concept - it's fatal to the line table contribution, but non-fatal to the line table section as a whole)

But if you/other folks feel this patch is the right direction, I won't stand in the way of it -just my 2c.

[MaskRay]

In D81165#2074048, @dblaikie wrote:

Redirecting conversation from D81102:

This is the sort of "let's try to curate where errors print out relative to output" changes/direction I think are pretty unfortunate - being able to return errors rather than passing in error handlers seems like a better general coding paradigm where possible (while the callback system for warnings is great, and callbacks for errors are sometimes necessary - when there is some ability to print an error and continue) - returning errors seems clearer/more obvious/simpler control flow (there's no ability to accidentally print an error and continue because you forgot the error handler doesn't terminate execution/need to separately return? and what value do you return if you need to communicate to the caller they need to stop too etc)...

The problem is that in the common case, most tools are doing the same thing with both recoverable and unrecoverable errors, it's just done at different times. This means that some error messages appear in the middle of parsing the table (before the terminating blank line), whilst others appear after the prologue has been printed, after the blank line, and seemingly with the next table. This is the incosistency and potential confusion I'm trying to avoid here.

I have a patch for this already, but essentially all that it does is change this function to return void, having passed the error to a separate UnrecoverableErrorHandler (which already exists in higher-level code). The change is NFC from the point of view of what gets parsed - the same handler will still get called allowing the client to stop or do whatever etc. The only difference is when the callback is called, and since it's a callback, the client can still handle the error as it did before. In practice, they both just get manifested as warnings most of the time, so the timing difference looks weird to most clients.

Whilst I get your point about simpler control flow, we are currently in a slightly weird hybrid situation where sometimes we use a callback and other times we return the error, so in some ways, I think this idea simplifies the interface.

I think we're going in a less elegant direction by supporting more dumping/printing while parsing, rather than less - making these sort of error handling V dumping complexities.

What about breaking up DWARFDebugLine::LineTable::parse's implementation - so a caller could explicitly parse the prologue & decide what to do with that error, rather than having parse responsible for dumping things - then call to parse the rest of the table (where I wouldn't mind revisiting the dumping+parsing mix there too but can save that for another day/refactor/review/etc).

I think my underlying thesis here is that mixing parsing and dumping is a bit problematic/difficult to maintain well (though I don't have a perfect answer for the verbose line table dumping, for instance - I imagine the realistic solution there is a callback-based API for the raw/verbose line table, and implementing the processed line table on top of that API - possibly inserting an adapter of sorts there when you want to retrieve the processed line table while at the same time dumping the verbose details) and that fatal errors are probably tidier handled via return values than via a callback since they should be stopping control flow anyway. (at some point they're not quite fatal - fatal within one line table contribution, but still allow the parser to continue reading the next contribution - at that API level you'd change to a callback/"non-fatal" concept - it's fatal to the line table contribution, but non-fatal to the line table section as a whole)

But if you/other folks feel this patch is the right direction, I won't stand in the way of it -just my 2c.

llvm::DWARFDebugLine::Prologue::parse has been called in three places. llvm::DWARFDebugLine::LineTable::parse is called in three places. Splitting llvm::DWARFDebugLine::LineTable::parse into two (parsePrologue and parseBody) seems fine to me.

[labath]

In D81165#2074048, @dblaikie wrote:

Redirecting conversation from D81102:

This is the sort of "let's try to curate where errors print out relative to output" changes/direction I think are pretty unfortunate - being able to return errors rather than passing in error handlers seems like a better general coding paradigm where possible (while the callback system for warnings is great, and callbacks for errors are sometimes necessary - when there is some ability to print an error and continue) - returning errors seems clearer/more obvious/simpler control flow (there's no ability to accidentally print an error and continue because you forgot the error handler doesn't terminate execution/need to separately return? and what value do you return if you need to communicate to the caller they need to stop too etc)...

The problem is that in the common case, most tools are doing the same thing with both recoverable and unrecoverable errors, it's just done at different times. This means that some error messages appear in the middle of parsing the table (before the terminating blank line), whilst others appear after the prologue has been printed, after the blank line, and seemingly with the next table. This is the incosistency and potential confusion I'm trying to avoid here.

I have a patch for this already, but essentially all that it does is change this function to return void, having passed the error to a separate UnrecoverableErrorHandler (which already exists in higher-level code). The change is NFC from the point of view of what gets parsed - the same handler will still get called allowing the client to stop or do whatever etc. The only difference is when the callback is called, and since it's a callback, the client can still handle the error as it did before. In practice, they both just get manifested as warnings most of the time, so the timing difference looks weird to most clients.

Whilst I get your point about simpler control flow, we are currently in a slightly weird hybrid situation where sometimes we use a callback and other times we return the error, so in some ways, I think this idea simplifies the interface.

I think we're going in a less elegant direction by supporting more dumping/printing while parsing, rather than less - making these sort of error handling V dumping complexities.

What about breaking up DWARFDebugLine::LineTable::parse's implementation - so a caller could explicitly parse the prologue & decide what to do with that error, rather than having parse responsible for dumping things - then call to parse the rest of the table (where I wouldn't mind revisiting the dumping+parsing mix there too but can save that for another day/refactor/review/etc).

I think my underlying thesis here is that mixing parsing and dumping is a bit problematic/difficult to maintain well (though I don't have a perfect answer for the verbose line table dumping, for instance - I imagine the realistic solution there is a callback-based API for the raw/verbose line table, and implementing the processed line table on top of that API - possibly inserting an adapter of sorts there when you want to retrieve the processed line table while at the same time dumping the verbose details) and that fatal errors are probably tidier handled via return values than via a callback since they should be stopping control flow anyway. (at some point they're not quite fatal - fatal within one line table contribution, but still allow the parser to continue reading the next contribution - at that API level you'd change to a callback/"non-fatal" concept - it's fatal to the line table contribution, but non-fatal to the line table section as a whole)

But if you/other folks feel this patch is the right direction, I won't stand in the way of it -just my 2c.

I also think it would be good to separate the (verbose) dumping from the parsing code. The way we did that for location list dumping is that the parser was a function taking a callback -- Error visitLocationList(uint64_t *Offset, function_ref<bool(const DWARFLocationEntry &)> Callback). I think a similar approach would work here, except that the callback argument would be Expected<Opcode>, because we can have "recoverable" errors when parsing opcodes (there are no such errors for location lists). Fatal errors would be reported via the return value. When dumping, once can pass in a callback which prints out the raw opcodes.

Doing something similar for line tables would be sort of simpler, because we don't need to support two very different encodings (debug_loc, debug_loclists), but it would also be more complicated because line tables are more complicated. But if we can do a small change like splitting prologue parsing that would move us in that direction, then I think we should do it.

[jhenderson]

One of my other pending patches is going to address truncated standard opcodes, similar to what D80797 does for extended opcodes. Unfortunately, because standard opcodes have no dedicated length field (unlike extended ones), it's not possible to resume parsing of the current table, if there's a problem reading the data. In all likelihood of course, it doesn't matter (the only errors are likely to be running off the table end, but technically an overflowing ULEB could also cause an error), but really this failure should be reported as an "unrecoverable" error, to indicate that table parsing is incomplete. This means that unrecoverable errors will be encountered when parsing the body as well as the prologue, and as such, I don't think splitting the prologue and body parsing into two separate functions will help solve the issue. I'll give it some thought and look at the .debug_loc code to consider alternatives. I do find parsing and printing simultaneously to be a little less than ideal, I must say.

[jhenderson]

I've spotted another example of where the current error handling technique doesn't work well combined with verbose output - when there is an error with the address size, the error will be reported mid line, which means that if the streams are interleaved, you'll get something like 0x0000002f: 00 DW_LNE_set_addresswarning: ... (0x00000008). There are potentially similar problems for other opcodes too. It could be possible to delay sending these errors to the handler until the full line has been printed, but I'm not particularly happy with the code changes required for that. This further illustrates that making the verbose printing independent of the parsing somehow might be preferable.

[jhenderson]

I've addressed the main issues I aimed to resolve in other patches. There may be some issues with odd output of error messages when the output is interleaved, but I have other priorities to work on at this point. If someone wants to work on this furhter, it's probably best done in a new review.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

bool getFileLineInfoForAddress(object::SectionedAddress Address,
                                    const char *CompDir,
                                    DILineInfoSpecifier::FileLineInfoKind Kind,
                                    DILineInfo &Result) const;
 
     void dump(raw_ostream &OS, DIDumpOptions DumpOptions) const;
     void clear();
 
     /// Parse prologue and all rows.
-    Error parse(DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
-                const DWARFContext &Ctx, const DWARFUnit *U,
-                function_ref<void(Error)> RecoverableErrorHandler,
-                raw_ostream *OS = nullptr, bool Verbose = false);
+    void parse(DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
+               function_ref<void(Error)> UnrecoverableErrorHandler,
 
     using RowVector = std::vector<Row>;
     using RowIter = RowVector::const_iterator;
     using SequenceVector = std::vector<Sequence>;
     using SequenceIter = SequenceVector::const_iterator;
 
     struct Prologue Prologue;
     RowVector Rows;

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp

if (!DebugLineData.isValidOffset(Offset))
     return createStringError(errc::invalid_argument, "offset 0x%8.8" PRIx64
                        " is not a valid debug line section offset",
                        Offset);
 
   std::pair<LineTableIter, bool> Pos =
       LineTableMap.insert(LineTableMapTy::value_type(Offset, LineTable()));
   LineTable *LT = &Pos.first->second;
   if (Pos.second) {
-    if (Error Err =
-            LT->parse(DebugLineData, &Offset, Ctx, U, RecoverableErrorHandler))
+    Error Err = Error::success();
+    LT->parse(DebugLineData, &Offset, Ctx, U, RecoverableErrorHandler,
+              [&Err](Error NewErr) {
+                // Only one unrecoverable error should be reported per call to
+                // parse.
+                cantFail(std::move(Err));
+                Err = std::move(NewErr);
+              });
+    if (Err)
       return std::move(Err);
     return LT;
   }
   return LT;
 }
 
 static StringRef getOpcodeName(uint8_t Opcode, uint8_t OpcodeBase) {
   assert(Opcode != 0);
DWARFDebugLine::ParsingState::handleSpecialOpcode(uint8_t Opcode,
   if (LineTable->Prologue.LineRange != 0)
     LineOffset =
         LineTable->Prologue.LineBase +
         (AddrAdvanceResult.AdjustedOpcode % LineTable->Prologue.LineRange);
   Row.Line += LineOffset;
   return {AddrAdvanceResult.AddrDelta, LineOffset};
 }
 
-Error DWARFDebugLine::LineTable::parse(
+void DWARFDebugLine::LineTable::parse(
     DWARFDataExtractor &DebugLineData, uint64_t *OffsetPtr,
     const DWARFContext &Ctx, const DWARFUnit *U,
-    function_ref<void(Error)> RecoverableErrorHandler, raw_ostream *OS,
+    function_ref<void(Error)> RecoverableErrorHandler,
+    function_ref<void(Error)> UnrecoverableErrorHandler, raw_ostream *OS,
     bool Verbose) {
   assert((OS || !Verbose) && "cannot have verbose output without stream");
   const uint64_t DebugLineOffset = *OffsetPtr;
 
   clear();
 
   Error PrologueErr =
       Prologue.parse(DebugLineData, OffsetPtr, RecoverableErrorHandler, Ctx, U);
 
   if (OS) {
     DIDumpOptions DumpOptions;
     DumpOptions.Verbose = Verbose;
     Prologue.dump(*OS, DumpOptions);
   }
 
   if (PrologueErr) {
-    // Ensure there is a blank line after the prologue to clearly delineate it
-    // from later dumps.
+    UnrecoverableErrorHandler(std::move(PrologueErr));
     if (OS)
       *OS << "\n";
-    // FIXME: Rather than return this error, we should handle it here in a
-    // callback, so that the error will (if printed) be placed before the new
-    // line above.
-    return PrologueErr;
+    return;
   }
 
   uint64_t ProgramLength = Prologue.TotalLength + Prologue.sizeofTotalLength();
   if (!DebugLineData.isValidOffsetForDataOfSize(DebugLineOffset,
                                                 ProgramLength)) {
     assert(DebugLineData.size() > DebugLineOffset &&
            "prologue parsing should handle invalid offset");
     uint64_t BytesRemaining = DebugLineData.size() - DebugLineOffset;
if (!Sequences.empty()) {
     // sometimes .so compiled from multiple object files contains a few
     // rudimentary sequences for address ranges [0x0, 0xsomething).
   }
 
   // Terminate the table with a final blank line to clearly delineate it from
   // later dumps.
   if (OS)
     *OS << "\n";
-
-  return Error::success();
 }
 
 uint32_t DWARFDebugLine::LineTable::findRowInSeq(
     const DWARFDebugLine::Sequence &Seq,
     object::SectionedAddress Address) const {
   if (!Seq.containsPC(Address))
     return UnknownRowIndex;
   assert(Seq.SectionIndex == Address.SectionIndex);

 DWARFDebugLine::LineTable DWARFDebugLine::SectionParser::parseNext(
     function_ref<void(Error)> RecoverableErrorHandler,
     function_ref<void(Error)> UnrecoverableErrorHandler, raw_ostream *OS, bool Verbose) {
   assert(DebugLineData.isValidOffset(Offset) &&
          "parsing should have terminated");
   DWARFUnit *U = prepareToParse(Offset);
   uint64_t OldOffset = Offset;
   LineTable LT;
-  if (Error Err = LT.parse(DebugLineData, &Offset, Context, U,
-                           RecoverableErrorHandler, OS, Verbose))
-    UnrecoverableErrorHandler(std::move(Err));
+  LT.parse(DebugLineData, &Offset, Context, U, RecoverableErrorHandler,
+           UnrecoverableErrorHandler, OS, Verbose);
   moveToNextTable(OldOffset, LT.Prologue);
   return LT;
 }
 
 void DWARFDebugLine::SectionParser::skip(
     function_ref<void(Error)> RecoverableErrorHandler,
     function_ref<void(Error)> UnrecoverableErrorHandler) {
   assert(DebugLineData.isValidOffset(Offset) &&

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test

 ## For non-fatal issues, the table data should be dumped:
 
 ## Version 0 table.
 # NONFATAL:      debug_line[0x00000048]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL-NEXT:    total_length: 0x00000002
 # NONFATAL-NEXT:          format: DWARF32
 # NONFATAL-NEXT:         version: 0
-# NONFATAL-EMPTY:
 # SOME-ERR-NEXT: warning: parsing line table prologue at offset 0x00000048: unsupported version 0
+# NONFATAL-EMPTY:
 
 ## Version 1 table.
 # NONFATAL:      debug_line[0x0000004e]
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL-NEXT:    total_length: 0x00000002
 # NONFATAL-NEXT:          format: DWARF32
 # NONFATAL-NEXT:         version: 1
-# NONFATAL-EMPTY:
 # SOME-ERR-NEXT: warning: parsing line table prologue at offset 0x0000004e: unsupported version 1
+# NONFATAL-EMPTY:
 
 ## Malformed directory format with no path component.
 # NONFATAL:      debug_line[0x00000054]
 # SOME-ERR-NEXT: warning: parsing line table prologue at 0x00000054 found an invalid directory or file table description at 0x00000073
 # SOME-ERR-NEXT: warning: failed to parse entry content descriptions because no path was found
 # NONFATAL-NEXT: Line table prologue
 # NONFATAL:      prologue_length: 0x00000013
 # NONFATAL-NOT:  include_directories