Always, no, never... forget to check your references

bitjammer · bitjammer · commit 10c44e1a3ac8 · 2016-08-16T11:43:58.000-07:00
for weak semantics, that is! 94a9c51 made some changes to loading weak references by adding some information in the lower bits with respect to locking. These bits need to be masked out when performing a load, such as when we want to get the metadata pointer for a class instance. This normally works fine when going through the normal weak loading functions in the runtime. When the runtime function swift_ClassMirror_subscript gets the offset of one of its stored properties, it immediately packages it into the the ad-hoc existential container, known as just `Mirror` in the runtime. However, the weak reference isn't aligned! It has bit 1 set. We weren't loading the weak reference here as we would during normal SILGen, such as with a weak_load instruction. Simulate that here and make the reference strong before putting it into the Mirror container, which also clears those lower bits. rdar://problem/27475034 There are still a couple of other cases to handle, namely the unowned(safe) and unowned(unsafe) reference kinds. There may be other places where an unaligned pointer is problematic in the runtime, which we should audit for correctness. rdar://problem/27809991
diff --git a/include/swift/ABI/MetadataValues.h b/include/swift/ABI/MetadataValues.h
@@ -520,6 +520,7 @@ class FieldType {
   // some high bits as well.
   enum : int_type {
     Indirect = 1,
+    Weak = 2,
 
     TypeMask = ((uintptr_t)-1) & ~(alignof(void*) - 1),
   };
@@ -537,10 +538,19 @@ class FieldType {
                      | (indirect ? Indirect : 0));
   }
 
+  constexpr FieldType withWeak(bool weak) const {
+    return FieldType((Data & ~Weak)
+                     | (weak ? Weak : 0));
+  }
+
   bool isIndirect() const {
     return bool(Data & Indirect);
   }
 
+  bool isWeak() const {
+    return bool(Data & Weak);
+  }
+
   const Metadata *getType() const {
     return (const Metadata *)(Data & TypeMask);
   }
diff --git a/lib/IRGen/GenMeta.cpp b/lib/IRGen/GenMeta.cpp
@@ -2414,8 +2414,10 @@ namespace {
                          NominalTypeDecl::StoredPropertyRange storedProperties){
     SmallVector<FieldTypeInfo, 4> types;
     for (VarDecl *prop : storedProperties) {
-      types.push_back(FieldTypeInfo(prop->getType()->getCanonicalType(),
-                                    /*indirect*/ false));
+      auto propertyType = prop->getType()->getCanonicalType();
+      types.push_back(FieldTypeInfo(propertyType,
+                                    /*indirect*/ false,
+                                    propertyType->is<WeakStorageType>()));
     }
     return getFieldTypeAccessorFn(IGM, type, types);
   }
@@ -2431,7 +2433,7 @@ namespace {
       auto caseType = elt.decl->getArgumentType()->getCanonicalType();
       bool isIndirect = elt.decl->isIndirect()
         || elt.decl->getParentEnum()->isIndirect();
-      types.push_back(FieldTypeInfo(caseType, isIndirect));
+      types.push_back(FieldTypeInfo(caseType, isIndirect, /*weak*/ false));
     }
     return getFieldTypeAccessorFn(IGM, type, types);
   }
@@ -2784,9 +2786,13 @@ irgen::emitFieldTypeAccessor(IRGenModule &IGM,
     
     auto metadata = IGF.emitTypeMetadataRef(fieldTy);
 
+    auto fieldTypeInfo = fieldTypes[i];
+
     // Mix in flag bits.
-    if (fieldTypes[i].isIndirect()) {
-      auto flags = FieldType().withIndirect(true);
+    if (fieldTypeInfo.hasFlags()) {
+      auto flags = FieldType()
+        .withIndirect(fieldTypeInfo.isIndirect())
+        .withWeak(fieldTypeInfo.isWeak());
       auto metadataBits = IGF.Builder.CreatePtrToInt(metadata, IGF.IGM.SizeTy);
       metadataBits = IGF.Builder.CreateOr(metadataBits,
                    llvm::ConstantInt::get(IGF.IGM.SizeTy, flags.getIntValue()));
diff --git a/lib/IRGen/IRGenModule.h b/lib/IRGen/IRGenModule.h
@@ -133,25 +133,30 @@ class IRGenModule;
 
 /// A type descriptor for a field type accessor.
 class FieldTypeInfo {
-  llvm::PointerIntPair<CanType, 1, unsigned> Info;
+  llvm::PointerIntPair<CanType, 2, unsigned> Info;
   /// Bits in the "int" part of the Info pair.
   enum : unsigned {
     /// Flag indicates that the case is indirectly stored in a box.
     Indirect = 1,
+    /// Indicates a weak optional reference
+    Weak = 2,
   };
 
-  static unsigned getFlags(bool indirect) {
-    return (indirect ? Indirect : 0);
+  static unsigned getFlags(bool indirect, bool weak) {
+    return (indirect ? Indirect : 0)
+         | (weak ? Weak : 0);
     //   | (blah ? Blah : 0) ...
   }
 
 public:
-  FieldTypeInfo(CanType type, bool indirect)
-    : Info(type, getFlags(indirect))
+  FieldTypeInfo(CanType type, bool indirect, bool weak)
+    : Info(type, getFlags(indirect, weak))
   {}
 
   CanType getType() const { return Info.getPointer(); }
   bool isIndirect() const { return Info.getInt() & Indirect; }
+  bool isWeak() const { return Info.getInt() & Weak; }
+  bool hasFlags() const { return Info.getInt() != 0; }
 };
 
 /// The principal singleton which manages all of IR generation.
diff --git a/stdlib/public/runtime/Reflection.mm b/stdlib/public/runtime/Reflection.mm
@@ -398,6 +398,40 @@ void swift_TupleMirror_subscript(String *outString,
   return fieldName;
 }
 
+
+static bool loadSpecialReferenceStorage(HeapObject *owner,
+                                        OpaqueValue *fieldData,
+                                        const FieldType fieldType,
+                                        Mirror *outMirror) {
+  // TODO: Switch over the other kinds of reference storage here:
+  // - unowned(safe)
+  // - unowned(unsafe)
+  // - class-bound existentials
+  // - Optional existential types
+  // This will require a change in the two low flag bits in the field type
+  // returned from the field type accessor generated by IRGen.
+
+  // isWeak() implies a reference type via Sema.
+  if (!fieldType.isWeak())
+    return false;
+
+  auto weakField = reinterpret_cast<WeakReference *>(fieldData);
+  auto strongValue = swift_unknownWeakLoadStrong(weakField);
+  fieldData = reinterpret_cast<OpaqueValue *>(&strongValue);
+
+  // This MagicMirror constructor creates a box to hold the loaded refernce
+  // value, which becomes the new owner for the value.
+  new (outMirror) MagicMirror(fieldData, fieldType.getType(), /*take*/ true);
+
+  // However, swift_StructMirror_subscript and swift_ClassMirror_subscript
+  // requires that the owner be consumed. Since we have the new heap box as the
+  // owner now, we need to release the old owner to maintain the contract.
+  if (owner->metadata->isAnyClass())
+    swift_unknownRelease(owner);
+
+  return true;
+}
+
 // -- Struct destructuring.
   
 SWIFT_CC(swift) SWIFT_RUNTIME_STDLIB_INTERFACE
@@ -416,7 +450,7 @@ void swift_StructMirror_subscript(String *outString,
                                   Mirror *outMirror,
                                   intptr_t i,
                                   HeapObject *owner,
-                                  const OpaqueValue *value,
+                                  OpaqueValue *value,
                                   const Metadata *type) {
   auto Struct = static_cast<const StructMetadata *>(type);
   
@@ -427,13 +461,17 @@ void swift_StructMirror_subscript(String *outString,
   auto fieldType = Struct->getFieldTypes()[i];
   auto fieldOffset = Struct->getFieldOffsets()[i];
   
-  auto bytes = reinterpret_cast<const char*>(value);
-  auto fieldData = reinterpret_cast<const OpaqueValue *>(bytes + fieldOffset);
+  auto bytes = reinterpret_cast<char*>(value);
+  auto fieldData = reinterpret_cast<OpaqueValue *>(bytes + fieldOffset);
 
   new (outString) String(getFieldName(Struct->Description->Struct.FieldNames, i));
 
   // 'owner' is consumed by this call.
   assert(!fieldType.isIndirect() && "indirect struct fields not implemented");
+
+  if (loadSpecialReferenceStorage(owner, fieldData, fieldType, outMirror))
+    return;
+
   new (outMirror) Mirror(reflect(owner, fieldData, fieldType.getType()));
 }
 
@@ -614,7 +652,7 @@ void swift_ClassMirror_subscript(String *outString,
                                  Mirror *outMirror,
                                  intptr_t i,
                                  HeapObject *owner,
-                                 const OpaqueValue *value,
+                                 OpaqueValue *value,
                                  const Metadata *type) {
   auto Clas = static_cast<const ClassMetadata*>(type);
 
@@ -655,10 +693,15 @@ void swift_ClassMirror_subscript(String *outString,
 #endif
   }
   
-  auto bytes = *reinterpret_cast<const char * const*>(value);
-  auto fieldData = reinterpret_cast<const OpaqueValue *>(bytes + fieldOffset);
-  
-  new (outString) String(getFieldName(Clas->getDescription()->Class.FieldNames, i));
+  auto bytes = *reinterpret_cast<char * const *>(value);
+  auto fieldData = reinterpret_cast<OpaqueValue *>(bytes + fieldOffset);
+
+  new (outString) String(getFieldName(Clas->getDescription()->Class.FieldNames,
+                                      i));
+
+ if (loadSpecialReferenceStorage(owner, fieldData, fieldType, outMirror))
+   return;
+
   // 'owner' is consumed by this call.
   new (outMirror) Mirror(reflect(owner, fieldData, fieldType.getType()));
 }
diff --git a/test/1_stdlib/WeakMirror.swift b/test/1_stdlib/WeakMirror.swift
@@ -0,0 +1,177 @@
+//===--- Mirror.swift -----------------------------------------------------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See http://swift.org/LICENSE.txt for license information
+// See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+//
+// RUN: if [ %target-runtime == "objc" ]; \
+// RUN: then \
+// RUN:   %target-clang %S/Inputs/Mirror/Mirror.mm -c -o %t/Mirror.mm.o -g && \
+// RUN:   %target-build-swift -Xfrontend -disable-access-control %s -I %S/Inputs/Mirror/ -Xlinker %t/Mirror.mm.o -o %t/Mirror; \
+// RUN: else \
+// RUN:   %target-build-swift %s -Xfrontend -disable-access-control -o %t/Mirror; \
+// RUN: fi
+// RUN: %target-run %t/Mirror
+// REQUIRES: executable_test
+
+import StdlibUnittest
+
+var mirrors = TestSuite("Mirrors")
+
+class NativeSwiftClass : NativeClassBoundExistential {}
+
+protocol NativeClassBoundExistential : class {}
+class NativeSwiftClassHasWeak {
+  weak var weakProperty: AnyObject?
+  let x = 0xAABBCCDDEE
+}
+
+class NativeSwiftClassHasNativeClassBoundExistential {
+  weak var weakProperty: NativeClassBoundExistential?
+  let x = 0xAABBCCDDEE
+}
+
+struct StructHasNativeWeakReference {
+  weak var weakProperty: AnyObject?
+  let x = 0xAABBCCDDEE
+}
+
+mirrors.test("class/NativeSwiftClassHasNativeWeakReference") {
+  let c1 = NativeSwiftClassHasWeak()
+  let c2 = NativeSwiftClass()
+  c1.weakProperty = c2
+  let classChild = _reflect(c1)[0].1.value
+  print(classChild)
+}
+
+mirrors.test("class/NativeSwiftClassHasNativeClassBoundExistential") {
+  let c1 = NativeSwiftClassHasNativeClassBoundExistential()
+  let e = NativeSwiftClass() as NativeClassBoundExistential
+  c1.weakProperty = e
+  let classChild = _reflect(c1)[0].1.value
+  print(classChild)
+}
+
+mirrors.test("struct/StructHasNativeWeakReference") {
+  var s = StructHasNativeWeakReference()
+  let c2 = NativeSwiftClass()
+  s.weakProperty = c2
+  let structChild = _reflect(s)[0].1.value
+  print(structChild)
+}
+
+#if _runtime(_ObjC)
+
+import Foundation
+
+@objc protocol ObjCClassBoundExistential : class {}
+
+@objc protocol ObjCProtocol : class {
+  weak var weakProperty: AnyObject? { get set }
+}
+
+class NativeSwiftClassHasObjCClassBoundExistential {
+  weak var weakProperty: NSObjectProtocol?
+  let x = 0xAABBCCDDEE
+}
+
+class ObjCClassHasWeak : NSObject {
+  weak var weakProperty: AnyObject?
+  let x = 0xAABBCCDDEE
+}
+
+class ObjCClassHasNativeClassBoundExistential : NSObject {
+  weak var weakProperty: NativeClassBoundExistential?
+  let x = 0xAABBCCDDEE
+}
+
+class ObjCClassHasObjCClassBoundExistential : NSObject {
+  weak var weakProperty: NSObjectProtocol?
+  let x = 0xAABBCCDDEE
+}
+
+struct StructHasObjCWeakReference {
+  weak var weakProperty: NSObject?
+  let x = 0xAABBCCDDEE
+}
+
+struct StructHasObjCClassBoundExistential {
+  weak var weakProperty: NSObjectProtocol?
+  let x = 0xAABBCCDDEE
+}
+
+mirrors.test("class/NativeSwiftClassHasObjCWeakReference") {
+  let c1 = NativeSwiftClassHasWeak()
+  let nso = NSObject()
+  c1.weakProperty = nso
+  let classChild = _reflect(c1)[0].1.value
+  print(classChild)
+}
+
+mirrors.test("class/NativeSwiftClassHasObjCClassBoundExistential") {
+  let c1 = NativeSwiftClassHasObjCClassBoundExistential()
+  let nso = NSObject() as NSObjectProtocol
+  c1.weakProperty = nso
+  let classChild = _reflect(c1)[0].1.value
+  print(classChild)
+}
+
+mirrors.test("class/ObjCClassHasNativeWeak") {
+  let c1 = ObjCClassHasWeak()
+  let c2 = NativeSwiftClass()
+  c1.weakProperty = c2
+  let classChild = _reflect(c1)[0].1.value
+  print(classChild)
+}
+
+mirrors.test("class/ObjcCClassHasObjCWeakReference") {
+  let c1 = ObjCClassHasWeak()
+  let nso = NSObject()
+  c1.weakProperty = nso
+  let classChild = _reflect(c1)[0].1.value
+  print(classChild)
+}
+
+mirrors.test("class/ObjCClassHasNativeClassBoundExistential") {
+  let c1 = ObjCClassHasNativeClassBoundExistential()
+  let e = NativeSwiftClass() as NativeClassBoundExistential
+  c1.weakProperty = e
+  let classChild = _reflect(c1)[0].1.value
+  print(classChild)
+}
+
+mirrors.test("class/ObjCClassHasObjCClassBoundExistential") {
+  let c1 = ObjCClassHasObjCClassBoundExistential()
+  let nsop = NSObject() as NSObjectProtocol
+  c1.weakProperty = nsop
+  let classChild = _reflect(c1)[0].1.value
+  print(classChild)
+}
+
+mirrors.test("struct/StructHasObjCWeakReference") {
+  var s = StructHasObjCWeakReference()
+  let nso = NSObject()
+  s.weakProperty = nso
+  let structChild = _reflect(s)[0].1.value
+  print(structChild)
+}
+
+mirrors.test("struct/StructHasObjCClassBoundExistential") {
+  var s = StructHasObjCClassBoundExistential()
+  let nsop = NSObject() as NSObjectProtocol
+  s.weakProperty = nsop
+  let structChild = _reflect(s)[0].1.value
+  print(structChild)
+}
+
+#endif
+
+runAllTests()
