Allow custom-gc SetFinalizer and clarify KeepAlive #3419
Conversation
src/runtime/runtime.go
Outdated
| // The compiler cannot see that alwaysFalse is always false, | ||
| // so it emits the test and keeps the call, giving the desired | ||
| // escape analysis result. The test is cheaper than the call. | ||
| var alwaysFalse bool |
There was a problem hiding this comment.
In Go, the variable is named cgoAlwaysFalse but I think this was just to reuse the same variable after the fact for something unrelated to cgo.
https://github.com/golang/go/blob/master/src/runtime/mfinal.go#L515
There was a problem hiding this comment.
I'm not convinced the new LTO logic will think alwaysFalse is potentially not always false. I think we need something else here that we can ensure at the compiler level.
There was a problem hiding this comment.
What do you think about just noinline being enough? If it seems so I could remove this stuff to prevent confusion.
There was a problem hiding this comment.
Went ahead and removed down to just adding go:noinline, verifying it actually does keepalive is probably quite tricky but in the meantime it can be attempted along with finalizers in custom GC.
anuraaga
force-pushed
the
setfinalizer
branch
from
c576539 to
843d29d
Compare
|
BTW I could implement https://github.com/corazawaf/coraza-proxy-wasm/compare/main...anuraaga:setfinalizer?expand=1 Only limitation is finalizers must be |
|
@dgryski Any other thoughts on this? Thanks! |
|
My only concern is if "no inline" really is enough to convince tinygo/llvm to keep allocations alive. We might need direct compiler support. /Cc @aykevl |
|
Not sure how hard that is, but assuming this PR doesn't make anything particularly worse, hopefully it's still possible to continue to fix forward. Mainly hoping to be able to implement a full custom GC for 0.27.0 including finalizer :) |
|
Saw 0.27.0 update merge to release branch - just curious if this could get in for 0.27.0. Otherwise finalizer would have to wait until 0.28, not the end of the world but figured doesn't hurt to check. |
|
No complaints from my end to get this into 0.27. |
|
@aykevl any other feedback before I squash/merge? |
aykevl
left a comment
aykevl
left a comment
There was a problem hiding this comment.
The SetFinalizer parts seem fine by me. But KeepAlive is still unimplemented with this PR, see my comment below.
src/runtime/runtime.go
Outdated
| //go:noinline | ||
| func KeepAlive(x interface{}) { | ||
| // Unimplemented. Only required with SetFinalizer(). | ||
| } | ||
|
|
||
| func SetFinalizer(obj interface{}, finalizer interface{}) { | ||
| // Unimplemented. | ||
| } |
There was a problem hiding this comment.
//go:noinline doesn't do anything here, except avoid calling a no-op function. Most importantly, this does nothing to keep an object alive: LLVM will happily remove the x parameter because it is not used.
I think the only reliable way to implement this is either by using a volatile operation, or by calling an assembly stub (that simply returns).
There was a problem hiding this comment.
Thanks for the hint, I went ahead and removed the inline and added a TODO based on it. Would like to dig into it in a followup
|
Thank you! I'll take a look at implementing |
|
Implemented |
4 participants
In #3336,
SetFinalizerwas moved to common code out of GC implementations. This prevents custom-gc from trying to implement finalizers (I'd like to). There is a bit of copy-paste among GC implementations because of it, but especially withgc_blocksit's less than it could have been.At the same time, this reimplements
KeepAlivein the same way as Go - while the Go document does make a big deal about finalizers, close reading showsensures that the object is not freed, which can be important with unsafe code. I don't thinkKeepAlivehas a strong relation toSetFinalizeras is currently documented. In tinygo wasi's case, I believe this specifically means that the shadow stack slot for a variable is not reused by a different one until the call toKeepAlive(for non-wasi, same thing but registers?). However, I need help in knowing whether theThe compiler cannot see that alwaysFalse is always false,note is valid for LLVM as well - intuitively for me, justnoinlinewould be enough to ensure the variable is kept alive for the function call.