Repository navigation
AOT: stack traces as in bun run, code from disk, workers, embedded files, module mocks, build cost - #48478
AOT: stack traces as in bun run, code from disk, workers, embedded files, module mocks, build cost#48478Jarred-Sumner wants to merge 12 commits into
Conversation
…les, module mocks, build cost On 89452d2, the merge of #48436. 2,584 files, +57,669 -2,633: scripts 1 file +1 -1 . src 295 files +12,098 -1,957 . test 2,284 files +45,549 -664 . docs 3 files +20 -10 . packages 1 file +1 -1. Pins the preview build of oven-sh/WebKit#1028. [skip size check], as in 607519a. Executables and builds - Stack traces name a source by its path relative to the folder of the build, as in `./src/lib/inner.ts:2:13` instead of `/$bunfs/root/lib/inner.ts`, and report the positions that `bun run` reports. What code looks files up by, such as `getFileName()` and import.meta, stays. - A build that compiles ahead of time refuses to write its executable over a file of its program (`--outfile`, `bun compile`, `compile.outfile`). - An entry point's own import statements always run, whatever "sideEffects" says: the command line tools of uuid and vitest printed nothing. - With `--aot-allow-code-from-disk`: files from disk find files on disk before compiled ones in packages that they ask for by name, `new Function("module", "return import(module)")` loads, and the library of evaluated code is kept. Found by postcss, @babel/cli, depcheck, knex, webpack and prettier. - import() of a data: URL that the program writes is part of the executable, also under `--disallow-code-generation-from-strings=strict`. - CommonJS: an ES module that imports a barrel runs its getters as `bun run` does, require() of a file whose `module.exports` is a getter runs it, a module loaded by a fresh require() names its parent. - Workers: files that URLs name are found beside any use of import.meta, a start that leads to no file warns (TS100082), a worker that runs the entry file has import.meta.main false, and a file that imports what cannot be resolved warns and reports the error event instead of stopping the build. - `--asset`: a folder outside the root that shares its base name with a folder below it is merged with it. Reads that an asset cannot answer warn (TS100087). A read through a symbolic link inside an --asset folder is warned of (TS100165), and the warning names the link to pass as an asset of its own. On Linux readFile() of an embedded file of at least 1 MB maps its pages. - On Windows an embedded file is found with a trailing separator, over a.txt/../a.txt and with a lower-case drive letter. Three lines of it are compiled on Windows alone and were built by nobody yet. - fs.fstatSync(), lstat() and open() of embedded files answer alike; below an embedded file they answer ENOTDIR. Plain fstatSync() costs no more than before. - A script without an extension takes octal escapes, and one with a byte order mark before `#!` is read. A comparison such as `process.argv[1] === __filename` is true where both name one file. - --trim: the functions that stand in for `Bun.sql`, `Bun.postgres` and `Bun.SQL` are one small class with one trap instead of proxies with nine. - The build no longer warns about a path that is overwritten before anything reads it. - A very long chain of operators no longer ends the build by a signal: it is checked for the stack. `bun test --aot` - `--isolate` with jsdom and happy-dom: what the build compiled runs in their realms. In 41 test files of a public suite that use jsdom 246 tests pass, 101 before, 249 in `bun test`. - Mocks: chains whose factories take their original, originals that import each other in a cycle, names that a mocked file's module lacks, files that fail behind a mock. A test file that never finishes loading names the mock that it waits for. - Coverage counts stored fields and emitted enums as in `bun test`. Build cost - Less memory: packed layouts and function records, smaller type records and string tables, pages that the front end is done with are given back. Less work: indexes of import sites built once a file, and quadratic cases in unions, enums, barrels and nested patterns. Known issues - With `--aot-harden=proof-checks`, a loop in a loop that ends at a test for `undefined` stops the build. Main builds it. - As on main: in the second pass of `--rerun-each`, an import() that is refused again behind an unrelated mock is rejected with another error object, and a CommonJS test file times out when a mock factory takes the original of a file that threw. In executables, a bare .cjs file loaded with import() runs in sloppy mode when a computed require() can also name it. - A line of a stack trace no longer includes import.meta.path; that is by design. The embedded file lookups on Windows were not run. Not built as one tree: CI's run of this pull request is its first.
|
Updated 10:05 PM PT - Nov 10th, 2026
❌ @Jarred-Sumner, your commit 7e51860 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 48478That installs a local version of the PR into your bun-48478 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
On e7f2347. 1 file, +2 -0: src 1 file +2 -0. macOS and FreeBSD did not compile: `stop_using_statx()` in src/sys/lib.rs uses `SUPPORTS_STATX_ON_LINUX`, which is defined on Linux and Android alone. The function and `IS_FSTATX_DIRECT` have the same condition now, as every use of them.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the new stop_using_statx in src/sys/lib.rs for a macOS/FreeBSD build break — it is gated on cfg(any(target_os = "linux", target_os = "android")) like its callers, so it is not affected — and the simdutf prefix handling in decodeWithReplacement (src/jsc/bindings/AOTTextCoding.cpp:147-156), which advances by utf16_length_from_utf8 of the valid prefix on error, matching the WebKit TextCodecUTF8 pattern. This change spans ~2,584 files and the bug hunt stopped at its budget rather than running dry, so coverage is partial; the build and test suite were not run as part of this review.
Extended reasoning...
The diff is a ~57k-line change across 2,584 files touching the AOT compiler (src/aot, src/sema), JSC bindings, allocator internals (MimallocArena), node:fs/statx paths, and the WebKit dependency pin. It touches memory-safety-sensitive native code and the removed-feature bindings but no auth, crypto, or injection surface. Five confirmed findings are posted inline, and the hunt exited on max_bugs rather than a dry streak, so approval is not possible; this note only records the two candidate issues that were examined and ruled out.
Findings marked 🟡 are optional suggestions and need no follow-up push.
…ct for a refused import, tests On 80771f5. 96 files, +2,668 -1,413: scripts 4 files +45 -1 . src 24 files +1,467 -1,342 . test 68 files +1,156 -70. Pins the preview build of oven-sh/WebKit#1028's commit 348972fa. [skip size check], as in 607519a. - `bun test --aot --rerun-each`: in the second pass, an import() that is refused again behind an unrelated mock is rejected with the same error object as under `bun test`. - BunPlugin.cpp and what calls it: 210 plainer names and shorter comments. No logic changes with the names. Two functions of the engine change their names with them, so this needs 348972fa. - scripts/verify-baseline-static: the three allowlists name PackWordsImpl and UnpackWordsImpl, two Highway functions that are new in this pull request and are called by run-time dispatch alone. The check was red without them. The Windows lines are by analogy with x64. - The counters of a page's units in MimallocArena.rs are 32 bits wide instead of 16, an atomic that the lists for arm64 already name. - A thread that returned the last block of a page while the page's owner laid another block there could clear the new block. Fixed; found in review. - Findings of the lints: ten of mordant and three of clippy. No behavior changes. Three stored records of the lowering are restated, and the lint of names that are compared as text allows the namespaces of hoisted imports. - On macOS a descriptor of an embedded file is known again after its times are set back, and on Windows paths of assets are compared with the separators of the platform. Tests that CI showed to be wrong are corrected (sizes, positions inside built-ins of builds with assertions, tables of three reads, folders without a test, rows of node:fs). Tests of what the engine's commits fix: WebAssembly loads at the end of the memory, stores in loops that only a trap ends, calls behind an entry test, two kinds of loop under `--aot-harden=proof-checks`, lists of zeros. Linux: the four libc signal functions that ask who called them are no longer inlined, so a chained handler gets the mask that the kernel would have set. Plain Bun too: on x64 a WebAssembly load whose low bytes alone are used returned a value at the end of the memory where it should trap, in the optimizing JIT. Fixed. Known issues: in executables, a bare .cjs file loaded with import() runs in sloppy mode when a computed require() can also name it, as on main. A program whose heap is exhausted can hang in the collection that runs without helper threads, as on main.
…ings of the review, tests On 8fa04f2. 93 files, +1,124 -118: scripts 1 file +1 -1 . src 16 files +482 -116 . test 76 files +641 -1. Pins the preview build of oven-sh/WebKit#1028's commit 83468e92. [skip size check], as in 607519a. Plain Bun too, in the optimizing JIT: `f32.demote_f64` of a converted 64-bit integer rounded once where it rounds twice, and a signaling NaN stayed signaling through `f32.demote_f64` of `f64.promote_f32`. Fixed. - Findings of the review: `Bun.sql` of an executable without it throws the error of a Proxy's trap instead of a second one over it. Exception checks of deep equality are in functions, not lambdas. The helpers on paths of embedded files stand in one crate. - Executables: a `require()` whose computed name the build has followed gives way to an `import()` that names the same CommonJS file, so a bare .cjs file that the import loads runs in strict mode as in `bun run`. - Tests of the conversions and of data segments of WebAssembly. Known issues: in executables, a text that grows by adding elements of the list that it is stored into, over many turns, can take gigabytes where `bun run` takes about 60 MB, and with `--aot-harden=proof-checks` a generator that stores into a `Float64Array` in a loop stops the build; main does neither. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main. A program whose heap is exhausted can hang in the collection that runs without helper threads, as on main.
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (1):
- Unresolved: 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| bool isCompiled = sandbox->type() == CompiledObjectType || sandbox->type() == SlimClosureType; | ||
| bool deleted = isCompiled ? sandbox->methodTable()->deleteProperty(sandbox, globalObject, propertyName, slot) : sandbox->deleteProperty(sandbox, globalObject, propertyName, slot); |
There was a problem hiding this comment.
🟡 (optional) In a compiled executable, delete x inside a vm context whose sandbox is a Proxy no longer runs the Proxy's deleteProperty trap, and the target keeps the property; the base AOT build honoured it. NodeVMGlobalObject::deleteProperty at src/jsc/bindings/NodeVM.cpp:1570-1571 now takes the method-table path only for CompiledObjectType and SlimClosureType, and every other sandbox is deleted through the static JSObject::deleteProperty, which bypasses ProxyObject's override. Fix: dispatch through sandbox->methodTable()->deleteProperty for every sandbox whose class overrides deleteProperty (at least ProxyObject and JSGlobalProxy), or for all sandboxes, as the base did when Bun::hasCompiledModules(). Pre-existing in bun run, where the same static call was already used.
Why this was flagged
A program compiled ahead of time calls vm.createContext(new Proxy(target, { deleteProperty(t, key) { ... } })) and then vm.runInContext("delete x", ctx). The delete reaches NodeVMGlobalObject::deleteProperty (src/jsc/bindings/NodeVM.cpp:1558). On the base branch that function used sandbox->methodTable()->deleteProperty whenever Bun::hasCompiledModules() was true, so ProxyObject::deleteProperty ran the trap. After the change, src/jsc/bindings/NodeVM.cpp:1570 sets isCompiled only for sandbox->type() == CompiledObjectType or SlimClosureType, and src/jsc/bindings/NodeVM.cpp:1571 otherwise calls sandbox->deleteProperty(sandbox, globalObject, propertyName, slot), the static JSObject::deleteProperty, which never consults the handler or the target. The user's trap is not invoked and the target's property survives, where the base AOT executable invoked the trap. No other guard covers it: Base::deleteProperty on line 1577 only removes the property from the NodeVMGlobalObject itself.
Verification: Base (src/jsc/bindings/NodeVM.cpp:1569) dispatched every sandbox through sandbox->methodTable()->deleteProperty in any compiled executable, so ProxyObject::deleteProperty ran the deleteProperty trap. Head (src/jsc/bindings/NodeVM.cpp:1570-1571) sets isCompiled only for CompiledObjectType or SlimClosureType; for a Proxy sandbox the static JSObject::deleteProperty runs instead, so the trap never runs and the target keeps the property.
On dd4008c. 69 files, +301 -5: scripts 1 file +1 -1 . src 3 files +24 -4 . test 65 files +276 -0. Pins the preview build of oven-sh/WebKit#1028's commit 33c7b694. [skip size check], as in 607519a. A file of at least 1 MB is embedded at a multiple of 16 bytes, so that the Buffer of `readFile()` is aligned as before. Tests of texts that grow by long additions, of lists beside stores into views, of mocks below originals, and of an import() after one whose graph failed to link. Known issues: in executables, with `--aot-harden=proof-checks` a loop over a list of the file that calls a function which keeps the list, and a coroutine that passes a parameter on, can stop the build; main builds them. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main.
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (2):
- Unresolved: 1 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| fn offset_in_executable(bytes: &[u8]) -> Option<usize> { | ||
| let mut path = bun_paths::path_buffer_pool::get(); | ||
| let length = crate::readlink(bun_core::zstr!("/proc/self/exe"), &mut path[..]).ok()?; | ||
| let executable = &path[..length]; | ||
| let begin = bytes.as_ptr().addr(); | ||
| let end = begin + bytes.len(); | ||
| let maps = crate::File::read_from(crate::Fd::cwd(), b"/proc/self/maps").ok()?; | ||
| bun_core::strings::split(&maps, b"\n").find_map(|line| { | ||
| let mut fields = line.splitn(6, |&byte| byte == b' '); | ||
| let mut range = bun_core::strings::split(fields.next()?, b"-").map(parse_hex); | ||
| let low = range.next()??; | ||
| let high = range.next()??; | ||
| let protection = fields.next()?; | ||
| let offset = parse_hex(fields.next()?)?; | ||
| let (_device, _inode) = (fields.next()?, fields.next()?); | ||
| let file = fields.next()?.trim_ascii_start(); | ||
| (low <= begin && end <= high && protection == b"r--p" && file == executable) | ||
| .then(|| offset + (begin - low)) | ||
| }) |
There was a problem hiding this comment.
🟡 (optional) On Linux, every fs.readFileSync of an embedded file of 1 MiB or more in an --aot executable now reads and parses the whole /proc/self/maps, which the base's single memcpy never did. embedded_file_buffer (src/runtime/node/node_fs.rs:538-544) calls private_copy::map on each call, and map calls offset_in_executable (src/sys/private_copy.rs:40-58), which readlinks /proc/self/exe, reads all of /proc/self/maps and scans every line; nothing caches the answer. Fix: resolve an embedded file's offset in the executable once (per File, or from the ImageFile descriptor and offset the graph already keeps) and reuse it on every read, keeping the copy fallback for bytes outside an r--p mapping.
Why this was flagged
A program compiled with --aot on Linux calls fs.readFileSync(path) (or fs.readFile) with no encoding on an embedded file whose size is at least MIN_MAPPED_BYTES = 1 << 20 (src/sys/private_copy.rs:6); read_file_in_graph (src/runtime/node/node_fs.rs:552-562) hands the bytes to embedded_file_buffer (node_fs.rs:538-549), which calls sys::private_copy::map(contents) on every call. map (private_copy.rs:12-31) calls offset_in_executable (private_copy.rs:40-58), which readlinks /proc/self/exe, reads the entire /proc/self/maps into a Vec and parses each line until it finds the r--p mapping of the executable, then opens /proc/self/exe again and mmaps. No field of File, Graph or a static caches the offset, so the maps scan repeats on every read of the same file. A JSC-based process has hundreds to thousands of VMAs, so generating and parsing /proc/self/maps costs on the order of a millisecond per call, where the base branch (contents.to_vec(), node_fs.rs:545-548) did one 1 MiB memcpy of tens of microseconds.
Verification: On Linux, an --aot program calls fs.readFileSync/readFile with no encoding on an embedded file of at least MIN_MAPPED_BYTES. src/runtime/node/node_fs.rs:538-544 calls sys::private_copy::map(contents) on every call; map calls offset_in_executable (src/sys/private_copy.rs:40-58), which reads /proc/self/maps (line 46). Nothing caches the offset. The base path was contents.to_vec(), a single memcpy.
…ts of the engine's fixes On 793f226. 50 files, +465 -7: scripts 1 file +1 -1 . src 3 files +13 -5 . test 46 files +451 -1. Pins the preview build of oven-sh/WebKit#1028's commit 4f3f6314. [skip size check], as in 607519a. - A worker of a compiled program frees what its functions of node:fs made: CI's leak check reported 16 bytes a worker. - `bun build --compile --aot --compile-inspector` over `export *` no longer stops the build. - Tests of two bounds in a row, of values that a class lays over a member of a constructor function, of loops that compare elements with numbers, of mocked files on one ring of imports, and of builds under `--aot-harden=proof-checks`. One test no longer asks for an order that `bun test` does not keep. Known issues: with `--aot-harden=proof-checks` a method that is replaced by that of another class stops a build; main builds it. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 3 findings from earlier reviews are still open above.
Still open from earlier reviews (3):
- Unresolved: 2 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
On e9687e9. 7 files, +123 -1: scripts 1 file +1 -1 . test 6 files +122 -0. Pins the preview build of oven-sh/WebKit#1028's commit 5473c680. [skip size check], as in 607519a. A variable that a function captures and that a loop of several versions asks for could stop the build: `const f = make(1);` and a counted loop that sums what `f(a, b)` gives. Fixed. Two stored programs: of captured variables that counted loops sum, and of results of calls that are unsigned integers. Four stored programs of CommonJS and ES modules that load each other are compared with stored outputs, since `bun run` itself does not always give the same. Known issues: in `bun test --aot`, a test file that mocks, with its original, a module of its own running graph, beside another such mock in a helper, can end with "never finished loading"; main does not. With `--aot-harden=proof-checks` a method that is replaced by that of another class stops a build, and a replaced generator method that is stepped by hand and some loops over lists of numbers can stop one; main builds all three. In `bun test --aot`, with `vi.doMock()`, a factory that imports its own file and `vi.importActual()`, and with a mock of a file that a package has imported before, the answer is wrong as on main, without main's notice. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main. With the React Compiler in the same build, a callback that isn't memoized can be left out of the executable, and calling it ends the program, as on main.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 3 findings from earlier reviews are still open above.
Still open from earlier reviews (3):
- Unresolved: 2 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
…omponents On 0e96f1b. 15 files, +188 -32: src 4 files +107 -32 . test 11 files +81 -0. Pins the same preview build, of oven-sh/WebKit#1028's commit 5473c680. [skip size check], as in 607519a. With the React Compiler in the same build, a callback that isn't memoized could be left out of the executable, and calling it ended the program. Fixed. With the React Compiler, a component beside an `export *` could stop the build. Fixed. Two more stored programs of CommonJS and ES modules are compared with stored outputs, since `bun run` itself does not always give the same. Known issues: in `bun test --aot`, a test file that mocks, with its original, a module of its own running graph, beside another such mock in a helper, can end with "never finished loading"; main does not. With `--aot-harden=proof-checks` a method that is replaced by that of another class stops a build, and a replaced generator method that is stepped by hand and some loops over lists of numbers can stop one; main builds all three. In `bun test --aot`, with `vi.doMock()`, a factory that imports its own file and `vi.importActual()`, and with a mock of a file that a package has imported before, the answer is wrong as on main, without main's notice. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main. A build's time doubles with each conditional join onto one field that a file read uses; main's does not.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 3 findings from earlier reviews are still open above.
Still open from earlier reviews (3):
- Unresolved: 2 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
On 01881b8. 12 files, +90 -8: src 2 files +57 -7 . test 10 files +33 -1. Pins the same preview build, of oven-sh/WebKit#1028's commit 5473c680. [skip size check], as in 607519a. With the React Compiler, a file that is imported only for code that the compiler drops did not run. Fixed. A stored program of how builds grow passes; its Todo line goes out. The tests of tsconfig globs compare fewer paths under Miri, where they took hours. Known issues: in `bun test --aot`, a test file that mocks, with its original, a module of its own running graph, beside another such mock in a helper, can end with "never finished loading"; main does not. With `--aot-harden=proof-checks` a method that is replaced by that of another class stops a build, and a replaced generator method that is stepped by hand and some loops over lists of numbers can stop one; main builds all three. In `bun test --aot`, with `vi.doMock()`, a factory that imports its own file and `vi.importActual()`, and with a mock of a file that a package has imported before, the answer is wrong as on main, without main's notice. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main. A build's time doubles with each conditional join onto one field that a file read uses; main's does not.
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Still open from earlier reviews (3):
- Unresolved: 2 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
| const hash = (bytes: Uint8Array) => Bun.hash(bytes).toString(16); | ||
| const first = readFileSync(path), second = readFileSync(path); | ||
| const unchanged = hash(second); | ||
| console.log(first.length, first.byteOffset === 0 || first.buffer.byteLength >= first.length, hash(first) === unchanged); |
There was a problem hiding this comment.
🟡 nit (optional): the second value printed on this line cannot be false, so the "3000001 true true" expectation proves nothing about the mapped buffer's placement. first.byteOffset === 0 || first.buffer.byteLength >= first.length holds for every typed array, since a view never extends past its buffer. Fix: assert a property that the private-copy path can actually get wrong, such as first.byteOffset === 0 alone or first.buffer.byteLength === first.length, so that a Buffer handed out by Buffer::from_private_copy with a wrong offset or length fails the test.
Why this was flagged
The test at test/bundler/compile-aot-run-time.test.ts:210-247 exercises the new private-copy path for embedded files of at least MIN_MAPPED_BYTES (src/runtime/node/node_fs.rs:526-537, src/sys/private_copy.rs:12-31). Its first printed line includes first.byteOffset === 0 || first.buffer.byteLength >= first.length (line 222). For any TypedArray, byteOffset + length <= buffer.byteLength, so the right-hand disjunct is always true and the expression is always true. The expectation "3000001 true true" at line 241 therefore cannot fail on this clause even if from_private_copy (src/jsc/array_buffer.rs:1104-1117) returned a buffer with a wrong byte_len or the JS wrapper was created with a wrong offset. REVIEW.md asks that every assertion be able to fail; this one is vacuous, and the base branch had no such test, so it adds no coverage of the invariant it names.
Verification: nit. Triggering condition: always — the sub-assertion is a tautology for every TypedArray, so it passes regardless of what the private-copy path produces. test/bundler/compile-aot-run-time.test.ts:222 prints first.byteOffset === 0 || first.buffer.byteLength >= first.length; the expectation at line 243 pins it to "3000001 true true". This violates the REVIEW.md rule "Every assertion must be able to fail".
| let (born, recorded) = (now.birthtim, opened.birthtim); | ||
| if born == now.mtim && (born.sec, born.nsec) < (recorded.sec, recorded.nsec) { | ||
| opened.birthtim = born; | ||
| } | ||
| let made = (opened.dev, opened.ino, opened.nlink, opened.birthtim); | ||
| if (now.dev, now.ino, now.nlink, now.birthtim) != made { |
There was a problem hiding this comment.
🟡 nit (optional): on Linux hosts where statx is unavailable (kernel < 4.11 or a seccomp profile that returns EPERM/ENOSYS), fs.fstatSync of an embedded-file fd stops reporting the embedded path's stats after any fchmod/fchown/futimes on that fd, and silently falls back to the temp copy's dev/ino/nlink forever. The identity check at src/standalone_graph/compiled_program.rs:1342-1343 includes birthtim, but the fstat fallback in stat_descriptor (compiled_program.rs:989) gets birthtim from ctime (src/sys/PosixStat.rs:152-163), which every metadata change alters. Fix: make the descriptor identity independent of fields that the non-statx fallback derives from ctime, e.g. compare birthtim only when statx supplied it, so the embedded stat survives fchmod/futimes on every Linux host.
Why this was flagged
An --aot executable on Linux where statx fails (stop_using_statx has cleared SUPPORTS_STATX_ON_LINUX, src/sys/lib.rs:2279 and 2299) calls fs.openSync on an embedded file; open_embedded_path records the fd with graph.add_descriptor (src/runtime/node/node_fs.rs:649). add_descriptor stores stat_descriptor(fd), and with statx off stat_descriptor uses bun_sys::fstat plus PosixStat::init (compiled_program.rs:989), whose birthtim on Linux is stat_ctime (src/sys/PosixStat.rs:152-163). The program then calls fs.fchmodSync(fd, mode) or fs.futimesSync(fd, ...), which changes the temp copy's ctime. The next fs.fstatSync(fd) reaches graph.fstat; at compiled_program.rs:1342-1345 now.birthtim (new ctime) differs from opened.birthtim, so the slot is set to None and the raw stat of the unnamed temp file is returned, and every later fstat of that fd does the same. With statx available, birthtim is the real btime or a constant zero, so the record survives. The base branch always returned the temp copy's stat, so this only undercuts the new feature on those hosts; no other guard re-registers the descriptor.
Verification: On a Linux host where statx(2) fails, stat_descriptor (src/standalone_graph/compiled_program.rs:985-989) takes bun_sys::fstat(fd), whose birthtim is the ctime (src/sys/PosixStat.rs:152-163). fchmod/fchown/futimens bump ctime on the temp file, so the comparison at compiled_program.rs:1342-1344 mismatches, *slot = None, and every later fs.fstatSync(fd) returns the unnamed temp file's stats.
On 087029c. 6 files, +168 -7: scripts 1 file +1 -1 . test 5 files +167 -6. Pins the preview build of oven-sh/WebKit#1028's commit 9ad6efd7. [skip size check], as in 607519a. In a loop, a remainder took the integer behind a sum past 2 ** 53 in place of the rounded number: where y + i reaches 2 ** 53 + 1, (y + i) % 3 was the remainder of 2 ** 53 + 1. Fixed in the engine; one stored program. With `--aot-harden=proof-checks`, a list that is read at its older length behind a change of its storage stopped a build. Fixed in the engine; two stored programs. The reader of Mach-O files that a test uses takes only sections of instructions for code: constants in __TEXT read as instructions. The scaling row of getters that a helper lays off a table is asked at 4,000 targets, where the peak of a build of nothing no longer decides the reading. Known issues: in `bun test --aot`, a test file that mocks, with its original, a module of its own running graph, beside another such mock in a helper, can end with "never finished loading"; main does not. With `--aot-harden=proof-checks` a method that is replaced by that of another class stops a build, a replaced generator method that is stepped by hand can stop one, and so can a test of a list's length against the length before a push() or pop(); main builds all three. In `bun test --aot`, with `vi.doMock()`, a factory that imports its own file and `vi.importActual()`, and with a mock of a file that a package has imported before, the answer is wrong as on main, without main's notice. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main. A build does not end where some eight or more branches of one if / else chain join onto the same field, or some 24 objects' fields are joined twice each from the next one's, and a file read uses the field; main's does.
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 5 findings from earlier reviews are still open above.
Still open from earlier reviews (5):
- Unresolved: 4 minor or pre-existing, 1 blocking on lines changed since (possibly already fixed).
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
…tack traces as in bun run (#1028) The JavaScriptCore side of a round of work on Bun's ahead-of-time compiler (`bun build --compile --aot` and `bun test --aot`, both experimental). Bun's side is oven-sh/bun#48478. This follows #1025. 565 files, +24,718 -4,414: aot/tests 90 files, +3,077 -724; the rest of aot/ 395 files, +18,764 -3,265; outside aot/ 80 files, +2,877 -425. In heap/ one condition and one comment, for compiled images; nothing in WTF or bmalloc, and nothing outside Source/JavaScriptCore. Executables - Two tests of bounds in a row, such as `if (v < 3) { a = 5; continue; } if (v > 9) { b = 7; continue; }`, took the first's arm for both where both arms are short. Fixed. - Stack traces and error messages are closer to `bun run`: positions of constructions, destructuring and `await`, names of methods under computed keys, paths relative to the folder of the build. An async function that awaits itself through `Promise.all` is in a trace once. - Modules: `require()` of an ES module whose graph holds a file that cannot be loaded runs the CommonJS files in front of it. Errors reach every load of a graph. A file that is imported as text and as JSON no longer mixes up its forms. After an imported CommonJS file throws, the next one that an import loads is the main module, as under `bun run`. - A collection of compiled code that has no helper threads could wait for helpers, so a program whose heap is exhausted, or that may not start threads, could hang. The collector's thread marks there itself; plain Bun is unchanged. `bun test --aot` - Mocks: a package module that failed at its first load, chains of mocks whose factories take their original, mocks of files that import each other in a cycle or that fail, `resetModules()` inside a running graph, mocks below originals, an import() after one whose graph failed to link. - `--rerun-each`: in the second pass, an import() that is refused again behind an unrelated mock is rejected with the same error object as under `bun test`. WebAssembly in the image - Frames are shrink-wrapped, the bounds checking register is not saved, direct calls pass the first mutable i32 global in a register, functions of Go's compiler are entered behind the switch on their parameter, an access that a succeeded one covers needs no check, on x64 comparisons read memory. A trap takes no lock. Plain Bun too - On x64 a WebAssembly load whose low bytes alone are used returned a value at the end of the memory where it should trap, in the optimizing JIT. Fixed. - In the optimizing JIT: `f32.demote_f64` of a converted 64-bit integer rounded once where it rounds twice, and a signaling NaN stayed signaling through `f32.demote_f64` of `f64.promote_f32`. Fixed. - arm64 with DotProd: `i32x4.relaxed_dot_i8x16_i7x16_add_s(x, x, c)` in the optimizing JIT multiplied by the addend where the result took x's register. Fixed. - The data segments of a WebAssembly module lie in one buffer, each behind a short header: esbuild.wasm has 98,450 of them. - B3: a Phi that follows its own Upsilon in a block is folded; assertion builds poison stale Value::owner pointers (#986). - Corrected assembler operations that nothing calls with such operands: equal registers, negative immediates and masks, pairs of floats at far offsets, blinded additions, shifts by 32 and more. Small corrections in the x64 assembler and in the register allocator. Speed (instructions, by our own benchmark rows) - Async code and generators: an awaited call of a compiled function continues it in place, 687 to 202 a call; a pipeline of generators is joined, 81 to 35 an element. - Sorting: `sort((a, b) => a - b)` from 65 numbers on goes by digits, 1,286 to 52 an element for 20,000 integers; `sort(comparator)` of typed arrays is compiled. - Strings: substrings, parseInt(), keys of a `Map` that are joined, one character of a string, split(), destructuring, TextEncoder and TextDecoder (3,811 to 1,465 a line). - Loops: lists in fields, rows of rows, nested loops, counters of any kind, views under strides and masks, sums of unknown values, BigInts, remainders, method chains, throwing callees. - Objects: comparisons of one layout, `instanceof`, `Object.hasOwn()` and `Object.keys()` of records, getters and setters of host and class objects, static fields, spread copies, constructor functions below native classes. Results of functions of node:path and node:fs have types. Size and build - Image tables are coded. Constants are compared and loaded in place, and element accesses under a constant index take fewer bytes. - Builds that took quadratic time: chains of classes and interfaces, unions with many tests, barrels with many stars under mocks, nested array patterns. - `--aot-harden=proof-checks` verifies each stage. A frame part is closed under dominance. `Wire::schemaHash` changes, so this engine and Bun's side go together. Known issues, right before this commit - In `bun test --aot`, a test file that mocks, with its original, a module of its own running graph, beside another such mock in a helper, can end with "never finished loading". - With `--aot-harden=proof-checks`, a method that is replaced by that of another class stops a build, a replaced generator method that is stepped by hand can stop one, and so can a test of a list's length against the length before a push() or pop(). As before - `bun test --aot`: with `vi.doMock()`, a factory that imports its own file and `vi.importActual()`, and with a mock of a file that a package has imported before, the answer is wrong as before, without the notice that was given before. A line of a stack trace names the source relative to the folder of the build, so it no longer includes `import.meta.path`; that is by design.
On dfb5a20. 1 file, +1 -1: scripts 1 file +1 -1. Pins the merged commit of oven-sh/WebKit#1028, d837f045. [skip size check], as in 607519a. Known issues: in `bun test --aot`, a test file that mocks, with its original, a module of its own running graph, beside another such mock in a helper, can end with "never finished loading"; main does not. With `--aot-harden=proof-checks` a method that is replaced by that of another class stops a build, a replaced generator method that is stepped by hand can stop one, and so can a test of a list's length against the length before a push() or pop(); main builds all three. In `bun test --aot`, with `vi.doMock()`, a factory that imports its own file and `vi.importActual()`, and with a mock of a file that a package has imported before, the answer is wrong as on main, without main's notice. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main. A build does not end where some eight or more branches of one if / else chain join onto the same field, or some 24 objects' fields are joined twice each from the next one's, and a file read uses the field; main's does. In a stack trace, where the conversion of an operand throws, or a constructor whose object is kept in a variable, the caller's line names the start of the expression and not the operator; main names the operator.
| slot(async () => { | ||
| using dir = tempDir("aot-sentry", program); | ||
| const folder = String(dir); | ||
| await installSDK(folder); |
There was a problem hiding this comment.
🟡 nit (optional), pre-existing pattern: the new test adds a second live bun install of @ sentry/bun from the public registry, so it fails offline and on a registry hiccup. installSDK at test/aot/cli/sentry-source-map.test.ts:118 runs bun install --ignore-scripts against the real registry, and the new test at :327 calls it again in a fresh temp folder. Fix: serve the SDK from a local in-process registry or the container harness (or a vendored tarball) for both tests, so neither contacts an external host.
Why this was flagged
Both tests in test/aot/cli/sentry-source-map.test.ts build a temp folder whose package.json (line 18) depends on "@ sentry/bun": "11.9.0" and then call installSDK (line 118), which spawns bun install --ignore-scripts with no registry override, so it fetches from the public npm registry. The base branch did this once inside build(); the diff extracts it and the new test "without it, events name the sources by their paths from the folder of the build, and their frames are in app" (lines 321-342) calls installSDK a second time at line 327. REVIEW.md asks that tests never contact external network hosts or live registries. On an offline or rate-limited CI runner the expect at lines 120-123 fails for both tests, and the failure has nothing to do with the stack-trace path change the test is meant to cover. No local registry fixture or skip guard is present in the file.
Verification: nit, pre-existing pattern. The public npm registry is unreachable or flaky when the suite runs. test/aot/cli/sentry-source-map.test.ts:118-124 installSDK spawns [bunExe(), "install", "--ignore-scripts"]; neither the folder nor bunEnv overrides the registry, so the install fetches from the live registry. The new test at line 326 calls await installSDK(folder), adding a second live install to the file.
| return JSC::jsEmptyString(vm); | ||
| if (bytes.size() > String::MaxLength) |
There was a problem hiding this comment.
🟡 nit (optional): a compiled program that decodes a very large ASCII buffer with TextDecoder now aborts the process when the string cannot be allocated, where the base throws a catchable error. tryDecodeUTF8 at src/jsc/bindings/AOTTextCoding.cpp:208 builds the result with String(byteCast(bytes)), whose StringImpl::create path uses fastMalloc and crashes on failure. The 16-bit branch of the same function already uses StringImpl::tryCreateUninitialized and returns nullptr to fall back. Fix: make every allocation in the fast path fallible, e.g. use StringImpl::tryCreateUninitialized for the ASCII branch and the small 16-bit branch too, returning nullptr so the generic decode throws its usual error.
Why this was flagged
The trigger is a TextDecoder.decode(view) call in an --aot executable where view is a Uint8Array of all-ASCII bytes and the process is near its memory limit, so the string allocation fails. At src/jsc/bindings/AOTTextCoding.cpp:208 the ASCII case returns JSC::jsString(vm, String(byteCast(bytes))); WTF::String's span constructor calls StringImpl::create, which allocates with StringImplMalloc::malloc (fastMalloc) and aborts the process when the allocation fails. On the base branch the same call reaches TextDecoder::decode_slice at src/runtime/webcore/TextDecoder.rs:339, which uses heap.try_create_latin1_string; that returns Err(memory_allocation_failed()) (src/jsc/bindgen.rs:137-143), so the program gets a catchable error instead of a crash. The large non-ASCII branch at src/jsc/bindings/AOTTextCoding.cpp:216 uses StringImpl::tryCreateUninitialized and returns nullptr to fall back, so the safeguard exists but does not cover the ASCII branch or the 256-byte 16-bit branch at src/jsc/bindings/AOTTextCoding.cpp:211.
Verification: An --aot compiled program calls TextDecoder.decode on an all-ASCII Uint8Array whose allocation fails because the process is near its memory limit. The ASCII case at src/jsc/bindings/AOTTextCoding.cpp:208 allocates with fastMalloc and CRASHes on a null result. The base path, TextDecoder::decode_slice in src/runtime/webcore/TextDecoder.rs line 339, yields a catchable memory_allocation_failed() error.
On 8452024. 2 files, +24 -1: scripts 1 file +1 -1 . test 1 file +23 -0. Pins the preview build of oven-sh/WebKit#1030's commit 3f5b3629. [skip size check], as in 607519a. In a loop, a sum or difference of two values tested only one of them for a boxed int32 where the type of the other says int32: JSON's 3 beside 0.5 is boxed as a double, and `p.a + p.b[0]` was 3 where both are 3. Fixed in the engine; one stored program. Known issues: in `bun test --aot`, a test file that mocks, with its original, a module of its own running graph, beside another such mock in a helper, can end with "never finished loading"; main does not. With `--aot-harden=proof-checks` a method that is replaced by that of another class stops a build. These can stop one: a replaced generator method that is stepped by hand, a test of a list's length against the length before a push() or pop(), a closure with two captured variables that is called in a loop and, beside it, inside a list callback, and `row[i] += factor * b[i]` in a nest of two loops where `factor` is undefined in one call; main builds all five. In `bun test --aot`, with `vi.doMock()`, a factory that imports its own file and `vi.importActual()`, and with a mock of a file that a package has imported before, the answer is wrong as on main, without main's notice. A bare .cjs file loaded with import() runs in sloppy mode when a require() that never runs also names it, as on main. A build does not end where some eight or more branches of one if / else chain join onto the same field, or some 24 objects' fields are joined twice each from the next one's, and a file read uses the field; main's does. In a stack trace, where the conversion of an operand throws, or a constructor whose object is kept in a variable, the caller's line names the start of the expression and not the operator; main names the operator.
Bun's side of a round of work on the ahead-of-time compiler (
bun build --compile --aotandbun test --aot, both experimental). The JavaScriptCore side is oven-sh/WebKit#1028 and #1030. This follows #48436.Merge order
oven-sh/WebKit#1030 merges first;
scripts/build/deps/webkit.tspins its preview until then.What's in it
2,878 files, +62,639 −4,042 against main.
test: stored programs, test runs and testssrc/aot: the compiler's front endsrc/sema: the type checkerdocsA stored program is a small program that CI compiles with
--aot, runs, and compares withbun run. A stored test run does the same withbun test --aotandbun test.If you don't use
--aotsrc/standalone_graph/compiled_program.rs,src/standalone_graph/StandaloneModuleGraph.rsand 7 morefstat(),open()and lookups answer as on disk.src/resolver/resolver.rs,src/resolver/compiled_program.rsand 2 moresrc/jsc/bindings/BunCompiledModules.cpp,src/jsc/bindings/BunCompiledModules.hand 11 moresrc/jsc/bindings/ErrorStackTrace.cpp,src/jsc/bindings/ErrorStackTrace.hand 4 moresrc/jsc/bindings/AOTTextCoding.cpp,src/jsc/bindings/webcore/JSTextEncoder.cppand 10 moreencode(),decode()and five functions ofnode:fs; native dictionaries read compiled objectssrc/jsc/bindings/BunCompiledObjectEquality.h,src/jsc/bindings/RemovedFeature.cppand 4 more--trimremoved;data:URLs;deletein anode:vmcontext over aProxysrc/jsc/bindings/TestEnvironment.cpp--aot --isolatesrc/runtime/cli/compile_command/build.rs,src/runtime/cli/compile_command/failure.rsand 10 morebun compile,--aot-hardensrc/js/ahead-of-time/internal/fs/streams.ts,src/js/ahead-of-time/node/events.tsand 10 moresrc/bun_alloc/MimallocArena.rs,src/bun_alloc/ast_alloc.rsand 8 morescripts/build/deps/webkit.tsscripts/verify-baseline-static/allowlist-x64.txt,scripts/verify-baseline-static/allowlist-x64-windows.txtand 1 moresrc/jsc/bindings/BunSignalActions.cppsrc/runtime/jsc_hooks.rsNodeFS, which only compiled code makesPlain
fstatSync()takes 255.5 M instructions in our loop, 256.7 M on main. Plain Bun too, in the optimizing JIT: on x64 a WebAssembly load whose low bytes alone are used returned a value at the end of the memory where it should trap,f32.demote_f64of a converted 64-bit integer rounded once where it rounds twice, and a signaling NaN stayed signaling throughf32.demote_f64off64.promote_f32. Fixed.Executables and builds
if (v < 3) { a = 5; continue; } if (v > 9) { b = 7; continue; }, took the first's arm for both where both arms are short. Fixed.--compile-inspectoroverexport *stopped the build. Fixed../src/inner.ts:2:13, before/$bunfs/root/inner.ts) and reportbun run's positions.bun build --compile; before, "sideEffects": false could leave it out.export *could stop the build; a file that is imported only for code that the compiler drops did not run. Fixed."sideEffects"says.--aot-allow-code-from-disk, a file from disk finds the files of its packages on disk before compiled ones, andnew Function("module", "return import(module)")loads.require()of a file whosemodule.exportsis a getter runs it, a module loaded by a freshrequire()names its parent.import.meta, a start that leads to no file warns (TS100082), and a worker that runs the entry file hasimport.meta.mainfalse.--asset: a folder outside the root that shares its base name with a folder below it is merged with it. Reads that an asset cannot answer warn (TS100087), as do reads through a symbolic link in its folder (TS100165). On LinuxreadFile()of an embedded file of at least 1 MB maps its pages.bun test --aot--isolatewith jsdom and happy-dom: what the build compiled runs in their realms. In 41 jsdom test files of a public suite 246 tests pass; before 101,bun test249.Build cost
Known issues
bun test --aot--aot-harden=proof-checksrow[i] += factor * b[i]in a nest of two loops wherefactoris undefined in one call, and a getter that switches on a variable of its file that holds a constant.bun test --aotvi.doMock(), a factory that imports its own file andvi.importActual(), and with a mock of a file that a package has imported before, the answer is wrong as on main, without main's notice..cjsfile loaded withimport()runs in sloppy mode when arequire()that never runs also names it.Lines of stack traces no longer include
import.meta.path, by design. The embedded file lookups on Windows were not run.CI
The engine is the preview of
3f5b3629. Nothing was built or run on this tree before the push.