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: ssa ARM64 LoweredAtomic[And|Or]8 generates architecturally unpredictable instruction #25823

Closed
matt2909 opened this issue Jun 11, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@matt2909
Copy link
Contributor

commented Jun 11, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version devel +30a63ec Mon Jun 11 12:13:11 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

GOARCH="amd64"
GOBIN="/work/go/go/bin/"
GOCACHE="/home/user/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/work/user/go"
GORACE=""
GOROOT="/work/go/go"
GOTMPDIR=""
GOTOOLDIR="/work/go/go/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build897149026=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Compiled some code for arm64. Inspected or executed the code sequence generated by LoweredAtomic[And|Or]8.

What did you expect to see?

The code to run without generating an illegal instruction.

What did you see instead?

This generated an illegal instruction panic.

@matt2909

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

This appears to be due to the sequence emitted by the code in src/cmd/compile/internal/ssa/gen/ARM64Ops.go (from line 604):

        case ssa.OpARM64LoweredAtomicAnd8,
                ssa.OpARM64LoweredAtomicOr8:
                // LDAXRB       (Rarg0), Rtmp                                                                                                                                                                                                                                                                                                                               
                // AND/OR       Rarg1, Rtmp                                                                                                                                                                                                                                                                                                                                 
                // STLXRB       Rtmp, (Rarg0), Rtmp                                                                                                                                                                                                                                                                                                                         
                // CBNZ         Rtmp, -3(PC)                                                                                                                                                                                                                                                                                                                                
                r0 := v.Args[0].Reg()
                r1 := v.Args[1].Reg()
                p := s.Prog(arm64.ALDAXRB)
                p.From.Type = obj.TYPE_MEM
                p.From.Reg = r0
                p.To.Type = obj.TYPE_REG
                p.To.Reg = arm64.REGTMP
                p1 := s.Prog(v.Op.Asm())
                p1.From.Type = obj.TYPE_REG
                p1.From.Reg = r1
                p1.To.Type = obj.TYPE_REG
                p1.To.Reg = arm64.REGTMP
                p2 := s.Prog(arm64.ASTLXRB)
                p2.From.Type = obj.TYPE_REG
                p2.From.Reg = arm64.REGTMP
                p2.To.Type = obj.TYPE_MEM
                p2.To.Reg = r0
                p2.RegTo2 = arm64.REGTMP
                p3 := s.Prog(arm64.ACBNZ)
                p3.From.Type = obj.TYPE_REG
                p3.From.Reg = arm64.REGTMP
                p3.To.Type = obj.TYPE_BRANCH
                gc.Patch(p3, p)

The use of STLXRB with identical src and transfer registers (in the above pseudo sequence STLXRB Rtmp, (Rarg0), Rtmp both are Rtmp) is constrained unpredictable (according to the Arm Architecture Reference Manual for Armv8-A).

image

@matt2909

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

Seems to be related to 38d35e7

@matt2909

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2018

If I compile something trivial like:

hello.go:

package main

func main() {
    println("Hello")
}

with GOOS=linux GOARCH=arm64 go install hello.go and then inspect the assembly I can see:

   260cc:       085ffc5b        ldaxrb  w27, [x2]
   260d0:       aa04037b        orr     x27, x27, x4
   260d4:       081bfc5b        stlxrb  w27, w27, [x2]
   260d8:       b5ffffbb        cbnz    x27, 260cc <runtime.greyobject+0x1bc>
   260dc:       17ffffc8        b       25ffc <runtime.greyobject+0xec>
   260e0:       f84307fe        ldr     x30, [sp], #48
   260e4:       d65f03c0        ret

These code snippets for And8 and Or8 are not guaranteed to work as expected (see above comments), when the s (source) and t (transfer) registers are the same register.

@odeke-em odeke-em changed the title src/cmd/compile/internal/ssa/gen/ARM64Ops.go: LoweredAtomic[And|Or]8 generates architecturally unpredictable instruction. cmd/compile: ssa ARM64 LoweredAtomic[And|Or]8 generates architecturally unpredictable instruction Jun 11, 2018

@odeke-em

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

Thank you for the report @matt2909!

/cc @cherrymui @randall77

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

@williamweixiao Does any hardware disallow same src and dst registers for STLXRB?

@gopherbot

This comment has been minimized.

Copy link

commented Jun 11, 2018

Change https://golang.org/cl/117976 mentions this issue: cmd/compile: use a different register for updated value in AtomicAnd8/Or8 on ARM64

@bcmills bcmills added this to the Go1.11 milestone Jun 11, 2018

@williamweixiao

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

It’s not easy to answer this question since there are so many implementations for Arm designed CPU. So I’ve just consulted our hardware design department and their answer is:

We can't be sure, but the Armv8-A architecture states that it is unpredictable, so even if it works on current hardware there's no guarantee that it will work on all future hardware. Furthermore it may be failing quietly even on existing hardware, since this incorrect usage is not required to generate a trap.

@gopherbot gopherbot closed this in 44b826b Jun 12, 2018

dna2github added a commit to dna2fork/go that referenced this issue Jun 14, 2018

cmd/compile: use a different register for updated value in AtomicAnd8…
…/Or8 on ARM64

ARM64 manual says it is "constrained unpredictable" if the src
and dst registers of STLXRB are same, although it doesn't seem
to cause any problem on real hardwares so far. Fix by allocating
a different register to hold the updated value for
AtomicAnd8/Or8. We do this by making the ops returns <val,mem>
like AtomicAdd, although val will not be used elsewhere.

Fixes golang#25823.

Change-Id: I735b9822f99877b3c7aee67a65e62b7278dc40df
Reviewed-on: https://go-review.googlesource.com/117976
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Wei Xiao <Wei.Xiao@arm.com>
@gopherbot

This comment has been minimized.

Copy link

commented Jun 25, 2018

Change https://golang.org/cl/120660 mentions this issue: cmd/internal/obj/arm64: add CONSTRAINED UNPREDICATEABLE behavior check for some load/store

gopherbot pushed a commit that referenced this issue Sep 6, 2018

cmd/internal/obj/arm64: add CONSTRAINED UNPREDICTABLE behavior check …
…for some load/store

According to ARM64 manual, it is "constrained unpredictable behavior"
if the src and dst registers of some load/store instructions are same.
In order to completely prevent such unpredictable behavior, adding the
check for load/store instructions that are supported by the assembler
in the assembler.

Add test cases.

Update #25823

Change-Id: I64c14ad99ee543d778e7ec8ae6516a532293dbb3
Reviewed-on: https://go-review.googlesource.com/120660
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.