[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong marked 22 inline comments as done.
martong added a comment.

Thank you guys for the assiduous review!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:22
+                            ProgramStateRef State) const;
+  mutable std::unique_ptr<BuiltinBug> BT_Placement;
+};
----------------
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.


================
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);
----------------
NoQ wrote:
> xazax.hun wrote:
> > Hmm, I think this logic might need some more testing. Could you add some tests with multi dimensional arrays?
> Yeah, this code is scary because at this point literally nobody knows when exactly do we an have element region wrapping the pointer (https://bugs.llvm.org/show_bug.cgi?id=43364).
> 
> `MemRegion` represents a segment of memory, whereas `loc::MemRegionVal` represents the point that is the left-hand side of that segment. I recommend using `C.getSVal(Place).getAsRegion()` only as a reference to that point, not the whole segment. Then you could decompose the region into a base region and an offset (i.e., `MemRegion::getAsOffset()`), and subtract the offset from the base region's extent to see how much space is there in the region.
Thanks! I've done the decomposition of Place to base region and offset. This way handling of multi-dimensional arrays is also looking good.


================
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)
----------------
xazax.hun wrote:
> 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`.
I removed this hunk, because it is no longer needed once we work with the base region and with the offset.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:64
+    CheckerContext &C, const CXXNewExpr *NE, ProgramStateRef State) const {
+  if (!State)
+    return SVal();
----------------
xazax.hun wrote:
> When do you see a null state?
Ok, I removed it.
(I think I got the idea of checking the state from the MallocChecker ... but I accept that it can never be null in my case).



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:65
+  if (!State)
+    return SVal();
+  SValBuilder &svalBuilder = C.getSValBuilder();
----------------
NoQ wrote:
> That produces an `UndefinedVal`. I think you'd much rather have `UnknownVal`.
I removed the check and with it the return, since @xazax.hun suggests that the state can never be null.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:72
+  } else {
+    ElementCount = svalBuilder.makeIntVal(1, true);
+  }
----------------
xazax.hun wrote:
> 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.
Ok, I changed that to return directly with the size of the type.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:79
+
+  if (ElementCount.getAs<NonLoc>()) {
+    // size in Bytes = ElementCount*TypeSize
----------------
xazax.hun wrote:
> You could:
> ```
> Optional<NonLoc> NL = ElementCount.getAs<NonLoc>();
> ```
> 
> And later you could replace the `castAs` with a deref of the optional.
Ok, I've done that.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:115-117
+          llvm::formatv("Argument of default placement new provides storage "
+                        "capacity of {0} bytes, but the allocated type "
+                        "requires storage capacity of {1} bytes",
----------------
NoQ wrote:
> whisperity wrote:
> > This message might be repeating phrases too much, and seems long. Also, I would expect things like //default placement new// or //argument of placement new// to be confusing. Not every person running Clang SA knows the nitty-gritty of the standard by heart...
> > 
> > More nitpicking: even the "default" (what does this even mean, again?) placement new takes **two** arguments, albeit written in a weird grammar, so there is no "argument of" by the looks of it. I really think this is confusing.
> > 
> > Something more concise, simpler, still getting the meaning across:
> > 
> > > Storage provided to placement new is only `N` bytes, whereas allocating a `T` requires `M` bytes
> > 
> Having long messages is usually not a problem for us (instead, we'd much rather have full sentences properly explaining what's going on), but i agree that your text is much neater and on point.
Ok, changed it.


================
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 {{}}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612