Page MenuHomePhabricator

[analyzer][NFC] Collect all -analyzer-config options in a .def file
ClosedPublic

Authored by Szelethus on Oct 15 2018, 3:09 AM.

Details

Summary

I'm in the process of refactoring AnalyzerOptions. The main motivation behind here is to emit warnings if an invalid -analyzer-config option is given from the command line, and be able to list them all.

In this patch, I'm moving all analyzer options to a def file, and move 2 enums to global namespace.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Oct 15 2018, 3:09 AM

Also, this patch does not contain checker options. That I would suspect be a little more invasive, so I'll do that in a followup patch.

Szelethus updated this revision to Diff 169688.Oct 15 2018, 5:43 AM
Szelethus added inline comments.Oct 15 2018, 6:23 AM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
128–135 ↗(On Diff #169688)

Do we actually ever use UMK_SHALLOW? If not, the whole .def file can be simplified, because the use of getDefaultValForUserMode(/* ShallowVal */, /* DeepVal */) is clunky. If we do, I'd also be fine with rewriting the function generating macro from

#define ANALYZER_OPTION_WITH_FN(TYPE, NAME, CMDFLAG, DESC, DEFAULT_VAL, \
                                CREATE_FN)

to

#define ANALYZER_OPTION_WITH_FN(TYPE, NAME, CMDFLAG, DESC, SHALLOW_VAL, \
                                DEEP_VAL, CREATE_FN)

because that'd be nicer than the current solution.

Szelethus updated this revision to Diff 169696.Oct 15 2018, 6:38 AM
Szelethus updated this revision to Diff 169697.Oct 15 2018, 6:40 AM
Szelethus updated this revision to Diff 169731.EditedOct 15 2018, 11:24 AM
Szelethus retitled this revision from [analyzer][NFC][WIP] Collect all -analyzer-config options in a .def file to [analyzer][NFC] Collect all -analyzer-config options in a .def file.
Szelethus edited the summary of this revision. (Show Details)

Added descriptions.

Now that they are here, is there a point in keeping doxygen comments? They are essentially the same and AFAIK doxygen won't pick those up anyways.

dkrupp added a subscriber: dkrupp.Oct 16 2018, 1:39 AM
NoQ accepted this revision.Oct 19 2018, 6:43 PM

I think this is awesome o_o

include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
70–71 ↗(On Diff #169731)

I think it's unnecessary to duplicate the --help text in comments.

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
128–135 ↗(On Diff #169731)

Xcode uses shallow mode when you tick the checkbox to run static analyzer during normal build, so that not to slow down builds too much.

rewriting the function generating macro

This seems indeed fancier because it makes everything constexpr, which opens up more possibilities. But i don't see any actual possibilities that it opens up. I guess we can always change it when it becomes necessary. Also it's probably better to make a separate macro for options that have different default values in shallow and deep modes, so that not to duplicate every default value twice.

This revision is now accepted and ready to land.Oct 19 2018, 6:43 PM
Szelethus updated this revision to Diff 170318.EditedOct 20 2018, 2:36 PM

Removed doxygen comments, rebased to D53276.

Szelethus added a comment.EditedOct 20 2018, 2:44 PM

I indent to refactor AnalyzerOptions a lot more, but this patch only regenerates what's already in it now.

In D53277#1269960, @NoQ wrote:

I think this is awesome o_o

I'm glad you like it ^-^

include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
386 ↗(On Diff #170318)

Found the use for this here: https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Frontend/ModelInjector.cpp#L51

Which was added by this commit:
https://github.com/llvm-mirror/clang/commit/fdf0d3513240fd8e4da6942e9cd26d2d730eb37b#diff-6e67e63f578935f02bd1d5b20488ea8c

This patch was contributed by Gábor Horváth as part of his Google Summer of Code project.

@xazax.hun, what would be a good description for this flag?

Szelethus updated this revision to Diff 170333.Oct 21 2018, 5:28 AM

Split ANALYZER_OPTION_WITH_FN up to

  • ANALYZER_OPTION_GEN_FN
  • ANALYZER_OPTION_GEN_FN_DEPENDS_ON_USER_MODE

and added some #error directives to make it harder to misuse the .def file.

Szelethus added inline comments.Oct 21 2018, 5:33 AM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
386 ↗(On Diff #170318)

This inline was meant for:
ANALYZER_OPTION_GEN_FN(StringRef, ModelPath, "model-path", "", "", getModelPath)

Added default value to ANALYZER_OPTION and split it upto

  • ANALYZER_OPTION and
  • ANALYZER_OPTION_DEPENDS_ON_USER_MODE.

The TYPE entry only holds bool, unsigned and `StringRef now. These things will make followup patches significantly more pleasant to implement.

xazax.hun added inline comments.Oct 22 2018, 2:43 AM
include/clang/StaticAnalyzer/Core/AnalyzerOptions.def
445 ↗(On Diff #170343)

Should we explain the feature in the commad line path?

The description might be somethinge like:
(string) The analyzer can inline an alternative implementation written in C at the call site if the called function's body is not available. This is a path where to look for those alternative implementations (called models).

Szelethus updated this revision to Diff 170417.Oct 22 2018, 8:00 AM
  • Removed redundant information (like the type or default value of the command line argument), these are now auto-generated in followup patches
  • Added description for "model-path".
NoQ accepted this revision.Nov 1 2018, 9:03 PM
In D53277#1269960, @NoQ wrote:

I think this is awesome o_o

I'm glad you like it ^-^

I mean, do i have a choice? Like, seriously, it's... beautiful...

This revision was automatically updated to reflect the committed changes.
NoQ added a comment.Nov 2 2018, 11:19 AM

Ugh, this breaks builds with modules, eg. http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/12074/console

I'm bad with modules, but you might consult my own failure of this kind: https://reviews.llvm.org/D15448#325856 - especially the include/clang/module.modulemap part.

There must be a cmake flag for this. I guess please try to revert and reproduce the build failure locally and ask me if you need any help with that :)

NoQ reopened this revision.Nov 2 2018, 11:19 AM
This revision is now accepted and ready to land.Nov 2 2018, 11:19 AM

@Szelethus you can see the command required to get modules build in the mentioned log. I think -DLLVM_ENABLE_MODULES=On might be enough.

@Szelethus @NoQ OK it seems you need to annotate those headers in modulemap.

E.g. take a look at:

george1@/Volumes/Transcend/code/monorepo/llvm-project/clang (master)≻ rg SVals.def
include/clang/module.modulemap
131:  textual header "StaticAnalyzer/Core/PathSensitive/SVals.def"

include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h
58:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
68:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
82:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"

include/clang/StaticAnalyzer/Core/PathSensitive/SValVisitor.h
39:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
44:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
51:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"
66:#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.def"

include/clang/StaticAnalyzer/Core/PathSensitive/SVals.def
1://===-- SVals.def - Metadata about SVal kinds -------------------*- C++ -*-===//

@NoQ @Szelethus Actually pushed a fix.

Just saw this -- Thank you! :D

Szelethus closed this revision.Nov 2 2018, 11:42 AM

Interesting, I didn't get a mail about this :/ In any case, let's hope no more problems arise.