Page MenuHomePhabricator

[analyzer] PR41269: SmartPtrModeling.
ClosedPublic

Authored by NoQ on Apr 16 2019, 2:24 PM.

Details

Summary

A tiny placeholder for a checker to assist modeling of smart pointers. Currently it doesn't emit any bug reports and it does only one thing: returns null for .get() and .release() methods and false for conversion to bool when the pointer is freshly moved from. It interacts with the MoveChecker in order to figure this out, which addresses https://bugs.llvm.org/show_bug.cgi?id=41269

This still doesn't quite follow my global plan of "make it possible to deal with C++ objects as if they're symbols", but i still keep my faith in this plan. Also it might be a good idea to untangle smart-pointer-related parts of MoveChecker and promote it to a more generic "dereference of a null smart pointer" check in the new checker.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Apr 16 2019, 2:24 PM
NoQ added a comment.Apr 16 2019, 2:29 PM

Why alpha? Because it needs a bug visitor to explain what's going on when it suddenly returns a note. Because it's going to be a bugtype-inspecific note that is emitted by a checker, i want to implement it via note tags (D58367), otherwise it's quite a layering nightmare to set up communication between core and checker visitors (especially given that we'd want to attach a new move visitor immediately after emitting the note).

Hi Artem,
There are some comments inline, but looks promising!

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
45 ↗(On Diff #195458)

const?

53 ↗(On Diff #195458)

I find the std::bind syntax a bit overcomplicated. Maybe a lambda would be better?
[Call](const auto &CD) { return Call->isCalled(CD); }

59 ↗(On Diff #195458)
  1. & operator doesn't return nullptr (if not overloaded)
  2. If CallRef can be nullptr, this can be kinda dangerous.

Should we use get() instead?

The patch looks OK in general! Left some comments inline, my main concern is whether this checker actually depends on MoveChecker or not.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
496 ↗(On Diff #195458)

Hmmm, shouldn't this depend from MoveChecker?

Dependencies<[MoveChecker]>,
clang/lib/StaticAnalyzer/Checkers/Move.h
30 ↗(On Diff #195458)
#endif // LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_TAINT_H

:^)

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
236 ↗(On Diff #195458)

Can't we just return just State->contains<TrackedRegionMap>(Region)? Unless you plan to add more values to RegionState::Kind, I think it would be fine.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
53 ↗(On Diff #195458)

+1 :)

NoQ updated this revision to Diff 195613.Apr 17 2019, 11:58 AM
NoQ marked 10 inline comments as done.

Fxd.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
496 ↗(On Diff #195458)

For now it probably should, but i think eventually it will be useful on its own, independently.

On the other hand, it might make sense to have MoveChecker depend on this checker, because this checker eliminates false positives in MoveChecker, so we shouldn't allow enabling MoveChecker without this checker. But for now it would mean depending on an alpha checker.

WDYT?

clang/lib/StaticAnalyzer/Checkers/Move.h
30 ↗(On Diff #195458)

Whoops, and it shouldn't say anything about TAINT, right?

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
236 ↗(On Diff #195458)

Unless you plan to add more values to RegionState::Kind, I think it would be fine.

I suspect that we may have to add an "Escaped" state eventually. I.e.,

T a;
escape(&a);
T b = std::move(a);
reset_escaped(); // resets 'a'
a.foo(); // no-warning

For now i don't know how to easily address this problem fully without storing every single escaping object in the program state (regardless of whether it is going to ever be moved), and this problem is pretty rare: i've only seen one move-checker false positive of this kind, it was on LLVM, and it was fixed on LLVM side independently in D57618. In this code the error object was escaping into an iterator and getting reset on every loop iteration, but now they made the idiom more sane.

Also if we ever end up tracking escaped objects, we might as well track *all* escaped objects for *all* checkers in *one* set-trait. Simply because escaped locals behave more like arbitrary heap objects than like locals. But for now i suspect that this is an overkill.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
53 ↗(On Diff #195458)

Oh well. I tried so hard to learn how to use std::bind but nobody likes it :)

59 ↗(On Diff #195458)

Yuck. Indeed, i'm forming a null reference in the middle while returning from operator*(). At least, i would be forming a null reference if we ever invoke evalCall() on a call that makes getCall() fail, 'cause for now we seem to never do that. Also we should simply pass a CallEvent to evalCall, as we trivially have it available on ExprEngine side :/

Szelethus added inline comments.Apr 17 2019, 12:29 PM
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
496 ↗(On Diff #195458)

Ugh, yea, good points. I think we should express, even if temporarily, that this checker depends on MoveChecker. Just leave a couple TODO's around that this has to revised!

On the long term, it makes sense for MoveChecker to depend on this one, agreed.

clang/lib/StaticAnalyzer/Checkers/Move.h
30 ↗(On Diff #195458)

Riiight. :)

NoQ marked an inline comment as done.Apr 17 2019, 4:27 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
59 ↗(On Diff #195458)

Whoops, i actually forgot to fix this one. On the other hand, i found the intended way of writing this code, and it's neither &* nor .get(). Behold!

NoQ updated this revision to Diff 195651.Apr 17 2019, 4:28 PM

The actual "Behold!".

NoQ updated this revision to Diff 195853.Apr 18 2019, 6:25 PM

On second thought, maybe let's restrict the scope to just bool conversions (don't model .get() and .release() yet) but enable the check by default immediately?

It's still bad that we don't explain why if (P) takes a false branch for a moved-from pointer P, but it's not that bad, because it's "just" a control-flow dependency of the bug; there won't be any actual bug reports about some bool being false (hopefully nobody would ever try to divide by such bool), but there may be actual bug reports about a pointer being null (eg., when it's dereferenced).

This way we will address the false positive quickly and hopefully don't cause too much trouble.

Szelethus accepted this revision.Apr 19 2019, 2:49 PM
In D60796#1472587, @NoQ wrote:

On second thought, maybe let's restrict the scope to just bool conversions (don't model .get() and .release() yet) but enable the check by default immediately?

It's still bad that we don't explain why if (P) takes a false branch for a moved-from pointer P, but it's not that bad, because it's "just" a control-flow dependency of the bug; there won't be any actual bug reports about some bool being false (hopefully nobody would ever try to divide by such bool), but there may be actual bug reports about a pointer being null (eg., when it's dereferenced).

This way we will address the false positive quickly and hopefully don't cause too much trouble.

Yup, let's just roll with it. LGTM!

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
23 ↗(On Diff #195853)

Do you actually need this include?

61 ↗(On Diff #195853)

How about now, after D58367 landed? :)

This revision is now accepted and ready to land.Apr 19 2019, 2:49 PM
NoQ updated this revision to Diff 195947.Apr 19 2019, 5:28 PM
NoQ marked an inline comment as done.

Fix unused include.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
61 ↗(On Diff #195853)

That's still not immediately possible because i believe that this note should be prunable.

This might be an ideal candidate for being hidden by default though (D60925).

NoQ added a comment.Apr 22 2019, 4:10 PM

This might be an ideal candidate for being hidden by default though (D60925).

Hmm, should i rename it into some sort of "SmartPtrModel" and later make a separate checker to emit actual warnings?

Yea, something along the lines of SmartPtrModeling would be neat -- logically, if you plan to emit errors down the line, subcheckers should be used for that, in order to enable checkers to depend purely on the modeling part.

NoQ marked an inline comment as done.Apr 22 2019, 4:23 PM

Yea, something along the lines of SmartPtrModeling would be neat -- logically, if you plan to emit errors down the line, subcheckers should be used for that, in order to enable checkers to depend purely on the modeling part.

Will do!

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
1 ↗(On Diff #195947)

Sigh.

NoQ updated this revision to Diff 196158.Apr 22 2019, 4:29 PM
NoQ retitled this revision from [analyzer] PR41269: SmartPtrChecker. to [analyzer] PR41269: SmartPtrModeling..

Rename the checker.

Szelethus accepted this revision.Apr 22 2019, 4:35 PM

I think you can land this one if you're comfortable with it! +1 on adding core in the test RUN: lines.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2019, 7:44 PM