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.

Sign up for GitHub

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: linux/s390x: inlining bug in s390x #64468

Closed
tolsen opened this issue Nov 30, 2023 · 11 comments
Closed

cmd/compile: linux/s390x: inlining bug in s390x #64468

tolsen opened this issue Nov 30, 2023 · 11 comments
Labels
arch-mips arch-ppc64x arch-s390x Issues solely affecting the s390x architecture. 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.22

Comments

@tolsen
Copy link

tolsen commented Nov 30, 2023

Go version

Go 1.21.4

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

GO111MODULE=''
GOARCH='s390x'
GOBIN=''
GOCACHE='/home/tolsen/.cache/go-build'
GOENV='/home/tolsen/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='s390x'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/tolsen/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/tolsen/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/golang/go1.21.4'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/golang/go1.21.4/pkg/tool/linux_s390x'
GOVCS=''
GOVERSION='go1.21.4'
GCCGO='gccgo'
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 -march=z196 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build156610009=/tmp/go-build -gno-record-gcc-switches'

What did you do?

https://go.dev/play/p/d9uIJAd1yEr

What did you expect to see?

ABCDEF12
ABCDEF12

What did you see instead?

ABCDEF12
ABCDEF12000000

( Note that go.dev/play gives the correct answer; presumably because it is not running on Linux/s390x )
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 30, 2023
@tolsen
Copy link
Author

tolsen commented Nov 30, 2023

Putting //go:noinline just before doOpOnStructElems() fixes the problem. Hence why I believe this is in part an inlining problem. Also note that the problem only reveals itself when the struct is first passed as an interface{} into the caller.

@tolsen
Copy link
Author

tolsen commented Nov 30, 2023

Also note that this problem is not present in Go 1.20.11

@tolsen
Copy link
Author

tolsen commented Nov 30, 2023

Btw, s390x is the only big endian platform I have access to. This problem does not occur on the multiple little endian platforms I have tested this on.

In other words, I don't know if this problem affects all big endian platforms or only s390x.

@mauri870
Copy link
Member

Just confirmed it also happens with ppc64

@randall77
Copy link
Contributor

Hm, I'm having trouble seeing the problem here.

GOOS=linux GOARCH=s390x go1.21.4/bin/go build -gcflags=-S ~/gowork/issue64468.go

Produces assembly for main.main that just has 2 identical blocks calling the runtime.print functions with identical args. I don't see how that could possibly generate different print outputs.

Unless there is a bug in print. Or this is a bug in the compiler when run on the platform. I will try to reproduce that way.
Anyone who reproduced, can you also reproduce when cross-compiling from amd64 or arm64?

@prattmic
Copy link
Member

cc @golang/s390x

@tolsen
Copy link
Author

tolsen commented Nov 30, 2023

Hm, I'm having trouble seeing the problem here.

GOOS=linux GOARCH=s390x go1.21.4/bin/go build -gcflags=-S ~/gowork/issue64468.go

Produces assembly for main.main that just has 2 identical blocks calling the runtime.print functions with identical args. I don't see how that could possibly generate different print outputs.

Unless there is a bug in print. Or this is a bug in the compiler when run on the platform. I will try to reproduce that way. Anyone who reproduced, can you also reproduce when cross-compiling from amd64 or arm64?

Not a cross-compile, but IBM has confirmed the bug w/ details on the assembly/ML : https://groups.google.com/g/golang-nuts/c/TYGuzUTTsN8/m/pHwtGqo8AAAJ

@randall77
Copy link
Contributor

I can reproduce on the ppc64 platform. For some reason, inlining happens differently when on platform then when cross compiling. (That's a separate issue which I will investigate - the toolchain should generate identical assembly no matter the platform.)

I think I see the bug, something wrong with the memcombine pass with big-endian and elements bigger than a byte.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 30, 2023
@dmitshur dmitshur added this to the Backlog milestone Nov 30, 2023
@dmitshur dmitshur added the arch-s390x Issues solely affecting the s390x architecture. label Nov 30, 2023
@gopherbot
Copy link

Change https://go.dev/cl/546355 mentions this issue: cmd/compile: fix memcombine pass for big endian, > 1 byte elements

@randall77
Copy link
Contributor

@gopherbot please open a backport issue for 1.21.

This is a pretty serious miscompilation with no easy workaround.

@gopherbot
Copy link

Backport issue(s) opened: #64472 (for 1.21).

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 gopherbot mentioned this issue Nov 30, 2023
Closed
@pmur pmur mentioned this issue Dec 1, 2023
Open
@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 Dec 6, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
arch-mips arch-ppc64x arch-s390x Issues solely affecting the s390x architecture. 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.22
Development

No branches or pull requests
6 participants
@tolsen @prattmic @dmitshur @randall77 @gopherbot @mauri870