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

go/reflect: Incorrect behavior on arm64 when using MakeFunc / Call #52788

Closed
znkr opened this issue May 9, 2022 · 12 comments
Closed

go/reflect: Incorrect behavior on arm64 when using MakeFunc / Call #52788

znkr opened this issue May 9, 2022 · 12 comments
Labels
arch-arm64 FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@znkr
Copy link
Contributor

znkr commented May 9, 2022

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

go version devel go1.19-c4311a4 Mon May 9 12:32:48 2022 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes (using gotip)

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

QEMU AARCH64 emulation (this bug does not reproduce on x86)

What did you do?

% GOARCH=arm64 gotip build main.go
% qemu-aarch64 main

With main.go

package main

import (
	"fmt"
	"reflect"
)

func f(next func() bool) {
	for b := next(); b; b = next() {
		fmt.Printf("next() returned %v\n", b)
	}
}

func main() {
	next := reflect.MakeFunc(reflect.TypeOf((func() bool)(nil)), func(_ []reflect.Value) []reflect.Value {
		return []reflect.Value{reflect.ValueOf(false)}
	})
	reflect.ValueOf(f).Call([]reflect.Value{next})
}

What did you expect to see?

Empty output

What did you see instead?

unexpected fault address 0x2000051c760
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x1 addr=0x2000051c760 pc=0x91518]

goroutine 1 [running]:
runtime.throw({0xb675b?, 0x56f54?})
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/panic.go:1028 +0x40 fp=0x400005f500 sp=0x400005f4d0 pc=0x40c40
runtime.sigpanic()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/signal_unix.go:830 +0x1a0 fp=0x400005f530 sp=0x400005f500 pc=0x55a10
fmt.(*pp).printArg(0x400010a680, {0xa65a0?, 0x2000051c760}, 0x76)
        /usr/local/google/home/floriank/sdk/gotip/src/fmt/print.go:661 +0x508 fp=0x400005f5e0 sp=0x400005f540 pc=0x91518
fmt.(*pp).doPrintf(0x400010a680, {0xb8d75, 0x13}, {0x400005f788?, 0x1, 0x1})
        /usr/local/google/home/floriank/sdk/gotip/src/fmt/print.go:1026 +0x210 fp=0x400005f6e0 sp=0x400005f5e0 pc=0x93f60
fmt.Fprintf({0xd77d8, 0x400000e018}, {0xb8d75, 0x13}, {0x400005f788, 0x1, 0x1})
        /usr/local/google/home/floriank/sdk/gotip/src/fmt/print.go:204 +0x54 fp=0x400005f740 sp=0x400005f6e0 pc=0x8e7a4
fmt.Printf(...)
        /usr/local/google/home/floriank/sdk/gotip/src/fmt/print.go:213
main.f(0x400007a240)
        /usr/local/google/home/floriank/bug/main.go:10 +0x80 fp=0x400005f7a0 sp=0x400005f740 pc=0x94e00
runtime.call16(0x400007a270, 0xbf058, 0x0, 0x0, 0x0, 0x8, 0x400005fce0)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/asm_arm64.s:461 +0x78 fp=0x400005f7c0 sp=0x400005f7a0 pc=0x6a488
runtime.reflectcall(0xa6860?, 0x400007a240?, 0x1?, 0xb8a18?, 0x0?, 0x12?, 0xa6860?)
        :1 +0x34 fp=0x400005f800 sp=0x400005f7c0 pc=0x6de14
reflect.Value.call({0xa6b20?, 0xbf058?, 0x1?}, {0xb6601, 0x4}, {0x4000104f50, 0x1, 0x1?})
        /usr/local/google/home/floriank/sdk/gotip/src/reflect/value.go:585 +0x61c fp=0x400005fe70 sp=0x400005f800 pc=0x819cc
reflect.Value.Call({0xa6b20?, 0xbf058?, 0xbf060?}, {0x4000104f50?, 0x0?, 0x0?})
        /usr/local/google/home/floriank/sdk/gotip/src/reflect/value.go:368 +0x90 fp=0x400005fef0 sp=0x400005fe70 pc=0x81300
main.main()
        /usr/local/google/home/floriank/bug/main.go:18 +0x148 fp=0x400005ff70 sp=0x400005fef0 pc=0x950d8
runtime.main()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:250 +0x254 fp=0x400005ffd0 sp=0x400005ff70 pc=0x43234
runtime.goexit()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/asm_arm64.s:1155 +0x4 fp=0x400005ffd0 sp=0x400005ffd0 pc=0x6c4a4

goroutine 2 [force gc (idle)]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:363 +0xe4 fp=0x400004efa0 sp=0x400004ef80 pc=0x43604
runtime.goparkunlock(...)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:369
runtime.forcegchelper()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:302 +0xb4 fp=0x400004efd0 sp=0x400004efa0 pc=0x43494
runtime.goexit()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/asm_arm64.s:1155 +0x4 fp=0x400004efd0 sp=0x400004efd0 pc=0x6c4a4
created by runtime.init.6
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:290 +0x24

goroutine 3 [GC sweep wait]:
runtime.gopark(0x0?, 0x0?, 0x0?, 0x0?, 0x0?)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:363 +0xe4 fp=0x400004f770 sp=0x400004f750 pc=0x43604
runtime.goparkunlock(...)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:369
runtime.bgsweep(0x0?)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mgcsweep.go:278 +0xa4 fp=0x400004f7b0 sp=0x400004f770 pc=0x309e4
runtime.gcenable.func1()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mgc.go:178 +0x28 fp=0x400004f7d0 sp=0x400004f7b0 pc=0x25408
runtime.goexit()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/asm_arm64.s:1155 +0x4 fp=0x400004f7d0 sp=0x400004f7d0 pc=0x6c4a4
created by runtime.gcenable
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mgc.go:178 +0x70

goroutine 4 [GC scavenge wait]:
runtime.gopark(0x4000074000?, 0xd71a8?, 0x1?, 0x0?, 0x0?)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:363 +0xe4 fp=0x400004ff50 sp=0x400004ff30 pc=0x43604
runtime.goparkunlock(...)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:369
runtime.(*scavengerState).park(0x158600)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mgcscavenge.go:389 +0x5c fp=0x400004ff80 sp=0x400004ff50 pc=0x2ea0c
runtime.bgscavenge(0x0?)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mgcscavenge.go:617 +0x44 fp=0x400004ffb0 sp=0x400004ff80 pc=0x2ef74
runtime.gcenable.func2()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mgc.go:179 +0x28 fp=0x400004ffd0 sp=0x400004ffb0 pc=0x253a8
runtime.goexit()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/asm_arm64.s:1155 +0x4 fp=0x400004ffd0 sp=0x400004ffd0 pc=0x6c4a4
created by runtime.gcenable
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mgc.go:179 +0xb4

goroutine 5 [finalizer wait]:
runtime.gopark(0x60000000000000?, 0x0?, 0x8?, 0x21?, 0x2000000000?)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:363 +0xe4 fp=0x400004e580 sp=0x400004e560 pc=0x43604
runtime.goparkunlock(...)
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/proc.go:369
runtime.runfinq()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mfinal.go:180 +0x128 fp=0x400004e7d0 sp=0x400004e580 pc=0x24628
runtime.goexit()
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/asm_arm64.s:1155 +0x4 fp=0x400004e7d0 sp=0x400004e7d0 pc=0x6c4a4
created by runtime.createfing
        /usr/local/google/home/floriank/sdk/gotip/src/runtime/mfinal.go:157 +0x94
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/405114 mentions this issue: cmd/compile: fix If lowering on ARM64

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/405115 mentions this issue: cmd/compile: more fix on boolean ops on ARM64

gopherbot pushed a commit that referenced this issue May 9, 2022
Following CL 405114, the extension rule is also wrong. It is safe
to drop the extension if the value is from a boolean-generating
instruction, but not a boolean-typed Value in general (e.g. a Phi
or a in-register parameter). Fix it.

Updates #52788.

Change-Id: Icf3028fe8e90806f9f57fbe2b38d47da27a97e2a
Reviewed-on: https://go-review.googlesource.com/c/go/+/405115
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/405116 mentions this issue: cmd/compile: fix boolean comparison on PPC64 and RISCV64

gopherbot pushed a commit that referenced this issue May 10, 2022
Following CL 405114, for PPC64.

Should fix PPC64 builds.

Updates #52788.

Change-Id: I193ac31cfba18b4f907dd2523b51368251fd6fad
Reviewed-on: https://go-review.googlesource.com/c/go/+/405116
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/405553 mentions this issue: cmd/compile: fix boolean comparison on RISCV64

gopherbot pushed a commit that referenced this issue May 12, 2022
Following CL 405114, for RISCV64.

May fix RISCV64 builds.

Updates #52788.

Change-Id: Ifc34658703d1e8b97665e7b862060152e3005d71
Reviewed-on: https://go-review.googlesource.com/c/go/+/405553
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
@cherrymui
Copy link
Member

@gopherbot please open backport issues for previous releases. This is a miscompilation that could cause valid programs to behave incorrectly. Thanks.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #53396 (for 1.17), #53397 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@erifan
Copy link

erifan commented Jun 16, 2022

It seems that reflect.intToReg does not clear the high bits of the return value register. 32-bit or 8-bit arm64 instruction operations usually clear the unused high bits, which is why normal function calls do not have this problem. Do we need to make this change?

@cherrymui
Copy link
Member

No. In general the compiler and the ABI only guarantee that the low bits of a Value is meaningful.

@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 3, 2022
@dmitshur dmitshur added this to the Go1.19 milestone Aug 3, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/421457 mentions this issue: [release-branch.go1.18] cmd/compile: fix If lowering on ARM64

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/421459 mentions this issue: [release-branch.go1.18] cmd/compile: fix boolean comparison on PPC64

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/421460 mentions this issue: [release-branch.go1.18] cmd/compile: fix boolean comparison on RISCV64

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/421458 mentions this issue: [release-branch.go1.18] cmd/compile: more fix on boolean ops on ARM64

gopherbot pushed a commit that referenced this issue Aug 8, 2022
On ARM64, an If block is lowered to (NZ cond yes no). This is
incorrect because cond is a boolean value and therefore only the
last byte is meaningful (same as AMD64, see ARM64Ops.go). But here
we are comparing a full register width with 0. Correct it by
comparing only the last bit.

For #52788.
Fixes #53397.

Change-Id: I2cacf9f3d2f45e149c361a290f511b2d4ed845c4
Reviewed-on: https://go-review.googlesource.com/c/go/+/405114
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/421457
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
gopherbot pushed a commit that referenced this issue Aug 8, 2022
Following CL 421457, the extension rule is also wrong. It is safe
to drop the extension if the value is from a boolean-generating
instruction, but not a boolean-typed Value in general (e.g. a Phi
or a in-register parameter). Fix it.

Updates #52788.
Updates #53397.

Change-Id: Icf3028fe8e90806f9f57fbe2b38d47da27a97e2a
Reviewed-on: https://go-review.googlesource.com/c/go/+/405115
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/421458
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Aug 8, 2022
Following CL 421457, for PPC64.

Should fix PPC64 builds.

Updates #52788.
Updates #53397.

Change-Id: I193ac31cfba18b4f907dd2523b51368251fd6fad
Reviewed-on: https://go-review.googlesource.com/c/go/+/405116
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/421459
Reviewed-by: Cherry Mui <cherryyz@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Aug 8, 2022
Following CL 421457, for RISCV64.

May fix RISCV64 builds.

Updates #52788.
Updates #53397.

Change-Id: Ifc34658703d1e8b97665e7b862060152e3005d71
Reviewed-on: https://go-review.googlesource.com/c/go/+/405553
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/421460
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@golang golang locked and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants