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

Jump to bottom

cmd/compile: ssa ARM64 LoweredAtomic[And|Or]8 generates architecturally unpredictable instruction #25823

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

cmd/compile: ssa ARM64 LoweredAtomic[And|Or]8 generates architecturally unpredictable instruction #25823

matt2909 opened this issue Jun 11, 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
Go1.11

Comments

@matt2909
Copy link
Contributor

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
Copy link
Contributor Author

matt2909 commented Jun 11, 2018 •
edited

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
Copy link
Contributor Author

Seems to be related to 38d35e7

@matt2909
Copy link
Contributor Author

matt2909 commented Jun 11, 2018 •
edited

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
Copy link
Member

Thank you for the report @matt2909!

/cc @cherrymui @randall77

@cherrymui
Copy link
Member

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

@gopherbot
Copy link

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 11, 2018
@bcmills bcmills added this to the Go1.11 milestone Jun 11, 2018
@williamweixiao
Copy link
Member

williamweixiao commented Jun 12, 2018 •
edited

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.

dna2github pushed a commit to dna2fork/go that referenced this issue Jun 14, 2018
@cherrymui @dna2github
cmd/compile: use a different register for updated value in AtomicAnd8…
59255b1 
…/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
Copy link

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
@zhangfannie @cherrymui
cmd/internal/obj/arm64: add CONSTRAINED UNPREDICTABLE behavior check …
7ab4b55 
…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>
@golang golang locked and limited conversation to collaborators Jun 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
6 participants
@matt2909 @odeke-em @bcmills @gopherbot @cherrymui @williamweixiao