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

Sun Aug 2 20:56:43 PDT 2020

NoQ added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:408-412
+        SmallString<128> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+        TagDetails.trackValidExpr(BR);
+        TagDetails.explainSmartPtrAction(Out);
+        return std::string(Out.str());
----------------
NoQ wrote:
> vrnithinkumar wrote:
> > NoQ wrote:
> > > NoQ wrote:
> > > > vrnithinkumar wrote:
> > > > > NoQ wrote:
> > > > > > NoQ wrote:
> > > > > > > Ok, note that note tags are attached to nodes independently of bug reports; when the report is thrown, only then we know what are the smart pointers that should be explained.
> > > > > > > 
> > > > > > > So there are two checks that you should do here:
> > > > > > > 
> > > > > > > 1. Check that the bug report is emitted by your checker (eg., by comparing bug types). If not, don't add notes.
> > > > > > > 
> > > > > > > 2. Check that the region about which the note speaks is related to your report (i.e., it's not a completely unrelated smart pointer). You can do that by marking the smart pointer as "interesting" (i.e., `PathSensitiveBugReport::markIntersting()`) when you emit the report, and then in the lambda you check whether the smart pointer is interesting before you emit a note. Additionally, you can carry over interestingness when smart pointers are copied.
> > > > > > > 
> > > > > > > This is what i was trying to accomplish with this code snippet that i included in the examples in the other comment:
> > > > > > > ```lang=c++
> > > > > > >   if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting())
> > > > > > >     return "";
> > > > > > > ```
> > > > > > (i strongly recommend having test cases for both of these issues)
> > > > > I was stuck on how to check the 2 cases from `SmartPtrModeling`.
> > > > > 
> > > > > 1. I was not able to figure out how to access `NullDereferenceBugType` defined in the `SmartPtrChecker` in `SmartPtrModeling` to check `&BR.getBugType() != &NullDereferenceBugType`. Since `NullDereferenceBugType` is part of the `SmartPtrChecker` I could not access it from `PathSensitiveBugReport`.  One way I figured out is to use `getCheckerName()` on BugType and compare the string. I feel this one as little hacky.
> > > > > 
> > > > > 
> > > > > 2. I got stuck on how will we implement the `!R->isInteresting()` in case of the bug report is added by some other checker on some other region. For below test case, bug report is added on a raw pointer by `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy and we will not be adding note tags where `unique_ptr` is released. I tried getting the LHS region for `A *AP = P.release();` assignment operation and check if the region is interesting but not sure whether its gonna work for some complex assignment cases.
> > > > > 
> > > > > ```
> > > > > void derefOnReleasedNullRawPtr() {
> > > > >   std::unique_ptr<A> P;
> > > > >   A *AP = P.release(); // expected-note {{'AP' initialized to a null pointer value}}
> > > > >   // expected-note at -1 {{Smart pointer 'P' is released and set to null}}
> > > > >   AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
> > > > >   // expected-note at -1{{Called C++ object pointer is null}}
> > > > > }
> > > > > ```
> > > > > Since `NullDereferenceBugType` is part of the `SmartPtrChecker` I could not access it from `PathSensitiveBugReport`.
> > > > 
> > > > You shouldn't be accessing it from the bug report, you should be accessing it from the lambda. See the example code snippets in D84600#inline-779418
> > > > 
> > > > > For below test case, bug report is added on a raw pointer by `CallAndMessageChecker` and the `!R->isInteresting()` will not satisfy and we will not be adding note tags where `unique_ptr` is released.
> > > > 
> > > > That's an interesting question (no pun intended). The way i imagine this working is: the note tag for `.release()` should try to figure out whether the raw pointer is tracked and mark the smart pointer as interesting based on that. If the raw pointer was a symbol that would have been easy (either null dereference checker or `trackExpressionValue()` could mark it as interesting). But for concrete null pointer this won't work.
> > > > 
> > > > Maybe we should consider introducing interesting expressions. I.e., when `trackExpressionValue()` reaches the call-expression `P.release()`, it has to stop there. But if it also marked the call-expression as interesting, the note tag provided by the checker could read that interestingness information and act upon it by marking the smart pointer region as interesting.
> > > >  That's an interesting question
> > > 
> > > I'd rather make a separate commit for this endeavor because it sounds pretty nasty.
> > > You shouldn't be accessing it from the bug report, you should be accessing it from the lambda. See the example code snippets in D84600#inline-779418
> > Sorry, I am still confused how will the lambda defined in the `SmartPtrModeling` can capture the `NullDereferenceBugType` from `SmartPtrChecker`?
> Lambda can capture anything that's available in its lexical context. For capturing a field of the surrounding class, i believe you should capture `this`. [[ https://github.com/llvm/llvm-project/blob/llvmorg-11-init/clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp#L214 | See how other checkers do that. ]]
Wait, i see what you mean. We're in the wrong checker!

Let's turn the null dereference bug type into an inter-checker API then? Make a header, like `Move.h` but say `NullDereference.h`, and introduce a `const BugType *getNullDereferenceBugType()` that we could invoke in the lambda.

Because the bug type is created with checker registration, it might make sense to maintain a static global pointer to the bug type that'll initially be null but initialized during registration.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84600