[go] cmd/internal/objabi: clarify initialization of experiments
13 views
Skip to first unread message
Austin Clements (Gerrit)
Apr 6, 2021, 6:14:45 PM4/6/21
to Matthew Dempsky, goph...@pubsubhelper.golang.org, Austin Clements, golang-co...@googlegroups.com
Attention is currently required from: Matthew Dempsky.
Austin Clements would like Matthew Dempsky to review this change.
cmd/internal/objabi: clarify initialization of experiments
DO NOT SUBMIT. This seemed like a good idea, but I'm not sure it
actually clarifies the code.
Currently the experiment-related package variables in objabi are set
by a package init function, which makes their initialization process
somewhat unclear (indeed, I've messed this up before) and opens the
possibility of accessing them from another init function before their
initialized.
This CL changes these packages variables to use initializers to make
this more robust.
Change-Id: Id0d2ac76ae463824bbf37a9305e8643a275f1365
---
M src/cmd/internal/objabi/exp.go
1 file changed, 43 insertions(+), 36 deletions(-)
diff --git a/src/cmd/internal/objabi/exp.go b/src/cmd/internal/objabi/exp.go
index c8508d2..513c20f 100644
--- a/src/cmd/internal/objabi/exp.go
+++ b/src/cmd/internal/objabi/exp.go
@@ -18,15 +18,17 @@
//
// (This is not necessarily the set of experiments the compiler itself
// was built with.)
-var Experiment = goexperiment.DefaultFlags
+var Experiment goexperiment.Flags = parseExperiments()
// EnabledExperiments is a list of enabled experiments, as lower-cased
// experiment names.
-var EnabledExperiments []string
+var EnabledExperiments []string = expList(&Experiment, nil)
// GOEXPERIMENT is a comma-separated list of enabled or disabled
// experiments that differ from the default experiment settings.
-var GOEXPERIMENT string
+// GOEXPERIMENT is exactly what a user would set on the command line
+// to get the set of enabled experiments.
+var GOEXPERIMENT string = strings.Join(expList(&Experiment, &goexperiment.DefaultFlags), ",")
// FramePointerEnabled enables the use of platform conventions for
// saving frame pointers.
@@ -37,16 +39,17 @@
// Note: must agree with runtime.framepointer_enabled.
var FramePointerEnabled = GOARCH == "amd64" || GOARCH == "arm64"
-func init() {
+func parseExperiments() goexperiment.Flags {
+ flags := goexperiment.DefaultFlags
env := envOr("GOEXPERIMENT", defaultGOEXPERIMENT)
// GOEXPERIMENT=none disables all experiments.
if env == "none" {
- Experiment = goexperiment.Flags{}
+ flags = goexperiment.Flags{}
} else {
// Create a map of known experiment names.
names := make(map[string]reflect.Value)
- rv := reflect.ValueOf(&Experiment).Elem()
+ rv := reflect.ValueOf(&flags).Elem()
rt := rv.Type()
for i := 0; i < rt.NumField(); i++ {
field := rv.Field(i)
@@ -73,54 +76,58 @@
// regabi is only supported on amd64.
if GOARCH != "amd64" {
- Experiment.Regabi = false
- Experiment.RegabiWrappers = false
- Experiment.RegabiG = false
- Experiment.RegabiReflect = false
- Experiment.RegabiDefer = false
- Experiment.RegabiArgs = false
+ flags.Regabi = false
+ flags.RegabiWrappers = false
+ flags.RegabiG = false
+ flags.RegabiReflect = false
+ flags.RegabiDefer = false
+ flags.RegabiArgs = false
}
// Setting regabi sets working sub-experiments.
- if Experiment.Regabi {
- Experiment.RegabiWrappers = true
- Experiment.RegabiG = true
- Experiment.RegabiReflect = true
- Experiment.RegabiDefer = true
+ if flags.Regabi {
+ flags.RegabiWrappers = true
+ flags.RegabiG = true
+ flags.RegabiReflect = true
+ flags.RegabiDefer = true
// Not ready yet:
- //Experiment.RegabiArgs = true
+ //flags.RegabiArgs = true
}
// Check regabi dependencies.
- if Experiment.RegabiG && !Experiment.RegabiWrappers {
+ if flags.RegabiG && !flags.RegabiWrappers {
panic("GOEXPERIMENT regabig requires regabiwrappers")
}
- if Experiment.RegabiArgs && !(Experiment.RegabiWrappers && Experiment.RegabiG && Experiment.RegabiReflect && Experiment.RegabiDefer) {
+ if flags.RegabiArgs && !(flags.RegabiWrappers && flags.RegabiG && flags.RegabiReflect && flags.RegabiDefer) {
panic("GOEXPERIMENT regabiargs requires regabiwrappers,regabig,regabireflect,regabidefer")
}
- // Now that all experiment flags are set, get the list of
- // enabled experiments, and the different-from-default list.
- var diff []string
- rv := reflect.ValueOf(&Experiment).Elem()
- rDef := reflect.ValueOf(&goexperiment.DefaultFlags).Elem()
+ return flags
+}
+
+// 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(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 {
- EnabledExperiments = append(EnabledExperiments, name)
+ baseVal := false
+ if base != nil {
+ baseVal = rBase.Field(i).Bool()
}
- if val != rDef.Field(i).Bool() {
+ if val != baseVal {
if val {
- diff = append(diff, name)
+ list = append(list, name)
} else {
- diff = append(diff, "no"+name)
+ list = append(list, "no"+name)
}
}
-
}
-
- // Set GOEXPERIMENT to the delta from the default experiments.
- // This way, GOEXPERIMENT is exactly what a user would set on
- // the command line to get the set of enabled experiments.
- GOEXPERIMENT = strings.Join(diff, ",")
+ return list
}
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Apr 6, 2021, 8:53:51 PM4/6/21
to Austin Clements, goph...@pubsubhelper.golang.org, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Austin Clements.
1 comment:
Patchset:
I think this is fine, though I'd also support just turning the dependent variables into functions (as mentioned in the earlier CL).
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Apr 7, 2021, 2:01:48 PM4/7/21
to Austin Clements, goph...@pubsubhelper.golang.org, Go Bot, Matthew Dempsky, golang-co...@googlegroups.com
Attention is currently required from: Matthew Dempsky.
1 comment:
Patchset:
I think this is fine, though I'd also support just turning the dependent variables into functions (a […]
Given the variable -> function change earlier, I like this CL better, so I'll drop the DO NOT SUBMIT.
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Apr 7, 2021, 2:25:03 PM4/7/21
to Austin Clements, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Matthew Dempsky.
Austin Clements uploaded patch set #2 to this change.
cmd/internal/objabi: clarify initialization of experiments
Currently the experiment-related package variables in objabi are set
by a package init function, which makes their initialization process
somewhat unclear (indeed, I've messed this up before) and opens the
possibility of accessing them from another init function before their
initialized.
This CL changes these packages variables to use initializers to make
this more robust.
Change-Id: Id0d2ac76ae463824bbf37a9305e8643a275f1365
---
M src/cmd/internal/objabi/exp.go
1 file changed, 44 insertions(+), 47 deletions(-)
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Apr 7, 2021, 3:04:31 PM4/7/21
to Austin Clements, goph...@pubsubhelper.golang.org, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Austin Clements.
Patch set 2:Code-Review +2
Austin Clements (Gerrit)
Apr 7, 2021, 8:38:04 PM4/7/21
to Austin Clements, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Austin Clements.
Austin Clements uploaded patch set #3 to this change.
cmd/internal/objabi: clarify initialization of Experiments
Currently objabi.Experiments is set via side-effect from an init
function, which makes their initialization process somewhat unclear
(indeed, I've messed this up before) and opens the possibility of
accessing them from another init function before it's initialized.
Originally, this init function set several variables, but at this
point it sets only objabi.Experiments, so switch to just using a
variable initializer to make the initialization process clear.
Change-Id: Id0d2ac76ae463824bbf37a9305e8643a275f1365
---
M src/cmd/internal/objabi/exp.go
1 file changed, 21 insertions(+), 20 deletions(-)
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Apr 7, 2021, 10:17:28 PM4/7/21
to Austin Clements, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go Bot, Matthew Dempsky, golang-co...@googlegroups.com
Austin Clements submitted this change.
Approvals:
Matthew Dempsky: Looks good to me, approved
Austin Clements: Trusted; Run TryBots
Go Bot: TryBots succeeded
cmd/internal/objabi: clarify initialization of Experiments
Currently objabi.Experiments is set via side-effect from an init
function, which makes their initialization process somewhat unclear
(indeed, I've messed this up before) and opens the possibility of
accessing them from another init function before it's initialized.
Originally, this init function set several variables, but at this
point it sets only objabi.Experiments, so switch to just using a
variable initializer to make the initialization process clear.
Change-Id: Id0d2ac76ae463824bbf37a9305e8643a275f1365
Reviewed-on: https://go-review.googlesource.com/c/go/+/307821
Trust: Austin Clements <aus...@google.com>
Run-TryBot: Austin Clements <aus...@google.com>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: Matthew Dempsky <mdem...@google.com>
---
M src/cmd/internal/objabi/exp.go
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/src/cmd/internal/objabi/exp.go b/src/cmd/internal/objabi/exp.go
index eaa8620..48201ae 100644
--- a/src/cmd/internal/objabi/exp.go
+++ b/src/cmd/internal/objabi/exp.go
@@ -18,7 +18,7 @@
//
// (This is not necessarily the set of experiments the compiler itself
// was built with.)
-var Experiment goexperiment.Flags
+var Experiment goexperiment.Flags = parseExperiments()
// FramePointerEnabled enables the use of platform conventions for
// saving frame pointers.
@@ -29,9 +29,9 @@
// Note: must agree with runtime.framepointer_enabled.
var FramePointerEnabled = GOARCH == "amd64" || GOARCH == "arm64"
-func init() {
- // Start with the baseline configuration.
- Experiment = goexperiment.BaselineFlags
+func parseExperiments() goexperiment.Flags {
+ // Start with the statically enabled set of experiments.
+ flags := goexperiment.BaselineFlags
// Pick up any changes to the baseline configuration from the
// GOEXPERIMENT environment. This can be set at make.bash time
@@ -41,7 +41,7 @@
if env != "" {
// Create a map of known experiment names.
names := make(map[string]reflect.Value)
- rv := reflect.ValueOf(&Experiment).Elem()
+ rv := reflect.ValueOf(&flags).Elem()
rt := rv.Type()
for i := 0; i < rt.NumField(); i++ {
field := rv.Field(i)
@@ -56,7 +56,7 @@
if f == "none" {
// GOEXPERIMENT=none restores the baseline configuration.
// (This is useful for overriding make.bash-time settings.)
- Experiment = goexperiment.BaselineFlags
+ flags = goexperiment.BaselineFlags
continue
}
val := true
@@ -74,29 +74,30 @@
+ return flags
}
// expList returns the list of lower-cased experiment names for
2 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
The name of the file: src/cmd/internal/objabi/exp.go
Insertions: 0, Deletions: 7.
```
@@ -22:28 @@
- // 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.
- var GOEXPERIMENT string = strings.Join(expList(&Experiment, &goexperiment.BaselineFlags), ",")
-
@@ -105:106 @@
-
@@ +131:139 @@
+ // 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, &goexperiment.BaselineFlags), ",")
+ }
+
```
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Koichi Shiraishi (Gerrit)
Apr 8, 2021, 2:50:11 AM4/8/21
to Austin Clements, goph...@pubsubhelper.golang.org, Go Bot, Matthew Dempsky, golang-co...@googlegroups.com
1 comment:
Patchset:
Just confirm, can't compile go runtime after merge this(or more) CL. can compile at least 307990.
What change?
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Dempsky (Gerrit)
Apr 8, 2021, 3:53:05 AM4/8/21
to Austin Clements, goph...@pubsubhelper.golang.org, Koichi Shiraishi, Go Bot, golang-co...@googlegroups.com
Attention is currently required from: Austin Clements.
1 comment:
Patchset:
Just confirm, can't compile go runtime after merge this(or more) CL. can compile at least 307990. […]
This CL series shouldn't have changed anything, unless you're maybe setting GOEXPERIMENT. Can you file an issue with instructions on how to reproduce your issue and tag Austin and me, and we can look in the morning? Thanks.
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Koichi Shiraishi (Gerrit)
Apr 8, 2021, 4:04:56 AM4/8/21
to Austin Clements, goph...@pubsubhelper.golang.org, Go Bot, Matthew Dempsky, golang-co...@googlegroups.com
Patchset:
This CL series shouldn't have changed anything, unless you're maybe setting GOEXPERIMENT. […]
Ah, yes use GOEXPERIMENT.
$ git rev-parse --verify HEAD
a7e16abb22f1b249d2691b32a5d20206282898f2repro command is here:
$ GOROOT_BOOTSTRAP=~/sdk/go1.16.3 GOEXPERIMENT='regabi' ./make.bash -a -d -v -no-banner
$ GOROOT_BOOTSTRAP=~/sdk/go1.16.3 GOEXPERIMENT='staticlockranking' ./make.bash -a -d -v -no-bannerHASH[build runtime/internal/sys]
HASH[build runtime/internal/sys]: "devel go1.17-a7e16abb22 Thu Apr 8 07:33:58 2021 +0000 X:regabi,regabiwrappers,regabig,regabireflect,regabidefer"
HASH[build runtime/internal/sys]: "compile\n"
HASH[build runtime/internal/sys]: "goos darwin goarch amd64\n"
HASH[build runtime/internal/sys]: "import \"runtime/internal/sys\"\n"
HASH[build runtime/internal/sys]: "omitdebug false standard true local false prefix \"\"\n"
HASH[build runtime/internal/sys]: "modinfo \"\"\n"
HASH[build runtime/internal/sys]: "compile XKf66D8YA5JrEXpG97GQ [] []\n"
HASH[build runtime/internal/sys]: "=\n"
HASH[build runtime/internal/sys]: "GOEXPERIMENT=\"regabi,regabiwrappers,regabig,regabireflect,regabidefer\"\n"
HASH /usr/local/go/src/runtime/internal/sys/arch.go: ce22176d2caf2b8aa9bbf63da6fc175efb8439ae1ba0e2d45eac8c05cb3a3054
HASH[build runtime/internal/sys]: "file arch.go ziIXbSyvK4qpu_Y9pvwX\n"
HASH /usr/local/go/src/runtime/internal/sys/arch_amd64.go: fadc2d5c6b829c35f310447c02396703879b092febc190c5c84f8422ee74af1f
HASH[build runtime/internal/sys]: "file arch_amd64.go -twtXGuCnDXzEER8Ajln\n"
HASH /usr/local/go/src/runtime/internal/sys/intrinsics.go: a3850aaa0f87059d181640ae9024e1762d073a195c2004b5a76c69f09fb2d0a5
HASH[build runtime/internal/sys]: "file intrinsics.go o4UKqg-HBZ0YFkCukCTh\n"
HASH /usr/local/go/src/runtime/internal/sys/intrinsics_common.go: b009ab2f510e179af3cd495b904b9f9ccc0484e9e6afd163199b587bfbc87741
HASH[build runtime/internal/sys]: "file intrinsics_common.go sAmrL1EOF5rzzUlbkEuf\n"
HASH /usr/local/go/src/runtime/internal/sys/sys.go: 55e021891200a7e6a5c371c8a1ab71b6c15aeb16ea6c1b192185d17df8c8b18f
HASH[build runtime/internal/sys]: "file sys.go VeAhiRIAp-alw3HIoatx\n"
HASH /usr/local/go/src/runtime/internal/sys/zgoarch_amd64.go: c6755168cd4207e7e370beb4e0512da5eba2bcb31427fddb67c7042a24bc61d4
HASH[build runtime/internal/sys]: "file zgoarch_amd64.go xnVRaM1CB-fjcL604FEt\n"
HASH /usr/local/go/src/runtime/internal/sys/zgoos_darwin.go: a949f3e3bd5cc5ab2a675d7a8b8d07e6f81053651381e73e8e8ba9c7da68e544
HASH[build runtime/internal/sys]: "file zgoos_darwin.go qUnz471cxasqZ116i40H\n"
HASH /usr/local/go/src/runtime/internal/sys/zversion.go: 8816974d6529db506967989004b9024d127f5d3346d5ac08e6975e3edcc73e05
HASH[build runtime/internal/sys]: "file zversion.go iBaXTWUp21BpZ5iQBLkC\n"
HASH[build runtime/internal/sys]: 12b36f00b92fcc494fa6012aff6960c943ef427e0822e24217cc6893d1916d7a
runtime/internal/sys true
go tool dist: unexpected stale targets reported by /usr/local/go/bin/go list -gcflags="" -ldflags="" for [std cmd]:
STALE archive/tar: stale dependency: internal/cpu
STALE archive/zip: stale dependency: internal/cpu
STALE bufio: stale dependency: internal/cpu
STALE bytes: stale dependency: internal/cpu
STALE compress/bzip2: stale dependency: internal/cpu
STALE compress/flate: stale dependency: internal/cpu
STALE compress/gzip: stale dependency: internal/cpu
STALE compress/lzw: stale dependency: internal/cpu
STALE compress/zlib: stale dependency: internal/cpu
STALE container/heap: stale dependency: internal/cpu
STALE container/list: build ID mismatch
STALE container/ring: build ID mismatch
STALE context: stale dependency: internal/cpu
STALE crypto: stale dependency: internal/cpu
# same error
.
.
.---
not repro:
$ GOROOT_BOOTSTRAP=~/sdk/go1.16.3 GOEXPERIMENT='' ./make.bash -a -d -v -no-banner
---
I'll create issue thread later.
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Koichi Shiraishi (Gerrit)
Apr 8, 2021, 4:06:25 AM4/8/21
to Austin Clements, goph...@pubsubhelper.golang.org, Go Bot, Matthew Dempsky, golang-co...@googlegroups.com
Patchset:
Ah, yes use GOEXPERIMENT. […]
forgot.
occur both of `regabi` and `staticlockranking`.
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Apr 9, 2021, 1:40:18 PM4/9/21
to Austin Clements, goph...@pubsubhelper.golang.org, Koichi Shiraishi, Go Bot, Matthew Dempsky, golang-co...@googlegroups.com
1 comment:
Patchset:
forgot. […]
Thanks. This should be fixed as of CL 308591.
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Koichi Shiraishi (Gerrit)
Apr 10, 2021, 11:52:10 AM4/10/21
to Austin Clements, goph...@pubsubhelper.golang.org, Go Bot, Matthew Dempsky, golang-co...@googlegroups.com
1 comment:
Patchset:
Thanks. This should be fixed as of CL 308591.
thanks. confirmed fix it.
To view, visit change 307821. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages