-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
Note that this is a test of a new API added for Go 1.22 (#61716). |
CC @rsc @FiloSottile Perhaps we should disable the tests on these platforms until there's a fix? |
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. |
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 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. |
Change https://go.dev/cl/543716 mentions this issue: |
FWIW I also sent out a revert. I'm pretty sure I'm overthinking this. |
Change https://go.dev/cl/543895 mentions this issue: |
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>
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. |
This looks suspicious in rand_test.go function TestBlockGeneric
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. |
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. |
Change https://go.dev/cl/546020 mentions this issue: |
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>
(Example: https://build.golang.org/log/889a11fa5066e2b265c65416d28270a42f2f1226)
The text was updated successfully, but these errors were encountered: