From b84f01d5382584fa0c0a1c786087f4d1b8c533aa Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Fri, 9 Sep 2022 18:11:05 -0700 Subject: [PATCH 1/2] model the ABISafeConversionComponent as a translation component, rather than physical Also renaming it to be UncheckedConversionComponent since it's a better name. As a physical component, we'd run into problems in assignment statements. The problem was that if we had something like: ``` SomeOtherComponent // first component GetterSetterComponent ABISafeConversionComponent // last component ``` When emitting the assignment, we always drill down through all but the last component by calling `project()` on each one. Then on the last component, we'd do the actual setting operation. But GetterSetterComponent cannot be projected when the access is for writing. So, to work around this I decided to model it as a TranslationComponent, because those are specifically designed to be handled during an assignment by popping those off the end of the component sequence, untranslating the value we're about to assign as we go, until we hit the GetterSetterComponent. By "untranslating" we're effectively putting Sendable back onto the set's argument prior to calling set, because the underlying property's type still has `@Sendable` on it (e.g., it's accessors still have that on its argument type). When "translating" we're effectively taking Sendable off after reading it. I think actually works really well and makes much more sense now. resolves rdar://99619834 --- lib/SILGen/LValue.h | 2 +- lib/SILGen/SILGenBuilder.cpp | 3 +- lib/SILGen/SILGenLValue.cpp | 86 ++++++++++++++++++++++++++++++------ 3 files changed, 75 insertions(+), 16 deletions(-) diff --git a/lib/SILGen/LValue.h b/lib/SILGen/LValue.h index 9b16da86bedcb..32d02f9efccfe 100644 --- a/lib/SILGen/LValue.h +++ b/lib/SILGen/LValue.h @@ -105,7 +105,6 @@ class PathComponent { CoroutineAccessorKind, // coroutine accessor ValueKind, // random base pointer as an lvalue PhysicalKeyPathApplicationKind, // applying a key path - ABISafeConversionKind, // unchecked_addr_cast // Logical LValue kinds GetterSetterKind, // property or subscript getter/setter @@ -118,6 +117,7 @@ class PathComponent { // Translation LValue kinds (a subtype of logical) OrigToSubstKind, // generic type substitution SubstToOrigKind, // generic type substitution + UncheckedConversionKind, // unchecked_X_cast FirstLogicalKind = GetterSetterKind, FirstTranslationKind = OrigToSubstKind, diff --git a/lib/SILGen/SILGenBuilder.cpp b/lib/SILGen/SILGenBuilder.cpp index 9a1508a94e6f1..afb59e47fbdd2 100644 --- a/lib/SILGen/SILGenBuilder.cpp +++ b/lib/SILGen/SILGenBuilder.cpp @@ -620,7 +620,8 @@ ManagedValue SILGenBuilder::createUncheckedBitCast(SILLocation loc, // updated. assert((isa(cast) || isa(cast) || - isa(cast)) && + isa(cast) || + isa(cast)) && "SILGenBuilder is out of sync with SILBuilder."); // If we have a trivial inst, just return early. diff --git a/lib/SILGen/SILGenLValue.cpp b/lib/SILGen/SILGenLValue.cpp index 6c15d8d30bc02..973d47ef04c98 100644 --- a/lib/SILGen/SILGenLValue.cpp +++ b/lib/SILGen/SILGenLValue.cpp @@ -2204,26 +2204,82 @@ namespace { } }; - /// A physical component which performs an unchecked_addr_cast - class ABISafeConversionComponent final : public PhysicalPathComponent { + /// A translation component that performs \c unchecked_*_cast 's as-needed. + class UncheckedConversionComponent final : public TranslationPathComponent { + private: + Type OrigType; + + /// \returns the type this component is trying to convert \b to + CanType getTranslatedType() const { + return getTypeData().SubstFormalType->getCanonicalType(); + } + + /// \returns the type this component is trying to convert \b from + CanType getUntranslatedType() const { + return OrigType->getRValueType()->getCanonicalType(); + } + + /// perform a conversion of ManagedValue -> ManagedValue + ManagedValue doUncheckedConversion(SILGenFunction &SGF, SILLocation loc, + ManagedValue val, CanType toType) { + auto toTy = SGF.getLoweredType(toType); + auto fromTy = val.getType(); + + if (fromTy == toTy) + return val; // nothing to do. + + // otherwise emit the right kind of cast based on whether it's an address. + assert(fromTy.isAddress() == toTy.isAddress()); + + if (toTy.isAddress()) + return SGF.B.createUncheckedAddrCast(loc, val, toTy); + + return SGF.B.createUncheckedBitCast(loc, val, toTy); + } + + /// perform a conversion of RValue -> RValue + RValue doUncheckedConversion(SILGenFunction &SGF, SILLocation loc, + RValue &&rv, CanType toType) { + auto val = std::move(rv).getAsSingleValue(SGF, loc); + val = doUncheckedConversion(SGF, loc, val, toType); + return RValue(SGF, loc, toType, val); + } + public: - ABISafeConversionComponent(LValueTypeData typeData) - : PhysicalPathComponent(typeData, ABISafeConversionKind, - /*actorIsolation=*/None) {} + /// \param OrigType is the type we are converting \b from + /// \param typeData will contain the type we are converting \b to + UncheckedConversionComponent(LValueTypeData typeData, Type OrigType) + : TranslationPathComponent(typeData, UncheckedConversionKind), + OrigType(OrigType) {} - ManagedValue project(SILGenFunction &SGF, SILLocation loc, - ManagedValue base) && override { - auto toType = SGF.getLoweredType(getTypeData().SubstFormalType) - .getAddressType(); + bool isLoadingPure() const override { return true; } - if (base.getType() == toType) - return base; // nothing to do + /// Used during write operations to convert the value prior to writing to + /// the base. + RValue untranslate(SILGenFunction &SGF, SILLocation loc, + RValue &&rv, SGFContext c) && override { + return doUncheckedConversion(SGF, loc, std::move(rv), + getUntranslatedType()); + } - return SGF.B.createUncheckedAddrCast(loc, base, toType); + /// Used during read operations to convert the value after reading the base. + RValue translate(SILGenFunction &SGF, SILLocation loc, + RValue &&rv, SGFContext c) && override { + return doUncheckedConversion(SGF, loc, std::move(rv), + getTranslatedType()); + } + + std::unique_ptr + clone(SILGenFunction &SGF, SILLocation loc) const override { + return std::make_unique(getTypeData(), + OrigType); } void dump(raw_ostream &OS, unsigned indent) const override { - OS.indent(indent) << "ABISafeConversionComponent\n"; + OS.indent(indent) << "UncheckedConversionComponent" + << "\n\tfromType: " << getUntranslatedType() + << "\n\ttoType: " << getTranslatedType() + << "\n"; } }; } // end anonymous namespace @@ -3741,7 +3797,9 @@ LValue SILGenLValue::visitABISafeConversionExpr(ABISafeConversionExpr *e, LValue lval = visitRec(e->getSubExpr(), accessKind, options); auto typeData = getValueTypeData(SGF, accessKind, e); - lval.add(typeData); + auto OrigType = e->getSubExpr()->getType(); + + lval.add(typeData, OrigType); return lval; } From 1b3cb9f80ebc41454ebd0546909e97b49238a08f Mon Sep 17 00:00:00 2001 From: Kavon Farvardin Date: Fri, 9 Sep 2022 20:54:31 -0700 Subject: [PATCH 2/2] update tests to cover rdar://99619834 --- test/SILGen/objc_preconcurrency.swift | 121 +++++++++++++++++++++++++- 1 file changed, 118 insertions(+), 3 deletions(-) diff --git a/test/SILGen/objc_preconcurrency.swift b/test/SILGen/objc_preconcurrency.swift index 69fa8b9474703..3b577a984b184 100644 --- a/test/SILGen/objc_preconcurrency.swift +++ b/test/SILGen/objc_preconcurrency.swift @@ -7,6 +7,13 @@ @preconcurrency var sendyHandler: @Sendable () -> Void { get set } } +@preconcurrency class OldWorld { + @preconcurrency var handler: (@Sendable () -> Void)? + @preconcurrency var mainHandler: (@MainActor () -> Void)? + @preconcurrency var nonOptionalHandler: @Sendable () -> Void = {} + @preconcurrency var nonOptionalMainHandler: @MainActor () -> Void = {} +} + // CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency19testDynamicDispatch1p17completionHandleryAA1P_p_yyctF // CHECK: dynamic_method_br // CHECK: bb{{[0-9]+}}(%{{[0-9]+}} : $@convention(objc_method) (@convention(block) @Sendable () -> (), @opened @@ -19,18 +26,121 @@ func testDynamicDispatch(p: P, completionHandler: @escaping () -> Void) { } // CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency21testOptionalVarAccessyySo12NSTouchGrassCF -// CHECK: unchecked_addr_cast {{.*}} : $*Optional<@Sendable @callee_guaranteed () -> ()> to $*Optional<@callee_guaranteed () -> ()> +// CHECK: unchecked_bitwise_cast {{.*}} : $Optional<@Sendable @callee_guaranteed () -> ()> to $Optional<@callee_guaranteed () -> ()> // CHECK: } // end sil function '$s19objc_preconcurrency21testOptionalVarAccessyySo12NSTouchGrassCF' func testOptionalVarAccess(_ grass: NSTouchGrass) { grass.cancellationHandler?() } +// CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency33testOptionalVarAccessPartialApplyyyycSgSo12NSTouchGrassCF +// CHECK: unchecked_bitwise_cast {{.*}} : $Optional<@Sendable @callee_guaranteed () -> ()> to $Optional<@callee_guaranteed () -> ()> +// CHECK: } // end sil function '$s19objc_preconcurrency33testOptionalVarAccessPartialApplyyyycSgSo12NSTouchGrassCF' +func testOptionalVarAccessPartialApply(_ grass: NSTouchGrass) -> (() -> Void)? { + let handler = grass.cancellationHandler + if let unwrapped = handler { + unwrapped() + } + return handler +} + +// CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency16testObjCVarWriteyySo12NSTouchGrassCF +// CHECK: unchecked_bitwise_cast {{.*}} : $Optional<@callee_guaranteed () -> ()> to $Optional<@Sendable @callee_guaranteed () -> ()> +// CHECK: objc_method {{.*}} : $NSTouchGrass, #NSTouchGrass.cancellationHandler!setter.foreign : (NSTouchGrass) -> ((@Sendable () -> ())?) -> (), $@convention(objc_method) (Optional<@convention(block) @Sendable () -> ()>, NSTouchGrass) -> () +// CHECK: } // end sil function '$s19objc_preconcurrency16testObjCVarWriteyySo12NSTouchGrassCF' +func testObjCVarWrite(_ grass: NSTouchGrass) { + grass.cancellationHandler = {} +} + +// the below looks kinda long and wonky, but is expected. for a summary, the steps are: +// 1. objc to native +// 2. Sendable to non-Sendable (major part of this test) +// 3. non-optional to optional +// 4. from non-Sendable to Sendable (major part of this test) +// 5. from native to objc (which involves unwrapping and rewrapping that optional; kinda silly but optimization will clean it up) +// +// CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency22testObjCVarWriteAcrossyySo12NSTouchGrassCF +// CHECK: [[GET_EXCEPTION:%[0-9]+]] = objc_method {{.*}} : $NSTouchGrass, #NSTouchGrass.exceptionHandler!getter.foreign +// CHECK: [[SENDABLE_BLOCK:%[0-9]+]] = apply [[GET_EXCEPTION]]({{.*}}) : $@convention(objc_method) (NSTouchGrass) -> @autoreleased @convention(block) @Sendable () -> () +// << step 1 >> +// CHECK: [[NATIVE_THUNK:%[0-9]+]] = function_ref @$sIeyBh_Iegh_TR : $@convention(thin) @Sendable (@guaranteed @convention(block) @Sendable () -> ()) -> () +// CHECK: [[NATIVE_SENDABLE_EXCEPTION:%[0-9]+]] = partial_apply [callee_guaranteed] [[NATIVE_THUNK]]([[SENDABLE_BLOCK]]) +// << step 2 >> +// CHECK: [[NATIVE_EXCEPTION:%[0-9]+]] = convert_function [[NATIVE_SENDABLE_EXCEPTION]] : $@Sendable @callee_guaranteed () -> () to $@callee_guaranteed () -> () +// << step 3 >> +// CHECK: [[OPTIONAL_NATIVE_EXCEPTION:%[0-9]+]] = enum $Optional<@callee_guaranteed () -> ()>, #Optional.some!enumelt, [[NATIVE_EXCEPTION]] : $@callee_guaranteed () -> () +// << step 4 >> +// CHECK: = unchecked_bitwise_cast [[OPTIONAL_NATIVE_EXCEPTION]] : $Optional<@callee_guaranteed () -> ()> to $Optional<@Sendable @callee_guaranteed () -> ()> +// << step 5 >> +// CHECK: switch_enum {{.*}} : $Optional<@Sendable @callee_guaranteed () -> ()> +// +// CHECK: bb1({{.*}} : @owned $@Sendable @callee_guaranteed () -> ()): +// CHECK: init_block_storage_header {{.*}} : $*@block_storage @Sendable @callee_guaranteed () -> () +// CHECK: } // end sil function '$s19objc_preconcurrency22testObjCVarWriteAcrossyySo12NSTouchGrassCF' +func testObjCVarWriteAcross(_ grass: NSTouchGrass) { + grass.cancellationHandler = grass.exceptionHandler // *slaps roof of assignment* this bad boy can fit so much conversion in it! +} + +// CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency25testOptionalAssignSetter1yyAA8OldWorldCF +// CHECK: unchecked_bitwise_cast {{.*}} : $Optional<@callee_guaranteed () -> ()> to $Optional<@Sendable @callee_guaranteed () -> ()> +// CHECK: #OldWorld.handler!setter : (OldWorld) -> ((@Sendable () -> ())?) -> () +// CHECK: } // end sil function '$s19objc_preconcurrency25testOptionalAssignSetter1yyAA8OldWorldCF' +func testOptionalAssignSetter1(_ oldWorld: OldWorld) { + oldWorld.handler = {} +} + +// CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency25testOptionalAssignSetter2yyAA8OldWorldCF +// CHECK: convert_function {{.*}} : $@callee_guaranteed () -> () to $@Sendable @callee_guaranteed () -> () +// CHECK: $OldWorld, #OldWorld.nonOptionalHandler!setter : (OldWorld) -> (@escaping @Sendable () -> ()) -> () +// CHECK: } // end sil function '$s19objc_preconcurrency25testOptionalAssignSetter2yyAA8OldWorldCF' +func testOptionalAssignSetter2(_ oldWorld: OldWorld) { + oldWorld.nonOptionalHandler = {} +} + +// CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency21testMainHandlerWritesyyAA8OldWorldCF +// CHECK: = unchecked_bitwise_cast {{.*}} : $Optional<@callee_guaranteed () -> ()> to $Optional<@Sendable @callee_guaranteed () -> ()> +// CHECK: = unchecked_bitwise_cast {{.*}} : $Optional<@Sendable @callee_guaranteed () -> ()> to $Optional<@callee_guaranteed () -> ()> +// CHECK: } // end sil function '$s19objc_preconcurrency21testMainHandlerWritesyyAA8OldWorldCF' +func testMainHandlerWrites(_ oldWorld: OldWorld) { + oldWorld.handler = oldWorld.mainHandler + oldWorld.mainHandler = oldWorld.handler +} + +// CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency32testMainHandlerNonOptionalWritesyyAA8OldWorldCF +// CHECK: = convert_function {{.*}} : $@callee_guaranteed () -> () to $@Sendable @callee_guaranteed () -> () +// CHECK: = convert_function {{.*}} : $@Sendable @callee_guaranteed () -> () to $@callee_guaranteed () -> () +// CHECK: } // end sil function '$s19objc_preconcurrency32testMainHandlerNonOptionalWritesyyAA8OldWorldCF' +func testMainHandlerNonOptionalWrites(_ oldWorld: OldWorld) { + oldWorld.nonOptionalHandler = oldWorld.nonOptionalMainHandler + oldWorld.nonOptionalMainHandler = oldWorld.nonOptionalHandler +} + +// CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency15testMixedWritesyyAA8OldWorldCF +// +// << sendable conversions should be here >> +// CHECK: = unchecked_bitwise_cast {{.*}} : $Optional<@Sendable @callee_guaranteed () -> ()> to $Optional<@callee_guaranteed () -> ()> +// CHECK: = convert_function {{.*}} : $@callee_guaranteed () -> () to $@Sendable @callee_guaranteed () -> () +// +// << but main actor type mismatches are accepted by SIL >> +// CHECK: [[NO_MAIN_ACTOR:%[0-9]+]] = partial_apply {{.*}} : $@convention(thin) (@guaranteed @callee_guaranteed () -> @out ()) -> () +// CHECK: [[SETTER:%[0-9]+]] = class_method {{.*}} : $OldWorld, #OldWorld.nonOptionalMainHandler!setter : (OldWorld) -> (@escaping @MainActor () -> ()) -> (), $@convention(method) (@owned @callee_guaranteed () -> (), @guaranteed OldWorld) -> () +// CHECK: apply [[SETTER]]([[NO_MAIN_ACTOR]] +// CHECK: } // end sil function '$s19objc_preconcurrency15testMixedWritesyyAA8OldWorldCF' +func testMixedWrites(_ oldWorld: OldWorld) { + oldWorld.nonOptionalHandler = oldWorld.handler ?? {} + oldWorld.nonOptionalMainHandler = oldWorld.mainHandler ?? {} +} + func modify(_ v: inout () -> Void) { v = {} } // CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency15testInoutAccessyySo12NSTouchGrassCF -// CHECK: unchecked_addr_cast {{.*}} : $*@Sendable @callee_guaranteed () -> () to $*@callee_guaranteed () -> () +// CHECK: [[BEFORE_MODIFY:%[0-9]+]] = convert_function {{.*}} : $@Sendable @callee_guaranteed () -> () to $@callee_guaranteed () -> () +// CHECK: store [[BEFORE_MODIFY]] to [init] [[INOUT_ALLOC:%[0-9]+]] : $*@callee_guaranteed () -> () +// CHECK: [[MODIFY_FN:%[0-9]+]] = function_ref @$s19objc_preconcurrency6modifyyyyyczF : $@convention(thin) (@inout @callee_guaranteed () -> ()) -> () +// CHECK: = apply [[MODIFY_FN]]([[INOUT_ALLOC]]) +// CHECK: [[AFTER_MODIFY:%[0-9]+]] = load [take] [[INOUT_ALLOC]] : $*@callee_guaranteed () -> () +// CHECK: convert_function [[AFTER_MODIFY]] : $@callee_guaranteed () -> () to $@Sendable @callee_guaranteed () -> () // CHECK: } // end sil function '$s19objc_preconcurrency15testInoutAccessyySo12NSTouchGrassCF' func testInoutAccess(_ grass: NSTouchGrass) { modify(&grass.exceptionHandler) @@ -38,7 +148,12 @@ func testInoutAccess(_ grass: NSTouchGrass) { // CHECK-LABEL: sil hidden [ossa] @$s19objc_preconcurrency21testProtocolVarAccess1pyAA1P_p_tF -// CHECK: unchecked_addr_cast {{.*}} : $*@Sendable @callee_guaranteed () -> () to $*@callee_guaranteed () -> () +// CHECK: [[BEFORE_MODIFY:%[0-9]+]] = convert_function {{.*}} : $@Sendable @callee_guaranteed () -> () to $@callee_guaranteed () -> () +// CHECK: store [[BEFORE_MODIFY]] to [init] [[INOUT_ALLOC:%[0-9]+]] : $*@callee_guaranteed () -> () +// CHECK: [[MODIFY_FN:%[0-9]+]] = function_ref @$s19objc_preconcurrency6modifyyyyyczF : $@convention(thin) (@inout @callee_guaranteed () -> ()) -> () +// CHECK: = apply [[MODIFY_FN]]([[INOUT_ALLOC]]) +// CHECK: [[AFTER_MODIFY:%[0-9]+]] = load [take] [[INOUT_ALLOC]] : $*@callee_guaranteed () -> () +// CHECK: convert_function [[AFTER_MODIFY]] : $@callee_guaranteed () -> () to $@Sendable @callee_guaranteed () -> () // CHECK: } // end sil function '$s19objc_preconcurrency21testProtocolVarAccess1pyAA1P_p_tF' func testProtocolVarAccess(p: P) { modify(&p.sendyHandler)