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.

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

internal/buildcfg: regabireflect breaks GOENV configuration loading #46815

Closed
tonistiigi opened this issue Jun 18, 2021 · 10 comments
Closed

internal/buildcfg: regabireflect breaks GOENV configuration loading #46815

tonistiigi opened this issue Jun 18, 2021 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.17

Comments

@tonistiigi
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version go1.17beta1 linux/amd64

go version devel go1.17-45f251ad6c Thu Jun 17 21:58:54 2021 +0000 linux/amd64

Does this issue reproduce with the latest release?

Only with 1.17. The regression point is at 8865548

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17beta1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build638292537=/tmp/go-build -gno-record-gcc-switches"

What did you do?

echo GOARCH=arm64 > myenv

cat > t.go <<eof
package main
func main(){}
eof

GOENV=myenv go build t.go

What did you expect to see?

Successful build

What did you see instead?

# GOENV=myenv go build t.go
# internal/abi
/usr/local/go/src/internal/abi/abi.go:19:10: undefined: IntArgRegs
/usr/local/go/src/internal/abi/abi.go:20:10: undefined: FloatArgRegs
/usr/local/go/src/internal/abi/abi.go:27:8: undefined: IntArgRegs
/usr/local/go/src/internal/abi/abi.go:38:24: undefined: IntArgRegs

The issue seems to be that

go/src/internal/buildcfg/cfg.go

Lines 24 to 27 in 45f251a

GOROOT = envOr("GOROOT", defaultGOROOT)
GOARCH = envOr("GOARCH", defaultGOARCH)
GOOS = envOr("GOOS", defaultGOOS)
GO386 = envOr("GO386", defaultGO386)
only reads environment variables directly and doesn't use the config loading cmd/go/internal/cfg. The same issue appears with changes to the default config file.

The issue does not appear if I disable regabireflect or if export GOARCH=arm64

GOENV=myenv GOEXPERIMENT=noregabiargs,noregabireflect go build t.go
@tonistiigi tonistiigi mentioned this issue Jun 18, 2021
Merged
@bcmills bcmills added this to the Go1.17 milestone Jun 18, 2021
@bcmills bcmills added release-blocker NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 18, 2021
@bcmills
Copy link
Contributor

bcmills commented Jun 18, 2021

CC @aclements @cherrymui @mknyszek

@bcmills bcmills changed the title [1.17beta1] buildcfg: regabireflect breaks GOENV configuration loading internal/buildcfg: regabireflect breaks GOENV configuration loading Jun 18, 2021
@mknyszek
Copy link
Contributor

If I'm understanding this correctly, is the problem that regabireflect appears to be on for GOARCH=arm64? That would explain the error message, to me anyway.

Looking at this more closely... is the issue just that GOEXPERIMENT isn't brought in from internal/buildcfg and used in cmd/go/internal/cfg? If so, that seems like an easy fix.

I'll try that.

@tonistiigi
Copy link
Contributor Author

@mknyszek I'm not familiar with this code but it seemed to me that the new regabireflect still thought it was building for GOARCH=amd64 (host default), while other packages picked up the correct arm64 value from GOENV. If GOARCH=arm64 is set directly in the environment then everything seems to work as internal/buildcfg and cmd/go/internal/cfg now both see arch as arm64.

@mknyszek
Copy link
Contributor

@tonistiigi Ah OK, I think I understand. The issue (as you're describing) is that GOEXPERIMENT parsing is happening too early, using GOARCH inferred from the environment.

That seems... a little more complicated to fix (and yeah just doing what I did doesn't seem to fix it -- GOEXPERIMENT has already been calculated).

@mknyszek
Copy link
Contributor

@mdempsky might be able to help a little with this? He reviewed some of the changes to GOEXPERIMENT that happened this release.

@mdempsky
Copy link
Contributor

The existing division of responsibilities here is still messy. I'll take a look at how to improve it.

@mdempsky mdempsky self-assigned this Jun 18, 2021
@mdempsky
Copy link
Contributor

I think the cleanest way to fix this is to move the GOENV logic into internal/buildcfg, so that all of the environment lookup and fallback logic is shared across tools. I have a CL that does this and fixes this issue, and I'm currently working on separating apart the big "move code between files" changes the much more modest actual changes.

Does have concerns with:

  1. Adding GOENV support to the Go tools (i.e., asm, cgo, compile, link)?
    • Note: GOENV only gets processed when accessing an environment variable that's not already explicitly set, and cmd/go explicitly sets all GO* environment variables for tools even when invoked like go tool compile. So this would only affects users that bypass cmd/go to execute the underlying tool (e.g., using go tool -n compile).
  2. Adding GOENV support to the go/build package (e.g., build.Default.GOARCH could come from GOENV too, not just GOARCH or runtime.GOARCH)?

These would be natural consequences of eliminating the code duplication here, and I think would be nice for consistency.

But if anyone thinks these aren't desirable or at least they shouldn't change this late in the release cycle, I can keep the existing behavior for either or both of those cases. I suspect 1 is uncontentious, but that folks might be more hesitant about 2.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/329655 mentions this issue: internal/buildcfg,go/build: enable GOENV for all tools

@bcmills
Copy link
Contributor

bcmills commented Jun 21, 2021

IMO, go/build should not read from the environment by default. It hasn't in the past, so starting to do so now could be a breaking change. It would be fine to add a Context method to explicitly configure from GOENV, I suppose, but that should probably go through the proposal process because it's not obvious to me where the best split-point is between GOENV and go/build.

I'm not sure whether go tool compile et all should read from them either. I think most users either invoke those tools indirectly via cmd/go, or indirectly via other build systems (e.g. bazel). GOENV support seems redundant for the former and perhaps confusing or harmful for the latter. So I would say probably GOENV should just be a cmd/go thing.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/329930 mentions this issue: cmd/go: update ToolTags based on GOARCH value

@mdempsky mdempsky mentioned this issue Jun 25, 2021
Closed
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 18, 2022
@golang golang locked and limited conversation to collaborators Jun 23, 2023
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. release-blocker
Projects
None yet
Milestone
Go1.17
Development

No branches or pull requests
6 participants
@mdempsky @tonistiigi @mknyszek @dmitshur @bcmills @gopherbot