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

Jump to bottom

cmd/go: GODEBUGCPU does not affect caching #25659

Closed
josharian opened this issue May 31, 2018 · 6 comments
Closed

cmd/go: GODEBUGCPU does not affect caching #25659

josharian opened this issue May 31, 2018 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.11

Comments

@josharian
Copy link
Contributor

https://golang.org/cl/91737 added a new environment variable that controls compiler output. However, cmd/go does not know about it. It should probably either migrate to a command line flag or be added to the list at

go/src/cmd/go/internal/work/exec.go

Line 226 in 30b6bc3

// TODO(rsc): Convince compiler team not to add more magic environment variables,
.

(Aside: Why does this need to be protected by a GOEXPERIMENT build flag? Not obvious to me. I thought we were trying to move away from those.)

cc @martisch @bradfitz
@josharian josharian added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 31, 2018
@josharian josharian added this to the Go1.11 milestone May 31, 2018
@bradfitz
Copy link
Contributor

Moving to Go 1.12. It's explicitly behind a GOEXPERIMENT flag because this isn't for users (yet?). It's for the build system to increase CPU coverage.

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 May 31, 2018
@martisch
Copy link
Contributor

martisch commented May 31, 2018 •
edited

@josharian: GODEBUGCPU itself should not effect compiler output as internal/cpu AFAIK is not used by the compiler to generate different code (the experiment flag does). Can you clarify that or how GODEBUGCPU (not GOEXPERIMENT) leads to different compiler output not just different runtime behaviour?

Its experimental as it was started before but committed into the freeze and is for the go build infrastructure and devs to test around in go1.11 without committing to e.g. syntax or flag vs environment variable.

For go1.11 not all features have options yet, not all runtime cpu feature variables have been migrated to internal/cpu and only Darwin and Linux are supported currently. Most of the these can be resolved for go1.12.

For go1.11 I think adding GOEXPERIMENT? to the list of magic env variables is the easier fix for caching than making it a flag.

For go1.12: I would be happy if we can remove the experiment protection and will write an issue to get agreement that this can be made permanent (possibly as a flag).

@martisch martisch self-assigned this May 31, 2018
@martisch martisch added the NeedsFix The path to resolution is known, but the work has not been done. label May 31, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 31, 2018
@martisch martisch modified the milestones: Go1.12, Go1.11 May 31, 2018
@martisch martisch added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 31, 2018
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label May 31, 2018
@martisch
Copy link
Contributor

I guess we should add GOEXPERIMENT to the build cache invalidation magic but not GODEBUGCPU:

smoke test with sha1sum ../bin/go for:

go clean -cache
GOEXPERIMENT=debugcpu ./make.bash
e0896e5dd8578cd817ab84979c310da86309ea3f  ../bin/go

go clean -cache
GOEXPERIMENT=debugcpu GODEBUGCPU=all=0 ./make.bash 
e0896e5dd8578cd817ab84979c310da86309ea3f  ../bin/go

./make.bash
04c3ef214c75d496dce67020d501dd0626a9ca74  ../bin/go

@martisch
Copy link
Contributor

running
go cache -clean
./all.bash
GOEXPERIMENT=debugcpu GODEBUG=gocacheverify=1 ./all.bash
triggers an error:

panic: go: internal cache error: cache verify failed: id=fb2313f2883b218bc066fd82b898c2e516581ef1d98d89306189fa0ae182d009 changed:<<<
compile
goos linux goarch amd64
import "cmd/cover"
omitdebug false standard true local false prefix ""
compile hN7U-hIzcC216X9D7jJ5 [] []
GO$GOARCH=
file cover.go -HJGpdKieEMMOCO-mPxL
file doc.go q8cjgpxRyr-B4c0-BSxm
file func.go XsiVkomYxidS7PwWXQ2n
file html.go 4Y1BIbPC6AVLRyO7P_1x
file profile.go 0v22dDl9QjcWlQUwc8ke
import bufio do57MGx2Y1ny-iTSZ8xm
import bytes hzsbwNhfBvX1MeqZuX_J
import cmd/internal/browser lB6CNWIvbAQhCJ87k4lE
import cmd/internal/edit Qi-YqEtW43LLmQRtn89f
import cmd/internal/objabi pM_kmeN0dsgOwIXnNIgo
import flag 0857hOxkwQ8UOUUhlxcR
import fmt pRrKIvjNZsLODwerYD22
import go/ast oT3qx9K-aLjOT_fzqq7i
import go/build YizkSjO0wdaq5FTNZf8Z
import go/parser tr0t3-B29FTNt99c_dgU
import go/token sD6jKS2szZrhwGYNJ7oL
import html/template ViNWlmNmRpv3WreAsU4V
import io ZV1onBPpy8p0twc7gW-W
import io/ioutil A6-6ihjVqFezyIyf8BSv
import log UnhGGF8ENdh01rORi6LM
import math NObQaiwNkm25aQ7ufbjg
import os MG0j3jpTqpfUHwK7Yev0
import path/filepath 1KlWzTqUkbxUippA0PjX
import regexp PY16zyZDtBOWVLfe1Cvf
import sort _LXX0vnnL-jngm1mYtYW
import strconv BGeyFISqHJ06yQc-FtDT
import strings UCe-z-5kkJNhLZ8_Tcu0
import text/tabwriter auwReVp8BRX0TAoxw0wB
import runtime XvdKklQdiVRyO4a8YuxR

>>>
old: 0261fce9b7f23ef9875b3a343eaaa896dddca566af80d58ac9b62fe37c620d14 217732
new: bd20f251999620804f182e43e18c7b9c5e1d402683fdbf5b13cebe27823e198c 217718

@martisch martisch changed the title cmd/go: GODEBUGCPU does not affect caching cmd/go: GOEXPERIMENT does not affect caching May 31, 2018
@martisch
Copy link
Contributor

actually the GOEXPERIMENT=debugcpu change is included in the compile buildID:

../bin/go tool compile -V=full
compile version devel +fffb3a5c20 Thu May 31 07:20:38 2018 +0000 buildID=x3ITGgvcvyYJ_LgYxLW-/4TMLSzuIc14CGpo_sKdY/yFu2yIDH6xKiavoqdEVl/UMK0LpWBgLKUUJI3ZA7s

GOEXPERIMENT=debugcpu ./make.bash
../bin/go tool compile -V=full
compile version devel +fffb3a5c20 Thu May 31 07:20:38 2018 +0000 X:framepointer,debugcpu buildID=PBWLlIi2-VanQZimHm8B/UmgAcoGA4iYbgsZme20i/Qo34Q7TaurQoNlVRUeNU/hN7U-hIzcC216X9D7jJ5

which is taken into account here:

go/src/cmd/go/internal/work/exec.go

Line 220 in 30b6bc3

fmt.Fprintf(h, "compile %s %q %q\n", b.toolID("compile"), forcedGcflags, p.Internal.Gcflags)

go cache -clean
GODEBUG=gocacheverify=1 ./all.bash
without GOEXPERIMENT seems to always fail on two different machines. Will file an extra bug for that.

Closing this bug.

@martisch martisch changed the title cmd/go: GOEXPERIMENT does not affect caching cmd/go: GODEBUGCPU does not affect caching May 31, 2018
@josharian
Copy link
Contributor Author

Thanks, and sorry for the noise.

@golang golang locked and limited conversation to collaborators May 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
4 participants
@bradfitz @josharian @martisch @gopherbot