r235949 - ms_struct does not imply the MS base-layout ABI; separate these

[John McCall]

Author: rjmccall
Date: Mon Apr 27 19:17:18 2015
New Revision: 235949

URL: http://llvm.org/viewvc/llvm-project?rev=235949&view=rev
Log:
ms_struct does not imply the MS base-layout ABI; separate these
conditions in the IRGen struct layout code.

rdar://20636558

Added:
    cfe/trunk/test/CodeGenCXX/ms_struct.cpp
Modified:
    cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp

Modified: cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp?rev=235949&r1=235948&r2=235949&view=diff
==============================================================================
--- cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGRecordLayoutBuilder.cpp Mon Apr 27 19:17:18 2015
@@ -99,10 +99,25 @@ struct CGRecordLowering {
   MemberInfo StorageInfo(CharUnits Offset, llvm::Type *Data) {
     return MemberInfo(Offset, MemberInfo::Field, Data);
   }
-  bool useMSABI() {
+
+  /// The Microsoft bitfield layout rule allocates discrete storage
+  /// units of the field's formal type and only combines adjacent
+  /// fields of the same formal type.  We want to emit a layout with
+  /// these discrete storage units instead of combining them into a
+  /// continuous run.
+  bool isDiscreteBitFieldABI() {
     return Context.getTargetInfo().getCXXABI().isMicrosoft() ||
            D->isMsStruct(Context);
   }
+
+  /// The Itanium base layout rule allows virtual bases to overlap
+  /// other bases, which complicates layout in specific ways.
+  ///
+  /// Note specifically that the ms_struct attribute doesn't change this.
+  bool isOverlappingVBaseABI() {
+    return !Context.getTargetInfo().getCXXABI().isMicrosoft();
+  }
+
   /// \brief Wraps llvm::Type::getIntNTy with some implicit arguments.
   llvm::Type *getIntNType(uint64_t NumBits) {
     return llvm::Type::getIntNTy(Types.getLLVMContext(),
@@ -119,8 +134,9 @@ struct CGRecordLowering {
   /// for itanium bitfields that are smaller than their declared type.
   llvm::Type *getStorageType(const FieldDecl *FD) {
     llvm::Type *Type = Types.ConvertTypeForMem(FD->getType());
-    return useMSABI() || !FD->isBitField() ? Type :
-        getIntNType(std::min(FD->getBitWidthValue(Context),
+    if (!FD->isBitField()) return Type;
+    if (isDiscreteBitFieldABI()) return Type;
+    return getIntNType(std::min(FD->getBitWidthValue(Context),
                              (unsigned)Context.toBits(getSize(Type))));
   }
   /// \brief Gets the llvm Basesubobject type from a CXXRecordDecl.
@@ -365,7 +381,7 @@ CGRecordLowering::accumulateBitFields(Re
   // used to determine if the ASTRecordLayout is treating these two bitfields as
   // contiguous.  StartBitOffset is offset of the beginning of the Run.
   uint64_t StartBitOffset, Tail = 0;
-  if (useMSABI()) {
+  if (isDiscreteBitFieldABI()) {
     for (; Field != FieldEnd; ++Field) {
       uint64_t BitOffset = getFieldBitOffset(*Field);
       // Zero-width bitfields end runs.
@@ -465,7 +481,7 @@ void CGRecordLowering::accumulateVBases(
   // smaller than the nvsize.  Here we check to see if such a base is placed
   // before the nvsize and set the scissor offset to that, instead of the
   // nvsize.
-  if (!useMSABI())
+  if (isOverlappingVBaseABI())
     for (const auto &Base : RD->vbases()) {
       const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
       if (BaseDecl->isEmpty())
@@ -486,7 +502,8 @@ void CGRecordLowering::accumulateVBases(
     CharUnits Offset = Layout.getVBaseClassOffset(BaseDecl);
     // If the vbase is a primary virtual base of some base, then it doesn't
     // get its own storage location but instead lives inside of that base.
-    if (!useMSABI() && Context.isNearlyEmpty(BaseDecl) &&
+    if (isOverlappingVBaseABI() &&
+        Context.isNearlyEmpty(BaseDecl) &&
         !hasOwnStorage(RD, BaseDecl)) {
       Members.push_back(MemberInfo(Offset, MemberInfo::VBase, nullptr,
                                    BaseDecl));

Added: cfe/trunk/test/CodeGenCXX/ms_struct.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/ms_struct.cpp?rev=235949&view=auto
==============================================================================
--- cfe/trunk/test/CodeGenCXX/ms_struct.cpp (added)
+++ cfe/trunk/test/CodeGenCXX/ms_struct.cpp Mon Apr 27 19:17:18 2015
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm %s -o - | FileCheck %s
+
+// rdar://20636558
+
+#pragma GCC diagnostic ignored "-Wincompatible-ms-struct"
+#define ATTR __attribute__((__ms_struct__))
+
+struct ATTR VBase {
+  virtual void foo() = 0;
+};
+
+struct ATTR Base : virtual VBase {
+  virtual void bar() = 0;
+};
+
+struct ATTR Derived : Base {
+  Derived();
+  void foo();
+  void bar();
+  int value;
+};
+
+// CHECK: [[DERIVED:%.*]] = type <{ [[BASE:%.*]], i32, [4 x i8] }>
+// CHECK: [[BASE]] = type { [[VBASE:%.*]] }
+// CHECK: [[VBASE]] = type { i32 (...)** }
+
+// CHECK: define void @_ZN7DerivedC2Ev
+// CHECK:   [[SELF:%.*]] = load [[DERIVED]]*
+// CHECK:   [[T0:%.*]] = bitcast [[DERIVED]]* [[SELF]] to [[BASE]]*
+// CHECK:   call void @_ZN4BaseC2Ev([[BASE]]* [[T0]], i8**
+// CHECK:   [[T0:%.*]] = getelementptr inbounds {{.*}} [[SELF]], i32 0, i32 1
+// CHECK:   store i32 20, i32* [[T0]],
+Derived::Derived() : value(20) {}