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@-1 {{Smart pointer 'P' is released and set to > > > > > null}} > > > > > AP->foo(); // expected-warning {{Called C++ object pointer is null > > > > > [core.CallAndMessage]}} > > > > > // expected-note@-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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits