[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:125
+        Msg = std::string(llvm::formatv(
+            "Possibly not enough {0} bytes for array allocation which "
+            "requires "
----------------
f00kat wrote:
> martong wrote:
> > Maybe the below could be a wording that's more easy to follow?
> > `{0} bytes is possibly not enough for array allocation which ...`
> Yeah. Sure. I have some troubles with english :)
Why do we say "possibly"? Where does the uncertainty come from?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:189
+  if (const MemRegion *MRegion = PlaceVal.getAsRegion()) {
+    if (const ElementRegion *TheElementRegion =
+            MRegion->getAs<ElementRegion>()) {
----------------
martong wrote:
> f00kat wrote:
> > f00kat wrote:
> > > NoQ wrote:
> > > > NoQ wrote:
> > > > > The sequence of `FieldRegion`s and `ElementRegion`s on top of a base region may be arbitrary: `var.a[0].b[1][2].c.d[3]` etc.
> > > > > 
> > > > > I'd rather unwrap those regions one-by-one in a loop and look at the alignment of each layer.
> > > > Alternatively, just decompose the whole region into base region and offset and see if base region has the necessary alignment and the offset is divisible by the necessary alignment.
> > > > The sequence of FieldRegions and ElementRegions on top of a base region may be arbitrary: var.a[0].b[1][2].c.d[3] etc.
> > > 
> > > But i think(hope) I already do this and even have tests for this cases. For example
> > > ```void test22() {
> > >   struct alignas(alignof(short)) Z {
> > >     char p;
> > >     char c[10];
> > >   };
> > > 
> > >   struct Y {
> > >     char p;
> > >     Z b[10];
> > >   };
> > > 
> > >   struct X {
> > >     Y a[10];
> > >   } Xi; // expected-note {{'Xi' initialized here}}
> > > 
> > >   // ok. 2(X align) + 1 (offset Y.p) + 1(align Z.p to 'short') + 1(offset Z.p) + 3(index)
> > >   ::new (&Xi.a[0].b[0].c[3]) long;
> > > }```
> > > 
> > > Cases with multidimensional arrays will also be handled correctly because method 'TheElementRegion->getAsArrayOffset()' calculates the offset for multidimensional arrays
> > > ```void testXX() {
> > > 	struct Y {
> > > 		char p;
> > > 		char b[10][10];
> > > 	};
> > > 
> > > 	struct X {
> > > 		Y a[10];
> > > 	} Xi;
> > > 
> > > 	::new (&Xi.a[0].b[0][0]) long;
> > > }```
> > > 
> > > I can explain the code below for ElementRegion if needed.
> > ?
> Yeah, the tests are convincing and I think that you are handling the regions well.
> 
> On the other hand, this code is getting really complex, we should make it easier to read and understand.
> E.g. `FieldOffsetValue` should be explained more, is it the offset started from the the start address of the multidimensional array, or it is just the offset from one element's start address?
> Also, you have two variables named as `Offset`. They are offsets from which starting address? Perhaps we should have in the comments a running example, maybe for `&Xi.a[0].b[0][0]`? I mean is `FieldOffseValue` is standing for `b` or for `a`?
> Alternatively, just decompose the whole region into base region and offset and see if base region has the necessary alignment and the offset is divisible by the necessary alignment.

I expect this to be, like, 5 lines of code. I don't understand why the current code is so complicated, it looks like you're considering multiple cases but ultimately doing the same thing.


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

https://reviews.llvm.org/D76996