Page MenuHomePhabricator

[analyzer][NFC] Add different interestingness kinds
ClosedPublic

Authored by Szelethus on Aug 4 2019, 11:21 AM.

Details

Summary

We defined (on the mailing list and here on phabricator) 2 different cases where retrieving information about a control dependency condition is very important:

  1. When the condition's last write happened in a different stack frame
  2. When the collapse point of the condition (when we can constrain it to be true/false) didn't happen in the actual condition.

It seems like we solved this problem with the help of expression value tracking, and have started working on better diagnostics notes about this process.

Expression value tracking is nothing more than registering a variety of visitors to construct reports about it. Each of the registered visitors (ReturnVisitor, FindLastStoreVisitor, NoStoreFuncVisitor, etc) have something to go by: a MemRegion, an SVal, an ExplodedNode, etc. For this reason, better explaining a last write is super simple, we can always just pass on some more information to the visitor in question (as seen in D65575).

ConditionBRVisitor is a different beast, as it was built for a different purpose. It is responsible for constructing events at, well, conditions, and is registered only once, and isn't a part of the "expression value tracking family". Unfortunately, it is also the visitor to tinker with for constructing better diagnostics about the collapse point problem.

This creates a need for alternative way to communicate with ConditionBRVisitor that a specific condition is being tracked for for the reason of being a control dependency. Since at almost all PathDiagnosticEventPiece construction the visitor checks interestingness, it makes sense to pair interestingness with a reason as to why we marked an entity as such.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Aug 4 2019, 11:21 AM
NoQ accepted this revision.Aug 7 2019, 3:09 PM
NoQ added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
92–101 ↗(On Diff #213258)

We usually rely on our "unhandled switch case" warnings for noticing all the places which we need to expand when we're adding a new case. Your approach requires adding static asserts all over the place instead - not sure what's better. I guess they're not generally mutually exclusive, however NumTrackingKinds makes it impossible to handle all cases in a switch (because NumTrackingKinds itself wouldn't ever be handled). I took some sort of middleground approach in CFGTerminator::Kind in D61814 but i'm very much open to suggestions.

This revision is now accepted and ready to land.Aug 7 2019, 3:09 PM

As this thing will now be part of the checker interface it would be great to have some guidelines which interestingness kind to use (or, when to use interestingness at all). I am totally fine with addressing this in a followup patch though.

Szelethus marked an inline comment as done.Aug 21 2019, 12:24 PM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
92–101 ↗(On Diff #213258)

I agree with your reasoning. I decided to remove NumTrackingKinds and use a switch AND llvm_unreachable as well if the warning wasn't scary enough.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2019, 2:34 PM