Skip to content

[embedded] Handle retain/retain ops inside deinit in Embedded Swift's swift_release #76231

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.

Sign up for GitHub

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
merged 3 commits into from
Sep 16, 2024
Merged

[embedded] Handle retain/retain ops inside deinit in Embedded Swift's swift_release #76231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
249f7e6
[embedded] Handle retain/retain ops inside deinit in Embedded Swift's…
kubamracek Sep 3, 2024
c8cbf06
[embedded] Add a more targeted retain/release-in-deinit test
kubamracek Sep 16, 2024
b4431db
[embedded] Add a more detailed explaining comment to swift_release_n_
kubamracek Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions stdlib/public/core/EmbeddedRuntime.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,17 @@ func swift_release_n_(object: UnsafeMutablePointer<HeapObject>?, n: UInt32) {

let resultingRefcount = subFetchAcquireRelease(refcount, n: Int(n)) & HeapObject.refcountMask
if resultingRefcount == 0 {
// Set the refcount to immortalRefCount before calling the object destroyer
// to prevent future retains/releases from having any effect. Unlike the
// full Swift runtime, we don't track the refcount inside deinit, so we
// won't be able to detect escapes or over-releases of `self` in deinit. We
// might want to reconsider that in the future.
//
// There can only be one thread with a reference at this point (because
// we're releasing the last existing reference), so a relaxed store is
// enough.
storeRelaxed(refcount, newValue: HeapObject.immortalRefCount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth commenting explicitly that there can only be one thread holding a reference to this object at this point (unless there's a bug somewhere), which is why a relaxed store is fine here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconded. On another note, the non-embedded runtime continues tracking refcounts, and this allows it to raise a fatal error when self escapes from deinit by checking the refcount when the memory is freed. See here:

swift/stdlib/public/runtime/HeapObject.cpp

Line 850 in 03abb1f

swift::fatalError(0,

Obviously this is an optional feature and not something you need to do now (or ever), but might be worth keeping in mind as a possible enhancement.


_swift_embedded_invoke_heap_object_destroy(object)
} else if resultingRefcount < 0 {
fatalError("negative refcount")
Expand Down Expand Up @@ -350,6 +361,9 @@ fileprivate func storeRelease(_ atomic: UnsafeMutablePointer<Int>, newValue: Int
Builtin.atomicstore_release_Word(atomic._rawValue, newValue._builtinWordValue)
}

fileprivate func storeRelaxed(_ atomic: UnsafeMutablePointer<Int>, newValue: Int) {
Builtin.atomicstore_monotonic_Word(atomic._rawValue, newValue._builtinWordValue)
}

/// Exclusivity checking

Expand Down
26 changes: 26 additions & 0 deletions test/embedded/deinit-release.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend %s -enable-experimental-feature Embedded -O -c -o %t/main.o
// RUN: %target-clang %t/main.o -o %t/a.out -dead_strip
// RUN: %target-run %t/a.out | %FileCheck %s

// REQUIRES: swift_in_compiler
// REQUIRES: executable_test
// REQUIRES: OS=macosx || OS=linux-gnu

class C {}

struct Foo {
let foo: C = C()
let bar: C = C()
}

class Bar {}
class SubBar: Bar {
var qwe = Foo()
}

var bar: SubBar? = SubBar()
bar = nil
print("OK!")

// CHECK: OK!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit lost as to how this test actually tests this scenario in question, and I worry it might silently stop testing this scenario with some compiler change. It might be better to use Unmanaged to do an explicit retain/release in a deinit {} block, or assign self to a global, call out to something opaque, then remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a more targeted test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

35 changes: 35 additions & 0 deletions test/embedded/deinit-release2.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// RUN: %empty-directory(%t)
// RUN: %target-swift-frontend %s -enable-experimental-feature Embedded -O -c -o %t/main.o
// RUN: %target-clang %t/main.o -o %t/a.out -dead_strip
// RUN: %target-run %t/a.out | %FileCheck %s

// REQUIRES: swift_in_compiler
// REQUIRES: executable_test
// REQUIRES: OS=macosx || OS=linux-gnu

public var global_c: C? = nil

@inline(never) func opaque() { print(global_c == nil ? "global is nil" : "global is not nil") }

public class C {
init() { print("init") }
deinit {
print("deinit")
global_c = self
opaque()
global_c = nil
opaque()
print("end of deinit")
}
}

var bar: C? = C()
bar = nil
print("OK!")

// CHECK: init
// CHECK: deinit
// CHECK: global is not nil
// CHECK: global is nil
// CHECK: end of deinit
// CHECK: OK!