[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:91
+  SVal SizeOfPlace = getExtentSizeOfPlace(C, Place, State);
+  const auto SizeOfTargetCI = SizeOfTarget.getAs<nonloc::ConcreteInt>();
+  if (!SizeOfTargetCI)
----------------
xazax.hun wrote:
> Here, instead of getting `SizeOfTarget` and `SizeOfPlace` as `ConcreteInt`s, I think you should rather use `evalBinOp` to compare them. That method is more future proof as if we cannot constraint these values down to a single integer but we still have some information about them a sufficiently smart solver could prove the relationship between the symbolic values.
I am not sure if `evalBinOp` is that useful here, because we need the concrete values anyway when we issue the diagnostics. We'd like to present the concrete sizes in bytes.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:32
+  const MemRegion *BaseRegion = MRegion->getBaseRegion();
+  assert(BaseRegion == Offset.getRegion());
+
----------------
NoQ wrote:
> 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`.
I think protecting with `if (Offset.hasSymbolicOffset())` is just fine, instead of the assertion.


================
Comment at: clang/test/Analysis/placement-new.cpp:6
+// RUN:   -analyzer-output=text -verify \
+// RUN:   -triple x86_64-unknown-linux-gnu
+
----------------
NoQ wrote:
> Wow, somebody actually remembers to add a target triple for tests that depend on the target triple! My respect, sir.
Thanks! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612