Repository navigation
[JSC] AOT: do not bind a require() of an ES module that uncompiled code can import - #988
Conversation
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and did not find any bugs; because it changes which require() sites the AOT pass binds and how Modules::evaluateWithOwnStack settles an import() capability, both of which carry run-time ordering and cost trade-offs the description itself calls out (inspector builds losing bound sites, the +13k-instruction cost per unbound require()), a human look from a JSC/AOT maintainer would still be worthwhile.
What was reviewed:
- Checked that the set callModules excludes matches the module order's set: flow.findsFilesOfProgramByName() is set from filesOfProgramAreLoadedByName || hostRunsFilesOutsideImage (AOTValueFlowResultsBuiltins.cpp:1472), the same expression AOTValueFlowResultsOrder.cpp:307 passes, and flow.unseenCodeCanRun() only differs from the solution's UnseenCodeRuns via a LoadsCodeByName record, which is only appended when letModulesBeLoadedByName/runUnseenCode have already raised UnseenCodeRuns.
- Checked the runtime change: m_records.get() plus takeStatus() goes through ensureRecord(), which leaves an Evaluating self-evaluated module's record unchanged and otherwise moves the table status into the record before the capability is fulfilled or rejected; the throw path is pinned by the two "throws" native tests.
- Checked the new namespace-scope names in AOTModuleProgramTests.cpp against the rest of aot/ for the unified-sources uniqueness rule; runInShell/checkShellOutput only gained a parameter and the new literals and helpers are unique.
Extended reasoning...
The change is two small source edits (8 lines in aot/passes/AOTCallModules.cpp, 5 lines in aot/runtime/AOTModuleEvaluation.cpp) plus 12 IR pass tests and ~720 lines of native shell tests in aot/tests/AOTModuleProgramTests.cpp. It touches no security-sensitive surface (no auth, injection or data-exposure paths), only the AOT compiler's require()-binding decision and the module-evaluation promise settlement. Deferral rather than approval rests on the path being covered by the Source/JavaScriptCore CODEOWNERS entry, on the design trade-off the change makes (every ES-module require() reachable by unseen code now goes through the run time, including all of them in inspector builds), and on the subtlety of the record-status handoff in evaluateWithOwnStack, which I traced but which deserves a maintainer's confirmation. The two candidate issues in the ruled-out list (mismatch for disk-loading builds without eval, and inspector builds losing bound sites) were re-examined: the first does not arise because UnseenCodeRuns is raised whenever a LoadsCodeByName record is produced, and the second is a disclosed cost rather than a correctness defect.
…luates Modules::evaluateWithOwnStack() settles the capability that an import() gave to the root while the root ran. It looked for the record with recordIfExists(), which answers only while the status byte of the module says InRecord. The table keeps the status of a module whose start evaluates it, and of a module without a body that this walk began, so the record that the import() made was not found and the promise stayed pending. evaluateWithOwnStack() now reads the record from m_records. A record with a pending capability takes the status, and the capability is fulfilled or rejected as before. Three native tests run such programs in the shell. Two fail without the change: module-programs/import-of-module-that-require-runs and module-programs/import-of-module-without-body-that-require-runs. The third passes with and without it, because a module that throws gives its record the status: module-programs/import-of-module-that-require-runs-and-that-throws.
…mpiled can name asks the run time In a program that compiles no import(), callModules() makes a require() of an ES module a call of its start. The start ends with one byte store, and no function of the run time sees the end of the evaluation. Code that the build has not compiled (the text of eval or of a Function constructor, a file from disk) may call import() with the name of that module while the require() evaluates it. Where the loader's step for that import() runs before the start returns, CyclicModuleRecord::evaluate() stores a pending capability in the record, and nothing settles it: no then-callback runs, and an error that the module throws is not delivered. callModules() now leaves the require() of such an ES module to the run time where FlowResults::unseenCodeCanRun(). The site is then Modules::evaluateSynchronously(), which ends in Modules::evaluateWithOwnStack(), where the promise is settled. The modules are modulesThatUnseenCodeNames(), the set that the module order of flow reads: a file that the host asks for, unless it is the one file that every realm begins with, and every module where code comes in at any place (CompileOptions::admitsUnseenCode()). Tests: twelve pass tests call-modules-where-*.ir, of which six fail without the change, and seven native tests under module-programs/, of which six fail without it. The seventh, import-by-text-of-entry-point-that-required-module-imports, stays pending as under plain Bun.
|
Preview build of de0f320: |
04a8708 to
de0f320
Compare
There was a problem hiding this comment.
I reviewed the re-pushed version of this PR and found no bugs; because it changes how the AOT run time settles an import() promise and which require() sites stay bound, a human look at the two source commits is still worthwhile.
What was reviewed:
callModules()gate: the set passed tomodulesThatUnseenCodeNames()usesflow.findsFilesOfProgramByName(), which isfilesOfProgramAreLoadedByName || hostRunsFilesOutsideImage— the same argument the module order builds inAOTValueFlowResultsOrder.cpp:307, so the two agree as the description claims.evaluateWithOwnStack():m_records.get(root)plustakeStatus()goes throughensureRecord(), which leaves the record not holding status while the table still says Evaluating for a self-evaluating/body-less root; the throw path then relies on the stack loop or the stored status, which the new-and-that-throwsnative tests exercise but I could not run here.- Unified-sources rule: the new file-static helpers in
AOTModuleProgramTests.cppeither are unique in aot/ or are overloads with distinct signatures of names already used in other test files.
Extended reasoning...
The change touches two source files in the JSC AOT subsystem: ~8 lines in the compiler pass AOTCallModules.cpp that add an isImportedWhileRequired term to the require()-binding decision, and ~5 lines in the runtime AOTModuleEvaluation.cpp that look up the root ModuleRecord directly and take its status before settling a pending top-level capability; the rest is ~1,100 lines of new .ir and native tests. No security-sensitive surface (auth, injection, data exposure) is involved. The bug hunt ran to a dry streak with no findings and the source edits are small, but they sit in module-evaluation and promise-settlement semantics whose correctness depends on run-time status bookkeeping I could only read, not execute, and the prior review on an earlier push could not be read, so a human look rather than approval is the right call.
|
CI on the previous head (
I moved this pull request to draft until that job is explained. |
Needs #961 (its commit is the first commit here) and #987 (the base branch).
Problem
bun build --compile --aotexecutable never settles one kind ofimport(): nothencallback runs. Uncompiled code (new Function,eval) names an ES module that arequire()still evaluates. The build made thatrequire()a direct call.mayLoadTwoGraphsAtOnce()(aot/passes/AOTCallModules.cpp) counts only a compiledimport(). [JSC] AOT: settle the import() of a module that a require() still evaluates #961 settles the promise inModules::evaluateWithOwnStack, which a direct call never enters.Fix
callModulesdoes not bind arequire()of an ES module inmodulesThatUnseenCodeNames(), whereFlowResults::unseenCodeCanRun(). The module order reads the same set ([JSC] AOT: count files that uncompiled code begins by name in the module order and in closure-factory copies #987).testaot.Background
require()is a direct call of the module's start (RequireSitesBound). An unbound one calls the run time, which settles promises at the end.import()runs before the start returns. Else it settles late.mayLoadTwoGraphsAtOnce(): it unbinds every ES-modulerequire()in each program with oneeval.Downsides
require()that is no longer bound calls the run time once. That costs about +13,000 instructions and one cell of 80 or 128 bytes.import()of another module inside a boundrequire()still runs later than in plain Bun.import()(Notes).Notes
Where this comes from
No user reported it. It is the case that #961 names as out of its reach ("planned as a change of its own, on top of this one"). The draft oven-sh/bun#47538 holds a program for it under
// Todo:. I wrote every program below.The branch is #987, then the commit of #961 (
b458ef4113, picked onto0ef2dfd320), then one commit.The program
bun build --compile --aot main.mjs running.mjs --outfile app && ./appPlain Bun and Node 26.3.0 (with
createRequire) printimport of running: fulfilled, ended: trueas the last line. The executable printed every line but that one and ended with exit code 0 (RequireSitesBound 1). With the change it prints the line (RequireSitesBound 0).When the promise stays pending
CyclicModuleRecord::evaluate()stores the capability when the loader's step for theimport()runs. That step runs before the bound start returns in three cases:require()inside the module asks the run time, which runs the steps of the loader before it returns (the program above).drainMicrotasks()ofbun:jsc.require()for itself (process.getBuiltinModule("module").createRequire(url)) of a file whose text callsimport().Where the text only calls
import(), the step runs when the start has returned and the promise settles. Each case has a test.The set, and the one file that stays bound
callModulesasksmodulesThatUnseenCodeNames(program, flow.findsFilesOfProgramByName(), build.options().admitsUnseenCode()), the function that #987 adds for the module order. It holds whatModules::refusesRequestFromHostlets such code name. Where only one of those files runs code or loads a file, every realm begins with it: arequire()of it evaluates nothing, and the site stays a call.That is not "the one entry point". With
new Worker(name), a realm begins with a file that a name finds, and that file mayrequire()the entry point.call-modules-where-a-file-that-a-name-finds-requires-the-entry-point.irand the native testimport-by-text-of-entry-point-that-file-found-by-name-requirespin it.A
require()that asks the run time runs the steps of the loader before it returns, so a textimport()can begin a module inside it. The module order must know the same names, or a compiled read runs before its store. The native testimport-by-text-of-file-found-by-name-while-required-file-found-by-name-runsis built withadmit=FilesRunByHostfor that: it ends with a ReferenceError, not with a segmentation fault.Under
--compile-inspectorthe Bun front end marks every file of the programIsListedInCache, so none was bound. The engine's term (admit=Inspector, every module) also reaches the ES-module faces of CommonJS files, which carry no such mark:RequireSitesBoundgoes from 25 to 17 in one barrel program and from 4 to 3 in three others, in inspector builds only.A build with
--aot-allow-code-from-diskrests on the front end, which marks every fileIsFoundByName.Tests
Engine, release with assertions, x64, on
0ef2dfd320:testaot run aot/tests: 5876 passed, 0 failed, 42 todo. Without the edit ofAOTCallModules.cpp: 5870 passed, 6 failed.testaot native: 3081 passed.module-programs/: 24 passed. Without the edit: 18 passed, 6 failed.tests/passes/call-modules-where-*.ir. Six are twins that stay bound: no text, a text with one entry point, a file that a name finds where no load names a file, a second entry point that runs nothing, a neighbor that no name finds, and a build that refuses code outside the image.import-by-text-of-entry-point-that-required-module-importsexpectspendingtwice, with and without the change. Plain Bun leaves that promise pending (CyclicModuleRecord::evaluate()). [JSC] Settle an Evaluate() of an EVALUATING module when the walk pops its cycle #871 changes that function, and the test must follow it.Bun level, on
0ef2dfd320with #987 and this PR, under oven-sh/bun#47605 (22621ef913, which pins the #982 preview):test/aot/parallel/modules, run bytest/aot/run-parallel.ts: 11 pass and 1 is marked// Todo:(the order below). Without the two PRs 10 fail.require()written asimport.meta.require,module.require,createRequire,import x = require(), in a function and in atry, and texts ofeval, indirecteval,AsyncFunction,node:vmand--aot-allow-code-from-disk.test/aot/tools/same-output.ts(machine code, function by function), engine0ef2dfd320against both PRs: IDENTICAL for 289 programs ofperf/idiomsand 3,126 programs ofclosure-factories,modules,closed-worldandcommonjs.Bun level, earlier runs on the engine at
115632d802(#972) with #961 and the same source patches, under oven-sh/bun#47605 at64ddaea8aa:test/aotfiles with "test-run" in the name (73 files, 538 tests) and the 19 files that setBUN_AOT_TESTS_NAME_WHAT_THEY_USE(132 tests): the same results before and after. Run four at once, 1 file failed before and 2 other files failed after. Each of the three passes alone with both builds.What still differs from plain Bun
import()of another module (another entry point, ornode:fs) inside a boundrequire()of a module that no name finds runs its loader steps after the caller goes on. Plain Bun runs them inside therequire(). Five of the 45 programs show it, for exampleother entry runsbehindmain ends. The repair isflow.unseenCodeCanRun()inmayLoadTwoGraphsAtOnce(), which unbinds every ES-modulerequire()in each program with one text.main.mjs, the one entry point, requiresrunning.mjs, whose text importsmain.mjs: the executable printsimport of main: fulfilled, done: 1, and plain Bun leaves that promise pending. Before and after.One program changes sides.
order-oob3(the crash program of #987 without thenode:fsimport, so itsrequire()was bound) printedmain reads port: 8080 name: service-0andimport of main: fulfilled, which is what Node 26.3.0 prints. It now prints plain Bun's line,import of main: rejected, undefined is not an object (evaluating 'config.port').Measurements
On the engine at
115632d802with #961 and the same source patches, unless a line says another base.--explain=statisticsover 1,219 programs oftest/aot/parallel(all that namerequire, all 85 with more than one entry point): no statistic changes between [JSC] AOT: count files that uncompiled code begins by name in the module order and in closure-factory copies #987 and this PR.RequireSitesBoundis 4,533 before and after, in 554 programs, 22 of them with more than one entry point.test/aot/perf/compare-builds.tson the seven servers oftest/aot/servers/app(56 rows, one request of each route), before against both PRs: 0 slower, 0 faster.jsccounters, assertions on, least of 7 runs, on9f5001ee2c). Instructions to the end of the entry point:require()That is 50 against 13,047 instructions a module in a row, and 20 against 21,220 nested, once per process. For 100 modules:
operationAOTRequireModule0 to 100, cells of 80 bytes 0 to 85, cells of 128 bytes 1 to 16.5869a3b791): 300.5 to 292.5 instructions per call and no call of the run time, because the module has run. In a build where every block is cold (heat=EverythingIsCold) it is 300.5 to 902.5, with one call of the run time per execution.Open
flow.unseenCodeCanRun()inmayLoadTwoGraphsAtOnce()? The second also repairs the order above. It costs one run-time call for each ES-modulerequire()in every program with one text.AbstractModuleRecord::evaluate()that ties the set toModules::refusesRequestFromHost. It must tell this fault from the shape that plain Bun leaves pending, and a wrong one stops assertion builds of valid programs. It is a change of its own, after [JSC] AOT: settle the import() of a module that a require() still evaluates #961 and [JSC] Settle an Evaluate() of an EVALUATING module when the walk pops its cycle #871.import-by-name-of-entry-point-require-runs-settleswithout the// Todo:line that the program has in test(aot): animport()of a module that arequire()still evaluates settles bun#47538.