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

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

cmd/compile: determine root cause for compiler crash in #23823 #31872

griesemer opened this issue May 6, 2019 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.14

Comments

@griesemer
Copy link
Contributor

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).
@griesemer griesemer added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 6, 2019
@griesemer griesemer added this to the Go1.14 milestone May 6, 2019
@griesemer griesemer self-assigned this May 6, 2019
@griesemer griesemer mentioned this issue May 6, 2019
Closed
@cuonglm
Copy link
Member

cuonglm commented Aug 12, 2019 •
edited

@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

go/test/fixedbugs/issue23823.go

Line 13 in 8b058cf

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

@griesemer
Copy link
Contributor Author

@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
Copy link
Member

cuonglm commented Aug 12, 2019 •
edited

@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
Copy link
Member

cuonglm commented Aug 12, 2019 •
edited

@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

go/src/cmd/compile/internal/gc/typecheck.go

Line 519 in 8b058cf

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
Copy link
Contributor Author

@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
Copy link
Member

cuonglm 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
Copy link
Contributor Author

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

@cuonglm cuonglm mentioned this issue Aug 15, 2019
Closed
@gopherbot
Copy link

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

tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
@cuonglm @tomocy
cmd/compile: make typecheck set n.Type.Nod when returning OTYPE
9e3a158 
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 pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@cuonglm @t4n6a1ka
cmd/compile: make typecheck set n.Type.Nod when returning OTYPE
2bba36c 
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>
@golang golang locked and limited conversation to collaborators Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
3 participants
@cuonglm @griesemer @gopherbot