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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

There's been quite a bit of discussion on the mailing list between @espindola and @dblaikie over the callback, and the consensus there seems to be that the callback should return an Error.

To clarify a few things (most of these came up with the discussion on the topic with @JDevlieghere earlier, but are summarised here for clarity):

- The callback is only called for recoverable problems (currently only a missing end sequence, but I think there is scope to extend it to e.g. unrecognised opcodes). By changing this to return an Error, we make the parser have to handle unhandled Errors in this context.
- At the moment, there are no users for stopping after the first recoverable error is found. Every user at the moment I think would benefit from getting all the information about minor issues rather than stopping, so I'd prefer not to extend this interface at this point (though I could be persuaded otherwise).
- In general, the table is in a (reasonably) valid state after a recoverable issue and the rest of the table can be parsed, but not after other issues. As such I'd be opposed to the default suggested by @espindola of issuing an error and stopping in this situation - this would be a change in behaviour from the existing behaviour, and I don't think brings any great benefit. The default should be to print a warning and continue.
- There are 3 different "bad" states of parsing currently: the recoverable issues discussed above, unrecoverable issues for parsing this line table (e.g. an invalid prologue length), but which do not prevent further reading of later tables (the unit length is valid, so the caller can use this to skip to the next table), and unrecoverable issues for which the unit length is invalid, preventing knowing where to skip to to continue reading. The latter is what I have described as "Fatal" errors.
- We need between 1 and 3 callbacks for these different types, if we want to handle everything via a callback (which would allow us to drop the custom error class). I could see the signatures being as follows:
  - For a 1-callback model:  `std::function<Error(StringRef, Severity)>`, where "Severity" is an enum representing "Recoverable/UnrecoverableButValidUnitLength/Fatal". It would be up to the callback code itself to set some external state if the caller needs to distinguish between the different failure modes. This would be my preference if people are opposed to a new error type.
  - For a 2-callback model, the first callback would be simply `std::function<Error(StringRef)>` and could be used for recoverable issues. The other would take a boolean as well, indicating fatal or not, which would be used by the callback to set some state to decide whether to attempt to parse the next table or not.
  - For a 3-callback model, each would have the same signature ( `std::function<Error(StringRef)>`), and the different callbacks could all be the same function, or could be different, if the caller cared about different states. Again, external state would need to be set if we cared about the difference between fatal and non-fatal issues.

@dblaikie said this on the mailing list in response to @espindola's comment "Having which errors are fatal be a property of the particular error type is odd."

> Agreed - I'd rather not introduce the complexity of semantically meaningful Error types, if it's reasonable to do so.
> 
> The callback would only be used for recoverable errors, though, yes? Allowing the user to differentiate the two cases ("recoverable things come through the callback, non-recoverable things are errors seen as a result that didn't pass through/come from the callback"). Seems fair to me.

I think you may have missed the difference between errors which allow parsing later tables and errors which prevent parsing any later tables. If the unrecoverable errors do not go through a callback, we need the extra information of whether the length field is valid or not available to the caller somehow. I suppose we could add an extra method to the LineTable class instead to query whether the length is valid or not instead of the custom error type, but there's a risk here that users will miss this function and still try to use the length field.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:301
                           uint64_t Address) const;
-    Optional<StringRef> getSourceByIndex(uint64_t FileIndex,
-                                         DILineInfoSpecifier::FileLineInfoKind Kind) const;
+    Optional<StringRef>
+    getSourceByIndex(uint64_t FileIndex,
----------------
espindola wrote:
> Why reformat this?
The second line is over 80 characters. I did this whilst I was touching the functions immediately below, but can revert it if it's bothersome.


Repository:
  rL LLVM

https://reviews.llvm.org/D44560