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:
> > > > 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.
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
