CL demo of above for easier cherry-picking is https://go-review.googlesource.com/28534 (git fetch https://go.googlesource.com/go refs/changes/34/28534/2 && git cherry-pick FETCH_HEAD)

I tried to distill it to a standalone test but couldn't reproduce in its smaller form, so my guess of the problem is likely incorrect.

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

Go does not guarantee that a finalizer will be run in some bounded amount
of time. In fact Go does not even guarantee that a finalizer will be run
before the program terminates.

This test seems to assume otherwise. If a program want to execute
"close(closed)" in a timely deterministic fashion the application needs to
be structure so that it does not rely on finalizers.

On Mon, Sep 5, 2016 at 5:19 PM, Brad Fitzpatrick notifications@github.com
wrote:

When writing a test for #16983 #16983,
I encountered what looks like perhaps a liveness bug with variadic calls.

If I set "const useVariadic" to true below, the test fails (buf1 is never
collected), but if I set it to false, buf1 is collected.

func twoReader(b1, b2 _bytes.Reader) Reader {
return MultiReader(b1, b2)
}
func TestMultiReaderFreesExhaustedReaders(t *testing.T) {
var mr Reader
closed := make(chan struct{})
{
buf1 := bytes.NewReader([]byte("foo"))
buf2 := bytes.NewReader([]byte("bar"))
const useVariadic = true // set to true to see the bug; false to see the workaround
if useVariadic {
mr = MultiReader(buf1, buf2)
} else {
mr = twoReader(buf1, buf2)
}
runtime.SetFinalizer(buf1, func(_bytes.Reader) {
close(closed)
})
}

    buf := make([]byte, 4)
    if n, err := ReadFull(mr, buf); err != nil || string(buf) != "foob" {
            t.Fatalf(`ReadFull = %d (%q), %v; want 3, "foo", nil`, n, buf[:n], err)
    }

    println("runtime.GC...")
    runtime.GC()
    println("waiting...")
    select {
    case <-closed:
    case <-time.After(5 * time.Second):
            t.Fatal("timeout waiting for collection of buf1")
    }

    if n, err := ReadFull(mr, buf[:2]); err != nil || string(buf[:2]) != "ar" {
            t.Fatalf(`ReadFull = %d (%q), %v; want 2, "ar", nil`, n, buf[:n], err)
    }

}

/cc @randall77 https://github.com/randall77 @josharian
https://github.com/josharian @minux https://github.com/minux
@cherrymui https://github.com/cherrymui

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#16996, or mute the thread
https://github.com/notifications/unsubscribe-auth/AA7WnylwzhlnxU5EMWVoJM_iRnk5CX7Oks5qnId9gaJpZM4J1UkH
.

@RLH, I know. This isn't a bug report about finalizers. I'm using finalizers as a means to demonstrate what I think is a compiler bug.

From a quick look, here's what's going on.

Note that I changed buf1 := bytes.NewReader([]byte("foo")) into buf1 := Reader(bytes.NewReader([]byte("foo"))) and similarly so for buf2, and fixed up the other types. That moves the interface wrapping goo out of the way so we can see more clearly what is happening.

Ok. mr = MultiReader(buf1, buf2) gets rewritten by the compiler to tmp := [2]Reader{buf1, buf2}; mr = MultiReader(tmp[:]). Assembly:

    0x01b1 00433 (multi_test.go:254)    MOVQ    $0, "".autotmp_238+336(SP)
    0x01bd 00445 (multi_test.go:254)    MOVQ    $0, "".autotmp_238+344(SP)
    0x01c9 00457 (multi_test.go:254)    MOVQ    $0, "".autotmp_238+352(SP)
    0x01d5 00469 (multi_test.go:254)    MOVQ    $0, "".autotmp_238+360(SP)
    0x01e1 00481 (multi_test.go:247)    LEAQ    go.itab.*bytes.Reader,io.Reader(SB), CX
    0x01e8 00488 (multi_test.go:254)    MOVQ    CX, "".autotmp_238+336(SP)
    0x01f0 00496 (multi_test.go:254)    MOVQ    "".autotmp_262+248(SP), DX
    0x01f8 00504 (multi_test.go:254)    MOVQ    DX, "".autotmp_238+344(SP)
    0x0200 00512 (multi_test.go:254)    MOVQ    CX, "".autotmp_238+352(SP)
    0x0208 00520 (multi_test.go:254)    MOVQ    AX, "".autotmp_238+360(SP)
    0x0210 00528 (multi_test.go:254)    LEAQ    "".autotmp_238+336(SP), AX
    0x0218 00536 (multi_test.go:254)    MOVQ    AX, (SP)
    0x021c 00540 (multi_test.go:254)    MOVQ    $2, 8(SP)
    0x0225 00549 (multi_test.go:254)    MOVQ    $2, 16(SP)
    0x022e 00558 (multi_test.go:254)    PCDATA  $0, $7
    0x022e 00558 (multi_test.go:254)    CALL    io.MultiReader(SB)

autotmp_238 here is the [2]Reader array. It gets zeroed (unnecessarily?) from 0x01b1-0x01d5 and filled with the proper interface values (itab, data pairs for buf1 and buf2) from 0x01e1-0x0208. Then a slice is constructed on the stack with len and cap 2 from 0x0210-0x0225.

After the call to io.MultiReader, autotmp_238 is never touched again. So the references to buf1 and buf2 remain on the stack until the function exits.

One fix would be to teach the compiler to explicitly zero the temporary that it creates (in mkdotargslice, in walk.go) after the call. This would add a bit of overhead to variadic calls, but allow faster reclamation.

I don't currently plan to work on this further.

We take the address of the [2]io.Reader which prevents the liveness analysis from ever knowing that the variable dies.
We mark it as addrtaken so it will be live for the duration of the vararg call. That's required for correctness. We want "live across the next call but not after". Maybe a judicious VarKill would fix it.

Josh's solution (zero it after the call) would also work.

Simple repro:

package c

import "runtime"

func f(x, y *int) {
    A(x, y)
    runtime.GC()
}
func A(args ...*int) {
    sink = args[0]
}

var sink *int

$go build -gcflags="-live -m"
# issue16996/c
./c.go:9: leaking param content: args
./c.go:5: leaking param: x
./c.go:5: leaking param: y
./c.go:6: f ... argument does not escape      <- this means we can allocate the ... array on the stack
./c.go:5: live at entry to f: x y
./c.go:6: live at call to A: autotmp_3
./c.go:7: live at call to GC: autotmp_3        <- the ... array is still live after the call for which it was allocated
./c.go:9: live at entry to A: args

It is indeed a missing VarKill. There seems to be some confusion around levels of escapedness. If the ... arg escapes, or does not escape, then we generate correct code. This example seems to be in a gray area - the arg of A is marked as "param content escapes". The label is enough to get the ... array stack allocated, but not enough to get a VarKill added after the call. Those two decisions should ideally match.
@dr2chase

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

CL above reverted. Reopening.

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

Reverted the revert at https://go-review.googlesource.com/c/28582/