Remove Builtin.UnknownObject completely #12325

jrose-apple · 2017-10-07T04:06:50Z

The runtime now provides prebuilt value witnesses for AnyObject (mangled name yXl).
The demangler and remangler continue to support "BO" because it's convenient to be able to demangle old symbols.
Reflection still uses the notion of "unknown object" to mean an object with unknown refcounting. That's different from AnyObject because AnyObject is still an existential, meaning it contains an object with unknown refcounting.
The reflection tests are broken at the moment.
Type-based alias analysis for Builtin.UnknownObject was incorrect, so it's a good thing we weren't using it.
Same with enum layout. (This one assumed UnknownObject never referred to an Objective-C tagged pointer. That certainly wasn't how we were using it!)

- The runtime now provides prebuilt value witnesses for AnyObject (mangled name yXl). - The demangler and remangler continue to support "BO" because it's convenient to be able to demangle old symbols. - Reflection still uses the notion of "unknown object" to mean an object with unknown refcounting. That's different from AnyObject because AnyObject is still an existential, meaning it /contains/ an object with unknown refcounting. - The reflection tests are broken at the moment. - Type-based alias analysis for Builtin.UnknownObject was incorrect, so it's a good thing we weren't using it. - Same with enum layout. (This one assumed UnknownObject never referred to an Objective-C tagged pointer. That certainly wasn't how we were using it!)

jrose-apple · 2017-10-07T04:06:59Z

@swift-ci Please test

jrose-apple

The reflection tests are busted; I just get "invalid lowering" when I try to use yXl.

jrose-apple · 2017-10-07T04:07:24Z

test/Reflection/typeref_lowering.swift

@@ -3,6 +3,8 @@
 // RUN: %target-build-swift %S/Inputs/TypeLowering.swift -parse-as-library -emit-module -emit-library -module-name TypeLowering -o %t/libTypesToReflect.%target-dylib-extension
 // RUN: %target-swift-reflection-dump -binary-filename %t/libTypesToReflect.%target-dylib-extension -binary-filename %platform-module-dir/libswiftCore.%target-dylib-extension -dump-type-lowering < %s | %FileCheck %s --check-prefix=CHECK-%target-ptrsize

+// REQUIRES: UnknownObject_removal


jrose-apple · 2017-10-07T04:07:29Z

test/Reflection/typeref_lowering_objc.swift

@@ -4,6 +4,7 @@

 // REQUIRES: objc_interop
 // REQUIRES: CPU=x86_64
+// REQUIRES: UnknownObject_removal


swift-ci · 2017-10-07T04:26:16Z

Build failed
Swift Test Linux Platform
Git Sha - 238215c

slavapestov · 2017-10-07T04:48:03Z

lib/IRGen/GenReflection.cpp

@@ -906,7 +906,6 @@ emitAssociatedTypeMetadataRecord(const ProtocolConformance *Conformance) {
 void IRGenModule::emitBuiltinReflectionMetadata() {
  if (getSwiftModule()->isStdlibModule()) {
    BuiltinTypes.insert(Context.TheNativeObjectType);
-    BuiltinTypes.insert(Context.TheUnknownObjectType);


Can you try something like BuiltinTypes.insert(Context.getAnyObjectType()->getCanonicalType())?

... it would have to emit the descriptor under the old name "BO" too. Also see my comments in Reflection/TypeLowering.cpp

slavapestov · 2017-10-07T04:50:02Z

@jrose-apple This is awesome, but I'm worried we're going to break reflection when binaries built with Swift 4.1 are used with an older version of the library.

The way it works is for "primitive" types, we emit "builtin type descriptors" which just state the size, alignment and extra inhabitants of the type.

You changed it to no longer emit this descriptor for UnknownObject, which makes sense, but I think it should now emit a descriptor for AnyObject, but probably under the old name so that the old library can find it.

slavapestov · 2017-10-07T04:52:56Z

stdlib/public/Reflection/TypeLowering.cpp

@@ -607,7 +607,7 @@ const TypeRef *TypeConverter::getUnknownObjectTypeRef() {
  if (UnknownObjectTR != nullptr)
    return UnknownObjectTR;

-  UnknownObjectTR = BuiltinTypeRef::create(Builder, "BO");
+  UnknownObjectTR = BuiltinTypeRef::create(Builder, "yXl");


Ok, keep this as "BO" -- the mangled name here doesn't 'mean' anything, it's just used to find the descriptor emitted in IRGen/GenReflection

slavapestov · 2017-10-07T04:53:14Z

stdlib/public/Reflection/TypeLowering.cpp

@@ -1048,9 +1048,6 @@ class LowerType
    if (B->getMangledName() == "Bo") {
      return TC.getReferenceTypeInfo(ReferenceKind::Strong,
                                     ReferenceCounting::Native);
-    } else if (B->getMangledName() == "BO") {


And keep this too, maybe add a comment that "BO" is no longer a real type

swift-ci · 2017-10-07T05:04:03Z

Build failed
Swift Test OS X Platform
Git Sha - 238215c

slavapestov · 2017-10-07T05:39:25Z

@jrose-apple Take a look at lldb too, thankfully most of the usages look trivially removable:

source/Symbol/SwiftASTContext.cpp:    case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:                       m_ast_context_ap->TheUnknownObjectType.getPointer());
source/Symbol/SwiftASTContext.cpp:            referent_type->is<swift::BuiltinUnknownObjectType>() ||
source/Symbol/SwiftASTContext.cpp:    if (can_type == GetASTContext()->TheUnknownObjectType.getPointer())
source/Symbol/SwiftASTContext.cpp:      type_kind == swift::TypeKind::BuiltinUnknownObject)
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:    case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:    case swift::TypeKind::BuiltinUnknownObject:
source/Symbol/SwiftASTContext.cpp:  case swift::TypeKind::BuiltinUnknownObject:

jrose-apple · 2017-10-09T16:36:09Z

Oops, thanks for the LLDB reminder! I'll probably need to talk to you about yXl vs BO, but this isn't high-priority.

jrose-apple · 2019-09-26T17:11:27Z

Closing in favor of #27378.

jrose-apple requested review from slavapestov and jckarter October 7, 2017 04:06

jrose-apple commented Oct 7, 2017

View reviewed changes

slavapestov reviewed Oct 7, 2017

View reviewed changes

jrose-apple mentioned this pull request Sep 26, 2019

Eliminate Builtin.UnknownObject as an AST type #27378

Merged

jrose-apple closed this Sep 26, 2019

typesanitizer mentioned this pull request Sep 24, 2019

[SR-5926] Remove Builtin.UnknownObject from the AST and SIL layers #48485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove Builtin.UnknownObject completely #12325

Remove Builtin.UnknownObject completely #12325

Uh oh!

jrose-apple commented Oct 7, 2017

Uh oh!

jrose-apple commented Oct 7, 2017

Uh oh!

jrose-apple left a comment

Uh oh!

jrose-apple Oct 7, 2017

Uh oh!

jrose-apple Oct 7, 2017

Uh oh!

swift-ci commented Oct 7, 2017

Uh oh!

slavapestov Oct 7, 2017

Uh oh!

slavapestov Oct 7, 2017

Uh oh!

slavapestov commented Oct 7, 2017

Uh oh!

slavapestov Oct 7, 2017

Uh oh!

slavapestov Oct 7, 2017

Uh oh!

swift-ci commented Oct 7, 2017

Uh oh!

slavapestov commented Oct 7, 2017

Uh oh!

jrose-apple commented Oct 9, 2017

Uh oh!

jrose-apple commented Sep 26, 2019

Uh oh!

Uh oh!

Remove Builtin.UnknownObject completely #12325

Remove Builtin.UnknownObject completely #12325

Uh oh!

Conversation

jrose-apple commented Oct 7, 2017

Uh oh!

jrose-apple commented Oct 7, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

jrose-apple Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Oct 7, 2017

Uh oh!

slavapestov Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

slavapestov Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Oct 7, 2017

Uh oh!

slavapestov Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

slavapestov Oct 7, 2017

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Oct 7, 2017

Uh oh!

slavapestov commented Oct 7, 2017

Uh oh!

jrose-apple commented Oct 9, 2017

Uh oh!

jrose-apple commented Sep 26, 2019

Uh oh!

Uh oh!