[analyzer] Improve PlacementNewChecker

[f00kat]

Summary

1. Added insufficient storage check for arrays
2. Added align support check

Based on https://reviews.llvm.org/D76229

[f00kat]

lint fixes

[NoQ]

That was fast! Looks alright.

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

25

Before i forget: Ideally @martong should have subscribed to [[ https://clang.llvm.org/doxygen/classclang_1_1ento_1_1CheckerDocumentation.html#a7fdb3b5ff726f4c5e782cef0d59c01ad | checkNewAllocator ]] because it fires before the construct-expression whereas this callback fires after construct-expression which is too late as the UB we're trying to catch has occured much earlier.

182–183

You're saying that A is a struct and a is of type A and &a is sufficiently aligned then for every field f in the struct &a.f is sufficiently aligned. I'm not sure it's actually the case.

213–214

I don't think you'll ever see this case in a real-world program. Even if you would, i doubt we'll behave as expected, because we have certain hacks in place that mess up arithmetic on concrete pointers. I appreciate your thinking but i suggest removing this section for now as it'll probably cause more false positives than true positives.

[martong]

Wohoow! I am impressed, this is really nice work, I like it! :) Could not find any glitch, looks good from my side once you address NoQ's concerns.

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

142

Maybe the below could be a wording that's more easy to follow?
{0} bytes is possibly not enough for array allocation which ...

[f00kat]

1. Removed code and tests for ConcreteInt cases
2. Fixed FieldRegion check
3. Added handling for ElementRegion cases such as

void f7() {
  short b[10];

  // ok. 2(short align) + 3*2(index '1' offset)
  ::new (&b[3]) long;
}

4. Fixed align error message
5. Maybe fixed lint warnings

[f00kat]

test fix

[martong]

clang/test/Analysis/placement-new.cpp

265

Maybe it is just me, but the contents of the parens here and above seems a bit muddled (index '2' offset). This should be (index '1' offset), shouldn't it?
What is the exact meaning of the number in the hyphens ('2' in this case), could you please elaborate?

[f00kat]

Fixed comments in tests

[martong]

LGTM! Thanks! But I am not that confident with the element regions and field regions, so @NoQ could you please take another look?

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

98–99

This will break build-bots that run with -Werror.

../../git/llvm-project/clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp:82:15: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
   if (IsArray = NE->isArray()) {

clang/test/Analysis/placement-new.cpp

256

First I was wondering if we indeed handle correctly structs with nested arrays whose element's type is a structs with nested arrays (... and so on).

So, I tried the below test, and it seems okay. Thus I think it might be worth to add something similar to it.

void f9_1() {
  struct Y {
    char a;
    alignas(alignof(short)) char b[20];
  };
  struct X {
    char e;
    Y f[20];
  } Xi; // expected-note {{'Xi' initialized here}}

  // ok 2(custom align) + 6*1(index '6' offset)
  ::new (&Xi.f[6].b[6]) long;

  // bad 2(custom align) + 1*1(index '1' offset)
  ::new (&Xi.f[1].b[1]) long; // expected-warning{{Storage type is aligned to 3 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
}

[f00kat]

1. Maybe build fix
2. Added tests for nested arrays of structures
3. Fixed bugs in implementation for ElementRegion cases

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

282

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.

I'd rather unwrap those regions one-by-one in a loop and look at the alignment of each layer.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

282

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.

[f00kat]

Ping? :)

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

25

Ops, I forgot about it. Will be fixed soon.

25

When I use checkNewAllocator instead of check::PreStmt<CXXNewExpr> an error occures in method PathDiagnosticBuilder::generate.
In the code ErrorNode->getLocation().getTag() because ProgramPoint contains an empty ProgramPointTag.
I`m not very good with analyzer so its hard to understand what`s going on, but I`m still trying..

142

Yeah. Sure. I have some troubles with english :)

182–183

Yeah..Maybe we should take into account struct type align plus field offset? Currently, I am relying only on the fact that just struct type is aligned properly

For example

struct X
{
  char a;
  int b;
} x;

Type X is aligned to 'int' and thus field 'x.a' is also aligned to 'int' because it goes first

struct Y
{
  char a;
  int b;
  char c;
  char d;
} y;

But here field 'y.d' is aligned to 'char'

I will learn more about RegionOffset and will be back :)

213–214

Okay I will remove it!

282

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.

282

?

clang/test/Analysis/placement-new.cpp

256

Thanks! Added tests for this cases.

265

Yeah, sorry it is copy-paste error. Of course there should be '1'.

// bad 2(custom align) + 1(index '1' offset)

[martong]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

282

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?

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

142

Why do we say "possibly"? Where does the uncertainty come from?

282

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.

[f00kat]

1. Rewroted ElementRegion processing and fixed tests for this cases.
2. Simplified the code a bit.

[martong]

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

261

Perhaps you could call instead checkVarRegionAlign()?

261

Also, I think BaseRegion can be an ElementRegion here as well. So, in that case we should call into checkElementRegionAlign(), shouldn't we? This draws a pattern that we should recursively descend down to the top most base region. I.e. the different check*RegionAlign methods should call into each other until we reach the top level base region.

The observation here is that the alignment of a region can be correct only if we can prove that its base region is aligned properly (and other requirements, e.g. the offset is divisible). But the base region may have another base region and we have to prove the alignment correctness to that as well.

I hope this makes sense, please correct me if I am wrong.

clang/test/Analysis/placement-new.cpp

245

So these tests failed after you rewrote ElementRegion processing, right?
Actually, I wonder why we thought that if x is divisible by 2 then (x+6) will be divisible by 8 unconditionally.It's good you have this fixed.

[f00kat]

1. Refactoring
2. Build fix

[f00kat]

1. Build fix

[martong]

... This draws a pattern that we should recursively descend down to the top most base region. I.e. the different check*RegionAlign methods should call into each other until we reach the top level base region.

The observation here is that the alignment of a region can be correct only if we can prove that its base region is aligned properly (and other requirements, e.g. the offset is divisible). But the base region may have another base region and we have to prove the alignment correctness to that as well.

This could be an issue not just with alignment but maybe with the size as well, I am not sure if we handle the offset properly in compound cases like this: Xi.b[0].a[1][6].

Even though the above issue is still not investigated/handled, I think this patch is now acceptable because seems like most of the practical cases are handled. We could further investigate the concern and improve in a follow-up patch.
I'd like to see this landed and thanks for your work!

[f00kat]

In D76996#2017572, @martong wrote:

... This draws a pattern that we should recursively descend down to the top most base region. I.e. the different check*RegionAlign methods should call into each other until we reach the top level base region.

The observation here is that the alignment of a region can be correct only if we can prove that its base region is aligned properly (and other requirements, e.g. the offset is divisible). But the base region may have another base region and we have to prove the alignment correctness to that as well.

This could be an issue not just with alignment but maybe with the size as well, I am not sure if we handle the offset properly in compound cases like this: Xi.b[0].a[1][6].

Even though the above issue is still not investigated/handled, I think this patch is now acceptable because seems like most of the practical cases are handled. We could further investigate the concern and improve in a follow-up patch.
I'd like to see this landed and thanks for your work!

Thanks for feedback!

I still have no rights to push in the repo so if you think that it is acceptable could you commit it please?

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

142

I don’t know what specific size Clang uses for its internal needs in array cases. If you can tell this size, I will use it here.

261

Perhaps you could call instead checkVarRegionAlign()?

Yeah, you are right. Thank you!

Also, I think BaseRegion can be an ElementRegion here as well. So, in that case we should call into checkElementRegionAlign(), shouldn't we? This draws a pattern that we should recursively descend down to the top most base region. I.e. the different check*RegionAlign methods should call into each other until we reach the top level base region.

I`m not sure that BaseRegion can be ElementRegion. Anyway there always must be some Variable in the end? Or maybe I am wrong?

The observation here is that the alignment of a region can be correct only if we can prove that its base region is aligned properly (and other requirements, e.g. the offset is divisible). But the base region may have another base region and we have to prove the alignment correctness to that as well.

I hope this makes sense, please correct me if I am wrong.

I split check into to two stages.

1. Check BaseRegion align.

But if Var has its own align specifier we ignore BaseRegion align.

2. Check that total offset is divisible by the necessary alignment.

When I say total offset I mean that it is calculated from the BaseRegion(through all Fields and Elements Regions. e.g. Xi.a[10].b[20].c[30]. Total offset of 'c[30]' is offset from &Xi).

So in this solution we no need to recursively check all regions align.

282

Thank you for feedback!
Rewroted the code for ElementRegion cases. Now it is much easier to understand, and there was also a bug in logic.

clang/test/Analysis/placement-new.cpp

245

Yes, It is my fault. For example variable can be allocated at address 0x149E730 and it is well aligned to 2,4,8. And the assert that '0+6' is well
aligned to '8' will be wrong.

[martong]

Thanks, just committed.

[NoQ]

https://bugs.llvm.org/show_bug.cgi?id=46266!

clang/lib/StaticAnalyzer/Checkers/CheckPlacementNew.cpp

 #include "llvm/Support/FormatVariadic.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 class PlacementNewChecker : public Checker<check::PreStmt<CXXNewExpr>> {
 public:
   void checkPreStmt(const CXXNewExpr *NE, CheckerContext &C) const;
 
 private:
+  bool checkPlaceCapacityIsSufficient(const CXXNewExpr *NE,
+                                      CheckerContext &C) const;
+
+  bool checkPlaceIsAlignedProperly(const CXXNewExpr *NE,
+                                   CheckerContext &C) const;
+
   // Returns the size of the target in a placement new expression.
   // E.g. in "new (&s) long" it returns the size of `long`.
-  SVal getExtentSizeOfNewTarget(const CXXNewExpr *NE, ProgramStateRef State,
-                                CheckerContext &C) const;
+  SVal getExtentSizeOfNewTarget(const CXXNewExpr *NE, CheckerContext &C,
+                                bool &IsArray) const;
   // Returns the size of the place in a placement new expression.
   // E.g. in "new (&s) long" it returns the size of `s`.
-  SVal getExtentSizeOfPlace(const Expr *NE, ProgramStateRef State,
-                            CheckerContext &C) const;
-  BugType BT{this, "Insufficient storage for placement new",
+  SVal getExtentSizeOfPlace(const CXXNewExpr *NE, CheckerContext &C) const;
+
+  void emitBadAlignReport(const Expr *P, CheckerContext &C,
+                          unsigned AllocatedTAlign,
+                          unsigned StorageTAlign) const;
+  unsigned getStorageAlign(CheckerContext &C, const ValueDecl *VD) const;
+
+  void checkElementRegionAlign(const ElementRegion *R, CheckerContext &C,
+                               const Expr *P, unsigned AllocatedTAlign) const;
+
+  void checkFieldRegionAlign(const FieldRegion *R, CheckerContext &C,
+                             const Expr *P, unsigned AllocatedTAlign) const;
+
+  void checkVarRegionAlign(const VarRegion *R, CheckerContext &C, const Expr *P,
+                           unsigned AllocatedTAlign) const;
+
+  BugType SBT{this, "Insufficient storage for placement new",
+              categories::MemoryError};
+  BugType ABT{this, "Bad align storage for placement new",
               categories::MemoryError};
 };
 } // namespace
 
-SVal PlacementNewChecker::getExtentSizeOfPlace(const Expr *Place,
-                                               ProgramStateRef State,
+SVal PlacementNewChecker::getExtentSizeOfPlace(const CXXNewExpr *NE,
                                                CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  const Expr *Place = NE->getPlacementArg(0);
+
   const MemRegion *MRegion = C.getSVal(Place).getAsRegion();
   if (!MRegion)
     return UnknownVal();
   RegionOffset Offset = MRegion->getAsOffset();
   if (Offset.hasSymbolicOffset())
     return UnknownVal();
   const MemRegion *BaseRegion = MRegion->getBaseRegion();
   if (!BaseRegion)
     return UnknownVal();
 
   SValBuilder &SvalBuilder = C.getSValBuilder();
   NonLoc OffsetInBytes = SvalBuilder.makeArrayIndex(
       Offset.getOffset() / C.getASTContext().getCharWidth());
   DefinedOrUnknownSVal ExtentInBytes =
       getDynamicSize(State, BaseRegion, SvalBuilder);
 
   return SvalBuilder.evalBinOp(State, BinaryOperator::Opcode::BO_Sub,
                                ExtentInBytes, OffsetInBytes,
                                SvalBuilder.getArrayIndexType());
 }
 
 SVal PlacementNewChecker::getExtentSizeOfNewTarget(const CXXNewExpr *NE,
-                                                   ProgramStateRef State,
-                                                   CheckerContext &C) const {
+                                                   CheckerContext &C,
+                                                   bool &IsArray) const {
+  ProgramStateRef State = C.getState();
   SValBuilder &SvalBuilder = C.getSValBuilder();
   QualType ElementType = NE->getAllocatedType();
   ASTContext &AstContext = C.getASTContext();
   CharUnits TypeSize = AstContext.getTypeSizeInChars(ElementType);
+  IsArray = false;
   if (NE->isArray()) {
+    IsArray = true;
     const Expr *SizeExpr = *NE->getArraySize();
     SVal ElementCount = C.getSVal(SizeExpr);
     if (auto ElementCountNL = ElementCount.getAs<NonLoc>()) {
       // size in Bytes = ElementCountNL * TypeSize
       return SvalBuilder.evalBinOp(
           State, BO_Mul, *ElementCountNL,
           SvalBuilder.makeArrayIndex(TypeSize.getQuantity()),
           SvalBuilder.getArrayIndexType());
     }
   } else {
     // Create a concrete int whose size in bits and signedness is equal to
     // ArrayIndexType.
     llvm::APInt I(AstContext.getTypeSizeInChars(SvalBuilder.getArrayIndexType())
                           .getQuantity() *
                       C.getASTContext().getCharWidth(),
                   TypeSize.getQuantity());
     return SvalBuilder.makeArrayIndex(I.getZExtValue());
   }
   return UnknownVal();
 }
 
-void PlacementNewChecker::checkPreStmt(const CXXNewExpr *NE,
-                                       CheckerContext &C) const {
-  // Check only the default placement new.
-  if (!NE->getOperatorNew()->isReservedGlobalPlacementOperator())
-    return;
-  if (NE->getNumPlacementArgs() == 0)
-    return;
-
-  ProgramStateRef State = C.getState();
-  SVal SizeOfTarget = getExtentSizeOfNewTarget(NE, State, C);
-  const Expr *Place = NE->getPlacementArg(0);
-  SVal SizeOfPlace = getExtentSizeOfPlace(Place, State, C);
+bool PlacementNewChecker::checkPlaceCapacityIsSufficient(
+    const CXXNewExpr *NE, CheckerContext &C) const {
+  bool IsArrayTypeAllocated;
+  SVal SizeOfTarget = getExtentSizeOfNewTarget(NE, C, IsArrayTypeAllocated);
+  SVal SizeOfPlace = getExtentSizeOfPlace(NE, C);
   const auto SizeOfTargetCI = SizeOfTarget.getAs<nonloc::ConcreteInt>();
   if (!SizeOfTargetCI)
-    return;
+    return true;
   const auto SizeOfPlaceCI = SizeOfPlace.getAs<nonloc::ConcreteInt>();
   if (!SizeOfPlaceCI)
-    return;
+    return true;
 
-  if (SizeOfPlaceCI->getValue() < SizeOfTargetCI->getValue()) {
-    if (ExplodedNode *N = C.generateErrorNode(State)) {
-      std::string Msg = std::string(
-          llvm::formatv("Storage provided to placement new is only {0} bytes, "
-                        "whereas the allocated type requires {1} bytes",
-                        SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+  if ((SizeOfPlaceCI->getValue() < SizeOfTargetCI->getValue()) ||
+      (IsArrayTypeAllocated &&
+       SizeOfPlaceCI->getValue() >= SizeOfTargetCI->getValue())) {
+    if (ExplodedNode *N = C.generateErrorNode(C.getState())) {
+      std::string Msg;
+      // TODO: use clang constant
+      if (IsArrayTypeAllocated &&
+          SizeOfPlaceCI->getValue() > SizeOfTargetCI->getValue())
+        Msg = std::string(llvm::formatv(
+            "{0} bytes is possibly not enough for array allocation which "
+            "requires {1} bytes. Current overhead requires the size of {2} "
+            "bytes",
+            SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue(),
+            SizeOfPlaceCI->getValue() - SizeOfTargetCI->getValue()));
+      else if (IsArrayTypeAllocated &&
+               SizeOfPlaceCI->getValue() == SizeOfTargetCI->getValue())
+        Msg = std::string(llvm::formatv(
+            "Storage provided to placement new is only {0} bytes, "
+            "whereas the allocated array type requires more space for "
+            "internal needs",
+            SizeOfPlaceCI->getValue(), SizeOfTargetCI->getValue()));
+      else
+        Msg = std::string(llvm::formatv(
+            "Storage provided to placement new is only {0} bytes, "
 
-      auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
-      bugreporter::trackExpressionValue(N, Place, *R);
+      auto R = std::make_unique<PathSensitiveBugReport>(SBT, Msg, N);
+      bugreporter::trackExpressionValue(N, NE->getPlacementArg(0), *R);
       C.emitReport(std::move(R));
+
+      return false;
+    }
+  }
+
+  return true;
+}
+
+void PlacementNewChecker::emitBadAlignReport(const Expr *P, CheckerContext &C,
+                                             unsigned AllocatedTAlign,
+                                             unsigned StorageTAlign) const {
+  ProgramStateRef State = C.getState();
+  if (ExplodedNode *N = C.generateErrorNode(State)) {
+    std::string Msg(llvm::formatv("Storage type is aligned to {0} bytes but "
+                                  "allocated type is aligned to {1} bytes",
+                                  StorageTAlign, AllocatedTAlign));
+
+    auto R = std::make_unique<PathSensitiveBugReport>(ABT, Msg, N);
+    bugreporter::trackExpressionValue(N, P, *R);
+    C.emitReport(std::move(R));
+  }
+}
+
+unsigned PlacementNewChecker::getStorageAlign(CheckerContext &C,
+                                              const ValueDecl *VD) const {
+  unsigned StorageTAlign = C.getASTContext().getTypeAlign(VD->getType());
+  if (unsigned SpecifiedAlignment = VD->getMaxAlignment())
+    StorageTAlign = SpecifiedAlignment;
+
+  return StorageTAlign / C.getASTContext().getCharWidth();
+}
+
+void PlacementNewChecker::checkElementRegionAlign(
+    const ElementRegion *R, CheckerContext &C, const Expr *P,
+    unsigned AllocatedTAlign) const {
+  auto IsBaseRegionAlignedProperly = [this, R, &C, P,
+                                      AllocatedTAlign]() -> bool {
+    // Unwind nested ElementRegion`s to get the type.
+    const MemRegion *SuperRegion = R;
+    while (true) {
+      if (SuperRegion->getKind() == MemRegion::ElementRegionKind) {
+        SuperRegion = cast<SubRegion>(SuperRegion)->getSuperRegion();
+        continue;
+      }
+
+      break;
+    }
+
+    const DeclRegion *TheElementDeclRegion = SuperRegion->getAs<DeclRegion>();
+    if (!TheElementDeclRegion)
+      return false;
+
+    const DeclRegion *BaseDeclRegion = R->getBaseRegion()->getAs<DeclRegion>();
+    if (!BaseDeclRegion)
+      return false;
+
+    unsigned BaseRegionAlign = 0;
+    // We must use alignment TheElementDeclRegion if it has its own alignment
+    // specifier
+    if (TheElementDeclRegion->getDecl()->getMaxAlignment())
+      BaseRegionAlign = getStorageAlign(C, TheElementDeclRegion->getDecl());
+    else
+      BaseRegionAlign = getStorageAlign(C, BaseDeclRegion->getDecl());
+
+    if (AllocatedTAlign > BaseRegionAlign) {
+      emitBadAlignReport(P, C, AllocatedTAlign, BaseRegionAlign);
+      return false;
+    }
+
+    return true;
+  };
+
+  auto CheckElementRegionOffset = [this, R, &C, P, AllocatedTAlign]() -> void {
+    RegionOffset TheOffsetRegion = R->getAsOffset();
+    if (TheOffsetRegion.hasSymbolicOffset())
+      return;
+
+    unsigned Offset =
+        TheOffsetRegion.getOffset() / C.getASTContext().getCharWidth();
+    unsigned AddressAlign = Offset % AllocatedTAlign;
+    if (AddressAlign != 0) {
+      emitBadAlignReport(P, C, AllocatedTAlign, AddressAlign);
+      return;
+    }
+  };
+
+  if (IsBaseRegionAlignedProperly()) {
+    CheckElementRegionOffset();
+  }
+}
+
+void PlacementNewChecker::checkFieldRegionAlign(
+    const FieldRegion *R, CheckerContext &C, const Expr *P,
+    unsigned AllocatedTAlign) const {
+  const MemRegion *BaseRegion = R->getBaseRegion();
+  if (!BaseRegion)
+    return;
+
+  if (const VarRegion *TheVarRegion = BaseRegion->getAs<VarRegion>()) {
+    const VarDecl *TheVarDecl = TheVarRegion->getDecl();
+
+    unsigned StorageTAlign = getStorageAlign(C, TheVarDecl);
+    if (AllocatedTAlign > StorageTAlign) {
+      emitBadAlignReport(P, C, AllocatedTAlign, StorageTAlign);
+
+      return;
+    }
+
+    // We've checked type align but, unless FieldRegion
+    // offset is zero, we also need to check its own
+    // align.
+    RegionOffset Offset = R->getAsOffset();
+    if (Offset.hasSymbolicOffset())
       return;
-    }
-  }
+
+    int64_t OffsetValue = Offset.getOffset() / C.getASTContext().getCharWidth();
+    unsigned AddressAlign = OffsetValue % AllocatedTAlign;
+    if (AddressAlign != 0)
+      emitBadAlignReport(P, C, AllocatedTAlign, AddressAlign);
+  }
+
+void PlacementNewChecker::checkVarRegionAlign(const VarRegion *R,
+                                              CheckerContext &C, const Expr *P,
+                                              unsigned AllocatedTAlign) const {
+  const VarDecl *TheVarDecl = R->getDecl();
+  unsigned StorageTAlign = getStorageAlign(C, TheVarDecl);
+  if (AllocatedTAlign > StorageTAlign)
+    emitBadAlignReport(P, C, AllocatedTAlign, StorageTAlign);
+}
+
+bool PlacementNewChecker::checkPlaceIsAlignedProperly(const CXXNewExpr *NE,
+                                                      CheckerContext &C) const {
+  const Expr *Place = NE->getPlacementArg(0);
+
+  QualType AllocatedT = NE->getAllocatedType();
+  unsigned AllocatedTAlign = C.getASTContext().getTypeAlign(AllocatedT) /
+                             C.getASTContext().getCharWidth();
+
+  SVal PlaceVal = C.getSVal(Place);
+  if (const MemRegion *MRegion = PlaceVal.getAsRegion()) {
+    if (const ElementRegion *TheElementRegion = MRegion->getAs<ElementRegion>())
+      checkElementRegionAlign(TheElementRegion, C, Place, AllocatedTAlign);
+    else if (const FieldRegion *TheFieldRegion = MRegion->getAs<FieldRegion>())
+      checkFieldRegionAlign(TheFieldRegion, C, Place, AllocatedTAlign);
+    else if (const VarRegion *TheVarRegion = MRegion->getAs<VarRegion>())
+      checkVarRegionAlign(TheVarRegion, C, Place, AllocatedTAlign);
+  }
+
+  return true;
+
+void PlacementNewChecker::checkPreStmt(const CXXNewExpr *NE,
+                                       CheckerContext &C) const {
+  // Check only the default placement new.
+  if (!NE->getOperatorNew()->isReservedGlobalPlacementOperator())
+    return;
+
+  if (NE->getNumPlacementArgs() == 0)
+    return;
+
+  if (!checkPlaceCapacityIsSufficient(NE, C))
+    return;
+
+  checkPlaceIsAlignedProperly(NE, C);
 }
 
 void ento::registerPlacementNewChecker(CheckerManager &mgr) {
   mgr.registerChecker<PlacementNewChecker>();
 }
 
 bool ento::shouldRegisterPlacementNewChecker(const CheckerManager &mgr) {
   return true;
 }

clang/test/Analysis/placement-new.cpp

struct Derived : Base {
   int y;
 };
 void f() {
   Base b;                           // expected-note {{'b' initialized here}}
   Derived *dp = ::new (&b) Derived; // expected-warning{{Storage provided to placement new is only 2 bytes, whereas the allocated type requires 8 bytes}} expected-note 1 {{}}
   (void)dp;
 }
 } // namespace testHierarchy
+
+namespace testArrayTypesAllocation {
+void f1() {
+  struct S {
+    short a;
+  };
+
+  // bad (not enough space).
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer1[sizeof(S) * N]; // expected-note {{'buffer1' initialized here}}
+  ::new (buffer1) S[N];                            // expected-warning{{Storage provided to placement new is only 64 bytes, whereas the allocated array type requires more space for internal needs}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct S {
+    short a;
+  };
+
+  // maybe ok but we need to warn.
+  const unsigned N = 32;
+  alignas(S) unsigned char buffer2[sizeof(S) * N + sizeof(int)]; // expected-note {{'buffer2' initialized here}}
+  ::new (buffer2) S[N];                                          // expected-warning{{68 bytes is possibly not enough for array allocation which requires 64 bytes. Current overhead requires the size of 4 bytes}} expected-note 1 {{}}
+}
+} // namespace testArrayTypesAllocation
+
+namespace testStructAlign {
+void f1() {
+  struct X {
+    char a[9];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to char).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f2() {
+  struct X {
+    char a;
+    char b;
+    long c;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f3() {
+  struct X {
+    char a;
+    char b;
+    long c;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to long but field 'b' is aligned to 1 because of its offset)
+  ::new (&Xi.b) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f4() {
+  struct X {
+    char a;
+    struct alignas(alignof(short)) Y {
+      char b;
+      char c;
+    } y;
+    long d;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. 'b' is aligned to short
+  ::new (&Xi.y.b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f5() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  ::new (&b) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f6() {
+  short b[10]; // expected-note {{'b' initialized here}}
+
+  // bad (same as previous but checks ElementRegion case)
+  ::new (&b[0]) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f7() {
+  alignas(alignof(long)) short b[10];
+
+  // ok. aligned to long(ok). offset 4*2(ok)
+  ::new (&b[4]) long;
+}
+
+void f8() {
+  alignas(alignof(long)) short b[10]; // expected-note {{'b' initialized here}}
+
+  // ok. aligned to long(ok). offset 3*2(ok)
+  ::new (&b[3]) long; // expected-warning{{Storage type is aligned to 6 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f9() {
+  struct X {
+    char a;
+    alignas(alignof(long)) char b[20];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok. aligned to long(ok). offset 8*1(ok)
+  ::new (&Xi.b[8]) long;
+
+  // bad. aligned to long(ok). offset 1*1(ok)
+  ::new (&Xi.b[1]) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f10() {
+  struct X {
+    char a[8];
+    alignas(2) char b;
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad (struct X is aligned to 2).
+  ::new (&Xi.a) long; // expected-warning{{Storage type is aligned to 2 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void f11() {
+  struct X {
+    char a;
+    char b;
+    struct Y {
+      long c;
+    } d;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void f12() {
+  struct alignas(alignof(long)) X {
+    char a;
+    char b;
+  } Xi;
+
+  // ok (struct X is aligned to long).
+  ::new (&Xi.a) long;
+}
+
+void test13() {
+  struct Y {
+    char a[10];
+  };
+
+  struct X {
+    Y b[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. X,A are aligned to 'char'
+  ::new (&Xi.b[0].a) long;  // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void test14() {
+  struct Y {
+    char a[10];
+  };
+
+  struct alignas(alignof(long)) X {
+    Y b[10];
+  } Xi;
+
+  // ok. X is aligned to 'long' and field 'a' goes with zero offset
+  ::new (&Xi.b[0].a) long;
+}
+
+void test15() {
+  struct alignas(alignof(long)) Y {
+    char a[10];
+  };
+
+  struct X {
+    Y b[10];
+  } Xi;
+
+  // ok. X is aligned to 'long' because it contains struct 'Y' which is aligned to 'long'
+  ::new (&Xi.b[0].a) long;
+}
+
+void test16() {
+  struct alignas(alignof(long)) Y {
+    char p;
+    char a[10];
+  };
+
+  struct X {
+    Y b[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. aligned to long(ok). offset 1(bad)
+  ::new (&Xi.b[0].a) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void test17() {
+  struct alignas(alignof(long)) Y {
+    char p;
+    char a[10];
+  };
+
+  struct X {
+    Y b[10];
+  } Xi;
+
+  // ok. aligned to long(ok). offset 1+7*1(ok)
+  ::new (&Xi.b[0].a[7]) long;
+}
+
+void test18() {
+  struct Y {
+    char p;
+    alignas(alignof(long)) char a[10];
+  };
+
+  struct X {
+    Y b[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok. aligned to long(ok). offset 8*1(ok)
+  ::new (&Xi.b[0].a[8]) long;
+
+  // bad. aligned to long(ok). offset 1(bad)
+  ::new (&Xi.b[0].a[1]) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void test19() {
+  struct Z {
+    char p;
+    char c[10];
+  };
+
+  struct Y {
+    char p;
+    Z b[10];
+  };
+
+  struct X {
+    Y a[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // bad. all structures X,Y,Z are aligned to char
+  ::new (&Xi.a[1].b[1].c) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void test20() {
+  struct Z {
+    char p;
+    alignas(alignof(long)) char c[10];
+  };
+
+  struct Y {
+    char p;
+    Z b[10];
+  };
+
+  struct X {
+    Y a[10];
+  } Xi;
+
+  // ok. field 'c' is aligned to 'long'
+  ::new (&Xi.a[1].b[1].c) long;
+}
+
+void test21() {
+  struct Z {
+    char p;
+    char c[10];
+  };
+
+  struct Y {
+    char p;
+    Z b[10];
+  };
+
+  struct alignas(alignof(long)) X {
+    Y a[10];
+  } Xi; // expected-note {{'Xi' initialized here}}
+
+  // ok. aligned to long(ok). offset 1+7*1(ok)
+  ::new (&Xi.a[0].b[0].c[7]) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+void test22() {
+  struct alignas(alignof(long)) Y {
+    char p;
+    char a[10][10];
+  };
+
+  struct X {
+    Y b[10];
+  } Xi;  // expected-note {{'Xi' initialized here}}
+
+  // ok. aligned to long(ok). offset ok. 1(field 'a' offset) + 0*10(index '0' * first dimension size '10') + 7*1(index '7')
+  ::new (&Xi.b[0].a[0][7]) long;
+
+  // ok. aligned to long(ok). offset ok. 1(field 'a' offset) + 1*10(index '1' * first dimension size '10') + 5*1(index '5')
+  ::new (&Xi.b[0].a[1][5]) long;
+
+  // bad. aligned to long(ok). offset ok. 1(field 'a' offset) + 1*10(index '1' * first dimension size '10') + 6*1(index '5')
+  ::new (&Xi.b[0].a[1][6]) long; // expected-warning{{Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes}} expected-note 1 {{}}
+}
+
+} // namespace testStructAlign