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

cmd/link: -X does not work on variables with non-constant default values #37369

Open
jayconrod opened this issue Feb 21, 2020 · 5 comments
Open

cmd/link: -X does not work on variables with non-constant default values #37369

jayconrod opened this issue Feb 21, 2020 · 5 comments
Milestone

Comments

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Feb 21, 2020

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

$ go version
go version go1.14rc1 darwin/amd64

Does this issue reproduce with the latest release?

This is a regression in Go 1.14rc1. It does not reproduce in 1.13.8.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jayconrod/Library/Caches/go-build"
GOENV="/Users/jayconrod/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/jayconrod/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/go/installed"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/go/installed/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/jayconrod/Code/test/tmp2/go.mod"
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=/var/folders/rq/x0692kqj6ml8cvrhcqh5bswc008xj1/T/go-build343063917=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Run the testscript below:

go run -ldflags='-X=example.com/test/stamp.STAMP=pass' ./main
stdout pass

-- go.mod --
module example.com/test

go 1.14
-- main/main.go --
package main

import (
	"fmt"

	"example.com/test/stamp"
)

func main() {
	fmt.Printf("STAMP = %s\n", stamp.STAMP)
}
-- stamp/stamp.go --
package stamp

var DEFAULT = "fail"
var STAMP = DEFAULT

What did you expect to see?

The program should print STAMP = pass, and the test should pass.

What did you see instead?

The program prints STAMP = fail.

This only happens when STAMP is assigned to another variable. The test passes if STAMP is assigned the value "fail" instead of DEFAULT.

Other thoughts

I've milestoned this Go1.14 and added the Soon label because this is a regression. It should be triaged before the 1.14 release.

I don't know if this was intended to work before. If not, feel free to close.

If it was intended to work, I'm not sure whether this should block 1.14.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Feb 21, 2020

Related to #34675. It looks to me that this is the opposite.

We used to forward the static initializer of DEFAULT to STAMP at compile time. But then if a -X flag is set on DEFAULT, it won't affect STAMP as STAMP is statically initialized with the old value. For this reason (i.e. #34675), we stopped forwarding the static initializer. Now STAMP is assigned dynamically (to the value of DEFAULT at that time), so it is not affected by link time modification.

This or #34675, it seems to me that we can only choose one, at least without a much more complex logic. var x = f() is already not affected by the -X flag, as we have to do the assignment dynamically. It looks to me that it is consistent to treat var x = y also as a dynamic assignment.

@jayconrod

This comment has been minimized.

Copy link
Contributor Author

@jayconrod jayconrod commented Feb 21, 2020

Perhaps this is a separate feature request, but would it make sense for the linker to emit an error when a -X flag can't be applied? I don't have a strong opinion about the semantics either way; I'd expect -X is normally used on variables that are unassigned or are statically initialized. But I'd rather have a loud failure when I use -X incorrectly.

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Feb 21, 2020

This is a bit harder. The linker has to understand it is initialized dynamically. I don't think we want the linker to walk the init functions to find out this.
Maybe the compiler could emit a marker when a string variable is initialized dynamically, for the linker to check.

@toothrot toothrot modified the milestones: Go1.14, Go1.15 Feb 25, 2020
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Mar 4, 2020

I've milestoned this Go1.14 and added the Soon label because this is a regression. It should be triaged before the 1.14 release.

For future reference, it's better to use the release-blocker label in order to ensure an issue is looked at before a release is made. Issues that are labeled Soon may be missed.

The Go 1.14 release has already been made, so I'll remove Soon label, because I understand this is no longer time-sensitive. Please update the issue if that isn't the case.

@dmitshur dmitshur removed the Soon label Mar 4, 2020
@jayconrod

This comment has been minimized.

Copy link
Contributor Author

@jayconrod jayconrod commented Mar 4, 2020

This can probably go to Backlog. I was initially worried this was an unintentional regression, but it's the result of #34675.

@jayconrod jayconrod modified the milestones: Go1.15, Backlog Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.