NFC. Refactored DIPrinter for better support of new print styles.

[aorlov]

Summary

This patch introduces a DIPrinter interface to implement by different output style printer implementations. DIPrinterGNU and DIPrinterLLVM implement the GNU and LLVM output style printing respectively. No functional changes.

This refactoring clarifies and simplifies the code, and makes a new output style addition easier.

[jhenderson]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

31

I'd be inclined to omit the DI part of this name. We're in a distinct namespace, and I'm not sure the DI prefix gives us anything.

34

StringRef is designed to be passed by value, as it's already a reference, so is cheap to copy.

44

Maybe OutputStream and ErrorStream? ES in particular is not a common abbreviation, so it's hard to follow the naming.

55

See above re. StringRef

58

I can see why you have this class, but I'm not a fan of the name. Why is this "generic", but the base class isn't, for example? Also, see above re. the DI prefix.

How about PlainPrinterBase or something like that? The subclasses could be LLVMPrinter and GNUPrinter.

60

Perhaps we could reduce the number of changes needed in this main part of the refactoring by moving the address printing in a separate prerequisite patch? Does that make sense?

70

Why is this virtual and protected?

81

I believe you don't need this, since it's in the base class.

94–95

Here and everywhere else, use override wherever you are overriding a base class method.

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

202–203

This looks extremely brittle. Using invalid_argument as an error code is not unusual, and probably doesn't always correspond to the case you think it does.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

78

Why have you changed what this function does? What was wrong with the previous approach, where the main printing was handled by the calling code? I'd think you could just pass in the Printer, and then call Printer.printError in place of the logAllUnhandledErrors function.

94–96

This kind of special-casing shows that this isn't the best design for this code.

161–165

This probably wants to be a completely different function, since an unparseable command is not really an error. How about another function printInvalidCommand? That would help resolve the brittleness I mention above, too.

[aorlov]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

44

I have changed the OS and ES to the requested names. On my taste the code became less readable, as the lines are longer and more line breaks are involved. Personally I’d prefer shorter names, OS is already used name people got used to, and ES is in line.

60

I can propose that small patch. Though it does not seem worth it as it will save just a few lines in this one, wouldn’t make a big difference.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

78

I didn’t change the error() function.

I have implemented a new function which effectively does print-res-or-err and implements the logic from main which used to be a sequence of branches specific to different data types. Since the handling of different data types are similar, we could use the template machinery to highlight the similarity. This produces a smaller cleaner code.

Printing out errors is a trivial one-liner, and does not deserve a separate function. That’s a small part of this new print-res-or-err function.

I have named this print-res-or-err function “print”, since I think it is a better name. print(ResOrErr) looks clean and right to the point. Much better than printResOrErr(ResOrErr).

94–96

You are right. We do not need this special case here.

161–165

since an unparseable command is not really an error

What is so special with the unparseable command besides someone attached some questionable logic to that case in the current implementation? This is one of regular errors, which shall be reported with regards to the specified output style.
It might look a bit different just because it is detected here and not returned by a library code. Does not make it something other than error, though.
Weirdness here is because of the bug-to-bug backward compatibility.

[jhenderson]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

34

Similarly, no need for the const anymore. Please delete.

44

Having seen the impact, I regret asking you to do this. If you prefer to use OS and ES, it's fine to change back.

57

60

My thinking is that it is actually a beneficial change, even without the other refactoring. It would be good to minimise noise as much as possible.

78–80

I think it would be a good idea to create a helper PrinterConfig class or similar, which contains the various Print* and Verbose argument. It will make the interface cleaner and easier to extend in the future.

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

42–43

123–124

If I'm reading this function correctly, I think you can push most of it into the base class, because it's very similar to the LLVM style. If you add a few new functions, something like printFunctionName, printSimpleLocation (which would take a filename at least), and printVerbose, with the printSimplyLocation implemented by the subclasses and the base class implementing the other two. The base class print would then call these functions in sequence.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

161–165

Weirdness here is because of the bug-to-bug backward compatibility.

One person's bug is another person's feature. For example, you might want to print some custom delimiters between symbolized addresses.

In the existing printers, the invalid input is not considered an error, whereas it might be for e.g. JSON. I therefore think you should have two functions: printInvalidCommand and printError. For the JSON implementation, printInvalidCommand would just forward straight to printError, saying something like "invalid input addresses are not accepted with JSON output style" (or possibly stuffing it in a JSON object with some unique tag or something. For the LLVM and GNU implementations, the function would just print and then return without needing to do anything special. It helps simplify the printError logic, which I find to be somewhat complicated for an error handling function, if I'm honest.

[aorlov]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

78–80

Adding PrinterConfig does not seem to make this patch better. It would be the same set of flags but passed in to the PrinterConfig constructor, which in its turn will be passed to a DIPrinter instance a line or two below that.

Here is the code snippet to illustrate:

PrinterConfig PrnCfg(Args.hasArg(OPT_addresses),
                     Opts.PrintFunctions != FunctionNameKind::None,
                     Args.hasArg(OPT_pretty_print),
                     SourceContextLines, Args.hasArg(OPT_verbose));

if (OutputStyle == DIPrinter::OutputStyle::GNU)
  Printer.reset(new GNUPrinter(outs(), errs(), PrnCfg));

+ PrinterConfig definition noise.

Note JSONPrinter does not require PrinterConfig at all.

I do not oppose the idea of having PrinterConfig, but I do oppose the idea of making it a dependency to this patch. If someone feels strong for having such class, it could be added later out of the scope of this patch.

[jhenderson]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

78–80

You don't have to have a constructor for the PrinterConfig class. Instead, you do something like the following:

class PrinterConfig {
public:
  bool PrintAddress = false; // Default initializer is optional.
  bool PrintFunctions = false;
  ...
};

void setupConfig(...)
{
  PrinterConfig Config;
  Config.PrintAddress = Args.hasArg(OPT_addresses);
  Config.PrintFunctions = Opts.PrintFunctions != FunctionNameKind::None;
  Config.ES = errs();
  ...
}

The difference is that you no longer have a list of boolean arguments that can easily end up in the wrong order. It also avoids potential for things like this:

LLVMPrinter(outs(), errs(), true, false, true, 0, false);

which I hope you'll agree is not all that legible.

This is similar to how the LLD config is created - some options are default initialised, and then argument parsing populates the remainder.

As this is about making the Printer interface nicer, it is part of this patch.

It's also worth pointing out that this removes the duplication of the big argument list where you construct the LLVM/GNUPrinter instances in the llvm-symbolizer code.

[aorlov]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

78–80

Note PrinterConfig is PlainPrinterBase specific, not the Printer interface.
So I pass PrinterConfig to LLVM/GNUPrinter constructors. We don't need it for JSONPrinter.
I kept OS and ES as is, because Config.OS is very similar to OutputStream story.

[aorlov]

Just for the record.
The build status is red because of a problem not related to this patch.

[aorlov]

Ping.

[dblaikie]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

34–35

I probably wouldn't bother with a ctor here - uses can use braced init to construct it with a similar amount of syntax.

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

333–355

If the DIPrinter represents whether to use GNU or LLVM style printing - I don't think the output style should otherwise be exposed to the code (if it is, it seems to suggest that DIPrinter doesn't sufficiently encapsulate the concept of output styling)

Then that DIPrinter::OutputStyle enum could move into llvm-symbolizer.cpp since it won't need to be known by any other code.

334–336

Probably use std::make_unique here

[aorlov]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

34–35

I have removed any streams from DIPrinter. Now it is a clean interface.
Note we don't need stderr in JSONPrinter.
And any other printer might not use the stdout and/or stderr at all.
But I kept OS and ES in PlainPrinterBase ctor.
I don't see how to gracefully use braced init of PlainPrinterBase.

[dblaikie]

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

34–35

Sorry, I don't think my suggestion has anything to do with OS and ES.

I mean remove the Request ctor, then later on where there's currently Printer.print(Request(ModuleName, Address), *ResOrErr); it can be replaced with Printer.print({ModuleName, Address}, *ResOrErr);

[dblaikie]

Looks good to me - one optional rename. I don't have a great idea of an alternative though.

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

63

Might be worth renaming this - I'm not sure "Plain" is obviously "not JSON or YAML or whatever". Human readable? Something else?

[jhenderson]

Thanks for wrapping up this review @dblaikie. Apologies for disappearing @aorlov. I was already pretty buried with work, and then I've just had two weeks off work, so am only now beginning to catch up on reviews.

llvm/include/llvm/DebugInfo/Symbolize/DIPrinter.h

 //===- llvm/DebugInfo/Symbolize/DIPrinter.h ---------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file declares the DIPrinter class, which is responsible for printing
 // structures defined in DebugInfo/DIContext.h
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_DEBUGINFO_SYMBOLIZE_DIPRINTER_H
 #define LLVM_DEBUGINFO_SYMBOLIZE_DIPRINTER_H
 
+#include "llvm/ADT/StringRef.h"
 #include <string>
+#include <vector>
 
 namespace llvm {
 struct DILineInfo;
 class DIInliningInfo;
 struct DIGlobal;
 struct DILocal;
+class ErrorInfoBase;
 class raw_ostream;
 
 namespace symbolize {
 
+struct Request {
+  StringRef ModuleName;
+  uint64_t Address = 0;
+};
+
 class DIPrinter {
 public:
-  enum class OutputStyle { LLVM, GNU };
+  DIPrinter(){};
+  virtual ~DIPrinter(){};
 
-private:
-  raw_ostream &OS;
-  bool PrintFunctionNames;
-  bool PrintPretty;
-  int PrintSourceContext;
+  virtual void print(const Request &Request, const DILineInfo &Info) = 0;
+  virtual void print(const Request &Request, const DIInliningInfo &Info) = 0;
+  virtual void print(const Request &Request, const DIGlobal &Global) = 0;
+  virtual void print(const Request &Request,
+                     const std::vector<DILocal> &Locals) = 0;
+
+  virtual void printInvalidCommand(const Request &Request,
+                                   const ErrorInfoBase &ErrorInfo) = 0;
+
+  virtual bool printError(const Request &Request,
+                          const ErrorInfoBase &ErrorInfo,
+                          StringRef ErrorBanner) = 0;
+};
+
+struct PrinterConfig {
+  bool PrintAddress;
+  bool PrintFunctions;
+  bool Pretty;
   bool Verbose;
-  OutputStyle Style;
+  int SourceContextLines;
+};
+
+class PlainPrinterBase : public DIPrinter {
+protected:
+  raw_ostream &OS;
+  raw_ostream &ES;
+  PrinterConfig Config;
 
   void print(const DILineInfo &Info, bool Inlined);
-  void printContext(const std::string &FileName, int64_t Line);
+  void printFunctionName(StringRef FunctionName, bool Inlined);
+  virtual void printSimpleLocation(StringRef Filename,
+                                   const DILineInfo &Info) = 0;
+  void printContext(StringRef FileName, int64_t Line);
+  void printVerbose(StringRef Filename, const DILineInfo &Info);
+  virtual void printFooter() {}
+
+private:
+  void printHeader(uint64_t Address);
+
+public:
+  PlainPrinterBase(raw_ostream &OS, raw_ostream &ES, PrinterConfig &Config)
+      : DIPrinter(), OS(OS), ES(ES), Config(Config) {}
+
+  void print(const Request &Request, const DILineInfo &Info) override;
+  void print(const Request &Request, const DIInliningInfo &Info) override;
+  void print(const Request &Request, const DIGlobal &Global) override;
+  void print(const Request &Request,
+             const std::vector<DILocal> &Locals) override;
+
+  void printInvalidCommand(const Request &Request,
+                           const ErrorInfoBase &ErrorInfo) override;
+
+  bool printError(const Request &Request, const ErrorInfoBase &ErrorInfo,
+                  StringRef ErrorBanner) override;
+};
+
+class LLVMPrinter : public PlainPrinterBase {
+private:
+  void printSimpleLocation(StringRef Filename, const DILineInfo &Info) override;
+  void printFooter() override;
+
+public:
+  LLVMPrinter(raw_ostream &OS, raw_ostream &ES, PrinterConfig &Config)
+      : PlainPrinterBase(OS, ES, Config) {}
+};
+
+class GNUPrinter : public PlainPrinterBase {
+private:
+  void printSimpleLocation(StringRef Filename, const DILineInfo &Info) override;
 
 public:
-  DIPrinter(raw_ostream &OS, bool PrintFunctionNames = true,
-            bool PrintPretty = false, int PrintSourceContext = 0,
-            bool Verbose = false, OutputStyle Style = OutputStyle::LLVM)
-      : OS(OS), PrintFunctionNames(PrintFunctionNames),
-        PrintPretty(PrintPretty), PrintSourceContext(PrintSourceContext),
-        Verbose(Verbose), Style(Style) {}
-
-  DIPrinter &operator<<(const DILineInfo &Info);
-  DIPrinter &operator<<(const DIInliningInfo &Info);
-  DIPrinter &operator<<(const DIGlobal &Global);
-  DIPrinter &operator<<(const DILocal &Local);
+  GNUPrinter(raw_ostream &OS, raw_ostream &ES, PrinterConfig &Config)
+      : PlainPrinterBase(OS, ES, Config) {}
 };
-}
-}
+} // namespace symbolize
+} // namespace llvm
 
 #endif

llvm/lib/DebugInfo/Symbolize/DIPrinter.cpp

 #include <cstddef>
 #include <cstdint>
 #include <memory>
 #include <string>
 
 namespace llvm {
 namespace symbolize {
 
+void PlainPrinterBase::printHeader(uint64_t Address) {
+  if (Config.PrintAddress) {
+    OS << "0x";
+    OS.write_hex(Address);
+    StringRef Delimiter = Config.Pretty ? ": " : "\n";
+    OS << Delimiter;
+  }
+}
+
 // Prints source code around in the FileName the Line.
-void DIPrinter::printContext(const std::string &FileName, int64_t Line) {
-  if (PrintSourceContext <= 0)
+void PlainPrinterBase::printContext(StringRef FileName, int64_t Line) {
+  if (Config.SourceContextLines <= 0)
     return;
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
       MemoryBuffer::getFile(FileName);
   if (!BufOrErr)
     return;
 
   std::unique_ptr<MemoryBuffer> Buf = std::move(BufOrErr.get());
   int64_t FirstLine =
-      std::max(static_cast<int64_t>(1), Line - PrintSourceContext / 2);
-  int64_t LastLine = FirstLine + PrintSourceContext;
+      std::max(static_cast<int64_t>(1), Line - Config.SourceContextLines / 2);
+  int64_t LastLine = FirstLine + Config.SourceContextLines;
   size_t MaxLineNumberWidth = std::ceil(std::log10(LastLine));
 
   for (line_iterator I = line_iterator(*Buf, false);
        !I.is_at_eof() && I.line_number() <= LastLine; ++I) {
     int64_t L = I.line_number();
     if (L >= FirstLine && L <= LastLine) {
       OS << format_decimal(L, MaxLineNumberWidth);
       if (L == Line)
         OS << " >: ";
       else
         OS << "  : ";
       OS << *I << "\n";
     }
   }
 }
 
-void DIPrinter::print(const DILineInfo &Info, bool Inlined) {
-  if (PrintFunctionNames) {
-    std::string FunctionName = Info.FunctionName;
+void PlainPrinterBase::printFunctionName(StringRef FunctionName, bool Inlined) {
+  if (Config.PrintFunctions) {
     if (FunctionName == DILineInfo::BadString)
       FunctionName = DILineInfo::Addr2LineBadString;
-
-    StringRef Delimiter = PrintPretty ? " at " : "\n";
-    StringRef Prefix = (PrintPretty && Inlined) ? " (inlined by) " : "";
+    StringRef Delimiter = Config.Pretty ? " at " : "\n";
+    StringRef Prefix = (Config.Pretty && Inlined) ? " (inlined by) " : "";
     OS << Prefix << FunctionName << Delimiter;
   }
-  std::string Filename = Info.FileName;
-  if (Filename == DILineInfo::BadString)
-    Filename = DILineInfo::Addr2LineBadString;
-  if (!Verbose) {
-    OS << Filename << ":" << Info.Line;
-    if (Style == OutputStyle::LLVM)
-      OS << ":" << Info.Column;
-    else if (Style == OutputStyle::GNU && Info.Discriminator != 0)
-      OS << " (discriminator " << Info.Discriminator << ")";
-    OS << "\n";
-    printContext(Filename, Info.Line);
-    return;
-  }
-  OS << "  Filename: " << Filename << "\n";
+}
+
+void LLVMPrinter::printSimpleLocation(StringRef Filename,
+                                      const DILineInfo &Info) {
+  OS << Filename << ':' << Info.Line << ':' << Info.Column << '\n';
+
+void GNUPrinter::printSimpleLocation(StringRef Filename,
+                                     const DILineInfo &Info) {
+  OS << Filename << ':' << Info.Line;
+  if (Info.Discriminator)
+    OS << " (discriminator " << Info.Discriminator << ')';
+  OS << '\n';
+  printContext(Filename, Info.Line);
+}
+
+void PlainPrinterBase::printVerbose(StringRef Filename,
+                                    const DILineInfo &Info) {
+  OS << "  Filename: " << Filename << '\n';
   if (Info.StartLine) {
-    OS << "  Function start filename: " << Info.StartFileName << "\n";
-    OS << "  Function start line: " << Info.StartLine << "\n";
+    OS << "  Function start filename: " << Info.StartFileName << '\n';
+    OS << "  Function start line: " << Info.StartLine << '\n';
   }
-  OS << "  Line: " << Info.Line << "\n";
-  OS << "  Column: " << Info.Column << "\n";
+  OS << "  Line: " << Info.Line << '\n';
+  OS << "  Column: " << Info.Column << '\n';
   if (Info.Discriminator)
-    OS << "  Discriminator: " << Info.Discriminator << "\n";
+    OS << "  Discriminator: " << Info.Discriminator << '\n';
 }
 
-DIPrinter &DIPrinter::operator<<(const DILineInfo &Info) {
+void LLVMPrinter::printFooter() { OS << '\n'; }
+
+void PlainPrinterBase::print(const DILineInfo &Info, bool Inlined) {
+  printFunctionName(Info.FunctionName, Inlined);
+  StringRef Filename = Info.FileName;
+  if (Filename == DILineInfo::BadString)
+    Filename = DILineInfo::Addr2LineBadString;
+  if (Config.Verbose)
+    printVerbose(Filename, Info);
+  else
+    printSimpleLocation(Filename, Info);
+}
+
+void PlainPrinterBase::print(const Request &Request, const DILineInfo &Info) {
+  printHeader(Request.Address);
   print(Info, false);
-  return *this;
+  printFooter();
 }
 
-DIPrinter &DIPrinter::operator<<(const DIInliningInfo &Info) {
+void PlainPrinterBase::print(const Request &Request,
+                             const DIInliningInfo &Info) {
+  printHeader(Request.Address);
   uint32_t FramesNum = Info.getNumberOfFrames();
-  if (FramesNum == 0) {
+  if (FramesNum == 0)
     print(DILineInfo(), false);
-    return *this;
-  }
-  for (uint32_t i = 0; i < FramesNum; i++)
-    print(Info.getFrame(i), i > 0);
-  return *this;
+  else
+    for (uint32_t I = 0; I < FramesNum; ++I)
+      print(Info.getFrame(I), I > 0);
+  printFooter();
 }
 
-DIPrinter &DIPrinter::operator<<(const DIGlobal &Global) {
-  std::string Name = Global.Name;
+void PlainPrinterBase::print(const Request &Request, const DIGlobal &Global) {
+  printHeader(Request.Address);
+  StringRef Name = Global.Name;
   if (Name == DILineInfo::BadString)
     Name = DILineInfo::Addr2LineBadString;
   OS << Name << "\n";
   OS << Global.Start << " " << Global.Size << "\n";
-  return *this;
+  printFooter();
 }
 
-DIPrinter &DIPrinter::operator<<(const DILocal &Local) {
-  if (Local.FunctionName.empty())
-    OS << "??\n";
+void PlainPrinterBase::print(const Request &Request,
+                             const std::vector<DILocal> &Locals) {
+  printHeader(Request.Address);
+  if (Locals.empty())
+    OS << DILineInfo::Addr2LineBadString << '\n';
   else
-    OS << Local.FunctionName << '\n';
+    for (const DILocal &L : Locals) {
+      if (L.FunctionName.empty())
+        OS << DILineInfo::Addr2LineBadString;
+      else
+        OS << L.FunctionName;
+      OS << '\n';
 
-  if (Local.Name.empty())
-    OS << "??\n";
+      if (L.Name.empty())
+        OS << DILineInfo::Addr2LineBadString;
       else
-    OS << Local.Name << '\n';
+        OS << L.Name;
+      OS << '\n';
 
-  if (Local.DeclFile.empty())
-    OS << "??";
+      if (L.DeclFile.empty())
+        OS << DILineInfo::Addr2LineBadString;
       else
-    OS << Local.DeclFile;
-  OS << ':' << Local.DeclLine << '\n';
+        OS << L.DeclFile;
+
+      OS << ':' << L.DeclLine << '\n';
 
-  if (Local.FrameOffset)
-    OS << *Local.FrameOffset << ' ';
+      if (L.FrameOffset)
+        OS << *L.FrameOffset;
       else
-    OS << "?? ";
+        OS << DILineInfo::Addr2LineBadString;
+      OS << ' ';
 
-  if (Local.Size)
-    OS << *Local.Size << ' ';
+      if (L.Size)
+        OS << *L.Size;
       else
-    OS << "?? ";
+        OS << DILineInfo::Addr2LineBadString;
+      OS << ' ';
 
-  if (Local.TagOffset)
-    OS << *Local.TagOffset << '\n';
+      if (L.TagOffset)
+        OS << *L.TagOffset;
       else
-    OS << "??\n";
-  return *this;
+        OS << DILineInfo::Addr2LineBadString;
+      OS << '\n';
+    }
+  printFooter();
+}
+
+void PlainPrinterBase::printInvalidCommand(const Request &Request,
+                                           const ErrorInfoBase &ErrorInfo) {
+  OS << ErrorInfo.message() << '\n';
+}
+
+bool PlainPrinterBase::printError(const Request &Request,
+                                  const ErrorInfoBase &ErrorInfo,
+                                  StringRef ErrorBanner) {
+  ES << ErrorBanner;
+  ErrorInfo.log(ES);
+  ES << '\n';
+  // Print an empty struct too.
+  return true;
 }
 
 } // end namespace symbolize
 } // end namespace llvm

llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp

public:
   SymbolizerOptTable() : OptTable(InfoTable, true) {}
 };
 } // namespace
 
 static cl::list<std::string> ClInputAddresses(cl::Positional,
                                               cl::desc("<input addresses>..."),
                                               cl::ZeroOrMore);
 
-template<typename T>
-static bool error(Expected<T> &ResOrErr) {
-  if (ResOrErr)
-    return false;
-  logAllUnhandledErrors(ResOrErr.takeError(), errs(),
-                        "LLVMSymbolizer: error reading file: ");
-  return true;
+template <typename T>
+static void print(const Request &Request, Expected<T> &ResOrErr,
+                  DIPrinter &Printer) {
+  if (ResOrErr) {
+    // No error, print the result.
+    Printer.print(Request, *ResOrErr);
+    return;
+  }
+
+  // Handle the error.
+  bool PrintEmpty = true;
+  handleAllErrors(std::move(ResOrErr.takeError()),
+                  [&](const ErrorInfoBase &EI) {
+                    PrintEmpty = Printer.printError(
+                        Request, EI, "LLVMSymbolizer: error reading file: ");
+                  });
+
+  if (PrintEmpty)
+    Printer.print(Request, T());
 }
 
+enum class OutputStyle { LLVM, GNU };
+
 enum class Command {
   Code,
   Data,
   Frame,
 };
 
 static bool parseCommand(StringRef BinaryName, bool IsAddr2Line,
                          StringRef InputString, Command &Cmd,
static bool parseCommand(StringRef BinaryName, bool IsAddr2Line,
   // GNU addr2line assumes the offset is hexadecimal and allows a redundant
   // "0x" or "0X" prefix; do the same for compatibility.
   if (IsAddr2Line)
     Offset.consume_front("0x") || Offset.consume_front("0X");
   return !Offset.getAsInteger(IsAddr2Line ? 16 : 0, ModuleOffset);
 }
 
 static void symbolizeInput(const opt::InputArgList &Args, uint64_t AdjustVMA,
-                           bool IsAddr2Line, DIPrinter::OutputStyle OutputStyle,
+                           bool IsAddr2Line, OutputStyle OutputStyle,
                            StringRef InputString, LLVMSymbolizer &Symbolizer,
                            DIPrinter &Printer) {
   Command Cmd;
   std::string ModuleName;
   uint64_t Offset = 0;
   if (!parseCommand(Args.getLastArgValue(OPT_obj_EQ), IsAddr2Line,
                     StringRef(InputString), Cmd, ModuleName, Offset)) {
-    outs() << InputString << "\n";
+    Printer.printInvalidCommand(
+        {ModuleName, Offset},
+        StringError(InputString,
+                    std::make_error_code(std::errc::invalid_argument)));
     return;
   }
 
-  if (Args.hasArg(OPT_addresses)) {
-    outs() << "0x";
-    outs().write_hex(Offset);
-    StringRef Delimiter = Args.hasArg(OPT_pretty_print) ? ": " : "\n";
-    outs() << Delimiter;
-  }
-  Offset -= AdjustVMA;
+  uint64_t AdjustedOffset = Offset - AdjustVMA;
   if (Cmd == Command::Data) {
-    auto ResOrErr = Symbolizer.symbolizeData(
-        ModuleName, {Offset, object::SectionedAddress::UndefSection});
-    Printer << (error(ResOrErr) ? DIGlobal() : ResOrErr.get());
+    Expected<DIGlobal> ResOrErr = Symbolizer.symbolizeData(
+        ModuleName, {AdjustedOffset, object::SectionedAddress::UndefSection});
+    print({ModuleName, Offset}, ResOrErr, Printer);
   } else if (Cmd == Command::Frame) {
-    auto ResOrErr = Symbolizer.symbolizeFrame(
-        ModuleName, {Offset, object::SectionedAddress::UndefSection});
-    if (!error(ResOrErr)) {
-      for (DILocal Local : *ResOrErr)
-        Printer << Local;
-      if (ResOrErr->empty())
-        outs() << "??\n";
-    }
+    Expected<std::vector<DILocal>> ResOrErr = Symbolizer.symbolizeFrame(
+        ModuleName, {AdjustedOffset, object::SectionedAddress::UndefSection});
+    print({ModuleName, Offset}, ResOrErr, Printer);
   } else if (Args.hasFlag(OPT_inlines, OPT_no_inlines, !IsAddr2Line)) {
-    auto ResOrErr = Symbolizer.symbolizeInlinedCode(
-        ModuleName, {Offset, object::SectionedAddress::UndefSection});
-    Printer << (error(ResOrErr) ? DIInliningInfo() : ResOrErr.get());
-  } else if (OutputStyle == DIPrinter::OutputStyle::GNU) {
+    Expected<DIInliningInfo> ResOrErr = Symbolizer.symbolizeInlinedCode(
+        ModuleName, {AdjustedOffset, object::SectionedAddress::UndefSection});
+    print({ModuleName, Offset}, ResOrErr, Printer);
+  } else if (OutputStyle == OutputStyle::GNU) {
     // With PrintFunctions == FunctionNameKind::LinkageName (default)
     // and UseSymbolTable == true (also default), Symbolizer.symbolizeCode()
     // may override the name of an inlined function with the name of the topmost
     // caller function in the inlining chain. This contradicts the existing
     // behavior of addr2line. Symbolizer.symbolizeInlinedCode() overrides only
     // the topmost function, which suits our needs better.
-    auto ResOrErr = Symbolizer.symbolizeInlinedCode(
-        ModuleName, {Offset, object::SectionedAddress::UndefSection});
-    if (!ResOrErr || ResOrErr->getNumberOfFrames() == 0) {
-      error(ResOrErr);
-      Printer << DILineInfo();
-    } else {
-      Printer << ResOrErr->getFrame(0);
-    }
+    Expected<DIInliningInfo> ResOrErr = Symbolizer.symbolizeInlinedCode(
+        ModuleName, {AdjustedOffset, object::SectionedAddress::UndefSection});
+    Expected<DILineInfo> Res0OrErr =
+        !ResOrErr
+            ? Expected<DILineInfo>(ResOrErr.takeError())
+            : ((ResOrErr->getNumberOfFrames() == 0) ? DILineInfo()
+                                                    : ResOrErr->getFrame(0));
+    print({ModuleName, Offset}, Res0OrErr, Printer);
   } else {
-    auto ResOrErr = Symbolizer.symbolizeCode(
-        ModuleName, {Offset, object::SectionedAddress::UndefSection});
-    Printer << (error(ResOrErr) ? DILineInfo() : ResOrErr.get());
+    Expected<DILineInfo> ResOrErr = Symbolizer.symbolizeCode(
+        ModuleName, {AdjustedOffset, object::SectionedAddress::UndefSection});
+    print({ModuleName, Offset}, ResOrErr, Printer);
   }
-  if (OutputStyle == DIPrinter::OutputStyle::LLVM)
-    outs() << "\n";
 }
 
 static void printHelp(StringRef ToolName, const SymbolizerOptTable &Tbl,
                       raw_ostream &OS) {
   const char HelpText[] = " [options] addresses...";
   Tbl.PrintHelp(OS, (ToolName + HelpText).str().c_str(),
                 ToolName.str().c_str());
   // TODO Replace this with OptTable API once it adds extrahelp support.
int main(int argc, char **argv) {
   bool IsAddr2Line = sys::path::stem(argv[0]).contains("addr2line");
   BumpPtrAllocator A;
   StringSaver Saver(A);
   SymbolizerOptTable Tbl;
   opt::InputArgList Args = parseOptions(argc, argv, IsAddr2Line, Saver, Tbl);
 
   LLVMSymbolizer::Options Opts;
   uint64_t AdjustVMA;
-  unsigned SourceContextLines;
+  PrinterConfig Config;
   parseIntArg(Args, OPT_adjust_vma_EQ, AdjustVMA);
   if (const opt::Arg *A = Args.getLastArg(OPT_basenames, OPT_relativenames)) {
     Opts.PathStyle =
         A->getOption().matches(OPT_basenames)
             ? DILineInfoSpecifier::FileLineInfoKind::BaseNameOnly
             : DILineInfoSpecifier::FileLineInfoKind::RelativeFilePath;
   } else {
     Opts.PathStyle = DILineInfoSpecifier::FileLineInfoKind::AbsoluteFilePath;
   }
   Opts.DebugFileDirectory = Args.getAllArgValues(OPT_debug_file_directory_EQ);
   Opts.DefaultArch = Args.getLastArgValue(OPT_default_arch_EQ).str();
   Opts.Demangle = Args.hasFlag(OPT_demangle, OPT_no_demangle, !IsAddr2Line);
   Opts.DWPName = Args.getLastArgValue(OPT_dwp_EQ).str();
   Opts.FallbackDebugPath =
       Args.getLastArgValue(OPT_fallback_debug_path_EQ).str();
   Opts.PrintFunctions = decideHowToPrintFunctions(Args, IsAddr2Line);
-  parseIntArg(Args, OPT_print_source_context_lines_EQ, SourceContextLines);
+  parseIntArg(Args, OPT_print_source_context_lines_EQ,
+              Config.SourceContextLines);
   Opts.RelativeAddresses = Args.hasArg(OPT_relative_address);
   Opts.UntagAddresses =
       Args.hasFlag(OPT_untag_addresses, OPT_no_untag_addresses, !IsAddr2Line);
   Opts.UseDIA = Args.hasArg(OPT_use_dia);
 #if !defined(LLVM_ENABLE_DIA_SDK)
   if (Opts.UseDIA) {
     WithColor::warning() << "DIA not available; using native PDB reader\n";
     Opts.UseDIA = false;
   }
 #endif
   Opts.UseSymbolTable = true;
+  Config.PrintAddress = Args.hasArg(OPT_addresses);
+  Config.PrintFunctions = Opts.PrintFunctions != FunctionNameKind::None;
+  Config.Pretty = Args.hasArg(OPT_pretty_print);
+  Config.Verbose = Args.hasArg(OPT_verbose);
 
   for (const opt::Arg *A : Args.filtered(OPT_dsym_hint_EQ)) {
     StringRef Hint(A->getValue());
     if (sys::path::extension(Hint) == ".dSYM") {
       Opts.DsymHints.emplace_back(Hint);
     } else {
       errs() << "Warning: invalid dSYM hint: \"" << Hint
              << "\" (must have the '.dSYM' extension).\n";
     }
   }
 
-  auto OutputStyle =
-      IsAddr2Line ? DIPrinter::OutputStyle::GNU : DIPrinter::OutputStyle::LLVM;
+  auto OutputStyle = IsAddr2Line ? OutputStyle::GNU : OutputStyle::LLVM;
   if (const opt::Arg *A = Args.getLastArg(OPT_output_style_EQ)) {
-    OutputStyle = strcmp(A->getValue(), "GNU") == 0
-                      ? DIPrinter::OutputStyle::GNU
-                      : DIPrinter::OutputStyle::LLVM;
+    OutputStyle = strcmp(A->getValue(), "GNU") == 0 ? OutputStyle::GNU
+                                                    : OutputStyle::LLVM;
   }
 
   LLVMSymbolizer Symbolizer(Opts);
-  DIPrinter Printer(outs(), Opts.PrintFunctions != FunctionNameKind::None,
-                    Args.hasArg(OPT_pretty_print), SourceContextLines,
-                    Args.hasArg(OPT_verbose), OutputStyle);
+  std::unique_ptr<DIPrinter> Printer;
+  if (OutputStyle == OutputStyle::GNU)
+    Printer = std::make_unique<GNUPrinter>(outs(), errs(), Config);
+  else
+    Printer = std::make_unique<LLVMPrinter>(outs(), errs(), Config);
 
   std::vector<std::string> InputAddresses = Args.getAllArgValues(OPT_INPUT);
   if (InputAddresses.empty()) {
     const int kMaxInputStringLength = 1024;
     char InputString[kMaxInputStringLength];
 
     while (fgets(InputString, sizeof(InputString), stdin)) {
       // Strip newline characters.
       std::string StrippedInputString(InputString);
       llvm::erase_if(StrippedInputString,
                      [](char c) { return c == '\r' || c == '\n'; });
       symbolizeInput(Args, AdjustVMA, IsAddr2Line, OutputStyle,
-                     StrippedInputString, Symbolizer, Printer);
+                     StrippedInputString, Symbolizer, *Printer);
       outs().flush();
     }
   } else {
     for (StringRef Address : InputAddresses)
       symbolizeInput(Args, AdjustVMA, IsAddr2Line, OutputStyle, Address,
-                     Symbolizer, Printer);
+                     Symbolizer, *Printer);
   }
 
   return 0;
 }