Skip to content
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

Debugging Aid #619

Open
migueldeicaza opened this issue Nov 28, 2024 · 2 comments
Open

Debugging Aid #619

migueldeicaza opened this issue Nov 28, 2024 · 2 comments

Comments

@migueldeicaza
Copy link
Owner

I am using this debugging aid to track down when Godot releases objects:

commit c0223d7edc87c39b07e2a6d7e7ed24f6746fef8f
Author: Miguel de Icaza <miguel@gnome.org>
Date:   Wed Nov 27 23:47:33 2024 -0500

    Clear the `handle` value from any wrapped objects when Godot tells us
    that the object is gone, this will swap out a series of crashes from
    one place, to another, but also if the handle is reused, it will now
    show that error.
    
    This is to help track down cases where we access an object after Godot
    has freed it (like we kept a pointer while Godot decided to destroy
    it).

diff --git a/Sources/SwiftGodot/Core/Wrapped.swift b/Sources/SwiftGodot/Core/Wrapped.swift
index 590713d..5c6de0f 100644
--- a/Sources/SwiftGodot/Core/Wrapped.swift
+++ b/Sources/SwiftGodot/Core/Wrapped.swift
@@ -558,7 +558,11 @@ func userTypeBindingFree (_ token: UnsafeMutableRawPointer?, _ instance: UnsafeM
     // I do not think this is necessary, since we are handling the release in the
     // user-binding catch-all (that also covers the Godot-triggers invocations)
     // freeFunc above.
-    pd ("SWIFT: instanceBindingFree token=\(String(describing: token)) instance=\(String(describing: instance)) binding=\(String(describing: binding))")
+    if let binding {
+        let wrapped = Unmanaged<Wrapped>.fromOpaque(binding)
+        let obj = wrapped.takeUnretainedValue()
+        obj.handle = nil
+    }
 }
 
 // This is invoked to take a reference on the object and ensure our Swift-land object
@@ -582,7 +586,11 @@ func frameworkTypeBindingCreate (_ token: UnsafeMutableRawPointer?, _ instance:
 }
 
 func frameworkTypeBindingFree (_ token: UnsafeMutableRawPointer?, _ instance: UnsafeMutableRawPointer?, _ binding: UnsafeMutableRawPointer?) {
-    // No longer needed
+    if let binding {
+        let wrapped = Unmanaged<Wrapped>.fromOpaque(binding)
+        let obj = wrapped.takeUnretainedValue()
+        obj.handle = nil
+    }
 }
 
 /// This function is called by Godot to invoke our callable, and contains our context in `userData`,

And also this companion:

diff --git a/Generator/Generator/MethodGen.swift b/Generator/Generator/MethodGen.swift
index 6d0bd37..f0d321d 100644
--- a/Generator/Generator/MethodGen.swift
+++ b/Generator/Generator/MethodGen.swift
@@ -746,6 +746,9 @@ func generateMethod(_ p: Printer, method: MethodDefinition, className: String, c
     }
     
     p ("\(declarationTokens)(\(argumentsList))\(returnClause)") {
+        if staticAttribute == nil {
+            p("if handle == nil { Wrapped.attemptToUseObjectFreedByGodot() }")
+        }
         if method.optionalHash == nil {
             if let godotReturnType {
                 p(makeDefaultReturn(godotType: godotReturnType))
diff --git a/Sources/SwiftGodot/Core/Wrapped.swift b/Sources/SwiftGodot/Core/Wrapped.swift
index 5c6de0f..9a411a5 100644
--- a/Sources/SwiftGodot/Core/Wrapped.swift
+++ b/Sources/SwiftGodot/Core/Wrapped.swift
@@ -115,6 +115,10 @@ open class Wrapped: Equatable, Identifiable, Hashable {
         []
     }
 
+    @inline(never)
+    public static func attemptToUseObjectFreedByGodot() {
+        fatalError ("Wrapped.handle was nil, which indicates the object was cleared by Godot")
+    }
     class func getVirtualDispatcher(name: StringName) ->  GDExtensionClassCallVirtual? {
         pd ("SWARN: getVirtualDispatcher (\"\(name)\") reached Wrapped on class \(self)")
         return nil

@migueldeicaza
Copy link
Owner Author

This is also necessary:

commit 56a471ccf309b9875cefe74039349755d61507ba
Author: Miguel de Icaza <miguel@gnome.org>
Date:   Sun Dec 1 11:21:31 2024 -0500

    Prevent the binding callbacks (in particular free) to be invoked from an object that has been deinited, follow up to c0223d7edc87c39b07e2a6d7e7ed24f6746fef8f

diff --git a/Sources/SwiftGodot/Core/Wrapped.swift b/Sources/SwiftGodot/Core/Wrapped.swift
index 9a411a5..4012db1 100644
--- a/Sources/SwiftGodot/Core/Wrapped.swift
+++ b/Sources/SwiftGodot/Core/Wrapped.swift
@@ -82,7 +82,7 @@ open class Wrapped: Equatable, Identifiable, Hashable {
     var ownsHandle: Bool
     public static var fcallbacks = OpaquePointer (UnsafeRawPointer (&Wrapped.frameworkTypeBindingCallback))
     public static var ucallbacks = OpaquePointer (UnsafeRawPointer (&Wrapped.userTypeBindingCallback))
-    
+
     /// Conformance to Identifiable by using the native handle to the object
     public var id: Int { Int (bitPattern: handle) }
     
@@ -125,6 +125,10 @@ open class Wrapped: Equatable, Identifiable, Hashable {
     }
 
     deinit {
+        if let handle {
+            gi.object_free_instance_binding(UnsafeMutableRawPointer (mutating: handle),  extensionInterface.getLibrary())
+        }
+
         if ownsHandle {
             if let handle {
                 guard extensionInterface.objectShouldDeinit(handle: handle) else { return }
@@ -186,7 +190,6 @@ open class Wrapped: Equatable, Identifiable, Hashable {
         free_callback: frameworkTypeBindingFree,
         reference_callback: nil) // frameworkTypeBindingReference)
 
-
     /// Returns the Godot's class name as a `StringName`, returns the empty string on error
     public var godotClassName: StringName {
         var sc: StringName.ContentType = StringName.zero
diff --git a/Sources/SwiftGodot/EntryPoint.swift b/Sources/SwiftGodot/EntryPoint.swift
index 5e70b5d..d2b9428 100644
--- a/Sources/SwiftGodot/EntryPoint.swift
+++ b/Sources/SwiftGodot/EntryPoint.swift
@@ -193,6 +193,7 @@ struct GodotInterface {
 
     let object_set_instance: GDExtensionInterfaceObjectSetInstance
     let object_set_instance_binding: GDExtensionInterfaceObjectSetInstanceBinding
+    let object_free_instance_binding: GDExtensionInterfaceObjectFreeInstanceBinding
     let object_get_class_name: GDExtensionInterfaceObjectGetClassName
     
     let object_method_bind_ptrcall: GDExtensionInterfaceObjectMethodBindPtrcall
@@ -328,6 +329,7 @@ func loadGodotInterface (_ godotGetProcAddrPtr: GDExtensionInterfaceGetProcAddre
         
         object_set_instance: load ("object_set_instance"),
         object_set_instance_binding: load ("object_set_instance_binding"),
+        object_free_instance_binding: load ("object_free_instance_binding"),
         object_get_class_name: load ("object_get_class_name"),
         object_method_bind_ptrcall: load ("object_method_bind_ptrcall"),
         object_destroy: load ("object_destroy"),

I am still testing internally, before I land this in public:

c0223d7edc87c39b07e2a6d7e7ed24f6746fef8f
56a471ccf309b9875cefe74039349755d61507ba
6cf7f05e00e4e2e48f699caee55ce84614c25d78

This one is a cute one:

diff --git a/Generator/Generator/MethodGen.swift b/Generator/Generator/MethodGen.swift
index 6d0bd37..f0d321d 100644
--- a/Generator/Generator/MethodGen.swift
+++ b/Generator/Generator/MethodGen.swift
@@ -746,6 +746,9 @@ func generateMethod(_ p: Printer, method: MethodDefinition, className: String, c
     }
     
     p ("\(declarationTokens)(\(argumentsList))\(returnClause)") {
+        if staticAttribute == nil {
+            p("if handle == nil { Wrapped.attemptToUseObjectFreedByGodot() }")
+        }
         if method.optionalHash == nil {
             if let godotReturnType {
                 p(makeDefaultReturn(godotType: godotReturnType))
diff --git a/Sources/SwiftGodot/Core/Wrapped.swift b/Sources/SwiftGodot/Core/Wrapped.swift
index 5c6de0f..9a411a5 100644
--- a/Sources/SwiftGodot/Core/Wrapped.swift
+++ b/Sources/SwiftGodot/Core/Wrapped.swift
@@ -115,6 +115,10 @@ open class Wrapped: Equatable, Identifiable, Hashable {
         []
     }
 
+    @inline(never)
+    public static func attemptToUseObjectFreedByGodot() {
+        fatalError ("Wrapped.handle was nil, which indicates the object was cleared by Godot")
+    }
     class func getVirtualDispatcher(name: StringName) ->  GDExtensionClassCallVirtual? {
         pd ("SWARN: getVirtualDispatcher (\"\(name)\") reached Wrapped on class \(self)")
         return nil

@migueldeicaza
Copy link
Owner Author

Also, useful, and important: track the liveness of framework objects, which I had previously removed, otherwise you can create two framework objects, and when the second goes away it clears the handle on the first one.

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

No branches or pull requests

1 participant