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

[James Henderson via Phabricator via llvm-commits]

jhenderson marked 5 inline comments as done.
jhenderson added a comment.

I was in the middle of updating this diff when you made your latest comment! Thanks for the explanation. I figured out what you meant subsequent to my previous comments (see the inline comment which I'd written before I saw your explanation). If you're okay with it, I'd prefer to keep the callback as it currently is in this diff (apart from anything else, it keeps the LLD-side changes simpler). If we get a concrete use-case later, maybe then is the time to make the change?



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:36
+  // section.
+  DebugLineError(bool IsFatal, const Twine &Msg)
+      : IsFatal(IsFatal), Msg(Msg.str()) {}
----------------
JDevlieghere wrote:
> jhenderson wrote:
> > JDevlieghere wrote:
> > > How about switching the order and giving the bool a default value (`false`?)
> > I tried doing this, but there was little benefit, since this is always created via createError, which is a function with variadic templates, making it impossible (to my knowledge) to use a default argument in createError. I could still change it, but `IsFatal` will still be specified explicitly every time it is called.
> Alright, I'll leave the decision up to you. My reasoning was that it doesn't harm and might make things easier in the future if we ever want to construct this error without the createError helper. 
That's a fair point. I'll make that change.


================
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);
----------------
jhenderson wrote:
> 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).
I realised that I completely misunderstood your second bullet-point here. From my understanding, you were suggesting that the callback would be called for "minor" issues, and it would then return Error (success for the default behaviour of just printing), i.e. the signature of the callback is `std::function<Error(StringRef)>`. I certainly agree that is kind of interesting, but I'm not sure what the use-case of it is currently. The obvious case is for wanting to treat warnings as errors, but the only current user of that in this situation to my knowledge is LLD, which has a function that does all the work for us (see the changes in D44562 and the `warn` function), so I suggest that this change is saved for when there is a concrete use-case.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:42
+                  [&](DebugLineError &Info) {
+                    WithColor(errs(), HighlightColor::Warning).get()
+                        << "warning: ";
----------------
JDevlieghere wrote:
> Should a fatal error not be displayed as an error instead of a warning?
The intention of this function is to provide a "default" handler for all current users of the code. Prior to my change, all problems detected were reported as warnings to the end user (or simply not printed at all), and whatever it was doing would simply stop, and not cause an error. I'd prefer to keep it that way for now (i.e. in this change). I think we could make a case for changing to emitting errors, not warnings, but I'm not sure this change is necessarily the right place to have that discussion, since it is a more fundamental change in behaviour in tools like llvm-dwarfdump.


Repository:
  rL LLVM

https://reviews.llvm.org/D44560