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

internal/chacha8rand: tests failing on big-endian builders #64284

Closed
bcmills opened this issue Nov 20, 2023 · 11 comments
Closed

internal/chacha8rand: tests failing on big-endian builders #64284

bcmills opened this issue Nov 20, 2023 · 11 comments
Labels
arch-mips arch-ppc64x arch-s390x Issues solely affecting the s390x architecture. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 20, 2023

#!watchflakes
post <- (goarch == "ppc64" || goarch == "mips" || goarch == "mips64" || goarch == "s390x") && pkg == "internal/chacha8rand"

(Example: https://build.golang.org/log/889a11fa5066e2b265c65416d28270a42f2f1226)

@bcmills bcmills added arch-ppc64x arch-s390x Issues solely affecting the s390x architecture. arch-mips labels Nov 20, 2023
@bcmills bcmills added this to the Go1.22 milestone Nov 20, 2023
@bcmills bcmills changed the title internal/chachaBrand: tests failing on big-endian builders internal/chacha8rand: tests failing on big-endian builders Nov 20, 2023
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Nov 20, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Nov 20, 2023

Note that this is a test of a new API added for Go 1.22 (#61716).

@mknyszek
Copy link
Contributor

CC @rsc @FiloSottile

Perhaps we should disable the tests on these platforms until there's a fix?

@bcmills
Copy link
Contributor Author

bcmills commented Nov 20, 2023

Given that it's a new API and failing on all big-endian platforms, my guess is it's an actual bug in the implementation — I would prefer to roll back the new API until it is fixed, unless there is some compelling reason that #61716 absolutely must land in 1.22.

@mknyszek
Copy link
Contributor

This issue (and #64285) appear to only affect non-first-class ports. The endianness issue seems like a result of some of the type punning done by the internal/chacha8rand package. If we suspect it's a bug on big-endian platforms (and riscv64) only, doesn't that mean it only affects non-first-class ports? I'm a little unclear as to where exactly this falls with respect to the policy. I agree this should be fixed before the release, and is probably not a great reason to call a port "broken" or anything if it's failing, but at least the big-endian issue seems fixable in the short-term.

I'll send out test skips for now to unblock the builders ASAP. If I'm wrong and we want to revert the whole thing I'll do that afterwards. (There will be a conflict to revert fully with the test skips, but since this is new API and there's no other development on it, fixing the conflict will be trivial.) Not trying to make a final decision here by any means.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/543716 mentions this issue: internal/chacha8rand: disable ChaCha8 tests on certain platforms

@mknyszek
Copy link
Contributor

FWIW I also sent out a revert. I'm pretty sure I'm overthinking this.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/543895 mentions this issue: Revert "math/rand/v2: add ChaCha8"

gopherbot pushed a commit that referenced this issue Nov 20, 2023
This reverts commit 6382893.

Reason for revert: Causes failures on big endian platforms and riscv64.
Possibly a bug in the generic implementation.

For #64284.
For #64285.

Change-Id: Ic1bb8533d9641fae28d0337b36d434b9a575cd7e
Reviewed-on: https://go-review.googlesource.com/c/go/+/543895
Reviewed-by: Heschi Kreinick <heschi@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
TryBot-Bypass: Michael Knyszek <mknyszek@google.com>
@bcmills
Copy link
Contributor Author

bcmills commented Nov 20, 2023

If we suspect it's a bug on big-endian platforms (and riscv64) only, doesn't that mean it only affects non-first-class ports? I'm a little unclear as to where exactly this falls with respect to the policy.

I agree that in principle the porting policy would allow us to just notify the big-endian port maintainers and leave those platforms with a known bug.

On the other hand, that wouldn't be very friendly toward those maintainers — especially for a problem as general as endian-sensitivity — and the porting policy encourages us to notify port maintainers of potential problems ahead of time. Since this breakage does not appear to have been expected (no TODOs in the code or mentions of the breakage in the commit message), a rollback seems like an appropriate starting point pending further investigation / discussion.

@laboger
Copy link
Contributor

laboger commented Nov 20, 2023

This looks suspicious in rand_test.go function TestBlockGeneric

seed := *(*[4]uint64)(unsafe.Pointer(&seed))

The seed on the right is a byte array, but then is assigned to a [4]uint64. The order of the bytes in the byte array are going to be in opposite order for big and little endian.

@FiloSottile
Copy link
Contributor

The seed in TestBlockGeneric is arbitrary, but src/internal/chacha8rand/chacha8.go and src/internal/chacha8rand/chacha8_generic.go have a few similar conversions which are probably the issue.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/546020 mentions this issue: math/rand/v2: add ChaCha8

@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 5, 2023
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
This is a replay of CL 516859, after its rollback in CL 543895,
with big-endian systems fixed and the tests disabled on RISC-V
since the compiler is broken there (golang#64285).

ChaCha8 provides a cryptographically strong generator
alongside PCG, so that people who want stronger randomness
have access to that. On systems with 128-bit vector math
assembly (amd64 and arm64), ChaCha8 runs at about the same
speed as PCG (25% slower on amd64, 2% faster on arm64).

Fixes golang#64284.

Change-Id: I6290bb8ace28e1aff9a61f805dbe380ccdf25b94
Reviewed-on: https://go-review.googlesource.com/c/go/+/546020
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@golang golang locked and limited conversation to collaborators Dec 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-mips arch-ppc64x arch-s390x Issues solely affecting the s390x architecture. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
Archived in project
Development

No branches or pull requests

6 participants