From 804ecc2581caf33ae347d6a1ce67436d1f74e93b Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Wed, 16 Jun 2021 21:41:28 -0700 Subject: [PATCH] [dev.typeparams] all: add GOEXPERIMENT=unified knob Setting `-gcflags=all=-d=unified` works for normal builds/tests, but seems to have trouble with the test/run.go regress tests. So add a GOEXPERIMENT knob to allow another way to turn on unified IR construction, which plays better with all.bash. While here, update two existing test expectations that currently fail during GOEXPERIMENT=unified ./all.bash: 1. misc/cgo/errors/testdata/err2.go is testing column positions, and types2 gets one case slightly better, and another case slightly worse. For now, the test case is updated to accept both. 2. fixedbugs/issue42284.go is added to the list of known failures, because it fails for unified IR. (It's an escape analysis test, and escape analysis is working as expected; but unified is formatting an imported constant value differently than the test's regexp expects.) Updates #46786. Change-Id: I40a4a70fa1b85ac87fcc85a43687f5d81e011ec0 Reviewed-on: https://go-review.googlesource.com/c/go/+/328215 Run-TryBot: Matthew Dempsky Trust: Matthew Dempsky Trust: Cuong Manh Le TryBot-Result: Go Bot Reviewed-by: Cuong Manh Le --- misc/cgo/errors/testdata/err2.go | 12 +++- src/cmd/compile/internal/base/flag.go | 4 ++ src/internal/goexperiment/exp_unified_off.go | 9 +++ src/internal/goexperiment/exp_unified_on.go | 9 +++ src/internal/goexperiment/flags.go | 4 ++ test/run.go | 66 +++++++++++++++----- 6 files changed, 86 insertions(+), 18 deletions(-) create mode 100644 src/internal/goexperiment/exp_unified_off.go create mode 100644 src/internal/goexperiment/exp_unified_on.go diff --git a/misc/cgo/errors/testdata/err2.go b/misc/cgo/errors/testdata/err2.go index a90598fe35b..aa941584c3c 100644 --- a/misc/cgo/errors/testdata/err2.go +++ b/misc/cgo/errors/testdata/err2.go @@ -91,10 +91,18 @@ func main() { // issue 26745 _ = func(i int) int { - return C.i + 1 // ERROR HERE: 14 + // typecheck reports at column 14 ('+'), but types2 reports at + // column 10 ('C'). + // TODO(mdempsky): Investigate why, and see if types2 can be + // updated to match typecheck behavior. + return C.i + 1 // ERROR HERE: \b(10|14)\b } _ = func(i int) { - C.fi(i) // ERROR HERE: 7 + // typecheck reports at column 7 ('('), but types2 reports at + // column 8 ('i'). The types2 position is more correct, but + // updating typecheck here is fundamentally challenging because of + // IR limitations. + C.fi(i) // ERROR HERE: \b(7|8)\b } C.fi = C.fi // ERROR HERE diff --git a/src/cmd/compile/internal/base/flag.go b/src/cmd/compile/internal/base/flag.go index 42c0c1b94b5..b8b205f4127 100644 --- a/src/cmd/compile/internal/base/flag.go +++ b/src/cmd/compile/internal/base/flag.go @@ -159,7 +159,11 @@ func ParseFlags() { Flag.LinkShared = &Ctxt.Flag_linkshared Flag.Shared = &Ctxt.Flag_shared Flag.WB = true + Debug.InlFuncsWithClosures = 1 + if buildcfg.Experiment.Unified { + Debug.Unified = 1 + } Debug.Checkptr = -1 // so we can tell whether it is set explicitly diff --git a/src/internal/goexperiment/exp_unified_off.go b/src/internal/goexperiment/exp_unified_off.go new file mode 100644 index 00000000000..4c16fd85624 --- /dev/null +++ b/src/internal/goexperiment/exp_unified_off.go @@ -0,0 +1,9 @@ +// Code generated by mkconsts.go. DO NOT EDIT. + +//go:build !goexperiment.unified +// +build !goexperiment.unified + +package goexperiment + +const Unified = false +const UnifiedInt = 0 diff --git a/src/internal/goexperiment/exp_unified_on.go b/src/internal/goexperiment/exp_unified_on.go new file mode 100644 index 00000000000..2b17ba3e79b --- /dev/null +++ b/src/internal/goexperiment/exp_unified_on.go @@ -0,0 +1,9 @@ +// Code generated by mkconsts.go. DO NOT EDIT. + +//go:build goexperiment.unified +// +build goexperiment.unified + +package goexperiment + +const Unified = true +const UnifiedInt = 1 diff --git a/src/internal/goexperiment/flags.go b/src/internal/goexperiment/flags.go index 71e38cd0479..b7a62b3e268 100644 --- a/src/internal/goexperiment/flags.go +++ b/src/internal/goexperiment/flags.go @@ -59,6 +59,10 @@ type Flags struct { PreemptibleLoops bool StaticLockRanking bool + // Unified enables the compiler's unified IR construction + // experiment. + Unified bool + // Regabi is split into several sub-experiments that can be // enabled individually. Not all combinations work. // The "regabi" GOEXPERIMENT is an alias for all "working" diff --git a/test/run.go b/test/run.go index 656519e3017..f8bb8c081c7 100644 --- a/test/run.go +++ b/test/run.go @@ -58,8 +58,9 @@ func defaultAllCodeGen() bool { } var ( - goos, goarch string - cgoEnabled bool + goos, goarch string + cgoEnabled bool + unifiedEnabled bool // dirs are the directories to look for *.go files in. // TODO(bradfitz): just use all directories? @@ -95,10 +96,27 @@ func main() { goos = getenv("GOOS", runtime.GOOS) goarch = getenv("GOARCH", runtime.GOARCH) - cgoEnv, err := exec.Command(goTool(), "env", "CGO_ENABLED").Output() - if err == nil { - cgoEnabled, _ = strconv.ParseBool(strings.TrimSpace(string(cgoEnv))) + + cgoCmd := exec.Command(goTool(), "env", "CGO_ENABLED") + cgoEnv, err := cgoCmd.Output() + if err != nil { + log.Fatalf("running %v: %v", cgoCmd, err) } + cgoEnabled, _ = strconv.ParseBool(strings.TrimSpace(string(cgoEnv))) + + // TODO(mdempsky): Change this to just "go env GOEXPERIMENT" after + // CL 328751 is merged back to dev.typeparams. In the mean time, we + // infer whether the "unified" experiment is defult enabled by + // inspecting the output from `go tool compile -V`. + compileCmd := exec.Command(goTool(), "tool", "compile", "-V") + compileOutput, err := compileCmd.Output() + if err != nil { + log.Fatalf("running %v: %v", compileCmd, err) + } + // TODO(mdempsky): This will give false negatives if the unified + // experiment is enabled by default, but presumably at that point we + // won't need to disable tests for it anymore anyway. + unifiedEnabled = strings.Contains(string(compileOutput), "unified") findExecCmd() @@ -290,6 +308,10 @@ type test struct { err error } +// usesTypes2 reports whether the compiler uses types2 for this test +// configuration (irrespective of flags specified by the test itself). +func (t *test) usesTypes2() bool { return unifiedEnabled || t.glevel != 0 } + func startTests(dir, gofile string, glevels []int) []*test { tests := make([]*test, len(glevels)) for i, glevel := range glevels { @@ -519,8 +541,8 @@ func (t *test) run() { close(t.donec) }() - if t.glevel > 0 && !*force { - // Files excluded from generics testing. + if t.usesTypes2() && !*force { + // Files excluded from types2 testing. filename := strings.Replace(t.goFileName(), "\\", "/", -1) // goFileName() uses \ on Windows if excludedFiles[filename] { if *verbose { @@ -666,27 +688,35 @@ func (t *test) run() { // at the specified -G level. If so, it may update flags as // necessary to test with -G. validForGLevel := func(tool Tool) bool { - if t.glevel == 0 { - // default -G level; always valid + if !t.usesTypes2() { + // tests should always pass when run w/o types2 (i.e., using the + // legacy typechecker). return true } + hasGFlag := false for _, flag := range flags { if strings.Contains(flag, "-G") { - // test provides explicit -G flag already - if *verbose { - fmt.Printf("excl\t%s\n", t.goFileName()) - } - return false + hasGFlag = true } } + if hasGFlag && t.glevel != 0 { + // test provides explicit -G flag already; don't run again + if *verbose { + fmt.Printf("excl\t%s\n", t.goFileName()) + } + return false + } + switch tool { case Build, Run: // ok; handled in goGcflags case Compile: - flags = append(flags, fmt.Sprintf("-G=%v", t.glevel)) + if !hasGFlag { + flags = append(flags, fmt.Sprintf("-G=%v", t.glevel)) + } default: // we don't know how to add -G for this test yet @@ -2026,6 +2056,9 @@ func overlayDir(dstRoot, srcRoot string) error { // List of files that the compiler cannot errorcheck with the new typechecker (compiler -G option). // Temporary scaffolding until we pass all the tests at which point this map can be removed. +// +// TODO(mdempsky): Split exclude list to disambiguate whether the +// failure is within types2, -G=3, or unified. var excludedFiles = map[string]bool{ "directive.go": true, // misplaced compiler directive checks "float_lit3.go": true, // types2 reports extra errors @@ -2079,10 +2112,11 @@ var excludedFiles = map[string]bool{ "fixedbugs/issue33460.go": true, // types2 reports alternative positions in separate error "fixedbugs/issue42058a.go": true, // types2 doesn't report "channel element type too large" "fixedbugs/issue42058b.go": true, // types2 doesn't report "channel element type too large" - "fixedbugs/issue46725.go": true, // fix applied to typecheck needs to be ported to irgen/transform + "fixedbugs/issue42284.go": true, // unified formats important constant expression differently in diagnostics "fixedbugs/issue4232.go": true, // types2 reports (correct) extra errors "fixedbugs/issue4452.go": true, // types2 reports (correct) extra errors "fixedbugs/issue4510.go": true, // types2 reports different (but ok) line numbers + "fixedbugs/issue46725.go": true, // fix applied to typecheck needs to be ported to irgen/transform "fixedbugs/issue5609.go": true, // types2 needs a better error message "fixedbugs/issue7525b.go": true, // types2 reports init cycle error on different line - ok otherwise "fixedbugs/issue7525c.go": true, // types2 reports init cycle error on different line - ok otherwise