Page MenuHomePhabricator

[clang-scan-deps][static analyzer] Support for clang --analyze in scan-deps
ClosedPublic

Authored by jkorous on Sep 26 2019, 10:40 AM.

Details

Summary

This is v2 of https://reviews.llvm.org/D67938 which I reverted as it broke clang-tidy and there wasn't any nice fix.

The full list of goals here is:

  • Support commands with --analyze in compilation database for clang-scan-deps.
  • Keep the macro spelling in one place only.
  • Don't break clang-tidy which relies on the fact that __clang_analyzer__ is defined as built-in macro. Otherwise it's treated as part of the input and exposed in diagnostics - specifically the test was failing because macros should be lowercase or uppercase depending on clang-tidy config.

Two things I'd appreciate feedback on:

  • name for the new option
  • preferences whether the option should be automatically set to true whenever cc1 action is RunAnalysis or whether it should always be set by driver in case input args contain --analyze

Diff Detail

Event Timeline

jkorous created this revision.Sep 26 2019, 10:40 AM
arphaman added inline comments.Sep 26 2019, 12:32 PM
clang/test/ClangScanDeps/static-analyzer.c
12

Please use existing Inputs/header.h instead of adding a new empty file.

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

Clang's driver should pass this, otherwise users of the ScanDeps library won't be able to rely on this behavior if they don't use the clang-scan-deps tool, but just the library itself.

jkorous updated this revision to Diff 222019.Sep 26 2019, 1:15 PM

Adressed comments.

Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 1:15 PM
jkorous marked 4 inline comments as done.Sep 26 2019, 1:16 PM
jkorous added inline comments.
clang/test/ClangScanDeps/static-analyzer.c
12

Good point!

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

Thanks, I was thinking only about the tool.

arphaman added inline comments.Sep 26 2019, 4:25 PM
clang/tools/clang-scan-deps/ClangScanDeps.cpp
265

This code now should no longer be necessary.

jkorous updated this revision to Diff 223628.Oct 7 2019, 11:04 AM
jkorous marked 3 inline comments as done.

Addressed comment.

hiraditya added inline comments.
clang/include/clang/Driver/CC1Options.td
849

The name doesn't quite reflect what it does.

jkorous marked an inline comment as done.Oct 7 2019, 2:18 PM
jkorous added inline comments.
clang/include/clang/Driver/CC1Options.td
849

setup-pp-for-analyzer? I'm open to suggestions.

NoQ added inline comments.Oct 7 2019, 5:48 PM
clang/include/clang/Driver/CC1Options.td
849

I actually suggest modifying the help text to something like "Behave as if the Static Analyzer is going to be invoked, even if it's not actually going to be invoked (for now this boils down to defining the clang_analyzer macro)" and keep the flag name roughly the same. This is the actual purpose of the option, right? We don't need to specify what precisely takes place when the option is invoked because this may change in the future, but the contract will remain.

jkorous marked an inline comment as done.Oct 7 2019, 7:41 PM
jkorous added inline comments.
clang/include/clang/Driver/CC1Options.td
849

Sorry, there were quite a few changes in the patch and it deviated from the original idea. After the most recent one the description wouldn't be correct anymore - please take a look at clang/lib/Driver/ToolChains/Clang.cpp below.
The -setup-static-analyzer flag can be passed on its own to frontend (without -analyze) but if you pass --analyze to driver then both -analyze and -setup-static-analyzer are passed to frontend.
Passing just -analyze without -setup-static-analyzer to frontend would result in skipping the __clang_analyzer__ macro definition.

This revision is now accepted and ready to land.Oct 8 2019, 12:38 PM

@NoQ , @hiraditya any suggestions for the option name and/or description?

NoQ accepted this revision.Oct 14 2019, 12:30 PM

Looks great, thanks!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2019, 1:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript