[PATCH] D76996: [analyzer] Improve PlacementNewChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:189
+  if (const MemRegion *MRegion = PlaceVal.getAsRegion()) {
+    if (const ElementRegion *TheElementRegion =
+            MRegion->getAs<ElementRegion>()) {
----------------
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`?


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

https://reviews.llvm.org/D76996