[PATCH] D35796: [analyzer] Misused polymorphic object checker

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs marked 3 inline comments as done.
rnkovacs added inline comments.



Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:296
 
+def MisusedPolymorphicObjectChecker: Checker<"MisusedPolymorphicObject">,
+ HelpText<"Reports deletions of polymorphic objects with a non-virtual "

dcoughlin wrote:
> I think users would find it helpful if this had a more specific name. There 
> are a lot of ways to misuse polymorphic objects, and this checker won't check 
> all of them.
> 
> What do you think about "DeleteWithNonVirtualDestructor"?
I settled with the slightly abbreviated "DeleteWithNonVirtualDtor" version to 
shorten it a little.


https://reviews.llvm.org/D35796



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

[PATCH] D35796: [analyzer] Misused polymorphic object checker

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

This looks like a useful checker! Have you run this on large codebases yet? 
Does it find bugs? What kind of false positives do you see? Do you have a sense 
of what additional work would it take to bring this out of alpha and have it 
turned on by default?

Other than some super tiny comments in-line, this looks good to me.




Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:296
 
+def MisusedPolymorphicObjectChecker: Checker<"MisusedPolymorphicObject">,
+ HelpText<"Reports deletions of polymorphic objects with a non-virtual "

I think users would find it helpful if this had a more specific name. There are 
a lot of ways to misuse polymorphic objects, and this checker won't check all 
of them.

What do you think about "DeleteWithNonVirtualDestructor"?



Comment at: lib/StaticAnalyzer/Checkers/MisusedPolymorphicObjectChecker.cpp:16
+// the last point where the derived-to-base conversion happened.
+//
+//===--===//

I think it would be helpful to future maintainers to provide a high-level 
description of how this differs from `-Wnon-virtual-dtor` and 
`-Wdelete-non-virtual-dtor`.



Comment at: lib/StaticAnalyzer/Checkers/MisusedPolymorphicObjectChecker.cpp:137
+  llvm::raw_svector_ostream OS(Buf);
+  OS << "Derived-to-base conversion happened here";
+  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),

A minor style suggestion to avoid the stacked noun phrase: "Conversion from 
derived to base happened here".


https://reviews.llvm.org/D35796



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

[PATCH] D35796: [analyzer] Misused polymorphic object checker

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs added a comment.

In https://reviews.llvm.org/D35796#819965, @NoQ wrote:

> It seems that this check is more powerful because it works by knowing the 
> dynamic type of the object. However, i still suspect that 
> `-Wnon-virtual-dtor` (the other one, without `delete-`, that simply asks to 
> make the destructor of polymorphic classes virtual) covers most practical 
> cases. The only thing i see not covered by `-Wnon-virtual-dtor` but covered 
> by this checker is the situation when the destructor is private. Reka, would 
> you confirm our understanding?


You're right, the flag covers many cases, but there are some common exceptions. 
E.g. deleting a polymorphic object without a virtual destructor might not be a 
problem if it's referenced by its precise type. The checker is aware of this. 
It looks for object deletions, checks their dynamic and static types, and only 
warns if an object is deleted through a base pointer (with no virtual 
destructor).

I added a slightly more sophisticated test case for the private destructor 
situation, I'm wondering whether you meant something like that.


https://reviews.llvm.org/D35796



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

[PATCH] D35796: [analyzer] Misused polymorphic object checker

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs updated this revision to Diff 108226.

https://reviews.llvm.org/D35796

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/MisusedPolymorphicObjectChecker.cpp
  test/Analysis/MisusedPolymorphicObject.cpp

Index: test/Analysis/MisusedPolymorphicObject.cpp
===
--- /dev/null
+++ test/Analysis/MisusedPolymorphicObject.cpp
@@ -0,0 +1,187 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.PolymorphicDtorNotCalled -std=c++11 -verify -analyzer-output=text %s
+
+struct Virtual {
+  virtual ~Virtual() {}
+};
+
+struct VDerived : public Virtual {};
+
+struct NonVirtual {
+  ~NonVirtual() {}
+};
+
+struct NVDerived : public NonVirtual {};
+struct NVDoubleDerived : public NVDerived {};
+
+struct Base {
+  virtual void destroy() = 0;
+};
+
+class PrivateDtor : public Base {
+public:
+  void destroy() { delete this; }
+private:
+  ~PrivateDtor() {}
+};
+
+struct ImplicitNV {
+  virtual void f();
+};
+
+struct ImplicitNVDerived : public ImplicitNV {};
+
+NVDerived *get();
+
+NonVirtual *create() {
+  NonVirtual *x = new NVDerived(); // expected-note{{Derived-to-base conversion happened here}}
+  return x;
+}
+
+void sink(NonVirtual *x) {
+  delete x; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void sinkCast(NonVirtual *y) {
+  delete reinterpret_cast(y);
+}
+
+void sinkParamCast(NVDerived *z) {
+  delete z;
+}
+
+void singleDerived() {
+  NonVirtual *sd;
+  sd = new NVDerived(); // expected-note{{Derived-to-base conversion happened here}}
+  delete sd; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void singleDerivedArr() {
+  NonVirtual *sda = new NVDerived[5]; // expected-note{{Derived-to-base conversion happened here}}
+  delete[] sda; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void doubleDerived() {
+  NonVirtual *dd = new NVDoubleDerived(); // expected-note{{Derived-to-base conversion happened here}}
+  delete (dd); // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void assignThroughFunction() {
+  NonVirtual *atf = get(); // expected-note{{Derived-to-base conversion happened here}}
+  delete atf; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void assignThroughFunction2() {
+  NonVirtual *atf2;
+  atf2 = get(); // expected-note{{Derived-to-base conversion happened here}}
+  delete atf2; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void createThroughFunction() {
+  NonVirtual *ctf = create(); // expected-note{{Calling 'create'}}
+  // expected-note@-1{{Returning from 'create'}}
+  delete ctf; // expected-warning {{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void deleteThroughFunction() {
+  NonVirtual *dtf = new NVDerived(); // expected-note{{Derived-to-base conversion happened here}}
+  sink(dtf); // expected-note{{Calling 'sink'}}
+}
+
+void singleCastCStyle() {
+  NVDerived *sccs = new NVDerived();
+  NonVirtual *sccs2 = (NonVirtual*)sccs; // expected-note{{Derived-to-base conversion happened here}}
+  delete sccs2; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void doubleCastCStyle() {
+  NonVirtual *dccs = new NVDerived();
+  NVDerived *dccs2 = (NVDerived*)dccs;
+  dccs = (NonVirtual*)dccs2; // expected-note{{Derived-to-base conversion happened here}}
+  delete dccs; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void singleCast() {
+  NVDerived *sc = new NVDerived();
+  NonVirtual *sc2 = reinterpret_cast(sc); // expected-note{{Derived-to-base conversion happened here}}
+  delete sc2; // expected-warning{{Polymorphic object without 

[PATCH] D35796: [analyzer] Misused polymorphic object checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

It seems that this check is more powerful because it works by knowing the 
dynamic type of the object. However, i still suspect that `-Wnon-virtual-dtor` 
(the other one, without `delete-`, that simply asks to make the destructor of 
polymorphic classes virtual) covers most practical cases. The only thing i see 
not covered by `-Wnon-virtual-dtor` but covered by this checker is the 
situation when the destructor is private. Reka, would you confirm our 
understanding?


https://reviews.llvm.org/D35796



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

[PATCH] D35796: [analyzer] Misused polymorphic object checker

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

How does this check differ from the `-Wdelete-non-virtual-dtor` warning class 
that comes out of the frontend?


https://reviews.llvm.org/D35796



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

[PATCH] D35796: [analyzer] Misused polymorphic object checker

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs created this revision.
Herald added subscribers: baloghadamsoftware, xazax.hun, whisperity, mgorny.

This check warns if a derived type object is deleted through a base pointer 
with a non-virtual destructor in its base class. It also places a note at the 
last point where the derived-to-base conversion happened.

Corresponding CERT rule: OOP52-CPP: Do not delete a polymorphic object without 
a virtual destructor. 



https://reviews.llvm.org/D35796

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/MisusedPolymorphicObjectChecker.cpp
  test/Analysis/MisusedPolymorphicObject.cpp

Index: test/Analysis/MisusedPolymorphicObject.cpp
===
--- /dev/null
+++ test/Analysis/MisusedPolymorphicObject.cpp
@@ -0,0 +1,168 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.PolymorphicDtorNotCalled -std=c++11 -verify -analyzer-output=text %s
+
+struct Virtual {
+  virtual ~Virtual() {}
+};
+
+struct VDerived : public Virtual {};
+
+struct NonVirtual {
+  ~NonVirtual() {}
+};
+
+struct NVDerived : public NonVirtual {};
+struct NVDoubleDerived : public NVDerived {};
+
+struct ImplicitNV {
+  virtual void f();
+};
+
+struct ImplicitNVDerived : public ImplicitNV {};
+
+NVDerived *get();
+
+NonVirtual *create() {
+  NonVirtual *x = new NVDerived(); // expected-note{{Derived-to-base conversion happened here}}
+  return x;
+}
+
+void sink(NonVirtual *x) {
+  delete x; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void sinkCast(NonVirtual *y) {
+  delete reinterpret_cast(y);
+}
+
+void sinkParamCast(NVDerived *z) {
+  delete z;
+}
+
+void singleDerived() {
+  NonVirtual *sd;
+  sd = new NVDerived(); // expected-note{{Derived-to-base conversion happened here}}
+  delete sd; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void singleDerivedArr() {
+  NonVirtual *sda = new NVDerived[5]; // expected-note{{Derived-to-base conversion happened here}}
+  delete[] sda; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void doubleDerived() {
+  NonVirtual *dd = new NVDoubleDerived(); // expected-note{{Derived-to-base conversion happened here}}
+  delete (dd); // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void assignThroughFunction() {
+  NonVirtual *atf = get(); // expected-note{{Derived-to-base conversion happened here}}
+  delete atf; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void assignThroughFunction2() {
+  NonVirtual *atf2;
+  atf2 = get(); // expected-note{{Derived-to-base conversion happened here}}
+  delete atf2; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void createThroughFunction() {
+  NonVirtual *ctf = create(); // expected-note{{Calling 'create'}}
+  // expected-note@-1{{Returning from 'create'}}
+  delete ctf; // expected-warning {{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void deleteThroughFunction() {
+  NonVirtual *dtf = new NVDerived(); // expected-note{{Derived-to-base conversion happened here}}
+  sink(dtf); // expected-note{{Calling 'sink'}}
+}
+
+void singleCastCStyle() {
+  NVDerived *sccs = new NVDerived();
+  NonVirtual *sccs2 = (NonVirtual*)sccs; // expected-note{{Derived-to-base conversion happened here}}
+  delete sccs2; // expected-warning{{Polymorphic object without virtual destructor deleted through base pointer}}
+  // expected-note@-1{{Polymorphic object without virtual destructor deleted through base pointer}}
+}
+
+void doubleCastCStyle() {
+  NonVirtual *dccs = new NVDerived();
+  NVDerived *dccs2 = (NVDerived*)dccs;
+  dccs = (NonVirtual*)dccs2; // expected-note{{Derived-to-base conversion happened here}}
+  delete dccs; // expected-warning{{Polymorphic object without virtual destructor deleted through base