[Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators

[ahatanak]

Summary

This improves upon https://reviews.llvm.org/D21099, which taught -Wcast-align to look at the aligned attribute of variables. I added a function that computes a more accurate alignment to ExprConstant.cpp.

rdar://problem/59242343

[rjmccall]

clang/include/clang/AST/Expr.h

710 ↗

(On Diff #259739)

Nothing about the name here suggests that it only works on expressions of pointer type. Maybe call it getPresumedAlignmentOfPointer or something and then make it also consider the type alignment?

clang/lib/AST/ExprConstant.cpp

14844 ↗

(On Diff #259739)

Does this do the right thing if getDeclAlign returns 0, or can that never happen?

[ahatanak]

clang/lib/AST/ExprConstant.cpp

14844 ↗

(On Diff #259739)

I looked at getDeclAlign and, as far as I can tell, it returns an alignment that is larger than 0.

[ahatanak]

The method has to compute the correct alignment for variables captured by lambdas or blocks too.

[ahatanak]

Define two helper functions in Sema that compute the alignment of a VarDecl and a constant offset from it and use them instead of the classes for evaluating lvalue and pointer expressions in ExprConstant.cpp.

Using the classes in ExprConstant.cpp to compute an expression alignment as I did in the previous patch is probably not a good idea since they are for evaluating constant expressions. They don't return the lvalue base variables and offsets in some cases ( for example, (A *)&m_ref in the test case) and using them for this purpose can make it harder to make changes to the way constant expressions are evaluated. The current patch is far from perfect as it misses many cases that could be detected by the classes in ExprConstant.cpp, but is at least an improvement over what we have now.

I was also thinking about fixing the alignment computation of captured variables, but I think I should do that separately in a follow-up as it might not be trivial. It will probably require looking up the captured variables in all the enclosing CapturingScopeInfos.

[rjmccall]

clang/lib/Sema/SemaChecking.cpp

13161

I think an Optional<std::pair> would be a more self-documenting type here, and you'd be able to test the result in an if.

13177

You still need to conservatively adjust the alignment when the index isn't a constant expression. You can do that by reducing the base result to a simple alignment, adding the array element size as an offset, and then reducing again.

13186

Reference variables don't have to have initializers; e.g. they can be extern.

13206

Derived-to-base conversions.

13211

This all seems like it'd be simpler if you just checked for the pointer/integer operands and then swapped if necessary.

13215

Derived-to-base conversions.

[ahatanak]

clang/lib/Sema/SemaChecking.cpp

13206

Does this include cases where bases are virtual? I think that would be more complex as it requires tracking the most-derived object. Do you think it's important to handle virtual bases in this patch or should it be done separately in a follow-up?

[rjmccall]

clang/lib/Sema/SemaChecking.cpp

13206

You should handle virtual bases, but it's okay to assume they're not within known-complete objects; i.e. you just need to reset the alignment to the alignment of the base.

[ahatanak]

Address review comments. Handle derived-to-base cast expressions and array subscript expressions that don't have constant indices.

[rjmccall]

clang/lib/Sema/SemaChecking.cpp

13122

Oh, this — and all the other places that do presumed alignment based on a pointee type — needs a special case for C++ records with virtual bases, because you need to get its presumed alignment as a base sub-object, not its presumed alignment as a complete object, which is what getTypeAlignInChars will return. The right way to merge that information is to get the normal alignment — which may be lower than expected if there's a typedef in play — and then min that with the base sub-object alignment.

13140

You should add a case for unary *.

13168

I don't think we guarantee that these will be no-op casts; all the explicit cast nodes should be handled the same as ImplicitCastExpr, in both of these functions.

13168

This should be handled the same as array subscripting. Maybe you can extract a helper for that that's given a pointer expression, an integer expression, and a flag indicating whether it's a subtraction?

13181

You should look through comma expressions.

13197

Can you do this with just a type analysis on the operands instead of eagerly doing these calls?

13211

There's an llvm::None which has this effect.

[ahatanak]

Address most of the review comments.

clang/lib/Sema/SemaChecking.cpp

13122

I think the base sub-object alignment in this case is the NonVirtualAlignment of the class (the CXXRecordDecl of Base in this function), but it's not clear to me what the normal alignment is. I don't think it's what Ctx.getTypeAlignInChars(Base->getType()) returns, is it? I see CodeGenModule::getDynamicOffsetAlignment seems to be doing what you are suggesting, but I'm not sure whether that's what we should be doing here. It looks like it's comparing the alignment of the derived class and the non-virtual alignment of the base class.

13168

The offset computation was wrong when the element type wasn't a char. The bug is fixed in the helper function.

[rjmccall]

Other changes look good to me, thanks.

clang/lib/Sema/SemaChecking.cpp

13122

The base sub-object alignment is the class's non-virtual alignment, right.

By the normal alignment, I mean the alignment you're starting from for the outer object — if that's less than the base's alignment, the base may be misaligned as well. For the non-base-conversion case, that's probably not necessary to consider.

[ahatanak]

Fix alignment computation of virtual bases.

[ahatanak]

clang/lib/Sema/SemaChecking.cpp

13122

Let me know if the comment explaining why we are taking the minimum makes sense. I added a test case for this too (&t10).

[rjmccall]

LGTM.

clang/lib/Sema/SemaChecking.cpp

13122

Looks good.

clang/lib/Sema/SemaChecking.cpp

 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
 #include "clang/AST/FormatString.h"
 #include "clang/AST/NSAPI.h"
 #include "clang/AST/NonTrivialTypeVisitor.h"
 #include "clang/AST/OperationKinds.h"
+#include "clang/AST/RecordLayout.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/TemplateBase.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/AST/UnresolvedSet.h"
 #include "clang/Basic/AddressSpaces.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/Diagnostic.h"
if (LangOpts.CPlusPlus && !Param->isInvalidDecl()) {
                                      RD, /*DeclIsField*/ false);
       }
     }
   }
 
   return HasInvalidParm;
 }
 
-/// A helper function to get the alignment of a Decl referred to by DeclRefExpr
-/// or MemberExpr.
-static CharUnits getDeclAlign(Expr *E, CharUnits TypeAlign,
-                              ASTContext &Context) {
-  if (const auto *DRE = dyn_cast<DeclRefExpr>(E))
-    return Context.getDeclAlign(DRE->getDecl());
+Optional<std::pair<CharUnits, CharUnits>>
+static getBaseAlignmentAndOffsetFromPtr(const Expr *E, ASTContext &Ctx);
+
+/// Compute the alignment and offset of the base class object given the
+/// derived-to-base cast expression and the alignment and offset of the derived
+/// class object.
+static std::pair<CharUnits, CharUnits>
+getDerivedToBaseAlignmentAndOffset(const CastExpr *CE, QualType DerivedType,
+                                   CharUnits BaseAlignment, CharUnits Offset,
+                                   ASTContext &Ctx) {
+  for (auto PathI = CE->path_begin(), PathE = CE->path_end(); PathI != PathE;
+       ++PathI) {
+    const CXXBaseSpecifier *Base = *PathI;
+    const CXXRecordDecl *BaseDecl = Base->getType()->getAsCXXRecordDecl();
+    if (Base->isVirtual()) {
+      // The complete object may have a lower alignment than the non-virtual
+      // alignment of the base, in which case the base may be misaligned. Choose
+      // the smaller of the non-virtual alignment and BaseAlignment, which is a
+      // conservative lower bound of the complete object alignment.
+      CharUnits NonVirtualAlignment =
+          Ctx.getASTRecordLayout(BaseDecl).getNonVirtualAlignment();
+      BaseAlignment = std::min(BaseAlignment, NonVirtualAlignment);
+      Offset = CharUnits::Zero();
+    } else {
+      const ASTRecordLayout &RL =
+          Ctx.getASTRecordLayout(DerivedType->getAsCXXRecordDecl());
+      Offset += RL.getBaseClassOffset(BaseDecl);
+    }
+    DerivedType = Base->getType();
+  }
+
+  return std::make_pair(BaseAlignment, Offset);
+}
+
+/// Compute the alignment and offset of a binary additive operator.
+static Optional<std::pair<CharUnits, CharUnits>>
+getAlignmentAndOffsetFromBinAddOrSub(const Expr *PtrE, const Expr *IntE,
+                                     bool IsSub, ASTContext &Ctx) {
+  QualType PointeeType = PtrE->getType()->getPointeeType();
+
+  if (!PointeeType->isConstantSizeType())
+    return llvm::None;
+
+  auto P = getBaseAlignmentAndOffsetFromPtr(PtrE, Ctx);
+
+  if (!P)
+    return llvm::None;
+
+  llvm::APSInt IdxRes;
+  CharUnits EltSize = Ctx.getTypeSizeInChars(PointeeType);
+  if (IntE->isIntegerConstantExpr(IdxRes, Ctx)) {
+    CharUnits Offset = EltSize * IdxRes.getExtValue();
+    if (IsSub)
+      Offset = -Offset;
+    return std::make_pair(P->first, P->second + Offset);
+  }
+
+  // If the integer expression isn't a constant expression, compute the lower
+  // bound of the alignment using the alignment and offset of the pointer
+  // expression and the element size.
+  return std::make_pair(
+      P->first.alignmentAtOffset(P->second).alignmentAtOffset(EltSize),
+      CharUnits::Zero());
+}
+
+/// This helper function takes an lvalue expression and returns the alignment of
+/// a VarDecl and a constant offset from the VarDecl.
+Optional<std::pair<CharUnits, CharUnits>>
+static getBaseAlignmentAndOffsetFromLValue(const Expr *E, ASTContext &Ctx) {
+  E = E->IgnoreParens();
+  switch (E->getStmtClass()) {
+  default:
+    break;
+  case Stmt::CStyleCastExprClass:
+  case Stmt::CXXStaticCastExprClass:
+  case Stmt::ImplicitCastExprClass: {
+    auto *CE = cast<CastExpr>(E);
+    const Expr *From = CE->getSubExpr();
+    switch (CE->getCastKind()) {
+    default:
+      break;
+    case CK_NoOp:
+      return getBaseAlignmentAndOffsetFromLValue(From, Ctx);
+    case CK_UncheckedDerivedToBase:
+    case CK_DerivedToBase: {
+      auto P = getBaseAlignmentAndOffsetFromLValue(From, Ctx);
+      if (!P)
+        break;
+      return getDerivedToBaseAlignmentAndOffset(CE, From->getType(), P->first,
+                                                P->second, Ctx);
+    }
+    }
+    break;
+  }
+  case Stmt::ArraySubscriptExprClass: {
+    auto *ASE = cast<ArraySubscriptExpr>(E);
+    return getAlignmentAndOffsetFromBinAddOrSub(ASE->getBase(), ASE->getIdx(),
+                                                false, Ctx);
+  }
+  case Stmt::DeclRefExprClass: {
+    if (auto *VD = dyn_cast<VarDecl>(cast<DeclRefExpr>(E)->getDecl())) {
+      // FIXME: If VD is captured by copy or is an escaping __block variable,
+      // use the alignment of VD's type.
+      if (!VD->getType()->isReferenceType())
+        return std::make_pair(Ctx.getDeclAlign(VD), CharUnits::Zero());
+      if (VD->hasInit())
+        return getBaseAlignmentAndOffsetFromLValue(VD->getInit(), Ctx);
+    }
+    break;
+  }
+  case Stmt::MemberExprClass: {
+    auto *ME = cast<MemberExpr>(E);
+    if (ME->isArrow())
+      break;
+    auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl());
+    if (!FD || FD->getType()->isReferenceType())
+      break;
+    auto P = getBaseAlignmentAndOffsetFromLValue(ME->getBase(), Ctx);
+    if (!P)
+      break;
+    const ASTRecordLayout &Layout = Ctx.getASTRecordLayout(FD->getParent());
+    uint64_t Offset = Layout.getFieldOffset(FD->getFieldIndex());
+    return std::make_pair(P->first,
+                          P->second + CharUnits::fromQuantity(Offset));
+  }
+  case Stmt::UnaryOperatorClass: {
+    auto *UO = cast<UnaryOperator>(E);
+    switch (UO->getOpcode()) {
+    default:
+      break;
+    case UO_Deref:
+      return getBaseAlignmentAndOffsetFromPtr(UO->getSubExpr(), Ctx);
+    }
+    break;
+  }
+  case Stmt::BinaryOperatorClass: {
+    auto *BO = cast<BinaryOperator>(E);
+    auto Opcode = BO->getOpcode();
+    switch (Opcode) {
+    default:
+      break;
+    case BO_Comma:
+      return getBaseAlignmentAndOffsetFromLValue(BO->getRHS(), Ctx);
+    }
+    break;
+  }
+  }
+  return llvm::None;
+}
 
-  if (const auto *ME = dyn_cast<MemberExpr>(E))
-    return Context.getDeclAlign(ME->getMemberDecl());
+/// This helper function takes a pointer expression and returns the alignment of
+/// a VarDecl and a constant offset from the VarDecl.
+Optional<std::pair<CharUnits, CharUnits>>
+static getBaseAlignmentAndOffsetFromPtr(const Expr *E, ASTContext &Ctx) {
+  E = E->IgnoreParens();
+  switch (E->getStmtClass()) {
+  default:
+    break;
+  case Stmt::CStyleCastExprClass:
+  case Stmt::CXXStaticCastExprClass:
+  case Stmt::ImplicitCastExprClass: {
+    auto *CE = cast<CastExpr>(E);
+    const Expr *From = CE->getSubExpr();
+    switch (CE->getCastKind()) {
+    default:
+      break;
+    case CK_NoOp:
+      return getBaseAlignmentAndOffsetFromPtr(From, Ctx);
+    case CK_ArrayToPointerDecay:
+      return getBaseAlignmentAndOffsetFromLValue(From, Ctx);
+    case CK_UncheckedDerivedToBase:
+    case CK_DerivedToBase: {
+      auto P = getBaseAlignmentAndOffsetFromPtr(From, Ctx);
+      if (!P)
+        break;
+      return getDerivedToBaseAlignmentAndOffset(
+          CE, From->getType()->getPointeeType(), P->first, P->second, Ctx);
+    }
+    }
+    break;
+  }
+  case Stmt::UnaryOperatorClass: {
+    auto *UO = cast<UnaryOperator>(E);
+    if (UO->getOpcode() == UO_AddrOf)
+      return getBaseAlignmentAndOffsetFromLValue(UO->getSubExpr(), Ctx);
+    break;
+  }
+  case Stmt::BinaryOperatorClass: {
+    auto *BO = cast<BinaryOperator>(E);
+    auto Opcode = BO->getOpcode();
+    switch (Opcode) {
+    default:
+      break;
+    case BO_Add:
+    case BO_Sub: {
+      const Expr *LHS = BO->getLHS(), *RHS = BO->getRHS();
+      if (Opcode == BO_Add && !RHS->getType()->isIntegralOrEnumerationType())
+        std::swap(LHS, RHS);
+      return getAlignmentAndOffsetFromBinAddOrSub(LHS, RHS, Opcode == BO_Sub,
+                                                  Ctx);
+    }
+    case BO_Comma:
+      return getBaseAlignmentAndOffsetFromPtr(BO->getRHS(), Ctx);
+    }
+    break;
+  }
+  }
+  return llvm::None;
+}
 
-  return TypeAlign;
+static CharUnits getPresumedAlignmentOfPointer(const Expr *E, Sema &S) {
+  // See if we can compute the alignment of a VarDecl and an offset from it.
+  Optional<std::pair<CharUnits, CharUnits>> P =
+      getBaseAlignmentAndOffsetFromPtr(E, S.Context);
+
+  if (P)
+    return P->first.alignmentAtOffset(P->second);
+
+  // If that failed, return the type's alignment.
+  return S.Context.getTypeAlignInChars(E->getType()->getPointeeType());
 }
 
 /// CheckCastAlign - Implements -Wcast-align, which warns when a
 /// pointer cast increases the alignment requirements.
 void Sema::CheckCastAlign(Expr *Op, QualType T, SourceRange TRange) {
   // This is actually a lot of work to potentially be doing on every
   // cast; don't do it if we're ignoring -Wcast_align (as is the default).
   if (getDiagnostics().isIgnored(diag::warn_cast_align, TRange.getBegin()))
void Sema::CheckCastAlign(Expr *Op, QualType T, SourceRange TRange) {
   QualType SrcPointee = SrcPtr->getPointeeType();
 
   // Whitelist casts from cv void*.  We already implicitly
   // whitelisted casts to cv void*, since they have alignment 1.
   // Also whitelist casts involving incomplete types, which implicitly
   // includes 'void'.
   if (SrcPointee->isIncompleteType()) return;
 
-  CharUnits SrcAlign = Context.getTypeAlignInChars(SrcPointee);
-
-  if (auto *CE = dyn_cast<CastExpr>(Op)) {
-    if (CE->getCastKind() == CK_ArrayToPointerDecay)
-      SrcAlign = getDeclAlign(CE->getSubExpr(), SrcAlign, Context);
-  } else if (auto *UO = dyn_cast<UnaryOperator>(Op)) {
-    if (UO->getOpcode() == UO_AddrOf)
-      SrcAlign = getDeclAlign(UO->getSubExpr(), SrcAlign, Context);
-  }
+  CharUnits SrcAlign = getPresumedAlignmentOfPointer(Op, *this);
 
   if (SrcAlign >= DestAlign) return;
 
   Diag(TRange.getBegin(), diag::warn_cast_align)
     << Op->getType() << T
     << static_cast<unsigned>(SrcAlign.getQuantity())
     << static_cast<unsigned>(DestAlign.getQuantity())
     << TRange << Op->getSourceRange();

clang/test/SemaCXX/warn-cast-align.cpp

-// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -Wcast-align -verify %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -fsyntax-only -Wno-unused-value -Wcast-align -verify %s
 
 // Simple casts.
 void test0(char *P) {
   char *a; short *b; int *c;
 
   a = (char*) P;
   a = static_cast<char*>(P);
   a = reinterpret_cast<char*>(P);
void test1(void *P) {
   b = ShortPtr(P);
 
   c = (int*) P;
   c = static_cast<int*>(P);
   c = reinterpret_cast<int*>(P);
   typedef int *IntPtr;
   c = IntPtr(P);
 }
+
+struct __attribute__((aligned(16))) A {
+  char m0[16];
+  char m1[16];
+};
+
+struct B0 {
+  char m0[16];
+};
+
+struct B1 {
+  char m0[16];
+};
+
+struct C {
+  A &m0;
+  B0 &m1;
+  A m2;
+};
+
+struct __attribute__((aligned(16))) D0 : B0, B1 {
+};
+
+struct __attribute__((aligned(16))) D1 : virtual B0 {
+};
+
+struct B2 {
+  char m0[8];
+};
+
+struct B3 {
+  char m0[8];
+};
+
+struct B4 {
+  char m0[8];
+};
+
+struct D2 : B2, B3 {
+};
+
+struct __attribute__((aligned(16))) D3 : B4, D2 {
+};
+
+struct __attribute__((aligned(16))) D4 : virtual D2 {
+};
+
+struct D5 : virtual D0 {
+  char m0[16];
+};
+
+struct D6 : virtual D5 {
+};
+
+struct D7 : virtual D3 {
+};
+
+void test2(int n, A *a2) {
+  __attribute__((aligned(16))) char m[sizeof(A) * 2];
+  char(&m_ref)[sizeof(A) * 2] = m;
+  extern char(&m_ref_noinit)[sizeof(A) * 2];
+  __attribute__((aligned(16))) char vararray[10][n];
+  A t0;
+  B0 t1;
+  C t2 = {.m0 = t0, .m1 = t1};
+  __attribute__((aligned(16))) char t3[5][5][5];
+  __attribute__((aligned(16))) char t4[4][16];
+  D0 t5;
+  D1 t6;
+  D3 t7;
+  D4 t8;
+  D6 t9;
+  __attribute__((aligned(1))) D7 t10;
+
+  A *a;
+  a = (A *)&m;
+  a = (A *)(m + sizeof(A));
+  a = (A *)(sizeof(A) + m);
+  a = (A *)((sizeof(A) * 2 + m) - sizeof(A));
+  a = (A *)((sizeof(A) * 2 + m) - 1); // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)(m + 1);                   // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)(1 + m);                   // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)(m + n);                   // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)&*&m[sizeof(A)];
+  a = (A *)(0, 0, &m[sizeof(A)]);
+  a = (A *)&(0, 0, *&m[sizeof(A)]);
+  a = (A *)&m[n]; // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)&m_ref;
+  a = (A *)&m_ref_noinit;        // expected-warning {{cast from 'char (*)[64]' to 'A *'}}
+  a = (A *)(&vararray[4][0]);    // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)(a2->m0 + sizeof(A)); // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)(&t2.m0);
+  a = (A *)(&t2.m1); // expected-warning {{cast from 'B0 *' to 'A *'}}
+  a = (A *)(&t2.m2);
+  a = (A *)(t2.m2.m1);
+  a = (A *)(&t3[3][3][0]); // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)(&t3[2][2][4]);
+  a = (A *)(&t3[0][n][0]); // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)&t4[n][0];
+  a = (A *)&t4[n][1]; // expected-warning {{cast from 'char *' to 'A *'}}
+  a = (A *)(t4 + 1);
+  a = (A *)(t4 + n);
+  a = (A *)(static_cast<B1 *>(&t5));
+  a = (A *)(&(static_cast<B1 &>(t5)));
+  a = (A *)(static_cast<B0 *>(&t6)); // expected-warning {{cast from 'B0 *' to 'A *'}}
+  a = (A *)(static_cast<B2 *>(&t7)); // expected-warning {{cast from 'B2 *' to 'A *'}}
+  a = (A *)(static_cast<B3 *>(&t7));
+  a = (A *)(static_cast<B2 *>(&t8));  // expected-warning {{cast from 'B2 *' to 'A *'}}
+  a = (A *)(static_cast<B3 *>(&t8));  // expected-warning {{cast from 'B3 *' to 'A *'}}
+  a = (A *)(static_cast<D5 *>(&t9));  // expected-warning {{cast from 'D5 *' to 'A *'}}
+  a = (A *)(static_cast<D3 *>(&t10)); // expected-warning {{cast from 'D3 *' to 'A *'}}
+}