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

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

Open
tishin opened this issue Feb 4, 2024 · 19 comments
Open

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

tishin opened this issue Feb 4, 2024 · 19 comments

Comments

@tishin
Copy link
Contributor

tishin commented Feb 4, 2024

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
Copy link
Contributor Author

tishin commented Feb 4, 2024

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 (godotengine/godot#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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Owner

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

@MrZak-dev
Copy link
Contributor

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
Copy link
Contributor

MrZak-dev commented Jun 13, 2024

#449 probably related

@migueldeicaza
Copy link
Owner

migueldeicaza commented Jun 16, 2024

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
Copy link

dsnopek commented Jun 17, 2024

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
Copy link

dsnopek commented Jun 17, 2024

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
Copy link
Contributor

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
Copy link

uaex commented Jun 24, 2024

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
Copy link
Owner

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
Copy link

uaex commented Jun 25, 2024

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
Copy link

samdeane commented Aug 13, 2024

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
Copy link
Owner

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
Copy link

samdeane commented Aug 16, 2024

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
Copy link
Owner

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

@migueldeicaza
Copy link
Owner

An interesting update:

godotengine/godot#90108 (comment)

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

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

6 participants