[Tooling/DependencyScanning] Make skipping excluded PP ranges during dependency scanning the default

[akyrtzi]

Summary

This is to improve maintenance a bit and remove need to maintain the additional option and related code-paths.

[jansvoboda11]

LGTM in general, with some suggestions in-line. Besides those, I think we should be able to remove PPSkipMappings from the condition in MinimizedVFSFile::create (DependencyScanningFilesystem.cpp).

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

206–207

I think we can remove this check as well now.

289–290

Do we need to keep this on the heap?

[akyrtzi]

Change APIs to accept a reference of ExcludedPreprocessorDirectiveSkipMapping instead of a pointer, since it is required now.

[akyrtzi]

@jansvoboda11 thanks for reviewing! I've changed APIs to use a reference instead of a pointer and removed the unnecessary check and heap allocations.

[jansvoboda11]

Looks good, thanks!

clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h

 /// This is not a thread safe VFS. A single instance is meant to be used only in
 /// one thread. Multiple instances are allowed to service multiple threads
 /// running in parallel.
 class DependencyScanningWorkerFilesystem : public llvm::vfs::ProxyFileSystem {
 public:
   DependencyScanningWorkerFilesystem(
       DependencyScanningFilesystemSharedCache &SharedCache,
       IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS,
-      ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings)
+      ExcludedPreprocessorDirectiveSkipMapping &PPSkipMappings)
       : ProxyFileSystem(std::move(FS)), SharedCache(SharedCache),
         PPSkipMappings(PPSkipMappings) {}
 
   llvm::ErrorOr<llvm::vfs::Status> status(const Twine &Path) override;
   llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
   openFileForRead(const Twine &Path) override;
 
   /// Disable minimization of the given file.
return SharedCache.getShardForFilename(Filename)
         .getOrInsertEntryForFilename(Filename, Entry);
   }
 
   /// The global cache shared between worker threads.
   DependencyScanningFilesystemSharedCache &SharedCache;
   /// The local cache is used by the worker thread to cache file system queries
   /// locally instead of querying the global cache every time.
   DependencyScanningFilesystemLocalCache LocalCache;
-  /// The optional mapping structure which records information about the
+  /// The mapping structure which records information about the
   /// excluded conditional directive skip mappings that are used by the
   /// currently active preprocessor.
-  ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings;
+  ExcludedPreprocessorDirectiveSkipMapping &PPSkipMappings;
   /// The set of files that should not be minimized.
   llvm::DenseSet<llvm::sys::fs::UniqueID> NotToBeMinimized;
 };
 
 } // end namespace dependencies
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_DEPENDENCYSCANNINGFILESYSTEM_H

clang/include/clang/Tooling/DependencyScanning/DependencyScanningService.h

 };
 
 /// The dependency scanning service contains the shared state that is used by
 /// the invidual dependency scanning workers.
 class DependencyScanningService {
 public:
   DependencyScanningService(ScanningMode Mode, ScanningOutputFormat Format,
                             bool ReuseFileManager = true,
-                            bool SkipExcludedPPRanges = true,
                             bool OptimizeArgs = false);
 
   ScanningMode getMode() const { return Mode; }
 
   ScanningOutputFormat getFormat() const { return Format; }
 
   bool canReuseFileManager() const { return ReuseFileManager; }
 
-  bool canSkipExcludedPPRanges() const { return SkipExcludedPPRanges; }
-
   bool canOptimizeArgs() const { return OptimizeArgs; }
 
   DependencyScanningFilesystemSharedCache &getSharedCache() {
     return SharedCache;
   }
 
 private:
   const ScanningMode Mode;
   const ScanningOutputFormat Format;
   const bool ReuseFileManager;
-  /// Set to true to use the preprocessor optimization that skips excluded PP
-  /// ranges by bumping the buffer pointer in the lexer instead of lexing the
-  /// tokens in the range until reaching the corresponding directive.
-  const bool SkipExcludedPPRanges;
   /// Whether to optimize the modules' command-line arguments.
   const bool OptimizeArgs;
   /// The global file system cache.
   DependencyScanningFilesystemSharedCache SharedCache;
 };
 
 } // end namespace dependencies
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_DEPENDENCYSCANNING_DEPENDENCYSCANNINGSERVICE_H

clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h

public:
   /// occurred, success otherwise.
   llvm::Error computeDependencies(StringRef WorkingDirectory,
                                   const std::vector<std::string> &CommandLine,
                                   DependencyConsumer &Consumer,
                                   llvm::Optional<StringRef> ModuleName = None);
 
 private:
   std::shared_ptr<PCHContainerOperations> PCHContainerOps;
-  std::unique_ptr<ExcludedPreprocessorDirectiveSkipMapping> PPSkipMappings;
+  ExcludedPreprocessorDirectiveSkipMapping PPSkipMappings;
 
   /// The physical filesystem overlaid by `InMemoryFS`.
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> RealFS;
   /// The in-memory filesystem laid on top the physical filesystem in `RealFS`.
   llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFS;
   /// The file system that is used by each worker when scanning for
   /// dependencies. This filesystem persists across multiple compiler
   /// invocations.

clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp

 class MinimizedVFSFile final : public llvm::vfs::File {
 public:
   MinimizedVFSFile(std::unique_ptr<llvm::MemoryBuffer> Buffer,
                    llvm::vfs::Status Stat)
       : Buffer(std::move(Buffer)), Stat(std::move(Stat)) {}
 
   static llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
   create(EntryRef Entry,
-         ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings);
+         ExcludedPreprocessorDirectiveSkipMapping &PPSkipMappings);
 
   llvm::ErrorOr<llvm::vfs::Status> status() override { return Stat; }
 
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
   getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator,
             bool IsVolatile) override {
     return std::move(Buffer);
   }
 
   std::error_code close() override { return {}; }
 
 private:
   std::unique_ptr<llvm::MemoryBuffer> Buffer;
   llvm::vfs::Status Stat;
 };
 
 } // end anonymous namespace
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>> MinimizedVFSFile::create(
-    EntryRef Entry, ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings) {
+    EntryRef Entry, ExcludedPreprocessorDirectiveSkipMapping &PPSkipMappings) {
   assert(!Entry.isError() && "error");
 
   if (Entry.isDirectory())
     return std::make_error_code(std::errc::is_a_directory);
 
   auto Result = std::make_unique<MinimizedVFSFile>(
       llvm::MemoryBuffer::getMemBuffer(Entry.getContents(),
                                        Entry.getStatus().getName(),
                                        /*RequiresNullTerminator=*/false),
       Entry.getStatus());
 
   const auto *EntrySkipMappings = Entry.getPPSkippedRangeMapping();
-  if (EntrySkipMappings && !EntrySkipMappings->empty() && PPSkipMappings)
-    (*PPSkipMappings)[Result->Buffer->getBufferStart()] = EntrySkipMappings;
+  if (EntrySkipMappings && !EntrySkipMappings->empty())
+    PPSkipMappings[Result->Buffer->getBufferStart()] = EntrySkipMappings;
 
   return llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>(
       std::unique_ptr<llvm::vfs::File>(std::move(Result)));
 }
 
 llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
 DependencyScanningWorkerFilesystem::openFileForRead(const Twine &Path) {
   SmallString<256> OwnedFilename;
   StringRef Filename = Path.toStringRef(OwnedFilename);
 
   llvm::ErrorOr<EntryRef> Result = getOrCreateFileSystemEntry(Filename);
   if (!Result)
     return Result.getError();
   return MinimizedVFSFile::create(Result.get(), PPSkipMappings);
 }

clang/lib/Tooling/DependencyScanning/DependencyScanningService.cpp

 #include "llvm/Support/TargetSelect.h"
 
 using namespace clang;
 using namespace tooling;
 using namespace dependencies;
 
 DependencyScanningService::DependencyScanningService(
     ScanningMode Mode, ScanningOutputFormat Format, bool ReuseFileManager,
-    bool SkipExcludedPPRanges, bool OptimizeArgs)
+    bool OptimizeArgs)
     : Mode(Mode), Format(Format), ReuseFileManager(ReuseFileManager),
-      SkipExcludedPPRanges(SkipExcludedPPRanges), OptimizeArgs(OptimizeArgs) {
+      OptimizeArgs(OptimizeArgs) {
   // Initialize targets for object file support.
   llvm::InitializeAllTargets();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmPrinters();
   llvm::InitializeAllAsmParsers();
 }

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

 
 /// A clang tool that runs the preprocessor in a mode that's optimized for
 /// dependency scanning for the given compiler invocation.
 class DependencyScanningAction : public tooling::ToolAction {
 public:
   DependencyScanningAction(
       StringRef WorkingDirectory, DependencyConsumer &Consumer,
       llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS,
-      ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings,
+      ExcludedPreprocessorDirectiveSkipMapping &PPSkipMappings,
       ScanningOutputFormat Format, bool OptimizeArgs,
       llvm::Optional<StringRef> ModuleName = None)
       : WorkingDirectory(WorkingDirectory), Consumer(Consumer),
         DepFS(std::move(DepFS)), PPSkipMappings(PPSkipMappings), Format(Format),
         OptimizeArgs(OptimizeArgs), ModuleName(ModuleName) {}
 
   bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation,
                      FileManager *FileMgr,
if (DepFS) {
         DepFS->disableMinimization(F);
 
       // Support for virtual file system overlays on top of the caching
       // filesystem.
       FileMgr->setVirtualFileSystem(createVFSFromCompilerInvocation(
           ScanInstance.getInvocation(), ScanInstance.getDiagnostics(), DepFS));
 
       // Pass the skip mappings which should speed up excluded conditional block
       // skipping in the preprocessor.
-      if (PPSkipMappings)
-        ScanInstance.getPreprocessorOpts()
-            .ExcludedConditionalDirectiveSkipMappings = PPSkipMappings;
+          .ExcludedConditionalDirectiveSkipMappings = &PPSkipMappings;
     }
 
     // Create the dependency collector that will collect the produced
     // dependencies.
     //
     // This also moves the existing dependency output options from the
     // invocation to the collector. The options in the invocation are reset,
     // which ensures that the compiler won't create new dependency collectors,
if (!DepFS)
       FileMgr->clearStatCache();
     return Result;
   }
 
 private:
   StringRef WorkingDirectory;
   DependencyConsumer &Consumer;
   llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS;
-  ExcludedPreprocessorDirectiveSkipMapping *PPSkipMappings;
+  ExcludedPreprocessorDirectiveSkipMapping &PPSkipMappings;
   ScanningOutputFormat Format;
   bool OptimizeArgs;
   llvm::Optional<StringRef> ModuleName;
 };
 
 } // end anonymous namespace
 
 DependencyScanningWorker::DependencyScanningWorker(
     DependencyScanningService &Service)
     : Format(Service.getFormat()), OptimizeArgs(Service.canOptimizeArgs()) {
   PCHContainerOps = std::make_shared<PCHContainerOperations>();
   PCHContainerOps->registerReader(
       std::make_unique<ObjectFilePCHContainerReader>());
   // We don't need to write object files, but the current PCH implementation
   // requires the writer to be registered as well.
   PCHContainerOps->registerWriter(
       std::make_unique<ObjectFilePCHContainerWriter>());
 
   auto OverlayFS = llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(
       llvm::vfs::createPhysicalFileSystem());
   InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
   OverlayFS->pushOverlay(InMemoryFS);
   RealFS = OverlayFS;
 
-  if (Service.canSkipExcludedPPRanges())
-    PPSkipMappings =
-        std::make_unique<ExcludedPreprocessorDirectiveSkipMapping>();
   if (Service.getMode() == ScanningMode::MinimizedSourcePreprocessing)
-    DepFS = new DependencyScanningWorkerFilesystem(
-        Service.getSharedCache(), RealFS, PPSkipMappings.get());
+    DepFS = new DependencyScanningWorkerFilesystem(Service.getSharedCache(),
+                                                   RealFS, PPSkipMappings);
   if (Service.canReuseFileManager())
     Files = new FileManager(FileSystemOptions(), RealFS);
 }
 
 static llvm::Error
 runWithDiags(DiagnosticOptions *DiagOpts,
              llvm::function_ref<bool(DiagnosticConsumer &, DiagnosticOptions &)>
                  BodyShouldSucceed) {
llvm::Error DependencyScanningWorker::computeDependencies(
 
   std::vector<const char *> FinalCCommandLine(CommandLine.size(), nullptr);
   llvm::transform(CommandLine, FinalCCommandLine.begin(),
                   [](const std::string &Str) { return Str.c_str(); });
 
   return runWithDiags(CreateAndPopulateDiagOpts(FinalCCommandLine).release(),
                       [&](DiagnosticConsumer &DC, DiagnosticOptions &DiagOpts) {
                         DependencyScanningAction Action(
-                            WorkingDirectory, Consumer, DepFS,
-                            PPSkipMappings.get(), Format, OptimizeArgs,
-                            ModuleName);
+                            WorkingDirectory, Consumer, DepFS, PPSkipMappings,
+                            Format, OptimizeArgs, ModuleName);
                         // Create an invocation that uses the underlying file
                         // system to ensure that any file system requests that
                         // are made by the driver do not go through the
                         // dependency scanning filesystem.
                         ToolInvocation Invocation(FinalCommandLine, &Action,
                                                   CurrentFiles.get(),
                                                   PCHContainerOps);
                         Invocation.setDiagnosticConsumer(&DC);
                         Invocation.setDiagnosticOptions(&DiagOpts);
                         return Invocation.run();
                       });
 }

clang/test/ClangScanDeps/regular_cdb.cpp

 // RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess-minimized-sources | \
 // RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
 
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess | \
 // RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
 // RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess | \
 // RUN:   FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
 
-// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -mode preprocess-minimized-sources \
-// RUN:   -skip-excluded-pp-ranges=0 | FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
-// RUN: clang-scan-deps -compilation-database %t_clangcl.cdb -j 1 -mode preprocess-minimized-sources \
-// RUN:   -skip-excluded-pp-ranges=0 | FileCheck --check-prefixes=CHECK1,CHECK2,CHECK2NO,CHECK3 %s
-//
 // Make sure we didn't produce any dependency files!
 // RUN: not cat %t.dir/regular_cdb.d
 // RUN: not cat %t.dir/regular_cdb_clangcl.d
 // RUN: not cat %t.dir/regular_cdb2.d
 // RUN: not cat %t.dir/regular_cdb2_clangcl.d
 //
 // The output order is non-deterministic when using more than one thread,
 // so check the output using two runs. Note that the 'NOT' check is not used

clang/tools/clang-scan-deps/ClangScanDeps.cpp

CompilationDB("compilation-database",
                   llvm::cl::desc("Compilation database"), llvm::cl::Required,
                   llvm::cl::cat(DependencyScannerCategory));
 
 llvm::cl::opt<bool> ReuseFileManager(
     "reuse-filemanager",
     llvm::cl::desc("Reuse the file manager and its cache between invocations."),
     llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));
 
-llvm::cl::opt<bool> SkipExcludedPPRanges(
-    "skip-excluded-pp-ranges",
-    llvm::cl::desc(
-        "Use the preprocessor optimization that skips excluded conditionals by "
-        "bumping the buffer pointer in the lexer instead of lexing the tokens  "
-        "until reaching the end directive."),
-    llvm::cl::init(true), llvm::cl::cat(DependencyScannerCategory));
-
 llvm::cl::opt<std::string> ModuleName(
     "module-name", llvm::cl::Optional,
     llvm::cl::desc("the module of which the dependencies are to be computed"),
     llvm::cl::cat(DependencyScannerCategory));
 
 enum ResourceDirRecipeKind {
   RDRK_ModifyCompilerPath,
   RDRK_InvokeCompiler,
AdjustingCompilations->appendArgumentsAdjuster(
         return AdjustedArgs;
       });
 
   SharedStream Errs(llvm::errs());
   // Print out the dependency results to STDOUT by default.
   SharedStream DependencyOS(llvm::outs());
 
   DependencyScanningService Service(ScanMode, Format, ReuseFileManager,
-                                    SkipExcludedPPRanges, OptimizeArgs);
+                                    OptimizeArgs);
   llvm::ThreadPool Pool(llvm::hardware_concurrency(NumThreads));
   std::vector<std::unique_ptr<DependencyScanningTool>> WorkerTools;
   for (unsigned I = 0; I < Pool.getThreadCount(); ++I)
     WorkerTools.push_back(std::make_unique<DependencyScanningTool>(Service));
 
   std::vector<tooling::CompileCommand> Inputs =
       AdjustingCompilations->getAllCompileCommands();
 

clang/unittests/Tooling/DependencyScannerTest.cpp

 namespace dependencies {
 TEST(DependencyScanningFilesystem, IgnoredFilesAreCachedSeparately1) {
   auto VFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
   VFS->addFile("/mod.h", 0,
                llvm::MemoryBuffer::getMemBuffer("#include <foo.h>\n"
                                                 "// hi there!\n"));
 
   DependencyScanningFilesystemSharedCache SharedCache;
-  auto Mappings = std::make_unique<ExcludedPreprocessorDirectiveSkipMapping>();
-  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings.get());
+  ExcludedPreprocessorDirectiveSkipMapping Mappings;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings);
 
   DepFS.enableMinimizationOfAllFiles(); // Let's be explicit for clarity.
   auto StatusMinimized0 = DepFS.status("/mod.h");
   DepFS.disableMinimization("/mod.h");
   auto StatusFull1 = DepFS.status("/mod.h");
 
   EXPECT_TRUE(StatusMinimized0);
   EXPECT_TRUE(StatusFull1);
   EXPECT_EQ(StatusMinimized0->getSize(), 17u);
   EXPECT_EQ(StatusFull1->getSize(), 30u);
   EXPECT_EQ(StatusMinimized0->getName(), StringRef("/mod.h"));
   EXPECT_EQ(StatusFull1->getName(), StringRef("/mod.h"));
 }
 
 TEST(DependencyScanningFilesystem, IgnoredFilesAreCachedSeparately2) {
   auto VFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
   VFS->addFile("/mod.h", 0,
                llvm::MemoryBuffer::getMemBuffer("#include <foo.h>\n"
                                                 "// hi there!\n"));
 
   DependencyScanningFilesystemSharedCache SharedCache;
-  auto Mappings = std::make_unique<ExcludedPreprocessorDirectiveSkipMapping>();
-  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings.get());
+  ExcludedPreprocessorDirectiveSkipMapping Mappings;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, VFS, Mappings);
 
   DepFS.disableMinimization("/mod.h");
   auto StatusFull0 = DepFS.status("/mod.h");
   DepFS.enableMinimizationOfAllFiles();
   auto StatusMinimized1 = DepFS.status("/mod.h");
 
   EXPECT_TRUE(StatusFull0);
   EXPECT_TRUE(StatusMinimized1);