Disable stack promotion for classes with isolated deinit

[nickolas-pohilets]

This provides a proper fix of the issue, reverting workaround implemented in #76960

[ktoso]

@swift-ci please smoke test

[ktoso]

Hah, great observation, fix sounds good to me.

[eeckstein]

lgtm

[eeckstein]

Thinking about this again, a better fix would be to make this check in EscapeWalker.handleDestroy, because then it would also be correct for other optimizations which use EscapeInfo:

swift/SwiftCompilerSources/Sources/Optimizer/Utilities/EscapeUtils.swift

Line 598 in fc52e27

// The object to destroy (= the argument of the destructor) cannot escape itself.

Though, it's a bit more complicated because it also needs to handle structs/tuples/enums which contain a class reference. This can be done by adding a flag in TypeLowering.RecursiveProperties, like done for SILType::isOrContainsRawPointer (which is called by SIL.Type.isTrivialNonPointer).

[ktoso]

@eeckstein @nickolas-pohilets shall we merge this to resolve the crashes and investigate the follow up alternative approach you’re suggesting, or hold off merging and do the alternative approach instead immediately — wdyt @eeckstein?

[eeckstein]

I'm fine with merging this and doing it the right way in a follow-up. But it would be nice to first file a github issue and reference it in a to-do comment in the stack promotion

[nickolas-pohilets]

I'm still reading the code of the EscapeUtils.swift and don't understand it well enough to judge on the merging strategy. But I'm generally in favour of small incremental steps. @eeckstein, could you pls file an issue and I'll update comments to reference it.

My vague understanding so far is that reference to objects with isolated deinit inside structs/tuples/enums is not a very big issue - current check in StackPromotion.swift will catch it.

By vice versa can be a more serious issue - if non-escaping value is stored in a seemings non-escaping object with isolated deinit, then it will escape, and check in StackPromotion.swift won't do anything about it.

Also type erasure can be an issue. When destroying type-erased reference we have to be conservative and assume that object has isolated deinit. Is this an issue, or escape analysis already conservatively assumes that any uses of the type-erased values can be escaping?

[nickolas-pohilets]

Another interesting challenge:

class ClassNoOp {
  @AnotherActor
  deinit {
  }
}

class Holder {
  var ref: ClassNoOp?
}

func testClass() {
  let a = ClassNoOp()
  let b = Holder()
  b.ref = a
}

In this example b does not escape, but ClassNoOp has isolated deinit, so a should be analysed as escaping. But analysed SIL code does not contain DestroyValueInst, ReleaseValueInst, or StrongReleaseInst for ClassNoOp for a:

// testClass()
// Isolation: unspecified
sil hidden @$s33async_task_locals_isolated_deinit9testClassyyF : $@convention(thin) () -> () {
[global: read,write,copy,destroy,allocate,deinit_barrier]
bb0:
  %0 = alloc_ref $ClassNoOp                       // users: %2, %1
  debug_value %0 : $ClassNoOp, let, name "self", argno 1 // id: %1
  %2 = end_init_let_ref %0 : $ClassNoOp           // users: %11, %3
  debug_value %2 : $ClassNoOp, let, name "a"      // id: %3
  %4 = alloc_ref $Holder                          // users: %6, %5
  debug_value %4 : $Holder, let, name "self", argno 1 // id: %5
  %6 = end_init_let_ref %4 : $Holder              // users: %17, %13, %10, %7
  %7 = ref_element_addr %6 : $Holder, #Holder.ref // users: %14, %9
  %8 = enum $Optional<ClassNoOp>, #Optional.none!enumelt // user: %9
  store %8 to %7 : $*Optional<ClassNoOp>          // id: %9
  debug_value %6 : $Holder, let, name "b"         // id: %10
  %11 = enum $Optional<ClassNoOp>, #Optional.some!enumelt, %2 : $ClassNoOp // users: %15, %12
  debug_value %11 : $Optional<ClassNoOp>, let, name "value", argno 1 // id: %12
  debug_value %6 : $Holder, let, name "self", argno 2 // id: %13
  %14 = begin_access [modify] [dynamic] [no_nested_conflict] %7 : $*Optional<ClassNoOp> // users: %16, %15
  store %11 to %14 : $*Optional<ClassNoOp>        // id: %15
  end_access %14 : $*Optional<ClassNoOp>          // id: %16
  strong_release %6 : $Holder                     // id: %17
  %18 = tuple ()                                  // user: %19
  return %18 : $()                                // id: %19
} // end sil function '$s33async_task_locals_isolated_deinit9testClassyyF'

So if handleDestroy() is the only place responsible for this logic, escape of a will not be detected. Maybe instead this logic should be attached to the alloc_ref?

[eeckstein]

Thinking about it again (2nd try), I think it's sufficient to do the check in stack promotion. An isolated deinit does not actually let self escape, it just can extend the lifetime of self. So escape analysis is fine. Just stack promotion needs to be fixed. So this PR looks good to be merged.

so a should be analysed as escaping

No, a is not escaping. It's just outliving b's lifetime.

[nickolas-pohilets]

And what is the difference between "escaping" and "extending the lifetime"?

[eeckstein]

"Escaping" - how we define it - means that a value (in this case an object reference) is visible at some other place. And "place" depends on the context in which escape analysis is used. For stack promotion this means: Is the object reference visible after the current function has returned?
Another example is alias analysis: here we use escape analysis to ask: is an address visible at another SIL instruction? A concrete example for alias analysis:

  %reference = alloc_ref $ClassWithIsolatedDeinit
  %fieldAddr = ref_element_addr %reference, #someField
  destroy_value %reference 
  ...
  apply %someUnrelatedFunction()   // %fieldAddr cannot be visible in this function, even
                                   // if the isolated deinit is executed after this apply
                                   // and therefore its lifetime extends over this apply

[nickolas-pohilets]

I'm concerned about the following scenario:

let x = SomeClass()
let y = ClassWithIsolatedDeinit()
y.prop = x

Here neither x nor y cannot be allocated on stack. Current check in StackPromotions will catch y, but not x.
If y is considered non-escaping, then I'm not sure which part is responsible for detecting that x is escaping.

[eeckstein]

Good example! You are right that this needs to be handled.
We can do that in stack promotion by adding the following method to ComputeOuterBlockrange:

  mutating func visitDef(def: Value, path: EscapePath) -> DefResult {
    if let allocRef = def as? AllocRefInst, allocRef.type.hasNonIsolatedDeinit {
      return .abortWalk
    }
    return .continueWalkUp
  }

[nickolas-pohilets]

I've added unit tests and seems that it works even without with change, because escape analysis of the explicit deinit is very conservative and assumes and anything getting there can escape.

But if this changes in the future, it may break. But added unit test should catch it. So I guess PR should be good as it is now.

[ktoso]

@swift-ci please smoke test

[ktoso]

@swift-ci please smoke test

[nickolas-pohilets]

TestFoundation/TestFileManager.swift:1361: error: TestFileManager.test_emptyFilename : XCTAssertEqual failed: ("Optional(FoundationEssentials.CocoaError.Code(rawValue: 512))") is not equal to ("Optional(FoundationEssentials.CocoaError.Code(rawValue: 4))") - 

This time not related to PR changes. Let's wait until https://github.com/swiftlang/swift-corelibs-foundation is fixed.

[nickolas-pohilets]

I see a few green pipelines in https://ci-external.swift.org/job/swift-PR-windows/. Let's try again.

[xwu]

@swift-ci please smoke test

[nickolas-pohilets]

Failing test has been disabled - swiftlang/swift-corelibs-foundation@974106b

[ktoso]

@swift-ci please smoke test Windows

[xwu]

@swift-ci please smoke test Windows

[ktoso]

Nice work! Looks good to me, merging and if anything pops up in additional testing I’ll keep an eye out