From a8e55538af40905961c263d980760b76e9c43593 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Tue, 6 Apr 2021 16:11:17 -0400 Subject: [PATCH] cmd/internal/objabi: make GOEXPERIMENT be a diff from default experiments Right now the rules around handling default-on experiments are complicated and a bit inconsistent. Notably, objabi.GOEXPERIMENT is set to a comma-separated list of enabled experiments, but this may not be the string a user should set the GOEXPERIMENT environment variable to get that list of experiments: if an experiment is enabled by default but gets turned off by GOEXPERIMENT, then the string we report needs to include "no"+experiment to capture that default override. This complication also seeps into the version string we print for "go tool compile -V", etc. This logic is further complicated by the fact that it only wants to include an experiment string if the set of experiments varies from the default. This CL rethinks how we handle default-on experiments. Now that experiment state is all captured in a struct, we can simplify a lot of this logic. objabi.GOEXPERIMENT will be set based on the delta from the default set of experiments, which reflects what a user would actually need to pass on the command line. Likewise, we include this delta in the "-V" output, which simplifies this logic because if there's nothing to show in the version string, the delta will be empty. Change-Id: I7ed307329541fc2c9f90edd463fbaf8e0cc9e8ee Reviewed-on: https://go-review.googlesource.com/c/go/+/307819 Trust: Austin Clements Run-TryBot: Austin Clements TryBot-Result: Go Bot Reviewed-by: Matthew Dempsky --- src/cmd/go/script_test.go | 2 +- src/cmd/go/testdata/script/README | 2 +- .../script/build_tag_goexperiment.txt | 16 ++--- src/cmd/internal/objabi/exp.go | 69 +++++++++++-------- src/cmd/internal/objabi/flag.go | 18 ++--- src/cmd/internal/objabi/util.go | 2 +- src/internal/goexperiment/flags.go | 5 ++ 7 files changed, 62 insertions(+), 52 deletions(-) diff --git a/src/cmd/go/script_test.go b/src/cmd/go/script_test.go index 1f38be8ee4..c353a9cb01 100644 --- a/src/cmd/go/script_test.go +++ b/src/cmd/go/script_test.go @@ -165,7 +165,7 @@ func (ts *testScript) setup() { "GOCACHE=" + testGOCACHE, "GODEBUG=" + os.Getenv("GODEBUG"), "GOEXE=" + cfg.ExeSuffix, - "GOEXPSTRING=" + objabi.Expstring()[2:], + "GOEXPERIMENT=" + objabi.GOEXPERIMENT(), "GOOS=" + runtime.GOOS, "GOPATH=" + filepath.Join(ts.workdir, "gopath"), "GOPROXY=" + proxyURL, diff --git a/src/cmd/go/testdata/script/README b/src/cmd/go/testdata/script/README index d658cebfce..b4dcb1f5a2 100644 --- a/src/cmd/go/testdata/script/README +++ b/src/cmd/go/testdata/script/README @@ -29,7 +29,7 @@ Scripts also have access to these other environment variables: GOARCH= GOCACHE= GOEXE= - GOEXPSTRING= + GOEXPERIMENT= GOOS= GOPATH=$WORK/gopath GOPROXY= diff --git a/src/cmd/go/testdata/script/build_tag_goexperiment.txt b/src/cmd/go/testdata/script/build_tag_goexperiment.txt index 26ad029845..dfda3d2629 100644 --- a/src/cmd/go/testdata/script/build_tag_goexperiment.txt +++ b/src/cmd/go/testdata/script/build_tag_goexperiment.txt @@ -1,6 +1,6 @@ # compile_ext will fail if the buildtags that are enabled (or not enabled) for the # framepointer and fieldtrack experiments are not consistent with the value of -# GOEXPSTRING (which comes from objabi.Expstring()). +# objabi.GOEXPERIMENT. [short] skip go run m @@ -19,10 +19,8 @@ func main() { } func hasExpEntry(s string) bool { - // script_test.go defines GOEXPSTRING to be the value of - // objabi.Expstring(), which gives the enabled experiments baked into the - // toolchain. - g := os.Getenv("GOEXPSTRING") + // script_test.go defines GOEXPERIMENT to be the enabled experiments. + g := os.Getenv("GOEXPERIMENT") for _, f := range strings.Split(g, ",") { if f == s { return true @@ -43,7 +41,7 @@ import ( func fp() { if hasExpEntry("framepointer") { - fmt.Println("in !framepointer build, but objabi.Expstring() has 'framepointer'") + fmt.Println("in !framepointer build, but objabi.GOEXPERIMENT has 'framepointer'") os.Exit(1) } } @@ -60,7 +58,7 @@ import ( func fp() { if !hasExpEntry("framepointer") { - fmt.Println("in framepointer build, but objabi.Expstring() does not have 'framepointer', is", os.Getenv("GOEXPSTRING")) + fmt.Println("in framepointer build, but objabi.GOEXPERIMENT does not have 'framepointer', is", os.Getenv("GOEXPERIMENT")) os.Exit(1) } } @@ -77,7 +75,7 @@ import ( func ft() { if hasExpEntry("fieldtrack") { - fmt.Println("in !fieldtrack build, but objabi.Expstring() has 'fieldtrack'") + fmt.Println("in !fieldtrack build, but objabi.GOEXPERIMENT has 'fieldtrack'") os.Exit(1) } } @@ -94,7 +92,7 @@ import ( func ft() { if !hasExpEntry("fieldtrack") { - fmt.Println("in fieldtrack build, but objabi.Expstring() does not have 'fieldtrack', is", os.Getenv("GOEXPSTRING")) + fmt.Println("in fieldtrack build, but objabi.GOEXPERIMENT does not have 'fieldtrack', is", os.Getenv("GOEXPERIMENT")) os.Exit(1) } } diff --git a/src/cmd/internal/objabi/exp.go b/src/cmd/internal/objabi/exp.go index 21a70d5dfe..eaa8620807 100644 --- a/src/cmd/internal/objabi/exp.go +++ b/src/cmd/internal/objabi/exp.go @@ -20,8 +20,6 @@ import ( // was built with.) var Experiment goexperiment.Flags -var defaultExpstring string // Set by package init - // FramePointerEnabled enables the use of platform conventions for // saving frame pointers. // @@ -32,13 +30,15 @@ var defaultExpstring string // Set by package init var FramePointerEnabled = GOARCH == "amd64" || GOARCH == "arm64" func init() { - // Capture "default" experiments. - defaultExpstring = Expstring() + // Start with the baseline configuration. + Experiment = goexperiment.BaselineFlags - goexperiment := envOr("GOEXPERIMENT", defaultGOEXPERIMENT) + // Pick up any changes to the baseline configuration from the + // GOEXPERIMENT environment. This can be set at make.bash time + // and overridden at build time. + env := envOr("GOEXPERIMENT", defaultGOEXPERIMENT) - // GOEXPERIMENT=none overrides all experiments enabled at dist time. - if goexperiment != "none" { + if env != "" { // Create a map of known experiment names. names := make(map[string]reflect.Value) rv := reflect.ValueOf(&Experiment).Elem() @@ -49,10 +49,16 @@ func init() { } // Parse names. - for _, f := range strings.Split(goexperiment, ",") { + for _, f := range strings.Split(env, ",") { if f == "" { continue } + if f == "none" { + // GOEXPERIMENT=none restores the baseline configuration. + // (This is useful for overriding make.bash-time settings.) + Experiment = goexperiment.BaselineFlags + continue + } val := true if strings.HasPrefix(f, "no") { f, val = f[2:], false @@ -93,40 +99,45 @@ func init() { } } -// expList returns the list of enabled GOEXPERIMENTs names. -func expList(flags *goexperiment.Flags) []string { +// expList returns the list of lower-cased experiment names for +// experiments that differ from base. base may be nil to indicate no +// experiments. +func expList(exp, base *goexperiment.Flags) []string { var list []string - rv := reflect.ValueOf(&Experiment).Elem() + rv := reflect.ValueOf(exp).Elem() + var rBase reflect.Value + if base != nil { + rBase = reflect.ValueOf(base).Elem() + } rt := rv.Type() for i := 0; i < rt.NumField(); i++ { + name := strings.ToLower(rt.Field(i).Name) val := rv.Field(i).Bool() - if val { - field := rt.Field(i) - list = append(list, strings.ToLower(field.Name)) + baseVal := false + if base != nil { + baseVal = rBase.Field(i).Bool() + } + if val != baseVal { + if val { + list = append(list, name) + } else { + list = append(list, "no"+name) + } } } return list } -// Expstring returns the GOEXPERIMENT string that should appear in Go -// version signatures. This always starts with "X:". -func Expstring() string { - list := expList(&Experiment) - if len(list) == 0 { - return "X:none" - } - return "X:" + strings.Join(list, ",") -} - -// GOEXPERIMENT returns a comma-separated list of enabled experiments. -// This is derived from the GOEXPERIMENT environment variable if set, -// or the value of GOEXPERIMENT when make.bash was run if not. +// GOEXPERIMENT is a comma-separated list of enabled or disabled +// experiments that differ from the baseline experiment configuration. +// GOEXPERIMENT is exactly what a user would set on the command line +// to get the set of enabled experiments. func GOEXPERIMENT() string { - return strings.Join(expList(&Experiment), ",") + return strings.Join(expList(&Experiment, &goexperiment.BaselineFlags), ",") } // EnabledExperiments returns a list of enabled experiments, as // lower-cased experiment names. func EnabledExperiments() []string { - return expList(&Experiment) + return expList(&Experiment, nil) } diff --git a/src/cmd/internal/objabi/flag.go b/src/cmd/internal/objabi/flag.go index 25b0185f64..6a8a69116d 100644 --- a/src/cmd/internal/objabi/flag.go +++ b/src/cmd/internal/objabi/flag.go @@ -91,16 +91,12 @@ func (versionFlag) Set(s string) error { name = name[strings.LastIndex(name, `\`)+1:] name = strings.TrimSuffix(name, ".exe") - // If there's an active experiment, include that, - // to distinguish go1.10.2 with an experiment - // from go1.10.2 without an experiment. - p := Expstring() - if p == defaultExpstring { - p = "" - } - sep := "" - if p != "" { - sep = " " + p := "" + + // If the enabled experiments differ from the defaults, + // include that difference. + if goexperiment := GOEXPERIMENT(); goexperiment != "" { + p = " X:" + goexperiment } // The go command invokes -V=full to get a unique identifier @@ -114,7 +110,7 @@ func (versionFlag) Set(s string) error { } } - fmt.Printf("%s version %s%s%s\n", name, Version, sep, p) + fmt.Printf("%s version %s%s\n", name, Version, p) os.Exit(0) return nil } diff --git a/src/cmd/internal/objabi/util.go b/src/cmd/internal/objabi/util.go index c2c05bd1b2..5a7a74cfde 100644 --- a/src/cmd/internal/objabi/util.go +++ b/src/cmd/internal/objabi/util.go @@ -128,5 +128,5 @@ func Getgoextlinkenabled() string { // or link object files that are incompatible with each other. This // string always starts with "go object ". func HeaderString() string { - return fmt.Sprintf("go object %s %s %s %s\n", GOOS, GOARCH, Version, Expstring()) + return fmt.Sprintf("go object %s %s %s X:%s\n", GOOS, GOARCH, Version, strings.Join(EnabledExperiments(), ",")) } diff --git a/src/internal/goexperiment/flags.go b/src/internal/goexperiment/flags.go index c5e3d2c91a..1c513d5a70 100644 --- a/src/internal/goexperiment/flags.go +++ b/src/internal/goexperiment/flags.go @@ -75,3 +75,8 @@ type Flags struct { // register arguments to defer/go). RegabiArgs bool } + +// BaselineFlags specifies the experiment flags that are enabled by +// default in the current toolchain. This is, in effect, the "control" +// configuration and any variation from this is an experiment. +var BaselineFlags = Flags{}