[robobun]

Problem

• A bun build --compile --aot executable never returns from a store into an object of a ten-line scope chain (Notes). It spins in forEachDescendant (aot/object/AOTMemberGuards.cpp:512).
• noteClassesOfCreatedObjects (aot/valueflow/AOTValueFlowResultsClasses.cpp) refused only an Object.create call site whose base is itself. Two call sites stayed classes, each the base of the other.
• A fuzz source found it (Bump WebKit (oven-sh/WebKit#969 preview): under --aot, Object.create(Object.prototype) makes no object of the class of its site bun#47588). --aot is canary only.

Fix

• A call site is a class only where the row of bases above it ends in a class. Call sites on a ring, and below one, make ordinary objects.
• A constructor function loses a base from the front end that closes a ring. The verifier refuses a ring with a base from flow.
• Verified: 9 new IR text tests fail on the old engine. CI on main: testaot all 8873 passed, 0 failed.
• Self-reviewed: 18 concerns raised, 15 addressed, 1 in part.

Background

• One Object.create() call site makes a class. The first object it is handed becomes the prototype object.
• The call site that made the prototype object gives the expected base. The run time walks these links without a bound.
• Considered a visited set in forEachDescendant: the image keeps the ring and every executable pays.

Downsides

• A ring program that runs today reads an own field there in 29 instructions, was 7.
• Rings that a cast closes stay and still hang (Notes).
• A checking build pays 7,364 more instructions at 1,294 layouts.

Notes

The programs

A scope chain with a factory for function scopes and one for block scopes. bun scope.js prints 1 2 true false. The executable of bun build --compile --aot scope.js never returns from globalScope.x = 1.

function functionScope(parent) { const scope = Object.create(parent); scope.isFunction = true; return scope; }
function blockScope(parent) { const scope = Object.create(parent); scope.isFunction = false; return scope; }
const globalScope = functionScope(null);
const body = blockScope(globalScope);
const inner = blockScope(body);
const fn = functionScope(inner);
globalScope.x = 1;
body.y = 2;
console.log(fn.x, fn.y, fn.isFunction, inner.isFunction);

The smallest one:

function one(below) { const created = Object.create(below); created.a = 1; return created; }
function two(below) { const created = Object.create(below); created.b = 2; return created; }
const first = one(null);
const again = one(two(first));
first.late = 4;

The one that the fuzz source wrote (test/aot/fuzz/places-of-object-create-handed-values-in-every-order.ts of oven-sh/bun#47588). #969 names the hang under "Related" as left for another change.

function make(below) { const created = Object.create(below); created.items = 8; return created; }
function leaf(below) { const created = Object.create(below); created.extra = 1; return created; }
const made = [];
made.push(make(made[0] ?? null));
made.push(leaf(make(null)));
made.push(make(Object.getPrototypeOf(made[made.length - 1] ?? {})));

All three hang on Bun 4f7acde0f with the engine at 4d45755fed, also when built with --aot-validate, and on canary 1.4.4-canary.1+da867c60b. No issue in either repository reports the hang. The fuzz source with seeds 1000 to 1479: 3 of 480 programs hang on the old engine in all four ways that it builds them, and none with the first version of this change.

Where it spins

The first two programs, at the store:

forEachDescendant<ClassTable::didChangeMember::$_0>   AOTMemberGuards.cpp:524
ClassTable::didChangeMember                            AOTMemberGuards.cpp:1337
didChangeClassHolder                                   AOTChainDependents.cpp:252
VM::invalidateStructureChainIntegrity                  VM.cpp:2588
tryCreateField                                         AOTObjectStorage.cpp:427
operationAOTPutByIdAndCache

The third, at the last make():

forEachDescendant<ClassTable::didChangeEveryMember::$_0>   AOTMemberGuards.cpp:526
ClassTable::didChangeEveryMember                            AOTMemberGuards.cpp:1439
ClassTable::disarmMemberGuards                              AOTMemberGuards.cpp:1515
ClassTable::defineClassOfCreatedObjects                     AOTClassTable.cpp:385
operationAOTSetLiteralPrototype                             AOTObjectBuiltinOperations.cpp:443

delete reaches the loop through didChangeMember, Object.setPrototypeOf and Object.freeze through didChangeEveryMember.

The cause

1. giveLayoutsToCreatedObjects gives each Object.create(x) call site a layout with IsOfCreatedObject.
2. The ledger says for each object that such a call site is handed which one class it is the prototype object of: the call site that is handed the fewest objects.
3. noteClassesOfCreatedObjects takes the layout of that object for the base. Its loop took a class back where its base was no class, or was the class itself. In the second program the base of one is two and the base of two is one. Each was the reason for the other to stay.
4. expectedBaseOf() answers classFlow.prototypeBase for such a layout, and encodeLayoutRecords writes expectedBase and the lists of classes below from it.

testaot print a.ir --until=ApplyFlowResults on 4d45755fed: layout L6 .. classFlow={evaluations=AtMostOnce prototypeBase=L7}, layout L7 .. classFlow={evaluations=AtMostOnce prototypeBase=L6}. With this change neither line has a classFlow, and the tables say expectedBase=- for both.

The new loop walks up the row of bases from each call site once and marks what it passes. A row that ends at a class without a base is a row of classes. A row that comes back to a mark of the same walk is a ring, and the walk takes back every call site that it passed. The two marks are two more values of the byte that each layout had.

What decides is the row of bases, not who is handed whose objects. In places-of-object-create-handed-each-others-objects-are-classes-where-the-row-above-them-ends.ir two call sites are handed each other's objects and all three call sites stay classes, before and after.

What a call site on a ring makes now

What a call site makes whose base is itself, or that is handed several objects: restoreCallsOfObjectCreate puts the call of Object.create back, and the objects are ordinary objects. A call site below a ring gets the same (a-place-of-object-create-below-a-ring-of-places-is-no-class.ir).

Constructor functions

function F() {}
class X extends (() => F)() {}
F.prototype = Object.create(X.prototype);
console.log(new F() instanceof X);

Plain Bun prints true. The front end names X as the base of F, and flow proves that X extends F. computeClassResults copied the base as read into prototypeBase, and dropLinkedBasesInCycles passed over a layout that has one. The executable hung in the same loop. The rule now looks at every constructor function: the base that closes the ring is dropped, and the base that flow has proven stays.

Tests

Engine 4d45755fed, release, x64 Linux. "Old" is that engine, "new" is the same with this change.

Test	Old	New
pipeline/places-of-object-create-above-each-other-are-no-classes.ir	fails: classFlow on both layouts, printed nothing for 100 seconds	passes
pipeline/places-of-object-create-in-a-ring-of-three-are-no-classes.ir	fails the same way	passes
pipeline/a-place-of-object-create-below-a-ring-of-places-is-no-class.ir	fails the same way	passes
pipeline/a-place-of-object-create-below-a-ring-of-places-is-no-class-where-its-row-is-walked-first.ir	fails the same way	passes
pipeline/a-constructor-function-named-below-a-class-that-extends-it-has-no-base.ir	fails: prototypeBase, printed nothing for 100 seconds	passes
valueflow/results/base-named-for-a-constructor-function-that-closes-a-cycle-names-no-base-class.ir	fails: prototypeBase	passes
answers/object-create-places-handed-each-others-objects.ir (the fuzz program, closed)	fails: no answer	passes
verifier/invalid-layout-ring-of-classes-of-created-objects.ir	fails: no diagnostic	passes
verifier/invalid-layout-ring-of-bases-that-flow-closes.ir	fails: no diagnostic	passes
pipeline/places-of-object-create-in-a-row-that-ends-below-a-literal-are-classes.ir	passes	passes
pipeline/places-of-object-create-handed-each-others-objects-are-classes-where-the-row-above-them-ends.ir	passes	passes

The ring tests hold STATISTIC: CopyClosureFactoriesChanges = 0: a copy of a maker for each call would be a call site of its own, and there would be no ring to test.

• testaot all on aot/tests, new: 8700 passed, 0 failed, 42 todo.
• testaot run on test/aot/engine-tests of oven-sh/bun, new: 1852 passed, 0 failed, 1 todo.
• test/aot/run-parallel.ts on all 7184 programs of test/aot/parallel in oven-sh/bun, new: 7172 pass, 12 are skipped by their own lines, 0 fail. With --more-flags="--aot-check=verify-each-stage", so that the wider check of the verifier sees each of them, new: 7172 pass, 12 skipped, 0 fail.
• Programs for test/aot/parallel in oven-sh/bun (branch robobun/9c88a8fe/aot-object-create-places-in-a-ring): 12 programs exit with 124 under timeout 10 on the old engine and print what plain Bun prints on the new one. They cover a new property, delete, Object.freeze and Object.setPrototypeOf on such an object, a ring of three, a ring that a loop closes, a ring that no run closes, a call site below a ring, the scope chain, three forms of the fuzz program, and the constructor function. Two more programs hold rings that stay, marked // Todo:. The branch becomes a pull request when a pin of Bun holds this change.

Measurements

Old is Bun 4f7acde0f with the engine at 4d45755fed. New is the same with this change. Both are release builds from source, x64 Linux. Where a line says "first version", the build had the rule for Object.create call sites and the verifier, and not yet the rule for constructor functions.

• Code of programs without a ring. test/aot/tools/same-output.ts: IDENTICAL on 865 of 865 programs (the 289 of perf/idioms, then parallel/objects, parallel/constructor-functions, parallel/function-prototypes, parallel/classes and servers/typescript-check.ts).
• Flow results. First version: the layout lines behind ApplyFlowResults are the same for 904 of 904 programs of test/aot/parallel and test/aot/perf that name Object.create (1,277 files).
• Instructions of compiled programs. First version, test/aot/perf/compare-builds.ts --servers none, every benchmark of idioms/ and everyday/ in three program sizes: 12,612 rows compared, 0 slower, 0 faster. 2 rows moved among the benchmarks that the tool lists as unsteady (stringify-a-long-text-with-nothing-to-escape -4.1%, weak-set-has -18.3%). The servers were not measured.
• The step itself. BUN_JSC_logAOTPhaseTimes=1, --aot-threads=1, line pass AnalyzeFlow | classes, least of 5 runs taken in turns. A program of 1,294 layouts and 29 call sites: 21,629,538 to 21,625,425 instructions, and all 5 new readings lie below all 5 old ones. A small program: 260,704 to 260,813, where the old readings spread over 1,243. The old loop tested every layout once, and once more for each round that took a class back. The new one walks from the 29 call sites and allocates nothing. The visits were not counted one by one: gdb cannot read the debug information of this binary here (DWARF Error: Cannot find DIE), so the instruction count of the step stands for them.
• Binary. size: text 136,063,048 bytes and data 217,612 bytes in both.
• A ring program that runs right today. Two or three call sites in a ring, and no object changes afterwards. Instructions a turn, (I(n) - I(n/4)) / (3n/4) with n = 400,000, the same in 3 of 3 runs, first version: a read of an own field 7.0 to 29.0, a read of a name above 46.0 to 50.0, a store to an own field 5.0 to 37.0, two objects made 4,808.4 to 3,655.2. None of the 904 programs above is one.
• Checking builds. --aot-validate, the first VerifyProgram line, a program of 1,294 layouts, least of 5, first version: 4,194,558 to 4,201,922 instructions, where the old readings spread over 318. A build without the checks prints no VerifyProgram line. A validating build of servers/typescript-check.ts on one thread takes 23 minutes here and was not measured five times.
• Rings. In 5 of 5 ring programs the links of the ring in the flow results and in the image go from 2 or 3 to 0.

What stays

A class takes its base as read from the type of its extends expression (find_base_class, src/aot/lower/layouts/classes.rs in oven-sh/bun). A cast can give that expression the type of a class that the value is not. Such a base can close a ring with a base that flow proves, or with another base as read. These three programs print under plain Bun and never return as an executable, before and after this change. Measured on Bun 4f7acde0f with the engine at 4d45755fed plus this change, and on Bun at oven-sh/bun#47571 (79841cf54) with the engine at 5869a3b791 plus the first version.

1. A base from a type assertion, and a base that flow proves. Plain Bun prints true null. --aot-validate now ends the build with InvalidLayout: 'X' (L0) is not a valid layout, where it passed before.

const early = process.argv.length < 5;
function pickForA(): any { return early ? null : X; }
function pickForX(): any { return A; }
class A extends (pickForA() as typeof X) {}
class X extends pickForX() {}
console.log(Object.getPrototypeOf(X) === A, Object.getPrototypeOf(A.prototype));

2. The same through a copy of a class that a mixin makes. Plain Bun prints 4 1 2 1 0 3 1. --aot-validate now ends the build with InvalidLayout: 'B' (L2) is not a valid layout.

const early = process.argv.length < 5;
class Root { r() { return 0; } }
const mix = Base => class extends Base { m() { return 1; } };
function pick() { return early ? Root : B; }
class A extends /** @type {typeof B} */ (pick()) { a() { return 1; } }
class B extends mix(A) { b() { return 2; } }
class C extends mix(Root) { c() { return 3; } }
A.prototype.late = 4;
const p = B.prototype;
console.log(p.late, p.a(), p.b(), p.m(), p.r(), new C().c(), new C().m());

3. Two bases as read. Plain Bun prints 4 1 2 0. No check sees this one: the verifier cannot tell which classes a program defines, so it leaves a ring of bases as read valid, as verifier/invalid-layout-cyclic-base.ir says.

// @ts-nocheck
const early = process.argv.length < 5;
class Root { r() { return 0; } }
function pickA(): any { return early ? Root : B; }
function pickB(): any { return early ? A : Root; }
class A extends (pickA() as typeof B) { a() { return 1; } }
class B extends (pickB() as typeof A) { b() { return 2; } }
(A.prototype as any).late = 4;
const p: any = B.prototype;
console.log(p.late, p.a(), p.b(), p.r());

What would close the class:

• The front end names no base that closes a ring of bases as read. constructor_function_parent (src/sema/check/aot/constructor_function_parents.rs) does that for constructor functions: a chain of parents that does not end names no parent.
• computeBaseResults looks at proven bases alone when it looks for a ring. A ring of a proven base and a base as read passes it.
• A rule over the class records of the image (checkLayoutRecords, place/AOTProgramTableCheck.cpp) would be the exact check that an image holds no ring, and would see the third program too.

None of the three is part of this change. The first two programs are // Todo: programs on the Bun branch. No issue tracks them: this account cannot file one.

belongsTo() in select/AOTPlannedSlotRules.cpp walks the expected bases without a bound at build time. This change takes two kinds of ring from it, and not those three.

The walks of the run time stay as they are: forEachDescendant, isDeclaredBetween and the loop in leaveGuardsOfStoresForLater. The image must hold no ring.

The branch

The parent is main at 9f5001ee2c. The change was written and measured on 4d45755fed, which Bun main pins: no Bun front end matches the IR schema of main today, so no whole program can be built against main here. noteClassesOfCreatedObjects and the rule for constructor functions are the same text on both commits, and the commit applies to main without a conflict. Every number above is from the build on 4d45755fed. The Test jobs of this pull request were the first run of testaot all on main with the change: 8873 passed, 0 failed, 42 todo, and 33 left out that need entries of Bun (Test bun-webkit-linux-amd64-lto-aot). All seven Test jobs pass, on x64 and arm64.

An earlier main (5869a3b791, with Bun at oven-sh/bun#47571) was built with the first version. Its tests passed, the programs printed what plain Bun prints, and testaot all was 8793 passed, 2 failed. The 2 failed the same way without the change in that build (property-lookup/absence-filter-names-what-stops-stores, valueflow/modules/text-file-answers-the-text-to-a-linked-require-and-its-namespace-to-the-loader.ir).

Self-review

Of 18 concerns, 15 are addressed:

• The branch follows main, and the Bun half is tests alone.
• Constructor functions get the rule, with two tests. They were a second producer of the same ring.
• The rings that stay are in this text with their programs, and two of them are // Todo: programs on the Bun branch.
• The comments of both walks give the reason. The first one said that a call site on a ring is handed more than one object, which is false.
• New tests: the makers stay uncopied (CopyClosureFactoriesChanges = 0), a call site below a ring in both orders of the walk, and call sites that are handed each other's objects and stay classes.
• This text says who found the hang and who can meet it, begins with the scope chain, and names the engine of each number.

Two are declined. A test of a call site that is handed null and then a literal: this change does not touch that case, and #969 adds tests of it. A comment on the Bun pull request that moves the pin: the next pin after this merges holds the change.

One is addressed in part: the review asked for a tracking issue for the rings that stay. This account cannot file issues, so the programs and the three ways to close the class are under "What stays".

[robobun]

Status

• Reproduced: the executable of bun build --compile --aot for the scope chain in the description never returns (exit 124 under timeout 10). That is on Bun 4f7acde0f with the engine at 4d45755fed, and on canary 1.4.4-canary.1+da867c60b. With this change it prints 1 2 true false, as plain Bun does.
• The programs for oven-sh/bun are in test(aot): programs whose expected base classes would form a ring bun#47766, a draft that waits for a pin of Bun that holds this change.

[github-actions]

Preview build of d5d6e38: autobuild-preview-pr-980-d5d6e38f

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 13 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: faa14b4b-9b54-42f7-8d2a-1b439af0029c

Commits

Reviewing files that changed from the base of the PR and between 9f5001e and d5d6e38.

Files selected for processing: 13

• Source/JavaScriptCore/aot/ir/AOTProgramVerifier.cpp
• Source/JavaScriptCore/aot/tests/answers/object-create-places-handed-each-others-objects.ir
• Source/JavaScriptCore/aot/tests/pipeline/a-constructor-function-named-below-a-class-that-extends-it-has-no-base.ir
• Source/JavaScriptCore/aot/tests/pipeline/a-place-of-object-create-below-a-ring-of-places-is-no-class-where-its-row-is-walked-first.ir
• Source/JavaScriptCore/aot/tests/pipeline/a-place-of-object-create-below-a-ring-of-places-is-no-class.ir
• Source/JavaScriptCore/aot/tests/pipeline/places-of-object-create-above-each-other-are-no-classes.ir
• Source/JavaScriptCore/aot/tests/pipeline/places-of-object-create-handed-each-others-objects-are-classes-where-the-row-above-them-ends.ir
• Source/JavaScriptCore/aot/tests/pipeline/places-of-object-create-in-a-ring-of-three-are-no-classes.ir
• Source/JavaScriptCore/aot/tests/pipeline/places-of-object-create-in-a-row-that-ends-below-a-literal-are-classes.ir
• Source/JavaScriptCore/aot/tests/valueflow/results/base-named-for-a-constructor-function-that-closes-a-cycle-names-no-base-class.ir
• Source/JavaScriptCore/aot/tests/verifier/invalid-layout-ring-of-bases-that-flow-closes.ir
• Source/JavaScriptCore/aot/tests/verifier/invalid-layout-ring-of-classes-of-created-objects.ir
• Source/JavaScriptCore/aot/valueflow/AOTValueFlowResultsClasses.cpp

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.