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: determine root cause for compiler crash in #23823 #31872

Closed
griesemer opened this issue May 6, 2019 · 8 comments

Comments

@griesemer
Copy link
Contributor

commented May 6, 2019

Reminder issue: #23823 documents a compiler crash for which we introduced a temp. fix. Fix root cause for the crash, and remove temp. work-around code (see align.go, dowith function).

@cuonglm

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

@griesemer With:

package p

type I = interface { 
	I2
}

type I2 interface {
	I
}

Should the error message point to line 3 instead of line 7?

Current workaround point the error to line 7

type I2 interface { // ERROR "invalid recursive type"

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@cuonglm It would be better for the error to be on line 3, but I'm happy we're getting a reasonable error in the first place. Cyclic interfaces using alias types are likely not working correct in all cases; we need to fix the root problems first (non-trivial) before dealing with positioning of error messages.

@cuonglm

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

@griesemer Yay, I have a patch in my local, which does report the error at line 3, that's why I asked question above. I will send the CL when I found a way to explain it easily.

@cuonglm

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

@griesemer Hmm, I cleanup my patch, and the only change needed to fix this issue is set n.Type.Nod in typecheck, OTINTER case, after this line

n.Type = tointerface(n.List.Slice())

The change + workaround removed does pass all the tests + change in issue23823.go to point error at line 3.

Since you said this should be a non-trivial change, I'm afraid I'm missing something.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@cuonglm Sorry for being unclear: What I meant is that fixing correct handling of cycles with alias types is non-trivial in general. It may well be that this specific line number fix is trivial.

@cuonglm

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

@griesemer when enable typecheck tracing, I got completely different error, is it intended behavior?

$ go tool compile -t p.go 
p.go:7:6: typecheck 0xc000354600 DCLTYPE <node DCLTYPE> tc=0
p.go:7:6: . typecheck1 0xc000354600 DCLTYPE <node DCLTYPE> tc=2
p.go:7:6: . . typecheck 0xc0003723c0 TYPE I2 tc=0
p.go:7:6: . . . typecheck1 0xc0003723c0 TYPE I2 tc=2
p.go:7:6: . . . . typecheckdef 0xc0003723c0 TYPE I2 tc=2
p.go:7:6: . . . . . typecheckdeftype 0xc0003723c0 TYPE I2 tc=2
p.go:7:9: . . . . . . typecheck 0xc000354580 TINTER <inter> tc=0
p.go:7:9: . . . . . . . typecheck1 0xc000354580 TINTER <inter> tc=2
p.go:3:6: . . . . . . . . typecheck 0xc0003722d0 TYPE I tc=0
p.go:3:6: . . . . . . . . . typecheck1 0xc0003722d0 TYPE I tc=2
p.go:3:6: . . . . . . . . . . typecheckdef 0xc0003722d0 TYPE I tc=2
p.go:3:10: . . . . . . . . . . . typecheck 0xc000354400 TINTER <inter> tc=0
p.go:3:10: . . . . . . . . . . . . typecheck1 0xc000354400 TINTER <inter> tc=2
p.go:4:2: . . . . . . . . . . . . . typecheck 0xc000354300 NONAME I2 tc=0
p.go:4:2: . . . . . . . . . . . . . . resolve 0xc000354300 NONAME I2 tc=0
p.go:7:6: . . . . . . . . . . . . . . => 0xc0003723c0 TYPE I2 tc=1 type=undefined I2
p.go:7:6: . . . . . . . . . . . . . . typecheck1 0xc0003723c0 TYPE I2 tc=2
p.go:7:6: . . . . . . . . . . . . . . . typecheckdef 0xc0003723c0 TYPE I2 tc=2
p.go:7:6: . . . . . . . . . . . . . . . => 0xc0003723c0 TYPE I2 tc=2 type=undefined I2
p.go:7:6: . . . . . . . . . . . . . . => 0xc0003723c0 TYPE I2 tc=2 type=undefined I2
p.go:7:6: . . . . . . . . . . . . . => 0xc0003723c0 TYPE I2 tc=1 type=undefined I2
p.go:3:10: . . . . . . . . . . . . => 0xc000354400 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:10: . . . . . . . . . . . => 0xc000354400 TYPE interface { I2 } tc=1 type=interface { I2 }
p.go:3:6: . . . . . . . . . . => 0xc0003722d0 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:6: . . . . . . . . . => 0xc0003722d0 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:6: . . . . . . . . => 0xc0003722d0 TYPE interface { I2 } tc=1 type=interface { I2 }
p.go:7:9: . . . . . . . => 0xc000354580 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:7:9: . . . . . . => 0xc000354580 TYPE interface { I2 } tc=1 type=interface { I2 }
p.go:7:6: . . . . . => 0xc0003723c0 TYPE I2 tc=1 type=interface { I2 }
p.go:7:6: . . . . => 0xc0003723c0 TYPE I2 tc=1 type=interface { I2 }
p.go:7:6: . . . => 0xc0003723c0 TYPE I2 tc=1 type=interface { I2 }
p.go:7:6: . . => 0xc0003723c0 TYPE I2 tc=1 type=interface { I2 }
p.go:7:6: . => 0xc000354600 DCLTYPE <node DCLTYPE> tc=2 type=<T>
p.go:7:6: => 0xc000354600 DCLTYPE <node DCLTYPE> tc=1 type=<T>
p.go:3:6: typecheck 0xc000354480 DCLTYPE <node DCLTYPE> tc=0
p.go:3:6: . typecheck1 0xc000354480 DCLTYPE <node DCLTYPE> tc=2
p.go:3:6: . . typecheck 0xc0003722d0 TYPE interface { I2 } tc=1
p.go:3:6: . . . typecheck1 0xc0003722d0 TYPE interface { I2 } tc=2
p.go:3:6: . . . . typecheckdef 0xc0003722d0 TYPE interface { I2 } tc=2
p.go:3:6: . . . . => 0xc0003722d0 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:6: . . . => 0xc0003722d0 TYPE interface { I2 } tc=2 type=interface { I2 }
p.go:3:6: . . => 0xc0003722d0 TYPE interface { I2 } tc=1 type=interface { I2 }
p.go:3:6: . => 0xc000354480 DCLTYPE <node DCLTYPE> tc=2 type=<T>
p.go:3:6: => 0xc000354480 DCLTYPE <node DCLTYPE> tc=1 type=<T>
p.go:4:2: interface contains embedded non-interface I2
@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@cuonglm No. Looks like tracing changes things, which it shouldn't.

@gopherbot

This comment has been minimized.

Copy link

commented Aug 25, 2019

Change https://golang.org/cl/191540 mentions this issue: cmd/compile: fix embedding interface cycles with type alias

@gopherbot gopherbot closed this in e87fe0f Aug 29, 2019

tomocy added a commit to tomocy/go that referenced this issue Sep 1, 2019
cmd/compile: make typecheck set n.Type.Nod when returning OTYPE
typecheck only set n.Type.Nod for declared type, and leave it nil for
anonymous types, type alias. It leads to compiler crashes, because
n.Type.Nod is nil at the time dowidth was called.

Fixing it by set n.Type.Nod right after n.Type initialization if n.Op is
OTYPE.

When embedding interface cycles involve in type alias, it also helps
pointing the error message to the position of the type alias
declaration, instead of position of embedding interface.

Fixes golang#31872

Change-Id: Ia18391e987036a91f42ba0c08b5506f52d07f683
Reviewed-on: https://go-review.googlesource.com/c/go/+/191540
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
cmd/compile: make typecheck set n.Type.Nod when returning OTYPE
typecheck only set n.Type.Nod for declared type, and leave it nil for
anonymous types, type alias. It leads to compiler crashes, because
n.Type.Nod is nil at the time dowidth was called.

Fixing it by set n.Type.Nod right after n.Type initialization if n.Op is
OTYPE.

When embedding interface cycles involve in type alias, it also helps
pointing the error message to the position of the type alias
declaration, instead of position of embedding interface.

Fixes golang#31872

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