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: should not spill Duff-adjusted address #16515

Closed
cherrymui opened this issue Jul 27, 2016 · 6 comments
Closed

cmd/compile: should not spill Duff-adjusted address #16515

cherrymui opened this issue Jul 27, 2016 · 6 comments
Labels
FrozenDueToAge
Milestone
Go1.7

Comments

@cherrymui
Copy link
Member

What version of Go are you using (go version)?
go version devel +c80e0d3 Wed Jul 27 05:43:36 2016 +0000 darwin/amd64

In SSA, Duff-adjusted address may be spilled and reused if there are more than one Duff calls within the same function. On AMD64, this address is currently spilled as a scalar, which means if the stack is moved, the spilled value is no longer useful. This is demonstrated with the first call of f in the following code:

package main

import "runtime"

type T [62]int

var sink interface{}

//go:noinline
func f(x *T) {
    // Two DUFFZEROs on the same address with a function call in between.
    // Duff-adjusted address will be spilled and loaded

    *x = T{} // DUFFZERO
    runtime.GC()
    (*x)[0] = 1
    g()      // call a function with large frame, trigger a stack move
    *x = T{} // DUFFZERO again
}

//go:noinline
func g() {
    var x [1000]int
    _ = x
}

//go:noinline
func h() {
    var d1 [1152]byte
    sink = &d1 // force heap allocation
    var d2 [1152]byte
    sink = &d2 // force heap allocation
    var d3 [1152]byte
    sink = &d3 // force heap allocation
    var d4 [1152]byte
    sink = &d4 // force heap allocation
    var d5 [1152]byte
    sink = &d5 // force heap allocation
    var d6 [1152]byte
    sink = &d6 // force heap allocation
    var d7 [1152]byte
    sink = &d7 // force heap allocation
    var d8 [1152]byte
    sink = &d8 // force heap allocation
}

func main() {
    var a T // on stack
    a[0] = 2
    f(&a)
    if a[0] != 0 {
        println("a[0] = ", a[0])
        panic("zeroing failed")
    }

    h() // allocate many objects to clear spans

    var s struct { a T; b [1986]int } // allocate 16K, hopefully it's in a new span and a few bytes before it is garbage
    sink = &s // force heap allocation
    s.a[0] = 2
    f(&s.a)
    if s.a[0] != 0 {
        println("s.a[0] =", s.a[0])
        panic("zeroing failed")
    }

}

where f is compiled to

    00000 (/tmp/duff.go:10) TEXT    "".f(SB), $0
    00001 (/tmp/duff.go:10) FUNCDATA    $0, "".gcargs·0(SB)
    00002 (/tmp/duff.go:10) FUNCDATA    $1, "".gclocals·1(SB)
    00003 (/tmp/duff.go:10) TYPE    "".x(FP)type.*"".T, $8
    00004 (/tmp/duff.go:10) TYPE    "".autotmp_9(SP)type.uint64, $8
v21 00005 (/tmp/duff.go:14) MOVQ    "".x(FP), AX
v6  00006 (/tmp/duff.go:14) TESTB   AX, (AX)
v24 00007 (/tmp/duff.go:14) LEAQ    -16(AX), DI
v22 00008 (/tmp/duff.go:14) MOVQ    DI, "".autotmp_9(SP)
v13 00009 (/tmp/duff.go:14) XORPS   X0, X0
v8  00010 (/tmp/duff.go:14) DUFFZERO    $155
v9  00011 (/tmp/duff.go:15) CALL    runtime.GC(SB)
v12 00012 (/tmp/duff.go:15) VARLIVE "".x(FP)
v17 00013 (/tmp/duff.go:16) MOVQ    "".x(FP), AX
v19 00014 (/tmp/duff.go:16) MOVQ    $1, (AX)
v20 00015 (/tmp/duff.go:17) CALL    "".g(SB)
v23 00016 (/tmp/duff.go:17) VARLIVE "".x(FP)
v25 00017 (/tmp/duff.go:18) MOVQ    "".autotmp_9(SP), DI
v27 00018 (/tmp/duff.go:14) XORPS   X0, X0
v26 00019 (/tmp/duff.go:18) DUFFZERO    $155
v28 00020 (/tmp/duff.go:19) VARLIVE "".x(FP)
b6  00021 (/tmp/duff.go:19) RET

Run with SSA on:

a[0] =  1
panic: zeroing failed

goroutine 1 [running]:
panic(0x568e0, 0xc42000a070)
    /Users/cherryyz/src/go-tip/src/runtime/panic.go:500 +0x1a1
main.main()
    /tmp/duff.go:53 +0x25b
exit status 2

(ok running with SSA off)

Note that it does not work if we change the type of the spill to pointer, since Duff-adjusted address (in this case x-16) may not point to valid object, causing stack scanning code to panic (may not always happen):

runtime: pointer 0xc420073ff0 to unused region of spanidx=0x39 span.base()=0xc420072000 span.limit=0xc420073f80 span.state=0
runtime: found in object at *(0xc420033cd8+0x0)
object=0xc420033cd8 k=0x6210019 s.base()=0xc420032000 s.limit=0x0 s.sizeclass=0 s.elemsize=0
fatal error: found bad pointer in Go heap (incorrect use of unsafe or cgo?)

One solution is to do Duff-adjustment in genValue, so reg allocator cannot see the intermediate value.

Found when doing ARM64 port.
/cc @aclements
@cherrymui
Copy link
Member Author

I can go on fix it if we agree to do Duff-adjustment in genValue.

@cherrymui
Copy link
Member Author

(note for me) ARM SSA may also has this problem, not for DUFFZERO, but for LoweredZero/LoweredMove for very large objects.

@aclements
Copy link
Member

/cc @RLH @randall77

The AMD64 aspect of this issue is a regression, so we should try to fix it for 1.7.

@aclements aclements added this to the Go1.7 milestone Jul 27, 2016
@cherrymui
Copy link
Member Author

Confirmed for ARM(32) with type T [16384]int and var s struct { a T }. Problem is the op for large zeroing may cause spill of the address of the end, which may be invalid. Changed to use the address of the last element. CL https://golang.org/cl/25300 sent.

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Jul 27, 2016
@cherrymui
[dev.ssa] cmd/compile: fix possible invalid pointer spill in large Ze…
114c059 
…ro/Move on ARM

Instead of comparing the address of the end of the memory to zero/copy,
comparing the address of the last element, which is a valid pointer.
Also unify large and unaligned Zero/Move, by passing alignment as AuxInt.

Fixes #16515 for ARM.

Change-Id: I19a62b31c5acf5c55c16a89bea1039c926dc91e5
Reviewed-on: https://go-review.googlesource.com/25300
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Jul 29, 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.7
Development

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