cmd/link: detect -X setting non-string variable? #9621

jessfraz · 2015-01-17T02:38:28Z

Ok this is a bit of an oddball bug. We tried for at least 3 hours to find a small reproducible case to give you all but could not find a way to do it.

We just switched the Docker project, https://github.com/docker/docker, to use go 1.4.1. The problem we are seeing is a few patches that have been submitted are compiling successfully but the resulting binary is failing to work. It cannot find the linked files, etc. But I should note, the master branch of docker is working correctly with Go1.4.1.

Some background on how we compile docker:

we create a static binary
the build tags/script we use is here: https://github.com/docker/docker/blob/master/project/make.sh#L116

We have found that we can get Docker to compile correctly on these failed patches, by adding an empty

func init(){
}

to the bottom of the dockerversion.go file which is the file being compiled with -X dockerversion.IAMSTATIC in the above script linked to our build process.

So we have an idea:

Maybe there is some compiler optimization we are hitting. The thoughts behind this stem from the fact, the variables in the dockerversion.go file are empty and get filled at the linker stage. But this theory is quite odd because as mentioned before, the master branch works with go1.4.1.

The patches that fail are:

These are not abnormal patches in anyway, we can't find any commonality between them that would cause this.

To sum up, all patches work successfully on Go1.4.0, but fail on 1.4.1. Also by fail I mean they compile with no errors but when trying to run the resulting binary it does not work because it cannot find the linked files.

cc @tianon @icecrime

Also again, apologies we could not find a simple reproducible case. Let me know anything else I can do information wise. and I welcome those I pinged above to add anything I may have forgotten or explained poorly.

Reproducing

Making a binary

$ git clone git@github.com:docker/docker
$ cd docker
$ ./hack/make.sh binary

# running the binary, this should fail with the patches
$ ./bundles/1.4.1-dev/binary/docker -d -D

Running the integration suite

These are the tests that run on the compiled binary. All should fail when you apply any of the above patches.

$ ./hack/make.sh test-integration-cli

The text was updated successfully, but these errors were encountered:

minux · 2015-01-17T03:00:22Z

The docker project is so huge that I can't easily reproduce the problem.

As there isn't many patches backported to Go 1.4.1, could you please
try to bisect exactly which commit breaks your build? Thanks.

Going through the history, I didn't see any suspicious commit that could
affect the -X flag, unless you're using some hacks with syso files, as
the only cmd/ld patch affects linking .syso files.

jessfraz · 2015-01-17T03:01:12Z

TLDR:

Compiling a static binary with go 1.4 works, and the binary works
Compiling a static binary with go 1.4.1 works, running it fails with not being able to find linked files

However the second statement is a craps shoot. Sometimes it works and sometimes it doesn't. Which is described in detail above.

jessfraz · 2015-01-17T03:01:38Z

Yes I will do a bisect. Sorry.

davecheney · 2015-01-17T03:22:58Z

On 17 Jan 2015, at 14:01, Jessie Frazelle notifications@github.com wrote:

TLDR:

Compiling a static binary with go 1.4 works, and the binary works
Compiling a static binary with go 1.4.1 works, running it fails with not being able to find linked files

That suggests to me that you don't have a static binary.

Can you post the output of the failure. I'll probably get shouted down for suggesting this, but also the output of ldd $binary

However the second statement is a craps shoot. Sometimes it works and sometimes it doesn't. Which is described in detail above.

—
Reply to this email directly or view it on GitHub.

tianon · 2015-01-17T03:47:13Z

Hopefully some of this helps, @davecheney: (while @jfrazelle works on that bisect)

$ # in docker/docker, https://github.com/docker/docker/pull/10151 checked out
$ git log -1 --oneline
fa2024b Add ability to set client runtime defaults
$ make shell
docker build ...
...
docker run ...
root@c2af08758a22:/go/src/github.com/docker/docker# git grep -n IAMSTATIC
dockerversion/dockerversion.go:12:  IAMSTATIC bool   // whether or not Docker itself was compiled statically via ./hack/make.sh binary
project/make.sh:127:    -X $DOCKER_PKG/dockerversion.IAMSTATIC true
utils/utils.go:97:  if dockerversion.IAMSTATIC {
root@c2af08758a22:/go/src/github.com/docker/docker# bash -x ./hack/make.sh binary
...
++ go build -o /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev -a -tags 'netgo static_build apparmor selinux btrfs_noversion daemon' -ldflags '
        -w 
    -X github.com/docker/docker/dockerversion.GITCOMMIT "fa2024b"
    -X github.com/docker/docker/dockerversion.VERSION "1.4.1-dev"


    -linkmode external
    -X github.com/docker/docker/dockerversion.IAMSTATIC true
    -extldflags "-static -lpthread -Wl,--unresolved-symbols=ignore-in-object-files"

    ' ./docker
++ echo 'Created binary: /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev'
...
root@c2af08758a22:/go/src/github.com/docker/docker# /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev -v
Docker version 1.4.1-dev, build fa2024b
root@c2af08758a22:/go/src/github.com/docker/docker# /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev -d
INFO[0000] +job serveapi(unix:///var/run/docker.sock)   
INFO[0000] Listening for HTTP on unix (/var/run/docker.sock) 
INFO[0000] +job init_networkdriver()                    
INFO[0000] enableIPv6 = false                           
INFO[0000] -job init_networkdriver() = OK (0)           
FATA[0000] Could not locate dockerinit: This usually means docker was built incorrectly. See http://docs.docker.com/contributing/devenvironment for official build instructions. 
root@c2af08758a22:/go/src/github.com/docker/docker# ldd /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev
    not a dynamic executable

So somehow, it's not getting the value of dockerversion.IAMSTATIC which is set to true via -ldflags -X (but it is getting the values of dockerversion.GITCOMMIT and dockerversion.VERSION, as seen from the -v output above).

If we do any of a number of small things, the problem goes away and the daemon starts just fine, including things as simple as:

adding dockerversion.IAMSTATIC to the docker -v output in docker/docker.go or to the dockerinit error message output in daemon/daemon.go
adding func init() { } (yes, empty) in dockerversion/dockerversion.go

davecheney · 2015-01-17T03:51:11Z

@tianon is it possible for you to show two binaries, the working and the failed ?

tianon · 2015-01-17T04:21:00Z

Absolutely!

root@8280b2f578af:/go/src/github.com/docker/docker# git diff
diff --git a/dockerversion/dockerversion.go b/dockerversion/dockerversion.go
index c130ac2..687382f 100644
--- a/dockerversion/dockerversion.go
+++ b/dockerversion/dockerversion.go
@@ -1,5 +1,8 @@
 package dockerversion

+func init() {
+}
+
 // FIXME: this should be embedded in the docker/docker.go,
 // but we can't because distro policy requires us to
 // package a separate dockerinit binary, and that binary needs
root@8280b2f578af:/go/src/github.com/docker/docker# ./hack/make.sh binary

bundles/1.4.1-dev already exists. Removing.

---> Making bundle: binary (in bundles/1.4.1-dev/binary)
Created binary: /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev

root@8280b2f578af:/go/src/github.com/docker/docker# ./bundles/1.4.1-dev/binary/docker -d
INFO[0000] +job serveapi(unix:///var/run/docker.sock)   
INFO[0000] Listening for HTTP on unix (/var/run/docker.sock) 
INFO[0000] +job init_networkdriver()                    
INFO[0000] enableIPv6 = false                           
INFO[0000] -job init_networkdriver() = OK (0)           
INFO[0000] Loading containers: start.                   

INFO[0000] Loading containers: done.                    
INFO[0000] docker daemon: 1.4.1-dev fa2024b-dirty; execdriver: native-0.2; graphdriver: btrfs 
INFO[0000] +job acceptconnections()                     
INFO[0000] -job acceptconnections() = OK (0)            
^CINFO[0013] Received signal 'interrupt', starting shutdown of docker... 
root@8280b2f578af:/go/src/github.com/docker/docker# # used Ctrl+C, since it hangs waiting for connections on the socket, running successfully as a daemon since it found the IAMSTATIC flag set to true

I can get you copies of the binaries too, if that'd be helpful.

davecheney · 2015-01-17T04:25:03Z

@tianon i'm sorry but I don't know much about the docket build process so quoting these large outputs is not informative.

Could you please show a minimum output that demonstrates the problem. My best guess so far is you built a go binary that was not static when it was expected to be. Is this correct ?

tianon · 2015-01-17T04:28:50Z

No, the problem is that one of our -X-injected variables isn't getting set -- we're getting static binaries just fine (both the working and the non-working binary ends up static).

We inject three variables there: dockerversion.VERSION, dockerversion.GITCOMMIT, and dockerversion.IAMSTATIC.

The first two are set to strings, which as you can see from the docker -v output come through just fine in either case. The one that doesn't is dockerversion.IAMSTATIC which is a bool that we set to true just to inform our daemon that it's a single-binary static build instead of a dual-binary dynamic build (with a pure-static smaller dockerinit binary, but that's not relevant here, it's just the context for what these variables are for -- the canonical way we build for our official releases is the single pure-static binary, which is where we're seeing the issue).

tianon · 2015-01-17T04:30:01Z

We've tried creating a smaller reproducing case than Docker itself, but we haven't had any success, especially since we haven't a clue what's causing it due to the strange things that cause it to be fixed (like adding an empty init() function in that otherwise just-vars package, which are all empty vars that get injected in the linker via -X).

minux · 2015-01-17T04:30:26Z

Wait, I just found that your IAMSTATIC variable is declared bool.

I understand what's happening now.

cmd/ld -X only supports setting string variables, so what really
happens is that the linker changes the type to string and layout
the following in the binary for variable IAMSTATIC:

IAMSTATIC:
*byte // pointer to the string content
int // length of the string

But any Go code referencing the IAMSTATIC variable will still
think it's a bool, just load the first byte it and see if it's non-zero.
On x86, the first byte happens to be the least significant byte of
the string address.

If it happens that the "true" string is laid out at some address
whose least-significant byte is 0 (i.e. aligned to 256-byte), the
binary will read a false IAMSTATIC.

Please change the IAMSTATIC variable to a string, and
use a string == "true". This is not a Go bug. I will see if cmd/ld
could detect the misuse, but no promises.

tianon · 2015-01-17T04:32:09Z

@minux oh man, that's genius -- didn't even think about that; testing now

minux · 2015-01-17T04:40:01Z

You can verify my explanation easily, assume docker is a failing
binary.

$ gdb ./docker
(gdb) x/16bx &'github.com/docker/docker/dockerversion.IAMSTATIC'

The first byte shown should be zero. And you will see that the first 8 byte
is an address, and the next 8 byte represents 4 (the length of "true").

tianon · 2015-01-17T04:44:07Z

Ahh, that does it. ❤️

Once IAMSTATIC is a string instead, all is well and right with the world. Didn't even realize we were using -X wrong. 😊

Indeed:

root@8280b2f578af:/go/src/github.com/docker/docker# gdb ./bundles/1.4.1-dev/binary/docker-1.4.1-dev
...
(gdb) x/16bx &'github.com/docker/docker/dockerversion.IAMSTATIC'
0x128c920 <github.com/docker/docker/dockerversion.IAMSTATIC>:   0x00    0x9a    0xe3    0x00    0x00    0x00    0x00    0x00
0x128c928 <github.com/docker/docker/dockerversion.IAMSTATIC+8>: 0x04    0x00    0x00    0x00    0x00    0x00    0x00    0x00

jessfraz · 2015-01-17T04:44:49Z

I still don't understand how it was ever working then haha but thanks so much, I will close this.

jessfraz · 2015-01-17T04:46:09Z

Sorry didn't realize you changed the title to keep open for other reasons.

minux · 2015-01-17T05:17:30Z

For the cmd/ld issue, the only thing we can do is to detect if:

the size of the object being set is the size of a string
the object being set is in the DATA section, rather than NOPTRDATA
or RODATA.

Either of these check will catch the OP's problem, but they still can't
catch every possible misuse.

I will leave this to other to decide.

josharian · 2015-02-03T23:23:05Z

@minux they won't catch every possible misuse, but there aren't many NOPTRDATA/RODATA string-sized objects around. If the check is cheap and small, it seems worth doing.

minux · 2015-02-05T05:54:14Z

We just need to save each variable set using -X and set them after all the object files are loaded. (Currently cmd/ld sets the variable as soon as each -X is saw on the command line.) The added benefit is that we can detect setting of undefined variable, which is a common problem when the user misses the package name prefix.

titanous · 2015-02-05T08:01:58Z

Please don't require the variable to be defined, we have build system sets a version variable on a specific package for all binaries built, and only some of them import the package to get access to the version.

josharian · 2015-02-05T18:34:17Z

Requiring the variable to be defined was discussed and rejected in #8820.

We could however warn in the case in which there is no package name prefix at all.

rsc · 2015-06-08T06:10:34Z

It should be easy to accept both undefined variables and variables declared as strings but nothing else.

gopherbot · 2015-06-29T18:00:46Z

CL https://golang.org/cl/11694 mentions this issue.

mikioh changed the title ~~Compiling with Go1.4.1 having binary problems~~ build: compiling with go1.4.1 having binary problems Jan 17, 2015

minux changed the title ~~build: compiling with go1.4.1 having binary problems~~ cmd/ld: detect -X setting non-string variable? Jan 17, 2015

minux added the repo-main label Jan 17, 2015

minux added this to the Go1.5 milestone Jan 17, 2015

jessfraz closed this as completed Jan 17, 2015

jessfraz reopened this Jan 17, 2015

tianon mentioned this issue Jan 17, 2015

Fix "-X" 6l usage ("define string data") moby/moby#10154

Merged

rsc removed the repo-main label Apr 14, 2015

rsc changed the title ~~cmd/ld: detect -X setting non-string variable?~~ cmd/link: detect -X setting non-string variable? Jun 8, 2015

rsc closed this as completed in 69f0d4c Jun 29, 2015

golang locked and limited conversation to collaborators Jun 28, 2016

gopherbot added the FrozenDueToAge label Jun 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/link: detect -X setting non-string variable? #9621

cmd/link: detect -X setting non-string variable? #9621

jessfraz commented Jan 17, 2015

minux commented Jan 17, 2015

jessfraz commented Jan 17, 2015

jessfraz commented Jan 17, 2015

davecheney commented Jan 17, 2015

tianon commented Jan 17, 2015

davecheney commented Jan 17, 2015

tianon commented Jan 17, 2015

davecheney commented Jan 17, 2015

tianon commented Jan 17, 2015

tianon commented Jan 17, 2015

minux commented Jan 17, 2015

tianon commented Jan 17, 2015

minux commented Jan 17, 2015

tianon commented Jan 17, 2015

jessfraz commented Jan 17, 2015

jessfraz commented Jan 17, 2015

minux commented Jan 17, 2015

josharian commented Feb 3, 2015

minux commented Feb 5, 2015 via email

titanous commented Feb 5, 2015

josharian commented Feb 5, 2015

rsc commented Jun 8, 2015

gopherbot commented Jun 29, 2015

cmd/link: detect -X setting non-string variable? #9621

cmd/link: detect -X setting non-string variable? #9621

Comments

jessfraz commented Jan 17, 2015

Reproducing

minux commented Jan 17, 2015

jessfraz commented Jan 17, 2015

jessfraz commented Jan 17, 2015

davecheney commented Jan 17, 2015

tianon commented Jan 17, 2015

davecheney commented Jan 17, 2015

tianon commented Jan 17, 2015

davecheney commented Jan 17, 2015

tianon commented Jan 17, 2015

tianon commented Jan 17, 2015

minux commented Jan 17, 2015

tianon commented Jan 17, 2015

minux commented Jan 17, 2015

tianon commented Jan 17, 2015

jessfraz commented Jan 17, 2015

jessfraz commented Jan 17, 2015

minux commented Jan 17, 2015

josharian commented Feb 3, 2015

minux commented Feb 5, 2015 via email

titanous commented Feb 5, 2015

josharian commented Feb 5, 2015

rsc commented Jun 8, 2015

gopherbot commented Jun 29, 2015