When emitting no-escape closures, make sure that we copy the closure … #23478

gottesmm · 2019-03-22T02:40:14Z

…before we pass it into the partial apply to eliminate an ownership violation.

The problem here is that without this patch we emit code like this:

bb0(%0 : @owned $T):
  %1 = partial_apply %foo(%0)
  %2 = mark_dependence %1 on %0

Since a partial_apply consumes the object, the mark_dependence is a use after
free (especially if one has any further uses of %0 after the mark_dependence).
So what I did was I copied the value before creating the partial_apply. So
now we get this:

bb0(%0 : @owned $T):
  %1 = copy_value %0
  %2 = partial_apply %foo(%1)
  %3 = mark_dependence %2 on %0
  ...
  destroy_value %0

This ensures that one can actually have uses after the mark_dependence of both
operands.

This enables ownership verification to be enabled on
Interpreter/enforce_exclusive_access.

rdar://48521061

@owned

…before we pass it into the partial apply to eliminate an ownership violation. The problem here is that without this patch we emit code like this: bb0(%0 : @owned $T): %1 = partial_apply %foo(%0) %2 = mark_dependence %1 on %0 Since a partial_apply consumes the object, the mark_dependence is a use after free (especially if one has any further uses of %0 after the mark_dependence). So what I did was I copied the value before creating the partial_apply. So now we get this: bb0(%0 : @owned $T): %1 = copy_value %0 %2 = partial_apply %foo(%1) %3 = mark_dependence %2 on %0 ... destroy_value %0 This ensures that one can actually have uses after the mark_dependence of both operands. This enables ownership verification to be enabled on Interpreter/enforce_exclusive_access. rdar://48521061

gottesmm · 2019-03-22T02:40:39Z

@swift-ci smoke test

aschwaighofer

lg

jrose-apple · 2019-03-22T15:20:21Z

Uh, what? A noescape closure shouldn't need copying; that defeats the purpose. Why do we need a mark_dependence if the partial_apply takes the closure at +1?

aschwaighofer · 2019-03-22T15:50:13Z

This is inside the code we generate for withoutActuallyEscaping expressions.

We need to make sure that the lifetime of the noescape closure context is not shortened beyond the use of the closure we insert to verify withoutActuallyEscaping.

The code looks like the following (before this patch). We need to make sure that the lifetime of the context of %0 is not shortened to before %3's uses e.g after inlining and other optimizations. Hence the mark_dependence.

@gottesmm I assume there are situations where we end up in this code and the operand to withoutActuallyEscaping is not a trivial value (@NoEscape closure)?

sil hidden [ossa] @$s11TestClosure9letEscape1fyycyyXE_tF : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> @owned @callee_guaranteed () -> () {
// %0                                             // users: %4, %3, %1
bb0(%0 : $@noescape @callee_guaranteed () -> ()):
  debug_value %0 : $@noescape @callee_guaranteed () -> (), let, name "f", argno 1 // id: %1
  // function_ref thunk for @callee_guaranteed () -> ()
  %2 = function_ref @$sIg_Ieg_TR : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> () // user: %3
  %3 = partial_apply [callee_guaranteed] %2(%0) : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> () // user: %4
  %4 = mark_dependence %3 : $@callee_guaranteed () -> () on %0 : $@noescape @callee_guaranteed () -> () // users: %11, %5
  %5 = begin_borrow %4 : $@callee_guaranteed () -> () // users: %10, %8, %7
  // function_ref closure #1 in letEscape(f:)
  %6 = function_ref @$s11TestClosure9letEscape1fyycyyXE_tFyycyycXEfU_ : $@convention(thin) (@guaranteed @callee_guaranteed () -> ()) -> @owned @callee_guaranteed () -> () // user: %7
  %7 = apply %6(%5) : $@convention(thin) (@guaranteed @callee_guaranteed () -> ()) -> @owned @callee_guaranteed () -> () // user: %12
  %8 = is_escaping_closure %5 : $@callee_guaranteed () -> () // user: %9
  cond_fail %8 : $Builtin.Int1                    // id: %9
  end_borrow %5 : $@callee_guaranteed () -> ()    // id: %10
  destroy_value %4 : $@callee_guaranteed () -> () // id: %11
  return %7 : $@callee_guaranteed () -> ()        // id: %12
}

jrose-apple · 2019-03-22T18:01:39Z

Sounds to me like a callee_guaranteed partial_apply shouldn't be taking its argument at +1. Am I missing something?

aschwaighofer · 2019-03-22T18:05:51Z

A regular partial_apply does capture its arguments.

aschwaighofer · 2019-03-22T18:08:40Z

callee_guaranteed describes that invoking the closure does not expect the context at +1 but at +0.

gottesmm · 2019-03-22T18:29:24Z

@jrose-apple this "footgun" is why we have wanted to split the capturing box from the partial apply itself. The capturing box in general takes values at +1 no matter the convention. This is easily confused if one sees a signature with a guaranteed argument or sees the partial_apply marked callee_guaranteed.

gottesmm · 2019-03-22T18:30:04Z

@aschwaighofer I will add that as a SILGen test case to document this.

gottesmm · 2019-03-22T18:35:04Z

@aschwaighofer I am going to do that in a subsequent commit.

aschwaighofer · 2019-03-22T18:36:04Z

@gottesmm This test case already exists: test/SILGen/without_actually_escaping.swift

gottesmm · 2019-03-22T18:40:38Z

@aschwaighofer ok. I will add to that one.

jrose-apple · 2019-03-22T18:48:17Z

What does it mean to copy a noescape closure?

aschwaighofer · 2019-03-22T18:50:10Z

It is a a trivial copy.

@gottesmm I assume there are situations where we end up in this code and the operand to withoutActuallyEscaping is not a trivial value (@NoEscape closure)?

I would not expect that ownership verifier to have a problem with trivial values.

gottesmm · 2019-03-22T19:05:29Z

This is the SILGen output when I put the bad SIL into withoutActuallyEscaping.swift. The copy_value that is added by this PR is %15 = copy_value %13. Notice how without that copy, %13 is consumed by the partial apply.

sil hidden [ossa] @$s25without_actually_escaping0A24ActuallyEscapingConflictyyF : $@convention(thin) () -> () {
bb0:
  %0 = alloc_box ${ var Int }, var, name "localVar" // users: %28, %8, %1
  %1 = project_box %0 : ${ var Int }, 0           // users: %20, %9, %6
  %2 = integer_literal $Builtin.IntLiteral, 0     // user: %5
  %3 = metatype $@thin Int.Type                   // user: %5
  // function_ref Int.init(_builtinIntegerLiteral:)
  %4 = function_ref @$sSi22_builtinIntegerLiteralSiBI_tcfC : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int // user: %5
  %5 = apply %4(%2, %3) : $@convention(method) (Builtin.IntLiteral, @thin Int.Type) -> Int // user: %6
  store %5 to [trivial] %1 : $*Int                // id: %6
  // function_ref closure #1 in withoutActuallyEscapingConflict()
  %7 = function_ref @$s25without_actually_escaping0A24ActuallyEscapingConflictyyFyycfU_ : $@convention(thin) (@guaranteed { var Int }) -> () // user: %10
  %8 = copy_value %0 : ${ var Int }               // user: %10
  mark_function_escape %1 : $*Int                 // id: %9
  %10 = partial_apply [callee_guaranteed] %7(%8) : $@convention(thin) (@guaranteed { var Int }) -> () // users: %27, %12, %11
  debug_value %10 : $@callee_guaranteed () -> (), let, name "nestedModify" // id: %11
  %12 = begin_borrow %10 : $@callee_guaranteed () -> () // users: %26, %13
  %13 = copy_value %12 : $@callee_guaranteed () -> () // users: %25, %17, %15
  // function_ref thunk for @escaping @callee_guaranteed () -> ()
  %14 = function_ref @$sIeg_Ieg_TR : $@convention(thin) (@guaranteed @callee_guaranteed () -> ()) -> () // user: %16
  %15 = copy_value %13 : $@callee_guaranteed () -> () // user: %16
  %16 = partial_apply [callee_guaranteed] %14(%15) : $@convention(thin) (@guaranteed @callee_guaranteed () -> ()) -> () // user: %17
  %17 = mark_dependence %16 : $@callee_guaranteed () -> () on %13 : $@callee_guaranteed () -> () // users: %24, %18
  %18 = begin_borrow %17 : $@callee_guaranteed () -> () // users: %23, %21, %20
  // function_ref closure #2 in withoutActuallyEscapingConflict()
  %19 = function_ref @$s25without_actually_escaping0A24ActuallyEscapingConflictyyFyyycXEfU0_ : $@convention(thin) (@guaranteed @callee_guaranteed () -> (), @inout_aliasable Int) -> () // user: %20
  %20 = apply %19(%18, %1) : $@convention(thin) (@guaranteed @callee_guaranteed () -> (), @inout_aliasable Int) -> ()
  %21 = is_escaping_closure %18 : $@callee_guaranteed () -> () // user: %22
  cond_fail %21 : $Builtin.Int1                   // id: %22
  end_borrow %18 : $@callee_guaranteed () -> ()   // id: %23
  destroy_value %17 : $@callee_guaranteed () -> () // id: %24
  destroy_value %13 : $@callee_guaranteed () -> () // id: %25
  end_borrow %12 : $@callee_guaranteed () -> ()   // id: %26
  destroy_value %10 : $@callee_guaranteed () -> () // id: %27
  destroy_value %0 : ${ var Int }                 // id: %28
  %29 = tuple ()                                  // user: %30
  return %29 : $()                                // id: %30
} // end sil function '$s25without_actually_escaping0A24ActuallyEscapingConflictyyF'

gottesmm · 2019-03-22T19:08:01Z

So the reason this happens is the thunk we introduce.

jrose-apple · 2019-03-22T21:51:51Z

It doesn't semantically make sense to copy a value, consume it to produce a new value, and then mark dependence on the thing it was copied from. What does that mean?

aschwaighofer · 2019-03-22T22:40:21Z

I don't understand your issue.

With

%17 = mark_dependence %16 : $@callee_guaranteed () -> () on %13 : $@callee_guaranteed () -> ()

we communicate to the optimizer that all uses of %17 depend on %13 (being alive) - the optimizer is not allowed to move the ultimate destroy of %13 before any of %17's uses. This is a precise semantic meaning?

jrose-apple · 2019-03-22T22:54:01Z

That's true! But it doesn't make sense to copy %13 and use the copy to generate %16 (and then have %16 rely on %13 instead of the copy). That means that %16 isn't actually consuming the copy, since if it were, it wouldn't need %13 to be kept alive.

aschwaighofer · 2019-03-22T23:17:36Z

You are right that the constraint is not needed in this case. It is an overly conservative constraint in this case (the argument to withoutAcutallyEscaping being an escaping closure) since the partial_apply captures the reference.

It is not redundant information if the input was a noescape closure (only the trivial value is captured by the partial_apply of a noescape closure type): in which case it means %17 depends on the closure context of %13 (via transitivity of the mark_dependence chain).

%17 = mark_dependence %16 : $@callee_guaranteed () -> () on %13 : $@NoEscape @callee_guaranteed () -> ()

If we inline s11TestClosure9letEscape1fyycyyXE_tF into s22closure_lifetime_fixup10testSimple1cyAA1CC_tF the optimizer sees that %4 is ultimately dependent on [ARG] and therefore must not move the ultimate release of [ARG] accross any of %4s uses.

// CHECK-LABEL: sil @$s22closure_lifetime_fixup10testSimple1cyAA1CC_tF : $@convention(thin) (@guaranteed C) -> () {
// CHECK: bb0([[ARG:%.*]] : $C):
// CHECK: [[F:%.*]]  = function_ref @$s22closure_lifetime_fixup10testSimple1cyAA1CC_tFyyXEfU_
// CHECK-NEXT:  strong_retain [[ARG]] : $C
// CHECK-NEXT:  [[PA:%.*]] = partial_apply [callee_guaranteed] [on_stack] [[F]]([[ARG]]) : $@convention(thin) (@guaranteed C) -> ()
// CHECK-NEXT:  [[CL:%.*]] = mark_dependence [[PA]] : $@noescape @callee_guaranteed () -> () on [[ARG]] : $C
// CHECK-NEXT:  // function_ref use_closure(_:)
// CHECK-NEXT:  [[F2:%.*]] = function_ref @$s11TestClosure9letEscape1fyycyyXE_tF : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
// CHECK-NEXT:  apply [[F2]]([[CL]]) : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
// CHECK-NEXT:  dealloc_stack [[PA]] : $@noescape @callee_guaranteed () -> ()
// CHECK-NEXT:  strong_release [[ARG]] : $C
// CHECK-NEXT:  tuple ()
// CHECK-NEXT:  return {{.*}} : $()


sil hidden [ossa] @$s11TestClosure9letEscape1fyycyyXE_tF : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> @owned @callee_guaranteed () -> () {
// %0                                             // users: %4, %3, %1
bb0(%0 : $@noescape @callee_guaranteed () -> ()):
  debug_value %0 : $@noescape @callee_guaranteed () -> (), let, name "f", argno 1 // id: %1
  // function_ref thunk for @callee_guaranteed () -> ()
  %2 = function_ref @$sIg_Ieg_TR : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> () // user: %3
  %3 = partial_apply [callee_guaranteed] %2(%0) : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> () // user: %4
  %4 = mark_dependence %3 : $@callee_guaranteed () -> () on %0 : $@noescape @callee_guaranteed () -> () // users: %11, %5
  %5 = begin_borrow %4 : $@callee_guaranteed () -> () // users: %10, %8, %7
  // function_ref closure #1 in letEscape(f:)
  %6 = function_ref @$s11TestClosure9letEscape1fyycyyXE_tFyycyycXEfU_ : $@convention(thin) (@guaranteed @callee_guaranteed () -> ()) -> @owned @callee_guaranteed () -> () // user: %7
  %7 = apply %6(%5) : $@convention(thin) (@guaranteed @callee_guaranteed () -> ()) -> @owned @callee_guaranteed () -> () // user: %12
  %8 = is_escaping_closure %5 : $@callee_guaranteed () -> () // user: %9
  cond_fail %8 : $Builtin.Int1                    // id: %9
  end_borrow %5 : $@callee_guaranteed () -> ()    // id: %10
  destroy_value %4 : $@callee_guaranteed () -> () // id: %11
  return %7 : $@callee_guaranteed () -> ()        // id: %12
}

jrose-apple · 2019-03-23T00:09:12Z

Forget the implementation. I'm saying that the semantics of "copy a value X to X2, consume X2 to produce Y, and mark that Y is dependent on the original X" are wrong. Either Y now owns X2 (and therefore doesn't need X), or X2 secretly depends on X (and therefore the copy didn't make an independent value). In this case it's the latter.

jrose-apple · 2019-03-23T00:11:35Z

~~destroy_value %4 : $@callee_guaranteed () -> () also doesn't make sense. @callee_guaranteed is a form of @guaranteed; it shouldn't be yours to destroy, right?~~

Sorry, I misinterpreted this part. The previous comment still applies.

jrose-apple · 2019-03-23T00:16:31Z

If partial_apply could take a borrow this would make a lot more sense, I think.

gottesmm requested a review from aschwaighofer March 22, 2019 02:40

aschwaighofer approved these changes Mar 22, 2019

View reviewed changes

gottesmm merged commit 6a7b7ad into swiftlang:master Mar 22, 2019

gottesmm deleted the pr-02277b9b2dc67311306e31d9429a74a956e3fadf branch March 22, 2019 18:35

gottesmm mentioned this pull request Mar 22, 2019

[silgen] Add explicit SILGen test for 7246806ef629915d01a278e11c9535f… #23493

Merged

When emitting no-escape closures, make sure that we copy the closure … #23478

When emitting no-escape closures, make sure that we copy the closure … #23478

Uh oh!

Conversation

gottesmm commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gottesmm commented Mar 22, 2019

Uh oh!

aschwaighofer left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Mar 22, 2019

Uh oh!

aschwaighofer commented Mar 22, 2019

Uh oh!

jrose-apple commented Mar 22, 2019

Uh oh!

aschwaighofer commented Mar 22, 2019

Uh oh!

aschwaighofer commented Mar 22, 2019

Uh oh!

gottesmm commented Mar 22, 2019

Uh oh!

gottesmm commented Mar 22, 2019

Uh oh!

gottesmm commented Mar 22, 2019

Uh oh!

aschwaighofer commented Mar 22, 2019

Uh oh!

gottesmm commented Mar 22, 2019

Uh oh!

jrose-apple commented Mar 22, 2019

Uh oh!

aschwaighofer commented Mar 22, 2019

Uh oh!

gottesmm commented Mar 22, 2019

Uh oh!

gottesmm commented Mar 22, 2019

Uh oh!

jrose-apple commented Mar 22, 2019

Uh oh!

aschwaighofer commented Mar 22, 2019

Uh oh!

jrose-apple commented Mar 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschwaighofer commented Mar 22, 2019

Uh oh!

jrose-apple commented Mar 23, 2019

Uh oh!

jrose-apple commented Mar 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Mar 23, 2019

Uh oh!

Uh oh!

gottesmm commented Mar 22, 2019 •

edited

Loading

jrose-apple commented Mar 22, 2019 •

edited

Loading

jrose-apple commented Mar 23, 2019 •

edited

Loading