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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added a comment.

In https://reviews.llvm.org/D44560#1046829, @jhenderson wrote:

> Currently trying to rebase this change following https://reviews.llvm.org/rLLD328284. In that change, LLD was changed to call getLineTableForUnit, instead of getOrParseLineTable. This function is effectively another level up the call stack, which, if I wanted to follow the current pattern, would result in lots of callers having to consume the Expected return value, as well as pass in a callback for the warning. I was discussing this with a colleague, and one suggestion he had was to instead register separate callbacks for warnings and errors in the `DWARFContext`. The parser would then simply call the appropriate callback.


Do you mean registering some kind of error handler in the dwarf context? I guess that sounds reasonable if it’s not too intrusive. We could provide a default implementation that simply prints “warning” and “error” so that the behavior remains unchanged initially. If we decide this is the way to go it should definitely go into a separate diff.

Potentially, other parts of the DWARF library could use the same callbacks ultimately. Tools would register their callback with their instance of the `DWARFContext`. Thus, for LLD, it would be to always call the LLD `warn` function (or whatever we choose to replace it - see the discussion on https://reviews.llvm.org/D44562), whilst other consumers would continue to use the default callbacks (currently `handleDebugLineParseError` and `DWARFDebugLine::warn`). This raises the question of how to handle the fatal error case (i.e. where a valid unit length cannot be read). I think the options here are the same as before:

> 
> 
> 1. As with the existing behaviour, reset the length field, and then have the caller check this.
> 2. Add a method to `LineTable::Prologue` that states whether the unit length is valid or not somehow. The caller would then be responsible for checking this, if it might want to continue onto the next table (as in the dwarfdump case), but can also ignore it and stop.
> 3. As in the current version of this diff, provide information in the `Error` returned to the caller, which the caller uses, if it wishes to.
> 4. Pass additional information into the callback, making the callback signature `Error(bool IsUnitLengthValid, StringRef Msg)` or similar.

As the error handling for line tables is significantly different,, I see no harm in having a dedicated signature for the callback in the error handler. Based on the layering I expect we don’t have access to the co text in the parser? If anything I’d prefer not to clutter the interface too much, and directly use the context’s error handler from the parser, if at all possible.

> I'd appreciate some feedback from people on which approach seems the best, both in terms of registering the callback(s), and in handling errors that prevent further section parsing (as opposed to further table parsing).

I’m currently on vacation but I’ll be able to give some more in-depth feedback early next week.


Repository:
  rL LLVM

https://reviews.llvm.org/D44560