[Try 2] Include sanitize blacklist and other extra deps as part of scan-deps output

[kousikk]

Summary

Clang's -M mode includes these extra dependencies in its output and clang-scan-deps
should have equivalent behavior, so adding these extradeps to output just like
how its being done for ".d" file generation mode.

This is a retry of https://reviews.llvm.org/rC375074. From the build-bot failure on
Windows, it seemed like somehow the blacklist file was already added as a dependency.
So the extra change in this patch is that I add deps to a set in clang-scan-deps
to eliminate duplicates and print in sorted order. Having a set achieves two purposes:

1. Prints out the dependencies in sorted order
2. Eliminates duplicates

Windows bot failure: http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/builds/15956/steps/ninja%20check%202/logs/stdio

An alternative fix to the test would have been to check only for the presence of
blacklsit file, but I didn't prefer it since explicit testing of all expected outputs
is better (and in this case has led us to capture duplicate outputs).

[kousikk]

Fix tests now that output is in ascending order

[jkorous]

I think you could've just used CHECK-DAG to fix the tests. It *might* be a bit more robust. Although just reordering checks seems perfectly fine too.
https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive

Using std::set here sounds reasonable to me but I'd like @arphaman to be aware of this. BTW should we change std::vector<std::string> Dependencies; in DependencyCollector to set too?

Also, I'm wondering why is the behavior different on Windows.

[kousikk]

Thanks for the comment @jkorous.

I think you could've just used CHECK-DAG to fix the tests. It *might* be a bit more robust. Although just reordering checks seems perfectly fine too. https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive. Using std::set here sounds reasonable to me but I'd like @arphaman to be aware of this.

Thanks for pointing me to it - I have a minor preference towards a sorted order of outputs vs order in which we visit the files. I think a sorted order is much more easy to reason about for a client. Having said that, @arphaman what do you think? If you and Jan both feel that we should maintain the current order, I'll stick to that and not change Dependencies to set.

BTW should we change std::vector<std::string> Dependencies; in DependencyCollector to set too?

I think this would change the output of *.d file? If so I'm a little hesitant to make that change because of the impact it would have, but once again I'll defer to your judgement on this (I'm very new to LLVM codebase).

Also, I'm wondering why is the behavior different on Windows.

I confirmed that there are duplicate blacklist.txt files without changing vector to set. I'm setting up a Windows machine to try to figure out why that is the case. Will share what I find.

[dexonsmith]

In D69090#1713983, @kousikk wrote:

Thanks for the comment @jkorous.

I think you could've just used CHECK-DAG to fix the tests. It *might* be a bit more robust. Although just reordering checks seems perfectly fine too. https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive. Using std::set here sounds reasonable to me but I'd like @arphaman to be aware of this.

Thanks for pointing me to it - I have a minor preference towards a sorted order of outputs vs order in which we visit the files. I think a sorted order is much more easy to reason about for a client. Having said that, @arphaman what do you think? If you and Jan both feel that we should maintain the current order, I'll stick to that and not change Dependencies to set.

Sorting seems reasonable to me, perhaps behind an option (I'll leave that up to others). But you don't need a std::set for this, just run llvm::sort and std::unique once the dependencies are collected.

[kousikk]

I was able to test this on a Windows machine. The ExtraDeps are already being added to the dependency output (this code does that - https://github.com/llvm/llvm-project/blob/master/clang/lib/Frontend/DependencyFile.cpp#L188). My suspicion for why I wasn't seeing them previously was that these ExtraDeps are added as relative paths and my consumer code (which integrates with scan-deps) was ignoring relative paths. I can handle that in the consumer code - I'll leave the behaviour of the tool as it is.

Sorry about the back and forth on this. I'm closing this out.

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

public:
 
   void finishedMainFile(DiagnosticsEngine &Diags) override {
     llvm::SmallString<256> CanonPath;
     for (const auto &File : getDependencies()) {
       CanonPath = File;
       llvm::sys::path::remove_dots(CanonPath, /*remove_dot_dot=*/true);
       C.handleFileDependency(*Opts, CanonPath);
     }
+    for (const auto& ExtraDep : Opts->ExtraDeps)
+      C.handleFileDependency(*Opts, ExtraDep);
   }
 
 private:
   std::unique_ptr<DependencyOutputOptions> Opts;
   DependencyConsumer &C;
 };
 
 /// A proxy file system that doesn't call `chdir` when changing the working

clang/test/ClangScanDeps/Inputs/non-header-dependency.json

+[
+    {
+      "directory": "DIR",
+      "command": "clang -E DIR/non-header-dependency_input.cpp -fsanitize=bounds -fsanitize-blacklist=DIR/Inputs/sanitize-blacklist.txt",
+      "file": "DIR/non-header-dependency_input.cpp"
+    }
+]

clang/test/ClangScanDeps/Inputs/sanitize-blacklist.txt

+fun:*

clang/test/ClangScanDeps/header_stat_before_open.m

 // RUN: rm -rf %t.dir
 // RUN: rm -rf %t.cdb
 // RUN: mkdir -p %t.dir
 // RUN: cp %s %t.dir/header_stat_before_open_input.m
 // RUN: mkdir %t.dir/Inputs
 // RUN: cp -R %S/Inputs/frameworks %t.dir/Inputs/frameworks
 // RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/header_stat_before_open_cdb.json > %t.cdb
 //
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | \
 // RUN:   FileCheck %s
 
 #include "Framework/Framework.h"
 #include "Framework/PrivateHeader.h"
 
 // CHECK: header_stat_before_open_input.o
-// CHECK-NEXT: header_stat_before_open_input.m
 // CHECK-NEXT: Inputs{{/|\\}}frameworks{{/|\\}}Framework.framework{{/|\\}}Headers{{/|\\}}Framework.h
 // CHECK-NEXT: Inputs{{/|\\}}frameworks{{/|\\}}Framework.framework{{/|\\}}PrivateHeaders{{/|\\}}PrivateHeader.h
+// CHECK-NEXT: header_stat_before_open_input.m

clang/test/ClangScanDeps/headerwithdirname.cpp

 // RUN: rm -rf %t.dir
 // RUN: rm -rf %t.dir/foodir
 // RUN: rm -rf %t.cdb
 // RUN: mkdir -p %t.dir
 // RUN: mkdir -p %t.dir/foodir
 // RUN: cp %s %t.dir/headerwithdirname_input.cpp
 // RUN: mkdir %t.dir/Inputs
 // RUN: cp %S/Inputs/foodir %t.dir/Inputs/foodir
 // RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirname.json > %t.cdb
 //
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
 
 #include <foodir>
 
 // CHECK: headerwithdirname_input.o
-// CHECK-NEXT: headerwithdirname_input.cpp
 // CHECK-NEXT: Inputs{{/|\\}}foodir
+// CHECK-NEXT: headerwithdirname_input.cpp

clang/test/ClangScanDeps/headerwithdirnamefollowedbyinclude.cpp

 // RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/headerwithdirnamefollowedbyinclude.json > %t.cdb
 //
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
 
 #include <foodir>
 #include "foodir/foodirheader.h"
 
 // CHECK: headerwithdirname_input.o
-// CHECK-NEXT: headerwithdirname_input.cpp
 // CHECK-NEXT: Inputs{{/|\\}}foodir
+// CHECK-NEXT: foodir{{/|\\}}foodirheader.h
+// CHECK-NEXT: headerwithdirname_input.cpp

clang/test/ClangScanDeps/no-werror.cpp

 // RUN: rm -rf %t.dir
 // RUN: rm -rf %t.cdb
 // RUN: mkdir -p %t.dir
 // RUN: cp %s %t.dir/no-werror_input.cpp
 // RUN: mkdir %t.dir/Inputs
 // RUN: cp %S/Inputs/sys-header.h %t.dir/Inputs/sys-header.h
 // RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/no-werror.json > %t.cdb
 //
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
 
 #define MACRO 201411L
 
 #include "sys-header.h"
 
-// CHECK: no-werror_input.cpp
-// CHECK-NEXT: Inputs{{/|\\}}sys-header.h
+// CHECK: Inputs{{/|\\}}sys-header.h
+// CHECK-NEXT: no-werror_input.cpp

clang/test/ClangScanDeps/non-header-dependency.cpp

+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: cp %s %t.dir/non-header-dependency_input.cpp
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %S/Inputs/sanitize-blacklist.txt %t.dir/Inputs/sanitize-blacklist.txt
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/non-header-dependency.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#define FOO "foo"
+
+// CHECK: Inputs{{/|\\}}sanitize-blacklist.txt
+// CHECK-NEXT: non-header-dependency_input.cpp

clang/test/ClangScanDeps/regular_cdb.cpp

 // RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess-minimized-sources | \
 // RUN:   FileCheck --check-prefix=CHECK2 %s
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 2 -mode preprocess | \
 // RUN:   FileCheck --check-prefix=CHECK2 %s
 
 #include "header.h"
 
 // CHECK1: regular_cdb_input2.cpp
-// CHECK1-NEXT: regular_cdb_input2.cpp
 // CHECK1-NEXT: Inputs{{/|\\}}header.h
 // CHECK1-NEXT: Inputs{{/|\\}}header2.h
+// CHECK1-NEXT: regular_cdb_input2.cpp
 
-// CHECK2: regular_cdb_input.cpp
+// CHECK2: regular_cdb_input.o
 // CHECK2-NEXT: Inputs{{/|\\}}header.h
-// CHECK2NO-NOT: header2
+// CHECK2NO-NOT: header2.h
+// CHECK2-NEXT: regular_cdb_input.cpp

clang/test/ClangScanDeps/subframework_header_dir_symlink.m

 
 #ifndef EMPTY
 #include "Framework/Framework.h"
 #endif
 
 // CHECK: subframework_header_dir_symlink_input.o
 // CHECK-NEXT: subframework_header_dir_symlink_input.m
 // CHECK: subframework_header_dir_symlink_input2.o
-// CHECK-NEXT: subframework_header_dir_symlink_input2.m
 // CHECK-NEXT: Inputs{{/|\\}}frameworks_symlink{{/|\\}}Framework.framework{{/|\\}}Headers{{/|\\}}Framework.h
+// CHECK-NEXT: subframework_header_dir_symlink_input2.m

clang/test/ClangScanDeps/symlink.cpp

 // REQUIRES: shell
 // RUN: rm -rf %t.dir
 // RUN: rm -rf %t.cdb
 // RUN: mkdir -p %t.dir
 // RUN: cp %s %t.dir/symlink_input.cpp
 // RUN: cp %s %t.dir/symlink_input2.cpp
 // RUN: mkdir %t.dir/Inputs
 // RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
 // RUN: ln -s %t.dir/Inputs/header.h %t.dir/Inputs/symlink.h
 // RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/symlink_cdb.json > %t.cdb
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -reuse-filemanager=0 | FileCheck %s
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 -reuse-filemanager=1 | FileCheck %s
 
 #include "symlink.h"
 #include "header.h"
 
-// CHECK: symlink_input.cpp
+// CHECK: Inputs{{/|\\}}header.h
 // CHECK-NEXT: Inputs{{/|\\}}symlink.h
-// CHECK-NEXT: Inputs{{/|\\}}header.h
+// CHECK-NEXT: symlink_input.cpp
 
-// CHECK: symlink_input2.cpp
+// CHECK: Inputs{{/|\\}}header.h
 // CHECK-NEXT: Inputs{{/|\\}}symlink.h
-// CHECK-NEXT: Inputs{{/|\\}}header.h
+// CHECK-NEXT: symlink_input2.cpp
 No newline at end of file

clang/test/ClangScanDeps/vfsoverlay.cpp

 // RUN: rm -rf %t.dir
 // RUN: rm -rf %t.cdb
 // RUN: mkdir -p %t.dir
 // RUN: cp %s %t.dir/vfsoverlay_input.cpp
 // RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/vfsoverlay.yaml > %t.dir/vfsoverlay.yaml
 // RUN: mkdir %t.dir/Inputs
 // RUN: cp %S/Inputs/header.h %t.dir/Inputs/header.h
 // RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/vfsoverlay_cdb.json > %t.cdb
 //
 // RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | \
 // RUN:   FileCheck %s
 
 #include "not_real.h"
 
 // CHECK: vfsoverlay_input.o
-// CHECK-NEXT: vfsoverlay_input.cpp
 // CHECK-NEXT: Inputs{{/|\\}}header.h
+// CHECK-NEXT: vfsoverlay_input.cpp

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

 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Options.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/Threading.h"
 #include <mutex>
+#include <set>
 #include <thread>
 
 using namespace clang;
 using namespace tooling::dependencies;
 
 namespace {
 
 class SharedStream {
llvm::Expected<std::string> getDependencyFile(const std::string &Input,
                                                 StringRef CWD) {
     /// Prints out all of the gathered dependencies into a string.
     class DependencyPrinterConsumer : public DependencyConsumer {
     public:
       void handleFileDependency(const DependencyOutputOptions &Opts,
                                 StringRef File) override {
         if (!this->Opts)
           this->Opts = std::make_unique<DependencyOutputOptions>(Opts);
-        Dependencies.push_back(File);
+        Dependencies.insert(File);
       }
 
       void printDependencies(std::string &S) {
         if (!Opts)
           return;
 
         class DependencyPrinter : public DependencyFileGenerator {
         public:
           DependencyPrinter(DependencyOutputOptions &Opts,
-                            ArrayRef<std::string> Dependencies)
+                            std::set<std::string> &Dependencies)
               : DependencyFileGenerator(Opts) {
             for (const auto &Dep : Dependencies)
               addDependency(Dep);
           }
 
           void printDependencies(std::string &S) {
             llvm::raw_string_ostream OS(S);
             outputDependencyFile(OS);
           }
         };
 
         DependencyPrinter Generator(*Opts, Dependencies);
         Generator.printDependencies(S);
       }
 
     private:
       std::unique_ptr<DependencyOutputOptions> Opts;
-      std::vector<std::string> Dependencies;
+      std::set<std::string> Dependencies;
     };
 
     DependencyPrinterConsumer Consumer;
     auto Result =
         Worker.computeDependencies(Input, CWD, Compilations, Consumer);
     if (Result)
       return std::move(Result);
     std::string Output;