Skip to content

Commit 48f4cfb

Browse files
trivikraduh95
authored andcommitted
ffi: fix optimized buffer conversions
Preserve pointer-like argument conversions for buffer and arraybuffer signatures after Fast API optimization. Keep memory-backed arguments on the specialized native fast path. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: openai:gpt-5.6-sol PR-URL: #64639Fixes: #64638 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Aviv Keller <me@aviv.sh>
1 parent b6ab546 commit 48f4cfb

6 files changed

Lines changed: 79 additions & 53 deletions

File tree

‎lib/ffi.js‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ const {
6868
}=require('internal/ffi-shared-buffer');
6969

7070
const{
71-
initializeFastBufferMetadata,
7271
wrapWithRawPointerConversions,
7372
}=require('internal/ffi/fast-api');
7473

@@ -90,7 +89,6 @@ function wrapFFIFunction(rawFn, owner) {
9089
returnType=rawFn[kSbReturn];
9190
}
9291
}
93-
initializeFastBufferMetadata(rawFn,argumentTypes);
9492
constwrapped=wrapWithSharedBuffer(
9593
rawFn,
9694
argumentTypes===undefined ? undefined : makeSignature(argumentTypes,returnType));

‎lib/internal/ffi/fast-api.js‎

Lines changed: 16 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ const {
66
ObjectDefineProperty,
77
ReflectApply,
88
StringPrototypeIncludes,
9-
Symbol,
109
TypeError,
1110
}=primordials;
1211

@@ -24,11 +23,8 @@ const {
2423
getRawPointer,
2524
kFastArguments,
2625
kFastBufferInvoke,
27-
kSbSharedBuffer,
2826
}=internalBinding('ffi');
2927

30-
constkFastBuffer=Symbol('kFastBuffer');
31-
3228
constU64_MAX=0xFFFFFFFFFFFFFFFFn;
3329
constI64_MAX=0x7FFFFFFFFFFFFFFFn;
3430
constI64_MIN=-0x8000000000000000n;
@@ -79,16 +75,13 @@ function validateFastIntegerArg(type, value, index) {
7975
}
8076
}
8177

82-
functionneedsRawPointerConversion(type,rawFn){
83-
if(rawFn!==undefined&&rawFn[kFastBuffer]===true&&
84-
(type==='buffer'||type==='arraybuffer')){
85-
returnfalse;
86-
}
78+
functionneedsRawPointerConversion(type){
8779
returntype==='buffer'||type==='arraybuffer';
8880
}
8981

9082
functionneedsPointerLikeConversion(type){
91-
returntype==='pointer'||type==='ptr'||type==='function';
83+
returntype==='pointer'||type==='ptr'||type==='function'||
84+
type==='buffer'||type==='arraybuffer';
9285
}
9386

9487
functionneedsStringPointerConversion(type){
@@ -100,12 +93,8 @@ function needsNullPointerConversion(type) {
10093
needsRawPointerConversion(type);
10194
}
10295

103-
functionneedsPointerConversion(type,rawFn){
104-
if(rawFn!==undefined&&rawFn[kFastBuffer]===true&&
105-
(type==='buffer'||type==='arraybuffer')){
106-
returnfalse;
107-
}
108-
returnneedsRawPointerConversion(type,rawFn)||
96+
functionneedsPointerConversion(type){
97+
returnneedsRawPointerConversion(type)||
10998
needsNullPointerConversion(type)||needsStringPointerConversion(type);
11099
}
111100

@@ -174,11 +163,11 @@ function convertPointerArg(type, value, stringState, index) {
174163
returnvalue;
175164
}
176165

177-
functiongetFastArgumentIndexes(argumentsTypes,rawFn){
166+
functiongetFastArgumentIndexes(argumentsTypes){
178167
letindexes=null;
179168
for(leti=0;i<argumentsTypes.length;i++){
180169
if(fastIntegerTypeInfo[argumentsTypes[i]]===undefined&&
181-
!needsPointerConversion(argumentsTypes[i],rawFn)){
170+
!needsPointerConversion(argumentsTypes[i])){
182171
continue;
183172
}
184173
if(indexes===null){
@@ -189,31 +178,12 @@ function getFastArgumentIndexes(argumentsTypes, rawFn) {
189178
returnindexes;
190179
}
191180

192-
functionconvertFastArg(type,value,rawFn,stringState,index){
181+
functionconvertFastArg(type,value,stringState,index){
193182
validateFastIntegerArg(type,value,index);
194-
returnneedsPointerConversion(type,rawFn) ?
183+
returnneedsPointerConversion(type) ?
195184
convertPointerArg(type,value,stringState,index) : value;
196185
}
197186

198-
functioninitializeFastBufferMetadata(rawFn,argumentTypes){
199-
if(rawFn===undefined||rawFn===null||argumentTypes===undefined){
200-
return;
201-
}
202-
if(rawFn[kSbSharedBuffer]!==undefined){
203-
return;
204-
}
205-
206-
if(rawFn[kFastArguments]!==undefined){
207-
for(leti=0;i<argumentTypes.length;i++){
208-
consttype=argumentTypes[i];
209-
if(type==='buffer'||type==='arraybuffer'){
210-
rawFn[kFastBuffer]=true;
211-
break;
212-
}
213-
}
214-
}
215-
}
216-
217187
functioninheritMetadata(wrapper,rawFn,nargs){
218188
ObjectDefineProperty(wrapper,'name',{
219189
__proto__: null,value: rawFn.name,configurable: true,
@@ -239,7 +209,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) {
239209
returnrawFn;
240210
}
241211

242-
constindexes=getFastArgumentIndexes(argumentTypes,rawFn);
212+
constindexes=getFastArgumentIndexes(argumentTypes);
243213
if(indexes===null){
244214
returnrawFn;
245215
}
@@ -295,9 +265,8 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) {
295265
(c1&&hasStringPointerArg(t1,a1));
296266
if(stringCall)enterStringConversion(stringState);
297267
try{
298-
returnrawFn(c0 ?
299-
convertFastArg(t0,a0,rawFn,stringState,0) : a0,
300-
c1 ? convertFastArg(t1,a1,rawFn,stringState,1) : a1);
268+
returnrawFn(c0 ? convertFastArg(t0,a0,stringState,0) : a0,
269+
c1 ? convertFastArg(t1,a1,stringState,1) : a1);
301270
}finally{
302271
if(stringCall)exitStringConversion(stringState);
303272
}
@@ -318,10 +287,9 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) {
318287
(c2&&hasStringPointerArg(t2,a2));
319288
if(stringCall)enterStringConversion(stringState);
320289
try{
321-
returnrawFn(c0 ?
322-
convertFastArg(t0,a0,rawFn,stringState,0) : a0,
323-
c1 ? convertFastArg(t1,a1,rawFn,stringState,1) : a1,
324-
c2 ? convertFastArg(t2,a2,rawFn,stringState,2) : a2);
290+
returnrawFn(c0 ? convertFastArg(t0,a0,stringState,0) : a0,
291+
c1 ? convertFastArg(t1,a1,stringState,1) : a1,
292+
c2 ? convertFastArg(t2,a2,stringState,2) : a2);
325293
}finally{
326294
if(stringCall)exitStringConversion(stringState);
327295
}
@@ -344,7 +312,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) {
344312
for(leti=0;i<indexes.length;i++){
345313
constindex=indexes[i];
346314
args[index]=convertFastArg(
347-
argumentTypes[index],args[index],rawFn,stringState,index);
315+
argumentTypes[index],args[index],stringState,index);
348316
}
349317
returnReflectApply(rawFn,undefined,args);
350318
}finally{
@@ -360,6 +328,5 @@ module.exports = {
360328
convertPointerArg,
361329
hasPointerMemoryArg,
362330
hasStringPointerArg,
363-
initializeFastBufferMetadata,
364331
wrapWithRawPointerConversions,
365332
};

‎src/ffi/fast.cc‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,12 +178,32 @@ bool IsPointerTypeName(const std::string& name) {
178178
return name == "pointer" || name == "ptr" || name == "function";
179179
}
180180

181+
boolIsBufferTypeName(const std::string& name) {
182+
return name == "buffer" || name == "arraybuffer";
183+
}
184+
181185
boolSignatureNeedsFastBufferInvoke(const FFIFunction& fn) {
182186
// The secondary buffer invoke is only generated for the hot monomorphic case
183187
// where a single pointer-like argument can be satisfied by a Buffer or
184188
// ArrayBuffer without allocating or caching a BigInt pointer in JS.
185189
return fn.arg_type_names.size() == 1 &&
186-
IsPointerTypeName(fn.arg_type_names[0]);
190+
(IsPointerTypeName(fn.arg_type_names[0]) ||
191+
IsBufferTypeName(fn.arg_type_names[0]));
192+
}
193+
194+
std::shared_ptr<FFIFunction> CloneWithRawPointerArgNames(
195+
const std::shared_ptr<FFIFunction>& fn) {
196+
// The primary Fast API entrypoint receives pointer-compatible values as
197+
// BigInts after the JS wrapper has converted strings, nullish values, and
198+
// memory-backed objects. A secondary entrypoint handles the monomorphic
199+
// memory-backed case without extracting the pointer in JS.
200+
auto clone = std::make_shared<FFIFunction>(*fn);
201+
for (std::string& name : clone->arg_type_names) {
202+
if (IsBufferTypeName(name)) {
203+
name = "pointer";
204+
}
205+
}
206+
return clone;
187207
}
188208

189209
std::shared_ptr<FFIFunction> CloneWithFastBufferArgNames(

‎src/ffi/fast.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ bool SignatureNeedsRawPointerConversions(const FFIFunction& fn);
6161
boolSignatureNeedsFastIntegerValidation(const FFIFunction& fn);
6262
boolIsPointerTypeName(const std::string& name);
6363
boolSignatureNeedsFastBufferInvoke(const FFIFunction& fn);
64+
std::shared_ptr<FFIFunction> CloneWithRawPointerArgNames(
65+
const std::shared_ptr<FFIFunction>& fn);
6466
std::shared_ptr<FFIFunction> CloneWithFastBufferArgNames(
6567
const std::shared_ptr<FFIFunction>& fn);
6668
std::unique_ptr<FastFFIMetadata> CreateFastFFIMetadata(const FFIFunction& fn);

‎src/node_ffi.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,8 @@ MaybeLocal<Function> DynamicLibrary::CreateFunction(
249249
// Try the generated Fast API path first. If metadata creation rejects the
250250
// signature, fall back to SharedBuffer for supported scalar shapes, then to
251251
// the generic libffi invoker.
252-
info->fast_metadata = CreateFastFFIMetadata(*fn);
252+
std::shared_ptr<FFIFunction> fast_fn = CloneWithRawPointerArgNames(fn);
253+
info->fast_metadata = CreateFastFFIMetadata(*fast_fn);
253254
bool use_fast_api = info->fast_metadata != nullptr;
254255
bool use_sb = !use_fast_api && IsSBEligibleSignature(*fn);
255256
bool has_ptr_args = use_sb && SignatureHasPointerArgs(*fn);

‎test/ffi/test-ffi-fast-buffer.js‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,3 +95,41 @@ test('fast FFI string buffers survive reentrant callbacks', {
9595
lib.close();
9696
}
9797
});
98+
99+
test('optimized buffer signatures preserve pointer-like conversions',()=>{
100+
constlib=newffi.DynamicLibrary(libraryPath);
101+
constasBuffer=lib.getFunction('pointer_to_usize',{
102+
arguments: ['buffer'],
103+
return: 'u64',
104+
});
105+
constasArrayBuffer=lib.getFunction('pointer_to_usize',{
106+
arguments: ['arraybuffer'],
107+
return: 'u64',
108+
});
109+
110+
functioncallBuffer(value){
111+
returnasBuffer(value);
112+
}
113+
114+
functioncallArrayBuffer(value){
115+
returnasArrayBuffer(value);
116+
}
117+
118+
try{
119+
for(leti=0;i<100_000;i++){
120+
assert.strictEqual(callBuffer(0n),0n);
121+
assert.strictEqual(callArrayBuffer(0n),0n);
122+
}
123+
124+
for(constcallof[callBuffer,callArrayBuffer]){
125+
assert.strictEqual(call(null),0n);
126+
assert.strictEqual(call(undefined),0n);
127+
assert.notStrictEqual(call('ffi'),0n);
128+
129+
constbytes=Buffer.alloc(1);
130+
assert.strictEqual(call(bytes),ffi.getRawPointer(bytes));
131+
}
132+
}finally{
133+
lib.close();
134+
}
135+
});

0 commit comments

Comments
 (0)