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

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

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
+  }
+}