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

math/big: Int.Lsh gives wrong results on s390x for n>=128 [1.15 backport] #45335

Closed
gopherbot opened this issue Apr 1, 2021 · 14 comments
Closed

math/big: Int.Lsh gives wrong results on s390x for n>=128 [1.15 backport] #45335

gopherbot opened this issue Apr 1, 2021 · 14 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Milestone
Go1.15.13

Comments

@gopherbot
Copy link
Contributor

@FiloSottile requested issue #42838 to be considered for backport to the next 1.15 minor release.

@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 gopherbot added the CherryPickCandidate Used during the release process for point releases label Apr 1, 2021
@gopherbot gopherbot mentioned this issue Apr 1, 2021
Closed
@gopherbot gopherbot added this to the Go1.15.12 milestone Apr 1, 2021
@toothrot toothrot added the CherryPickApproved Used during the release process for point releases label Apr 1, 2021
@toothrot
Copy link
Contributor

toothrot commented Apr 1, 2021

Approved. This is a serious issue with no workaround.

@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Apr 1, 2021
@cagedmantis
Copy link
Contributor

@jonathan-albrecht-ibm @mdempsky Would you be willing to work on backport cherry-pick CL?

@jonathan-albrecht-ibm
Copy link
Contributor

@cagedmantis yes, I'll try to send it shortly.

@jonathan-albrecht-ibm
Copy link
Contributor

@cagedmantis @toothrot I'm testing the CL locally on release-branch.go1.15 and I found that this change needs to be applied first or the tests won't pass:

https://go-review.googlesource.com/c/go/+/250137

Does that CL need to be backported separately?

@cagedmantis
Copy link
Contributor

@jonathan-albrecht-ibm Yes, the CL needs to be backported separately. Make sure to reference this issue in the commit message.

@jonathan-albrecht-ibm
Copy link
Contributor

@cagedmantis sorry but I'm having problems cherry picking https://go-review.googlesource.com/c/go/+/250137, probably I'm misunderstanding what needs to be done next.

I tried from the Gerrit UI but it said it has merge conflicts so I canceled. I tried from the command line but when I try to codereview mail it is rejected because I'm not the original author which is true (email address <AUTHORS_EMAIL> is not registered in your account, and you lack 'forge author' permission.).

I should have been more clear above so just to avoid any confusion I'm the author of https://go-review.googlesource.com/c/go/+/274442/ which is the CL this issue was opened to backport but https://go-review.googlesource.com/c/go/+/250137 needs to be backported first so the tests in CL 274442 will pass.

@heschi heschi modified the milestones: Go1.15.12, Go1.15.13 May 6, 2021
@gopherbot
Copy link
Contributor Author

Change https://golang.org/cl/319831 mentions this issue: [release-branch.go1.15] math/big: fix TestShiftOverlap for test -count arguments > 1

@cagedmantis
Copy link
Contributor

@jonathan-albrecht-ibm I've created the backport CL for golang.org/cl/250137 which is golang.org/cl/319831.

@gopherbot
Copy link
Contributor Author

Closed by merging 44a6805 to release-branch.go1.15.

gopherbot pushed a commit that referenced this issue May 14, 2021
@SparrowLii @cagedmantis
[release-branch.go1.15] math/big: fix TestShiftOverlap for test -coun…
44a6805 
…t arguments > 1

Don't overwrite incoming test data.

The change uses copy instead of assigning statement to avoid this.

For #45335

Change-Id: Ib907101822d811de5c45145cb9d7961907e212c3
Reviewed-on: https://go-review.googlesource.com/c/go/+/250137
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
(cherry picked from commit 41bc0a1)
Reviewed-on: https://go-review.googlesource.com/c/go/+/319831
Run-TryBot: Carlos Amedee <carlos@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Carlos Amedee <carlos@golang.org>
Reviewed-by: Jonathan Albrecht <jonathan.albrecht@ibm.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
@cagedmantis
Copy link
Contributor

@jonathan-albrecht-ibm You should be able to proceed with the backport CL now.

@cagedmantis cagedmantis reopened this May 14, 2021
@gopherbot
Copy link
Contributor Author

Change https://golang.org/cl/320169 mentions this issue: [release-branch.go1.15] math/big: remove the s390x assembly for shlVU and shrVU

@jonathan-albrecht-ibm
Copy link
Contributor

Thanks @cagedmantis, I've cherry picked the CL.

@cagedmantis
Copy link
Contributor

This should not be closed yet. It's been closed because of a bug in gopherbot #29599.

@cagedmantis cagedmantis reopened this May 17, 2021
gopherbot pushed a commit that referenced this issue May 20, 2021
@jonathan-albrecht-ibm @dmitshur
[release-branch.go1.15] math/big: remove the s390x assembly for shlVU…
9636878 
… and shrVU

The s390x assembly for shlVU does a forward copy when the shift amount s
is 0. This causes corruption of the result z when z is aliased to the
input x.

This fix removes the s390x assembly for both shlVU and shrVU so the pure
go implementations will be used.

Test cases have been added to the existing TestShiftOverlap test to
cover shift values of 0, 1 and (_W - 1).

Fixes #45335

Change-Id: I75ca0e98f3acfaa6366a26355dcd9dd82499a48b
Reviewed-on: https://go-review.googlesource.com/c/go/+/274442
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Trust: Robert Griesemer <gri@golang.org>
(cherry picked from commit b1369d5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/320169
Trust: Carlos Amedee <carlos@golang.org>
Run-TryBot: Carlos Amedee <carlos@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@dmitshur
Copy link
Contributor

Closed by merging 9636878 (CL 320169) to release-branch.go1.15.

@golang golang locked and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge
Projects
None yet
Milestone
Go1.15.13
Development

No branches or pull requests
6 participants
@toothrot @cagedmantis @dmitshur @gopherbot @heschi @jonathan-albrecht-ibm