-
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 multiple reflect.Value uintptr=>unsafe.Pointer conversions in single statement #15329
Comments
/cc @rsc @ianlancetaylor for input on the issue / proposed solution |
This sounds like a bug in the current compiler and what you're proposing is a band-aid for a symptom, not a fix of the underlying cause. When evaluating f(e1, e2, e3) we should evaluate (completely, as far as can be observed) f, then e1, then e2, then e3. The spec says so. I'm not sure why we're hopping around evaluating parts of the e's in zigzag order. My guess is that it was done to improve codegen in the legacy compiler. Maybe you've just found a corner case in the "as far as can be observed" logic, and we just fix that. |
cmd/compile already rewrites statements to respect the Go spec's order of evaluation rules. E.g., it rewrites:
into
Note that the Go spec doesn't define the order of evaluation for all kinds of subexpressions (such as array/slice indexing), so cmd/compile only rewrites statements to avoid more than one ordered subexpression per statement. Conversions don't generally have visible side effects (aside from changing value representation), so the Go spec doesn't specify an ordering for them. Similarly, cmd/compile doesn't proactively rewrite them right now either. However, |
Maybe it would be enough to treat ptr<->uintptr conversions as a function call? |
cmd/compile/internal/gc/order.go must be changed to make ordercall include any outer uintptr->unsafe.Pointer conversion (but critically NOT the reverse) when pulling out the piece to be ordered. |
@rsc Yep, that's roughly what I was thinking as well. |
CL https://golang.org/cl/22273 mentions this issue. |
@rsc Any comments on CL 22273, and whether we should still try to submit it for 1.7? The issue has existed for a while, so it wouldn't be unreasonable to defer to 1.8. Also, I don't have any evidence of it being a problem that affects real-world applications. I've briefly reviewed all of the uses of unsafe.Pointer(reflectValue.Pointer()) in Google's internal code, and none of them appear at risk of triggering this issue. I'm personally a little wary of touching order.go right now after #15602 and #15604. |
I think this can / should wait for Go 1.8. |
[I thought I've pointed this out before, but I can't find the issue for it now, so filing again.]
tip.golang.org/pkg/unsafe says:
However, the Go compiler does not correctly handle this case when there are multiple reflect.Value.Pointer invocations in a single statement. For example: http://play.golang.org/p/lBhYwaBvSa
The problem is that when there are multiple function calls in a statement, cmd/compile rewrites all of the calls into separate statements that spill to temporary stack variables with the same type as their return value. Conversions are only applied after all of the function calls are evaluated, which leaves the reflect.Value.Pointer return value sitting on the stack for a while, at risk of becoming invalidated. I.e.,
is compiled as roughly:
I think the least surprising fix would be that we recognize the pattern
unsafe.Pointer(x())
wherex()
has typeuintptr
, and ensure its autotmp variable is typeunsafe.Pointer
instead ofuintptr
. This should fix both of the test cases in the sample program above.The text was updated successfully, but these errors were encountered: