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.

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

reflect: method parameters getting wrong values on amd64p32 #33628

Closed
seebs opened this issue Aug 13, 2019 · 4 comments
Closed

reflect: method parameters getting wrong values on amd64p32 #33628

seebs opened this issue Aug 13, 2019 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-NaCl GOOS=nacl, Native Client, removed in Go 1.14
Milestone

Comments

@seebs
Copy link
Contributor

seebs commented Aug 13, 2019

What version of Go are you using (go version)?

Playground. Can't reproduce elsewhere, but the actual behavior seems very unlikely to be playground-specific, except obviously it is.

$ go version
1.12.7

Does this issue reproduce with the latest release?

I think so.

What operating system and processor architecture are you using (go env)?

Various. Only shows up on playground, which I think is 386.

What did you do?

Obtain a method using reflect, convert it to a function of its type, and call the function.

Here's a slightly verbose version, using both int32 and int64 parameters.
https://play.golang.org/p/PX0pVeYzswQ

Calls to the same method, directly and using reflect, report in their stack traces:

main.(*Updater).Update(0x414020, 0x40c108, 0x7d0, 0x0, 0x41a7a8, 0x1) // works
main.(*Updater).Update(0x414020, 0x7d0, 0x0, 0x213, 0x0, 0x0) // doesn't work
main.(*Updater32).Update(0x414078, 0x7d0, 0x13846d, 0x3) // works
main.(*Updater32).Update(0x414078, 0x7d0, 0x0, 0x446514) // also works

The same lines from a local machine, running GOARCH=386, and running 386 go:

main.(*Updater).Update(0x8812138, 0x7d0, 0x0, 0x3)
main.(*Updater).Update(0x8812138, 0x7d0, 0x0, 0x0)
main.(*Updater32).Update(0x88121a8, 0x7d0, 0x811492c)
main.(*Updater32).Update(0x88121a8, 0x7d0, 0x0)

So there's two things that stand out here. One is that on the playground, with int64, only the one that looks wrong is actually working.

What did you expect to see?

I expected that to work.

What did you see instead?

If the parameter in question is int64, I see garbage values because the call parameters appear to be out of sync in some way.

@tv42
Copy link

tv42 commented Aug 13, 2019

Minimized: https://play.golang.org/p/RfvQzdI-ZnJ

I'm printing in hex because the number seems so nicely rounded.

@seebs
Copy link
Contributor Author

seebs commented Aug 13, 2019

Yeah, agreed. And I think in the stack trace, it looks like the working one passed in that extra 0x40c108, and thus aligned the argument with where the called function expected it, but I don't actually know what that value is supposed to be.

@cherrymui
Copy link
Member

Thanks for the report.

This seems amd64p32 specific (which the playground uses).

main.(*Updater).Update(0x414020, 0x40c108, 0x7d0, 0x0, 0x41a7a8, 0x1) // works
main.(*Updater).Update(0x414020, 0x7d0, 0x0, 0x213, 0x0, 0x0) // doesn't work

Apparently we didn't marshal arguments correctly in reflect method call on amd64p32.

@cherrymui cherrymui changed the title playground(???): method parameters getting wrong values (using reflect) reflect: method parameters getting wrong values on amd64p32 Aug 13, 2019
@gopherbot
Copy link

Change https://golang.org/cl/190297 mentions this issue: reflect: align first argument in callMethod

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 15, 2019
@dmitshur dmitshur added this to the Go1.13 milestone Aug 15, 2019
@dmitshur dmitshur added the OS-NaCl GOOS=nacl, Native Client, removed in Go 1.14 label Aug 15, 2019
@golang golang locked and limited conversation to collaborators Aug 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-NaCl GOOS=nacl, Native Client, removed in Go 1.14
Projects
None yet
Development

No branches or pull requests

5 participants