This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][NFC] Prepare visitors for different tracking kinds
ClosedPublic

Authored by Szelethus on Jul 5 2019, 2:42 PM.

Details

Summary

When we're tracking a variable that is responsible for a null pointer dereference or some other sinister programming error, we of course would like to gather as much information why we think that the variable has that specific value as possible. However, the newly introduced condition tracking shows that tracking all values this thoroughly could easily cause an intolerable growth in the bug report's length.

Take, for instance the following report as an example: BEFORE condition tracking, AFTER condition tracking. Having an already lengthy, bug report with 18 events grew to 45 is inexcusable.

There are a variety of heuristics we discussed on the mailing list to combat this, all of them requiring to differentiate in between tracking a "regular value" and a "condition".

This patch introduces the new bugreporter::TrackingKind enum, adds it to FindLastStoreBRVisitor as a non-optional argument, and moves some functions around to make the code a little more coherent.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Jul 5 2019, 2:42 PM
NoQ accepted this revision.Jul 5 2019, 9:59 PM

Approve!

null pointer dereference

...should in my opinion be tracked in a mild manner, as if it's a condition, because D62978. I think the definition we're looking for is "when we care only about why it's null, regardless of what it is or where it comes from".

Say, in case of MallocChecker the malloc() call is the "origin" of the tracked pointer; we need to track it back to its original malloc. Similarly, in case of null dereference, the collapse point should be treated as an "origin" of the null pointer. The same for condition.

This revision is now accepted and ready to land.Jul 5 2019, 9:59 PM
Charusso accepted this revision.Jul 6 2019, 2:21 AM

Document!!4!44! It is great you have started to limit the notes, thanks!

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
110 ↗(On Diff #208241)

What about the following?

enum class TrackKind {
  Full,     // comment what it does
  Condition // comment what it does
};

Please consider the following example by unspoken naming conventions:

enum class ColoringKind {
  RedColoring,
  BlueColoring
}

would be

enum class Color { Red, Blue }

where each trivial what is does, by name.

Please also note that, the thoroughness not working with redecls, assumptions, loop conditions, anything we failed to inline, etc... so it is not really that full tracking. But in our capabilities, it is the full what we could do.

xazax.hun added inline comments.Jul 17 2019, 11:38 AM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
81 ↗(On Diff #210337)

Do we need this overload? What about a default argument?

Szelethus marked an inline comment as done.Jul 17 2019, 1:17 PM
Szelethus added inline comments.
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
110 ↗(On Diff #208241)

There's a good reasoning behind this -- if you take a look at this, and followup patches, the behavior the different TrackingKinds cause may differ from visitor to visitor, so I decided to document each behavior there. That way, you can't avoid the documents even if you didn't glance over this enum.

Please also note that, the thoroughness not working with redecls, assumptions, loop conditions, anything we failed to inline, etc... so it is not really that full tracking. But in our capabilities, it is the full what we could do.

I don't see what you mean here?

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
81 ↗(On Diff #210337)

I don't want to expose this functionality outside of this file. I don't insist though!

NoQ added inline comments.Jul 17 2019, 3:50 PM
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
81 ↗(On Diff #210337)

I think we'll have to expose it because different checkers would want different tracking kinds for their values. For instance, null dereference checker could really use condition tracking as its default tracking mode.

Szelethus updated this revision to Diff 211752.Jul 25 2019, 7:50 AM
  • Make sure that the tracking kind is forwarded for each invocation of trackExpressionValue in BugReporterVisitor.cpp
  • Expose the tracking kind to all users
  • Add some more docs
Szelethus marked 3 inline comments as done.Jul 25 2019, 7:51 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 5:48 PM