[PATCH] D71612: [analyzer] Add PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
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);
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:65
+  if (!State)
+    return SVal();
+  SValBuilder &svalBuilder = C.getSValBuilder();
----------------
That produces an `UndefinedVal`. I think you'd much rather have `UnknownVal`.


================
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",
----------------
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.


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


================
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 {{}}
----------------
I'm legit curious what's hidden behind the regex.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71612