[DWARF] Move error printing from DWARF classes to Support library. NFC.

[vleschuk]

Summary

Both DWARFDebugLine and DWARFDebugAddr use the same callback mechanism for handling recoverable errors. They both implemented similar warn() function to be used as such callbacks.

In this revision we introduce Display[Error|Warning|Note]() functions in Support library and get rid of code duplication in DWARF classes.

[vleschuk]

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

249–250

I know that this exceeds 80 chars per line, but that's what clang-format suggested.

[jhenderson]

I'm not convinced "Display" is the right term for the message. "Print" or "Dump" would be much more typical for reporting messages.

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

249–250

clang-format?

include/llvm/Support/Error.h

912 ↗

(On Diff #161697)

Comments should have trailing full stops.

I don't understand the phrase here. Better would be to say something like "Print an Error as an error message on stderr". I think the use of WithColor should be irrelevant to the function description.

913 ↗

(On Diff #161697)

These should use lower case first for the first letter (i.e. displayError etc).

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

22–23 ↗

(On Diff #161697)

Unrelated whitespace change should not be in this commit.

[jhenderson]

In D51033#1207437, @jhenderson wrote:

I'm not convinced "Display" is the right term for the message.

This should say "for the function name"

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

249–250

Sorry, you posted this whilst I was in the middle of writing the review!

Are you sure about this? It seems unusual, especially as there are ways to deal with it (indent the arguments less).

[vleschuk]

In D51033#1207437, @jhenderson wrote:

I'm not convinced "Display" is the right term for the message. "Print" or "Dump" would be much more typical for reporting messages.

Let's use dumpError().

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

249–250

Yes, 100% sure. I specifically checked.

[vleschuk]

include/llvm/Support/Error.h

912 ↗

(On Diff #161697)

Will do.

913 ↗

(On Diff #161697)

Yeah, let's use dumpError().

lib/DebugInfo/DWARF/DWARFDebugAddr.cpp

22–23 ↗

(On Diff #161697)

Will revert.

[vleschuk]

• Changed names of functions from Display* to dump*
• Changed comments
• Got rid of unrelated whitespace change
• Manually fixed indentation when clang-format suggested smth strange

[vleschuk]

lib/Support/Error.cpp

127 ↗

(On Diff #161707)

Actually it appears that this code is invalid:

WithColor::error() and friends can be used only in-place. If you pass it as param:

void foo(raw_ostream& OS) { OS << "test\n"; }
void bar() { foo(WithColor::error()); }

Will result in two lines:

error: test
error:

What can we do here? The easiest way is duplicate the call to handleAllErrors() in each of 3 functions. But I don't like it.

[vleschuk]

lib/Support/Error.cpp

127 ↗

(On Diff #161707)

Actually the code looks correct to me, but the actual behavior looks strange.

[jhenderson]

I'm thinking a bit more about this and I'm concerned that by adding these functions to Error.h, people will be more likely to use them from libraries, which can lead to various issues when the client of the library doesn't want to report errors and warnings the same way as this implements. The warn function was really only intended as a default implementation so that we didn't have to propagate things all the way out of DWARFContext.cpp, for the single client (llvm-dwarfdump) to consume.

Given that, I think the right thing to do is to just move the DWARFDebugLine::warn function out into the DWARFContext::dump function, and pass it around as necessary (or make it a static function in the file). If you go that approach, I'd be tempted to do the same with an error function too (note that there are several places within DWARFContext.cpp that use WithColor::error()).

Your thoughts?

[vleschuk]

In D51033#1208987, @jhenderson wrote:

I'm thinking a bit more about this and I'm concerned that by adding these functions to Error.h, people will be more likely to use them from libraries, which can lead to various issues when the client of the library doesn't want to report errors and warnings the same way as this implements. The warn function was really only intended as a default implementation so that we didn't have to propagate things all the way out of DWARFContext.cpp, for the single client (llvm-dwarfdump) to consume.

Given that, I think the right thing to do is to just move the DWARFDebugLine::warn function out into the DWARFContext::dump function, and pass it around as necessary (or make it a static function in the file).

I like that idea, however, we also have dsymutil which was using DWARFDebugLine's implementation. I think it would be correct to leave warning as function in DWARFContext.h, not as static function.

If you go that approach, I'd be tempted to do the same with an error function too (note that there are several places within DWARFContext.cpp that use WithColor::error()).

While warnings are used in one way only and their purpose is mainly to handleAllErrors, error reporting format is not that unified...

[vleschuk]

• Removed all functions from Support library in order not to tempt other Support users
• dumpWarning() now lives in DWARFContext
• Had to get rid of default callbacks in DebugLine and DebugAddr to avoid circular dependency with DWARFContext

[vleschuk]

@jhenderson Here is the variant where dumpWarning() lives in DWARFContext. Patch became much simpler and smaller. What do you think?

[jhenderson]

Okay, LGTM.

include/llvm/DebugInfo/DWARF/DWARFContext.h

public:
   /// Improves readability of dumped DWARF expressions. Requires the caller to
   /// have initialized the relevant target descriptions.
   Error loadRegisterInfo(const object::ObjectFile &Obj);
 
   /// Get address size from CUs.
   /// TODO: refactor compile_units() to make this const.
   uint8_t getCUAddrSize();
 
+  /// Dump Error as warning message to stderr.
+  static void dumpWarning(Error Warning);
+
 private:
   /// Return the compile unit which contains instruction with provided
   /// address.
   DWARFCompileUnit *getCompileUnitForAddress(uint64_t Address);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_DEBUGINFO_DWARF_DWARFCONTEXT_H

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h

struct LineTable {
     /// corresponding to Address. Returns true on success.
     bool getFileLineInfoForAddress(uint64_t Address, const char *CompDir,
                                    DILineInfoSpecifier::FileLineInfoKind Kind,
                                    DILineInfo &Result) const;
 
     void dump(raw_ostream &OS, DIDumpOptions DumpOptions) const;
     void clear();
 
     /// Parse prologue and all rows.
-    Error parse(DWARFDataExtractor &DebugLineData, uint32_t *OffsetPtr,
-                const DWARFContext &Ctx, const DWARFUnit *U,
-                std::function<void(Error)> RecoverableErrorCallback = warn,
-                raw_ostream *OS = nullptr);
+    Error parse(
+        DWARFDataExtractor &DebugLineData, uint32_t *OffsetPtr,
+        std::function<void(Error)> RecoverableErrorCallback,
 
     using RowVector = std::vector<Row>;
     using RowIter = RowVector::const_iterator;
     using SequenceVector = std::vector<Sequence>;
     using SequenceIter = SequenceVector::const_iterator;
 
     struct Prologue Prologue;
     RowVector Rows;
     SequenceVector Sequences;
 
   private:
     uint32_t findRowInSeq(const DWARFDebugLine::Sequence &Seq,
                           uint64_t Address) const;
     Optional<StringRef>
     getSourceByIndex(uint64_t FileIndex,
                      DILineInfoSpecifier::FileLineInfoKind Kind) const;
   };
 
   const LineTable *getLineTable(uint32_t Offset) const;
   Expected<const LineTable *> getOrParseLineTable(
       DWARFDataExtractor &DebugLineData, uint32_t Offset,
       const DWARFContext &Ctx, const DWARFUnit *U,
-      std::function<void(Error)> RecoverableErrorCallback = warn);
+      std::function<void(Error)> RecoverableErrorCallback);
 
   /// Helper to allow for parsing of an entire .debug_line section in sequence.
   class SectionParser {
   public:
     using cu_range = DWARFUnitVector::iterator_range;
     using tu_range = DWARFUnitVector::iterator_range;
     using LineToUnitMap = std::map<uint64_t, DWARFUnit *>;
 
     SectionParser(DWARFDataExtractor &Data, const DWARFContext &C, cu_range CUs,
                   tu_range TUs);
 
     /// Get the next line table from the section. Report any issues via the
     /// callbacks.
     ///
     /// \param RecoverableErrorCallback - any issues that don't prevent further
     /// parsing of the table will be reported through this callback.
     /// \param UnrecoverableErrorCallback - any issues that prevent further
     /// parsing of the table will be reported through this callback.
     /// \param OS - if not null, the parser will print information about the
     /// table as it parses it.
     LineTable
-    parseNext(function_ref<void(Error)> RecoverableErrorCallback = warn,
-              function_ref<void(Error)> UnrecoverableErrorCallback = warn,
-              raw_ostream *OS = nullptr);
+    parseNext(
+        function_ref<void(Error)> RecoverableErrorCallback,
+        function_ref<void(Error)> UnrecoverableErrorCallback,
 
     /// Skip the current line table and go to the following line table (if
     /// present) immediately.
     ///
     /// \param ErrorCallback - report any prologue parsing issues via this
     /// callback.
-    void skip(function_ref<void(Error)> ErrorCallback = warn);
+    void skip(function_ref<void(Error)> ErrorCallback);
 
     /// Indicates if the parser has parsed as much as possible.
     ///
     /// \note Certain problems with the line table structure might mean that
     /// parsing stops before the end of the section is reached.
     bool done() const { return Done; }
 
     /// Get the offset the parser has reached.
     uint32_t getOffset() const { return Offset; }
 
   private:
     DWARFUnit *prepareToParse(uint32_t Offset);
     void moveToNextTable(uint32_t OldOffset, const Prologue &P);
 
     LineToUnitMap LineToUnit;
 
     DWARFDataExtractor &DebugLineData;
     const DWARFContext &Context;
     uint32_t Offset = 0;
     bool Done = false;
   };
 
-  /// Helper function for DWARFDebugLine parse functions, to report issues
-  /// identified during parsing.
-  ///
-  /// \param Err The Error to report.
-  static void warn(Error Err);
-
 private:
   struct ParsingState {
     ParsingState(struct LineTable *LT);
 
     void resetRowAndSequence();
     void appendRowToMatrix(uint32_t Offset);
 
     /// Line table we're currently parsing.

lib/DebugInfo/DWARF/DWARFContext.cpp

else {
     }
   }
 }
 
 // Dump the .debug_addr section.
 static void dumpAddrSection(raw_ostream &OS, DWARFDataExtractor &AddrData,
                             DIDumpOptions DumpOpts, uint16_t Version,
                             uint8_t AddrSize) {
-  // TODO: Make this more general: add callback types to Error.h, create
-  // implementation and make all DWARF classes use them.
-  static auto WarnCallback = [](Error Warn) {
-    handleAllErrors(std::move(Warn), [](ErrorInfoBase &Info) {
-      WithColor::warning() << Info.message() << '\n';
-    });
-  };
   uint32_t Offset = 0;
   while (AddrData.isValidOffset(Offset)) {
     DWARFDebugAddrTable AddrTable;
     uint32_t TableOffset = Offset;
-    if (Error Err = AddrTable.extract(AddrData, &Offset, Version,
-                                      AddrSize, WarnCallback)) {
+    if (Error Err = AddrTable.extract(AddrData, &Offset, Version, AddrSize,
+                                      DWARFContext::dumpWarning)) {
       WithColor::error() << toString(std::move(Err)) << '\n';
       // Keep going after an error, if we can, assuming that the length field
       // could be read. If it couldn't, stop reading the section.
       if (!AddrTable.hasValidLength())
         break;
       uint64_t Length = AddrTable.getLength();
       Offset = TableOffset + Length;
     } else {
if (shouldDump(Explicit, ".debug_aranges", DIDT_ID_DebugAranges,
     while (set.extract(arangesData, &offset))
       set.dump(OS);
   }
 
   auto DumpLineSection = [&](DWARFDebugLine::SectionParser Parser,
                              DIDumpOptions DumpOpts) {
     while (!Parser.done()) {
       if (DumpOffset && Parser.getOffset() != *DumpOffset) {
-        Parser.skip();
+        Parser.skip(dumpWarning);
         continue;
       }
       OS << "debug_line[" << format("0x%8.8x", Parser.getOffset()) << "]\n";
       if (DumpOpts.Verbose) {
-        Parser.parseNext(DWARFDebugLine::warn, DWARFDebugLine::warn, &OS);
+        Parser.parseNext(dumpWarning, dumpWarning, &OS);
       } else {
-        DWARFDebugLine::LineTable LineTable = Parser.parseNext();
+        DWARFDebugLine::LineTable LineTable =
+            Parser.parseNext(dumpWarning, dumpWarning);
         LineTable.dump(OS, DumpOpts);
       }
     }
   };
 
   if (shouldDump(Explicit, ".debug_line", DIDT_ID_DebugLine,
                  DObj->getLineSection().Data)) {
     DWARFDataExtractor LineData(*DObj, DObj->getLineSection(), isLittleEndian(),

 const AppleAcceleratorTable &DWARFContext::getAppleObjC() {
   return getAccelTable(AppleObjC, *DObj, DObj->getAppleObjCSection(),
                        DObj->getStringSection(), isLittleEndian());
 }
 
 const DWARFDebugLine::LineTable *
 DWARFContext::getLineTableForUnit(DWARFUnit *U) {
   Expected<const DWARFDebugLine::LineTable *> ExpectedLineTable =
-      getLineTableForUnit(U, DWARFDebugLine::warn);
+      getLineTableForUnit(U, dumpWarning);
   if (!ExpectedLineTable) {
-    DWARFDebugLine::warn(ExpectedLineTable.takeError());
+    dumpWarning(ExpectedLineTable.takeError());
     return nullptr;
   }
   return *ExpectedLineTable;
 }
 
 Expected<const DWARFDebugLine::LineTable *> DWARFContext::getLineTableForUnit(
     DWARFUnit *U, std::function<void(Error)> RecoverableErrorCallback) {
   if (!Line)
uint8_t DWARFContext::getCUAddrSize() {
   // them independently, not to enable varying the address size.
   uint8_t Addr = 0;
   for (const auto &CU : compile_units()) {
     Addr = CU->getAddressByteSize();
     break;
   }
   return Addr;
 }
+
+void DWARFContext::dumpWarning(Error Warning) {
+  handleAllErrors(std::move(Warning), [](ErrorInfoBase &Info) {
+      WithColor::warning() << Info.message() << '\n';
+  });
+}

lib/DebugInfo/DWARF/DWARFDebugLine.cpp

if (!P.totalLengthIsValid()) {
     return;
   }
 
   Offset = OldOffset + P.TotalLength + P.sizeofTotalLength();
   if (!DebugLineData.isValidOffset(Offset)) {
     Done = true;
   }
 }
-
-void DWARFDebugLine::warn(Error Err) {
-  handleAllErrors(std::move(Err), [](ErrorInfoBase &Info) {
-    WithColor::warning() << Info.message() << '\n';
-  });
-}

tools/dsymutil/DwarfLinker.cpp

void DwarfLinker::patchLineTableForUnit(CompileUnit &Unit,
   // Parse the original line info for the unit.
   DWARFDebugLine::LineTable LineTable;
   uint32_t StmtOffset = *StmtList;
   DWARFDataExtractor LineExtractor(
       OrigDwarf.getDWARFObj(), OrigDwarf.getDWARFObj().getLineSection(),
       OrigDwarf.isLittleEndian(), Unit.getOrigUnit().getAddressByteSize());
 
   Error Err = LineTable.parse(LineExtractor, &StmtOffset, OrigDwarf,
-                              &Unit.getOrigUnit());
-  DWARFDebugLine::warn(std::move(Err));
+                              &Unit.getOrigUnit(), DWARFContext::dumpWarning);
+  DWARFContext::dumpWarning(std::move(Err));
 
   // This vector is the output line table.
   std::vector<DWARFDebugLine::Row> NewRows;
   NewRows.reserve(LineTable.Rows.size());
 
   // Current sequence of rows being extracted, before being inserted
   // in NewRows.
   std::vector<DWARFDebugLine::Row> Seq;