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/compile: initialization optimization fails when using -ldflags=-X=VAL #28969

Open
tjamet opened this Issue Nov 27, 2018 · 10 comments

Comments

Projects
None yet
6 participants
@tjamet

tjamet commented Nov 27, 2018

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

$ go version
go version go1.11.1 darwin/amd64

Does this issue reproduce with the latest release?

yes, tested with go version go1.11.2 darwin/amd64

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/thibault.jamet/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/thibault.jamet/dev/"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/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/z3/r2mc8fw12vb83952716284n00000gq/T/go-build606665018=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I narrowed down the problem to a simple go program:


import "fmt"

var version = "dev"
var versions = struct {
    API  string
    Code string
}{"0.1.0", version}

func main() {
    fmt.Println(version)
    fmt.Println(versions)
}

Then build it using the specific flag described in the toolchain tricks, go build -o test -ldflags "-X main.version=1.0.0" .

What did you expect to see?

When running ./test I would expect to get an output of

1.0.0
{0.1.0 1.0.0}

What did you see instead?

An output of

1.0.0
{0.1.0 dev}

I suspect this is related to when main.version is evaluated, using ldflags makes the version resolved at link time, where the compiler may have already inlined its use and thus remove the reference used by the linker

@ianlancetaylor ianlancetaylor changed the title from Arbitrary link information failing with static objects to cmd/compile: initialization optimization fails when using -ldflags=-X=VAL Nov 27, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Nov 27, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Nov 27, 2018

@randall77

This comment has been minimized.

Contributor

randall77 commented Nov 27, 2018

This looks like an issue in cmd/compile/internal/gc/sinit.go:sstaticcopy. We resolve another global on the RHS of a global assignment (including in a struct literal) by copying the definition.

It would be unfortunate to lose this optimization because -X doesn't work. Ideas welcome.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Nov 27, 2018

I'd document it as a limitation of -X: it's a special-purpose mechanism in the first place.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Nov 27, 2018

I'm sort of OK with a doc change, but what would that doc change be? We need a simple guideline here that won't confuse people.

@randall77

This comment has been minimized.

Contributor

randall77 commented Nov 27, 2018

Kind of a big hammer, but this optimization does not apply cross-package. We could recommend that all -X overrideable variables live in a separate package.

@tjamet

This comment has been minimized.

tjamet commented Nov 27, 2018

I am also thinking about this option to solve my problem @randall77
Though, I was thinking about this, not sure how this could be applicable, would it be possible to some kind of -X compilation flag that would override somehow the variable definition?
Not sure how this would translate but possibly this could eventually also mean changing --ldflags '-X github.com/example/project/pkg.variable=value' to --gcflags '-X pkg.variable=value'
This would possibly make the -X link flag redundant and mean it could be deprecated

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Nov 27, 2018

I suppose we could make it a compiler flag also, but it has to stay as a linker flag since the point is to set the value at link time. You don't want to have to rebuild your entire world to change a link-time variable value.

@tjamet

This comment has been minimized.

tjamet commented Nov 28, 2018

I see, would another option be to provide a compile option to disable cmd/compile/internal/gc/sinit.go:sstaticcopy and document the limitation with the work around?

@agnivade

This comment has been minimized.

Member

agnivade commented Nov 28, 2018

The above problem is easily solved by moving the struct inside main()

var version = "dev"
var versions = struct {
	API  string
	Code string
}{}

func main() {
	fmt.Println(version)
	versions = struct {
		API  string
		Code string
	}{"0.1.0", version}
	fmt.Println(versions)
}

I vote to just document it and keep it at that.

Something in the lines of - "Use the -X linker flag only to assign values to global variables. If there are expressions reading from that variable, the compiler might replace the variable with the actual value, preventing the replacement from happening."

@josharian

This comment has been minimized.

Contributor

josharian commented Nov 30, 2018

I suppose we could make it a compiler flag also

And presumably also a cmd/go flag, which would then manage passing it appropriately to the compiler and linker?

I suspect that this is one of the primary uses of -ldflags, and (speaking for myself) I'd rather have our compatibility layer be cmd/go and have ~0 reasons for regular users to reach for -ldflags or -gcflags on a regular basis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment