steakhal marked an inline comment as done.
steakhal added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:325-326
+ if (ReferrerMemSpace && ReferredMemSpace) {
+ if (ReferredFrame == PoppedFrame &&
+ ReferrerFrame->isParentOf(PoppedFrame)) {
+ V.emplace_back(Referrer, Referred);
----------------
NoQ wrote:
> You probably meant `||`?
No, I think `&&` is justified here. We have to make sure that the popped frame
is the one that was referred to by some other frame, below that frame.
{F18569514}
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:397-398
+ Ctx.getLocationContext());
+ Report->addNote("The temporary object gets destroyed at the end of the "
+ "full expression",
+ L);
----------------
NoQ wrote:
> steakhal wrote:
> > NoQ wrote:
> > > This is part of the path, right? We're reporting these bugs at
> > > `checkEndFunction` rather than at the store site so the destruction has
> > > already happened by the time we report it. In this case it should be a
> > > path note, not an extra blue-bubble note.
> > >
> > > As for wording, I suggest `full-expression` with a dash (that's what
> > > other clang diagnostics use) and probably drop the initial "The"(?)
> > > This is part of the path, right? We're reporting these bugs at
> > > `checkEndFunction` rather than at the store site so the destruction has
> > > already happened by the time we report it. In this case it should be a
> > > path note, not an extra blue-bubble note.
> >
> > I'm not exactly sure how to address this. AFAIK there is no way currently
> > detecting the completion of a full-expression.
> > I was thinking of `Check::RegionChanges`, `Check::PostStmt<Expr>`,
> > `Check::Live`, but none of these fits my needs. Do you have anything
> > specific in mind that could help me?
> >
> > Aside from that, the report looks reasonable even with a 'blue' bubble:
> > (scan-build): {F18363520}
> > (CodeChecker): {F18364795}
> >
> > ---
> >
> > > As for wording, I suggest full-expression with a dash (that's what other
> > > clang diagnostics use) and probably drop the initial "The"(?)
> > I see, thanks.
> Wait, no, in your example the end of the full-expression is not part of the
> path; it happens after the warning is emitted.
>
> If it was, the right thing to do would have been to catch the destruction of
> the object of interest rather than the end of the full-expression as such.
>
> But in this case the note seems to be redundant. Lifetime of the temporary
> object is fairly irrelevant to the report. It only matters that such lifetime
> is longer than that of the stack address. But the note doesn't tell the user
> how //long// that lifetime is; it only says how //short// it is. So i think
> this note is redundant.
Okay, I won't emit the extra note.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107078/new/
https://reviews.llvm.org/D107078
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
