fix(compile): ensure bytecode alignment accounts for section header - #26299
Conversation
|
Updated 8:45 AM PT - Jan 20th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 26299That installs a local version of the PR into your bun-26299 --bun |
WalkthroughReplaces the prior 256-byte alignment approach with a padding-based alignment that ensures (current_offset + padding) % 128 == 120 when embedding bytecode into the standalone executable; adds a regression test validating standalone bytecode compilation, execution, multi-module behavior, and disk-cache handling. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/regression/issue/26298.test.ts`:
- Around line 34-41: The variable buildStdout captured from Promise.all is
unused; update the destructuring to either omit it (e.g., const [, buildStderr,
buildExitCode] = await Promise.all([...]) or destructure only build.stderr and
build.exited) or rename it to _buildStdout to mark it intentionally unused;
apply the same change to the other similar destructuring block that collects
build.stdout, build.stderr, build.exited so there are no unused variable
warnings.
90e72a5 to
110dcb9
Compare
|
Fixed the unused variable warning by changing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/regression/issue/26298.test.ts`:
- Around line 138-141: The test depends on an exact internal JSC diagnostic
string ("[Disk Cache] Cache hit for sourceCode") which is brittle; update the
assertion around exeStderr in test/regression/issue/26298.test.ts to be
resilient: either replace the strict toContain check with a relaxed check (e.g.,
assert that exeStderr contains "Disk Cache" or matches a regex like /Disk
Cache.*Cache hit/), or convert it to an optional/non-fatal verification (log a
warning instead of failing) and add a short comment near the expect for future
maintainers explaining the dependency on an internal JSC message; keep the
existing checks on exeStdout and exeExitCode unchanged (exeStdout, exeExitCode).
110dcb9 to
ce56e85
Compare
|
Updated the disk cache assertion to use a more flexible regex pattern ( |
ce56e85 to
724eb9b
Compare
|
Addressed the review comment about platform-specific alignment:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/regression/issue/26298.test.ts`:
- Around line 114-145: The test currently runs the Bun.spawn build (variable
build) but doesn't assert its stdout/stderr or exit code before running the
produced executable; capture the build outputs (await Promise.all for
build.stdout.text(), build.stderr.text(), and build.exited or await build.exited
then read streams) and add assertions that the build exit code is 0 and
optionally that build.stderr is empty or contains expected messages, referencing
the existing Bun.spawn call that creates build and the outfile variable so
failures during the build are diagnosed before running exe.
724eb9b to
daf731e
Compare
|
Added build step assertions to the third test case - now captures and validates that build stderr is empty and exit code is 0 before running the produced executable, matching the pattern used in the other two tests. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/StandaloneModuleGraph.zig`:
- Around line 432-472: The code advances string_builder.len by padding without
writing bytes, which can leave uninitialized data in the embedded blob; before
bumping string_builder.len use the string_builder.writable() slice at the
current offset (or writable[0..padding] relative to current writable) and memset
or fill it with zeros (or write zero bytes) to ensure deterministic
output—adjust the logic around computing padding and before setting
string_builder.len and aligned_offset (symbols: target_mod, padding,
string_builder.len, writable, aligned_offset) so the padding region is
explicitly zeroed.
The bytecode offset in standalone executables needs to be aligned such that when loaded at runtime, the bytecode pointer is 128-byte aligned. Previously, the bytecode was aligned based on arbitrary memory addresses during compilation (`std.mem.alignInSlice`), which only ensured alignment relative to the compilation buffer's address. However, at runtime, the data starts 8 bytes after the PE/Mach-O section header (which is page-aligned). This caused the bytecode to be misaligned by 8 bytes, leading to crashes in JSC's bytecode cache deserialization on Windows (segfault at `JSC::CachedJSValue::decode`). The fix calculates the bytecode offset to satisfy: (8 + offset) % 128 == 0, ensuring proper alignment regardless of the section's virtual address. Fixes #26298 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
daf731e to
0b34010
Compare
|
Fixed the uninitialized padding bytes issue - now explicitly zeroing the padding region with |
Summary
Fixes bytecode alignment in standalone executables to prevent crashes when loading bytecode cache on Windows.
The bytecode offset needs to be aligned such that when loaded at runtime, the bytecode pointer is 128-byte aligned. Previously, alignment was based on arbitrary memory addresses during compilation, which didn't account for the 8-byte section header prepended at runtime. This caused the bytecode to be misaligned, leading to segfaults in
JSC::CachedJSValue::decodeon Windows.Root Cause
At runtime, embedded data starts 8 bytes after the PE/Mach-O section virtual address (which is page-aligned, hence 128-byte aligned). For bytecode at offset
Oto be aligned:The previous code used
std.mem.alignInSlice()which found aligned addresses based on the compilation buffer's arbitrary address, not accounting for the 8-byte header offset at load time.Changes
src/StandaloneModuleGraph.zig: Calculate bytecode offset to satisfyoffset % 128 == 120instead of usingalignInSlicetest/regression/issue/26298.test.ts: Added regression tests for bytecode cache in standalone executablesTest plan
test/regression/issue/26298.test.tswith 3 test casesHelloWorldBytecodetest passesFixes #26298
🤖 Generated with Claude Code