Merge pull request #22739 from rjmccall/empty-field-offset-variables-5.0

[5.0] Always give known-empty class properties a zero offset in the static layout

Author: AnnaZaks

lib/IRGen/ClassLayout.cpp

@@ -28,6 +28,7 @@ ClassLayout::ClassLayout(const StructLayoutBuilder &builder,
                          bool isFixedSize,
                          bool metadataRequiresInitialization,
                          bool metadataRequiresRelocation,
+                         bool classHasObjCAncestry,
                          llvm::Type *classTy,
                          ArrayRef<VarDecl *> allStoredProps,
                          ArrayRef<FieldAccess> allFieldAccesses,
@@ -38,21 +39,29 @@ ClassLayout::ClassLayout(const StructLayoutBuilder &builder,
     IsFixedSize(isFixedSize),
     MetadataRequiresInitialization(metadataRequiresInitialization),
     MetadataRequiresRelocation(metadataRequiresRelocation),
+    ClassHasObjCAncestry(classHasObjCAncestry),
     Ty(classTy),
     AllStoredProperties(allStoredProps),
     AllFieldAccesses(allFieldAccesses),
     AllElements(allElements) { }
 
 Size ClassLayout::getInstanceStart() const {
-  if (AllElements.empty())
-    return getSize();
+  ArrayRef<ElementLayout> elements = AllElements;
+  while (!elements.empty()) {
+    auto element = elements.front();
+    elements = elements.drop_front();
 
-  auto element = AllElements[0];
-  if (element.getKind() == ElementLayout::Kind::Fixed ||
-      element.getKind() == ElementLayout::Kind::Empty) {
-    // FIXME: assumes layout is always sequential!
-    return element.getByteOffset();
+    // Ignore empty elements.
+    if (element.isEmpty()) {
+      continue;
+    } else if (element.hasByteOffset()) {
+      // FIXME: assumes layout is always sequential!
+      return element.getByteOffset();
+    } else {
+      return Size(0);
+    }
   }
 
-  return Size(0);
+  // If there are no non-empty elements, just return the computed size.
+  return getSize();
 }

lib/IRGen/ClassLayout.h

@@ -63,6 +63,9 @@ class ClassLayout {
   /// Does the class metadata require relocation?
   bool MetadataRequiresRelocation;
 
+  /// Does the class have ObjC ancestry?
+  bool ClassHasObjCAncestry;
+
   /// The LLVM type for instances of this class.
   llvm::Type *Ty;
 
@@ -82,6 +85,7 @@ class ClassLayout {
               bool isFixedSize,
               bool metadataRequiresInitialization,
               bool metadataRequiresRelocation,
+              bool classHasObjCAncestry,
               llvm::Type *classTy,
               ArrayRef<VarDecl *> allStoredProps,
               ArrayRef<FieldAccess> allFieldAccesses,
@@ -98,6 +102,15 @@ class ClassLayout {
 
   bool isFixedSize() const { return IsFixedSize; }
 
+  /// Returns true if the runtime may attempt to assign non-zero offsets to
+  /// empty fields for this class.  The ObjC runtime will do this if it
+  /// decides it needs to slide ivars.  This is the one exception to the
+  /// general rule that the runtime will not try to assign a different offset
+  /// than was computed statically for a field with a fixed offset.
+  bool mayRuntimeAssignNonZeroOffsetsToEmptyFields() const {
+    return ClassHasObjCAncestry;
+  }
+
   bool doesMetadataRequireInitialization() const {
     return MetadataRequiresInitialization;
   }

lib/IRGen/GenClass.cpp

@@ -293,6 +293,7 @@ namespace {
                          isFixedSize(),
                          doesMetadataRequireInitialization(),
                          doesMetadataRequireRelocation(),
+                         ClassHasObjCAncestry,
                          classTy,
                          allStoredProps,
                          allFieldAccesses,
@@ -393,7 +394,7 @@ namespace {
         }
 
         auto element = ElementLayout::getIncomplete(*eltType);
-        addField(element, LayoutStrategy::Universal);
+        bool isKnownEmpty = !addField(element, LayoutStrategy::Universal);
 
         // The 'Elements' list only contains superclass fields when we're
         // building a layout for tail allocation.
@@ -402,7 +403,7 @@ namespace {
 
         if (!superclass) {
           AllStoredProperties.push_back(var);
-          AllFieldAccesses.push_back(getFieldAccess());
+          AllFieldAccesses.push_back(getFieldAccess(isKnownEmpty));
         }
       }
 
@@ -459,7 +460,12 @@ namespace {
       }
     }
 
-    FieldAccess getFieldAccess() {
+    FieldAccess getFieldAccess(bool isKnownEmpty) {
+      // If the field is known empty, then its access pattern is always
+      // constant-direct.
+      if (isKnownEmpty)
+        return FieldAccess::ConstantDirect;
+
       // If the layout so far has a fixed size, the field offset is known
       // statically.
       if (isFixedSize())
@@ -635,8 +641,7 @@ irgen::getClassFieldOffset(IRGenModule &IGM, SILType baseType, VarDecl *field) {
 
   auto fieldInfo = classLayout.getFieldAccessAndElement(field);
   auto element = fieldInfo.second;
-  assert(element.getKind() == ElementLayout::Kind::Fixed ||
-         element.getKind() == ElementLayout::Kind::Empty);
+  assert(element.hasByteOffset());
   return element.getByteOffset();
 }
 

lib/IRGen/GenMeta.cpp

@@ -2318,7 +2318,7 @@ static void emitFieldOffsetGlobals(IRGenModule &IGM,
 
     llvm::Constant *fieldOffsetOrZero;
 
-    if (element.getKind() == ElementLayout::Kind::Fixed) {
+    if (element.hasByteOffset()) {
       // Use a fixed offset if we have one.
       fieldOffsetOrZero = IGM.getSize(element.getByteOffset());
     } else {
@@ -2346,11 +2346,21 @@ static void emitFieldOffsetGlobals(IRGenModule &IGM,
       // If it is constant in the fragile layout only, newer Objective-C
       // runtimes will still update them in place, so make sure to check the
       // correct layout.
+      //
+      // The one exception to this rule is with empty fields with
+      // ObjC-resilient heritage.  The ObjC runtime will attempt to slide
+      // these offsets if it slides the rest of the class, and in doing so
+      // it will compute a different offset than we computed statically.
+      // But this is ultimately unimportant because we do not care about the
+      // offset of an empty field.
       auto resilientInfo = resilientLayout.getFieldAccessAndElement(prop);
-      if (resilientInfo.first == FieldAccess::ConstantDirect) {
+      if (resilientInfo.first == FieldAccess::ConstantDirect &&
+          (!resilientInfo.second.isEmpty() ||
+           !resilientLayout.mayRuntimeAssignNonZeroOffsetsToEmptyFields())) {
         // If it is constant in the resilient layout, it should be constant in
         // the fragile layout also.
         assert(access == FieldAccess::ConstantDirect);
+        assert(element.hasByteOffset());
         offsetVar->setConstant(true);
       }
 

lib/IRGen/GenRecord.h

@@ -82,6 +82,10 @@ template <class FieldImpl> class RecordField {
   ElementLayout::Kind getKind() const {
     return Layout.getKind();
   }
+
+  bool hasFixedByteOffset() const {
+    return Layout.hasByteOffset();
+  }
 
   Size getFixedByteOffset() const {
     return Layout.getByteOffset();
@@ -763,7 +767,7 @@ class RecordTypeInfo<Impl, Base, FieldImpl,
                            Explosion &src,
                            unsigned startOffset) const override {
     for (auto &field : getFields()) {
-      if (field.getKind() != ElementLayout::Kind::Empty) {
+      if (!field.isEmpty()) {
         unsigned offset = field.getFixedByteOffset().getValueInBits()
           + startOffset;
         cast<LoadableTypeInfo>(field.getTypeInfo())
@@ -776,7 +780,7 @@ class RecordTypeInfo<Impl, Base, FieldImpl,
                              Explosion &dest, unsigned startOffset)
                             const override {
     for (auto &field : getFields()) {
-      if (field.getKind() != ElementLayout::Kind::Empty) {
+      if (!field.isEmpty()) {
         unsigned offset = field.getFixedByteOffset().getValueInBits()
           + startOffset;
         cast<LoadableTypeInfo>(field.getTypeInfo())

lib/IRGen/GenStruct.cpp

@@ -172,8 +172,7 @@ namespace {
     llvm::Constant *getConstantFieldOffset(IRGenModule &IGM,
                                            VarDecl *field) const {
       auto &fieldInfo = getFieldInfo(field);
-      if (fieldInfo.getKind() == ElementLayout::Kind::Fixed
-          || fieldInfo.getKind() == ElementLayout::Kind::Empty) {
+      if (fieldInfo.hasFixedByteOffset()) {
         return llvm::ConstantInt::get(
             IGM.Int32Ty, fieldInfo.getFixedByteOffset().getValue());
       }
@@ -791,8 +790,7 @@ class ClangRecordLowering {
     ElementLayout layout = ElementLayout::getIncomplete(fieldType);
     auto isEmpty = fieldType.isKnownEmpty(ResilienceExpansion::Maximal);
     if (isEmpty)
-      layout.completeEmpty(fieldType.isPOD(ResilienceExpansion::Maximal),
-                           NextOffset);
+      layout.completeEmpty(fieldType.isPOD(ResilienceExpansion::Maximal));
     else
       layout.completeFixed(fieldType.isPOD(ResilienceExpansion::Maximal),
                            NextOffset, LLVMFields.size());

lib/IRGen/StructLayout.cpp

@@ -304,8 +304,7 @@ void StructLayoutBuilder::addNonFixedSizeElement(ElementLayout &elt) {
 
 /// Add an empty element to the aggregate.
 void StructLayoutBuilder::addEmptyElement(ElementLayout &elt) {
-  elt.completeEmpty(elt.getType().isPOD(ResilienceExpansion::Maximal),
-                    CurSize);
+  elt.completeEmpty(elt.getType().isPOD(ResilienceExpansion::Maximal));
 }
 
 /// Add an element at the fixed offset of the current end of the

lib/IRGen/StructLayout.h

@@ -81,6 +81,7 @@ class ElementLayout {
 public:
   enum class Kind {
     /// The element is known to require no storage in the aggregate.
+    /// Its offset in the aggregate is always statically zero.
     Empty,
 
     /// The element can be positioned at a fixed offset within the
@@ -95,10 +96,12 @@ class ElementLayout {
     /// offset zero.  This is necessary because LLVM forbids even a
     /// 'gep 0' on an unsized type.
     InitialNonFixedSize
+
+    // IncompleteKind comes here
   };
 
 private:
-  enum : unsigned { IncompleteKind  = 4 };
+  enum : unsigned { IncompleteKind  = unsigned(Kind::InitialNonFixedSize) + 1 };
 
   /// The swift type information for this element's layout.
   const TypeInfo *Type;
@@ -137,19 +140,17 @@ class ElementLayout {
     Index = other.Index;
   }
 
-  void completeEmpty(IsPOD_t isPOD, Size byteOffset) {
+  void completeEmpty(IsPOD_t isPOD) {
     TheKind = unsigned(Kind::Empty);
     IsPOD = unsigned(isPOD);
-    // We still want to give empty fields an offset for use by things like
-    // ObjC ivar emission. We use the first field in a class layout as the
-    // instanceStart.
-    ByteOffset = byteOffset.getValue();
+    ByteOffset = 0;
     Index = 0; // make a complete write of the bitfield
   }
 
   void completeInitialNonFixedSize(IsPOD_t isPOD) {
     TheKind = unsigned(Kind::InitialNonFixedSize);
     IsPOD = unsigned(isPOD);
+    ByteOffset = 0;
     Index = 0; // make a complete write of the bitfield
   }
 
@@ -189,10 +190,25 @@ class ElementLayout {
     return IsPOD_t(IsPOD);
   }
 
+  /// Can we access this element at a static offset?
+  bool hasByteOffset() const {
+    switch (getKind()) {
+    case Kind::Empty:
+    case Kind::Fixed:
+      return true;
+
+    // FIXME: InitialNonFixedSize should go in the above, but I'm being
+    // paranoid about changing behavior.
+    case Kind::InitialNonFixedSize:
+    case Kind::NonFixed:
+      return false;
+    }
+    llvm_unreachable("bad kind");
+  }
+
   /// Given that this element has a fixed offset, return that offset in bytes.
   Size getByteOffset() const {
-    assert(isCompleted() &&
-           (getKind() == Kind::Fixed || getKind() == Kind::Empty));
+    assert(isCompleted() && hasByteOffset());
     return Size(ByteOffset);
   }
 

test/IRGen/class_resilience.swift

@@ -16,6 +16,9 @@
 
 // CHECK: @"$s16class_resilience21ResilientGenericChildCMo" = {{(protected )?}}{{(dllexport )?}}global [[BOUNDS:{ (i32|i64), i32, i32 }]] zeroinitializer
 
+// CHECK: @"$s16class_resilience27ClassWithEmptyThenResilientC9resilient0H7_struct0G3IntVvpWvd" = hidden global [[INT]] 0,
+// CHECK: @"$s16class_resilience27ClassWithResilientThenEmptyC9resilient0H7_struct0E3IntVvpWvd" = hidden global [[INT]] 0,
+
 // CHECK: @"$s16class_resilience26ClassWithResilientPropertyCMo" = {{(protected )?}}{{(dllexport )?}}constant [[BOUNDS]]
 // CHECK-SAME-32: { [[INT]] 52, i32 2, i32 13 }
 // CHECK-SAME-64: { [[INT]] 80, i32 2, i32 10 }
@@ -113,6 +116,9 @@
 // CHECK-SAME-32: { [[INT]] 64, i32 2, i32 16 }
 // CHECK-SAME-64: { [[INT]] 104, i32 2, i32 13 }
 
+// CHECK: @"$s16class_resilience27ClassWithEmptyThenResilientC5emptyAA0E0VvpWvd" = hidden constant [[INT]] 0,
+// CHECK: @"$s16class_resilience27ClassWithResilientThenEmptyC5emptyAA0G0VvpWvd" = hidden constant [[INT]] 0,
+
 // CHECK: @"$s16class_resilience14ResilientChildC5fields5Int32VvgTq" = {{(protected )?}}{{(dllexport )?}}alias %swift.method_descriptor, getelementptr inbounds
 // CHECK: @"$s16class_resilience14ResilientChildC5fields5Int32VvsTq" = {{(protected )?}}{{(dllexport )?}}alias %swift.method_descriptor, getelementptr inbounds
 // CHECK: @"$s16class_resilience14ResilientChildC5fields5Int32VvMTq" = {{(protected )?}}{{(dllexport )?}}alias %swift.method_descriptor, getelementptr inbounds
@@ -244,6 +250,34 @@ extension ResilientGenericOutsideParent {
   }
 }
 
+// rdar://48031465
+// Field offsets for empty fields in resilient classes should be initialized
+// to their best-known value and made non-constant if that value might
+// disagree with the dynamic value.
+
+@_fixed_layout
+public struct Empty {}
+
+public class ClassWithEmptyThenResilient {
+  public let empty: Empty
+  public let resilient: ResilientInt
+
+  public init(empty: Empty, resilient: ResilientInt) {
+    self.empty = empty
+    self.resilient = resilient
+  }
+}
+
+public class ClassWithResilientThenEmpty {
+  public let resilient: ResilientInt
+  public let empty: Empty
+
+  public init(empty: Empty, resilient: ResilientInt) {
+    self.empty = empty
+    self.resilient = resilient
+  }
+}
+
 // ClassWithResilientProperty.color getter
 
 // CHECK-LABEL: define{{( dllexport)?}}{{( protected)?}} swiftcc i32 @"$s16class_resilience26ClassWithResilientPropertyC5colors5Int32Vvg"(%T16class_resilience26ClassWithResilientPropertyC* swiftself)

test/IRGen/class_resilience_objc.swift

@@ -1,10 +1,17 @@
-// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -enable-objc-interop -emit-ir -o - -primary-file %s | %FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-%target-ptrsize
+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -enable-resilience -enable-class-resilience -emit-module-path=%t/resilient_struct.swiftmodule -module-name=resilient_struct %S/../Inputs/resilient_struct.swift
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -I %t -enable-resilience -enable-class-resilience -enable-objc-interop -emit-ir -o - -primary-file %s | %FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-%target-ptrsize -DINT=i%target-ptrsize
 
 // XFAIL: CPU=armv7k
 
-// CHECK: %swift.type = type { [[INT:i32|i64]] }
-
 import Foundation
+import resilient_struct
+
+// Note that these are all mutable to allow for the runtime to slide them.
+// CHECK: @"$s21class_resilience_objc27ClassWithEmptyThenResilientC9resilient0I7_struct0H3IntVvpWvd" = hidden global [[INT]] 0,
+// CHECK: @"$s21class_resilience_objc27ClassWithResilientThenEmptyC9resilient0I7_struct0F3IntVvpWvd" = hidden global [[INT]] 0,
+// CHECK: @"$s21class_resilience_objc27ClassWithEmptyThenResilientC5emptyAA0F0VvpWvd" = hidden global [[INT]] 0,
+// CHECK: @"$s21class_resilience_objc27ClassWithResilientThenEmptyC5emptyAA0H0VvpWvd" = hidden global [[INT]] 0,
 
 public class FixedLayoutObjCSubclass : NSObject {
   // This field could use constant direct access because NSObject has
@@ -89,3 +96,26 @@ func testConstantIndirectFieldAccess<T>(_ o: GenericObjCSubclass<T>) {
   // because the field offset vector only contains Swift field offsets.
   o.field = 10
 }
+
+@_fixed_layout
+public struct Empty {}
+
+public class ClassWithEmptyThenResilient : DummyClass {
+  public let empty: Empty
+  public let resilient: ResilientInt
+
+  public init(empty: Empty, resilient: ResilientInt) {
+    self.empty = empty
+    self.resilient = resilient
+  }
+}
+
+public class ClassWithResilientThenEmpty : DummyClass {
+  public let resilient: ResilientInt
+  public let empty: Empty
+
+  public init(empty: Empty, resilient: ResilientInt) {
+    self.empty = empty
+    self.resilient = resilient
+  }
+}