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: miscompilation in star-tex.org/x/cmd/star-tex #59432

Closed
sbinet opened this issue Apr 4, 2023 · 13 comments
Closed

cmd/compile: miscompilation in star-tex.org/x/cmd/star-tex #59432

sbinet opened this issue Apr 4, 2023 · 13 comments
Assignees
@dr2chase
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@sbinet
Copy link
Member

sbinet commented Apr 4, 2023

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

$ go version
go version devel go1.21-f62c9701b4 Tue Apr 4 07:17:19 2023 +0000 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/binet/.cache/go-build"
GOENV="/home/binet/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/binet/dev/go/gocode/pkg/mod"
GONOPROXY="gitlab.cern.ch/tile-in-one/tio-go,gitlab.cern.ch/tile-in-one/tio-0021/*"
GONOSUMDB="gitlab.cern.ch/tile-in-one/tio-go,gitlab.cern.ch/tile-in-one/tio-0021/*"
GOOS="linux"
GOPATH="/home/binet/dev/go/gocode"
GOPRIVATE="gitlab.cern.ch/tile-in-one/tio-go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/binet/sdk/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/binet/sdk/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.21-f62c9701b4 Tue Apr 4 07:17:19 2023 +0000"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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 -ffile-prefix-map=/tmp/go-build2001148705=/tmp/go-build -gno-record-gcc-switches"

What did you do?

since 1c783f7, star-tex test is failing:

$> cd star-tex.org/x/tex/cmd/star-tex
$> go test
--- FAIL: TestProcess (0.00s)
    --- FAIL: TestProcess/hello.tex (0.00s)
        main_test.go:43: could not process TeX document: xtex.pasError(-2)
FAIL
exit status 1

ie: it was working with Go-1.18.x, but not anymore starting with Go-1.19.x.

What did you expect to see?

a sucessfully working test.

What did you see instead?

a failing test.

bisecting Go, I've found out the first revision of go test that triggered the failing test.

star-tex is relying on web2go to automatically transpile Donald Knuth's WEB tex.web file to Go.
the result is under x/tex/internal/xtex.
it's machine generated so, not super easy on the eye:

the failure is actually coming from:

looking at 1c783f7 CL, a possible hint at the root cause generating this issue might be in how some of the C-trickeries performed in tex.web are translated into Go, meaning "unsafe":

(perhaps the interplay of unsafe.Pointer and uintptr at L145 and L149 ?)
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Apr 4, 2023
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 4, 2023
@mknyszek mknyszek added this to the Backlog milestone Apr 4, 2023
@mknyszek
Copy link
Contributor

mknyszek commented Apr 4, 2023

CC @golang/compiler

@cherrymui
Copy link
Member

Does the test pass with an older version of Go (which is known to work) with go test -race or go test -gcflags=all=-d=checkptr=1 ? So we can see if the unsafe conversions are okay? Thanks.

@sbinet
Copy link
Member Author

sbinet commented Apr 4, 2023

yes, with 581603c (Go1.18.10) and 7ffc1e4 (one revision before 1c783f7), I get:

$> cd x/tex/cmd/star-tex
$> go version; go test -gcflags=all=-d=checkptr=1; go test -race
go version devel go1.20-7ffc1e47b4 Mon Oct 17 14:53:58 2022 +0000 linux/amd64
PASS
ok  	star-tex.org/x/tex/cmd/star-tex	0.010s
PASS
ok  	star-tex.org/x/tex/cmd/star-tex	0.080s

@sbinet
Copy link
Member Author

sbinet commented Apr 4, 2023 •
edited

interestingly, with today's tip (ecc5ba4), I get:

$> go version; go test -gcflags=all=-d=checkptr=1; go test -race
go version devel go1.21-ecc5ba4611 Tue Apr 4 17:04:36 2023 +0000 linux/amd64
--- FAIL: TestProcess (0.00s)
    --- FAIL: TestProcess/hello.tex (0.00s)
        main_test.go:43: could not process TeX document: xtex.pasError(-2)
FAIL
exit status 1
FAIL	star-tex.org/x/tex/cmd/star-tex	0.003s
PASS
ok  	star-tex.org/x/tex/cmd/star-tex	0.080s

(ie: go test -race succeeds)

it's the same for 1c783f7 (the first "bad" revision):

$> go version; go test -gcflags=all=-d=checkptr=1; go test -race
go version devel go1.20-1c783f7c68 Mon Oct 17 15:11:16 2022 +0000 linux/amd64
--- FAIL: TestProcess (0.00s)
    --- FAIL: TestProcess/hello.tex (0.00s)
        main_test.go:43: could not process TeX document: xtex.pasError(-2)
FAIL
exit status 1
FAIL	star-tex.org/x/tex/cmd/star-tex	0.002s
PASS
ok  	star-tex.org/x/tex/cmd/star-tex	0.060s

@dr2chase
Copy link
Contributor

dr2chase commented Apr 5, 2023

Can confirm that undoing that single CL "fixes" the bug. As they say, hmmmmmmm.

@dr2chase
Copy link
Contributor

dr2chase commented Apr 5, 2023

I wasn't able to write a test case, so shall include the debugging debris here:
This line (1666 in github.com/go-latex/star-tex/internal/xtex/xtex.go)

a = (10 * a) + int32(tex.xord[n]) - 48

compiles to (just before late lower)

b109: ← b107
  v1495 (+1667) = CMPBconst <flags> [9] v2107
  v1895 (+1666) = LEAL4 <int32> v2103 v2103
  v878 (+1666) = LEAL2 <int32> v675 v1895
  v879 (1666) = LEAL2 <int32> [-48] v675 v1895 (a[int32])
EQ v1495 → b137 b144 (1667)

late lower transforms this to

b109: ← b107-
  v1495 (+1667) = CMPBconst <flags> [9] v2107
  v1895 (+1666) = LEAL4 <int32> v2103 v2103
  v1953 (1666) = LEAL2 <byte> v675 v1895
  v878 (+1666) = LEAL2 <int32> v675 v1895
  v879 (1666) = ADDLconst <int32> [-48] v1953 (a[int32])
EQ v1495 → b137 b144 (1667)

After additional optimization, v879 is moved to another block, and its input (v1953) is spilled as a byte:

b109: ← b107-
  v1895 (+1666) = LEAL4 <int32> v2103 v2103 : SI
  v878 (+1666) = LEAL2 <int32> v675 v1895 : R8
  v1953 (1666) = LEAL2 <byte> v675 v1895 : SI
  v1495 (+1667) = CMPBconst <flags> [9] v2107
EQ v1495 → b137 b144 (unlikely) (1667)
b144: ← b109-
  v1810 (1666) = StoreReg <byte> v1953 : .autotmp_185[byte]
...
b105: ← b230-
...
  v1684 (1666) = LoadReg <byte> v1810 : SI
  v879 (1666) = ADDLconst <int32> [-48] v1684 : SI (a[int32])

and the root cause of this is the wrong type used to create the optimized LEA instructions.

@gopherbot
Copy link

Change https://go.dev/cl/482536 mentions this issue: cmd/compile: use correct type in amd64 late-lower rules

@dr2chase
Copy link
Contributor

dr2chase commented Apr 6, 2023

@gopherbot please open the backport tracking issues. This is a severe compiler bug.

This was referenced Apr 6, 2023
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #59467 (for 1.19), #59468 (for 1.20).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link

Change https://go.dev/cl/482815 mentions this issue: [release-branch.go1.20] cmd/compile: use correct type in amd64 late-lower rules

@dr2chase dr2chase reopened this Apr 6, 2023
@dr2chase
Copy link
Contributor

dr2chase commented Apr 6, 2023

Fix CL had a problem, going to just remove the offending rewrite rules instead.

@gopherbot
Copy link

Change https://go.dev/cl/482820 mentions this issue: cmd/compile: remove broken LEA "optimization"

@gopherbot
Copy link

Change https://go.dev/cl/482164 mentions this issue: cmd/compile: remove broken LEA "optimization"

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 24, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Apr 24, 2023
gopherbot pushed a commit that referenced this issue Apr 24, 2023
@dr2chase @gopherbot
[release-branch.go1.20] cmd/compile: remove broken LEA "optimization"
446493f 
CL 440035 added rewrite rules to simplify "costly" LEA
instructions, but the types in the rewrites were wrong and
the code would go bad if the wrong-typed register was spilled.

CL 482536 attempted to fix this by correcting the type in the
rewrite, but that "fix" broke something on windows-amd64-race.

Instead / for-now, remove the offending rewrite rules.

Updates #21735.
Updates #59432.
Fixes #59468.

Change-Id: I0497c42db414f2055e1378e0a53e2bceee9cd5d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/482820
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 6a97a60)
Reviewed-on: https://go-review.googlesource.com/c/go/+/482164
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue May 25, 2023
@dr2chase @bradfitz
[release-branch.go1.20] cmd/compile: remove broken LEA "optimization"
3449b49 
CL 440035 added rewrite rules to simplify "costly" LEA
instructions, but the types in the rewrites were wrong and
the code would go bad if the wrong-typed register was spilled.

CL 482536 attempted to fix this by correcting the type in the
rewrite, but that "fix" broke something on windows-amd64-race.

Instead / for-now, remove the offending rewrite rules.

Updates golang#21735.
Updates golang#59432.
Fixes golang#59468.

Change-Id: I0497c42db414f2055e1378e0a53e2bceee9cd5d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/482820
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 6a97a60)
Reviewed-on: https://go-review.googlesource.com/c/go/+/482164
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
bradfitz pushed a commit to tailscale/go that referenced this issue May 25, 2023
@dr2chase @bradfitz
[release-branch.go1.20] cmd/compile: remove broken LEA "optimization"
4ed3c81 
CL 440035 added rewrite rules to simplify "costly" LEA
instructions, but the types in the rewrites were wrong and
the code would go bad if the wrong-typed register was spilled.

CL 482536 attempted to fix this by correcting the type in the
rewrite, but that "fix" broke something on windows-amd64-race.

Instead / for-now, remove the offending rewrite rules.

Updates golang#21735.
Updates golang#59432.
Fixes golang#59468.

Change-Id: I0497c42db414f2055e1378e0a53e2bceee9cd5d9
Reviewed-on: https://go-review.googlesource.com/c/go/+/482820
Run-TryBot: David Chase <drchase@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 6a97a60)
Reviewed-on: https://go-review.googlesource.com/c/go/+/482164
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dr2chase dr2chase

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
6 participants
@sbinet @mknyszek @dmitshur @dr2chase @gopherbot @cherrymui