fix(simd): correct big-endian bitmask in scalar v128 fallback#88
Merged
Conversation
Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
satyamg1620
force-pushed
the
endianess-fix
branch
from
d76edb1 to
94af0f5
Compare
Contributor
Author
|
@Brooooooklyn, can you please review this PR |
The NEON `bitmask()` builds the escape mask with `vreinterpretq_u16_u8` + `vshrn_n_u16`, packing one nibble per lane into a u64. On big-endian (aarch64_be) the u16 reinterpret reverses the two byte-lanes inside each pair, so `vshrn` emits swapped nibbles; `NeonBits::as_little_endian`'s `swap_bytes()` only fixed the byte order, leaving every lane pair (2i, 2i+1) transposed and `clear_high_bits` operating on non-canonical bits — the same class of wrong-lane escapes / underflow the v128 fallback had, but on the NEON path. Canonicalize at construction in `NeonBits::new` (reverse all 16 nibbles on big-endian) so every `NeonBits` is `lane i -> nibble i`, mirroring the v128 fix; `as_little_endian` then becomes a no-op clone. Little-endian is unchanged. Verified on real aarch64_be under qemu: the differential stress test passes through the NEON kernel, and the LE NEON path still passes. Signed-off-by: LongYinan <lynweklm@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`-C target-cpu=native` makes LLVM emit AVX-512-class instructions unconditionally, based on whatever CPU the assigned CI runner exposes. CodSpeed runs the benchmark under Valgrind (simulation mode), which cannot execute those instructions, so the process dies with SIGILL (exit 132) whenever CI lands on a newer runner. This is flaky by construction and unrelated to any source change. Cap the ISA at x86-64-v3 (AVX2 baseline): deterministic, within what Valgrind supports, and still exercises the AVX2 escape kernel. Signed-off-by: LongYinan <lynweklm@gmail.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Merged
IWANABETHATGUY
pushed a commit
to rolldown/rolldown
that referenced
this pull request
Jul 10, 2026
…ix (#10211) ### What this solves Bumps `json-escape-simd` to `3.1.1`, which fixes JSON string escaping on big-endian targets (s390x). The crate's portable SIMD fallback (used on s390x — no SSE2/AVX/NEON) built its lane bitmask with a bit order inconsistent with how the first-escape offset was read back. On big-endian this corrupted escaped output — e.g. `"node:module"` → `"node:modle\0\4…"` — breaking sourcemaps, ESM import paths, and asset/data-URL escaping. In debug builds it panics (`attempt to subtract with overflow` in `json-escape-simd`); in release it silently corrupts output. It reaches rolldown two ways, both fixed by this single bump: - **directly** — `crates/rolldown/src/ecmascript/format/esm.rs`, `parse_to_ecma_ast.rs` - **transitively** — `oxc_sourcemap` → `json-escape-simd` Root cause fixed upstream in napi-rs/json-escape-simd#88 and released as `3.1.1` (the earlier `3.1.0` is buggy, so the requirement is pinned to `3.1.1`). ### Alternatives explored Interim we vendored the crate and used `[patch.crates-io]` (first a local copy, then a fixed fork). This PR drops all of that in favour of the published `3.1.1`. ### For reviewers - The bug only manifests on **big-endian**, so existing tests don't fail on x86 CI. - rolldown CI currently **cross-builds** s390x but never runs the suite on big-endian — which is why this shipped undetected. ### Tests The existing `utils::render_ecma_module::tests::*` (sourcemap) tests already cover this — they **panic without this bump and pass with it when run on s390x**. No new test is added because the failure is not reproducible on x86 CI (byte-order specific). Verified natively on s390x: `cargo test --workspace --exclude rolldown_binding` → previously-panicking tests pass, 1805 passed. --------- Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
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
2 participants
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.
The v128 fallback (used on targets without a dedicated SIMD kernel, e.g. s390x) built its escape bitmask with reversed bit order on big-endian and first_offset applied swap_bytes — reordering bytes, not bits — so trailing_zeros landed on the wrong bit, causing subtract-overflow panics and wrong-lane escapes. The bitmask is computed in software and has no byte order, so use one canonical layout (lane i -> bit i) on all targets.
Also allow(dead_code) on BitMask items used only by the wide-SIMD kernels, which are dead on scalar-only targets and broke clippy -D warnings there.