[robobun]

Draft. The bun side is oven-sh/bun#44226, pinned to the preview build here.

Problem

• bun:ffi CString must return a string primitive from new CString(ptr) (bun:ffi: use the engine-native FFI when available bun#35246). As an InternalFunction it is a constructor to every engine algorithm, and those trust [[Construct]] to return an object.
• Array.from.call(CString, [1, 2, 3]) aborts a release build: RELEASE_ASSERT(baseValue.isObject()) in llint_slow_path_put_by_val_direct.

Fix

• A subclass of InternalFunction may put JSCell::getConstructData in its method table (using JSCell::getConstructData;). It then reports no construct data, and m_functionForConstruct stays installed.
• The constructor of InternalFunction asserted that no subclass changes that slot. The assertion now accepts this one function.
• Correct because new f() and super() read no construct data on an InternalFunction: setUpCall, linkFor and virtualFor jump to a trampoline that loads m_functionForConstruct. Every other construct asks the method table first.
• Verified: JSTests/stress/internal-function-new-without-construct-data.js, debug ASAN jsc, 7 tier configurations.

Background

• isConstructor(), construct(), Reflect.construct, bind, Proxy, Array.of, Array.from, extends and species read construct data through the method table.
• Compiler threads call that slot, so the assertion exists. JSCell::getConstructData returns an empty CallData and reads nothing.
• The first version used a type-info flag: 5 more instructions in getConstructData for every native constructor, and one of 4 free type-info bits.

Downsides

• None for code that bun ships. InternalFunction::getConstructData has the same 13 instructions before and after, and the release text of bun has the same size (see Notes).
• super() of a class whose [[Prototype]] is such a function still runs the handler. The handler must check new.target.

Notes

Why not an isObject check in Interpreter::executeConstruct. It runs after the handler has read the length as a pointer, and Array.from is a JS builtin that never gets there.

The test helper. $vm.createInternalFunctionWithoutConstructData() makes a function of a class that takes JSCell::getConstructData. Its call handler returns "call:<argument count>" and its construct handler returns "new:<argument count>", or throws a TypeError when new.target is not the function. JSDollarVM is compiled into the library only when assertions are on, so a release build of bun does not have it.

Tier configurations run (local build of this branch, jsc, Debug with ASAN, Linux x64), all with --validateExceptionChecks=true: default with FTL, --useLLInt=false, --useJIT=false, --useFTLJIT=false, DFG eager with --useConcurrentJIT=false and graph validation, FTL eager with --useConcurrentJIT=false and graph validation, --forceOSRExitToLLInt=true.

Without the change to the assertion a build with assertions stops when the helper makes its function: ASSERTION FAILED: methodTable()->getConstructData == InternalFunction::info()->methodTable.getConstructData.

Existing tests run on the same build, unchanged, all pass: is-constructor.js, is-constructor-should-be-correct-in-ai.js, internal-function-construct.js, internal-function-call.js, reflect-construct.js, proxy-construct.js, array-of-non-array-constructor.js, array-species-functions.js, array-from-with-iterable.js, class-subclassing-function.js.

Release builds, measured (Linux x64). nm -S and objdump -d on the lto jsc of f20ce774 and of 42d31a6b: InternalFunction::getConstructData is 41 bytes and the same 13 instructions in both. A release build of bun has 80,692,188 bytes of text with each of the two packages, and no JSDollarVM symbol. The jsc shell has $vm in every build, so its lto text grows from 47,724,559 to 47,726,911 bytes.

The flag version, measured. gdb single-stepped one call of InternalFunction::getConstructData in the lto jsc of f20ce774 and of the first commit of this PR: 13 to 18 instructions for a function that is a constructor, 41 to 61 bytes.

What this does not cover. A native function made with JSFunction::create does not read its construct data from the method table (getConstructDataInline in JSObjectInlines.h goes to JSFunction::getConstructData for a JSFunctionType cell). The functions of oven-sh/bun#34575 are of that kind.

[github-actions]

Preview build of 42d31a6: autobuild-preview-pr-739-42d31a6b

[Jarred-Sumner]

@robobun wake up!!

[robobun]

Status:

• The bun side is built and tested against a build of this branch. I open it as a draft next and link it in the description here.
• I am changing this PR from a type-info flag to the method table. A subclass takes JSCell::getConstructData with a using-declaration, and the only engine change is the assertion in the constructor of InternalFunction. That adds nothing to getConstructData (the flag added 5 instructions for every native constructor) and uses no type-info bit. I push it when the local build and the stress test pass.

One decision is needed from a maintainer. After the bun change, CString is not a constructor to the engine, so these forms throw a TypeError:

form	release today	debug or ASAN today	after
Reflect.construct(CString, [ptr])	"hello"	"hello"	TypeError
const rc = Reflect.construct; rc(CString, [ptr])	"hello"	ASSERTION FAILED: cell->isObjectSlow()	TypeError
new (CString.bind(null))(ptr)	"hello"	ASSERTION FAILED: cell->isObjectSlow()	TypeError
new (new Proxy(CString, {}))(ptr)	"hello"	ASSERTION FAILED: cell->isObjectSlow()	TypeError

new CString(ptr) and CString(ptr) do not change, in every JIT tier.

If those forms must keep their result, this change is the wrong tool, because IsConstructor is what makes them throw.

Edited: the first version of this comment said that all of these forms fail an assertion on a debug build. The literal Reflect.construct(...) call does not, because the engine compiles it to bytecode.