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

Jump to bottom

cmd/compile: MOVWreg missing sign-extension following a Copy from a floating-point LoadReg #50671

Closed
bcmills opened this issue Jan 18, 2022 · 9 comments
Closed

cmd/compile: MOVWreg missing sign-extension following a Copy from a floating-point LoadReg #50671

bcmills opened this issue Jan 18, 2022 · 9 comments
Labels
arch-mips FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented Jan 18, 2022 

runtime: out of memory: cannot allocate 28862184423424-byte block (3932160 in use)
fatal error: out of memory

goroutine 6 [running]:
runtime.throw({0x1e98f5, 0xd})
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/runtime/panic.go:992 +0x6c fp=0xc0000bb848 sp=0xc0000bb820 pc=0x52c44
runtime.(*mcache).allocLarge(0xfff3af4108, 0x1a4000000960, 0x1)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/runtime/mcache.go:215 +0x2dc fp=0xc0000bb890 sp=0xc0000bb848 pc=0x29cf4
runtime.mallocgc(0x1a4000000960, 0x1d6600, 0x1)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/runtime/malloc.go:1096 +0x5b4 fp=0xc0000bb910 sp=0xc0000bb890 pc=0x1e324
runtime.makeslice(0x1d6600, 0x11800000064, 0x11800000064)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/runtime/slice.go:103 +0x94 fp=0xc0000bb938 sp=0xc0000bb910 pc=0x71014
golang.org/x/image/draw.newDistrib(0x2e6510, 0x64, 0x118)
	/tmp/workdir-host-linux-mipsle-mengzhuo/gopath/src/golang.org/x/image/draw/scale.go:248 +0xc4 fp=0xc0000bba28 sp=0xc0000bb938 pc=0x1981a4
golang.org/x/image/draw.(*Kernel).newScaler(0x2e6510, 0x64, 0x64, 0x118, 0x168, 0x0)
	/tmp/workdir-host-linux-mipsle-mengzhuo/gopath/src/golang.org/x/image/draw/scale.go:142 +0x50 fp=0xc0000bbad8 sp=0xc0000bba28 pc=0x197d20
golang.org/x/image/draw.(*Kernel).Scale(0x2e6510, {0x21f080, 0xc00006a940}, {{0x0, 0x0}, {0x64, 0x64}}, {0x21ecb0, 0xc0000a2080}, {{0x0, ...}, ...}, ...)
	/tmp/workdir-host-linux-mipsle-mengzhuo/gopath/src/golang.org/x/image/draw/scale.go:126 +0x80 fp=0xc0000bbb58 sp=0xc0000bbad8 pc=0x197bb0
golang.org/x/image/draw.testInterp(0xc000084680, 0x64, 0x64, {0x1e789d, 0x4}, {0x1e9499, 0xc}, {0x1e973b, 0xd})
	/tmp/workdir-host-linux-mipsle-mengzhuo/gopath/src/golang.org/x/image/draw/scale_test.go:83 +0xa50 fp=0xc0000bbf18 sp=0xc0000bbb58 pc=0x19a578
golang.org/x/image/draw.TestScaleDown(0xc000084680)
	/tmp/workdir-host-linux-mipsle-mengzhuo/gopath/src/golang.org/x/image/draw/scale_test.go:119 +0x90 fp=0xc0000bbf68 sp=0xc0000bbf18 pc=0x19ab18
testing.tRunner(0xc000084680, 0x1f3b60)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/testing/testing.go:1440 +0x13c fp=0xc0000bbfc0 sp=0xc0000bbf68 pc=0xfbed4
testing.(*T).Run.func1()
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/testing/testing.go:1487 +0x68 fp=0xc0000bbfd8 sp=0xc0000bbfc0 pc=0xfd0c8
runtime.goexit()
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/runtime/asm_mips64x.s:617 +0x4 fp=0xc0000bbfd8 sp=0xc0000bbfd8 pc=0x90074
created by testing.(*T).Run
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/testing/testing.go:1487 +0x494

goroutine 1 [chan receive]:
testing.(*T).Run(0xc0000844e0, {0x1e97fe, 0xd}, 0x1f3b60)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/testing/testing.go:1488 +0x4b4
testing.runTests.func1(0xc0000844e0)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/testing/testing.go:1840 +0x9c
testing.tRunner(0xc0000844e0, 0xc000043cf8)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/testing/testing.go:1440 +0x13c
testing.runTests(0xc00000c078, {0x2eb340, 0xe, 0xe}, {0xc071da63e247fe9a, 0x8bb2d7b1d6, 0x2f4380})
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/testing/testing.go:1838 +0x520
testing.(*M).Run(0xc00007a140)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/testing/testing.go:1720 +0x730
main.main()
	_testmain.go:187 +0x24c
FAIL	golang.org/x/image/draw	0.066s

greplogs --dashboard -md -l -e '(?ms)\Alinux-mips64le-mengzhuo.*runtime: out of memory'

2022-01-18T14:56:19-6944b10-5b3ebc8/linux-mips64le-mengzhuo

This failure looks like memory corruption to me, because the failing allocation is for a “28862184423424-byte block” (26.2 TiB). It's not obvious to me whether the corruption is due to a bug in x/image/draw itself, or in the compiler, runtime, and/or platform.

(CC @mengzhuo)
@bcmills bcmills added this to the Backlog milestone Jan 18, 2022
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 18, 2022
@cherrymui
Copy link
Member 

runtime.makeslice(0x1d6600, 0x11800000064, 0x11800000064)
	/tmp/workdir-host-linux-mipsle-mengzhuo/go/src/runtime/slice.go:103 +0x94 fp=0xc0000bb938 sp=0xc0000bb910 pc=0x71014

That argument is enormously large. It seems it should be 0x64. Maybe it is a compiler bug.

@cherrymui
Copy link
Member 

v9 (233) = Arg <int32> {dw} : dw[int32] (dw[int32])
...
v174 (234) = LoadReg <int32> v9 : F2
...
v321 (248) = Copy <int32> v174 : R1
v27 (+248) = MOVWreg <int> v321 : R1 (sources.len[int], sources.cap[int])
...
v234 (+248) = MOVVstore <mem> [16] v2 v27 v233
v180 (+248) = MOVVstore <mem> [24] v2 v27 v234
v30 (248) = CALLstatic <mem> {AuxCall{runtime.makeslice}} [32] v180

It is supposed to call makeslice with len/cap being dw with proper extension. The MOWWreg is supposed to do that. But it has an optimization that if the input is from a LoadReg with proper type, it should be already extended, so it won't extend again. Normally if the LoadReg is loaded into an integer register it is indeed properly extended. But in this case it is loaded into a floating point register (!) (due to another use as floating point) then moved to an integer register, which does not have the proper extension.

@bcmills bcmills changed the title x/image/draw: strange "out of memory" error on linux-mips64le-mengzhuo cmd/compile: MOVWreg missing sign-extension following a Copy from a floating-point LoadReg Jan 18, 2022
@bcmills
Copy link
Contributor Author

bcmills commented Jan 18, 2022

@cherrymui, I made a stab at updating the title based on your diagnosis. Does it look accurate? 😅

@cherrymui
Copy link
Member

Thanks. Yeah, it looks right.

@gopherbot
Copy link

Change https://golang.org/cl/379236 mentions this issue: cmd/compile: don't elide extension for LoadReg to FP register on MIPS64

@bcmills
Copy link
Contributor Author

bcmills commented Jan 19, 2022

@gopherbot, please backport to Go 1.16 and Go 1.17. This is a compiler bug that causes subtle run-time data corruption; it is hard to predict which code might be affected, and the corrupted paths might not be triggered by tests, but it appears to cause a nonzero rate of failures in the Go builders. The fix is small and MIPS-specific, so backporting it seems low-risk.

This was referenced Jan 19, 2022
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #50682 (for 1.16), #50683 (for 1.17).

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

@gopherbot
Copy link

Change https://golang.org/cl/379514 mentions this issue: [release-branch.go1.17] cmd/compile: don't elide extension for LoadReg to FP register on MIPS64

@gopherbot
Copy link

Change https://golang.org/cl/379515 mentions this issue: [release-branch.go1.16] cmd/compile: don't elide extension for LoadReg to FP register on MIPS64

gopherbot pushed a commit that referenced this issue Feb 7, 2022
@cherrymui
[release-branch.go1.16] cmd/compile: don't elide extension for LoadRe…
fcd482a 
…g to FP register on MIPS64

For an extension operation like MOVWreg, if the operand is already
extended, we optimize the second extension out. Usually a LoadReg
of a proper type would come already extended, as a MOVW/MOVWU etc.
instruction does. But for a LoadReg to a floating point register,
the instruction does not do the extension. So we cannot elide the
extension.

Updates #50671.
Fixes #50682.

Change-Id: Id8991df78d5acdecd3fd6138c558428cbd5f6ba3
Reviewed-on: https://go-review.googlesource.com/c/go/+/379236
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit d93ff73)
Reviewed-on: https://go-review.googlesource.com/c/go/+/379515
gopherbot pushed a commit that referenced this issue Feb 7, 2022
@cherrymui
[release-branch.go1.17] cmd/compile: don't elide extension for LoadRe…
1651426 
…g to FP register on MIPS64

For an extension operation like MOVWreg, if the operand is already
extended, we optimize the second extension out. Usually a LoadReg
of a proper type would come already extended, as a MOVW/MOVWU etc.
instruction does. But for a LoadReg to a floating point register,
the instruction does not do the extension. So we cannot elide the
extension.

Updates #50671.
Fixes #50683.

Change-Id: Id8991df78d5acdecd3fd6138c558428cbd5f6ba3
Reviewed-on: https://go-review.googlesource.com/c/go/+/379236
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
(cherry picked from commit d93ff73)
Reviewed-on: https://go-review.googlesource.com/c/go/+/379514
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
@cherrymui @jproberts
cmd/compile: don't elide extension for LoadReg to FP register on MIPS64
d77bd0e 
For an extension operation like MOWWreg, if the operand is already
extended, we optimize the second extension out. Usually a LoadReg
of a proper type would come already extended, as a MOVW/MOVWU etc.
instruction does. But for a LoadReg to a floating point register,
the instruction does not do the extension. So we cannot elide the
extension.

Fixes golang#50671.

Change-Id: Id8991df78d5acdecd3fd6138c558428cbd5f6ba3
Reviewed-on: https://go-review.googlesource.com/c/go/+/379236
Trust: Cherry Mui <cherryyz@google.com>
Run-TryBot: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@golang golang locked and limited conversation to collaborators Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
arch-mips FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@bcmills @gopherbot @cherrymui