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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun 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) {
----------------
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?


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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:110
+  SmartPtrChecker *Checker = Mgr.registerChecker<SmartPtrChecker>();
+  NullDereferenceBugTypePtr = &Checker->NullDereferenceBugType;
 }
----------------
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.


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


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:199
+    } else {
+      const auto *TrackingExpr = getFirstArgExpr(Call);
+      C.addTransition(State, C.getNoteTag([ThisRegion, TrackingExpr](
----------------
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.


================
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 {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600