[PATCH] D158156: [analyzer] Add C++ array delete checker

[Discookie via Phabricator via cfe-commits]

Discookie marked 12 inline comments as done.
Discookie added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:192
+
+  const Stmt *S = N->getStmtForDiagnostics();
+  if (!S)
----------------
steakhal wrote:
> Discookie wrote:
> > steakhal wrote:
> > > Aren't you actually interested in N->getLocation().getAs<StmtPoint>().getStmt()?
> > > 
> > > The diag stmt can be fuzzy, but the PP is exact.
> > As far as I can tell, getStmtForDiagnostics() does exactly that, but with a bit more edge case handling and a couple fallbacks.
> If they do the same, and it does not depend on the mentioned fallbacks, I think we should use the Stmt of the programpoint to be explicit.
I'd rather not, since this pattern isn't used anywhere else, while getStmtForDiagnostics is widely used across the codebase.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:217
+  // Stop traversal on this path.
+  Satisfied = true;
+
----------------
steakhal wrote:
> There are so many early returns going on. I feel, we could miss the program point where it should have been marked satisfied. After this point, the visitor will never or should never emit a note.
Since we're emitting notes for all casts, this isn't needed anymore.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:194-201
   // Only interested in DerivedToBase implicit casts.
   // Explicit casts can have different CastKinds.
+  // FIXME: The checker currently matches all explicit casts,
+  // but only ones casting to a base class (or simular) should be matcherd.
   if (const auto *ImplCastE = dyn_cast<ImplicitCastExpr>(CastE)) {
     if (ImplCastE->getCastKind() != CK_DerivedToBase)
       return nullptr;
----------------
steakhal wrote:
> Have you considered `dyn_cast<CastExpr>()` to make it work for both implicit and explicit casts?
> BTW `simular` -> `similar`
I did exactly that a little earlier :D but I reworked the logic to filter based on the availability of types instead of the CastType.


================
Comment at: clang/test/Analysis/ArrayDelete.cpp:85-88
+    Derived *d3 = new DoubleDerived[10];
+    Base *b3 = d3; // expected-note{{Conversion from derived to base happened here}}
+    delete[] b3; // expected-warning{{Deleting an array of polymorphic objects is undefined}}
+    // expected-note at -1{{Deleting an array of polymorphic objects is undefined}}
----------------
donat.nagy wrote:
> steakhal wrote:
> > donat.nagy wrote:
> > > steakhal wrote:
> > > > Hmm, the static type of `d3` doesn't tell us much about how it refers to an object of type `DoubleDerived`.
> > > > To me, it would make sense to have multiple `Conversion from derived to base happened here`, even telling us what static type it converted to what other static type in the message.
> > > > And it should add a new visitor of the same kind tracking the castee.
> > > > 
> > > > ```
> > > > Derived *d3 = new DoubleDerived[10]; // note: `DoubleDerived` -> `Derived` here
> > > > Base *b3 = d3; // note: `Derived` -> `Base` here
> > > > delete[] b3; // warn: Deleting `Derived` objects as `Base` objects.
> > > > ```
> > > > WDYT @donat.nagy ?
> > > I agree that it would be good to be good to mention the class names in the message.
> > Do you also agree that we should have all steps where such a conversion happened?
> > Notice the 2 `note:` markers in my example. @donat.nagy 
> It would be a nice addition if it wouldn't seriously complicate the implementation.
> 
> If we want to report multiple/all conversions, then we would need to create messages for back-and-forth conversions (e.g. allocating Derived, converting it to Base, back to Derived, back to Base, then deleting it illegally).
Added, as requested! While casts of reference types are not supported, it already makes the flow of classes much clearer.


================
Comment at: clang/test/Analysis/ArrayDelete.cpp:90-93
+    Base *b4 = new DoubleDerived[10];
+    Derived *d4 = reinterpret_cast<Derived*>(b4);
+    DoubleDerived *dd4 = reinterpret_cast<DoubleDerived*>(d4);
+    delete[] dd4; // no-warning
----------------
donat.nagy wrote:
> steakhal wrote:
> > I think in such cases a `static_cast` should suffice; unless you intentionally wanted to test `reinterpret_cast` of course.
> Note that the effects of `reinterpret_cast` and `static_cast` are different [1]: `static_cast<Derived*>(BasePtr)` adjusts the pointer value to ensure that it points to the derived object whose Base ancestor object would be located at BasePtr, while a `reinterpret_cast` keeps the raw pointer value (which is complete nonsense from an object-oriented point-of-view, but could be relevant in some low-level hacks).
> 
> I'd guess that this part is directly testing the strange behavior of `reinterpret_cast`; but it'd be good to add a comment that clarifies this.
> 
> [1]: https://stackoverflow.com/questions/9138730/why-is-it-important-to-use-static-cast-instead-of-reinterpret-cast-here
Sadly there isn't any deeper meaning to `reinterpret_cast` here, just a mistake on my part. Fixed and changed to `static_cast`.


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

https://reviews.llvm.org/D158156