Page MenuHomePhabricator

[analyzer][WIP] Enable subcheckers to possess checker options
ClosedPublic

Authored by Szelethus on Feb 1 2019, 4:29 AM.

Details

Summary

Under the term "subchecker", I mean checkers that do not have a checker class on their own, like unix.MallocChecker to unix.DynamicMemoryModeling.

Since a checker object was required in order to retrieve checker options, subcheckers couldn't possess options on their own. This patch also aims to fix a bug mentioned in D54438#1375858, but since I couldn't reproduce the error, I'm marking it as WIP.

This patch is also an excuse to change the argument order of getChecker*Option, it always bothered me, now it resembles the actual command line argument (checkername:option=value).

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Feb 1 2019, 4:29 AM
Herald added a project: Restricted Project. · View Herald Transcript
Szelethus marked an inline comment as done.Feb 1 2019, 5:02 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
1466–1478 ↗(On Diff #184720)

@george.karpenkov This hack should work and does work on lit tests. Could you please provide a file that should work but fails?

Also, removing this hack is a real headache: While it should be a feature to either enable osx.cocoa.RetainCount or osx.OSObjectRetainCount, it should also be possible to disable the latter through osx.cocoa.RetainCount's checker option. This would both imply and make it impossible at the same time for the two to have some sort of circular dependency. I'm struggling to come up with a "hack-free" solution to this.

The elegant way out would be to move all checker option acquisitions to the registry functions, buuuuut (unless we hardcode it) osx.OSObjectRetainCount can't obtain the name "osx.cocoa.RetainCount". So,

Does CheckOSObject have to belong to osx.cocoa.RetainCount? Is this already used? If not, simply moving it to the other one would be a perfect solution. If it is, for the following invocation

clang blahblah -analyze and so on \
  -analyzer-disable-checker=osx.cocoa.RetainCount \
  -analyzer-checker=osx.OSObjectRetainCount \
  -analyzer-config osx.cocoa.RetainCount:CheckOSObject=false"

since the checker registry function for osx.cocoa.RetainCount wouldn't be called, osx.OSObjectRetainCount would be enabled, but is that that big of a problem?

But, if hardcoding isn't painful, that's obviously the cheapest solution. And is surely backward compatible.

Thoughts on this? :)

Szelethus edited the summary of this revision. (Show Details)Feb 1 2019, 5:34 AM
xazax.hun added inline comments.Feb 1 2019, 6:32 AM
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
351

While I do know it is not your code, but we should not validate user input with asserts. Maybe adding a fixme here would be useful.

Szelethus marked an inline comment as done.Feb 1 2019, 7:22 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Checkers/PaddingChecker.cpp
351

Well I'm very happy to let you know that I'm already working a solution to this locally, that being the 3rd part of the whole AnalyzerOptions refactoring, which will enable checker options to be listed, validated, and registered just as non-checker options. :)

Until then, I guess this still is some sort of, if not a very nice, input error handling. Since the next release isn't anywhere near, I don't think there's a point in removing this before I overhaul the whole thing.

xazax.hun accepted this revision.Feb 11 2019, 2:33 AM

LGTM! But having a lit test that fails before and passes after would be great.

NoQ accepted this revision.Feb 15 2019, 3:59 PM

since I couldn't reproduce the error

Me neither, actually :/ This patch does provide a backup plan and it makes the code prettier, so we should definitely land it and i'm really greatful for it, but the option seems to work correctly in all possible ways even without it. Long-term, i guess, we could try to unlock ourselves the capability to rename checkers as much as we want by introducing checker aliases (which would work both for the purposes of enabling/disabling checkers and for checker options).

unittests/StaticAnalyzer/AnalyzerOptionsTest.cpp
67

"CheckerTwo" was probably meant here.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 3 2019, 4:30 PM
This revision was automatically updated to reflect the committed changes.