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

cmd/compile: enable typechecking tracing causes side effects #33658

Open
cuonglm opened this issue Aug 15, 2019 · 14 comments

Comments

@cuonglm
Copy link
Contributor

commented Aug 15, 2019

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

$ go version
go version devel +61bb56ad63 Mon Aug 12 23:12:29 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cuonglm/.cache/go-build"
GOENV="/home/cuonglm/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cuonglm/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/cuonglm/sources/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/cuonglm/sources/go/pkg/tool/linux_amd64"
GCCGO="/usr/local/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
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-build490999314=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Build go from source with compiler typechecking tracing enable (Debugging #31872 (comment)).

What did you expect to see?

Same compiler crashes, the same as when no typechecking tracing.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x70 pc=0xbdf2a9]

goroutine 1 [running]:
cmd/compile/internal/gc.dowidth(0xc00034d7a0)
	/home/cuonglm/sources/go/src/cmd/compile/internal/gc/align.go:175 +0xa9
cmd/compile/internal/types.(*Type).Fields(0xc00034d7a0, 0x200)
	/home/cuonglm/sources/go/src/cmd/compile/internal/types/type.go:863 +0x6c
cmd/compile/internal/gc.expandiface(0xc00034d6e0)
	/home/cuonglm/sources/go/src/cmd/compile/internal/gc/align.go:53 +0x1a4
cmd/compile/internal/gc.dowidth(0xc00034d6e0)
	/home/cuonglm/sources/go/src/cmd/compile/internal/gc/align.go:263 +0xa7b
cmd/compile/internal/types.(*Type).Fields(0xc00034d6e0, 0x40bc56)
	/home/cuonglm/sources/go/src/cmd/compile/internal/types/type.go:863 +0x6c
cmd/compile/internal/gc.expandiface(0xc00034d7a0)
	/home/cuonglm/sources/go/src/cmd/compile/internal/gc/align.go:53 +0x1a4
cmd/compile/internal/gc.dowidth(0xc00034d7a0)
	/home/cuonglm/sources/go/src/cmd/compile/internal/gc/align.go:263 +0xa7b
cmd/compile/internal/gc.resumecheckwidth()
	/home/cuonglm/sources/go/src/cmd/compile/internal/gc/align.go:450 +0x51
cmd/compile/internal/gc.Main(0xe5e098)
	/home/cuonglm/sources/go/src/cmd/compile/internal/gc/main.go:553 +0x298f
main.main()
	/home/cuonglm/sources/go/src/cmd/compile/main.go:51 +0xac
FAIL

What did you see instead?

Compiler doesn't crash, produce different error.

p.go:7:6: typecheck 0xc000336600 DCLTYPE <node DCLTYPE> tc=0
p.go:7:6: . typecheck1 0xc000336600 DCLTYPE <node DCLTYPE> tc=2
p.go:7:6: . . typecheck 0xc0003523c0 TYPE I2 tc=0
p.go:7:6: . . . typecheck1 0xc0003523c0 TYPE I2 tc=2
p.go:7:6: . . . . typecheckdef 0xc0003523c0 TYPE I2 tc=2
p.go:7:6: . . . . . typecheckdeftype 0xc0003523c0 TYPE I2 tc=2
p.go:7:9: . . . . . . typecheck 0xc000336580 TINTER <inter> tc=0
p.go:7:9: . . . . . . . typecheck1 0xc000336580 TINTER <inter> tc=2
p.go:3:6: . . . . . . . . typecheck 0xc0003522d0 TYPE I tc=0
p.go:3:6: . . . . . . . . . typecheck1 0xc0003522d0 TYPE I tc=2
p.go:3:6: . . . . . . . . . . typecheckdef 0xc0003522d0 TYPE I tc=2
p.go:3:10: . . . . . . . . . . . typecheck 0xc000336400 TINTER <inter> tc=0
p.go:3:10: . . . . . . . . . . . . typecheck1 0xc000336400 TINTER <inter> tc=2
p.go:4:2: . . . . . . . . . . . . . typecheck 0xc000336300 NONAME I2 tc=0
p.go:4:2: . . . . . . . . . . . . . . resolve 0xc000336300 NONAME I2 tc=0
p.go:7:6: . . . . . . . . . . . . . . => 0xc0003523c0 TYPE I2 tc=1 type=undefined I2
p.go:7:6: . . . . . . . . . . . . . . typecheck1 0xc0003523c0 TYPE I2 tc=2
p.go:7:6: . . . . . . . . . . . . . . . typecheckdef 0xc0003523c0 TYPE I2 tc=2
p.go:7:6: . . . . . . . . . . . . . . . => 0xc0003523c0 TYPE I2 tc=2 type=undefined I2
p.go:7:6: . . . . . . . . . . . . . . => 0xc0003523c0 TYPE I2 tc=2 type=undefined I2
p.go:7:6: . . . . . . . . . . . . . => 0xc0003523c0 TYPE I2 tc=1 type=undefined I2
p.go:3:10: . . . . . . . . . . . . => 0xc000336400 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:10: . . . . . . . . . . . => 0xc000336400 TYPE interface { I2 } tc=1 type=interface { I2 }
p.go:3:6: . . . . . . . . . . => 0xc0003522d0 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:6: . . . . . . . . . => 0xc0003522d0 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:6: . . . . . . . . => 0xc0003522d0 TYPE interface { I2 } tc=1 type=interface { I2 }
p.go:7:9: . . . . . . . => 0xc000336580 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:7:9: . . . . . . => 0xc000336580 TYPE interface { I2 } tc=1 type=interface { I2 }
p.go:7:6: . . . . . => 0xc0003523c0 TYPE I2 tc=1 type=interface { I2 }
p.go:7:6: . . . . => 0xc0003523c0 TYPE I2 tc=1 type=interface { I2 }
p.go:7:6: . . . => 0xc0003523c0 TYPE I2 tc=1 type=interface { I2 }
p.go:7:6: . . => 0xc0003523c0 TYPE I2 tc=1 type=interface { I2 }
p.go:7:6: . => 0xc000336600 DCLTYPE <node DCLTYPE> tc=2 type=<T>
p.go:7:6: => 0xc000336600 DCLTYPE <node DCLTYPE> tc=1 type=<T>
p.go:3:6: typecheck 0xc000336480 DCLTYPE <node DCLTYPE> tc=0
p.go:3:6: . typecheck1 0xc000336480 DCLTYPE <node DCLTYPE> tc=2
p.go:3:6: . . typecheck 0xc0003522d0 TYPE interface { I2 } tc=1
p.go:3:6: . . . typecheck1 0xc0003522d0 TYPE interface { I2 } tc=2
p.go:3:6: . . . . typecheckdef 0xc0003522d0 TYPE interface { I2 } tc=2
p.go:3:6: . . . . => 0xc0003522d0 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:6: . . . => 0xc0003522d0 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:6: . . => 0xc0003522d0 TYPE interface { I2 } tc=1 type=interface { I2 }
p.go:3:6: . => 0xc000336480 DCLTYPE <node DCLTYPE> tc=2 type=<T>
p.go:3:6: => 0xc000336480 DCLTYPE <node DCLTYPE> tc=1 type=<T>
p.go:4:2: interface contains embedded non-interface I2
FAI
@andybons

This comment has been minimized.

Copy link
Member

commented Aug 15, 2019

@andybons andybons added this to the Unplanned milestone Aug 15, 2019

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Sounds like fmt.Printf format causes this, it transforms embedded interface to full definition.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 16, 2019

Change https://golang.org/cl/190537 mentions this issue: cmd/compile: fix typecheck tracing break Node

@mdempsky

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

To recap some of the discussion on CL 190537 so far, the root cause issue here is that tracing calls fmt.Printf on types; and when printing interface types, we call Type.Fields(), which calls Dowidth, which tries to flatten an interface, if it hasn't already.

In the context of tracing the typechecking of

type I = interface { I2 }
type I2 interface { I }

this causes a problem because:

  1. We start at typechecking I2 (because main.go skips top-level aliases until after top-level declared types).
  2. To typecheck I2, we set I2's Type to a TFORW type and defer width checking, and then recursively typecheck I.
  3. To typecheck I, we recursively typecheck I2, which immediately succeeds.
  4. When typechecking I returns, we try to trace the progress so far, and fmt.Printf interface { I2 }.
  5. The fmt.Printf fails because at this point I2 is still a TFORW type, not yet its final TINTER type.

I suggested @cuonglm make a change that would avoid expanding the iface in fmt.go if it's not already expanded, but this led to problems with swt.go and typehash.

To fix this, I think we need better discipline about making sure Dowidth is only called whenever it's actually safe to do so.

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

To fix this, I think we need better discipline about making sure Dowidth is only called whenever it's actually safe to do so.

Dowidth is called in both gc and types, so adding a discipline may complicated. Also do we already know when is safe to call Dowidth?

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@mdempsky Ah, never mind, it's not complicated as I thought, updated the CL.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@cuonglm By better discipline, I mean structuring the code to avoid calling functions when it's unsafe (or potentially unsafe) to do so. Generally that means reducing the number of flags and special cases.

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2019

@mdempsky My latest patch in the CL actually add another flag to Type 😄 dowidth is used in many places, at least in align.go, both checkwidth and expandiface will call dowidth, Also, dowidth is called inside package types, IIUC, we can't know the current context of typecheck when we are here.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

dowidth is used in many places, at least in align.go, both checkwidth and expandiface will call dowidth, Also, dowidth is called inside package types, IIUC, we can't know the current context of typecheck when we are here.

Right. That's why I said on the CL that I think fixing this is going to be more involved than I expected.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 30, 2019

Change https://golang.org/cl/192721 mentions this issue: cmd/compile: simplify {defer,resume}checkwidth logic

@gopherbot

This comment has been minimized.

Copy link

commented Aug 30, 2019

Change https://golang.org/cl/192724 mentions this issue: cmd/compile: replace copytype to setUnderlying

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@mdempsky just read CL 192721, if it went in, we can now derfercheckwidth and resumecheckwidth inside traceprint right?

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@mdempsky just read CL 192721, if it went in, we can now derfercheckwidth and resumecheckwidth inside traceprint right?

I tried and it doesn't work. The thing is that calling types.Fields will always trigger Dowidth, then for TINTER, Dowidth always call expandiface.

So defercheckwidth won't help in this case.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

@cuonglm Thanks for the test report. I'm not surprised it still doesn't work. The couple of CLs I've mailed so far are just some low hanging fruit while I think about what needs to be done.

gopherbot pushed a commit that referenced this issue Sep 3, 2019
cmd/compile: simplify {defer,resume}checkwidth logic
This CL extends {defer,resume}checkwidth to support nesting, which
simplifies usage.

Updates #33658.

Change-Id: Ib3ffb8a7cabfae2cbeba74e21748c228436f4726
Reviewed-on: https://go-review.googlesource.com/c/go/+/192721
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit that referenced this issue Sep 3, 2019
cmd/compile: replace copytype to setUnderlying
While here, change the params to be easier to understand: "t" is now
always the type being updated, and "underlying" is now used to
represent the underlying type.

Updates #33658.

Change-Id: Iabb64414ca3abaa8c780e4c9093e0c77b76fabf9
Reviewed-on: https://go-review.googlesource.com/c/go/+/192724
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
cmd/compile: simplify {defer,resume}checkwidth logic
This CL extends {defer,resume}checkwidth to support nesting, which
simplifies usage.

Updates golang#33658.

Change-Id: Ib3ffb8a7cabfae2cbeba74e21748c228436f4726
Reviewed-on: https://go-review.googlesource.com/c/go/+/192721
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
cmd/compile: replace copytype to setUnderlying
While here, change the params to be easier to understand: "t" is now
always the type being updated, and "underlying" is now used to
represent the underlying type.

Updates golang#33658.

Change-Id: Iabb64414ca3abaa8c780e4c9093e0c77b76fabf9
Reviewed-on: https://go-review.googlesource.com/c/go/+/192724
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.