This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Improve VirtualCallChecker diagnostics and move to optin package.
ClosedPublic

Authored by dcoughlin on Nov 16 2016, 2:42 PM.

Details

Summary

The VirtualCallChecker is in alpha because its interprocedural diagnostics represent the call path textually in the diagnostic message rather than with a path sensitive diagnostic.

This patch turns off the AST-based interprocedural analysis in the checker so that no call path is needed and improves with diagnostic text. With these changes, the checker is ready to be moved into the optin package.

Ultimately the right fix is to rewrite this checker to be path sensitive -- but there is still value in enabling the checker for intraprocedural analysis only. The interprocedural mode can be re-enabled with an -analyzer-config flag.

Diff Detail

Repository
rL LLVM

Event Timeline

dcoughlin updated this revision to Diff 78263.Nov 16 2016, 2:42 PM
dcoughlin retitled this revision from to [analyzer] Improve VirtualCallChecker diagnostics and move out of alpha.
dcoughlin updated this object.
dcoughlin added reviewers: zaks.anna, NoQ.
dcoughlin added subscribers: cfe-commits, alexfh.
NoQ accepted this revision.Nov 17 2016, 12:10 AM
NoQ edited edge metadata.

LGTM!

test/Analysis/virtualcall.h
23 ↗(On Diff #78263)

Hmm, line break handling is quite strange.

This revision is now accepted and ready to land.Nov 17 2016, 12:10 AM
zaks.anna edited edge metadata.Nov 28 2016, 3:14 PM

Not sure if we should make pure vs not an option so that users could turn the checking off. Is there a way to suppress the warning?

lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
210 ↗(On Diff #78263)

Please, add an intra-procedual test case for pure in we don't have one already.

212 ↗(On Diff #78263)

Can we detect if we are in a constructor or if we are in the destructor and make the error message more precise?

dcoughlin updated this revision to Diff 79981.Dec 1 2016, 2:18 PM
dcoughlin edited edge metadata.
  • Update the diagnostic to be explicit about whether the issue occurs during construction or destruction
  • Add test for pure, intraprocedural case (Sema also catches this).
dcoughlin updated this revision to Diff 79992.Dec 1 2016, 3:48 PM
  • Add a PureOnly analyzer-config option that, when set, will limit diagnostics to calls to only pure virtual functions.

This should have gone in with the prevision updated diff; my apologies for the noise.

dcoughlin marked 2 inline comments as done.Dec 1 2016, 3:50 PM

Not sure if we should make pure vs not an option so that users could turn the checking off. Is there a way to suppress the warning?

There is not a way to suppress this. I've added an analyzer-config option to limit diagnostics to pure-virtual calls only.

zaks.anna accepted this revision.Dec 1 2016, 10:00 PM
zaks.anna edited edge metadata.

Looks great!
Thank you.

I evaluated this checker on three internal codebases that make large use of virtual functions.

Project 1: ~190,000 lines of C++. 16 alarms. I triaged all of them. There were 2 definite false positives (FPs) and 14 likely FPs.
Project 2: ~320,000 lines of C++. 116 alarms. I triaged 45. All likely FPs.
Project 3: ~23,000 lines of C++. 43 alarms. I triaged 13. 3 definite FPs and 10 likely FPs.

The definite false positives were cases where the programmer seemed aware of the semantics of virtual calls during construction/destruction and had each subclass explicitly call its own version of the virtual method in question. The likely false positives were cases where there was no subclass of the constructed class that overrode the method in question.

I think there is value in this checker: virtual calls in constructors are a definite code smell and are hard to get right. But I don't think we can turn it on by default given the sheer number of alarms. I think this needs to stay in alpha until we can reduce the number of false positives

dcoughlin updated this revision to Diff 80953.Dec 9 2016, 2:01 PM
dcoughlin retitled this revision from [analyzer] Improve VirtualCallChecker diagnostics and move out of alpha to [analyzer] Improve VirtualCallChecker diagnostics.
dcoughlin edited edge metadata.

Keep the diagnostic improvements but remove the change to take the checker out of alpha.

dcoughlin updated this revision to Diff 80979.Dec 9 2016, 4:58 PM
dcoughlin retitled this revision from [analyzer] Improve VirtualCallChecker diagnostics to [analyzer] Improve VirtualCallChecker diagnostics and move to optin package..
dcoughlin updated this object.

Move the checker to the optin package. The checker will be off by default but supported. Projects that are interested in adopting the rule of 'no virtual calls in constructors/destructors' can turn this checker on explicitly.

This revision was automatically updated to reflect the committed changes.

The definite false positives were cases where the programmer seemed aware of the semantics of virtual calls during construction/destruction and had each subclass explicitly call its own version of the virtual method in question.

How is this avoiding the check for a qualifier on the call?

if (CME->getQualifier())
  callIsNonVirtual = true;

The likely false positives were cases where there was no subclass of the constructed class that overrode the method in question.

I'd like to be told about these so that the class can be marked final.

The definite false positives were cases where the programmer seemed aware of the semantics of virtual calls during construction/destruction and had each subclass explicitly call its own version of the virtual method in question.

How is this avoiding the check for a qualifier on the call?

if (CME->getQualifier())
  callIsNonVirtual = true;

They didn't use the qualified version. Rather, each class had duplication of the same logic calling the virtual method its individual constructor/destructor. In the cases I examined either the virtual method didn't call super (most common pattern) or it did and was idempotent if called multiple times.

The likely false positives were cases where there was no subclass of the constructed class that overrode the method in question.

I'd like to be told about these so that the class can be marked final.

I agree, but this produces too many alarms to be on by default in the analyzer. I put the check in it in the 'optin' package so individual projects can decide whether they want to enforce this rule or not.