Skip to content

cmd/compile: strange use of GOEXPERIMENT #22223

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.

Sign up for GitHub

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

Jump to bottom
Closed
rsc opened this issue Oct 12, 2017 · 7 comments
Closed

cmd/compile: strange use of GOEXPERIMENT #22223

rsc opened this issue Oct 12, 2017 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@rsc
Copy link
Contributor

rsc commented Oct 12, 2017

cmd/compile/internal/gc/main.go looks at $GOEXPERIMENT and if it's non-empty disables concurrent compilation.
I can't tell why. GOEXPERIMENT is only meaningful during make.bash; after that the experiment set is baked into the toolchain and not reloaded from the environment.

I suspect this use of $GOEXPERIMENT should just be removed.

/cc @josharian @mdempsky
@rsc rsc added this to the Go1.10 milestone Oct 12, 2017
@mdempsky
Copy link
Contributor

It looks like there's a similar check in cmd/go/internal/work/build.go too.

I think the intent was that @josharian wasn't confident the non-standard code paths enabled by experiments were safe for concurrency.

Agreed the checks do not function as intended.

@griesemer griesemer self-assigned this Nov 29, 2017
@griesemer
Copy link
Contributor

If trivial, do for 1.10. Otherwise move to 1.11.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/80760 mentions this issue: cmd/compile: fix GOEXPERIMENT checks

@bradfitz
Copy link
Contributor

bradfitz commented Dec 4, 2017

Reopening this, because GOEXPERIMENT is now broken since that CL.

I left comments on https://go-review.googlesource.com/c/go/+/80760 with details.

/cc @rsc

@bradfitz bradfitz reopened this Dec 4, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 4, 2017
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/81776 mentions this issue: cmd/go: disable concurrent compilation under GOEXPERIMENTs

gopherbot pushed a commit that referenced this issue Dec 4, 2017
@mdempsky
cmd/go: disable concurrent compilation under GOEXPERIMENTs
509ffb9 
Duplicate cmd/compile check into cmd/go. Manually tested that
"GOEXPERIMENT=fieldtrack make.bash" passes now.

Updates #22223.

Change-Id: I441970a8a5ad4aadf5bd4fbd4d6cc71847b43308
Reviewed-on: https://go-review.googlesource.com/81776
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.11 Dec 6, 2017
@ianlancetaylor
Copy link
Member

As far as I can tell this is a valid issue, but there is no trivial fix, and things are working today, so rolling this forward to 1.11.

@mdempsky
Copy link
Contributor

mdempsky commented Dec 6, 2017

The original issue (that cmd/compile was erroneously checking the GOEXPERIMENT environment variable) is fixed, so I think this can be closed again. (I saw @bradfitz's comment on the CL, but didn't notice he reopened this issue.)

@mdempsky mdempsky closed this as completed Dec 6, 2017
@golang golang locked and limited conversation to collaborators Dec 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
6 participants
@bradfitz @mdempsky @rsc @ianlancetaylor @griesemer @gopherbot