[PATCH] D85796: [Analysis] Bug fix for exploded graph branching in evalCall for constructor

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar marked 5 inline comments as done.
vrnithinkumar added a comment.





================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:619
       getCheckerManager().runCheckersForEvalCall(DstEvaluated, *I, *Call, *this,
-                                                 CallOpts);
+                                                 CallOpts, Bldr);
   }
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > We should probably delete the copy-constructor for node builders. I've no idea what it's supposed to do anyway and the whole problem that we're having here is due to there being //too many// of them already.
> > So we should disable the copying of `NodeBuilder` and create a heap allocated `NodeBuilder` and use pointer to pass around functions?
> No-no, keep it on the stack and don't pass it around *at all*.
Sorry, I am still little confused. 

Do we have to make the fix without passing around the `NodeBuilder`?
And delete copy-constructor for `NodeBuilder` in this patch? 


================
Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:39
 void derefAfterRelease() {
-  std::unique_ptr<A> P(new A());
+  std::unique_ptr<A> P(new A()); // expected-note {{Smart pointer 'P' is constructed}}
   P.release(); // expected-note {{Smart pointer 'P' is released and set to null}}
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > Ok, these notes shouldn't be there; a note on `.release()` is sufficient to understand the warning and it looks like that's one more place where we should mark the region as uninteresting.
> > > 
> > > Can you try to debug why did they suddenly show up?
> > I checked the exploded graph for this test case. 
> > Before the bug fix, there exists a path where the no Note Tag is added to the corresponding `CXXConstructExpr`. After the fix removed this branching theres always a Note Tag on Ctr. {F12591752}
> > 
> > Since the note on .release() is sufficient to understand the warning and I agree we should mark this region as uninteresting.
> Ok, fair enough! Let's add a FIXME.
Added the FIXME


================
Comment at: clang/test/Analysis/smart-ptr.cpp:143
     pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-    P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+    P->foo(); // no-warning
   }
----------------
NoQ wrote:
> That's indeed the intended consequence of your patch: we're no longer exploring a bogus execution path that's parallel to the previous null dereference on line 133, therefore we're no longer emitting this warning but instead correctly interrupting the entire analysis after we've found that other null dereference.
> 
> That said, we should preserve the test so that it was still testing whatever it was testing previously. Can you split up this function into smaller functions so that each test was running independently?
Separated the function into smaller functions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85796/new/

https://reviews.llvm.org/D85796