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

build: move GOEXPERIMENT knob from make.bash to cmd/go #42681

Closed
mdempsky opened this issue Nov 17, 2020 · 16 comments
Closed

build: move GOEXPERIMENT knob from make.bash to cmd/go #42681

mdempsky opened this issue Nov 17, 2020 · 16 comments

Comments

@mdempsky
Copy link
Member

GOEXPERIMENT currently allows trying out a handful of different experimental toolchain features, none of which need to be decided at make.bash time. We can just make the rest of the toolchain GOEXPERIMENT-aware. (If you really want the toolchain itself to be built with an experiment enabled, "GOEXPERIMENT=foo go install -a cmd" would work.)

@bcmills
Copy link
Contributor

bcmills commented Nov 18, 2020

How many of the GOEXPERIMENT values could instead be -gcflags or build constraints, both of which cmd/go already understands?

I'd rather we reduce than add to the number of ways to vary the build configuration.

@mdempsky
Copy link
Member Author

mdempsky commented Nov 18, 2020

I don't feel strongly about whether GOEXPERIMENT should be the knob used by cmd/compile and other tools. I think it would be a simpler incremental step to continue using that, but I'm fine switching to normal flags too.

I do think cmd/go should probably be in charge of orchestrating everything still somehow. I've seen users too frequently use -gcflags=foo instead of -gcflags=all=foo; use just -gcflags without realizing they need to pass linker flags too; etc.

@gopherbot
Copy link

Change https://golang.org/cl/271217 mentions this issue: cmd/compile: fix panic in field tracking logic

gopherbot pushed a commit that referenced this issue Nov 18, 2020
Within the frontend, we generally don't guarantee uniqueness of
anonymous types. For example, each struct type literal gets
represented by its own types.Type instance.

However, the field tracking code was using the struct type as a map
key. This broke in golang.org/cl/256457, because that CL started
changing the inlined parameter variables from using the types.Type of
the declared parameter to that of the call site argument. These are
always identical types (e.g., types.Identical would report true), but
they can be different pointer values, causing the map lookup to fail.

The easiest fix is to simply get rid of the map and instead use
Node.Opt for tracking the types.Field. To mitigate against more latent
field tracking failures (e.g., if any other code were to start trying
to use Opt on ODOT/ODOTPTR fields), we store this field
unconditionally. I also expect having the types.Field will be useful
to other frontend code in the future.

Finally, to make it easier to test field tracking without having to
run make.bash with GOEXPERIMENT=fieldtrack, this commit adds a
-d=fieldtrack flag as an alternative way to enable field tracking
within the compiler. See also #42681.

Fixes #42686.

Change-Id: I6923d206d5e2cab1e6798cba36cae96c1eeaea55
Reviewed-on: https://go-review.googlesource.com/c/go/+/271217
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
@bcmills
Copy link
Contributor

bcmills commented Nov 19, 2020

I think this kind of interacts with the various proposals for build configurations (#39005, #42343, and perhaps others; CC @mvdan @dominikh @jayconrod @matloob).

In particular, you can think of a GOEXPERIMENT value as essentially just a predefined shorthand for a set of build flags. If we add some mechanism for users to define their own shorthand for sets of build flags, we should try to get GOEXPERIMENT to converge (or at least harmonize) with that.

@gopherbot
Copy link

Change https://golang.org/cl/280113 mentions this issue: [dev.regabi] cmd: move GOEXPERIMENT knob from make.bash to cmd/go

@mdempsky
Copy link
Member Author

CL 280113 demonstrates what I had in mind for this. I've tested and confirmed it works for fieldtrack. I don't see any reason it shouldn't work for others.

I think there are some rough edges still (e.g., I suspect GOENV doesn't work to set GOEXPERIMENT, and setting GOEXPERIMENT should probably update the package suffix), but I'll clean those up if this is a worthwhile direction to pursue. I think it is.

/cc @rsc @aclements @cherrymui @thanm

@aclements
Copy link
Member

We're definitely running into this with the register ABI work. We currently use GOEXPERIMENT as a way to control the compiler, assembler, and various runtime packages all together, but that only has to be done on a whole-build basis, not a whole make.bash-basis. And the current behavior of GOEXPERIMENT means that we can't easily build a working tool chain and then do individual, more targeted builds with the option enabled.

We don't necessarily need GOEXPERIMENT to change, but some coherent whole-build option would make this much easier. We already have some things that work like this, like go build's -race option, but not anything that's easily extensible like GOEXPERIMENT.

@aclements
Copy link
Member

Here's a variation on @mdempsky 's proposal that would be backwards compatible while satisfying our needs as I understand them:

Add a flag to go build, say, -goexperiment, that allows for setting the experiments in a single build. These always apply to the whole build, and would be plumbed into the compiler, assembler, and runtime by the go tool (using the same conventions GOEXPERIMENT uses today). The value of GOEXPERIMENT at make.bash time would set the default value of this flag; hence this would behave exactly as today if you don't pass -goexperiment to the go tool. (Alternatively, GOEXPERIMENT at go build time could override the make.bash setting, but I think giving it meaning both for make.bash and go build is potentially confusing and also I like fewer environment variables.)

If we eventually have some broader build configuration mechanism, I'd be more than happy to switch to that, but I think this is something we could do today (@mdempsky already did most of the work). And since this is meant for tool chain development, it's something we could back out or replace with another mechanism later.

@aclements
Copy link
Member

aclements commented Feb 24, 2021

I hadn't appreciated that @mdempsky 's proposal was that the GOEXPERIMENT set at make.bash time becomes the default, but can be overridden at go build time by setting GOEXPERIMENT then (so it is mostly backwards compatible).

A bunch of us from the runtime/compiler team chatted about this today and everybody seems to think this is a good idea. There wasn't strong consensus on whether the go build override should be the GOEXPERIMENT environment variable or a flag, but people generally favored the environment variable because environment variables tend to more obviously flow to the whole process.

@rsc
Copy link
Contributor

rsc commented Mar 10, 2021

It looks like the four active experiments, from cmd/internal/objabi/util.go, are:

{"fieldtrack", &Fieldtrack_enabled},
{"preemptibleloops", &Preemptibleloops_enabled},
{"staticlockranking", &Staticlockranking_enabled},
{"regabi", &Regabi_enabled},

preemptibleloops sounds like it is dead code that should be removed.
fieldtrack is Google-internal but not a big deal for others to have easier access to.

staticlockranking looks like it could just as easily be a build tag so it's fine to keep as an experiment instead.

So really this looks like it is about exposing regabi, which seems fine as well.

I wondered a bit what the distinction between this and GODEBUG is. GODEBUG is purely runtime, I suppose.
But there are definitely some things we control with GODEBUG that feel like the above (GODEBUG=asyncpreemptoff=1 vs GOEXPERIMENT=preemptibleloops for example).

Overall seems reasonable. I think this is something we all working on the go command and compiler need to agree on, but not something that needs community buy-in. This is an internal detail of how Go is built, more than anything. And we all seem to agree.

Marking accepted.

@rsc rsc changed the title proposal: move GOEXPERIMENT knob from make.bash to cmd/go proposal: build: move GOEXPERIMENT knob from make.bash to cmd/go Mar 10, 2021
@rsc rsc moved this from Incoming to Accepted in Proposals (old) Mar 10, 2021
@rsc
Copy link
Contributor

rsc commented Mar 10, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: build: move GOEXPERIMENT knob from make.bash to cmd/go build: move GOEXPERIMENT knob from make.bash to cmd/go Mar 10, 2021
@rsc rsc modified the milestones: Proposal, Backlog Mar 10, 2021
@gopherbot
Copy link

Change https://golang.org/cl/300991 mentions this issue: cmd: move GOEXPERIMENT knob from make.bash to cmd/go

@gopherbot
Copy link

Change https://golang.org/cl/302969 mentions this issue: test: switch fieldtrack test to use GOEXPERIMENT

gopherbot pushed a commit that referenced this issue Mar 18, 2021
Now that we can set GOEXPERIMENT at build time, we no longer need
-d=fieldtrack in the compiler to enabled field tracking at build time.
Switch the one test that uses -d=fieldtrack to use GOEXPERIMENT
instead so we can eliminate this debug flag and centralize on
GOEXPERIMENT.

Updates #42681.

Change-Id: I14c352c9a97187b9c5ec8027ff672d685f22f543
Reviewed-on: https://go-review.googlesource.com/c/go/+/302969
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@rsc
Copy link
Contributor

rsc commented Mar 24, 2021

This seems to have been committed before being accepted. Please hold off in the future.

@aclements
Copy link
Member

@rsc, I'm not sure what you mean. It looks to me like it was accepted on March 10th and committed on March 11th.

@rsc
Copy link
Contributor

rsc commented Mar 25, 2021

@aclements, indeed, my bad. Many apologies!

[Looking back I see what I did wrong during the meeting - I did something slightly different from my normal routine and ended up opening this issue in the middle of a batch of "likely accept" tabs. When I got to it, I completely missed that it was out of place and read it as a "likely accept". Entirely my fault. Then I placed it on the "accept" list for the minutes bot, and the bot correctly found no state updates to apply, although it did dutifully include the entry in the minutes. So that's why this issue is listed as accepted again this week in the minutes. Again, my bad. Sorry!]

@golang golang locked and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

5 participants