-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
cmd/compile: mishandling of unsafe-uintptr arguments #24491
Comments
Also for context, these most likely only affect |
Apparently, we do use
but neither includes a And
I couldn't find any obvious at-risk calls within Google's Go code base. |
Mostly unrelated, but: It would be nice if |
Is there anything preventing us from recognizing this pattern when we lower call ASTs to SSA and inserting the keep-alives then? I think this is similar to your suggestion, but I'm proposing to take this out of "order" entirely. Case 3 is interesting. I think you're right that the only option may be to wrap it in a closure, since (at least for |
@josharian That sounds like an interesting issue, but seems pretty different in scope from this one to me. I suspect it overlaps more significantly with the ideas we've had to optimize exactly-once defer statements. @aclements We might be able to move all of the work into |
See also: https://go-review.googlesource.com/c/go/+/114797 |
Hello @dr2chase, CL https://go-review.googlesource.com/c/go/+/114797 was merged in October 2018, I am just paging you to let you know as per your comment above. Thanks. |
https://play.golang.org/p/KEf2cpdL_mA demonstrates the go/defer issue. (From #38581, which I closed as duplicate of this.) |
It's interesting that as of go1.15, the program now sometimes print ok for defer (just run it several times on the playground link). |
@cuonglm Interesting observation. It looks like the "go" test occasionally fails too. It looks like calling Edit: Actually, it's still not reliable. I just ran it again on the playground, and I saw "1 / 100" for defer. |
Change https://golang.org/cl/253457 mentions this issue: |
Currently, the statement: go g(uintptr(f())) gets rewritten into: tmp := f() newproc(8, g, uintptr(tmp)) runtime.KeepAlive(tmp) which doesn't guarantee that tmp is still alive by time the g call is scheduled to run. This CL fixes the issue, by wrapping g call in a closure: go func(p unsafe.Pointer) { g(uintptr(p)) }(f()) then this will be rewritten into: tmp := f() go func(p unsafe.Pointer) { g(uintptr(p)) runtime.KeepAlive(p) }(tmp) runtime.KeepAlive(tmp) // superfluous, but harmless So the unsafe.Pointer p will be kept alive at the time g call runs. Updates #24491 Change-Id: Ic10821251cbb1b0073daec92b82a866c6ebaf567 Reviewed-on: https://go-review.googlesource.com/c/go/+/253457 Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> Reviewed-by: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
@mdempsky I wonder how can we write a test for the second case (tmp is kept alive longer than expected?). The first can be test by:
|
Change https://golang.org/cl/254397 mentions this issue: |
@cuonglm I think you just need an expression like:
And then we want to make sure the value is still alive in For example, this appears to work:
|
I worry this test could easily be flaky; finalizers are run in another thread, with no particular guarantee of timeliness. |
Yes, it’s flaky, but we end up with different one afb5fca |
When calling certain functions that take uintptr parameters with unsafe.Pointer arguments, the compiler rewrites the call to ensure the pointer is kept alive through the call. (See #13372 for more background.) For example, given
the expression statement
gets rewritten during order into (roughly)
There are a handful of related issues with how this is currently implemented. (I expect some of these will be solved separately, so splitting them into separate issues may be appropriate. I'm just noting them all together for now in case it helps us think of a more holistic solution.)
If the caller directly returns the results, the KeepAlive statement is ineffectual because it's dropped by ssa.go as dead code. That's because
gets rewritten into
so tmp isn't actually kept alive through the call to g.
Because the KeepAlives are added after the statement, the pointer is kept alive longer than necessary. For example,
gets rewritten into
so tmp is kept alive
They don't work with
defer
orgo
calls. For example,gets rewritten into
which doesn't guarantee that tmp is still alive by time the g call is scheduled to run.
I expect 1 and 2 can be fixed by attaching the KeepAlives directly to the OCALLFUNC node, so that we can insert the OpVarLive immediately after the OpStaticCall/etc.
I expect 3 will need to be fixed by wrapping the calls in a closure.
/cc @ianlancetaylor @aclements
The text was updated successfully, but these errors were encountered: