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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the comments @JDevlieghere. I'll get an updated diff posted later, maybe Monday.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:59
+/// in the section.
+bool handleDebugLineParseErrors(Error ParseErrors);
+
----------------
JDevlieghere wrote:
> How do you feel about the following idea: what if we use this function as the callback and have it take *and* return an error. If the function handles the error (i.e. print it), this returns success, and the caller continues. If the error is fatal and/or prevents further parsing, we defer the problem and we have the parse method return the underlying error. Would that fit this use case?
It's possible I don't fully understand what you're saying, but I'm not sure that would work. This would need to be a different callback to the warn callback, since the behaviour in the latter is to continue parsing the table, which is different to what is needed for other problems. We could perhaps have the callback take a message, and a severity, which could allow it to print a message or raise an error as appropriate, but I'm not sure I see any benefit in that over what is in D44382.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:65
+/// \param Message The message to report.
+void warnForMinorIssue(StringRef Message);
+
----------------
JDevlieghere wrote:
> I think something like `warn` would be sufficiently clear.
It's a namespacing thing - I don't want this function confused with, say, LLD's "warn" function, which may not do the same thing. I guess an argument could be made for adding a "warn" function the Support library, or similar, though I'm not sure about that. Maybe I could make it a static method of DWARFDebugLine?


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:286
+          const DWARFContext &Ctx, const DWARFUnit *U,
+          std::function<void(StringRef)> MinorIssueCallback = warnForMinorIssue,
+          raw_ostream *OS = nullptr);
----------------
JDevlieghere wrote:
> I'm not convinced on the minor/major nomenclature. I think we could two things here:
> 
>  -  Rename this to something like `warnCallback` to make it clear that this is what the callback is for.
>  - Make this a callback that takes an error *and* returns an error, allowing the callback to decide whether to handle it and how. Probably I'm over-engineering it here, but conceptually it would be nice to have the client decide whether the issue is "minor" or not. For example, one client could decide to just print the input error as a warning (and then return success(), while another could simply forward the error. This does add complexity to the parse method, as we then would need to check whether the callback returned success or not, and return the error in the latter case. 
> 
> FWIW I don't think we need the second option here, but from an interface-design point of view it could be interesting. 
I really don't think it's the place of the callback to decide whether or not parsing a LineTable can continue or not. The parser knows the current state. It should decide.

I deliberately chose the naming scheme to make it clear that this doesn't cover other kinds of issues found in parsing (i.e. the ones for which an Error are returned). However, I guess from the point of view of the parser, there are issues preventing it completing (which could be considered errors), and other issues that don't prevent reading (which could be considered warnings).


Repository:
  rL LLVM

https://reviews.llvm.org/D44560