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

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

Szelethus requested review of this revision.
Szelethus added a comment.

Since this change had a few blocking issues, I'm placing it back to review for greater visibility.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:67
   CE_CXXAllocator,
+  CE_CXXDeallocator,
   CE_BEG_FUNCTION_CALLS = CE_Function,
----------------
NoQ wrote:
> After this you probably received compiler warnings saying "case isn't covered in switch". You'll need to clean them up.
> 
> Another thing to do would be to update `CallEventManager`'s `getCall()` and `getCaller()` methods that'd allow the users to construct the new `CallEvent` easily without thinking about what specific kind of call event it is.
No, I did not, infuriatingly. I did however get //errors// after trying to make a `toString` function for `CallEventKind`, apparently both `CE_CXXDeallocator` and `CE_Block` has the value of 7. When I moved the enum, everything was fine, and I did get the warnings you mentioned.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:958
+
+  unsigned getNumArgs() const override { return getDecl()->getNumParams(); }
+
----------------
steakhal wrote:
> NoQ wrote:
> > Charusso wrote:
> > > `return getOriginExpr()->getNumArgs()`
> > This wouldn't compile. `CXXDeleteExpr` is not a `CallExpr`.
> > 
> > It sounds like there are currently [[ http://www.cplusplus.com/reference/new/operator%20delete/ | five different `operator delete`s ]]:
> > {F11474783}
> > 
> > And, unlike `operator new`, there's no option to provide custom "placement" arguments.
> > 
> > So i think the logic in this patch is correct but we should do some custom logic for all 5 cases in the `getArgExpr` method, where argument expressions for the extra arguments will have to be conjured out of thin air (or we might as well return null).
> > It sounds like there are currently five different `operator delete`s:
> I think it is even worse since C++17 and C++20 introduced a couple of others like:
>  - overloads with `std::align_val_t` parameter (C++17)
>  - overloads with `std::destroying_delete_t` parameter (C++20) which I haven't heard of yet :D
> 
> You can check it in the draft: http://eel.is/c++draft/new.delete
> And of course at cppreference as well: https://en.cppreference.com/w/cpp/memory/new/operator_delete
Okay so here is the biggest issue: non-simple `delete`s don't appear in the AST as `CXXDeleteExpr`, but rather as a `CXXOperatorCall`. This is a big problem, what could be the reason for it?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75430/new/

https://reviews.llvm.org/D75430