Always give known-empty class properties a zero offset in the static layout.

Field offset vectors are always filled out with either zero or the static layout's offset, depending on the metadata initialization strategy.  This change means that the static layout's offset will only be non-zero for properties with a statically-known layout.  Existing runtimes doing dynamic class layout assign class properties a zero offset if the field offset vector entry is zero and the property is zero-sized.  So this effectively brings the compiler into accord with the runtime (for all newly-compiled Swift code, which will eventually be all Swift code because the current public releases of Swift 5 are not yet considered ABI-stable) and guarantees a zero value for the offset everywhere.

Since the runtime will agree with the compiler about the zero value of the offset, the compiler can continue to emit such offset variables as constant.  The exception to this rule is if the class has non-fragile ObjC ancestry, in which case the ObjC runtime (which is not aware of this special rule for empty fields) will attempt to slide it along with everything else.

Fixes rdar://48031465, in which the `FixedClassMetadataBuilder` for a class with a legacy-fixed layout was writing a non-zero offset for an empty field into the field offset vector, causing the runtime to not apply the special case and thus to compute a non-zero offset, which it then attempted to copy into the global field offset variable, which the compiler had emitted as a true-constant zero.

Author: rjmccall

lib/IRGen/ClassLayout.cpp

@@ -40,15 +40,22 @@ ClassLayout::ClassLayout(const StructLayoutBuilder &builder,
     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

@@ -157,6 +157,15 @@ class ClassLayout {
              Options.contains(ClassMetadataFlags::ClassHasObjCAncestry));
   }
 
+  /// 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 Options.contains(ClassMetadataFlags::ClassHasObjCAncestry);
+  }
+
   /// Returns true iff everything about the class metadata layout is statically
   /// known except field offsets and the instance size and alignment.
   ///

lib/IRGen/GenClass.cpp

@@ -314,7 +314,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.
@@ -323,7 +323,7 @@ namespace {
 
         if (!superclass) {
           AllStoredProperties.push_back(var);
-          AllFieldAccesses.push_back(getFieldAccess());
+          AllFieldAccesses.push_back(getFieldAccess(isKnownEmpty));
         }
       }
 
@@ -381,7 +381,12 @@ namespace {
       }
     }
 
-    FieldAccess getFieldAccess() {
+    FieldAccess getFieldAccess(bool isKnownEmpty) {
+      // If the field known empty, then its access pattern is always
+      // constant-direct.
+      if (isKnownEmpty)
+        return FieldAccess::ConstantDirect;
+
       // If layout so far depends on generic parameters, we have to load the
       // offset from the field offset vector in class metadata.
       if (Options.contains(ClassMetadataFlags::ClassHasGenericLayout))
@@ -556,8 +561,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

@@ -2349,7 +2349,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 {
@@ -2377,11 +2377,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,11 +1,18 @@
-// 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 -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-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
 // XFAIL: CPU=powerpc64le
 
-// 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
@@ -90,3 +97,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
+  }
+}