[SwiftKit] Move 'selfPointer' to 'JNISwiftInstance' & downcall trace logs #322
Conversation
ktoso
changed the title
ktoso
changed the title
ktoso
force-pushed
the
swiftkit-selfpointer
branch
from
7c96b3b to
e7612f0
Compare
`FFMSwiftInstance` doesn't need it.
ktoso
force-pushed
the
swiftkit-selfpointer
branch
from
2d2318a to
49e95c6
Compare
| printer.print( | ||
| """ | ||
| long selfPointer = this.pointer(); | ||
| long \(selfVarName) = this.$memoryAddress(); |
There was a problem hiding this comment.
These are basically unsafe, so I figured we shoudl hide them using our $ pattern.
| var arguments = translatedDecl.translatedFunctionSignature.parameters.map(\.name) | ||
| arguments.append("selfPointer") | ||
|
|
||
| let selfVarName = "self$" |
There was a problem hiding this comment.
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
| CallTraces.traceDowncall("\(type.swiftNominal.name).\(funcName)", | ||
| "this", this, | ||
| "self", self$); | ||
| } |
There was a problem hiding this comment.
a bit of downtracing helped diagnose issues
| fatalError("Missing self pointer in call to \\(#function)!") | ||
| } | ||
| """ | ||
| ) |
There was a problem hiding this comment.
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.
|
|
||
| // 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); |
There was a problem hiding this comment.
fields should be before methods; we should install a formatting tool tbh
|
|
||
| @Override | ||
| public SwiftInstanceCleanup createCleanupAction() { | ||
| public SwiftInstanceCleanup $createCleanup() { |
There was a problem hiding this comment.
Also not a method end users should be invoking explicitly, so changed it while at it.
|
|
||
| public abstract class SwiftInstance { | ||
| /// Pointer to the "self". | ||
| private final long selfPointer; |
There was a problem hiding this comment.
"The change" the SwiftInstance does not need storage, we can just have FFM or JNI one implement the abstract method instead
| */ | ||
| protected SwiftInstance(long pointer, SwiftArena arena) { | ||
| this.selfPointer = pointer; | ||
| arena.register(this); |
There was a problem hiding this comment.
Necessary to remove this since otherwise we'd race the register with a non-initialized memory address value in the class calling super()
| /** | ||
| * Utility functions, similar to @{link java.util.Objects} | ||
| */ | ||
| public class SwiftObjects { |
There was a problem hiding this comment.
Following java.util.Objects but with more helpers for us ;)
| @Override | ||
| public long $memoryAddress() { | ||
| return $memorySegment().address(); | ||
| } |
| 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)!") |
There was a problem hiding this comment.
I think we can move the above assert message to this fatalError instead. The UnsafeMutablePointer initializer will fail if the bitPattern is zero.
|
Continued spm instability on linux :( |
3 participants
Seems I could not push "to" the PR here #321
Minor refactor, we can easily keep the pointer in the base type