Page MenuHomePhabricator

[analyzer][NFC] Fix some incorrect uses of -analyzer-config options
ClosedPublic

Authored by Szelethus on Oct 15 2018, 2:43 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 found some flags that should've been used as checker options, or have absolutely no mention of in AnalyzerOptions, or are nonexistent.

  • NonLocalizedStringChecker now uses its "AggressiveReport" flag as a checker option
  • lib/StaticAnalyzer/Frontend/ModelInjector.cpp now accesses the "model-path" option through a getter in AnalyzerOptions
  • -analyzer-config path-diagnostics-alternate=false is not a thing, I removed it,
  • lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp and lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h are weird, they actually only contain an option getter. I deleted them, and fixed RetainCountChecker to get it's "leak-diagnostics-reference-allocation" option as a checker option,
  • "region-store-small-struct-limit" has a proper getter now.

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Oct 15 2018, 2:43 AM
Szelethus retitled this revision from [analyzer][NFC] Fix some incorrect uses of AnalyzerOptions to [analyzer][NFC] Fix some incorrect uses of -analyzer-config options.Oct 15 2018, 4:17 AM
NoQ added inline comments.
lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
1401–1403

Nice :)

I'll poke Devin about this, please wait on me before committing.

Szelethus updated this revision to Diff 170315.Oct 20 2018, 1:25 PM
Szelethus edited the summary of this revision. (Show Details)

Added two more fixes, and added this to the summary:

  • lib/StaticAnalyzer/Checkers/AllocationDiagnostics.cpp and lib/StaticAnalyzer/Checkers/AllocationDiagnostics.h are weird, they actually only contain an option getter. I deleted them, and fixed RetainCountChecker to get it's "leak-diagnostics-reference-allocation" option as a checker option,
  • "region-store-small-struct-limit" has a proper getter now.
Szelethus added inline comments.Oct 20 2018, 1:26 PM
lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
1401–1403

I added a similar change to RetainCountChecker, it takes its "leak-diagnostics-reference-allocation" as a checker option. Mention that too I guess.

Szelethus updated this revision to Diff 171130.EditedOct 25 2018, 10:19 AM

RetainCountChecker now gets it's option when registering, like every other checker.

RetainCountChecker stored a reference to AnalyzerOptions, evaluated an option during construction, and one later down the line. This hid an interesting problem when I made IncludeAllocationLine a checker-specific option: since checkers get their names after their construction, it looked for -analyzer-config :IncludeAllocationLine=<val> instead of -analyzer-config osx.cocoa.RetainCount:IncludeAllocationLine=<val>.

Another thing that helped this issue hide so well is that this option has no test associated with it, but that's an unrelated observation.

Followup patches of this refactoring effort will in part focus on catching incorrect uses of AnalyzerOptions like this one.

george.karpenkov requested changes to this revision.Oct 25 2018, 10:31 AM

Minor nitpicks, otherwise makes sense!

include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
756

The comment seems completely distinct from the function name.

758

Comment?

This revision now requires changes to proceed.Oct 25 2018, 10:31 AM

Thanks! I guess it's up to @NoQ whether he and @dcoughlin are okay with making some options checker options.

Szelethus added a comment.EditedOct 29 2018, 11:10 AM

@NoQ did you have time to ping Devin about this? (Sorry for the early ping, but this patch is blocking a lot of my other patches.)

NoQ accepted this revision.Nov 1 2018, 8:54 PM

Yup, seems fine, please feel free to land! The AggressiveReport option looks unused, we should probably remove it. Sry it took so long><

This revision was not accepted when it landed; it landed in state Needs Review.Nov 2 2018, 8:50 AM
This revision was automatically updated to reflect the committed changes.