cmd/compile: multiple compiler crashes and failures with -G=3 #46704
Description
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):
- Importing an inlinable function body that contains
newof an instantiated type crashes. - Recursive instantiation of a generic method crashes during dictionary insertion.
- Importer crashes on channel types (and probably others) embedded into a type parameter constraint.
- 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.
- Inconsistent mangling when instantiating a type with anonymous interface as a type argument:
X[interface{}]vsX[interface {}]. - Method expressions involving an instantiated interface type fail.
- Importer crashes on importing an instantiated interface.
- Embedded
comparableconstraint 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, likeuint32andfloat32orstringandstruct { *byte; int }). - Exporter does not include intermediary types for composite literal keyed elements.
- 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.)
- Compiler crash on invalid code involving
//go:notinheaprather 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:
go/src/cmd/compile/internal/reflectdata/reflect.go
Lines 1794 to 1806 in b207473
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