-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
Comments
How many of the I'd rather we reduce than add to the number of ways to vary the build configuration. |
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 |
Change https://golang.org/cl/271217 mentions this issue: |
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>
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 |
Change https://golang.org/cl/280113 mentions this issue: |
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 |
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 |
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 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. |
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. |
It looks like the four active experiments, from cmd/internal/objabi/util.go, are:
preemptibleloops sounds like it is dead code that should be removed. 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. 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. |
No change in consensus, so accepted. 🎉 |
Change https://golang.org/cl/300991 mentions this issue: |
Change https://golang.org/cl/302969 mentions this issue: |
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>
This seems to have been committed before being accepted. Please hold off in the future. |
@rsc, I'm not sure what you mean. It looks to me like it was accepted on March 10th and committed on March 11th. |
@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!] |
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.)
The text was updated successfully, but these errors were encountered: