vrnithinkumar added inline comments.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:86 + if (const auto *DR = dyn_cast<DeclRegion>(DerefRegion)) { + auto SmartPtrName = DR->getDecl()->getName(); + OS << " '" << SmartPtrName << "'"; ---------------- NoQ wrote: > Please `getNameAsString()` because `getName()` crashes on anonymous > declarations (eg., lambda captures, anonymous nested structs/unions, etc.). Changed to use `getNameAsString()` ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp:95-96 + + const auto *RecordDecl = MethodDecl->getParent(); + if (RecordDecl && RecordDecl->getDeclName().isIdentifier()) { + OS << " of type '" << RecordDecl->getQualifiedNameAsString() << "'"; ---------------- NoQ wrote: > Aha, this time you checked for anonymous declarations! Good. > > That said, i'm not sure type is actually useful for this warning, because > they're roughly all the same. Okay. I was trying to be consistent with `MoveChecker`. Just removed smart pointer type. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:179 + + switch (SmartPtrMethod) { + case Constructor: { ---------------- NoQ wrote: > NoQ wrote: > > I feel this is a bit over-engineered. There's no need to create an enum and > > later convert it into a string when you can capture a string directly. > > Like, this entire "details" structure of your, it should be just captures > > instead, and your strings would make it immediately obvious what kind of > > note is emitted: > > ```lang=c++ > > C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) { > > if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting()) > > return ""; > > > > return Twine("Default constructed smart pointer '") + getSmartPtrName(R) > > + "' is null"; > > })); > > ``` > > > > Hmm, maybe we should also provide an overload with lambdas of > > signature`void(PathSensitiveBugReport &BR, llvm::raw_ostream OS)` so that > > to make the same one-liners possible with streams? Something like this: > > > > ```lang=c++ > > class CheckerContext { > > // ... > > NoteTag *getNoteTag(std::function<void(PathSensitiveBugReport &BR, > > llvm::raw_ostream OS)> f) { > > return getNoteTag([](PathSensitiveBugReport &BR) -> std::string { > > llvm::SmallString<128> Str; > > llvm::raw_svector_ostream OS(Str); > > f(BR, OS); > > return OS.str(); > > }); > > } > > // ... > > }; > > ``` > > > > Then you could do: > > ```lang=c++ > > C.addTransition(State, getNoteTag([R](PathSensitiveBugReport &BR) { > > if (&BR.getBugType() != &NullDereferenceBugType || !R->isInteresting()) > > return; > > > > OS << "Default constructed smart pointer '" << getSmartPtrName(R) << "' > > is null"; > > })); > > ``` > (forgot `, llvm::raw_ostream OS` in the last snippet; probably forgot a bunch > of other stuff because i didn't try to actually compile these snippets) - Added overload for `getNoteTag` with lambda accepting a `llvm::raw_ostream` - Used the overload instead of `SmartPtrTagDetails` structure to add tags. - Removed the `SmartPtrTagDetails` and changed related code. ================ 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: > 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. > (i strongly recommend having test cases for both of these issues) Added the tests ================ 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()); ---------------- vrnithinkumar wrote: > NoQ wrote: > > 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. > > (i strongly recommend having test cases for both of these issues) > > Added the tests > > That's an interesting question > > I'd rather make a separate commit for this endeavor because it sounds pretty > nasty. Will make a separate commit ================ 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()); ---------------- vrnithinkumar wrote: > vrnithinkumar wrote: > > NoQ wrote: > > > 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. > > > (i strongly recommend having test cases for both of these issues) > > > > Added the tests > > > That's an interesting question > > > > I'd rather make a separate commit for this endeavor because it sounds > > pretty nasty. > > Will make a separate commit - Created `NullDereference.h` and added `const BugType *getNullDereferenceBugType()` - Added a static global pointer to the bug type. - Added the check if the region is interesting and bug type is matching. 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