704147 - FunctionType.ptr doesn't expose call/apply

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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"))

Comment 1

[Josh Matthews [:jdm]]

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.

Comment 2

[Bobby Holley (:bholley)]

(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! :-)

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(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!

Comment 4

[Bobby Holley (:bholley)]

(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.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Make ctypes function pointers callable via call and apply.

This turned out to be super duper easy.

Comment 6

[Bobby Holley (:bholley)]

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? ;-)

Comment 7

[Josh Matthews [:jdm]]

Of course!

Comment 8

[David Teller [:Yoric] - still alive but not very active]

As for me, I'm interested in using it. There are, of course, workarounds, but this is a partial blocker for bug 563742.

Comment 9

[Bobby Holley (:bholley)]

jdm, you still interested?

Comment 10

[Josh Matthews [:jdm]]

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.

Comment 11

[Bobby Holley (:bholley)]

Yoric - interested?

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(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.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Complete patch

Here is a cleaned-up patch. Working on the testsuite.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Partial testsuite

Attaching a Unix testsuite. I need to investigate how I can write a Windows version.

Comment 15

[Nathan Froyd [:froydnj]]

I plan on poking at this in the coming week unless Yoric beats me to it.

Comment 16

[Josh Matthews [:jdm]]

Attachment: Make ctypes function pointers callable via call and apply.

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?

Comment 17

[Bobby Holley (:bholley)]

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?

Comment 18

[Josh Matthews [:jdm]]

Attachment: Make ctypes function pointers callable via call and apply.

Comment 19

[Josh Matthews [:jdm]]

Attachment: Make ctypes function pointers callable via call and apply.

Comment 20

[Bobby Holley (:bholley)]

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.

Comment 21

[Josh Matthews [:jdm]]

Attachment: Make ctypes function pointers callable via call and apply.

Comment 22

[Bobby Holley (:bholley)]

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.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Any word on this? I will need it if I want to ever land OS.File stuff.

Comment 24

[Jason Orendorff [:jorendorff]]

Comment on attachment 597159 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.

Yes, by all means, land this. :)

Comment 25

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fd4b5485c24f

Comment 26

[Doug Turner (:dougt)]

Busted build. Backed out.  https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe4b3fbd351

Comment 27

[(no longer active)]

Landing: https://hg.mozilla.org/mozilla-central/rev/fd4b5485c24f
Backout: https://hg.mozilla.org/mozilla-central/rev/2fe4b3fbd351

Comment 28

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d3fb71fc5292

Comment 29

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d3fb71fc5292