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/compile: regression in b65122f causes link time error error: R_PPC64_ADDR16_LO_DS not a multiple of 4 #24799

Closed
laboger opened this issue Apr 10, 2018 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@laboger
Copy link
Contributor

laboger commented Apr 10, 2018

Please answer these questions before submitting your issue. Thanks!

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

tip

Does this issue reproduce with the latest release?

yes

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

ppc64le

What did you do?

Trying to build openshift.

What did you expect to see?

Successful build.

What did you see instead?

Link errors at build time. Here is the full output:

INFO] [15:25:53-0400] Detected go version: go version devel +b65122f Mon Apr 9 21:16:47 2018 +0000 linux/ppc64le.
[WARNING] [15:25:53-0400] Detected golang version doesn't match required Go version.
[WARNING] [15:25:53-0400] This version mismatch could lead to differences in execution between this run and the CI systems.
++ Building go targets for linux/ppc64le: cmd/hypershift cmd/openshift cmd/oc cmd/oadm cmd/template-service-broker vendor/k8s.io/kubernetes/cmd/hyperkube pkg/network/sdn-cni-plugin vendor/github.com/containernetworking/plugins/plugins/ipam/host-local vendor/github.com/containernetworking/plugins/plugins/main/loopback
 
# github.com/openshift/origin/cmd/template-service-broker
/home/boger/golang/pristine/go/pkg/tool/linux_ppc64le/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-160549453/go.o: In function `github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit.ordLevel':
/home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x6f0664): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: /home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x6f0a24): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: /home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x6f1184): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status

# github.com/openshift/origin/cmd/hypershift
/home/boger/golang/pristine/go/pkg/tool/linux_ppc64le/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-878320159/go.o: In function `github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit.ordLevel':
/home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x1048bc4): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: /home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x1048f84): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: /home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x10496e4): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status

# github.com/openshift/origin/cmd/openshift
/home/boger/golang/pristine/go/pkg/tool/linux_ppc64le/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-204861902/go.o: In function `github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit.ordLevel':
/home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x1675bc4): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: /home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x1675f84): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: /home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0x16766e4): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status

# github.com/openshift/origin/vendor/k8s.io/kubernetes/cmd/hyperkube
/home/boger/golang/pristine/go/pkg/tool/linux_ppc64le/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-651277169/go.o: In function `github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit.ordLevel':
/home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0xcbf254): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: /home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0xcbf614): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: /home/boger/openshift/origin/_output/local/go/src/github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/apis/audit/helpers.go:21:(.text+0xcbfd74): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status

[ERROR] [15:33:31-0400] PID 25323: hack/lib/build/binaries.sh:236: `GOOS=${platform%/*} GOARCH=${platform##*/} go install -pkgdir "${pkgdir}/${platform}" -tags "${OS_GOFLAGS_TAGS-} ${!platform_gotags_envvar:-}" -ldflags="${local_ldflags}" "${goflags[@]:+${goflags[@]}}" -gcflags "${gogcflags}" "${nonstatics[@]}"` exited with status 2.
[INFO] [15:33:31-0400] 		Stack Trace: 
[INFO] [15:33:31-0400] 		  1: hack/lib/build/binaries.sh:236: `GOOS=${platform%/*} GOARCH=${platform##*/} go install -pkgdir "${pkgdir}/${platform}" -tags "${OS_GOFLAGS_TAGS-} ${!platform_gotags_envvar:-}" -ldflags="${local_ldflags}" "${goflags[@]:+${goflags[@]}}" -gcflags "${gogcflags}" "${nonstatics[@]}"`
[INFO] [15:33:31-0400] 		  2: hack/lib/build/binaries.sh:156: os::build::internal::build_binaries
[INFO] [15:33:31-0400] 		  3: hack/build-go.sh:25: os::build::build_binaries
[INFO] [15:33:31-0400]   Exiting with code 2.
[ERROR] [15:33:31-0400] PID 25273: hack/lib/build/binaries.sh:150: `( os::build::internal::build_binaries "${binaries[@]+"${binaries[@]}"}" )` exited with status 2.
[INFO] [15:33:31-0400] 		Stack Trace: 
[INFO] [15:33:31-0400] 		  1: hack/lib/build/binaries.sh:150: `( os::build::internal::build_binaries "${binaries[@]+"${binaries[@]}"}" )`
[INFO] [15:33:31-0400] 		  2: hack/build-go.sh:25: os::build::build_binaries
[INFO] [15:33:31-0400]   Exiting with code 2.
[ERROR] [15:33:31-0400] hack/build-go.sh exited with code 2 after 00h 07m 38s
Makefile:43: recipe for target 'all' failed
make: *** [all] Error 2

This error does not occur with the commit before it.
Looks like the change was intended to merge loads even if they were unaligned, but if a DS load is chosen then the offset must be a multiple of 4 (otherwise you get the above error message).
@mundaym

@mundaym mundaym added this to the Go1.11 milestone Apr 10, 2018
@mundaym mundaym added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 10, 2018
@mundaym
Copy link
Member

mundaym commented Apr 10, 2018

The commit in question is CL 102558.

I think the optimizations enabled by this CL are safe on any architecture (even ones that don't allow unaligned accesses). It relies on the SSA rules to actually generate unaligned loads. This makes me think that we need more constraints on load merging when accessing global symbols on ppc64le.

@laboger: does the code below cause a bad relocation?

package main

import "encoding/binary"

var x [5]byte

func main() {
    if binary.LittleEndian.Uint32(x[1:5]) == 1 {
        panic("wrong")
    }
}

@laboger
Copy link
Contributor Author

laboger commented Apr 10, 2018

The list of DS instructions with this restriction is small.
ld, ldu, std, lwa. The rest are not a problem and would not cause this error message.

Your above example does not cause an error since it is a 4 bytes unsigned. I tried to change it to 8 bytes but that doesn't fail because the rules check to combine LE loads checks that the compile time offset is a multiple of 4.

I believe I fixed all the rules where this was a problem due to the compile time offset being incorrect, because the problem did come up when I first added them. Also, asm9.go checks the constant offset field when assembling an instruction and if the offset field is not a multiple of 4 for a DS instruction an error is issued.

I believe the fact that the error is coming from the linker means that the compile time offset is OK but the relocated offset ends up with a value or 1 or 2. And this error would probably only come from the external linker because I doubt the golang linker knows this is a problem.

I will look into the failing .o file and see if I can find exactly the symbol that is causing the failure so a smaller reproducer can be created.

@laboger
Copy link
Contributor Author

laboger commented Apr 11, 2018

Here is the before and after:
Previously made a call to runtime.memequal, and because of the way the address was computed there was no relocation issue (i.e., no DS instruction was involved).

func ordLevel(l Level) int {
        switch l {
        case LevelMetadata:
  **cc0e80:       00 00 e0 3f     lis     r31,0
                        cc0e80: R_PPC64_ADDR16_HA       go.string.*+0x85e1
  cc0e84:       00 00 7f 38     addi    r3,r31,0
                        cc0e84: R_PPC64_ADDR16_LO       go.string.*+0x85e1**
  cc0e88:       20 00 61 f8     std     r3,32(r1)
  cc0e8c:       28 00 61 f8     std     r3,40(r1)
  cc0e90:       08 00 60 38     li      r3,8
  cc0e94:       30 00 61 f8     std     r3,48(r1)
  cc0e98:       01 00 00 48     bl      cc0e98 <github.com/openshift/origin/vendor/k8s.io/apiserver/pkg/audit.LogImpersonatedUser+0x78>
                        cc0e98: R_PPC64_REL24   runtime.memequal
  cc0e9c:       38 00 61 88     lbz     r3,56(r1)
  cc0ea0:       48 00 81 e8     ld      r4,72(r1)

Now:
        case LevelMetadata:
  **6f0660:       00 00 e0 3f     lis     r31,0
                        6f0660: R_PPC64_ADDR16_HA       go.string.*+0x577f
  6f0664:       00 00 bf e8     ld      r5,0(r31)
                        6f0664: R_PPC64_ADDR16_LO_DS    go.string.*+0x577f**
  6f0668:       00 00 e0 3f     lis     r31,0
                        6f0668: R_PPC64_ADDR16_HA       $i64.617461646174654d
  6f066c:       00 00 df e8     ld      r6,0(r31)
                        6f066c: R_PPC64_ADDR16_LO_DS    $i64.617461646174654d
  6f0670:       00 30 25 7c     cmpd    r5,r6

The code in asm9.go for symbolAccess is not checking if the offset being added to the relocation is a multiple of 4 when using DS relocation and maybe it should... I think the assumption was that 8 byte constants or static variables would always be aligned correctly but in this case it is not.

@mundaym
Copy link
Member

mundaym commented Apr 11, 2018

Ah interesting, yes, static variables only have to be aligned to 1 byte.

Increasing the linkers minimum data alignment to 4 might solve your problem:

minAlign = 1 // min data alignment

We have it set to 2 on s390x, though thinking about it it might make sense to push it up to 8 since that would make the rewrite rules easier.

@laboger
Copy link
Contributor Author

laboger commented Apr 11, 2018

It does work to change the alignment as you suggest above.

I also think you were right originally, this is a problem in the combine rules, because it shouldn't be combining bytes to do an 8 byte load if it isn't aligned properly. I've been trying to add to the rule to check for alignment and I can't get it to build. Here is what I added:

(OR <t> s6:(SLDconst x7:(MOVBZload [i7] {s} p mem) [56])
        o5:(OR <t> s5:(SLDconst x6:(MOVBZload [i6] {s} p mem) [48])
        o4:(OR <t> s4:(SLDconst x5:(MOVBZload [i5] {s} p mem) [40])
        o3:(OR <t> s3:(SLDconst x4:(MOVBZload [i4] {s} p mem) [32])
        x0:(MOVWZload {s} [i0] p mem)))))
        && s.(*types.Type).Alignment()%4 == 0
        && !config.BigEndian
        && i0%4 == 0
        && i4 == i0+4
        && i5 == i0+5
        && i6 == i0+6
        && i7 == i0+7
        && x0.Uses == 1 && x4.Uses == 1 && x5.Uses == 1 && x6.Uses ==1 && x7.Uses == 1
        && o3.Uses == 1 && o4.Uses == 1 && o5.Uses == 1
        && s3.Uses == 1 && s4.Uses == 1 && s5.Uses == 1 && s6.Uses == 1
        && mergePoint(b, x0, x4, x5, x6, x7) != nil
        && clobber(x0) && clobber(x4) && clobber(x5) && clobber(x6) && clobber(x7)
        && clobber(s3) && clobber(s4) && clobber(s5) && clobber (s6)
        && clobber(o3) && clobber(o4) && clobber(o5)
          -> @mergePoint(b,x0,x4,x5,x6,x7) (MOVDload <t> {s} [i0] p mem)

But that causes this error:

/home/boger/golang/test/go/src/runtime/mbitmap.go:189:1: internal compiler error: panic during lower while compiling (*mspan).refillAllocCache:

interface conversion: interface {} is nil, not *types.Type

goroutine 1 [running]:
bootstrap/cmd/compile/internal/ssa.Compile.func1(0xc8237b27e8, 0xc8230f1a40)
	/home/boger/golang/test/go/src/cmd/compile/internal/ssa/compile.go:38 +0xa4
bootstrap/cmd/compile/internal/ssa.rewriteValuePPC64_OpPPC64OR_20(0xc82266c680, 0xc82266c500)
	/home/boger/golang/test/go/src/cmd/compile/internal/ssa/rewritePPC64.go:10739 +0x75c4
bootstrap/cmd/compile/internal/ssa.rewriteValuePPC64(0xc82266c680, 0x0)
	/home/boger/golang/test/go/src/cmd/compile/internal/ssa/rewritePPC64.go:491 +0x440
bootstrap/cmd/compile/internal/ssa.applyRewrite(0xc8230f1a40, 0x85e9a8, 0x85ea00)
	/home/boger/golang/test/go/src/cmd/compile/internal/ssa/rewrite.go:54 +0x270
bootstrap/cmd/compile/internal/ssa.lower(0xc8230f1a40)
	/home/boger/golang/test/go/src/cmd/compile/internal/ssa/lower.go:10 +0x44
bootstrap/cmd/compile/internal/ssa.Compile(0xc8230f1a40)
	/home/boger/golang/test/go/src/cmd/compile/internal/ssa/compile.go:70 +0x550
bootstrap/cmd/compile/internal/gc.buildssa(0xc820fc7080, 0x0, 0x0)
	/home/boger/golang/test/go/src/cmd/compile/internal/gc/ssa.go:201 +0xd9c
bootstrap/cmd/compile/internal/gc.compileSSA(0xc820fc7080, 0x0)
	/home/boger/golang/test/go/src/cmd/compile/internal/gc/pgen.go:259 +0x34
bootstrap/cmd/compile/internal/gc.compile(0xc820fc7080)
	/home/boger/golang/test/go/src/cmd/compile/internal/gc/pgen.go:238 +0xe4
bootstrap/cmd/compile/internal/gc.funccompile(0xc820fc7080)
	/home/boger/golang/test/go/src/cmd/compile/internal/gc/pgen.go:209 +0x230
bootstrap/cmd/compile/internal/gc.Main(0x85e7f8)
	/home/boger/golang/test/go/src/cmd/compile/internal/gc/main.go:631 +0x2f68
main.main()
	/home/boger/golang/test/go/src/cmd/compile/main.go:49 +0x1cc

Any advice on what is wrong with my rule?

@laboger
Copy link
Contributor Author

laboger commented Apr 12, 2018

This reproduces the problem.

package main

import (
        "fmt"
)

type Level string

const (
        LevelBad Level = "badvals"
        LevelNone Level = "No"
        LevelMetadata Level = "Metadata"
        LevelRequest Level = "Request"
        LevelRequestResponse Level = "RequestResponse"
)

func ordLevel(l Level) int {
        switch l {
        case LevelMetadata:
                return 1
        case LevelRequest:
                return 2
        case LevelRequestResponse:
                return 3
        default:
                return 0
        }
}

func test(l Level) {
        if ordLevel(l) < ordLevel(LevelMetadata) {
                fmt.Printf("### OK ###\n")
        }
}

func main() {
        test(LevelBad)
        test(LevelNone)
        test(LevelMetadata)
        test(LevelRequestResponse)
        fmt.Printf("### all done ###\n")
}

Fails with both the internal and external linker, with different messages:

go build test-movd.go
# command-line-arguments
main.test: bad DS reloc for main.test: 846795
boger@willow3:~/gotests/bugs$ go build -ldflags '-linkmode=external' test-movd.go
# command-line-arguments
/home/boger/golang/upstream/go/pkg/tool/linux_ppc64le/link: running gcc failed: exit status 1
/usr/bin/ld: /tmp/go-link-223221599/go.o: In function `main.ordLevel':
/home/boger/gotests/bugs/test-movd.go:19:(.text+0x80354): error: R_PPC64_ADDR16_LO_DS not a multiple of 4
/usr/bin/ld: final link failed: Bad value
collect2: error: ld returned 1 exit status

I think there are multiple ways to fix it, just trying to decide which one is best.

  • force alignment of go.string constants
  • fix the rule so these are not combined (but I'm not sure how to do that yet)
  • fix the assembler by changing code in obj9.go and/or asm9.go so that a DS instruction is not used in this case. It would have to use another instruction which makes it trickier.

I think the last choice is best since that doesn't force alignment on go.strings, and doesn't prevent some matching.

@laboger
Copy link
Contributor Author

laboger commented Apr 18, 2018

I'm working on a fix to load an 8 byte go.string in a way that doesn't use relocation.

@gopherbot
Copy link

Change https://golang.org/cl/107855 mentions this issue: cmd/compile: generate load without DS relocation for go.string on ppc64le

@golang golang locked and limited conversation to collaborators Apr 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants