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

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

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

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

jayconrod opened this issue Feb 21, 2020 · 7 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@jayconrod
Copy link
Contributor

jayconrod commented Feb 21, 2020 •
edited
Loading

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.
@jayconrod jayconrod added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) labels Feb 21, 2020
@jayconrod jayconrod added this to the Go1.14 milestone Feb 21, 2020
@jayconrod jayconrod mentioned this issue Feb 21, 2020
Closed
@cherrymui
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor

dmitshur commented Mar 4, 2020 •
edited
Loading

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 This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) label Mar 4, 2020
@jayconrod
Copy link
Contributor Author

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
@harshavardhana
Copy link
Contributor

I was able to reproduce this as well this caused regression for binaries which relied on -X to add versions, what seems to be new approach it is not clear?

@nitisht nitisht mentioned this issue Jun 4, 2020
Merged
@ianlancetaylor
Copy link
Member

@harshavardhana Until this issue is fixed, if you need to set a variable with -X, either initialize it with a constant or don't initialize it at all.

dvaldivia added a commit to minio/operator that referenced this issue Jun 4, 2020
@nitisht @dvaldivia @harshavardhana
Fixing multiple issues (#137)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
b5da0e2 
- Remove the usage of Selector in minio, kes & mcs. Selector is
supposed to be used internally for filtering pods, so there is no
need to use selectors.

- Update docker image tags for minio, kes & mcs.

- Add access privileges as required for certificates and secrets.

- Use go 1.13 till golang/go#37369 is
addressed.

- Fix openAPIV3Schema for mcs and add kes validation. (fixes #125)

Co-authored-by: Daniel Valdivia <hola@danielvaldivia.com>
Co-authored-by: Harshavardhana <harsha@minio.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
6 participants
@toothrot @jayconrod @harshavardhana @dmitshur @ianlancetaylor @cherrymui