[clang-check] Adjust argument adjusters for clang-check to strip options blocking the static analyzer

[OikawaKirie]

Summary

Output generation options (like -save-temps) will make the analyzer not executed even --analyze option is provided in the driver arguments.
Besides, the original approach of adding --analyze option will not work when (more than one) -fsyntax-only options are provided in the driver arguments.

This patch fixes these two problems by using the syntax-only adjuster to remove output generation options and manually filter out redundant -fsyntax-only options.

In the new implementation, the adjusters added by ClangTool will not be removed but used as dependencies for clang-check adjusters for analyzer options.

[labath]

I'm sorry, but I don't feel qualified to review this. Last time I saw this file was in 2013.

[sammccall]

clang/tools/clang-check/ClangCheck.cpp

217

you can pass a vector to getInsertArgumentsAdjuster

225

You've removed the mention of the default syntax-only adjuster, so this comment seems confusing.

"The syntax-only adjuster is installed by default.
Good: It strips options that trigger extra output, like --save-temps.
Bad: We don't want the -fsyntax-only arg itself, as it suppresses analyzer output"

Then strip -fsyntax-only, then add -analyze?

236

this seems like a particularly confusing way to say "if (Arg == "-fsyntax-only && !HasAnalyze)"

The "replace first -fsyntax-only with -analyze, strip others" seems more clearly expressed as "strip -fsyntax-only" and "add -analyze" as separate steps.

[OikawaKirie]

Updated as suggested.

[sammccall]

Thanks, LG!

[OikawaKirie]

Could you please commit it on my behalf (Ella Ma <[e-mail]>)?
Thanks a lot.

[OikawaKirie]

It seems that it is not committed on my behalf. Maybe you have forgotten to add me as the author?
Sad : (

clang/test/Tooling/clang-check-analyze-save-temps.cpp

+// Check whether output generation options (like -save-temps) will not affect
+// the execution of the analyzer.
+
+// RUN: clang-check -analyze %s -- -save-temps -c -Xclang -verify
+
+// Check whether redundant -fsyntax-only options will affect the execution of
+// the analyzer.
+
+// RUN: clang-check -analyze %s -- \
+// RUN:   -fsyntax-only -c -fsyntax-only -Xclang -verify 2>&1 | \
+// RUN:   FileCheck %s --allow-empty
+
+// CHECK-NOT: argument unused during compilation: '--analyze'
+
+void a(int *x) {
+  if (x) {
+  }
+  *x = 47; // expected-warning {{Dereference of null pointer}}
+}

clang/tools/clang-check/ClangCheck.cpp

int main(int argc, const char **argv) {
   if (!ExpectedParser) {
     llvm::errs() << ExpectedParser.takeError();
     return 1;
   }
   CommonOptionsParser &OptionsParser = ExpectedParser.get();
   ClangTool Tool(OptionsParser.getCompilations(),
                  OptionsParser.getSourcePathList());
 
-  // Clear adjusters because -fsyntax-only is inserted by the default chain.
-  Tool.clearArgumentsAdjusters();
-
-  // Reset output path if is provided by user.
+  if (Analyze) {
+    // Set output path if is provided by user.
+    //
+    // As the original -o options have been removed by default via the
+    // strip-output adjuster, we only need to add the analyzer -o options here
+    // when it is provided by users.
+    if (!AnalyzerOutput.empty())
       Tool.appendArgumentsAdjuster(
-      Analyze ? [&](const CommandLineArguments &Args, StringRef File) {
-                  auto Ret = getClangStripOutputAdjuster()(Args, File);
-                  if (!AnalyzerOutput.empty()) {
-                    Ret.emplace_back("-o");
-                    Ret.emplace_back(AnalyzerOutput);
-                  }
-                  return Ret;
-                }
-              : getClangStripOutputAdjuster());
-
-  Tool.appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
+          getInsertArgumentAdjuster(CommandLineArguments{"-o", AnalyzerOutput},
+                                    ArgumentInsertPosition::END));
 
     // Running the analyzer requires --analyze. Other modes can work with the
     // -fsyntax-only option.
-  Tool.appendArgumentsAdjuster(getInsertArgumentAdjuster(
-      Analyze ? "--analyze" : "-fsyntax-only", ArgumentInsertPosition::BEGIN));
+    //
+    // The syntax-only adjuster is installed by default.
+    // Good: It also strips options that trigger extra output, like -save-temps.
+    // Bad:  We don't want the -fsyntax-only when executing the static analyzer.
+    //
+    // To enable the static analyzer, we first strip all -fsyntax-only options
+    // and then add an --analyze option to the front.
+    Tool.appendArgumentsAdjuster(
+        [&](const CommandLineArguments &Args, StringRef /*unused*/) {
+          CommandLineArguments AdjustedArgs;
+          for (const std::string &Arg : Args)
+            if (Arg != "-fsyntax-only")
+              AdjustedArgs.emplace_back(Arg);
+          return AdjustedArgs;
+        });
+    Tool.appendArgumentsAdjuster(
+        getInsertArgumentAdjuster("--analyze", ArgumentInsertPosition::BEGIN));
+  }
 
   ClangCheckActionFactory CheckFactory;
   std::unique_ptr<FrontendActionFactory> FrontendFactory;
 
   // Choose the correct factory based on the selected mode.
   if (Analyze)
     FrontendFactory = newFrontendActionFactory<clang::ento::AnalysisAction>();
   else if (Fixit)
     FrontendFactory = newFrontendActionFactory<ClangCheckFixItAction>();
   else if (SyntaxTreeDump || TokensDump)
     FrontendFactory = newFrontendActionFactory<DumpSyntaxTree>();
   else
     FrontendFactory = newFrontendActionFactory(&CheckFactory);
 
   return Tool.run(FrontendFactory.get());
 }