[pp-trace] Delete -ignore and add a new option -callbacks

[MaskRay]

Summary

-ignore specifies a list of PP callbacks to ignore. It cannot express a
whitelist, which may be more useful than a blacklist.
Add a new option -callbacks to replace it.

-ignore= (default) => -callbacks='*' (default)
-ignore=FileChanged,FileSkipped => -callbacks='*,-FileChanged,-FileSkipped'

-callbacks='Macro*' : print only MacroDefined,MacroExpands,MacroUndefined,...

[aaron.ballman]

I think the release notes should be updated to mention that a previously-supported option has been removed and that there's a new option available as a replacement; I don't think we have any other docs for pp-trace to worry about updating, do we?

pp-trace/PPTrace.cpp

70–71 ↗

(On Diff #190405)

Can re-flow the string literal.

[MaskRay]

Reflow comments

[MaskRay]

Reflow string literal

[MaskRay]

Update ReleaseNotes.rst

[MaskRay]

In D59296#1430813, @aaron.ballman wrote:

I think the release notes should be updated to mention that a previously-supported option has been removed and that there's a new option available as a replacement; I don't think we have any other docs for pp-trace to worry about updating, do we?

Added a note to docs/ReleaseNotes.rst. I can't find other documentation mentioning this tool. This tool isn't used a lot...

[MaskRay]

@aaron.ballman Does the release note look good now? 😊

[aaron.ballman]

LGTM aside from some small nits. Feel free to fix and commit without further review if you'd like.

docs/ReleaseNotes.rst

141 ↗

(On Diff #190937)

How about: Added a new option, -callbacks, to filter preprocessor callbacks; replaces the -ignore option.

pp-trace/PPTrace.cpp

168 ↗

(On Diff #190937)

Do you want to trim the pattern, in case the user specifies a pattern with whitespace around the commas? e.g., -callbacks 'foo, bar, baz' (You should add a test for this as well.)

[MaskRay]

Add a test with spaces around patterns

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...
+
+Improvements to pp-trace
+------------------------
+
+- Added a new option `-callbacks` to filter preprocessor callbacks. It replaces
+  the `-ignore` option.

clang-tools-extra/trunk/docs/pp-trace.rst

 `Clang Compiler Options <https://clang.llvm.org/docs/UsersManual.html#command-line-options>`_,
 which must follow the <source-file>.
 
 .. _CommandLineOptions:
 
 Command Line Options
 --------------------
 
-.. option:: -ignore <callback-name-list>
+.. option:: -callbacks <comma-separated-globs>
 
-  This option specifies a comma-separated list of names of callbacks
-  that shouldn't be traced. It can be used to eliminate unwanted
-  trace output. The callback names are the name of the actual
-  callback function names in the PPCallbacks class:
+  This option specifies a comma-separated list of globs describing the list of
+  callbacks that should be traced. Globs are processed in order of appearance.
+  Positive globs add matched callbacks to the set, netative globs (those with
+  the '-' prefix) remove callacks from the set.
 
   * FileChanged
   * FileSkipped
   * FileNotFound
   * InclusionDirective
   * moduleImport
   * EndOfMainFile
   * Ident

clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.h

 #ifndef PPTRACE_PPCALLBACKSTRACKER_H
 #define PPTRACE_PPCALLBACKSTRACKER_H
 
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/GlobPattern.h"
 #include <string>
 #include <vector>
 
 /// \brief This class represents one callback function argument by name
 ///   and value.
 class Argument {
 public:
   Argument(llvm::StringRef Name, llvm::StringRef Value)

 public:
   CallbackCall(llvm::StringRef Name) : Name(Name) {}
   CallbackCall() = default;
 
   std::string Name;
   std::vector<Argument> Arguments;
 };
 
+using FilterType = std::vector<std::pair<llvm::GlobPattern, bool>>;
+
 /// \brief This class overrides the PPCallbacks class for tracking preprocessor
 ///   activity by means of its callback functions.
 ///
 /// This object is given a vector for storing the trace information, built up
 /// of CallbackCall and subordinate Argument objects for representing the
 /// callback calls and their arguments.  It's a reference so the vector can
 /// exist beyond the lifetime of this object, because it's deleted by the
 /// preprocessor automatically in its destructor.
 ///
 /// This class supports a mechanism for inhibiting trace output for
 /// specific callbacks by name, for the purpose of eliminating output for
 /// callbacks of no interest that might clutter the output.
 ///
 /// Following the constructor and destructor function declarations, the
 /// overidden callback functions are defined.  The remaining functions are
 /// helpers for recording the trace data, to reduce the coupling between it
 /// and the recorded data structure.
 class PPCallbacksTracker : public clang::PPCallbacks {
 public:
   /// \brief Note that all of the arguments are references, and owned
   /// by the caller.
-  /// \param Ignore - Set of names of callbacks to ignore.
+  /// \param Filters - List of (Glob,Enabled) pairs used to filter callbacks.
   /// \param CallbackCalls - Trace buffer.
   /// \param PP - The preprocessor.  Needed for getting some argument strings.
-  PPCallbacksTracker(llvm::SmallSet<std::string, 4> &Ignore,
+  PPCallbacksTracker(const FilterType &Filters,
                      std::vector<CallbackCall> &CallbackCalls,
                      clang::Preprocessor &PP);
 
   ~PPCallbacksTracker() override;
 
   // Overidden callback functions.
 
   void FileChanged(clang::SourceLocation Loc,
public:
   /// \brief Get the raw source string of the range.
   llvm::StringRef getSourceString(clang::CharSourceRange Range);
 
   /// \brief Callback trace information.
   /// We use a reference so the trace will be preserved for the caller
   /// after this object is destructed.
   std::vector<CallbackCall> &CallbackCalls;
 
-  /// \brief Names of callbacks to ignore.
-  llvm::SmallSet<std::string, 4> &Ignore;
+  // List of (Glob,Enabled) pairs used to filter callbacks.
+  const FilterType &Filters;
+
+  // Whether a callback should be printed.
+  llvm::StringMap<bool> CallbackIsEnabled;
 
   /// \brief Inhibit trace while this is set.
   bool DisableTrace;
 
   clang::Preprocessor &PP;
 };
 
 #endif // PPTRACE_PPCALLBACKSTRACKER_H

clang-tools-extra/trunk/pp-trace/PPCallbacksTracker.cpp

 
 // Mapping strings.
 static const char *const MappingStrings[] = { "0",          "MAP_IGNORE",
                                               "MAP_REMARK", "MAP_WARNING",
                                               "MAP_ERROR",  "MAP_FATAL" };
 
 // PPCallbacksTracker functions.
 
-PPCallbacksTracker::PPCallbacksTracker(llvm::SmallSet<std::string, 4> &Ignore,
+PPCallbacksTracker::PPCallbacksTracker(const FilterType &Filters,
                                        std::vector<CallbackCall> &CallbackCalls,
                                        clang::Preprocessor &PP)
-    : CallbackCalls(CallbackCalls), Ignore(Ignore), PP(PP) {}
+    : CallbackCalls(CallbackCalls), Filters(Filters), PP(PP) {}
 
 PPCallbacksTracker::~PPCallbacksTracker() {}
 
 // Callback functions.
 
 // Callback invoked whenever a source file is entered or exited.
 void PPCallbacksTracker::FileChanged(
     clang::SourceLocation Loc, clang::PPCallbacks::FileChangeReason Reason,
void PPCallbacksTracker::Endif(clang::SourceLocation Loc,
   appendArgument("Loc", Loc);
   appendArgument("IfLoc", IfLoc);
 }
 
 // Helper functions.
 
 // Start a new callback.
 void PPCallbacksTracker::beginCallback(const char *Name) {
-  DisableTrace = Ignore.count(std::string(Name));
+  auto R = CallbackIsEnabled.try_emplace(Name, false);
+  if (R.second) {
+    llvm::StringRef N(Name);
+    for (const std::pair<llvm::GlobPattern, bool> &Filter : Filters)
+      if (Filter.first.match(N))
+        R.first->second = Filter.second;
+  }
+  DisableTrace = !R.first->second;
   if (DisableTrace)
     return;
   CallbackCalls.push_back(CallbackCall(Name));
 }
 
 // Append a bool argument to the top trace item.
 void PPCallbacksTracker::appendArgument(const char *Name, bool Value) {
   appendArgument(Name, (Value ? "true" : "false"));

clang-tools-extra/trunk/pp-trace/PPTrace.cpp

 //
 // The pp-trace tool supports the following general command line format:
 //
 //    pp-trace [pp-trace options] (source file) [compiler options]
 //
 // Basically you put the pp-trace options first, then the source file or files,
 // and then any options you want to pass to the compiler.
 //
-// These are the pp-trace options:
-//
-//    -ignore (callback list)     Don't display output for a comma-separated
-//                                list of callbacks, i.e.:
-//                                  -ignore "FileChanged,InclusionDirective"
-//
-//    -output (file)              Output trace to the given file in a YAML
-//                                format, e.g.:
-//
-//                                  ---
-//                                  - Callback: Name
-//                                    Argument1: Value1
-//                                    Argument2: Value2
-//                                  (etc.)
-//                                  ...
-//
-// Future Directions:
-//
-// 1. Add option opposite to "-ignore" that specifys a comma-separated option
-// list of callbacs.  Perhaps "-only" or "-exclusive".
-//
 //===----------------------------------------------------------------------===//
 
 #include "PPCallbacksTracker.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Driver/Options.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/OptTable.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/GlobPattern.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/WithColor.h"
 #include <algorithm>
 #include <fstream>
 #include <iterator>
 #include <string>
 #include <vector>
 
 using namespace clang;
 using namespace clang::tooling;
 using namespace llvm;
 
 // Options:
 
 // Collect the source files.
 static cl::list<std::string> SourcePaths(cl::Positional,
                                          cl::desc("<source0> [... <sourceN>]"),
                                          cl::OneOrMore);
 
-// Option to specify a list or one or more callback names to ignore.
-static cl::opt<std::string> IgnoreCallbacks(
-    "ignore", cl::init(""),
-    cl::desc("Ignore callbacks, i.e. \"Callback1, Callback2...\"."));
+static cl::opt<std::string> Callbacks(
+    "callbacks", cl::init("*"),
+    cl::desc("Comma-separated list of globs describing the list of callbacks "
+             "to output. Globs are processed in order of appearance. Globs "
+             "with the '-' prefix remove callbacks from the set. e.g. "
+             "'*,-Macro*'."));
 
 // Option to specify the trace output file name.
 static cl::opt<std::string> OutputFileName(
     "output", cl::init(""),
     cl::desc("Output trace to the given file name or '-' for stdout."));
 
 // Collect all other arguments, which will be passed to the front end.
 static cl::list<std::string>
     CC1Arguments(cl::ConsumeAfter,
                  cl::desc("<arguments to be passed to front end>..."));
 
 // Frontend action stuff:
 
 namespace {
 // Consumer is responsible for setting up the callbacks.
 class PPTraceConsumer : public ASTConsumer {
 public:
-  PPTraceConsumer(SmallSet<std::string, 4> &Ignore,
+  PPTraceConsumer(const FilterType &Filters,
                   std::vector<CallbackCall> &CallbackCalls, Preprocessor &PP) {
     // PP takes ownership.
-    PP.addPPCallbacks(llvm::make_unique<PPCallbacksTracker>(Ignore,
-                                                            CallbackCalls, PP));
+    PP.addPPCallbacks(
+        llvm::make_unique<PPCallbacksTracker>(Filters, CallbackCalls, PP));
   }
 };
 
 class PPTraceAction : public SyntaxOnlyAction {
 public:
-  PPTraceAction(SmallSet<std::string, 4> &Ignore,
+  PPTraceAction(const FilterType &Filters,
                 std::vector<CallbackCall> &CallbackCalls)
-      : Ignore(Ignore), CallbackCalls(CallbackCalls) {}
+      : Filters(Filters), CallbackCalls(CallbackCalls) {}
 
 protected:
   std::unique_ptr<clang::ASTConsumer>
   CreateASTConsumer(CompilerInstance &CI, StringRef InFile) override {
-    return llvm::make_unique<PPTraceConsumer>(Ignore, CallbackCalls,
+    return llvm::make_unique<PPTraceConsumer>(Filters, CallbackCalls,
                                               CI.getPreprocessor());
   }
 
 private:
-  SmallSet<std::string, 4> &Ignore;
+  const FilterType &Filters;
   std::vector<CallbackCall> &CallbackCalls;
 };
 
 class PPTraceFrontendActionFactory : public FrontendActionFactory {
 public:
-  PPTraceFrontendActionFactory(SmallSet<std::string, 4> &Ignore,
+  PPTraceFrontendActionFactory(const FilterType &Filters,
                                std::vector<CallbackCall> &CallbackCalls)
-      : Ignore(Ignore), CallbackCalls(CallbackCalls) {}
+      : Filters(Filters), CallbackCalls(CallbackCalls) {}
 
   PPTraceAction *create() override {
-    return new PPTraceAction(Ignore, CallbackCalls);
+    return new PPTraceAction(Filters, CallbackCalls);
   }
 
 private:
-  SmallSet<std::string, 4> &Ignore;
+  const FilterType &Filters;
   std::vector<CallbackCall> &CallbackCalls;
 };
 } // namespace
 
 // Output the trace given its data structure and a stream.
 static int outputPPTrace(std::vector<CallbackCall> &CallbackCalls,
                          llvm::raw_ostream &OS) {
   // Mark start of document.

 
 // Program entry point.
 int main(int Argc, const char **Argv) {
 
   // Parse command line.
   cl::ParseCommandLineOptions(Argc, Argv, "pp-trace.\n");
 
   // Parse the IgnoreCallbacks list into strings.
-  SmallVector<StringRef, 32> IgnoreCallbacksStrings;
-  StringRef(IgnoreCallbacks).split(IgnoreCallbacksStrings, ",",
-                                   /*MaxSplit=*/ -1, /*KeepEmpty=*/false);
-  SmallSet<std::string, 4> Ignore;
-  for (SmallVector<StringRef, 32>::iterator I = IgnoreCallbacksStrings.begin(),
-                                            E = IgnoreCallbacksStrings.end();
-       I != E; ++I)
-    Ignore.insert(*I);
+  SmallVector<StringRef, 32> Patterns;
+  FilterType Filters;
+  StringRef(Callbacks).split(Patterns, ",",
+                             /*MaxSplit=*/-1, /*KeepEmpty=*/false);
+  for (StringRef Pattern : Patterns) {
+    Pattern = Pattern.trim();
+    bool Enabled = !Pattern.consume_front("-");
+    if (Expected<GlobPattern> Pat = GlobPattern::create(Pattern))
+      Filters.emplace_back(std::move(*Pat), Enabled);
+    else {
+      WithColor::error(llvm::errs(), "pp-trace")
+          << toString(Pat.takeError()) << '\n';
+      return 1;
+    }
+  }
 
   // Create the compilation database.
   SmallString<256> PathBuf;
   sys::fs::current_path(PathBuf);
   std::unique_ptr<CompilationDatabase> Compilations;
   Compilations.reset(
       new FixedCompilationDatabase(Twine(PathBuf), CC1Arguments));
 
   // Store the callback trace information here.
   std::vector<CallbackCall> CallbackCalls;
 
   // Create the tool and run the compilation.
   ClangTool Tool(*Compilations, SourcePaths);
-  PPTraceFrontendActionFactory Factory(Ignore, CallbackCalls);
+  PPTraceFrontendActionFactory Factory(Filters, CallbackCalls);
   int HadErrors = Tool.run(&Factory);
 
   // If we had errors, exit early.
   if (HadErrors)
     return HadErrors;
 
   // Do the output.
   if (!OutputFileName.size()) {

clang-tools-extra/trunk/test/pp-trace/pp-trace-conditional.cpp

-// RUN: pp-trace -ignore FileChanged %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged' %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
 
 #if 1
 #endif
 
 #if 0
 #endif
 
 #if 1

clang-tools-extra/trunk/test/pp-trace/pp-trace-filter.cpp

+// RUN: pp-trace -callbacks 'File*,Macro*,-MacroUndefined' %s | FileCheck %s
+// RUN: pp-trace -callbacks ' File* , Macro* , -MacroUndefined ' %s | FileCheck %s
+// RUN: not pp-trace -callbacks '[' %s 2>&1 | FileCheck --check-prefix=INVALID %s
+
+#define M 1
+int i = M;
+#undef M
+
+// CHECK:      ---
+// CHECK:      - Callback: FileChanged
+// CHECK:      - Callback: MacroDefined
+// CHECK:      - Callback: MacroExpands
+// CHECK-NOT:  - Callback: MacroUndefined
+// CHECK-NOT:  - Callback: EndOfMainFile
+// CHECK:      ...
+
+// INVALID: error: invalid glob pattern: [

clang-tools-extra/trunk/test/pp-trace/pp-trace-ident.cpp

-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
 
 #ident "$Id$"
 
 // CHECK: ---
 // CHECK-NEXT: - Callback: Ident
 // CHECK-NEXT:   Loc: "{{.*}}{{[/\\]}}pp-trace-ident.cpp:3:2"
 // CHECK-NEXT:   Str: "$Id$"
 // CHECK-NEXT: - Callback: EndOfMainFile
 // CHECK-NEXT: ...

clang-tools-extra/trunk/test/pp-trace/pp-trace-macro.cpp

-// RUN: pp-trace -ignore FileChanged %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged' %s -undef -target x86_64 -std=c++11 | FileCheck --strict-whitespace %s
 
 #define MACRO 1
 int i = MACRO;
 #if defined(MACRO)
 #endif
 #undef MACRO
 #if defined(MACRO)
 #endif

clang-tools-extra/trunk/test/pp-trace/pp-trace-modules.cpp

 // RUN: rm -rf %t
-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -x objective-c++ -undef -target x86_64 -std=c++11 -fmodules -fcxx-modules -fmodules-cache-path=%t -I%S -I%S/Input | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -x objective-c++ -undef -target x86_64 -std=c++11 -fmodules -fcxx-modules -fmodules-cache-path=%t -I%S -I%S/Input | FileCheck --strict-whitespace %s
 
 // CHECK: ---
 
 @import Level1A;
 
 // CHECK-NEXT: - Callback: moduleImport
 // CHECK-NEXT:   ImportLoc: "{{.*}}{{[/\\]}}pp-trace-modules.cpp:[[@LINE-3]]:2"
 // CHECK-NEXT:   Path: [{Name: Level1A, Loc: "{{.*}}{{[/\\]}}pp-trace-modules.cpp:[[@LINE-4]]:9"}]

clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-general.cpp

-// RUN: pp-trace -ignore FileChanged,MacroDefined %s | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s | FileCheck --strict-whitespace %s
 
 #pragma clang diagnostic push
 #pragma clang diagnostic pop
 #pragma clang diagnostic ignored "-Wformat"
 #pragma clang diagnostic warning "-Wformat"
 #pragma clang diagnostic error "-Wformat"
 #pragma clang diagnostic fatal "-Wformat"
 

clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-ms.cpp

-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -target x86_64-unknown-windows-msvc -fms-extensions -w | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -target x86_64-unknown-windows-msvc -fms-extensions -w | FileCheck --strict-whitespace %s
 
 #pragma comment(compiler, "compiler comment")
 #pragma comment(exestr, "exestr comment")
 #pragma comment(lib, "lib comment")
 #pragma comment(linker, "linker comment")
 #pragma comment(user, "user comment")
 
 #pragma detect_mismatch("name argument", "value argument")

clang-tools-extra/trunk/test/pp-trace/pp-trace-pragma-opencl.cpp

-// RUN: pp-trace -ignore FileChanged,MacroDefined %s -x cl | FileCheck --strict-whitespace %s
+// RUN: pp-trace -callbacks '*,-FileChanged,-MacroDefined' %s -x cl | FileCheck --strict-whitespace %s
 
 #pragma OPENCL EXTENSION all : disable
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : disable
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 
 // CHECK: ---
 // CHECK-NEXT: - Callback: PragmaDirective
 // CHECK-NEXT:   Loc: "{{.*}}{{[/\\]}}pp-trace-pragma-opencl.cpp:3:1"