Skip to content

[move-only] Perform an exclusive borrow when passing a var to a consuming var. #63591

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

gottesmm
Copy link
Contributor

Consider the following example:

class Klass {}

@_moveOnly struct Butt {
  var k = Klass()
}

func mixedUse(_: inout Butt, _: __owned Butt) {}

func foo() {
    var y = Butt()
    mixedUse(&y, y)
}

In this case, we want to have an exclusivity violation. Before this patch, we did a by-value load [copy] of y and then performed the inout access. Since the access scopes did not overlap, we would not get an exclusivity violation. Additionally, since the checker assumes that exclusivity violations will be caught in such a situation, we convert the load [copy] to a load [take] causing a later memory lifetime violation as seen in the following SIL:

sil hidden [ossa] @$s4test3fooyyF : $@convention(thin) () -> () {
bb0:
  %0 = alloc_stack [lexical] $Butt, var, name "y" // users: %4, %5, %8, %12, %13
  %1 = metatype $@thin Butt.Type                  // user: %3
  // function_ref Butt.init()
  %2 = function_ref @$s4test4ButtVACycfC : $@convention(method) (@thin Butt.Type) -> @owned Butt // user: %3
  %3 = apply %2(%1) : $@convention(method) (@thin Butt.Type) -> @owned Butt // user: %4
  store %3 to [init] %0 : $*Butt                  // id: %4
  %5 = begin_access [modify] [static] %0 : $*Butt // users: %7, %6
  %6 = load [take] %5 : $*Butt                    // user: %10                // <————————— This was a load [copy].
  end_access %5 : $*Butt                          // id: %7
  %8 = begin_access [modify] [static] %0 : $*Butt // users: %11, %10
  // function_ref mixedUse2(_:_:)
  %9 = function_ref @$s4test9mixedUse2yyAA4ButtVz_ADntF : $@convention(thin) (@inout Butt, @owned Butt) -> () // user: %10
  %10 = apply %9(%8, %6) : $@convention(thin) (@inout Butt, @owned Butt) -> ()
  end_access %8 : $*Butt                          // id: %11
  destroy_addr %0 : $*Butt                        // id: %12
  dealloc_stack %0 : $*Butt                       // id: %13
  %14 = tuple ()                                  // user: %15
  return %14 : $()                                // id: %15
} // end sil function '$s4test3fooyyF'

Now, instead we create a [consume] access and get the nice exclusivity error we are looking for.

NOTE: As part of this I needed to tweak the verifier so that [deinit] accesses are now allowed to have any form of access enforcement before we are in LoweredSIL. I left in the original verifier error in LoweredSIL and additionally left in the original error in IRGen. The reason why I am doing this is that I need the deinit access to represent semantically what consuming from a ref_element_addr, global, or escaping mutable var look like at the SIL level so that the move checker can error upon it. Since we will error upon such consumptions in Canonical SIL, such code patterns will never actually hit Lowered/IRGen SIL, so it is safe to do so (and the verifier/errors will help us if we make any mistakes). In the case of a non-escaping var though, we will be able to use deinit statically and the move checker will make sure that it is not reused before it is reinitialized.

rdar://101767439

…ming var.

Consider the following example:

```
class Klass {}

@_moveOnly struct Butt {
  var k = Klass()
}

func mixedUse(_: inout Butt, _: __owned Butt) {}

func foo() {
    var y = Butt()
    mixedUse(&y, y)
}
```

In this case, we want to have an exclusivity violation. Before this patch, we
did a by-value load [copy] of y and then performed the inout access. Since the
access scopes did not overlap, we would not get an exclusivity violation.
Additionally, since the checker assumes that exclusivity violations will be
caught in such a situation, we convert the load [copy] to a load [take] causing
a later memory lifetime violation as seen in the following SIL:

```
sil hidden [ossa] @$s4test3fooyyF : $@convention(thin) () -> () {
bb0:
  %0 = alloc_stack [lexical] $Butt, var, name "y" // users: %4, %5, %8, %12, %13
  %1 = metatype $@thin Butt.Type                  // user: %3
  // function_ref Butt.init()
  %2 = function_ref @$s4test4ButtVACycfC : $@convention(method) (@thin Butt.Type) -> @owned Butt // user: %3
  %3 = apply %2(%1) : $@convention(method) (@thin Butt.Type) -> @owned Butt // user: %4
  store %3 to [init] %0 : $*Butt                  // id: %4
  %5 = begin_access [modify] [static] %0 : $*Butt // users: %7, %6
  %6 = load [take] %5 : $*Butt                    // user: %10                // <————————— This was a load [copy].
  end_access %5 : $*Butt                          // id: %7
  %8 = begin_access [modify] [static] %0 : $*Butt // users: %11, %10
  // function_ref mixedUse2(_:_:)
  %9 = function_ref @$s4test9mixedUse2yyAA4ButtVz_ADntF : $@convention(thin) (@inout Butt, @owned Butt) -> () // user: %10
  %10 = apply %9(%8, %6) : $@convention(thin) (@inout Butt, @owned Butt) -> ()
  end_access %8 : $*Butt                          // id: %11
  destroy_addr %0 : $*Butt                        // id: %12
  dealloc_stack %0 : $*Butt                       // id: %13
  %14 = tuple ()                                  // user: %15
  return %14 : $()                                // id: %15
} // end sil function '$s4test3fooyyF'
```

Now, instead we create a [consume] access and get the nice exclusivity error we
are looking for.

NOTE: As part of this I needed to tweak the verifier so that [deinit] accesses
are now allowed to have any form of access enforcement before we are in
LoweredSIL. I left in the original verifier error in LoweredSIL and additionally
left in the original error in IRGen. The reason why I am doing this is that I
need the deinit access to represent semantically what consuming from a
ref_element_addr, global, or escaping mutable var look like at the SIL level so
that the move checker can error upon it. Since we will error upon such
consumptions in Canonical SIL, such code patterns will never actually hit
Lowered/IRGen SIL, so it is safe to do so (and the verifier/errors will help us
if we make any mistakes). In the case of a non-escaping var though, we will be
able to use deinit statically and the move checker will make sure that it is not
reused before it is reinitialized.

rdar://101767439
@gottesmm
Copy link
Contributor Author

@swift-ci smoke test

@gottesmm gottesmm merged commit 7059248 into swiftlang:main Feb 11, 2023
@gottesmm gottesmm deleted the pr-230be51d2a48c6f5cb763dcc7b448f202685dcf7 branch February 11, 2023 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant