[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

That was fast! Looks alright.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:25
 public:
   void checkPreStmt(const CXXNewExpr *NE, CheckerContext &C) const;
 
----------------
Before i forget: Ideally @martong should have subscribed to [[ https://clang.llvm.org/doxygen/classclang_1_1ento_1_1CheckerDocumentation.html#a7fdb3b5ff726f4c5e782cef0d59c01ad | `checkNewAllocator` ]] because it fires before the construct-expression whereas this callback fires after construct-expression which is too late as the UB we're trying to catch has occured much earlier.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:165-166
+  if (const MemRegion *MRegion = PlaceVal.getAsRegion()) {
+    if (const FieldRegion *TheFieldRegion = MRegion->getAs<FieldRegion>())
+      MRegion = TheFieldRegion->getBaseRegion();
+
----------------
You're saying that `A` is a struct and `a` is of type `A` and `&a` is sufficiently aligned then for //every// field `f` in the struct `&a.f` is sufficiently aligned. I'm not sure it's actually the case.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:196-197
+    }
+  } else if (Optional<loc::ConcreteInt> TheConcreteInt =
+                 PlaceVal.getAs<loc::ConcreteInt>()) {
+    uint64_t PlaceAlign = *TheConcreteInt.getValue().getValue().getRawData();
----------------
I don't think you'll ever see this case in a real-world program. Even if you would, i doubt we'll behave as expected, because we have [[ https://github.com/llvm/llvm-project/blob/llvmorg-10.0.0/clang/lib/StaticAnalyzer/Core/Store.cpp#L456 | certain hacks ]] in place that mess up arithmetic on concrete pointers. I appreciate your thinking but i suggest removing this section for now as it'll probably cause more false positives than true positives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76996