Szelethus added a comment.
In D75430#2028456 <https://reviews.llvm.org/D75430#2028456>, @NoQ wrote:
> Sorry i'm not very responsive these days >.<
No worries, thanks! ^-^
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1052
+/// This is a call to "operator delete".
+// FIXME: CXXDeleteExpr isn't present for custom delete operators, or even for
+// some those that are in the standard library, like the no-throw or align_val
----------------
Szelethus wrote:
> xazax.hun wrote:
> > How important and hard it is to fix this? If you did not find the reason
> > why those CXXDeleteExprs are missing you can do two things:
> > 1. Look at how such AST is consumed by CodeGen
> > 2. Ask around on the mailing list with a minimal example.
> Fair point, but it surely is a thing to keep in mind because it could, or
> more probably //will// cause surprises. A `TODO` or a `NOTE` would be more
> appropriate. In any case, I'd prefer to address these in a followup patch.
Actually, see my other inline. I do believe that this should be the one class
to solve all deletes, or at the very least, standard-specified deletes.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp:95
ExplodedNode *N = C.generateNonFatalErrorNode();
+ if (!N)
+ return;
----------------
NoQ wrote:
> xazax.hun wrote:
> > I think this change might be a better fit for a separate commit. I think
> > you don't even need to have such a small change reviewed. You could just
> > commit it as is. Just do not forget to describe that these cases are really
> > hard to have a test for but this is the idiomatic way of doing this in
> > checkers.
> I'm also curious why did this suddenly become necessary. This is obviously
> the right thing to do but why the change? It might indicate that we
> accidentally started incorrectly agglutinating nodes that were previously
> considered different. Like, we might have messed up our program points
> somewhere in this patch.
The creduced program that causes this checker to crash (without the early
return):
```lang=cpp
struct a {};
struct b : a {};
void c() {
a *d = new b;
delete d;
}
```
Turns out, as I said in an another inline, I accidentally run `PreStmt` on
delete expressions twice. In any case, I decided to add this early return and a
test case in this patch, because I think strongly related, and as seen here,
tests the added functionality.
================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:1652
+ ExplodedNodeSet PostVisit;
+ getCheckerManager().runCheckersForPreStmt(PostVisit, PreVisit, S, *this);
----------------
Oh wow. I accidentally ran checkers on `PreStmt` here as well. Thats why I
needed the new early return to not crash.
================
Comment at: clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp:31-32
- // FIXME: All calls to operator new should be CXXAllocatorCall.
- // FIXME: PostStmt<CXXDeleteExpr> should be present.
+ // FIXME: All calls to operator new should be CXXAllocatorCall, and calls to
+ // operator delete should be CXXDeallocatorCall.
{
----------------
NoQ wrote:
> That's fairly unobvious to me. Isn't the whole point of
> `CXXAllocatorCall`/`CXXDeallocatorCall` to give access to specific aspects of
> new/delete-expressions that aren't available in manual invocations with
> function syntax? On the other hand, i agree that it's nice to be able to cast
> the call event to `CXXAllocatorCall`/`CXXDeallocatorCall` and be done with
> allocators/deallocators.
Yup, in my mind, this isn't `CXXNewCall`, but rather `CXXAllocatorCall` (same
for delete), I would expect this to handle it all (maybe change the interfaces
so that `getOriginExpr` returns with `Expr`, and add a `getOriginAsCXXNewExpr`
method).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75430/new/
https://reviews.llvm.org/D75430
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
