[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#1041532, @jhenderson wrote:

> 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?


Yup, totally fine. Now we have some kind of record in case this issue ever comes up.

Also, great work on extending the dwarf generator, it looks very useful!



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLine.h:310
+      const DWARFContext &Ctx, const DWARFUnit *U,
+      std::function<void(StringRef)> MinorIssueCallback = warn);
 
----------------
s/MinorIssueCallback/WarnCallback/


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:437
       unsigned OldOffset = Offset;
-      if (!LineTable.Prologue.parse(LineData, &Offset, *this, U))
+      Error Err = LineTable.Prologue.parse(LineData, &Offset, *this, U);
+      bool FoundError(Err);
----------------
Not sure if this much better, but this way you don't need to "check" the error twice?
```
if (Error Err = LineTable.Prologue.parse(LineData, &Offset, *this, U)) 
  if (handleDebugLineParseErrors(std::move(Err)))
    break;
else if (!DumpOffset || OldOffset == *DumpOffset))
  LineTable.dump(OS, DumpOpts);
```


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:445
+               LineTable.Prologue.sizeofTotalLength();
     }
   }
----------------
Also looks like verbose dumping of the line tables is missing for DWO. I filed PR36800.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLine.cpp:42
+                  [&](DebugLineError &Info) {
+                    WithColor(errs(), HighlightColor::Warning).get()
+                        << "warning: ";
----------------
jhenderson wrote:
> 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.
Alright, I'm okay with keeping it a warning for now. We can always revisit this in the future. 


Repository:
  rL LLVM

https://reviews.llvm.org/D44560