Page MenuHomePhabricator

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

Authored by OikawaKirie on Feb 23 2021, 1:45 AM.

Details

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.

Diff Detail

Event Timeline

OikawaKirie requested review of this revision.Feb 23 2021, 1:45 AM
OikawaKirie created this revision.

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
80

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.

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
80

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 ...
214–222

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.

216

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 retitled this revision from [clang] Allow clang-check to customize output file name to [clang] Allow clang-check to customize analyzer output file or dir name.
OikawaKirie edited the summary of this revision. (Show Details)
  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.

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

@sammccall ping.

Updated as you mentioned in D97265#2581653

ping @sammccall again

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

sammccall accepted this revision.Wed, Apr 28, 4:23 AM

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

This revision is now accepted and ready to land.Wed, Apr 28, 4:23 AM

If this patch is OK, could you please commit it on my behalf (Ella Ma <alansnape3058@gmail.com>)?
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.

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