[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 , @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 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

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

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

This revision was automatically updated to reflect the committed changes.
Szelethus marked 14 inline comments as done.
Closed by commit rG9d69072fb807: [analyzer][NFC] Introduce CXXDeallocatorCall, 
deploy it in MallocChecker (authored by Szelethus).

Changed prior to commit:
  https://reviews.llvm.org/D75430?vs=254780=264737#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75430

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/CallEventTest.cpp

Index: clang/unittests/StaticAnalyzer/CallEventTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/CallEventTest.cpp
@@ -0,0 +1,89 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "CheckerRegistration.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+void reportBug(const CheckerBase *Checker, const CallEvent ,
+   CheckerContext , StringRef WarningMsg) {
+  C.getBugReporter().EmitBasicReport(
+  nullptr, Checker, "", categories::LogicError, WarningMsg,
+  PathDiagnosticLocation(Call.getOriginExpr(), C.getSourceManager(),
+ C.getLocationContext()),
+  {});
+}
+
+class CXXDeallocatorChecker : public Checker {
+  std::unique_ptr BT_uninitField;
+
+public:
+  CXXDeallocatorChecker()
+  : BT_uninitField(new BuiltinBug(this, "CXXDeallocator")) {}
+
+  void checkPreCall(const CallEvent , CheckerContext ) const {
+const auto *DC = dyn_cast();
+if (!DC) {
+  return;
+}
+
+SmallString<100> WarningBuf;
+llvm::raw_svector_ostream WarningOS(WarningBuf);
+WarningOS << "NumArgs: " << DC->getNumArgs();
+
+reportBug(this, *DC, C, WarningBuf);
+  }
+};
+
+void addCXXDeallocatorChecker(AnalysisASTConsumer ,
+  AnalyzerOptions ) {
+  AnOpts.CheckersAndPackages = {{"test.CXXDeallocator", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry ) {
+Registry.addChecker("test.CXXDeallocator",
+   "Description", "");
+  });
+}
+
+// TODO: What we should really be testing here is all the different varieties
+// of delete operators, and wether the retrieval of their arguments works as
+// intended. At the time of writing this file, CXXDeallocatorCall doesn't pick
+// up on much of those due to the AST not containing CXXDeleteExpr for most of
+// the standard/custom deletes.
+TEST(CXXDeallocatorCall, SimpleDestructor) {
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode(R"(
+struct A {};
+
+void f() {
+  A *a = new A;
+  delete a;
+}
+  )",
+ Diags));
+  EXPECT_EQ(Diags, "test.CXXDeallocator:NumArgs: 1\n");
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -6,6 +6,7 @@
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
   CallDescriptionTest.cpp
+  CallEventTest.cpp
   StoreTest.cpp
   RegisterCustomCheckersTest.cpp
   SymbolReaperTest.cpp
Index: clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
===
--- 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Sorry i'm not very responsive these days >.<




Comment at: 
clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp:95
   ExplodedNode *N = C.generateNonFatalErrorNode();
+  if (!N)
+return;

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.



Comment at: clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp:31-32
 
-  // FIXME: All calls to operator new should be CXXAllocatorCall.
-  // FIXME: PostStmt should be present.
+  // FIXME: All calls to operator new should be CXXAllocatorCall, and calls to
+  // operator delete should be CXXDeallocatorCall.
   {

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.


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

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

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

Szelethus marked 5 inline comments as done.
Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:59
 enum CallEventKind {
+  CE_CXXDeallocator,
   CE_Function,

xazax.hun wrote:
> Szelethus wrote:
> > I need to move this below `CE_Function`, as its not in the range of 
> > `CE_BEG_FUNCTION_CALLS` and `CE_END_FUNCTION_CALLS`.
> This is marked as done. If the conclusion is that you do not need to move it 
> I think it would be worth to add a comment why :)
Marked done automatically, I'll address this before commiting :)



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

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.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1083
+
+  const Expr *getArgExpr(unsigned Index = 0) const override {
+return getOriginExpr()->getArgument();

xazax.hun wrote:
> Is the index unused on purpose? If yes, wouldn't this trigger a warning on 
> build bots? Adding a maybe unused annotation and/or a comment would be 
> welcome.
Unused argument warnings are disabled by the build system, but a comment 
wouldn't hurt. `CXXDeleteExpr`s can only have a single argument, but as earlier 
inlines suggests, we will need to mind custom deletes later on. 


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

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Overall looks good to me some questions and nits inline.




Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:59
 enum CallEventKind {
+  CE_CXXDeallocator,
   CE_Function,

Szelethus wrote:
> I need to move this below `CE_Function`, as its not in the range of 
> `CE_BEG_FUNCTION_CALLS` and `CE_END_FUNCTION_CALLS`.
This is marked as done. If the conclusion is that you do not need to move it I 
think it would be worth to add a comment why :)



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

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.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:1083
+
+  const Expr *getArgExpr(unsigned Index = 0) const override {
+return getOriginExpr()->getArgument();

Is the index unused on purpose? If yes, wouldn't this trigger a warning on 
build bots? Adding a maybe unused annotation and/or a comment would be welcome.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp:95
   ExplodedNode *N = C.generateNonFatalErrorNode();
+  if (!N)
+return;

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.


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

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

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

Szelethus added a comment.

Ping


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

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

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

Szelethus added a comment.

Ping^2 :)


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

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

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

Szelethus added a comment.

Just a gentle ping :)


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

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

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

Szelethus marked an inline comment as done.
Szelethus added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:59
 enum CallEventKind {
+  CE_CXXDeallocator,
   CE_Function,

I need to move this below `CE_Function`, as its not in the range of 
`CE_BEG_FUNCTION_CALLS` and `CE_END_FUNCTION_CALLS`.


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

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

Szelethus updated this revision to Diff 254780.
Szelethus marked 2 inline comments as done.
Szelethus added a comment.
Herald added a subscriber: mgorny.

- Add `PostStmt`
- Add PostCall for `CXXDeallocatorCall`
- Add a unittest, which is kind of pointless as-is, but I realized only later 
that most of the delete operators aren't recognized as `CXXDeleteExpr`
- Add a bunch of `TODO`s about incorrect `CallEvent` types.


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

https://reviews.llvm.org/D75430

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/CallEventTest.cpp

Index: clang/unittests/StaticAnalyzer/CallEventTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/CallEventTest.cpp
@@ -0,0 +1,89 @@
+//===--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "CheckerRegistration.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+namespace {
+
+void reportBug(const CheckerBase *Checker, const CallEvent ,
+   CheckerContext , StringRef WarningMsg) {
+  C.getBugReporter().EmitBasicReport(
+  nullptr, Checker, "", categories::LogicError, WarningMsg,
+  PathDiagnosticLocation(Call.getOriginExpr(), C.getSourceManager(),
+ C.getLocationContext()),
+  {});
+}
+
+class CXXDeallocatorChecker : public Checker {
+  std::unique_ptr BT_uninitField;
+
+public:
+  CXXDeallocatorChecker()
+  : BT_uninitField(new BuiltinBug(this, "CXXDeallocator")) {}
+
+  void checkPreCall(const CallEvent , CheckerContext ) const {
+const auto *DC = dyn_cast();
+if (!DC) {
+  return;
+}
+
+SmallString<100> WarningBuf;
+llvm::raw_svector_ostream WarningOS(WarningBuf);
+WarningOS << "NumArgs: " << DC->getNumArgs();
+
+reportBug(this, *DC, C, WarningBuf);
+  }
+};
+
+void addCXXDeallocatorChecker(AnalysisASTConsumer ,
+  AnalyzerOptions ) {
+  AnOpts.CheckersAndPackages = {{"test.CXXDeallocator", true}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry ) {
+Registry.addChecker("test.CXXDeallocator",
+   "Description", "");
+  });
+}
+
+// TODO: What we should really be testing here is all the different varieties
+// of delete operators, and wether the retrieval of their arguments works as
+// intended. At the time of writing this file, CXXDeallocatorCall doesn't pick
+// up on much of those due to the AST not containing CXXDeleteExpr for most of
+// the standard/custom deletes.
+TEST(CXXDeallocatorCall, SimpleDestructor) {
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCode(R"(
+struct A {};
+
+void f() {
+  A *a = new A;
+  delete a;
+}
+  )",
+ Diags));
+  EXPECT_EQ(Diags, "test.CXXDeallocator:NumArgs: 1\n");
+}
+
+} // namespace
+} // namespace ento
+} // namespace clang
Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -5,6 +5,7 @@
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
   CallDescriptionTest.cpp
+  CallEventTest.cpp
   StoreTest.cpp
   RegisterCustomCheckersTest.cpp
   SymbolReaperTest.cpp
Index: clang/test/Analysis/cxx-dynamic-memory-analysis-order.cpp

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:958
+
+  unsigned getNumArgs() const override { return getDecl()->getNumParams(); }
+

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


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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I love where this is going.

> the reason behind this is that neither does `preStmt` have a 
> `postStmt` pair. But I have no clue why that is :^)

I'm pretty sure it's forgotten :/




Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:67
   CE_CXXAllocator,
+  CE_CXXDeallocator,
   CE_BEG_FUNCTION_CALLS = CE_Function,

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.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:958
+
+  unsigned getNumArgs() const override { return getDecl()->getNumParams(); }
+

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



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:853
+  CDE, Pred->getState(), Pred->getLocationContext());
+  getCheckerManager().runCheckersForPreCall(Dst, Pred, *Call, *this);
 }

Yes, we should absolutely have `PostCall` here as well, even if nothing happens 
in between.

And once it's there, the next major step would be to invoke 
`ExprEngine::evalCall()` instead if the operator was overloaded, so that we 
could //inline// the deallocator (or evaluate it conservatively, which might 
also be of value as it would invalidate the necessary globals).


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

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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

This might be a silly question but is `CXXDeleteExpr` the only way to invoke a 
deallocator? What would happen if the user would call it by hand? Should we 
expect `ExprEngine` to trigger a callback in that case?


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

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

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Cool! May it worth to mention the corresponding mail from the mailing list in 
the Summary: http://lists.llvm.org/pipermail/cfe-dev/2020-February/064754.html




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:941
+  CXXDeallocatorCall(const CXXDeleteExpr *E, ProgramStateRef St,
+ const LocationContext *LCtx)
+  : AnyFunctionCall(E, St, LCtx) {}

`St` -> `State`



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:958
+
+  unsigned getNumArgs() const override { return getDecl()->getNumParams(); }
+

`return getOriginExpr()->getNumArgs()`


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

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

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

Szelethus created this revision.
Szelethus added reviewers: balazske, martong, NoQ, xazax.hun, rnkovacs, 
dcoughlin, baloghadamsoftware.
Herald added subscribers: cfe-commits, steakhal, Charusso, gamesh411, dkrupp, 
donat.nagy, mikhail.ramalho, a.sidorin, szepet, whisperity.
Herald added a project: clang.
Szelethus added a parent revision: D68165: [analyzer][MallocChecker][NFC] Split 
checkPostCall up, deploy CallDescriptionMap.

This is my first ever patch touching `ExprEngine`, sorry if the code causes 
lasting damage to the reader.

One of the pain points in simplifying `MallocChecker`s interface by gradually 
changing to `CallEvent` is that a variety of C++ allocation and deallocation 
functionalities are modeled through `preStmt<...>` where `CallEvent` is 
unavailable, and a single one of these callbacks can prevent a mass parameter 
change.

This patch introduces a new `CallEvent`, `CXXDeallocatorCall`, which happens 
//after// `preStmt`, and can completely replace that callback as 
demonstrated. Note how you can retrieve this call through `preCall`, yet there 
is no `postCall` to pair it with -- the reason behind this is that neither does 
`preStmt` have a `postStmt` pair. But I have no 
clue why that is :^)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75430

Files:
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/operator-delete-analysis-order.cpp

Index: clang/test/Analysis/operator-delete-analysis-order.cpp
===
--- /dev/null
+++ clang/test/Analysis/operator-delete-analysis-order.cpp
@@ -0,0 +1,67 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -fblocks -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=debug.AnalysisOrder \
+// RUN:   -analyzer-config debug.AnalysisOrder:PreStmtCXXDeleteExpr=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:PostStmtCXXDeleteExpr=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:PreCall=true \
+// RUN:   -analyzer-config debug.AnalysisOrder:PostCall=true \
+// RUN:   2>&1 | FileCheck %s
+
+// expected-no-diagnostics
+
+namespace fundamental_dealloc {
+void f() {
+  int *p = new int;
+  delete p;
+}
+} // namespace fundamental_dealloc
+
+namespace record_dealloc {
+struct S {
+  int x, y;
+};
+
+void f() {
+  S *s = new S;
+  delete s;
+}
+} // namespace record_dealloc
+
+namespace nontrivial_destructor {
+struct NontrivialDtor {
+  int x, y;
+
+  ~NontrivialDtor() {
+// Casually mine bitcoin.
+  }
+};
+
+void f() {
+  NontrivialDtor *s = new NontrivialDtor;
+  delete s;
+}
+} // namespace nontrivial_destructor
+
+// Mind that the results here are in reverse order compared how functions are
+// defined in this file.
+
+// CHECK:  PreCall (operator new)
+// CHECK-NEXT: PostCall (operator new)
+// CHECK-NEXT: PreCall (nontrivial_destructor::NontrivialDtor::NontrivialDtor)
+// CHECK-NEXT: PostCall (nontrivial_destructor::NontrivialDtor::NontrivialDtor)
+// CHECK-NEXT: PreCall (nontrivial_destructor::NontrivialDtor::~NontrivialDtor)
+// CHECK-NEXT: PostCall (nontrivial_destructor::NontrivialDtor::~NontrivialDtor)
+// CHECK-NEXT: PreStmt
+// CHECK-NEXT: PreCall (operator delete)
+
+// CHECK:  PreCall (operator new)
+// CHECK-NEXT: PostCall (operator new)
+// CHECK-NEXT: PreCall (record_dealloc::S::S)
+// CHECK-NEXT: PostCall (record_dealloc::S::S)
+// CHECK-NEXT: PreStmt
+// CHECK-NEXT: PreCall (operator delete)
+
+// CHECK:  PreCall (operator new)
+// CHECK-NEXT: PostCall (operator new)
+// CHECK-NEXT: PreStmt
+// CHECK-NEXT: PreCall (operator delete)
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -36,17 +36,22 @@
 // CHECK-NEXT: deadcode.DeadStores:WarnForDeadNestedAssignments = true
 // CHECK-NEXT: debug.AnalysisOrder:* = false
 // CHECK-NEXT: debug.AnalysisOrder:Bind = false
+// CHECK-NEXT: debug.AnalysisOrder:EndAnalysis = false
 // CHECK-NEXT: debug.AnalysisOrder:EndFunction = false
 // CHECK-NEXT: debug.AnalysisOrder:LiveSymbols = false
 // CHECK-NEXT: debug.AnalysisOrder:NewAllocator = false
 // CHECK-NEXT: debug.AnalysisOrder:PointerEscape = false
 // CHECK-NEXT: debug.AnalysisOrder:PostCall = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtArraySubscriptExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXConstructExpr = false
+// CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXDeleteExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCXXNewExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtCastExpr = false
 // CHECK-NEXT: debug.AnalysisOrder:PostStmtOffsetOfExpr =