[llvm-pdbutil] Move global state (Filters) inside LinePrinter class.

[CarlosAlbertoEnciso]

Summary

The changes described by:

https://reviews.llvm.org/D121801
https://reviews.llvm.org/D122226

Moved some llvm-pdbutil functionality to the debug PDB library.

This patch addresses one outstanding issue concerning the global
state (Filters) created in the PDB library.

[CarlosAlbertoEnciso]

@aganea this patch addresses the issue you mentioned in https://reviews.llvm.org/D122226#inline-1170048

[aganea]

Thanks for following up on this @CarlosAlbertoEnciso!

llvm/include/llvm/DebugInfo/PDB/Native/InputFile.h

182

There's no need to pass the Printer here, you already have a reference in HeaderScope.

202

Could you rather pass Printer.Filters as an argument rather?

216

Same here and below, HeaderScope.P could be used instead of passing it by arg.

llvm/include/llvm/DebugInfo/PDB/Native/LinePrinter.h

89

I think you can leave this function where it was initially, and instead just pass Filters as an argument at the call site. It doesn't need to know about the LinePrinter since they only need the Filters.

103

On a second thought, I think it'd better if this was a (const?) reference (FilterOptions &Filters), to make it explicit that the data is held somewhere else (by the caller).

llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp

484 ↗

(On Diff #419681)

You shouldn't be needing those changes from here below.

[CarlosAlbertoEnciso]

Address feedback from @aganea:

Main points:

• Use the Printer which is already in HeaderScope.
• Keep shouldDumpSymbolGroup in the original location and pass Filters as argument.

[CarlosAlbertoEnciso]

llvm/include/llvm/DebugInfo/PDB/Native/InputFile.h

182

Very good point.

202

Now I am passing the Filters. I have to add extra logic because Filters should be optional<..>.

llvm/include/llvm/DebugInfo/PDB/Native/LinePrinter.h

89

The function has been moved to its original location.

103

Now is const reference.

[CarlosAlbertoEnciso]

Fix clang-format warning.

[aganea]

Do you think it would be possible to omit Optional and just pass PrintScope &HeaderScope everywhere? It feels like without the printer, these functions don't have much purpose?

[CarlosAlbertoEnciso]

As suggested by @aganea,

• Omit Optional and just pass PrintScope &HeaderScope everywhere.

[aganea]

LGTM, thanks again @CarlosAlbertoEnciso !

[CarlosAlbertoEnciso]

Thanks for your review @aganea

[aganea]

In D122887#3438999, @CarlosAlbertoEnciso wrote:

Thanks for your review @aganea

Thank you for your work and the patience! :-)

[CarlosAlbertoEnciso]

In D122887#3439017, @aganea wrote:

In D122887#3438999, @CarlosAlbertoEnciso wrote:

Thanks for your review @aganea

Thank you for your work and the patience! :-)

Happy to help. :-)

llvm/include/llvm/DebugInfo/PDB/Native/InputFile.h

private:
   SymbolGroup Value;
 };
 
 Expected<ModuleDebugStreamRef>
 getModuleDebugStream(PDBFile &File, StringRef &ModuleName, uint32_t Index);
 Expected<ModuleDebugStreamRef> getModuleDebugStream(PDBFile &File,
                                                     uint32_t Index);
 
-bool shouldDumpSymbolGroup(uint32_t Idx, const SymbolGroup &Group);
+bool shouldDumpSymbolGroup(uint32_t Idx, const SymbolGroup &Group,
+                           const Optional<FilterOptions> &Filters);
 
 // TODO: Change these callbacks to be function_refs (de-templatify them).
 template <typename CallbackT>
 Error iterateOneModule(InputFile &File, const Optional<PrintScope> &HeaderScope,
                        const SymbolGroup &SG, uint32_t Modi,
                        CallbackT Callback) {
   if (HeaderScope) {
     HeaderScope->P.formatLine(
         "Mod {0:4} | `{1}`: ",
         fmt_align(Modi, AlignStyle::Right, HeaderScope->LabelWidth), SG.name());
   }
 
   AutoIndent Indent(HeaderScope);
   return Callback(Modi, SG);
 }
 
 template <typename CallbackT>
 Error iterateSymbolGroups(InputFile &Input,
                           const Optional<PrintScope> &HeaderScope,
                           CallbackT Callback) {
   AutoIndent Indent(HeaderScope);
 
-  if (llvm::pdb::Filters.DumpModi > 0) {
-    assert(llvm::pdb::Filters.DumpModi == 1);
-    uint32_t Modi = llvm::pdb::Filters.DumpModi;
+  Optional<FilterOptions> Filters;
+  uint32_t Modi = 0;
+  if (HeaderScope) {
+    Filters = HeaderScope->P.getFilters();
+    Modi = Filters->DumpModi;
+  }
+
+  if (Modi > 0) {
+    assert(Modi == 1);
     SymbolGroup SG(&Input, Modi);
     return iterateOneModule(Input, withLabelWidth(HeaderScope, NumDigits(Modi)),
                             SG, Modi, Callback);
   }
 
   uint32_t I = 0;
 
   for (const auto &SG : Input.symbol_groups()) {
-    if (shouldDumpSymbolGroup(I, SG))
+    if (shouldDumpSymbolGroup(I, SG, Filters))
       if (auto Err =
               iterateOneModule(Input, withLabelWidth(HeaderScope, NumDigits(I)),
                                SG, I, Callback))
         return Err;
 
     ++I;
   }
   return Error::success();
 }
 
 template <typename SubsectionT>
 Error iterateModuleSubsections(
     InputFile &File, const Optional<PrintScope> &HeaderScope,
     llvm::function_ref<Error(uint32_t, const SymbolGroup &, SubsectionT &)>
         Callback) {
 
   return iterateSymbolGroups(
       File, HeaderScope, [&](uint32_t Modi, const SymbolGroup &SG) -> Error {
         for (const auto &SS : SG.getDebugSubsections()) {
           SubsectionT Subsection;
 
           if (SS.kind() != Subsection.kind())

llvm/include/llvm/DebugInfo/PDB/Native/LinePrinter.h

 };
 
 namespace llvm {
 namespace msf {
 class MSFStreamLayout;
 } // namespace msf
 namespace pdb {
 
-extern FilterOptions Filters;
-
 class ClassLayout;
 class PDBFile;
+class SymbolGroup;
 
 class LinePrinter {
   friend class WithColor;
 
 public:
   LinePrinter(int Indent, bool UseColor, raw_ostream &Stream,
-              FilterOptions &Filters);
+              const FilterOptions &Filters);
 
   void Indent(uint32_t Amount = 0);
   void Unindent(uint32_t Amount = 0);
   void NewLine();
 
   void printLine(const Twine &T);
   void print(const Twine &T);
   template <typename... Ts> void formatLine(const char *Fmt, Ts &&...Items) {
public:
   raw_ostream &getStream() { return OS; }
   int getIndentLevel() const { return CurrentIndent; }
 
   bool IsClassExcluded(const ClassLayout &Class);
   bool IsTypeExcluded(llvm::StringRef TypeName, uint64_t Size);
   bool IsSymbolExcluded(llvm::StringRef SymbolName);
   bool IsCompilandExcluded(llvm::StringRef CompilandName);
 
+  const FilterOptions &getFilters() const { return Filters; }
+
 private:
   template <typename Iter>
   void SetFilters(std::list<Regex> &List, Iter Begin, Iter End) {
     List.clear();
     for (; Begin != End; ++Begin)
       List.emplace_back(StringRef(*Begin));
   }
 
   raw_ostream &OS;
   int IndentSpaces;
   int CurrentIndent;
   bool UseColor;
+  const FilterOptions &Filters;
 
   std::list<Regex> ExcludeCompilandFilters;
   std::list<Regex> ExcludeTypeFilters;
   std::list<Regex> ExcludeSymbolFilters;
 
   std::list<Regex> IncludeCompilandFilters;
   std::list<Regex> IncludeTypeFilters;
   std::list<Regex> IncludeSymbolFilters;

llvm/lib/DebugInfo/PDB/Native/InputFile.cpp

if (Name.equals_insensitive("* linker *"))
     return false;
   if (Name.startswith_insensitive("f:\\binaries\\Intermediate\\vctools"))
     return false;
   if (Name.startswith_insensitive("f:\\dd\\vctools\\crt"))
     return false;
   return true;
 }
 
-bool llvm::pdb::shouldDumpSymbolGroup(uint32_t Idx, const SymbolGroup &Group) {
-  if (llvm::pdb::Filters.JustMyCode && !isMyCode(Group))
+bool llvm::pdb::shouldDumpSymbolGroup(uint32_t Idx, const SymbolGroup &Group,
+                                      const Optional<FilterOptions> &Filters) {
+  // If no available Filters, always process all modules.
+  if (!Filters)
+    return true;
+
+  if (Filters->JustMyCode && !isMyCode(Group))
     return false;
 
   // If the arg was not specified on the command line, always dump all modules.
-  if (llvm::pdb::Filters.DumpModi == 0)
+  if (Filters->DumpModi == 0)
     return true;
 
   // Otherwise, only dump if this is the same module specified.
-  return (llvm::pdb::Filters.DumpModi == Idx);
+  return (Filters->DumpModi == Idx);
 }

llvm/lib/DebugInfo/PDB/Native/LinePrinter.cpp

 #include "llvm/Support/Regex.h"
 
 #include <algorithm>
 
 using namespace llvm;
 using namespace llvm::msf;
 using namespace llvm::pdb;
 
-// TODO: Move this Filters state inside the LinePrinter class and pass it by
-// reference to the iterate* functions.
-FilterOptions llvm::pdb::Filters;
-
 namespace {
 bool IsItemExcluded(llvm::StringRef Item,
                     std::list<llvm::Regex> &IncludeFilters,
                     std::list<llvm::Regex> &ExcludeFilters) {
   if (Item.empty())
     return false;
 
   auto match_pred = [Item](llvm::Regex &R) { return R.match(Item); };
 
   // Include takes priority over exclude.  If the user specified include
   // filters, and none of them include this item, them item is gone.
   if (!IncludeFilters.empty() && !any_of(IncludeFilters, match_pred))
     return true;
 
   if (any_of(ExcludeFilters, match_pred))
     return true;
 
   return false;
 }
 } // namespace
 
 using namespace llvm;
 
 LinePrinter::LinePrinter(int Indent, bool UseColor, llvm::raw_ostream &Stream,
-                         FilterOptions &Filters)
-    : OS(Stream), IndentSpaces(Indent), CurrentIndent(0), UseColor(UseColor) {
-  llvm::pdb::Filters = Filters;
+                         const FilterOptions &Filters)
+    : OS(Stream), IndentSpaces(Indent), CurrentIndent(0), UseColor(UseColor),
+      Filters(Filters) {
   SetFilters(ExcludeTypeFilters, Filters.ExcludeTypes.begin(),
              Filters.ExcludeTypes.end());
   SetFilters(ExcludeSymbolFilters, Filters.ExcludeSymbols.begin(),
              Filters.ExcludeSymbols.end());
   SetFilters(ExcludeCompilandFilters, Filters.ExcludeCompilands.begin(),
              Filters.ExcludeCompilands.end());
 
   SetFilters(IncludeTypeFilters, Filters.IncludeTypes.begin(),