[clang] Allow clang-check to customize analyzer output file or dir name

[OikawaKirie]

Summary

Required by https://stackoverflow.com/questions/58073606

As the output argument is stripped out in the clang-check tool, it seems impossible for clang-check users to customize the output file name, even with -extra-args and -extra-arg-before.

This patch adds the -analyzer-output-path argument to allow users to adjust the output name. And if the argument is not set or the analyzer is not enabled, the original strip output adjuster will remove the output arguments.

[sammccall]

It looks like either we're in syntax-only mode (and -o will be ignored regardless) or we're in analyzer mode, and want -o.
It seems tempting to just stop stripping it in this case, but then we'll end up with -o foo.o -o foo.html, which is no good.
So the logic of this patch seems sound.

---

I wonder about the CLI though, I think the user will have to pass four flags with different styles to use it!

clang-check -analyze ... -extra-arg=-Xanalyzer -extra-arg=-analyzer-output=html -output=html_output ....

(Maybe -analyze should imply -Xanalyzer, and analyzer-output should be promoted to a real flag without the legacy plist default...)

clang/test/Tooling/clang-check-reset-o.cpp

10 ↗

(On Diff #325710)

it looks like you're checking for the analysis finding in stdout of clang-check, rather than in the generated html file.
So i'm not sure this test verifies it actually works!

clang/tools/clang-check/ClangCheck.cpp

84

I think this should have a name with clear semantics like -analyzer-output-file (and only be used in analyzer mode).

While it does just append -o to the command-line, I think it will confuse users to treat it as a generic output flag.

[OikawaKirie]

In D97265#2581653, @sammccall wrote:

It looks like either we're in syntax-only mode (and -o will be ignored regardless) or we're in analyzer mode, and want -o.
It seems tempting to just stop stripping it in this case, but then we'll end up with -o foo.o -o foo.html, which is no good.
So the logic of this patch seems sound.

The strip output adjuster is invoked before appending the new -o option (line 215). Therefore, there will be only one -o option left if the new -output option is set when running clang-check.

I wonder about the CLI though, I think the user will have to pass four flags with different styles to use it!

clang-check -analyze ... -extra-arg=-Xanalyzer -extra-arg=-analyzer-output=html -output=html_output ....

(Maybe -analyze should imply -Xanalyzer, and analyzer-output should be promoted to a real flag without the legacy plist default...)

You are right. It is better if we can add the clang-check version of -Xanalyzer options. However, the analyzer options can only determine the format of the output, rather than the name of the output. Therefore, we still need another option to set the output name.

My original idea is to add a generic way for the clang-check users to customize the output name. Although there is currently only one user, the static analyzer, there may be other users of this option in the future. Therefore, I thought that it would be better to add just a generic output option here.

According to your suggestions, it seems to be a good idea to use -Xanalyzer to set the output name, and the users can have a good experience when using clang-check to analyze their code. However, the disadvantage of this method is that the argument parsing procedure will become more complex. IMO, we can also add a clang-check option to set the analyzer output format together with the analyzer output name, and leave other analyzer options that are not commonly used to the -Xanalyzer option (refer to the second inline comment).

clang/test/Tooling/clang-check-reset-o.cpp

10 ↗

(On Diff #325710)

Different kinds of output formats can be either a directory or a file named after the -o option. It is inconvenient to check the exact output file or directory.

This test case is created by imitating the clang/test/Tooling/clang-check-strip-o.cpp test case, which only checks whether the cc1 arguments are correctly set.

clang/tools/clang-check/ClangCheck.cpp

84

I think this should have a name with clear semantics like -analyzer-output-file (and only be used in analyzer mode).
While it does just append -o to the command-line, I think it will confuse users to treat it as a generic output flag.

My original idea is to add a generic output flag to the clang-check tool, as it seems impossible for tool users to customize the output name. However, it will probably confuse users with the useless -o options set via -extra-arg or appended after -- and with other -fsyntax-only actions, just as what you mentioned.

So, which you think is better? All with -Xanalyzer option

$ clang-check -analyze -Xanalyzer -analyzer-output-name=html_output -Xanalyzer -analyzer-output=html -Xanalyzer -analyzer-other-options=value input1.cc input2.cc ...

OR -analyzer-output-name without -Xanalyzer option

$  clang-check -analyze -analyzer-output-name=html_output -Xanalyzer -analyzer-output=html -Xanalyzer -analyzer-other-options=value input1.cc input2.cc ...

OR even more aggressively, only non-commonly used options with -Xanalyzer option

$  clang-check -analyze -analyzer-output-name=html_output -analyzer-output=html -Xanalyzer -analyzer-other-options=value input1.cc input2.cc ...

215–223

If we use the -analyzer-output-name option, we need to check whether -analyze is enabled and use the value to determine which adjuster should be used.

217

The original -o options will be first eliminated with the clang strip output adjuster in the lambda adjuster. Then, the new -o options will be appended if necessary.

[OikawaKirie]

1. Update option name from -output to -analyzer-output-path. As it can either be a file name or a directory name, the word path may not be accurate. Please give me your suggestions on this option name.
2. Add more verifications on the output files, rather than verifying the value of the -o option. For simplicity, the regression test case only checks the plist format.

[OikawaKirie]

Replace concatenating the output from clang-check and plist with separated checks to workaround the test failure on Windows.

[OikawaKirie]

@sammccall ping.

Updated as you mentioned in D97265#2581653

[OikawaKirie]

ping @sammccall again

Do I need to add other reviewers if you are busy doing your job?

[sammccall]

Really sorry about the long delays. Working on clearing my backlog...
This seems reasonable to me!

[OikawaKirie]

If this patch is OK, could you please commit it on my behalf (Ella Ma <[e-mail]>)?
Thx.
And I will continue working on the -Xanalyzer option for clang-check, as it is too complex when adding analyzer options via the -extra-arg option.

[OikawaKirie]

Ping @sammccall again for landing this patch on my behalf.
Thanks.

[kbobyrev]

Hey, I'm really sorry, I missed this, too but I just landed the patch for you, apologies for such a huge delay.

clang/test/Tooling/clang-check-set-analyzer-output-path.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: echo '[{"directory":".","command":"clang++ -c %t/test.cpp -o foo -ofoo","file":"%t/test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: echo '// CHECK: {{qwerty}}' > %t/cclog-check
+// RUN: clang-check -p "%t" "%t/test.cpp" -analyze -analyzer-output-path=%t/qwerty -extra-arg=-v -extra-arg=-Xclang -extra-arg=-verify 2>&1 | FileCheck %t/cclog-check
+// RUN: FileCheck %s --input-file=%t/qwerty
+
+// CHECK: DOCTYPE plist
+// CHECK: Division by zero
+int f() {
+  return 1 / 0; // expected-warning {{Division by zero}}
+}

clang/tools/clang-check/ClangCheck.cpp

 static cl::opt<std::string> ASTDumpFilter(
     "ast-dump-filter",
     cl::desc(Options.getOptionHelpText(options::OPT_ast_dump_filter)),
     cl::cat(ClangCheckCategory));
 static cl::opt<bool>
     Analyze("analyze",
             cl::desc(Options.getOptionHelpText(options::OPT_analyze)),
             cl::cat(ClangCheckCategory));
+static cl::opt<std::string>
+    AnalyzerOutput("analyzer-output-path",
+                   cl::desc(Options.getOptionHelpText(options::OPT_o)),
+                   cl::cat(ClangCheckCategory));
 
 static cl::opt<bool>
     Fixit("fixit", cl::desc(Options.getOptionHelpText(options::OPT_fixit)),
           cl::cat(ClangCheckCategory));
 static cl::opt<bool> FixWhatYouCan(
     "fix-what-you-can",
     cl::desc(Options.getOptionHelpText(options::OPT_fix_what_you_can)),
     cl::cat(ClangCheckCategory));
 
 static cl::opt<bool> SyntaxTreeDump("syntax-tree-dump",
if (!ExpectedParser) {
     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();
-  Tool.appendArgumentsAdjuster(getClangStripOutputAdjuster());
+
+  // Reset output path if is provided by user.
+  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());
 
   // Running the analyzer requires --analyze. Other modes can work with the
   // -fsyntax-only option.
   Tool.appendArgumentsAdjuster(getInsertArgumentAdjuster(
       Analyze ? "--analyze" : "-fsyntax-only", ArgumentInsertPosition::BEGIN));
 
   ClangCheckActionFactory CheckFactory;