Fix bug 26547 - alignof should return ABI alignment, not preferred alignment

[• ubsan]

Summary

• Add UETT_PreferredAlignOf to account for the difference between __alignof and alignof
• AlignOfType now returns ABI alignment instead of preferred alignment iff clang-abi-compat > 7, and one uses _Alignof or alignof

bug link: https://bugs.llvm.org/show_bug.cgi?id=26547

[rsmith]

Thanks, this generally looks good, but it needs tests. For _Alignof, test/Sema/align-x86.c` would be a reasonable place for the test; for C++ alignof, I don't see a suitable existing test file, but you could add one to test/SemaCXX, perhaps based on test/Sema/align-x86.c.

include/clang/Basic/LangOptions.h

128

Clang 7 was branched from SVN r338536.

131

Nit: Capitalize "See", add period. But I think you can actually just drop this line: we don't really need to justify why we now follow the rules of the relevant language standards.

132

You will also need to update the code in lib/Frontend/CompilerInvocation.cpp to use this new enumerator (search for Ver6 to find the code you need to change).

284

Please avoid reflowing unrelated lines; it makes future "svn annotate"s less useful.

include/clang/Basic/TypeTraits.h

99–104

Nit: comments should start with a capital letter and end with a period. Maybe also make these documentation comments (use ///)?

lib/AST/ExprConstant.cpp

5960

LLVM / Clang convention is to start variable names with a capital letter.

[• ubsan]

add tests

fix nits

[rsmith]

Please extend the tests to check that the -fclang-abi-compat=7 switch undoes your change here.

One other comment, and otherwise this looks good. Will you need someone to commit this for you?

test/SemaCXX/align-x86.cpp

4–5 ↗

(On Diff #169503)

It's not OK to include standard library headers here -- there's no guarantee that anything sensible will be on the include path, and we want our tests to be self-contained. (Feel free to define your own mini-std::complex in this test.)

[• ubsan]

I'm having real difficulty with this - I get really odd errors in seemingly unrelated tests when I change things.

[rsmith]

In D53207#1268535, @ubsan wrote:

I'm having real difficulty with this - I get really odd errors in seemingly unrelated tests when I change things.

Is it possible you have LLVM and Clang checked out at different revisions? That's a common source of weird behavior, particularly in CodeGen tests.

[• ubsan]

Is it possible you have LLVM and Clang checked out at different revisions? That's a common source of weird behavior, particularly in CodeGen tests.

Unfortunately no - when I don't specifically don't set the type trait kind of __alignof to UETT_PreferredAlignOf, and remove the code in AlignOfType to differentiate between PreferredAlignOf and AlignOf, all the tests pass again.

[rsmith]

In D53207#1273727, @ubsan wrote:

when I don't specifically don't set the type trait kind of __alignof to UETT_PreferredAlignOf, and remove the code in AlignOfType to differentiate between PreferredAlignOf and AlignOf, all the tests pass again.

You've extended UnaryExprOrTypeTrait from having 4 enumerators to 5, which means it no longer fits in a 2-bit bitfield. Stmt::UnaryExprOrTypeTraitExprBitfields::Kind needs to be made wider.

[• ubsan]

... Yup, that'd do it. Thanks Richard!

[• ubsan]

Finish doing the thing! all tests are passing, at least on the revision I'm running on :)

@rsmith please check the thing!

[rsmith]

Thanks! Can you also write something for the release notes (docs/ReleaseNotes.rst) describing the ABI change?

include/clang/Basic/DiagnosticSemaKinds.td

5478 ↗

(On Diff #170814)

Please factor out this repeated %select and use a %sub instead.

lib/Sema/SemaExpr.cpp

3772

Please reflow this to under 80 columns.

[• ubsan]

Add ABI breakage information and reflow

[• ubsan]

I don't actually know how to send this upstream, since it's been accepted... How should I proceed?

[rsmith]

In D53207#1276062, @ubsan wrote:

I don't actually know how to send this upstream, since it's been accepted... How should I proceed?

If you don't have an SVN account, you can ask someone who does to commit it for you. (In this case, I'll do it.)

A few minor nit first, though :)

docs/ReleaseNotes.rst

80 ↗

(On Diff #170994)

I don't think we need to say this. It could be set to 7 before, it just had no effect.

152 ↗

(On Diff #170994)

which -> that

include/clang/Basic/DiagnosticSemaKinds.td

5478 ↗

(On Diff #170814)

I think you may have missed this comment.

[• ubsan]

fix nits

[ldionne]

I'm surprised that nobody discussed the potential for ABI breakage of this change, which changes the result of the alignof operator. I'm trying to evaluate what to do in libc++ as a result of this change (now std::alignment_of does not match alignof) for PR39713, and I'm wondering whether this issue was considered here. I'm not saying this is a bad change, by the way, I'd simply like to understand what the implications are and find good arguments to convince me to follow the lead in libc++. Pinging @dexonsmith for awareness.

[• ubsan]

The ABI breakage was already there, in the difference between GCC and Clang

• if one compiles against libc++ with gcc, it is not compatible with things

that are compiled with clang. I frankly think that being ABI compatible
with gcc (and correct according to the standard) is far more important than
being ABI compatible with previous versions of clang - if people need the
ABI compatibility, they can and should use -fclang-abi-compat

include/clang/ASTMatchers/ASTMatchers.h

 /// alignof.
 inline internal::Matcher<Stmt> alignOfExpr(
     const internal::Matcher<UnaryExprOrTypeTraitExpr> &InnerMatcher) {
-  return stmt(unaryExprOrTypeTraitExpr(allOf(
-      ofKind(UETT_AlignOf), InnerMatcher)));
+  return stmt(unaryExprOrTypeTraitExpr(
+      allOf(anyOf(ofKind(UETT_AlignOf), ofKind(UETT_PreferredAlignOf)),
+            InnerMatcher)));
 }
 
 /// Same as unaryExprOrTypeTraitExpr, but only matching

include/clang/Basic/LangOptions.h

     /// whether we reuse base class tail padding in some ABIs.
     Ver6,
 
+    /// Attempt to be ABI-compatible with code generated by Clang 7.0.x
+    /// (SVN r344257). This causes alignof (C++) and _Alignof (C11) to be
+    /// compatible with __alignof (i.e., return the preferred alignment)
+    /// rather than returning the required alignment.
+    /// see https://bugs.llvm.org/show_bug.cgi?id=26547 for explanation
+    Ver7,
+
     /// Conform to the underlying platform's C and C++ ABIs as closely
     /// as we can.
     Latest

 /// Floating point control options
 class FPOptions {
 public:
-  FPOptions() : fp_contract(LangOptions::FPC_Off), 
-                fenv_access(LangOptions::FEA_Off) {}
+  FPOptions()
+      : fp_contract(LangOptions::FPC_Off), fenv_access(LangOptions::FEA_Off) {}
 
   // Used for serializing.
   explicit FPOptions(unsigned I)

   unsigned getInt() const { return fp_contract | (fenv_access << 2); }
 
 private:
-  /// Adjust BinaryOperator::FPFeatures to match the total bit-field size 
+  /// Adjust BinaryOperator::FPFeatures to match the total bit-field size
   /// of these two.
   unsigned fp_contract : 2;
   unsigned fenv_access : 1;

include/clang/Basic/TypeTraits.h

   /// Names for the "expression or type" traits.
   enum UnaryExprOrTypeTrait {
     UETT_SizeOf,
+    // used for GCC's __alignof,
+    UETT_PreferredAlignOf,
+    // used for C's _Alignof and C++'s alignof
+    // this distinction is important because __alignof
+    // returns preferred alignment
+    // _Alignof and alignof return the required alignment
     UETT_AlignOf,
     UETT_VecStep,
     UETT_OpenMPRequiredSimdAlign,

lib/AST/ASTDumper.cpp

   case UETT_SizeOf:
     OS << " sizeof";
     break;
+  case UETT_PreferredAlignOf:
+    OS << " __alignof";
+    break;
   case UETT_AlignOf:
     OS << " alignof";
     break;

lib/AST/Expr.cpp

 
   // Check to see if we are in the situation where alignof(decl) should be
   // dependent because decl's alignment is dependent.
-  if (ExprKind == UETT_AlignOf) {
+  if (ExprKind == UETT_AlignOf || ExprKind == UETT_PreferredAlignOf) {
     if (!isValueDependent() || !isInstantiationDependent()) {
       E = E->IgnoreParens();
 

lib/AST/ExprConstant.cpp

   return ExprEvaluatorBaseTy::VisitCastExpr(E);
 }
 
-static CharUnits GetAlignOfType(EvalInfo &Info, QualType T) {
+static CharUnits GetAlignOfType(EvalInfo &Info, QualType T,
+                                bool PreferredAlignOf) {
   // C++ [expr.alignof]p3:
   //     When alignof is applied to a reference type, the result is the
   //     alignment of the referenced type.
   if (const ReferenceType *Ref = T->getAs<ReferenceType>())
     T = Ref->getPointeeType();
 
-  // __alignof is defined to return the preferred alignment.
   if (T.getQualifiers().hasUnaligned())
     return CharUnits::One();
-  return Info.Ctx.toCharUnitsFromBits(
-    Info.Ctx.getPreferredTypeAlign(T.getTypePtr()));
+
+  const bool alignOfReturnsPreferred =
+      Info.Ctx.getLangOpts().getClangABICompat() <= LangOptions::ClangABI::Ver7;
+  // __alignof is defined to return the preferred alignment.
+  // before 8, clang returned the preferred alignment for alignof and _Alignof
+  // as well
+  if (PreferredAlignOf || alignOfReturnsPreferred)
+    return Info.Ctx.toCharUnitsFromBits(
+        Info.Ctx.getPreferredTypeAlign(T.getTypePtr()));
+  // alignof (C++) and _Alignof (C11) are defined to return the ABI alignment
+  // see https://bugs.llvm.org/show_bug.cgi?id=26547 for explanation
+  else
+    return Info.Ctx.getTypeAlignInChars(T.getTypePtr());
 }
 
-static CharUnits GetAlignOfExpr(EvalInfo &Info, const Expr *E) {
+static CharUnits GetAlignOfExpr(EvalInfo &Info, const Expr *E,
+                                bool PreferredAlignOf) {
   E = E->IgnoreParens();
 
   // The kinds of expressions that we have special-case logic here for

     return Info.Ctx.getDeclAlign(ME->getMemberDecl(),
                                  /*RefAsPointee*/true);
 
-  return GetAlignOfType(Info, E->getType());
+  return GetAlignOfType(Info, E->getType(), PreferredAlignOf);
 }
 
 // To be clear: this happily visits unsupported builtins. Better name welcomed.

         BaseAlignment = Info.Ctx.getDeclAlign(VD);
       } else {
         BaseAlignment =
-          GetAlignOfExpr(Info, OffsetResult.Base.get<const Expr*>());
+            GetAlignOfExpr(Info, OffsetResult.Base.get<const Expr *>(), false);
       }
 
       if (BaseAlignment < Align) {

 /// a result as the expression's type.
 bool IntExprEvaluator::VisitUnaryExprOrTypeTraitExpr(
                                     const UnaryExprOrTypeTraitExpr *E) {
-  switch(E->getKind()) {
+  auto const kind = E->getKind();
+  switch (kind) {
+  case UETT_PreferredAlignOf:
   case UETT_AlignOf: {
     if (E->isArgumentType())
-      return Success(GetAlignOfType(Info, E->getArgumentType()), E);
+      return Success(GetAlignOfType(Info, E->getArgumentType(),
+                                    kind == UETT_PreferredAlignOf),
+                     E);
     else
-      return Success(GetAlignOfExpr(Info, E->getArgumentExpr()), E);
+      return Success(GetAlignOfExpr(Info, E->getArgumentExpr(),
+                                    kind == UETT_PreferredAlignOf),
+                     E);
   }
 
   case UETT_VecStep: {

lib/AST/ItaniumMangle.cpp

     case UETT_SizeOf:
       Out << 's';
       break;
+    case UETT_PreferredAlignOf:
     case UETT_AlignOf:
       Out << 'a';
       break;

lib/AST/StmtPrinter.cpp

     else
       OS << "__alignof";
     break;
+  case UETT_PreferredAlignOf:
+    OS << "__alignof";
+    break;
   case UETT_VecStep:
     OS << "vec_step";
     break;

lib/Parse/ParseExpr.cpp

                                                           CastRange);
 
   UnaryExprOrTypeTrait ExprKind = UETT_SizeOf;
-  if (OpTok.isOneOf(tok::kw_alignof, tok::kw___alignof, tok::kw__Alignof))
+  if (OpTok.is(tok::kw___alignof))
+    ExprKind = UETT_PreferredAlignOf;
+  else if (OpTok.isOneOf(tok::kw_alignof, tok::kw__Alignof))
     ExprKind = UETT_AlignOf;
   else if (OpTok.is(tok::kw_vec_step))
     ExprKind = UETT_VecStep;

lib/Sema/SemaChecking.cpp

   if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
     if (const auto *UE =
             dyn_cast<UnaryExprOrTypeTraitExpr>(Arg->IgnoreParenImpCasts()))
-      if (UE->getKind() == UETT_AlignOf)
+      if (UE->getKind() == UETT_AlignOf ||
+          UE->getKind() == UETT_PreferredAlignOf)
         Diag(TheCall->getBeginLoc(), diag::warn_alloca_align_alignof)
             << Arg->getSourceRange();
 

   }
 
   AnalyzeImplicitConversions(S, E->getRHS(), E->getOperatorLoc());
-  
   // Diagnose implicitly sequentially-consistent atomic assignment.
   if (E->getLHS()->getType()->isAtomicType())
     S.Diag(E->getRHS()->getBeginLoc(), diag::warn_atomic_implicit_seq_cst);

lib/Sema/SemaExpr.cpp

 
   // C99 6.5.3.4p1:
   if (T->isFunctionType() &&
-      (TraitKind == UETT_SizeOf || TraitKind == UETT_AlignOf)) {
+      (TraitKind == UETT_SizeOf || TraitKind == UETT_AlignOf ||
+       TraitKind == UETT_PreferredAlignOf)) {
     // sizeof(function)/alignof(function) is allowed as an extension.
     S.Diag(Loc, diag::ext_sizeof_alignof_function_type)
       << TraitKind << ArgRange;

   // the expression to be complete. 'sizeof' requires the expression's type to
   // be complete (and will attempt to complete it if it's an array of unknown
   // bound).
-  if (ExprKind == UETT_AlignOf) {
+  if (ExprKind == UETT_AlignOf || ExprKind == UETT_PreferredAlignOf) {
     if (RequireCompleteType(E->getExprLoc(),
                             Context.getBaseElementType(E->getType()),
                             diag::err_sizeof_alignof_incomplete_type, ExprKind,

 
   // The operand for sizeof and alignof is in an unevaluated expression context,
   // so side effects could result in unintended consequences.
-  if ((ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf) &&
+  if ((ExprKind == UETT_SizeOf || ExprKind == UETT_AlignOf ||
+       ExprKind == UETT_PreferredAlignOf) &&
       !inTemplateInstantiation() && E->HasSideEffects(Context, false))
     Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
 

   // C11 6.5.3.4/3, C++11 [expr.alignof]p3:
   //   When alignof or _Alignof is applied to an array type, the result
   //   is the alignment of the element type.
-  if (ExprKind == UETT_AlignOf || ExprKind == UETT_OpenMPRequiredSimdAlign)
+  if (ExprKind == UETT_AlignOf || ExprKind == UETT_PreferredAlignOf ||
+      ExprKind == UETT_OpenMPRequiredSimdAlign)
     ExprType = Context.getBaseElementType(ExprType);
 
   if (ExprKind == UETT_VecStep)

   bool isInvalid = false;
   if (E->isTypeDependent()) {
     // Delay type-checking for type-dependent expressions.
-  } else if (ExprKind == UETT_AlignOf) {
+  } else if (ExprKind == UETT_AlignOf || ExprKind == UETT_PreferredAlignOf) {
     isInvalid = CheckAlignOfExpr(*this, E);
   } else if (ExprKind == UETT_VecStep) {
     isInvalid = CheckVecStepExpr(E);
   } else if (ExprKind == UETT_OpenMPRequiredSimdAlign) {
-      Diag(E->getExprLoc(), diag::err_openmp_default_simd_align_expr);
-      isInvalid = true;
-  } else if (E->refersToBitField()) {  // C99 6.5.3.4p1.
+  } else if (E->refersToBitField()) { // C99 6.5.3.4p1.
     Diag(E->getExprLoc(), diag::err_sizeof_alignof_typeof_bitfield) << 0;
     isInvalid = true;
   } else {