Reflection: Fix class layout, again

slavapestov · slavapestov · commit 1c7a50dcc52a · 2016-11-16T19:31:18.000-08:00
There were a few problems here with subclasses of Objective-C classes.
Use the InstanceStart field from rodata to correctly lay out instance
variables, and verify the results match with dynamic and static layout.

Better fix for &lt;rdar://problem/27932061&gt;.
diff --git a/include/swift/Reflection/ReflectionContext.h b/include/swift/Reflection/ReflectionContext.h
@@ -92,13 +92,13 @@ class ReflectionContext
         // Figure out where the stored properties of this class begin
         // by looking at the size of the superclass
         bool valid;
-        unsigned size, align;
-        std::tie(valid, size, align) =
-            this->readInstanceSizeAndAlignmentFromClassMetadata(MetadataAddress);
+        unsigned start;
+        std::tie(valid, start) =
+            this->readInstanceStartAndAlignmentFromClassMetadata(MetadataAddress);
 
         // Perform layout
         if (valid)
-          TI = TC.getClassInstanceTypeInfo(TR, size, align);
+          TI = TC.getClassInstanceTypeInfo(TR, start);
 
         break;
       }
diff --git a/include/swift/Reflection/TypeLowering.h b/include/swift/Reflection/TypeLowering.h
@@ -246,8 +246,7 @@ class TypeConverter {
   ///
   /// Not cached.
   const TypeInfo *getClassInstanceTypeInfo(const TypeRef *TR,
-                                           unsigned start,
-                                           unsigned align);
+                                           unsigned start);
 
 private:
   friend class swift::reflection::LowerType;
diff --git a/include/swift/Remote/MetadataReader.h b/include/swift/Remote/MetadataReader.h
@@ -561,94 +561,28 @@ class MetadataReader {
   }
 
   /// Given a remote pointer to class metadata, attempt to discover its class
-  /// instance size and alignment.
-  std::tuple<bool, unsigned, unsigned>
-  readInstanceSizeAndAlignmentFromClassMetadata(StoredPointer MetadataAddress) {
+  /// instance size and whether fields should use the resilient layout strategy.
+  std::pair<bool, unsigned>
+  readInstanceStartAndAlignmentFromClassMetadata(StoredPointer MetadataAddress) {
     auto meta = readMetadata(MetadataAddress);
     if (!meta || meta->getKind() != MetadataKind::Class)
-      return std::make_tuple(false, 0, 0);
-
-    auto classMeta = cast<TargetClassMetadata<Runtime>>(meta);
-
-    // See swift_initClassMetadata_UniversalStrategy()
-    uint32_t size, align;
-
-    // If this class is defined in Objective-C, return the value of the
-    // InstanceStart field from the ROData.
-    if (!classMeta->isTypeMetadata()) {
-      // The following algorithm only works on the non-fragile Apple runtime.
-
-      // Grab the RO-data pointer.  This part is not ABI.
-      StoredPointer roDataPtr = readObjCRODataPtr(MetadataAddress);
-      if (!roDataPtr)
-        return std::make_tuple(false, 0, 0);
-
-      // Get the address of the InstanceStart field.
-      auto address = roDataPtr + sizeof(uint32_t) * 1;
-
-      align = 16;
-
-      if (!Reader->readInteger(RemoteAddress(address), &size))
-        return std::make_tuple(false, 0, 0);
-
-      assert((size & (align - 1)) == 0);
-      return std::make_tuple(true, size, align);
-    }
-
-    // Otherwise, it is a Swift class. Get the superclass.
-    auto superAddr = readSuperClassFromClassMetadata(MetadataAddress);
-    if (superAddr) {
-      auto superMeta = readMetadata(superAddr);
-      if (!superMeta || superMeta->getKind() != MetadataKind::Class)
-        return std::make_tuple(false, 0, 0);
-
-      auto superclassMeta = cast<TargetClassMetadata<Runtime>>(superMeta);
-
-      // If the superclass is an Objective-C class, we start layout
-      // from the InstanceSize of the superclass, aligned up to
-      // 16 bytes.
-      if (superclassMeta->isTypeMetadata()) {
-        // Superclass is a Swift class. Get the size of an instance,
-        // and start layout from that.
-        size = superclassMeta->getInstanceSize();
-        align = 1;
-
-        return std::make_tuple(true, size, align);
-      }
-
-      std::string superName;
-      if (!readObjCClassName(superAddr, superName))
-        return std::make_tuple(false, 0, 0);
-
-      if (superName != "SwiftObject") {
-        // Grab the RO-data pointer.  This part is not ABI.
-        StoredPointer roDataPtr = readObjCRODataPtr(superAddr);
-        if (!roDataPtr)
-          return std::make_tuple(false, 0, 0);
-
-        // Get the address of the InstanceSize field.
-        auto address = roDataPtr + sizeof(uint32_t) * 2;
-
-        // malloc alignment boundary.
-        align = 16;
-
-        if (!Reader->readInteger(RemoteAddress(address), &size))
-          return std::make_tuple(false, 0, 0);
+      return std::make_pair(false, 0);
 
-        // Round up to the alignment boundary.
-        size = (size + (align - 1)) & ~(align - 1);
+    // The following algorithm only works on the non-fragile Apple runtime.
 
-        return std::make_tuple(true, size, align);
-      }
+    // Grab the RO-data pointer.  This part is not ABI.
+    StoredPointer roDataPtr = readObjCRODataPtr(MetadataAddress);
+    if (!roDataPtr)
+      return std::make_pair(false, 0);
 
-      // Fall through.
-    }
+    // Get the address of the InstanceStart field.
+    auto address = roDataPtr + sizeof(uint32_t) * 1;
 
-    // No superclass, just an object header. 12 bytes on 32-bit, 16 on 64-bit
-    size = Reader->getPointerSize() + sizeof(uint64_t);
-    align = 1;
+    unsigned start;
+    if (!Reader->readInteger(RemoteAddress(address), &start))
+      return std::make_pair(false, 0);
 
-    return std::make_tuple(true, size, align);
+    return std::make_pair(true, start);
   }
 
   /// Given a remote pointer to metadata, attempt to turn it into a type.
diff --git a/stdlib/public/Reflection/TypeLowering.cpp b/stdlib/public/Reflection/TypeLowering.cpp
@@ -1251,8 +1251,7 @@ const TypeInfo *TypeConverter::getTypeInfo(const TypeRef *TR) {
 }
 
 const TypeInfo *TypeConverter::getClassInstanceTypeInfo(const TypeRef *TR,
-                                                        unsigned start,
-                                                        unsigned align) {
+                                                        unsigned start) {
   const FieldDescriptor *FD = getBuilder().getFieldTypeInfo(TR);
   if (FD == nullptr) {
     DEBUG(std::cerr << "No field descriptor: "; TR->dump());
diff --git a/validation-test/Reflection/Inputs/ObjCClasses/ObjCClasses.h b/validation-test/Reflection/Inputs/ObjCClasses/ObjCClasses.h
@@ -0,0 +1,21 @@
+#ifndef SWIFT_TEST_OBJC_CLASSES_H
+#define SWIFT_TEST_OBJC_CLASSES_H
+
+#import <Foundation/Foundation.h>
+
+@interface FirstClass : NSObject
+@property void *x;
+@end
+
+@interface SecondClass : NSObject
+@property void *x;
+@property void *y;
+@end
+
+@interface ThirdClass : NSObject
+@property void *x;
+@property void *y;
+@property void *z;
+@end
+
+#endif
diff --git a/validation-test/Reflection/Inputs/ObjCClasses/ObjCClasses.m b/validation-test/Reflection/Inputs/ObjCClasses/ObjCClasses.m
@@ -0,0 +1,16 @@
+#import "ObjCClasses.h"
+
+@implementation FirstClass : NSObject
+@synthesize x;
+@end
+
+@implementation SecondClass : NSObject
+@synthesize x;
+@synthesize y;
+@end
+
+@implementation ThirdClass : NSObject
+@synthesize x;
+@synthesize y;
+@synthesize z;
+@end
diff --git a/validation-test/Reflection/Inputs/ObjCClasses/module.map b/validation-test/Reflection/Inputs/ObjCClasses/module.map
@@ -0,0 +1,3 @@
+module ObjCClasses {
+  header "ObjCClasses.h"
+}
diff --git a/validation-test/Reflection/inherits_NSObject.swift b/validation-test/Reflection/inherits_NSObject.swift
@@ -6,6 +6,7 @@
 // REQUIRES: executable_test
 
 import Foundation
+import simd
 
 import SwiftReflectionTest
 
@@ -14,11 +15,6 @@ class BaseNSClass : NSObject {
   var x: Bool = false
 }
 
-class DerivedNSClass : BaseNSClass {
-  var y: Bool = false
-  var z: Int = 0
-}
-
 let baseClass = BaseNSClass()
 reflect(object: baseClass)
 
@@ -42,16 +38,21 @@ reflect(object: baseClass)
 // CHECK-32: (class inherits_NSObject.BaseNSClass)
 
 // CHECK-32: Type info:
-// CHECK-32-NEXT: (class_instance size=21 alignment=4 stride=24 num_extra_inhabitants=0
-// CHECK-32-NEXT:   (field name=w offset=16
+// CHECK-32-NEXT: (class_instance size=17 alignment=4 stride=20 num_extra_inhabitants=0
+// CHECK-32-NEXT:   (field name=w offset=12
 // CHECK-32-NEXT:     (struct size=4 alignment=4 stride=4 num_extra_inhabitants=0
 // CHECK-32-NEXT:       (field name=_value offset=0
 // CHECK-32-NEXT:         (builtin size=4 alignment=4 stride=4 num_extra_inhabitants=0))))
-// CHECK-32-NEXT:   (field name=x offset=20
+// CHECK-32-NEXT:   (field name=x offset=16
 // CHECK-32-NEXT:     (struct size=1 alignment=1 stride=1 num_extra_inhabitants=254
 // CHECK-32-NEXT:       (field name=_value offset=0
 // CHECK-32-NEXT:         (builtin size=1 alignment=1 stride=1 num_extra_inhabitants=254)))))
 
+class DerivedNSClass : BaseNSClass {
+  var y: Bool = false
+  var z: Int = 0
+}
+
 let derivedClass = DerivedNSClass()
 reflect(object: derivedClass)
 
@@ -85,4 +86,106 @@ reflect(object: derivedClass)
 // CHECK-32-NEXT:       (field name=_value offset=0
 // CHECK-32-NEXT:         (builtin size=4 alignment=4 stride=4 num_extra_inhabitants=0)))))
 
+// Note: dynamic layout starts at offset 8, not 16
+class GenericBaseNSClass<T> : NSObject {
+  var w: T = 0 as! T
+}
+
+let genericBaseClass = GenericBaseNSClass<Int>()
+reflect(object: genericBaseClass)
+
+// CHECK-64: Reflecting an object.
+// CHECK-64: Type reference:
+// CHECK-64: (bound_generic_class inherits_NSObject.GenericBaseNSClass
+// CHECK-64:   (struct Swift.Int))
+
+// CHECK-64: Type info:
+// CHECK-64-NEXT: (class_instance size=16 alignment=8 stride=16 num_extra_inhabitants=0
+// CHECK-64-NEXT:   (field name=w offset=8
+// CHECK-64-NEXT:     (struct size=8 alignment=8 stride=8 num_extra_inhabitants=0
+// CHECK-64-NEXT:       (field name=_value offset=0
+// CHECK-64-NEXT:         (builtin size=8 alignment=8 stride=8 num_extra_inhabitants=0)))))
+
+// CHECK-32: Reflecting an object.
+// CHECK-32: Type reference:
+// CHECK-32: (bound_generic_class inherits_NSObject.GenericBaseNSClass
+// CHECK-32:   (struct Swift.Int))
+
+// CHECK-32: Type info:
+// CHECK-32-NEXT: (class_instance size=8 alignment=4 stride=8 num_extra_inhabitants=0
+// CHECK-32-NEXT:   (field name=w offset=4
+// CHECK-32-NEXT:     (struct size=4 alignment=4 stride=4 num_extra_inhabitants=0
+// CHECK-32-NEXT:       (field name=_value offset=0
+// CHECK-32-NEXT:         (builtin size=4 alignment=4 stride=4 num_extra_inhabitants=0)))))
+
+class AlignedNSClass : NSObject {
+  var w: Int = 0
+  var x: int4 = [1,2,3,4]
+}
+
+let alignedClass = AlignedNSClass()
+reflect(object: alignedClass)
+
+// CHECK-64: Reflecting an object.
+// CHECK-64: Type reference:
+// CHECK-64: (class inherits_NSObject.AlignedNSClass)
+
+// CHECK-64: Type info:
+// CHECK-64-NEXT: (class_instance size=48 alignment=16 stride=48 num_extra_inhabitants=0
+// CHECK-64-NEXT:   (field name=w offset=16
+// CHECK-64-NEXT:     (struct size=8 alignment=8 stride=8 num_extra_inhabitants=0
+// CHECK-64-NEXT:       (field name=_value offset=0
+// CHECK-64-NEXT:         (builtin size=8 alignment=8 stride=8 num_extra_inhabitants=0))))
+// CHECK-64-NEXT:   (field name=x offset=32
+// CHECK-64-NEXT:     (builtin size=16 alignment=16 stride=16 num_extra_inhabitants=0)))
+
+// CHECK-32: Reflecting an object.
+// CHECK-32: Type reference:
+// CHECK-32: (class inherits_NSObject.AlignedNSClass)
+
+// CHECK-32: Type info:
+// CHECK-32-NEXT: (class_instance size=32 alignment=16 stride=32 num_extra_inhabitants=0
+// CHECK-32-NEXT:   (field name=w offset=12
+// CHECK-32-NEXT:     (struct size=4 alignment=4 stride=4 num_extra_inhabitants=0
+// CHECK-32-NEXT:       (field name=_value offset=0
+// CHECK-32-NEXT:         (builtin size=4 alignment=4 stride=4 num_extra_inhabitants=0))))
+// CHECK-32-NEXT:   (field name=x offset=16
+// CHECK-32-NEXT:     (builtin size=16 alignment=16 stride=16 num_extra_inhabitants=0)))
+
+class GenericAlignedNSClass<T> : NSObject {
+  var w: T = 0 as! T
+  var x: int4 = [1,2,3,4]
+}
+
+let genericAlignedClass = GenericAlignedNSClass<Int>()
+reflect(object: genericAlignedClass)
+
+// CHECK-64: Reflecting an object.
+// CHECK-64: Type reference:
+// CHECK-64: (bound_generic_class inherits_NSObject.GenericAlignedNSClass
+// CHECK-64:   (struct Swift.Int))
+
+// CHECK-64: Type info:
+// CHECK-64-NEXT: (class_instance size=48 alignment=16 stride=48 num_extra_inhabitants=0
+// CHECK-64-NEXT:   (field name=w offset=16
+// CHECK-64-NEXT:     (struct size=8 alignment=8 stride=8 num_extra_inhabitants=0
+// CHECK-64-NEXT:       (field name=_value offset=0
+// CHECK-64-NEXT:         (builtin size=8 alignment=8 stride=8 num_extra_inhabitants=0))))
+// CHECK-64-NEXT:   (field name=x offset=32
+// CHECK-64-NEXT:     (builtin size=16 alignment=16 stride=16 num_extra_inhabitants=0)))
+
+// CHECK-32: Reflecting an object.
+// CHECK-32: Type reference:
+// CHECK-32: (bound_generic_class inherits_NSObject.GenericAlignedNSClass
+// CHECK-32:   (struct Swift.Int))
+
+// CHECK-32: Type info:
+// CHECK-32-NEXT: (class_instance size=48 alignment=16 stride=48 num_extra_inhabitants=0
+// CHECK-32-NEXT:   (field name=w offset=16
+// CHECK-32-NEXT:     (struct size=4 alignment=4 stride=4 num_extra_inhabitants=0
+// CHECK-32-NEXT:       (field name=_value offset=0
+// CHECK-32-NEXT:         (builtin size=4 alignment=4 stride=4 num_extra_inhabitants=0))))
+// CHECK-32-NEXT:   (field name=x offset=32
+// CHECK-32-NEXT:     (builtin size=16 alignment=16 stride=16 num_extra_inhabitants=0)))
+
 doneReflecting()
diff --git a/validation-test/Reflection/inherits_ObjCClasses.swift b/validation-test/Reflection/inherits_ObjCClasses.swift

Original file line number	Diff line number	Diff line change
`@@ -1251,8 +1251,7 @@ const TypeInfo TypeConverter::getTypeInfo(const TypeRef TR) {`
`1251`	`1251`	`}`
`1252`	`1252`
`1253`	`1253`	`const TypeInfo TypeConverter::getClassInstanceTypeInfo(const TypeRef TR,`
`1254`		`- unsigned start,`
`1255`		`- unsigned align) {`
	`1254`	`+ unsigned start) {`
`1256`	`1255`	`const FieldDescriptor *FD = getBuilder().getFieldTypeInfo(TR);`
`1257`	`1256`	`if (FD == nullptr) {`
`1258`	`1257`	`DEBUG(std::cerr << "No field descriptor: "; TR->dump());`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module ObjCClasses {`
	`2`	`+ header "ObjCClasses.h"`
	`3`	`+}`