[SwiftKit] Move 'selfPointer' to 'JNISwiftInstance' & downcall trace logs #322

ktoso · 2025-07-16T00:33:48Z

Seems I could not push "to" the PR here #321

Minor refactor, we can easily keep the pointer in the base type

`FFMSwiftInstance` doesn't need it.

printing

ktoso · 2025-07-16T12:41:51Z

Sources/JExtractSwiftLib/JNI/JNISwift2JavaGenerator+JavaBindingsPrinting.swift


      let returnKeyword = translatedDecl.translatedFunctionSignature.resultType.isVoid ? "" : "return "

      printer.print(
        """
-        long selfPointer = this.pointer();
+        long \(selfVarName) = this.$memoryAddress();


These are basically unsafe, so I figured we shoudl hide them using our $ pattern.

ktoso · 2025-07-16T12:42:21Z

Sources/JExtractSwiftLib/JNI/JNISwift2JavaGenerator+JavaBindingsPrinting.swift

@@ -206,13 +206,15 @@ extension JNISwift2JavaGenerator {
    printDeclDocumentation(&printer, decl)
    printer.printBraceBlock("public \(renderFunctionSignature(decl))") { printer in
      var arguments = translatedDecl.translatedFunctionSignature.parameters.map(\.name)
-      arguments.append("selfPointer")
+
+      let selfVarName = "self$"


The reason we're doing $ in names in bodies is so they don't clash with any parameter name that someone can come up with, so I moved to self$ here -- FYI @madsodgaard

ktoso · 2025-07-16T12:42:41Z

Sources/JExtractSwiftLib/JNI/JNISwift2JavaGenerator+JavaBindingsPrinting.swift

+          CallTraces.traceDowncall("\(type.swiftNominal.name).\(funcName)",
+              "this", this,
+              "self", self$);
+        }


a bit of downtracing helped diagnose issues

ktoso · 2025-07-16T12:43:34Z

Sources/JExtractSwiftLib/JNI/JNISwift2JavaGenerator+SwiftThunkPrinting.swift

+        fatalError("Missing self pointer in call to \\(#function)!")
+      }
+      """
+    )


This code was repeated 2 times, so now it's in 1 place.

The reason to split it up into more lines and not just the single line with multiple ! is so we have better messages when things fail; It took me a while to diagnose an issue without this so I'd suggest to keep this in.

ktoso · 2025-07-16T12:44:03Z

SwiftKitCore/src/main/java/org/swift/swiftkit/core/SwiftInstance.java

+
+    // TODO: make this a flagset integer and/or use a field updater
+    /** Used to track additional state of the underlying object, e.g. if it was explicitly destroyed. */
+    private final AtomicBoolean $state$destroyed = new AtomicBoolean(false);


fields should be before methods; we should install a formatting tool tbh

ktoso · 2025-07-16T12:44:19Z

SwiftKitCore/src/main/java/org/swift/swiftkit/core/JNISwiftInstance.java

@@ -42,7 +55,7 @@ protected JNISwiftInstance(long pointer, SwiftArena arena) {
    protected abstract Runnable $createDestroyFunction();

    @Override
-    public SwiftInstanceCleanup createCleanupAction() {
+    public SwiftInstanceCleanup $createCleanup() {


Also not a method end users should be invoking explicitly, so changed it while at it.

ktoso · 2025-07-16T12:44:55Z

SwiftKitCore/src/main/java/org/swift/swiftkit/core/SwiftInstance.java

@@ -17,49 +17,38 @@
 import java.util.concurrent.atomic.AtomicBoolean;

 public abstract class SwiftInstance {
-    /// Pointer to the "self".
-    private final long selfPointer;


"The change" the SwiftInstance does not need storage, we can just have FFM or JNI one implement the abstract method instead

ktoso · 2025-07-16T12:45:26Z

SwiftKitCore/src/main/java/org/swift/swiftkit/core/SwiftInstance.java

-     */
-    protected SwiftInstance(long pointer, SwiftArena arena) {
-        this.selfPointer = pointer;
-        arena.register(this);


Necessary to remove this since otherwise we'd race the register with a non-initialized memory address value in the class calling super()

ktoso · 2025-07-16T12:45:45Z

SwiftKitCore/src/main/java/org/swift/swiftkit/core/SwiftObjects.java

+/**
+ * Utility functions, similar to @{link java.util.Objects}
+ */
+public class SwiftObjects {


Following java.util.Objects but with more helpers for us ;)

ktoso · 2025-07-16T12:45:54Z

SwiftKitFFM/src/main/java/org/swift/swiftkit/ffm/FFMSwiftInstance.java

+    @Override
+    public long $memoryAddress() {
+        return $memorySegment().address();
+    }


madsodgaard

Very nice 👍

madsodgaard · 2025-07-16T15:45:43Z

Sources/JExtractSwiftLib/JNI/JNISwift2JavaGenerator+SwiftThunkPrinting.swift

+      let selfBits$ = Int(Int64(fromJNI: \(selfPointerParam.name), in: env$))
+      assert(selfBits$ != 0, "$self memory address was null: \(selfPointerParam.name) = \\(\(selfPointerParam.name))" )
+      guard let \(newSelfParamName) = UnsafeMutablePointer<\(swiftParentName)>(bitPattern: selfBits$) else {
+        fatalError("Missing self pointer in call to \\(#function)!")


I think we can move the above assert message to this fatalError instead. The UnsafeMutablePointer initializer will fail if the bitPattern is zero.

Good point, done

ktoso · 2025-07-17T04:53:39Z

Continued spm instability on linux :(

#314

ktoso requested a review from rintaro July 16, 2025 00:33

ktoso mentioned this pull request Jul 16, 2025

[SwiftKit] Move 'selfPointer' to 'JNISwiftInstance' #321

Closed

ktoso changed the title ~~Swiftkit selfpointer~~ [SwiftKit] Move 'selfPointer' to 'JNISwiftInstance' Jul 16, 2025

ktoso changed the title ~~[SwiftKit] Move 'selfPointer' to 'JNISwiftInstance'~~ [SwiftKit] Move 'selfPointer' to 'JNISwiftInstance' & downcall trace logs Jul 16, 2025

ktoso marked this pull request as draft July 16, 2025 09:37

ktoso force-pushed the swiftkit-selfpointer branch from 7c96b3b to e7612f0 Compare July 16, 2025 11:03

rintaro and others added 10 commits July 16, 2025 21:00

[SwiftKit] Move 'selfPointer' to 'JNISwiftInstance'

c41bfb0

`FFMSwiftInstance` doesn't need it.

keep long returning memory address func in base SwiftInstance

e444700

Add CallTraces to swiftkit core and use it while downcalling

21e409d

Change pointer() to $memoryAddress

f20c103

Centralize the printing of long -> "self$" unsafe pointer in JNI

7ca6bc9

printing

rename cleanup create function to $ func as well

17197e2

reorder fields in SwiftInstance, fields before methods

4092f36

fix bug in that we must initialize pointer before registering cleanup

4dffc5f

fix tests after thunk body refactors

602a6ac

update tests

49e95c6

ktoso force-pushed the swiftkit-selfpointer branch from 2d2318a to 49e95c6 Compare July 16, 2025 12:00

ktoso commented Jul 16, 2025

View reviewed changes

fix: test harness would incorrectly match less than "detect" size chunks

e7c0635

madsodgaard reviewed Jul 16, 2025

View reviewed changes

fix tests since we changed how we get env

52d4d92

review followup: assertion message

b9fd0a4

ktoso marked this pull request as ready for review July 17, 2025 03:30

ktoso merged commit c0f166d into swiftlang:main Jul 17, 2025
108 of 112 checks passed

[SwiftKit] Move 'selfPointer' to 'JNISwiftInstance' & downcall trace logs #322

[SwiftKit] Move 'selfPointer' to 'JNISwiftInstance' & downcall trace logs #322

Uh oh!

Conversation

ktoso commented Jul 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madsodgaard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!