[PATCH] D81165: [DebugInfo] Refactor how unrecoverable debug line parsing errors work

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 4 10:27:08 PDT 2020


dblaikie added a comment.

Redirecting conversation from D81102 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81165/new/

https://reviews.llvm.org/D81165





More information about the llvm-commits mailing list