migueldeicaza / SwiftGodot

New Godot bindings for Swift
https://migueldeicaza.github.io/SwiftGodotDocs/tutorials/swiftgodot-tutorials/
MIT License
1.07k stars 69 forks source link

Extension dylibs aren't being unloaded on Hot Reload #384

Open tishin opened 7 months ago

tishin commented 7 months ago

This was originally mentioned in #273 I made a simplistic entry point using GDExtension target as the only dependency:

import GDExtension

@_cdecl("swift_entry_point") public func enterExtension (interface: OpaquePointer?, library: OpaquePointer?, extension: OpaquePointer?) -> UInt8 {
    guard let library, let interface, let `extension` else { return 0 }
    initializeSwiftModule (interface, library, `extension`)
    return 1
}

public func initializeSwiftModule (_ godotGetProcAddrPtr: OpaquePointer, _ libraryPtr: OpaquePointer, _ extensionPtr: OpaquePointer) {
    let initialization = UnsafeMutablePointer<GDExtensionInitialization> (extensionPtr)
    initialization.pointee.deinitialize = extension_deinitialize
    initialization.pointee.initialize = extension_initialize
    initialization.pointee.minimum_initialization_level = GDEXTENSION_INITIALIZATION_SCENE
}

func extension_initialize (userData: UnsafeMutableRawPointer?, l: GDExtensionInitializationLevel) {}
func extension_deinitialize (userData: UnsafeMutableRawPointer?, l: GDExtensionInitializationLevel) {}

final class SomeClass {}

And configured gdextension as reloadable:

[configuration]
entry_symbol = "swift_entry_point"
compatibility_minimum = 4.2
reloadable = true

This was enough to reproduce the issue:

  1. Build the extension
  2. Run godot
  3. Build the extension again replacing the dylib This will result in
    objc[79981]: Class _TtC15GodotPlayground9SomeClass is implemented in both /gd/GodotPlayground/.build/arm64-apple-macosx/debug/libGodotPlayground.dylib (0x1113b80d8) and /gd/GodotPlayground/.build/arm64-apple-macosx/debug/libGodotPlayground.dylib (0x11fc440d8). One of the two will be used. Which one is undefined.
tishin commented 7 months ago

In fact the extension can be as simple as

@_cdecl("swift_entry_point") public func enterExtension (interface: OpaquePointer?, library: OpaquePointer?, extension: OpaquePointer?) -> UInt8 {
    return 1
}

class SomeClass {}

with no dependencies at all and the issue will still be reproducible. It just needs a little tweak in Godot's source code (https://github.com/godotengine/godot/pull/87938), since Godot does not properly handle null in initialization.deinitialize

Looking into Godot's extension reload, dlclose(p_library_handle) on the old library does not fail, but the dlopen after it reports class implementation ambiguity. That being said, dlclose success does not mean the library is supposed to be unloaded immediately:

The function dlclose() decrements the reference count on the dynamic library handle handle. If the reference count drops to zero and no other loaded libraries use symbols in it, then the dynamic library is unloaded.

migueldeicaza commented 7 months ago

I do not think this is a Godot issue, but an interoperability issue between Swift and loading dynamic modules.

I created a very simple sample (https://tirania.org/tmp/demo-swift-dlopen.tar.gz) that exhibits the same issue, here is the sample for the main program:

import Foundation

func run () {
        let h = dlopen ("/tmp/libj.dylib", RTLD_LAZY)
        let methodSymbol = dlsym (h, "swift_entry_point")
        typealias MethodFunction = @convention(c) () -> UInt8
        let method = unsafeBitCast(methodSymbol, to: MethodFunction.self)
        let result = method()
        print("Result of method call: \(result)")

        // Close the dynamic library
        print ("Closing: \(dlclose(h))")
}

run ()
print ("Swap the library")
getchar()
run ();

And then I replace libj.dylib with libk.dylib in the pause, and I get the same error.

migueldeicaza commented 7 months ago

And I can no longer reproduce the error. I am not sure what I did, but now I am able to load both libraries, and the warning is gone, and the libraries do show the right output.

MrZak-dev commented 3 months ago

I am unable to hot reload the swift library in my project as well .

i am getting these editor errors , when updating the library and building it using swift build:

ERROR: Attempt to register extension class 'PlayerController', which appears to be already registered.
   at: _register_extension_class_internal (core/extension/gdextension.cpp:463)
ERROR: Attempt to register extension class 'PxPlayer', which appears to be already registered.
   at: _register_extension_class_internal (core/extension/gdextension.cpp:463)

this is my gdextension config

[configuration]

entry_symbol = "swift_entry_point"
compatibility_minimum = 4.3
reloadable = true

[libraries]

macos.debug = "res://bin/.build/arm64-apple-macosx/debug/libMyLib.dylib"

[dependencies]

macos.debug = {"res://bin/.build/arm64-apple-macosx/debug/libSwiftGodot.dylib" : ""}
migueldeicaza commented 3 months ago

There is a new fix for this specific issue on main, can you try it?

MrZak-dev commented 3 months ago

There is a new fix for this specific issue on main, can you try it?

I did my testing on main at the same time i wrote the github comment.

MrZak-dev commented 3 months ago

449 probably related

migueldeicaza commented 2 months ago

Ok, I spent some quality time on this issue, and there are two sets of problems:

(a) The current de-init code is slightly buggy, and it only de-initializes some parts, but not all.
(b) Even if this bug is fixed, we go back to the first issue, which is that our code is not being unloaded at all.

So even if you produce a new binary, dlopen still returns a reference to the original one.

What is puzzling is that a standalone C program loading Swift code and unloading it works, but something that Godot is doing prevents these extensions from being reloaded, because the same sample that can be unloaded and reloaded by the C program (with no SwiftGodot dependencies) fails to be reloaded when used inside Godot.

The sample extension I am using with Swift is this, notice that there are not dependencies on SwiftGodot at all:

var value = 1
@_cdecl("extension_init")
public func enterExtension () {
       print ("This is the library \(value)")
}

And this extension with this sample C program can be modified, and it will successfully reload it:

#include <stdio.h>
#include <dlfcn.h>

int main () {
  void *lib;

  while (1) {
    lib = dlopen("lib.dylib", RTLD_NOW);
    if (lib == NULL) {
      fprintf (stderr, "Failed to load %s\n", dlerror());
      return 1;
    }
    void (*addr)() = dlsym (lib, "extension_init");
    if (addr == NULL){
      fprintf (stderr, "No symbol");
      return 1;
    }
    (*addr)();
    printf ("Return to reload\n");
    getchar ();
      printf("REloading\n");
      dlclose (lib);
      lib = NULL;
  }
  return 0;
}
dsnopek commented 2 months ago

What is puzzling is that a standalone C program loading Swift code and unloading it works, but something that Godot is doing prevents these extensions from being reloaded, because the same sample that can be unloaded and reloaded by the C program (with no SwiftGodot dependencies) fails to be reloaded when used inside Godot.

It sounds like you're saying the simple sample here won't be reloaded correctly if used as a GDExtension with Godot?

I attempted to reproduce this on Linux, and it seemed to work fine for me, although, this is my first time using Swift, so I may be missing something. :-)

I used your sample program above:

var value = 1
@_cdecl("extension_init")
public func enterExtension () {
       print ("This is the library \(value)")
}

Which I compiled with:

swiftc test.swift -emit-module -emit-library -o swifttest.so

With this swifttest.gdextension:

[configuration]

entry_symbol = "extension_init"
compatibility_minimum = 4.3
reloadable = true

[libraries]

linux.debug = "res://swifttest.so"

To test, I changed value in the sample program, re-compiled, and then switched to/from the editor. My changes were represented in the output in the terminal!

I'll try the same process on MacOS in a little bit...

dsnopek commented 2 months ago

Interestingly, I am able to reproduce the problem on MacOS, following the same process as above! The output in the terminal won't change until I restart the Godot editor. So, it seems to be specific to MacOS in some way.

I also tried making a similar sample extension in C with this code:

#include <stdio.h>

int c_extension_init(void *p1, void *p2, void *p3) {
    int value = 1;
    printf("C Lib %d\n", value);
    return 0;
}

Compiled with:

gcc -fPIC -rdynamic -shared test.c -o ctest.so

And, the C version didn't have this issue! I could change value, re-compile and switching to/from the editor would lead to the new terminal output right away.

So, the issue is somehow specific to MacOS and Swift, and (per @migueldeicaza's earlier testing) it only happens when the extension is loaded by Godot. This is very interesting problem...

MrZak-dev commented 2 months ago

the issue is somehow specific to MacOS and Swift

I did some testing on both platforms, Hot reloading works fine on windows but it does not when using the SwiftGodot on MacOs

uaex commented 2 months ago

Swift is associated with Objective-C on the Apple platform, and the library loader (dyld) prevents uninstalling dynamic libraries with objc sections. Therefore, dlclose is usually not functional on the Apple platform.

@MrZak-dev @dsnopek @migueldeicaza

migueldeicaza commented 2 months ago

But in this case, we have a library that can be unloaded without Godot, but not with Godot.

Perhaps it is Godot's use of Objective-C that triggers this behavior? But if so, why would Rust still work.

uaex commented 2 months ago

Check objc sections in your library (with MachOView or other tools)

image

By the way, Empty.swift also emit __objc_imageinfo section, and then the library can never be unloaded. I think SwiftGodot does not work well for hot-reloading in Apple platform.

@migueldeicaza

samdeane commented 4 weeks ago

This post from Apple Developer Support may give a little more context: https://forums.developer.apple.com/forums/thread/122591

In particular, Quinn's final reply:

As a general rule, you shouldn’t expect dlclose to unload the code because there are many different constructs that cause that not to happen. The ones that spring immediately to mind are Objective-C or Swift runtime registration and C++ ODR. It’s very hard to tell whether you’re using such constructs and so behaviour like this can change in non-obvious ways.

I don't think that this is ever likely to work, sadly.

migueldeicaza commented 3 weeks ago

I am less concern about unloading the dynamic library than being able to load a new instance of it.

If the code remains in memory and unused, it can be swapped out, what I believe should be done is that Godot should copy the library to a unique name on MacOS, and then load the new library, that will still keep the old around (but it is not in use) and load the new one.

samdeane commented 3 weeks ago

If the code remains in memory and unused, it can be swapped out, what I believe should be done is that Godot should copy the library to a unique name on MacOS, and then load the new library, that will still keep the old around (but it is not in use) and load the new one.

Wouldn't you still get symbol collisions? I'm not sure that the dynamic linker would be smart enough to prefer the latest loaded version.

Something like the approach here and/or here might work though.

migueldeicaza commented 3 weeks ago

I don't think so. You use dlsym manually against a specific handle.

migueldeicaza commented 3 weeks ago

An interesting update:

https://github.com/godotengine/godot/issues/90108#issuecomment-2296583560

I will try the samples here later, but did not want to miss this information.