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

math/big: Int.Lsh gives wrong results on s390x for n>=128 #42838

Closed
mdempsky opened this issue Nov 25, 2020 · 11 comments
Closed

math/big: Int.Lsh gives wrong results on s390x for n>=128 #42838

mdempsky opened this issue Nov 25, 2020 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@mdempsky
Copy link
Member

The dev.regabi branch has switched cmd/compile to using go/constant to represent compile-time constants, and it appears to be causing one of package math's unit tests to fail only on s390x: https://build.golang.org/log/0af0cbb9e4d7754d6f16f93a8ea452e265037a40

--- FAIL: TestPow10 (0.00s)
    all_test.go:2840: Pow10(200) = 3.027621884171472e+200, want 1e+200
FAIL
FAIL	math	0.004s

/cc @mundaym @griesemer

@mdempsky mdempsky added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Nov 25, 2020
@mdempsky mdempsky added this to the Go1.17 milestone Nov 25, 2020
@mdempsky
Copy link
Member Author

At least when cross-compiling from amd64 with GOARCH=s390x, math/pow10.go seems to compile identically before and after.

@gopherbot
Copy link

Change https://golang.org/cl/273271 mentions this issue: go/constant: add test for #42838

@mdempsky
Copy link
Member Author

It looks like math/big.Rat.SetString is misbehaving on s390x. For SetString("1e192"), it's producing "3027621884171471837698311284263877940814570137795579187554254752777931419931368381990738248285955635616430044818426562647473833981586768834462721163780572037404681705524261520844063170669576192/1".

@mdempsky mdempsky changed the title math: Pow10 giving bad results on s390x on dev.regabi branch math/big: Rat.SetString broken on s390x Nov 25, 2020
@mdempsky
Copy link
Member Author

In hex, 1e192 should be (big-endian, 64 bits per line):

381c3de34e49d55a
a16ef894fd1ec505
c3c46289d6388cec
73add001e6a2cf4c
fea73c80f1b8a046
769dbb7e6412e125
a9e17e1fac815d01
0000000000000000
0000000000000000
0000000000000000

But the incorrect value that s390x is producing is:

a9e17e1fac815d01
fea73c80f1b8a046
769dbb7e6412e125
a9e17e1fac815d01
fea73c80f1b8a046
769dbb7e6412e125
a9e17e1fac815d01
0000000000000000
0000000000000000
0000000000000000

Notably, the last 6 words are correct, but then the first four words are words 5--7 repeating in cycle. It reminds me of the kind of data corruption you get when you use memcpy() instead of memmove() for overlapping data.

@mdempsky
Copy link
Member Author

More reduced test case. Fails on s390x, but passes with -tags=math_big_pure_go:

func TestIssue42838_2(t *testing.T) {
	i := new(big.Int)
	i.SetString("159309191113245227702888039776771180559110455519261878607388585338616290151305816094308987472018268594098344692611135542392730712890625", 0)
	i = i.Lsh(i, 192)
	got := i.String()
	want := "1" + strings.Repeat("0", 192)
	if got != want {
		t.Errorf("got %v, want %v", got, want)
	}
}

@mdempsky
Copy link
Member Author

Notably, changing i = i.Lsh(i, 192) to i = i.Lsh(new(big.Int).Set(i), 192) also makes it pass. So s390x's shlVU implementation appears not to handle when the x and z arguments alias.

@mdempsky mdempsky modified the milestones: Go1.17, Go1.16 Nov 26, 2020
@mdempsky mdempsky changed the title math/big: Rat.SetString broken on s390x math/big: Int.Lsh gives wrong results on s390x for n>=128 Nov 26, 2020
@mdempsky mdempsky 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 Nov 26, 2020
@mundaym mundaym self-assigned this Nov 26, 2020
@mundaym
Copy link
Member

mundaym commented Nov 26, 2020

Thanks for the detailed bug report @mdempsky. The problem is a forward copy in shlVU that is performed when the shift amount is 0. A member of my team is preparing a fix. Most likely we will simply remove the assembly implementation since compiler is generating pretty good code for shlVU_g these days.

@griesemer
Copy link
Contributor

@mundaym You may also want to verify that shrVU is doing the right thing (or likewise use the Go implementation).

@gopherbot
Copy link

Change https://golang.org/cl/274442 mentions this issue: math/big: remove the s390x assembly for shlVU and shrVU

@FiloSottile
Copy link
Contributor

@gopherbot please backport to Go 1.15. This was included in the latest release, but remains an issue without workaround in the previous one.

@gopherbot
Copy link

Backport issue(s) opened: #45335 (for 1.15).

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

@golang golang locked and limited conversation to collaborators Apr 1, 2022
@rsc rsc unassigned mundaym Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants