[clang][DependencyScanner] Remove all warning flags when suppressing warnings

[Bigcheese]

Summary

Since system modules don't emit most warnings, remove the warning
flags to increase module reuse.

[jansvoboda11]

clang/test/ClangScanDeps/optimize-system-warnings.m

19

I'd like to see a check line that would fail if the scanner reports another variant of module A. The CHECK: lines make it possible for FileCheck to skip an entire element in the modules array. (Structural matching on JSON in FileCheck would be a godsend.)

30

I'd expect the scanner to still produce two variants of module B, since it's a user module. Is that correct? If so, let's add explicit check for it, since we intentionally want to preserve the warning flags there.

[Bigcheese]

clang/test/ClangScanDeps/optimize-system-warnings.m

19

The way FileCheck works I do not believe this can happen. A CHECK: line finds the first line that matches, and CHECK-NEXT: follows exactly after that. There's no case where we have a CHECK: that can skip over another entire module entry.

30

Both A and B are system modules. A is found via -isystem, and B has the [system] attribute. The FileCheck lines will only succeed if there are exactly two modules returned.

[jansvoboda11]

LGTM, thanks!

clang/test/ClangScanDeps/optimize-system-warnings.m

19

You're right, this is good then.

30

Thanks for pointing that out. Can you put a comment saying just that at the start of the test along with description of the expected behavior?

[ributzka]

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

50

This removes also all warnings when building your own module, because of the [system] attribute. I would prefer this optimization to be limited to modules that are in the sysroot and therefore ignore the [system] attribute.

[Bigcheese]

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

50

This is not really simple to do. We don't track why a module is a system module, and it's not really correct to do a string based match on the path for if it's from a system directory.

[ributzka]

I see. LGTM

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

std::function<void(const serialization::ModuleFile *)> VisitMF =
             VisitMF(Import);
       };
   VisitMF(&MF);
 
   for (auto Idx : SearchPathUsage.set_bits())
     Opts.UserEntries.push_back(Entries[Idx]);
 }
 
+static void optimizeDiagnosticOpts(DiagnosticOptions &Opts,
+                                   bool IsSystemModule) {
+  // If this is not a system module or -Wsystem-headers was passed, don't
+  // optimize.
+  if (!IsSystemModule)
+    return;
+  bool Wsystem_headers = false;
+  for (StringRef Opt : Opts.Warnings) {
+    bool isPositive = !Opt.consume_front("no-");
+    if (Opt == "system-headers")
+      Wsystem_headers = isPositive;
+  }
+  if (Wsystem_headers)
+    return;
+
+  // Remove all warning flags. System modules suppress most, but not all,
+  // warnings.
+  Opts.Warnings.clear();
+  Opts.UndefPrefixes.clear();
+  Opts.Remarks.clear();
+}
+
 static std::vector<std::string> splitString(std::string S, char Separator) {
   SmallVector<StringRef> Segments;
   StringRef(S).split(Segments, Separator, /*MaxSplit=*/-1, /*KeepEmpty=*/false);
   std::vector<std::string> Result;
   Result.reserve(Segments.size());
   for (StringRef Segment : Segments)
     Result.push_back(Segment.str());
   return Result;
MDC.ScanInstance.getASTReader()->visitTopLevelModuleMaps(
       *MF, [&](FileEntryRef FE) {
         if (FE.getNameAsRequested().endswith("__inferred_module.map"))
           return;
         MD.ModuleMapFileDeps.emplace_back(FE.getNameAsRequested());
       });
 
   CompilerInvocation CI = MDC.makeInvocationForModuleBuildWithoutOutputs(
       MD, [&](CompilerInvocation &BuildInvocation) {
-        if (MDC.OptimizeArgs)
+        if (MDC.OptimizeArgs) {
           optimizeHeaderSearchOpts(BuildInvocation.getHeaderSearchOpts(),
                                    *MDC.ScanInstance.getASTReader(), *MF);
+          optimizeDiagnosticOpts(
+              BuildInvocation.getDiagnosticOpts(),
+              BuildInvocation.getFrontendOpts().IsSystemModule);
+        }
       });
 
   MDC.associateWithContextHash(CI, MD);
 
   // Finish the compiler invocation. Requires dependencies and the context hash.
   MDC.addOutputPaths(CI, MD);
 
   MD.BuildArguments = CI.getCC1CommandLine();

clang/test/ClangScanDeps/optimize-system-warnings.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s|DIR|%/t|g" %t/build/compile-commands.json.in > %t/build/compile-commands.json
+// RUN: clang-scan-deps -compilation-database %t/build/compile-commands.json \
+// RUN:   -j 1 -format experimental-full -optimize-args > %t/deps.db
+// RUN: cat %t/deps.db | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// CHECK:      {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "clang-module-deps": [],
+// CHECK-NEXT:       "clang-modulemap-file":
+// CHECK-NEXT:       "command-line": [
+// CHECK-NOT:          "-W
+// CHECK:            ],
+// CHECK-NEXT:       "context-hash": "{{.*}}",
+// CHECK-NEXT:       "file-deps": [
+// CHECK:            ],
+// CHECK-NEXT:       "name": "A"
+// CHECK-NEXT:     },
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "clang-module-deps": [],
+// CHECK-NEXT:       "clang-modulemap-file":
+// CHECK-NEXT:       "command-line": [
+// CHECK-NOT:          "-W
+// CHECK:            ],
+// CHECK-NEXT:       "context-hash": "{{.*}}",
+// CHECK-NEXT:       "file-deps": [
+// CHECK:            ],
+// CHECK-NEXT:       "name": "B"
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "translation-units": [
+// CHECK:        ]
+// CHECK:      }
+
+//--- build/compile-commands.json.in
+
+[
+{
+  "directory": "DIR",
+  "command": "clang -c DIR/A.m -isystem modules/A -I modules/B -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps",
+  "file": "DIR/A.m"
+},
+{
+  "directory": "DIR",
+  "command": "clang -c DIR/B.m -isystem modules/A -I modules/B -fmodules -fmodules-cache-path=DIR/module-cache -fimplicit-module-maps -Wmaybe-unused",
+  "file": "DIR/B.m"
+}
+]
+
+//--- modules/A/module.modulemap
+
+module A {
+  umbrella header "A.h"
+}
+
+//--- modules/A/A.h
+
+typedef int A_t;
+
+//--- modules/B/module.modulemap
+
+module B [system] {
+  umbrella header "B.h"
+}
+
+//--- modules/B/B.h
+
+typedef int B_t;
+
+//--- A.m
+
+#include <A.h>
+#include <B.h>
+
+A_t a = 0;
+
+//--- B.m
+
+#include <A.h>
+#include <B.h>
+
+A_t b = 0;