[PATCH] D75430: [analyzer][NFC] Introduce CXXDeallocatorCall, deploy it in MallocChecker

[Kristóf Umann via Phabricator via cfe-commits]

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