Closed Bug 704147 Opened 8 years ago Closed 8 years ago

FunctionType.ptr doesn't expose call/apply

Categories

(Core :: js-ctypes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: Fallen, Assigned: jdm)

References

Details

Attachments

(1 file, 6 obsolete files)

Creating a FunctionType.ptr using library.declare() returns an object that is obviously different to the JS Function, but to allow wrapping js-ctypes functions it would be nice to allow using jsctypefunc.apply(null, arguments) or similar.

I tested this on the current comm-central/mozilla-central and the result of library.declare() gives me:

ctypes.FunctionType(ctypes.default_abi, ctypes.char.ptr, [ctypes.char.ptr]).ptr(ctypes.UInt64("0x2ae73a942f20"))
Looks like we would just need to tack on call and apply methods to the FunctionType objects. I could whip up a patch if it were desired.
(In reply to Josh Matthews [:jdm] from comment #1)
> Looks like we would just need to tack on call and apply methods to the
> FunctionType objects. I could whip up a patch if it were desired.

I think that could be a nice feature. Jorendorff suggests trying the following:

ctypes.FunctionType.prototype.prototype.call = Function.prototype.call;

If it works, then we can just use the native js_fun_apply and js_fun_call within C++, which should just be a few lines of code.

Make it so, jdm! :-)
(In reply to Bobby Holley (:bholley) from comment #2)

> ctypes.FunctionType.prototype.prototype.call = Function.prototype.call;
This give me: Error: not a CType

replacing prototype with __proto__ doesn't help either.

Thanks for looking into this!
(In reply to Philipp Kewisch [:Fallen] from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> 
> > ctypes.FunctionType.prototype.prototype.call = Function.prototype.call;
> This give me: Error: not a CType
> 
> replacing prototype with __proto__ doesn't help either.
> 
> Thanks for looking into this!

Looks like you found a bug! I filed and wrote a patch for it today: bug 705019.

In the mean time, hopefully jdm can get us a c++ implementation so that we don't need the little workaround at all.
This turned out to be super duper easy.
Attachment #576826 - Flags: review?(bobbyholley+bmo)
Comment on attachment 576826 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.

This will make these functions appear on every pointer, regardless of whether they're function pointers.

However, I understand why you did it this way, since there isn't really an obvious alternative. The basic problem is that we don't ever really use FunctionType, and instead insist that all function-y instances go through FunctionType().ptr. So on the surface, functions are just pointers, and share the same prototype with all other pointers. Which means that we don't have anywhere to put function-pointer-specific stuff.

The weird thing is though, we _have_ a FunctionType data prototype, and store it in SLOT_FUNCTIONDATAPROTO. But we never use it, because we throw in CType::ConstructData if someone's trying to instantiate a FunctionType instance. The actual magic of function instantiation all comes in PointerType::ConstructData().

So this gives us a nice opportunity: We can put SLOT_FUNCTIONDATAPROTO to work by placing it as an intermediary on the prototype chain for function pointers.

So my_regular_ptr.__proto__ is POINTERDATAPROTO, but my_fn_ptr.__proto__ is FUNCTIONDATAPROTO, and FUNCTIONDATAPROTO.__proto__ is POINTERDATAPROTO.

This requires a few special cases here and there, but I think it makes the most sense, since we we give function pointers special treatment in lots of ways.

This does, of course, make this a much more sizable task. But also more fun! What do you think jdm? Are you interested in doing it? ;-)
Attachment #576826 - Flags: review?(bobbyholley+bmo) → review-
Of course!
Assignee: nobody → josh
As for me, I'm interested in using it. There are, of course, workarounds, but this is a partial blocker for bug 563742.
jdm, you still interested?
I'm happy to let someone else take a shot at it. I got bogged down in trying to figure out how to properly set up the prototypes in JSAPI and couldn't get it working correctly.
Yoric - interested?
(In reply to Bobby Holley (:bholley) from comment #11)
> Yoric - interested?

Interesting in using it more than in implementing it :)
But yes, once I have finished with bug 720771, I can try and spend some time on this.
Attached patch Complete patch (obsolete) — Splinter Review
Here is a cleaned-up patch. Working on the testsuite.
Attachment #594537 - Flags: review?(jorendorff)
Attached patch Partial testsuite (obsolete) — Splinter Review
Attaching a Unix testsuite. I need to investigate how I can write a Windows version.
Attachment #594537 - Attachment is obsolete: true
Attachment #594537 - Flags: review?(jorendorff)
Attachment #594542 - Attachment is obsolete: true
I plan on poking at this in the coming week unless Yoric beats me to it.
I was messing around during the superbowl and suddenly my tests started passing. I admit to not fully understanding why; care to tell me the reasons why this change is wrong?
Attachment #594614 - Flags: review?(bobbyholley+bmo)
Attachment #576826 - Attachment is obsolete: true
Comment on attachment 594614 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.


>@@ -833,18 +839,19 @@ InitTypeClasses(JSContext* cx, JSObject*
> 
>   if (!InitTypeConstructor(cx, parent, CTypeProto, CDataProto,
>          sStructFunction, sStructFunctions, sStructProps,
>          sStructInstanceFunctions, NULL,
>          protos[SLOT_STRUCTPROTO], protos[SLOT_STRUCTDATAPROTO]))
>     return false;
>   js::AutoObjectRooter sroot(cx, protos[SLOT_STRUCTDATAPROTO]);
> 
>-  if (!InitTypeConstructor(cx, parent, CTypeProto, CDataProto,
>-         sFunctionFunction, NULL, sFunctionProps, NULL, NULL,
>+  if (!InitTypeConstructor(cx, parent, CTypeProto,
>+                           JS_GetPrototype(cx, protos[SLOT_POINTERDATAPROTO]),

Why are you calling JS_GetPrototype here? That effectively sets the prototype to CDATAPROTO, which is what the code was already doing. This means that function pointers won't have the pointer-esque functionality.


>+         sFunctionFunction, NULL, sFunctionProps, sFunctionInstanceFunctions, NULL,

Indentation.

>@@ -3275,18 +3282,25 @@ PointerType::CreateInternal(JSContext* c

>-  dataProto = CType::GetProtoFromType(cx, baseType, SLOT_POINTERDATAPROTO);
>+  if (CType::GetTypeCode(cx, baseType) == TYPE_function)
>+    fninfo = FunctionType::GetFunctionInfo(cx, baseType);
>+  if (fninfo) {
>+    dataProto = CType::GetProtoFromType(cx, fninfo->mReturnType, SLOT_FUNCTIONDATAPROTO);
>+  } else {
>+    dataProto = CType::GetProtoFromType(cx, baseType, SLOT_POINTERDATAPROTO);
>+  }

GetFunctionInfo can never return null. So this logic can be simplified a fair bit.


>diff --git a/toolkit/components/ctypes/tests/unit/test_jsctypes.js.in b/toolkit/components/ctypes/tests/unit/test_jsctypes.js.in

>   // open the library
>   let libfile = do_get_file(CTYPES_TEST_LIB);
>   let library = ctypes.open(libfile.path);
>+  run_function_tests(library);

What's this about?
Attachment #594614 - Flags: review?(bobbyholley+bmo) → review-
Attachment #597088 - Flags: review?(bobbyholley+bmo)
Attachment #594614 - Attachment is obsolete: true
Attachment #597091 - Flags: review?(bobbyholley+bmo)
Attachment #597088 - Attachment is obsolete: true
Attachment #597088 - Flags: review?(bobbyholley+bmo)
Comment on attachment 597091 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.


>   // Get ctypes.PointerType.prototype and the common prototype for CData objects
>   // of this type.
>-  JSObject* typeProto;
>   JSObject* dataProto;
>-  typeProto = CType::GetProtoFromType(cx, baseType, SLOT_POINTERPROTO);
>-  dataProto = CType::GetProtoFromType(cx, baseType, SLOT_POINTERDATAPROTO);
>+  JSObject* typeProto = CType::GetProtoFromType(cx, baseType, SLOT_POINTERPROTO);
>+  if (CType::GetTypeCode(cx, baseType) == TYPE_function) {
>+    FunctionInfo* fninfo = FunctionType::GetFunctionInfo(cx, baseType);

This isn't necessary. CTypes is set up so that every type constructor prototype has pointers to everything (see AttachProtos). So you can just pull what you need off of baseType here.

>diff --git a/toolkit/components/ctypes/tests/unit/test_jsctypes.js.in b/toolkit/components/ctypes/tests/unit/test_jsctypes.js.in

>   checkParentIsCTypes(Object.getPrototypeOf(t.prototype));
>-  do_check_true(t.prototype.__proto__.__proto__ === ctypes.CData.prototype);
>+  if (t instanceof ctypes.FunctionType)
>+    do_check_true(t.prototype.__proto__.__proto__.__proto__ === ctypes.CData.prototype);
>+  else
>+    do_check_true(t.prototype.__proto__.__proto__ === ctypes.CData.prototype);

Rather than just looking one level higher, I'd rather the special case specifically check that the proto is ctypes.PointerType.prototype.prototype (whose proto is indeed ctypes.Data.prototype).

>   // Make sure we can access 'prototype' on a CTypeProto.
>-  do_check_true(Object.getPrototypeOf(c.prototype.prototype) === ctypes.CType.prototype.prototype);
>+  if (t instanceof ctypes.FunctionType)
>+    do_check_true(Object.getPrototypeOf(Object.getPrototypeOf(c.prototype.prototype)) === ctypes.CType.prototype.prototype);
>+  else
>+    do_check_true(Object.getPrototypeOf(c.prototype.prototype) === ctypes.CType.prototype.prototype);

Same thing here.
Attachment #597091 - Flags: review?(bobbyholley+bmo) → review-
Attachment #597091 - Attachment is obsolete: true
Comment on attachment 597159 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.

Looks good! Flagging jorendorff for feedback just to make sure he's down with the whole thing.
Attachment #597159 - Flags: review?(bobbyholley+bmo)
Attachment #597159 - Flags: review+
Attachment #597159 - Flags: feedback?(jorendorff)
Any word on this? I will need it if I want to ever land OS.File stuff.
Comment on attachment 597159 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.

Yes, by all means, land this. :)
Attachment #597159 - Flags: feedback?(jorendorff) → feedback+
https://hg.mozilla.org/mozilla-central/rev/d3fb71fc5292
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.