Skip to content
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.

Sign up for GitHub

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

Jump to bottom

cmd/compile: mishandling multiple reflect.Value uintptr=>unsafe.Pointer conversions in single statement #15329

Closed
mdempsky opened this issue Apr 16, 2016 · 9 comments
Closed

cmd/compile: mishandling multiple reflect.Value uintptr=>unsafe.Pointer conversions in single statement #15329

mdempsky opened this issue Apr 16, 2016 · 9 comments
Labels
FrozenDueToAge
Milestone
Go1.8

Comments

@mdempsky
Copy link
Member

mdempsky commented Apr 16, 2016 •
edited

[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:

(5) Conversion of the result of reflect.Value.Pointer or reflect.Value.UnsafeAddr from uintptr to Pointer.

Package reflect's Value methods named Pointer and UnsafeAddr return type uintptr instead of unsafe.Pointer to keep callers from changing the result to an arbitrary type without first importing "unsafe". However, this means that the result is fragile and must be converted to Pointer immediately after making the call, in the same expression:

p := (*int)(unsafe.Pointer(reflect.ValueOf(new(int)).Pointer()))

As in the cases above, it is invalid to store the result before the conversion:

// INVALID: uintptr cannot be stored in variable
// before conversion back to Pointer.
u := reflect.ValueOf(new(int)).Pointer()
p := (*int)(unsafe.Pointer(u))

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.,

    eq(unsafe.Pointer(f(1).Pointer()), unsafe.Pointer(f(2).Pointer()))

is compiled as roughly:

    var autotmp_1 uintptr = f(1).Pointer()
    var autotmp_2 uintptr = f(2).Pointer()
    eq(unsafe.Pointer(autotmp_1), unsafe.Pointer(autotmp_2))

I think the least surprising fix would be that we recognize the pattern unsafe.Pointer(x()) where x() has type uintptr, and ensure its autotmp variable is type unsafe.Pointer instead of uintptr. This should fix both of the test cases in the sample program above.
@mdempsky
Copy link
Member Author

/cc @rsc @ianlancetaylor for input on the issue / proposed solution

@randall77
Copy link
Contributor

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.

@mdempsky
Copy link
Member Author

mdempsky commented Apr 19, 2016 •
edited

cmd/compile already rewrites statements to respect the Go spec's order of evaluation rules. E.g., it rewrites:

 x := y[f()] + int(g())

into

autotmp_f := f()
autotmp_g := g()
x := y[autotmp_f] + int(autotmp_g)

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, uintptr to unsafe.Pointer conversions do have visible side effects, as demonstrated above.

@bradfitz bradfitz added this to the Go1.7 milestone Apr 19, 2016
@randall77
Copy link
Contributor

Maybe it would be enough to treat ptr<->uintptr conversions as a function call?

@rsc
Copy link
Contributor

rsc commented Apr 19, 2016

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.

@mdempsky
Copy link
Member Author

@rsc Yep, that's roughly what I was thinking as well.

@mdempsky mdempsky self-assigned this Apr 19, 2016
@gopherbot
Copy link

CL https://golang.org/cl/22273 mentions this issue.

@mdempsky
Copy link
Member Author

@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.

@rsc
Copy link
Contributor

rsc commented May 17, 2016

I think this can / should wait for Go 1.8.

@rsc rsc modified the milestones: Go1.8, Go1.7 May 17, 2016
@golang golang locked and limited conversation to collaborators May 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
5 participants
@bradfitz @mdempsky @rsc @randall77 @gopherbot