[PATCH] D84600: [Analyzer] Support note tags for smart ptr checker

[Nithin VR via Phabricator via cfe-commits]

vrnithinkumar added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:312
+  const NoteTag *getNoteTag(
+      std::function<void(PathSensitiveBugReport &BR, llvm::raw_ostream &OS)> Cb,
+      bool IsPrunable = false) {
----------------
xazax.hun wrote:
> The callback is taken is an rvalue reference in other `getNoteTag` APIs. I think these overloads should be consistent.
> Also, I wonder if the caller should be able to manipulate the buffer size of the small string (as a template parameter), but I do not have strong feelings about this. 
> 
> As a side note, since Clang is using C++14, maybe the lambda captures in the `getNoteTag` overloads above should utilize it and capture the callback by move. This is more of a note to ourselves independent of this patch. 
> 
> Side note 2: maybe a modernize tidy check would be useful to discover where rvalue references are captured by value in lambdas?
Changed to rvalue reference.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:21
+namespace ento {
+namespace nullDereference {
+
----------------
NoQ wrote:
> Namespaces are traditionally snake_case rather than camelCase.
removed the new namespace


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NullDereference.h:23
+
+/// Returns NullDereferenceBugType.
+const BugType *getNullDereferenceBugType();
----------------
xazax.hun wrote:
> I think this comment does not really add much value.
Yeah.
removed it.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker<SmartPtrChecker>();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > xazax.hun wrote:
> > > NoQ wrote:
> > > > vrnithinkumar wrote:
> > > > > NoQ wrote:
> > > > > > Wait, i don't understand again. You're taking the bug type from that checker and using it in that checker. Why did you need to make it global? I thought your problem was about capturing the bug type from the //raw// null dereference checker?
> > > > > > Wait, i don't understand again. You're taking the bug type from that checker and using it in that checker. Why did you need to make it global? I thought your problem was about capturing the bug type from the //raw// null dereference checker?
> > > > > 
> > > > > The check for bug type is in `SmartPtrModeling` but the bug type is defined in `SmartPtrChecker`
> > > > Aha, ok, i misunderstood again. So i guess we didn't need a new header then. That said, it's an interesting layering violation that we encounter in this design: it used to be up to the checker to attach a visitor and so should be activating a note tag, but note tags are part of modeling, not checking.
> > > > 
> > > > I guess that's just how it is and it's the responsibility of the checkers to inform the modeling about bug types. I guess the ultimate API could look like `BugReport->activateNoteTag(NoteTagTag TT)` (where `TT` is a crying smiley that cries about "tag tags" T_T). But that's a story for another time.
> > > I hope we will be able to figure out a nicer solution at some point. But I do not have a better alternative now, so I am ok with committing as is.
> > > 
> > > The API Artem is suggesting looks good to me (although it is out of scope for the GSoC). But I think that might not solve the problem of how multiple checkers should share the information about those tags. (Or at least I do not see how.)
> > > 
> > > Note that if we had both checks in the same file we might be able to work this problem around using `CheckerManager::getChecker`. I am not suggesting merging them, only wanted to point out.
> > >  So i guess we didn't need a new header then.
> > So we should remove the `NullDereference.h` and add the inter checker api to get the BugType to `SmartPtr.h`?
> > 
> > >  I guess the ultimate API could look like `BugReport->activateNoteTag(NoteTagTag TT)`
> > Do we have to make these api changes on this review? Or some follow up changes?
> > So we should remove the `NullDereference.h` and add the inter checker api to get the BugType to `SmartPtr.h`?
> Yes.
> 
> > Do we have to make these api changes on this review? Or some follow up changes?
> You don't have to do this at all; i think you have enough stuff on your plate. But if you like you can put up another patch later.
Removed the new header file and added to `SmartPtr.h`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:131
+  }
+  return nullptr;
+}
----------------
NoQ wrote:
> You never ever check for this case. Therefore this function entirely boils down to `Call.getArgExpr(0)` which is shorter than `getFirstArgExpr(Call)` anyway.
removed it and inlined.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:134
+
+static std::string getSmartPtrName(const MemRegion *Region) {
+  std::string SmartPtrName = "";
----------------
xazax.hun wrote:
> I wonder whether `MemRegion::printPretty` is what we want here.
Yeah Thanks.
This is what we exactly needed. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:199
+    } else {
+      const auto *TrackingExpr = getFirstArgExpr(Call);
+      C.addTransition(State, C.getNoteTag([ThisRegion, TrackingExpr](
----------------
NoQ wrote:
> xazax.hun wrote:
> > I am a bit confused. As far as I understand we ALWAYS add the note tag below, regardless of the first argument being null.
> > I think I do understand that we only trigger this note tag when there is a null dereference but consider the code below:
> > 
> > ```
> > void f(int *p) {
> >   std::unique_ptr<int> u(p); // p is not always null here.
> >   if (!p) {
> >     *u; // p is always null here
> >   }
> > }
> > ```
> > 
> > Do we ant to output the message that the smart pointer is constructed using a null value? While this is technically true, there is a later event in the path that uncovers the fact that `p` is null.
> > 
> > @NoQ what do you think? I do not have any strong feelings about this, I only wonder how users would react to a bug report like that.
> Indeed, nice!! We can only proclaim the pointer to be null if it's known to be null in the current state. I.e., if previous steps of the report indicated that it's null. If it's discovered to be null later, we cannot call it null yet. I think we should check the current state and say "is constructed using a null value" if it's already null or simply "is constructed" if it's not yet known to be null.
Added a check if the value is null or not and add message based on that.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:352
+// Gets the SVal to track for a smart pointer memory region
+SVal SmartPtrModeling::getSValForRegion(const CallEvent &Call,
+                                        CheckerContext &C) const {
----------------
vrnithinkumar wrote:
> xazax.hun wrote:
> > I am a bit unhappy with this function.
> > I feel like it does not really have a purpose. It returns the null constant for default constructor calls, for every other case it just returns the first argument. The branch we have in this function would completely go away if we would inline this to all the call sites and the result would be a single line each place. I'd rather just remove this function.
> Also we have to assert that we are adding a pointer value to TrackedRegionMap.
Removed the function and inlined it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600