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

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

This checker is a straightforward, clean implementation of the SEI-CERT rule EXP51-CPP, it'll be a good addition to Static Analyzer. I'm satisfied with the implementation, although there might be others who have a more refined taste in C++ coding style. The only issue that I found is the misleading name of the "common ancestor" hidden checker that I mentioned in the inline comment.

The test suite nicely covers all the basic situations, but I'd be happy to see some corner cases like virtual and non-virtual diamond inheritance or even a degenerate example like

  class Base { /*...*/ };
  class Derived: public Base {
    Base basesAsMember[10];
    /*...*/
  };
  void f() {
    Derived *d = new Derived(...);
    delete[] (d->basesAsMember);
  }



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:757-760
+def CXXArrayModeling: Checker<"CXXArrayModeling">,
+  HelpText<"The base of a few CXX array related checkers.">,
+  Documentation<NotDocumented>,
+  Hidden;
----------------
The name and help text of this hidden dependency is very misleading: it's not particularly connected to arrays (the connection between the two child checkers is that they both handle the "delete" operator) and it's not a modeling checker (which would provide function evaluation, constraints and transitions) but an "empty shell" checker that does nothing by itself, but provides a singleton checker object where the registration of the child checkers can enable the two analysis modes.

Please rename this to `CXXDeleteHelper` or something similar.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:88
+  const auto *BaseClassRegion = MR->getAs<TypedValueRegion>();
+  const auto *DerivedClassRegion = MR->getBaseRegion()->getAs<SymbolicRegion>();
+  if (!BaseClassRegion || !DerivedClassRegion)
----------------
This logic (which is inherited from the earlier checker) is a bit surprising because it strips //all// Element, Field, BaseObject and DerivedObject layers from the MemRegion object.

I think it works in practice because
(1) there is an `isDerivedFrom() check after it and
(2) it's rare to write (buggy) code like
```lang=c++
struct Inner { /*...*/ };
struct Outer { Inner inner; /*...*/ };
void f() {
  Outer *o = new Outer(...);
  delete(o->inner);
}
```

Perhaps this could be explained by a comment if you feel that it's warranted; but I mostly wrote this as a note for myself and other reviewers. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158156