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: bad compilation #16016

Closed
dsnet opened this issue Jun 8, 2016 · 7 comments
Closed

cmd/compile: bad compilation #16016

dsnet opened this issue Jun 8, 2016 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.7

Comments

@dsnet
Copy link
Member

dsnet commented Jun 8, 2016 •
edited

Using go1.7beta1

Consider the following program:

package foo

import (
    "testing"
    "time"
)

type T struct{}

func (*T) Foo(vals []interface{}) {
    switch v := vals[0].(type) {
    case string:
        _ = v
    }
}

type R struct{ *T }

type Q interface {
    Foo([]interface{})
}

func TestQueryStreaming(t *testing.T) {
    var q Q = &R{&T{}}
    for i := 0; i < 10000; i++ {
        go func() {
            defer q.Foo([]interface{}{"meow"})
            time.Sleep(100 * time.Millisecond)
        }()
    }
    time.Sleep(1 * time.Second)
}

The method Foo is a read only method that does a type assertion on the input. It should be safe to run that operation concurrently.

Currently this leads to corrupted memory:

$ go test foo_test.go
unexpected fault address 0x1643
unexpected fault address 0x14f5
fatal error: fault
unexpected fault address 0x15e8
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x1 addr=0x1643 pc=0x46d48c]

goroutine 464 [running]:
runtime.throw(0x50a6eb, 0x5)
    /usr/local/go.tip/src/runtime/panic.go:566 +0x95 fp=0xc42022b6b8 sp=0xc42022b698
runtime.sigpanic()
    /usr/local/go.tip/src/runtime/sigpanic_unix.go:27 +0x288 fp=0xc42022b710 sp=0xc42022b6b8
command-line-arguments.(*T).Foo(0x5adfd8, 0xc420234000, 0x1, 0x1)
    /home/joetsai/foo_test.go:11 +0x3c fp=0xc42022b758 sp=0xc42022b710
command-line-arguments.TestQueryStreaming.func1(0x580320, 0xc42003a028)
    /home/joetsai/foo_test.go:29 +0xe0 fp=0xc42022b7b0 sp=0xc42022b758
runtime.goexit()
    /usr/local/go.tip/src/runtime/asm_amd64.s:2060 +0x1 fp=0xc42022b7b8 sp=0xc42022b7b0
created by command-line-arguments.TestQueryStreaming
    /home/joetsai/foo_test.go:29 +0x9c

This passes with SSA or GC off:

$ GOGC=off go test foo_test.go
ok      command-line-arguments  1.030s
$ go test -gcflags=-ssa=0 foo_test.go
ok      command-line-arguments  1.048s

/cc @randall77 @ianlancetaylor
@dsnet dsnet added this to the Go1.7 milestone Jun 8, 2016
@dsnet dsnet added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 8, 2016
@adg
Copy link
Contributor

adg commented Jun 9, 2016

cc @randall77 @ianlancetaylor

@dr2chase
Copy link
Contributor

dr2chase commented Jun 9, 2016

I'm sitting in a meeting, so I'm bisecting.

@dr2chase
Copy link
Contributor

dr2chase commented Jun 9, 2016

commit 3572c64 cmd/compile: keep pointer input arguments live throughout function
Preceding good commit is d520226

@randall77
Copy link
Contributor

Looks like the live variable info at the start of one of the autogenerated functions is not correct.
good CL:
:1: live at entry to (_R).Foo: .this vals
bad CL:
:1: live at entry to (_R).Foo: .this

vals is missing from the live list, so if we trigger a GC right at the start of (*R).Foo, we won't scan its argument (the slice of interface{}).

There's no "use" of the arg variable visible in the assembly. The JMPRET implicitly uses the args, though, as they are read by the tail callee. The fix may be to consider JMPRET as having read all the args.

@randall77 randall77 self-assigned this Jun 9, 2016
@randall77 randall77 mentioned this issue Jun 9, 2016
Closed
@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Sep 22, 2016
@cherrymui
cmd/compile: ensure args are live in tail calls for LR machines
3dfb92f 
On link-register machines we uses RET (sym), instead of JMP (sym),
for tail call (so the assembler knows and may rewrite it to
restore link register if necessary). Add RET to the analysis.

Fixes #17186.
Fixes #16016 on link-register machines.

Change-Id: I8690ac57dd9d49beeea76a5f291988e9a1d3afe5
Reviewed-on: https://go-review.googlesource.com/29570
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Sep 22, 2016
@cherrymui
test: errorcheck auto-generated functions
d586aae 
Add an "errorcheckwithauto" action which performs error check
including lines with auto-generated functions (excluded by
default). Comment "// ERRORAUTO" matches these lines.

Add testcase for CL 29570 (as an example).

Updates #16016, #17186.

Change-Id: Iaba3727336cd602f3dda6b9e5f97dafe0848e632
Reviewed-on: https://go-review.googlesource.com/29652
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@golang golang locked and limited conversation to collaborators Sep 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.7
Development

No branches or pull requests
5 participants
@dr2chase @dsnet @randall77 @adg @gopherbot