[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32
+  const MemRegion *BaseRegion = MRegion->getBaseRegion();
+  assert(BaseRegion == Offset.getRegion());
+
----------------
martong wrote:
> This assertion fails on real code quite often (e.g. on LLVM/Clang code). I don't really understand why. @NoQ what is you understanding on this? Perhaps I could try to reduce a case from the real code to see an example.
`RegionOffset` cannot really represent symbolic offsets :( You should be able to re-add the assertion after checking for symbolic offsets.

And, yeah, you can always easily reduce any crash with `creduce`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22
+                            ProgramStateRef State) const;
+  mutable std::unique_ptr<BuiltinBug> BT_Placement;
+};
----------------
martong wrote:
> xazax.hun wrote:
> > I think now it is safe to have the bugtype by value and use member initialization.
> Ok, I've made it to be a simple member. Also could remove the `mutable` specifier.
And use a default initializer instead of constructor >.>
`BuiltinBug BT_Placement{this, "Insufficient storage BB"}`

(also i don't understand `BuiltinBug` and i'm afraid of using it, can we just have `BugType` with explicit description and category?)


================
Comment at: clang/test/Analysis/placement-new.cpp:11
+void f() {
+  short s; // expected-note-re {{'s' declared{{.*}}}}
+  long *lp = ::new (&s) long; // expected-warning{{Argument of default placement new provides storage capacity of 2 bytes, but the allocated type requires storage capacity of 8 bytes}} expected-note 3 {{}}
----------------
martong wrote:
> NoQ wrote:
> > I'm legit curious what's hidden behind the regex.
> Ok, I removed the regexes.
> But keep in mind that, this note is coming from `trackExpressionValue` and is changing if the initializer of `s` is changing.
> I did not want to formulate expectations on a note that is coming from an implementation detail and can change easily. Also, If `trackExpressionValue` changes than we need to rewrite all these tests, unless a regex is used, perhaps just a simple expected-note {{}} would be the best from this point of view.
Because it's up to the checker whether to use `trackExpressionValue` or provide its own visitor, i'd rather keep the notes tested. If the notes provided by `trackExpressionValue` aren't good enough, it's ultimately the checker's problem (which may or may not be fixed by improving `trackExpressionValue`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612