[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22
+                            ProgramStateRef State) const;
+  mutable std::unique_ptr<BuiltinBug> BT_Placement;
+};
----------------
I think now it is safe to have the bugtype by value and use member initialization.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:35
+  //   long *lp = ::new (buf) long;
+  if (const auto *TVRegion = dyn_cast<TypedValueRegion>(MRegion))
+    // FIXME Handle offset other than 0. E.g.:
----------------
Add braces here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:40
+    if (const auto *ERegion = dyn_cast<ElementRegion>(TVRegion)) {
+      const auto *SRegion = cast<SubRegion>(ERegion->getSuperRegion());
+      DefinedOrUnknownSVal Extent = SRegion->getExtent(svalBuilder);
----------------
Hmm, I think this logic might need some more testing. Could you add some tests with multi dimensional arrays?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:44
+      // This is modelled by the MallocChecker.
+      const llvm::APSInt *ExtentInt = svalBuilder.getKnownValue(State, Extent);
+      if (!ExtentInt)
----------------
This query will only check if you know the exact value of the target of placement you.

What you actually care about if the size is at least as big as the placed object.

So instead of getting a known value I think it might be a better idea to evaluate a less than or equal operator with `evalBinOp`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:64
+    CheckerContext &C, const CXXNewExpr *NE, ProgramStateRef State) const {
+  if (!State)
+    return SVal();
----------------
When do you see a null state?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:72
+  } else {
+    ElementCount = svalBuilder.makeIntVal(1, true);
+  }
----------------
Probably you could just return the size without building a more complex symbolic expression in this case? I.e. just put the right value in the makeIntVal.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:79
+
+  if (ElementCount.getAs<NonLoc>()) {
+    // size in Bytes = ElementCount*TypeSize
----------------
You could:
```
Optional<NonLoc> NL = ElementCount.getAs<NonLoc>();
```

And later you could replace the `castAs` with a deref of the optional.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:81
+    // size in Bytes = ElementCount*TypeSize
+    SVal SizeInBytes = svalBuilder.evalBinOpNN(
+        State, BO_Mul, ElementCount.castAs<NonLoc>(),
----------------
Prefer to use `evalBinOp` over `evalBinOpNN`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612