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 main.version" when version has a value and is later used in a global struct literal doesn't work #34675

Closed
segevfiner opened this issue Oct 3, 2019 · 4 comments

Comments

@segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Oct 3, 2019

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

$ go version
go version go1.13.1 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="<snip>/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/segev/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go@1.12/1.12.9/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go@1.12/1.12.9/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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/b_/c6yh0ksn63d1yy192x2p9f4c0000gn/T/go-build796234501=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Steps

  1. Save main.go, go build and run it.
  2. You get "Version: dev"
  3. Run go build -ldflags "-X main.version=foo"
  4. You get "Version: dev", instead of "Version: foo".

If you change var version = "dev" to var version string, it works. It also works if you just fmt.Println("Version:", version) without going through the struct.

The documentation says:

	This is only effective if the variable is declared in the source code either uninitialized
	or initialized to a constant string expression. -X will not work if the initializer makes
	a function call or refers to other variables.

Which doesn't suggest to me that this should somehow not work.

This will be a common scenario when using https://github.com/spf13/cobra which uses a global struct like that with a Version field, and is where I encountered this originally.

main.go

package main

import "fmt"

var version = "dev"

type cmd struct {
	Version string
}

var rootCmd = cmd{
	Version: version,
}

func main() {
	fmt.Println("Version:", rootCmd.Version)
}

What did you expect to see?

"Version: foo" when running go build -ldflags "-X main.version=foo".

What did you see instead?

"Version: dev" when running go build -ldflags "-X main.version=foo".

@andybons andybons changed the title "-X main.version" when version has a value and is later used in a global struct literal doesn't work cmd/link: "-X main.version" when version has a value and is later used in a global struct literal doesn't work Oct 3, 2019
@andybons

This comment has been minimized.

Copy link
Member

@andybons andybons commented Oct 3, 2019

@andybons andybons added this to the Unplanned milestone Oct 3, 2019
@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 3, 2019

@mdempsky

This comment has been minimized.

Copy link
Member

@mdempsky mdempsky commented Oct 3, 2019

The issue is that for something like:

var A = "foo"
var B = A

the compiler recognizes that A is statically initialized to a constant and B is statically initialized to a copy of A, so it's compiled as:

var A = "foo"
var B = "foo"    // optimized by cmd/compile

Then later when we link with -X mypkg.A=bar, it only changes it to:

var A = "bar"    // modified by cmd/link -X
var B = "foo"

because the linker doesn't know it needs to modify B as well.

I see at least a few options here:

  1. Change cmd/link's documentation to only guarantee -X works for uninitialized string variables. cmd/compile already pessimistically optimizes references to uninitialized package-scope variables, assuming that they might be initialized externally (e.g., assembly code or the linker).

  2. Change cmd/compile to not apply the optimization mentioned above to string-typed variables. This might produce slightly worse code in some cases, but I think should be limited to emitting slightly less efficient program initialization code (i.e., we need to dynamically initialized some variables instead of statically initialize them).

  3. Change cmd/compile to track how string-typed variables are used and where the values get copied so that cmd/link can patch up everywhere it's copied to. I'm worried this will be a lot of complexity for little value.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 3, 2019

Change https://golang.org/cl/198657 mentions this issue: cmd/compile: don't statically copy string-typed variables

@gopherbot gopherbot closed this in c33d45a Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.