Repository navigation
Conversation
|
The fix for this issue is actually much simpler than this. Detailed in #468, the solution is to make the initial values different for the left and right lanes. In my own code I used the first 8 constants from the BLAKE hash function instead of just the first four: // Pinched these constants from the BLAKE implementation
__m128i v0 = _mm_set_epi64x(0x6a09e667f3bcc908ULL, 0xbb67ae8584caa73bULL);
__m128i v1 = _mm_set_epi64x(0x3c6ef372fe94f82bULL, 0xa54ff53a5f1d36f1ULL);
__m128i v2 = _mm_set_epi64x(0x510e527fade682d1ULL, 0x9b05688c2b3e6c1fULL);
__m128i v3 = _mm_set_epi64x(0x1f83d9abfb41bd6bULL, 0x5be0cd19137e2179ULL);See for the values: https://en.wikipedia.org/wiki/BLAKE_(hash_function)#Initialization_vector |
Author
|
That is a simpler solution. Closing this PR, and will look for the custom hash function pointer to eventually land. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Clay__HashDatacan produce colliding (and in the worst case, uniformlyzero) hashes for legitimate, differently-sized inputs. This causes Clay's measurement cache to serve stale/wrong measurements for unrelated text, which leads to wrong text element layout.Reproduction
(SIMD/x86_64): feed any repeating pattern of 8 bytes like
0123456701234567into the hash function.Clay__HashDatareturns0as the hash for the different-length strings, causing the text-measurement cache to hand back a stale, wrong-width measurement.Root cause
Both SIMD paths (x86 SSE, ARM NEON) mix input in two 64-bit lanes using identical per-lane
add-rotate-xorops, with no cross-lane mixing, combining lanes only at the end viaresult[0] ^ result[1]. Any input whose 16-byte chunks have equal low/high 8-byte halves — true for any 8-byte-periodic repeating pattern — keeps both lanes bit-identical throughout, so the final XOR is always exactly0, regardless of pattern length or repeat count.The scalar fallback uses an unrelated single-accumulator loop with no lane structure, doesn't reproduce this collision, and is left unchanged.
Fix
Both SIMD paths (x86 and ARM NEON) now mix in the original input length as one additional block immediately before finalization, split asymmetrically across the two lanes (
{length, ~length}).Verification
Two standalone C programs were run to verify, with the following output.
Caveat
I don't have ARM hardware to run the real compiled NEON path; the fix there was validated against a scalar simulation run in the test files.