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/asm: incorrectly signaled 'R15 as clobbered' in MULXQ #58632

Closed
armfazh opened this issue Feb 22, 2023 · 5 comments
Closed

cmd/asm: incorrectly signaled 'R15 as clobbered' in MULXQ #58632

armfazh opened this issue Feb 22, 2023 · 5 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@armfazh
Copy link

armfazh commented Feb 22, 2023

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

$ go version
go version go1.20.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env

GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.20.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1448370878=/tmp/go-build -gno-record-gcc-switches"

What did you do?

main.go

package main
var myGlobal = []byte("helloworld")
//go:noescape
func a1()
//go:noescape
func a2()
//go:noescape
func a3()
//go:noescape
func a4()
func main() {
  _ = myGlobal
  a1(); a2(); a3(); a4()
}

mulx_amd64.s

TEXT ·a1(SB),0,$0-0
    CMPL ·myGlobal(SB), $0
    MULXQ R15, AX, DX
    RET

TEXT ·a2(SB),0,$0-0
    CMPL ·myGlobal(SB), $0
    MULXQ AX, R15, DX
    RET

TEXT ·a3(SB),0,$0-0
    CMPL ·myGlobal(SB), $0
    MULXQ AX, DX, R15
    RET

TEXT ·a4(SB),0,$0-0
    CMPL ·myGlobal(SB), $0
    MOVQ AX, R15
    RET

Command:

$ go mod init example.com/mulx
$ go build -buildmode=plugin

What did you expect to see?

Compiler triggers 1 error. This is a true error, since R15 is being used as an input of MULX.

asm: mulx_amd64.s:3: when dynamic linking, R15 is clobbered by a global variable access and is used here: 00003 (/home/user/mulx_amd64.s:3)	MULXQ	R15, AX, DX
asm: assembly failed

What did you see instead?

The compiler signals 3 errors. The first one is a true error. The second and third are false positives, since R15 is being used as an output of MULX.
To confirm this, note that the a4 function has no error as R15 is used as output of MOVQ.

asm: mulx_amd64.s:3: when dynamic linking, R15 is clobbered by a global variable access and is used here: 00003 (/home/user/mulx_amd64.s:3)	MULXQ	R15, AX, DX
asm: mulx_amd64.s:8: when dynamic linking, R15 is clobbered by a global variable access and is used here: 00007 (/home/user/mulx_amd64.s:8)	MULXQ	AX, R15, DX
asm: mulx_amd64.s:13: when dynamic linking, R15 is clobbered by a global variable access and is used here: 00011 (/home/user/mulx_amd64.s:13)	MULXQ	AX, DX, R15
asm: assembly failed

Other info:
The closest CL related to this issue is: https://go-review.googlesource.com/c/go/+/283474
I found this issue while working in cloudflare/circl#407

@armfazh armfazh changed the title go build: incorrectly signaled 'R15 as clobbered' in MULXQ cmd/asm: incorrectly signaled 'R15 as clobbered' in MULXQ Feb 22, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 22, 2023
@randall77
Copy link
Contributor

This is a tricky one, as there's no solid indication at the assembly level that a register is output only. Lots of instructions in x86 update the output register (e.g. ADDQ AX, CX uses CX as both an input and an output). MULX is additionally weird in that it has 2 output registers.

For instructions we know are pure overwriters, we could add additional cases to the end of cmd/internal/obj/x86/obj6.go:progOverwritesR15. But I'm hesitant to add such instructions one or two at a time - we'd want a principled way to detect them all, and I don't know what the right way to do that is.

An easy workaround is to just add a MOVQ $0, R15 or similar before the instruction that really does overwrite R15 (or after the global variable access).

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 22, 2023
@armfazh
Copy link
Author

armfazh commented Feb 23, 2023

Based on the Intel docs [1] [2]:
MULX instruction has two output (write-only) registers.

Is it possible to add this exception for MULX in the meantime a sophisticated abstraction for all instructions is implemented?

Note that MULX is a special instruction that brings speedups over the conventional MULQ [2]. So the proposed workaround will impact negatively on the performance.

[1] https://www.intel.com/content/dam/www/public/us/en/documents/white-papers/ia-large-integer-arithmetic-paper.pdf
[2] https://www.intel.com/content/www/us/en/docs/intrinsics-guide/index.html#text=mulx&ig_expand=5245

@randall77
Copy link
Contributor

Is it possible to add this exception for MULX in the meantime a sophisticated abstraction for all instructions is implemented?

It is possible, sure. I'd just rather fix it once and for all instead of doing one-off instruction patches repeatedly. Some other instructions that have the same problem/feature: LEAQ, 3-arg IMUL, MOVLQSX (and other sign extensions), ...

Note that MULX is a special instruction that brings speedups over the conventional MULQ [2]. So the proposed workaround will impact negatively on the performance.

The workaround doesn't need to replace MULX with MULQ. You just need to zero R15 somewhere between the global variable access and the MULX. is that substantially detrimental to performance?

@armfazh
Copy link
Author

armfazh commented Feb 26, 2023

The overhead is not so big considering the CPU issues only micro-instructions for MOVL $0, R15.

In my use case, by applying this workaround, the patches to the current assembler code were reduced. This indicates that the original code is correct, but the compiler emits a false positive.

So I left this ticket open until the systematic solution for all instructions is completed.

Workaround

TEXT ·a2(SB),0,$0-0
    CMPL ·myGlobal(SB), $0
+   MOVL $0, R15
    MULXQ AX, R15, DX
    RET

TEXT ·a3(SB),0,$0-0
    CMPL ·myGlobal(SB), $0
+   MOVL $0, R15
    MULXQ AX, DX, R15
    RET

@mknyszek mknyszek added this to the Backlog milestone Mar 1, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/476295 mentions this issue: cmd/asm: improve detector for incorrect R15 usage when dynamic linking

@github-project-automation github-project-automation bot moved this from Todo to Done in Go Compiler / Runtime Mar 15, 2023
@golang golang locked and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. 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

5 participants