TestFilesystemInfo miscompiled on ppc64el by gccgo-4.9 in 1.25

[Curtis Hovey]

As seen in
    http://reports.vapour.ws/releases/issue/559a68b2749a562e2e8bcd6b

TestFilesystemInfo can fail for hilarious reasons:

storage_test.go:187:
    c.Assert(info, jc.DeepEquals, expect)
... obtained state.FilesystemAttachmentInfo = state.FilesystemAttachmentInfo{MountPoint:"", ReadOnly:true}
... expected state.FilesystemAttachmentInfo = state.FilesystemAttachmentInfo{MountPoint:"", ReadOnly:true}
... mismatch at .ReadOnly: unequal; obtained true; expected true

mgz believes at the time of evaluation the value was false, but by the time of printing, the value is true.

This issue first appeared in 1.25
    https://github.com/juju/juju/commit/9c086f523c4c71300ce4f260f48bd7267d422c91
    https://github.com/juju/juju/commit/c657210ad63761e7695056df20cd1c38a475776c

CI will try to make this test pass 3 times, then curse the branch. We have seen that with 6 tries the test can pass.

[Menno Finlay-Smits (menno.smits)]

It's a strange one because there's only a single goroutine involved, the test and related implementation code are quite straightforward, and the structs in question are copies (no pointers being passed around). It hard to see how the struct values could be changed in unexpected ways.

I've looked into it on stilson-09 (the ppc build slave) and have found some pretty weird behaviour. The "expected" struct is created here: https://github.com/juju/juju/blob/1.25/state/filesystem_test.go#L259, like this:

    filesystemAttachmentInfo := state.FilesystemAttachmentInfo{MountPoint: "/srv"}

There's a bool field called ReadOnly which isn't being explicitly specified.

If I add:

    fmt.Printf("%+v\n", filesystemAttachmentInfo)

on the next line it almost always emits {MountPoint: , ReadOnly: true}. WTF?!

If I change the line that creates the struct to:

    filesystemAttachmentInfo := state.FilesystemAttachmentInfo{MountPoint: "/srv", ReadOnly: false}

the Printf shows the expected field values and the test consistently passes.

The only explanation I can come up with is a compiler bug. I've sent the above to davecheney and mwhudson. Dave agrees it's probably a compiler bug but that it's unlikely that anyone will both fixing gccgo4.9 given that go1.5 now has the focus in terms of supporting ppc with Go. Waiting to see if Michael has any further comments.

[Cheryl Jennings (cherylj)]

1.25 PR: https://github.com/juju/juju/pull/3789

[Michael Hudson-Doyle (mwhudson)]

I can't claim to entirely understand what's going on, but the version of gccgo in trusty is definitely doing something wrong.

filesystemAttachmentInfo lives on the stack (at 504(r31)) and it is initialized by these instructions:

    102bf88c: 14 ff 22 3d addis r9,r2,-236
    102bf890: d8 d7 29 39 addi r9,r9,-10280
    102bf894: 00 00 09 e9 ld r8,0(r9)
    102bf898: 08 00 49 e9 ld r10,8(r9)
    102bf89c: 10 00 29 e9 ld r9,16(r9)
    102bf8a0: f8 01 1f f9 std r8,504(r31)
    102bf8a4: 00 02 5f f9 std r10,512(r31)
    102bf8a8: 08 02 3f f9 std r9,520(r31)

which is basically copying the data from somewhere else. But that data is all messed up.

The test works on wily -- the code is compiled the same way, but it is copying the correct data this time.

[Michael Hudson-Doyle (mwhudson)]

The test appears to work fine on arm64.

[Michael Hudson-Doyle (mwhudson)]

arm64 trusty, I mean.