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: multiple compiler crashes and failures with -G=3 #46704

Open
mdempsky opened this issue Jun 11, 2021 · 12 comments
Open

cmd/compile: multiple compiler crashes and failures with -G=3 #46704

mdempsky opened this issue Jun 11, 2021 · 12 comments
Assignees
Labels
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 11, 2021

Background

My main motivation for unified IR is to reduce complexity and duplicated code, under the expectation that this will reduce the frequency of compiler issues and ultimately reduce the engineering cost of having a production-quality generics implementation ready to ship for Go 1.18.

After getting unified IR to its first milestone of feature completion and review readiness, I decided to do a first round of aggressive testing to help empirically test my hypothesis. Up to this point, I'd mostly just relied on existing tests and toolstash -cmp. The only additional test case I'd written specifically for unified IR was nested.go, to test type shadowing and generic types declared within generic functions (as those are intentionally not supported by -G=3 mode).

Methodology

I set a goal of finding 10 issues. I wrote test cases manually and avoided local type definitions (again, because -G=3 intentionally doesn't support them). I skimmed through the -G=3 code for inspiration (e.g., that's how I found the comparable failure in case 8), but largely focused on variations of cases that I know from experience have been a sore point for the Go compiler.

I did not specifically set out looking for compiler crashes. I was focused on broad testing of functionality, not deep testing, so whenever I found an issue I just minimized it, and then moved on to trying other ideas. I did not explore if any of the compiler crashes could be turned into miscompilation errors.

Results

I found 10 issues within 3 hours. They're available at https://go-review.googlesource.com/c/go/+/326169 as test/run.go compatible tests. @cuonglm also reported an 11th in the comments.

All 11 issues affect -G=3 mode. Only 1 of them affects unified IR (see below for details). I think these findings support my hypothesis that unified IR's design reduces the frequency of compiler issues.

9 of the issues are the compiler crashing on valid code. 1 issue is the compiler accepting invalid code. 1 issue is the compiler crashing on invalid code, rather than printing a proper diagnostic.

None of the tests are particularly long, but here's an overview of the failure cases (caveat: I haven't investigated most of these, so take these descriptions as educated guesses at the root cause):

  1. Importing an inlinable function body that contains new of an instantiated type crashes.
  2. Recursive instantiation of a generic method crashes during dictionary insertion.
  3. Importer crashes on channel types (and probably others) embedded into a type parameter constraint.
  4. Importer/exporter don't support labeled for loops, so instantiating an imported generic function that contains one crashes during SSA because the label references are unresolved.
  5. Inconsistent mangling when instantiating a type with anonymous interface as a type argument: X[interface{}] vs X[interface {}].
  6. Method expressions involving an instantiated interface type fail.
  7. Importer crashes on importing an instantiated interface.
  8. Embedded comparable constraint is lost during -G=3 noding, so consequently it isn't exported or re-imported. This causes incorrect code to be accepted, and could also (potentially) cause correct code to be misoptimized (e.g., assuming that simple GC-shape stenciling is appropriate, rather distinguishing types that require different equality operations, like uint32 and float32 or string and struct { *byte; int }).
  9. Exporter does not include intermediary types for composite literal keyed elements.
  10. Method expression involving an imported instantiated interface types result in a linker error because of a missing wrapper function. (Note: currently masked by issue 6.)
  11. Compiler crash on invalid code involving //go:notinheap rather than an error message.

Issue 10 and Unified IR

I knew one backend feature I needed for unified IR was to create type descriptors for imported generic types and mark them as DUPOK. While looking at the reflectdata code to make this happen, I saw a Type.IsFullyInstantiated flag had been added for -G=3 and was used to handle this already. I also saw other code checked this. Rather than duplicate this logic, I thought it would be less invasive to arrange for Type.IsFullyInstantiated to report true for unified IR's instantiated types too.

However, as issue 10 demonstrates, the existing support for that flag is incomplete. In particular, reflectdata.methodWrapper was extended for -G=3 to generate method wrappers for imported generic concrete types, but not to generate method wrappers for imported generic interface types:

// Only generate (*T).M wrappers for T.M in T's own package, except for
// instantiated methods.
if rcvr.IsPtr() && rcvr.Elem() == method.Type.Recv().Type &&
rcvr.Elem().Sym() != nil && rcvr.Elem().Sym().Pkg != types.LocalPkg &&
!rcvr.Elem().IsFullyInstantiated() {
return lsym
}
// Only generate I.M wrappers for I in I's own package
// but keep doing it for error.Error (was issue #29304).
if rcvr.IsInterface() && rcvr.Sym() != nil && rcvr.Sym().Pkg != types.LocalPkg && rcvr != types.ErrorType {
return lsym
}

Future work

I think fuzz testing would be very useful (/cc @thepudds). I think there's a lot of room for testing more complex language features (e.g., function literals; go, defer, range, and select statements; multi-package tests with re-exported generic types and functions; tests with identically named packages).

I also think as dictionary and GC shape support continues, actually fuzz testing the resulting code (i.e., as opposed to just verifying the compiler doesn't crash) will be important too.

/cc @danscales @randall77 @griesemer @ianlancetaylor @rsc @eliben

@mdempsky mdempsky added this to the Go1.18 milestone Jun 11, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jun 11, 2021

Change https://golang.org/cl/327055 mentions this issue: [dev.typeparams] cmd/compile: fix wrapper generation for imported generics

gopherbot pushed a commit that referenced this issue Jun 11, 2021
…erics

This CL fixes reflectdata.methodWrapper to compile wrapper functions
for method expressions involving imported, instantiated interface
types. CL 322193 fixed a similar issue for generating wrappers for
imported, instantiated concrete types, but missed this case.

This is necessary to fix CL 326169's test case 10. However, that test
case is not included currently, because -G=3 mode crashes on method
expressions involving *any* instantiated interface type. Adding a test
will have to wait until either this issue is fixed in -G=3 mode, or
unified IR is merged.

Updates #46704.

Change-Id: Ib02d3c20e7c69d16288f1286cd1c98e7cbbba114
Reviewed-on: https://go-review.googlesource.com/c/go/+/327055
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Dan Scales <danscales@google.com>
Trust: Dan Scales <danscales@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jun 14, 2021

After #46725 fixed, test/fixedbugs/issue46725.go is also failed with -G=3

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jun 28, 2021

Another failure case:

$ cat go.mod
module foo

go 1.18

$ cat a/a.go
package a

type S[T any] struct {
        F T
}

var X = S[int]{}

$ cat main.go
package main

import (
	"foo/a"
)

func main() {
	_ = a.X
}

$ go run -gcflags='all=-G=3' main.go
# command-line-arguments
./a/a.go:3:6: internal compiler error: failed to find type loop for: a.S[int]
<truncated stack trace>
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jun 28, 2021

@cuonglm That looks pretty similar to test case 1's failure. Maybe the issue is related to the blank assignment, not the new call like I initially conjectured.

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jun 28, 2021

@mdempsky it seems to me iimport.go:L707 missed the condition for fully instantiated type.

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jul 2, 2021

Another test program that crashes with -G=3 (and go2go), but passes with unified IR. This one fails with "mer.go:14:5: internal compiler error: Unexpected op with CALL during stenciling: METHEXPR" when compiled with -G=3:

package main

type Mer interface{ M() }

func F[T Mer](expectPanic bool) {
	defer func() {
		err := recover()
		if (err != nil) != expectPanic {
			print("FAIL: (", err, " != nil) != ", expectPanic, "\n")
		}
	}()

	var t T
	T.M(t)
}

type MyMer int

func (MyMer) M() {}

func main() {
	F[Mer](true)
	F[struct{ Mer }](true)
	F[*struct{ Mer }](true)

	F[MyMer](false)
	F[*MyMer](true)
	F[struct{ MyMer }](false)
	F[struct{ *MyMer }](true)
	F[*struct{ MyMer }](true)
	F[*struct{ *MyMer }](true)
}
@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented Jul 2, 2021

This program fails to compile with -G=3 due to "internal compiler error: 'main': Value live at entry. It shouldn't be. func main, node .dict0, value nil":

package main

type T[_ any] int
func (T[_]) M() {}

type I interface{ M() }

type U = T[int]
var x = U(0)
var i I = x

func main() {
	U.M(x)
	i.M()
}
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 2, 2021

Change https://golang.org/cl/326169 mentions this issue: [dev.typeparams] test: add regress tests that fail(ed) with -G=3

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 2, 2021

Change https://golang.org/cl/332551 mentions this issue: [dev.typeparams] test: add expected failure mechanism

gopherbot pushed a commit that referenced this issue Jul 7, 2021
This CL changes the existing excluded-test mechanism into a
known-failure mechanism instead. That is, it runs the test regardless,
but only reports if it failed (or succeeded) unexpectedly.

It also splits the known failures list into fine-grain failure lists
for types2, types2 w/ 32-bit target, -G=3, and unified.

Updates #46704.

Change-Id: I1213cbccf1bab6a92d9bfcf0d971a2554249bbff
Reviewed-on: https://go-review.googlesource.com/c/go/+/332551
Trust: Matthew Dempsky <mdempsky@google.com>
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
gopherbot pushed a commit that referenced this issue Jul 7, 2021
This CL includes multiple test cases that exercise unique failures
with -G=3 mode that did not affect unified IR mode. Most of these were
found over a period of about 3 hours of manual experimentation.

Thanks to Cuong Manh Le for test cases 11 and 12.

Updates #46704.

Change-Id: Ia2fa619536732b121b6c929329065c85b9384511
Reviewed-on: https://go-review.googlesource.com/c/go/+/326169
Trust: Matthew Dempsky <mdempsky@google.com>
Trust: Dan Scales <danscales@google.com>
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
@danscales danscales self-assigned this Jul 7, 2021
@danscales
Copy link

@danscales danscales commented Jul 7, 2021

Thanks for reporting and making great examples for the bugs, Matthew and Cuong! Here's an update:

I diagnosed all of the bugs fairly easily (in about 90 minutes), and also created fixes for most of them. I'll be making CLs with the fixes over the next few days. Two of them are already out for review.

A couple notes:

  • 6.go and 10.go are already fixed - I believe that they were both dictionary-related bugs. We are actively developing dictionaries and unified IR does not include our dictionary analyzing/transforming code. 14.go is also a known dictionary issue and I have a fix out for that.
  • 3.go is just relating to switching the default from 1.17 to 1.18
  • 4.go is the known bug that we don't export/import functions with labeled break statements. (I know that unified IR can already do this.) This is the only bug that I won't immediately fix.
  • 11.go appears to be a bug in types2 (types2 is not finding a mismatch between two types which only differ in having/not having //go:notinheap). This bug doesn't show up in unified IR, because unified IR is running both types2 and types1 typechecking on most code. -G=3 uses only types2 typechecking for most user code. I'll report that to Robert.
@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jul 7, 2021

11.go appears to be a bug in types2 (types2 is not finding a mismatch between two types which only differ in having/not having //go:notinheap). This bug doesn't show up in unified IR, because unified IR is running both types2 and types1 typechecking on most code. -G=3 uses only types2 typechecking for most user code. I'll report that to Robert.

I think this is not a bug in types2, as pragma is not part of the language spec, and it exists for the compiler, not for the typechecker.

@danscales
Copy link

@danscales danscales commented Jul 7, 2021

11.go appears to be a bug in types2 (types2 is not finding a mismatch between two types which only differ in having/not having //go:notinheap). This bug doesn't show up in unified IR, because unified IR is running both types2 and types1 typechecking on most code. -G=3 uses only types2 typechecking for most user code. I'll report that to Robert.

I think this is not a bug in types2, as pragma is not part of the language spec, and it exists for the compiler, not for the typechecker.

OK, thanks, Robert confirmed that he does not currently deal with pragmas in types2 and is hoping to keep it that way. I will try to handle this in noder2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants