cmd/compile, x/sync/errgroup: TestWithContext failing after devirtualization CL #42284

zikaeroh · 2020-10-29T22:40:01Z

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

$ go version
go version devel +fe70a3a0fd Thu Oct 29 21:46:54 2020 +0000 linux/amd64

Does this issue reproduce with the latest release?

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/jake/.cache/go-build"
GOENV="/home/jake/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/jake/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/jake/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/jake/sdk/gotip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/jake/sdk/gotip/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/jake/zikaeroh/sync/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build875916662=/tmp/go-build -gno-record-gcc-switches"
GOROOT/bin/go version: go version devel +fe70a3a0fd Thu Oct 29 21:46:54 2020 +0000 linux/amd64
GOROOT/bin/go tool compile -V: compile version devel +fe70a3a0fd Thu Oct 29 21:46:54 2020 +0000
uname -sr: Linux 5.9.1-zen2-1-zen
/usr/lib/libc.so.6: GNU C Library (GNU libc) release release version 2.32.
gdb --version: GNU gdb (GDB) 9.2

What did you do?

In the golang.org/x/sync/errgroup package, run go test.

What did you expect to see?

Passing tests.

What did you see instead?

--- FAIL: TestWithContext (0.00s)
panic: interface conversion: context.Context is *context.cancelCtx, not *context.emptyCtx [recovered]
	panic: interface conversion: context.Context is *context.cancelCtx, not *context.emptyCtx

goroutine 29 [running]:
testing.tRunner.func1.2(0x5b0e80, 0xc0000a4e70)
	/home/jake/sdk/gotip/src/testing/testing.go:1123 +0x332
testing.tRunner.func1(0xc000083080)
	/home/jake/sdk/gotip/src/testing/testing.go:1126 +0x4b6
panic(0x5b0e80, 0xc0000a4e70)
	/home/jake/sdk/gotip/src/runtime/panic.go:965 +0x1b9
golang.org/x/sync/errgroup_test.TestWithContext(0xc000083080)
	/home/jake/zikaeroh/sync/errgroup/errgroup_test.go:166 +0x652
testing.tRunner(0xc000083080, 0x5f4c88)
	/home/jake/sdk/gotip/src/testing/testing.go:1173 +0xef
created by testing.(*T).Run
	/home/jake/sdk/gotip/src/testing/testing.go:1218 +0x2b3
exit status 2
FAIL	golang.org/x/sync/errgroup	0.005s

https://build.golang.org/log/8759ffb3dd087325958f6d2d67773f3dd2f70cf2
https://build.golang.org/log/32b25547fddc3b46c217c9efffb9aa124df3766e

The dashboard shows this test consistently failing after the devirtualization CL, but this differs from #42279 as it affects runtime behavior.

cc @mdempsky

The text was updated successfully, but these errors were encountered:

dmitshur · 2020-10-30T03:39:20Z

Now that #42279 is resolved, is this still an issue? I'm not sure I understand how this issue is different.

zikaeroh · 2020-10-30T03:40:23Z

See https://build.golang.org/?repo=golang.org%2fx%2fsync; the page is all red on tip.

dmitshur · 2020-10-30T03:44:06Z

Thanks. I can reproduce locally:

$ gotip version
go version devel +f7e26467b Fri Oct 30 01:31:10 2020 +0000 darwin/amd64
$ gotip test golang.org/x/sync/errgroup
--- FAIL: TestWithContext (0.00s)
panic: interface conversion: context.Context is *context.cancelCtx, not *context.emptyCtx [recovered]
	panic: interface conversion: context.Context is *context.cancelCtx, not *context.emptyCtx

goroutine 14 [running]:
testing.tRunner.func1.2(0x11eab00, 0xc00007ae70)
	/Users/dmitshur/sdk/gotip/src/testing/testing.go:1123 +0x332
testing.tRunner.func1(0xc000154000)
	/Users/dmitshur/sdk/gotip/src/testing/testing.go:1126 +0x4b6
panic(0x11eab00, 0xc00007ae70)
	/Users/dmitshur/sdk/gotip/src/runtime/panic.go:965 +0x1b9
golang.org/x/sync/errgroup_test.TestWithContext(0xc000154000)
	/Users/dmitshur/go/src/golang.org/x/sync/errgroup/errgroup_test.go:166 +0x652
testing.tRunner(0xc000154000, 0x1233b40)
	/Users/dmitshur/sdk/gotip/src/testing/testing.go:1173 +0xef
created by testing.(*T).Run
	/Users/dmitshur/sdk/gotip/src/testing/testing.go:1218 +0x2b3
FAIL	golang.org/x/sync/errgroup	0.323s
FAIL

gopherbot · 2020-10-30T09:03:37Z

Change https://golang.org/cl/266518 mentions this issue: cmd/compile: mark inlined functions's results as reassigned

mdempsky · 2020-10-30T16:24:01Z

Sorry, missed this yesterday. Investigating.

mdempsky · 2020-10-30T16:31:57Z

A workaround for the issue would be to to modify errgroup.WithContext like this:

 func WithContext(ctx context.Context) (*Group, context.Context) {
-       ctx, cancel := context.WithCancel(ctx)
-       return &Group{cancel: cancel}, ctx
+       ctx1, cancel := context.WithCancel(ctx)
+       return &Group{cancel: cancel}, ctx1
 }

It's a compiler bug that the current code (i.e., reassigning ctx) is miscompiled, but this change would be desirable anyway because it would play better with the current devirtualization code.

zikaeroh · 2020-10-30T17:30:49Z

It's a compiler bug that the current code (i.e., reassigning ctx) is miscompiled, but this change would be desirable anyway because it would play better with the current devirtualization code.

Reassigning over ctx is probably the most common thing to do with contexts; it'd be a shame if things didn't work properly without making variable name changes. But, maybe in fixing this bug it'll fix that too.

gopherbot · 2020-10-30T17:41:59Z

Change https://golang.org/cl/266618 mentions this issue: cmd/compile: fix reassignVisitor

mdempsky · 2020-10-30T18:04:06Z

Reassigning over ctx is probably the most common thing to do with contexts; it'd be a shame if things didn't work properly without making variable name changes.

Once we move escape analysis to SSA, this should be straightforward to handle. But right now, the Node AST doesn't make it easy to do. We'd have to effectively reimplement SSA renaming in the frontend.

zikaeroh · 2020-10-30T18:14:26Z

Ah, I had no idea this was at the AST level where you couldn't distinguish it. Clearly I'm not paying enough attention during your streams. 🙂

cespare · 2020-10-30T20:36:05Z

@mdempsky I'm really excited that we're getting these devirtualization optimizations and there are several places in my own code where I anticipate them making a big difference. That said, it's a little concerning when an optimization applies only when code is written in a non-standard (and arguably worse) way. Depending on how many release cycles the devirtualization handles the ctx1 case but not the ctx case, it's the kind of thing that can work its way into the performance lore. I really hope in a year or two I don't start seeing high-performance code has been manually SSA-ified to satisfy the devirtualizer :)

mdempsky · 2020-10-30T21:18:35Z

@cespare That's a fair point. There's certainly a trade off though: the more effort put into improving the early devirtualization code is effort taken away from moving escape analysis to SSA. The current early devirtualization pass is basically just a minor extension of the inlining / escape analysis improvements implemented for #41474

If you do see that practice developing, let us know and we can revisit whether there are more cases that we can handle without too much trouble.

kivikakk · 2020-12-20T08:31:36Z

I might be off base here, disregard if so, but this ticket is the one search result for

panic: interface conversion: context.Context is *context.cancelCtx, not *context.emptyCtx

and I'm hitting it, so thought it may be related. It's unfortunately deeply nested in use (websocket library used by a Discord client library), I'm not quite able to extract a simple repro yet, and I'm on ARM64. Not a support request, just dropping this in case it's useful.

Here's the line that it hits on:

		case <-readCtx.Done():

I've tried to extract this into a self-contained reproduction, but it doesn't fail :( Here's what it looks like running:

$ ~/Code/gopath/bin/gotip version
go version devel +55b5801 Sat Dec 19 00:20:38 2020 +0000 darwin/arm64
$ ~/Code/gopath/bin/gotip run main.go 
panic: interface conversion: context.Context is *context.cancelCtx, not *context.emptyCtx

goroutine 66 [running]:
nhooyr.io/websocket.(*Conn).timeoutLoop(0x140000141a0)
        /Users/kameliya/Code/gopath/pkg/mod/nhooyr.io/[email protected]/conn_notjs.go:161 +0x3d0
created by nhooyr.io/websocket.newConn
        /Users/kameliya/Code/gopath/pkg/mod/nhooyr.io/[email protected]/conn_notjs.go:114 +0x37c
exit status 2

mdempsky · 2020-12-20T08:52:46Z

@kivikakk Thanks for the report! I was able to create a reproducer.

kivikakk · 2020-12-20T08:53:19Z

Nice, thank you! I'll watch with interest.

dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 30, 2020

dmitshur added this to the Backlog milestone Oct 30, 2020

dmitshur modified the milestones: Backlog, Go1.16 Oct 30, 2020

dmitshur added the release-blocker label Oct 30, 2020

mdempsky self-assigned this Oct 30, 2020

gopherbot closed this as completed in 7191f11 Oct 30, 2020

mdempsky mentioned this issue Dec 20, 2020

cmd/compile: runtime panic due to bad devirtualization #43292

Closed

golang locked and limited conversation to collaborators Dec 20, 2021

gopherbot added the FrozenDueToAge label Dec 20, 2021

rsc unassigned mdempsky Jun 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile, x/sync/errgroup: TestWithContext failing after devirtualization CL #42284

cmd/compile, x/sync/errgroup: TestWithContext failing after devirtualization CL #42284

zikaeroh commented Oct 29, 2020

dmitshur commented Oct 30, 2020

zikaeroh commented Oct 30, 2020

dmitshur commented Oct 30, 2020 •

edited

Loading

gopherbot commented Oct 30, 2020

mdempsky commented Oct 30, 2020

mdempsky commented Oct 30, 2020

zikaeroh commented Oct 30, 2020

gopherbot commented Oct 30, 2020

mdempsky commented Oct 30, 2020

zikaeroh commented Oct 30, 2020

cespare commented Oct 30, 2020

mdempsky commented Oct 30, 2020

kivikakk commented Dec 20, 2020

mdempsky commented Dec 20, 2020

kivikakk commented Dec 20, 2020

cmd/compile, x/sync/errgroup: TestWithContext failing after devirtualization CL #42284

cmd/compile, x/sync/errgroup: TestWithContext failing after devirtualization CL #42284

Comments

zikaeroh commented Oct 29, 2020

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

dmitshur commented Oct 30, 2020

zikaeroh commented Oct 30, 2020

dmitshur commented Oct 30, 2020 • edited Loading

gopherbot commented Oct 30, 2020

mdempsky commented Oct 30, 2020

mdempsky commented Oct 30, 2020

zikaeroh commented Oct 30, 2020

gopherbot commented Oct 30, 2020

mdempsky commented Oct 30, 2020

zikaeroh commented Oct 30, 2020

cespare commented Oct 30, 2020

mdempsky commented Oct 30, 2020

kivikakk commented Dec 20, 2020

mdempsky commented Dec 20, 2020

kivikakk commented Dec 20, 2020

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

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

dmitshur commented Oct 30, 2020 •

edited

Loading