vrnithinkumar added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:35
bool isNullAfterMoveMethod(const CallEvent &Call) const;
+ BugType NullDereferenceBugType{this, "Null-smartPtr-deref",
+ "C++ smart pointer"};
----------------
xazax.hun wrote:
> Nit: We do not use hypens/dashes in diagnostic names.
Fixed the format
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:53
+ // Set of STL smart pointer class which we are trying to model.
+ const llvm::StringSet<> StdSmartPtrs = {
+ "shared_ptr",
----------------
xazax.hun wrote:
> It might be just a personal preference but I tend to put these at the top of
> the file for easier discoverability. Feel free to ignore this comment unless
> other reviewers have the same preference as me.
Thanks!
Moved to top.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:81
CheckerContext &C) const {
- if (!isNullAfterMoveMethod(Call))
+ if (isNullAfterMoveMethod(Call)) {
+ ProgramStateRef State = C.getState();
----------------
xazax.hun wrote:
> Don't we want this to be also protected by the `isStdSmartPointerClass` check?
added `isStdSmartPointerClass` check in the beginning.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:115
+ const auto *MethodDecl = dyn_cast<CXXMethodDecl>(Call.getDecl());
+ if (!MethodDecl || !isStdSmartPointerClass(MethodDecl->getParent()))
+ return;
----------------
xazax.hun wrote:
> I wonder if making `isStdSmartPointerClass` operate on `CallEvent` would
> simllify the call sites of this function.
Yeah. Thanks!
That makes it better.
Made changes to pass `CallEvent` to `isStdSmartPointerClass`
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:124
+ return;
+ updateTrackedRegion(Call, C, ThisValRegion);
+}
----------------
NoQ wrote:
> NoQ wrote:
> > Not all constructors behave this way. In particular, if it's a copy/move
> > constructor this function would track the value of the original smart
> > pointer to this smart pointer, but what we need is to track the value of
> > the raw pointer instead.
> Actually, let's add an assertion into `updateTrackedRegion` that the value
> that's put into the map is of pointer type. This would give us an automatic
> notification when we make such mistakes.
- Added check to filter out copy/move constructor
- Added assert to check the value is of type pointer.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:137
+ if (IsRegDead) {
+ State = State->remove<TrackedRegionMap>(Region);
+ }
----------------
NoQ wrote:
> xazax.hun wrote:
> > In LLVM we often omit braces for small single statement branches. Also,
> > Artem mentioned that we could interact with the malloc checker. Maybe it is
> > worth considering to notify the malloc checker to mark the appropriate
> > region as deallocated? This would help find double release errors, avoid
> > spurious leak errors and so on.
> > Maybe it is worth considering to notify the malloc checker to mark the
> > appropriate region as deallocated?
>
> This happens in destructor.
removed the braces
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:155
+
+ if (MethodDecl && MethodDecl->isOverloadedOperator()) {
+ OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
----------------
xazax.hun wrote:
> Early returns can decrease the indentation.
Updated with early return.
Thanks!
================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:175
+ return;
+ updateTrackedRegion(Call, C, ThisValRegion);
+}
----------------
NoQ wrote:
> When you're doing `evalCall`, you're responsible for all aspects of the call
> - not just your private GDM map but also the Store and the Environment.
>
> For `.reset()` the other important thing to do would be to invalidate the
> region in the Store so that the Store did not think that it contains the old
> value. We can't set the new value correctly but invalidating it would still
> be better than doing nothing - simply because "not being sure" is not as bad
> as "being confidently incorrect". That said, once you model *all* methods of
> the smart pointers, you would probably no longer need to invalidate the
> Store, because it will never be actually accessed during analysis. So my
> conclusion here is:
> - For this first patch invalidating the Store is probably not worth it but
> let's add a FIXME.
> - We should make sure that we eventually add the invalidation if we don't
> have time to model all methods.
>
> Now, you're calling this same function for `.release()` as well. And for
> `.release()` there is another important thing that we're responsible for:
> //model the return value//. The `.release()` method returns the raw pointer -
> which is something this checker is accidentally an expert on! - so let's
> `BindExpr()` the value of the call-expression to the value of the raw
> pointer. If you want to delay this exercise until later patches, i recommend
> to temporarily modeling the return value as a conjured symbol instead, but we
> cannot completely drop this part.
>
> Another useful thing that we should do with `.release()` in the future is to
> tell `MallocChecker` to start tracking the raw pointer. This will allow us to
> emit memory leak warnings against it. Let's add a TODO for that as well.
- Added TODO for `reset()`
- Added `BindExpr()` for the value of the call-expression to the value of the
raw pointer.
- Added TODO for `release()` to track raw pointer in future.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81315/new/
https://reviews.llvm.org/D81315
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
