Last Comment Bug 704147 - FunctionType.ptr doesn't expose call/apply
: FunctionType.ptr doesn't expose call/apply
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Josh Matthews [:jdm] (on vacation until Dec 5)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: OS.File
  Show dependency treegraph
 
Reported: 2011-11-21 07:51 PST by Philipp Kewisch [:Fallen]
Modified: 2012-04-06 11:33 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make ctypes function pointers callable via call and apply. (2.51 KB, patch)
2011-11-24 14:33 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
bobbyholley: review-
Details | Diff | Splinter Review
Complete patch (26.88 KB, patch)
2012-02-05 05:30 PST, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Partial testsuite (26.88 KB, patch)
2012-02-05 07:30 PST, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Make ctypes function pointers callable via call and apply. (5.37 KB, patch)
2012-02-05 18:42 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
bobbyholley: review-
Details | Diff | Splinter Review
Make ctypes function pointers callable via call and apply. (7.31 KB, patch)
2012-02-14 10:49 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
no flags Details | Diff | Splinter Review
Make ctypes function pointers callable via call and apply. (7.26 KB, patch)
2012-02-14 10:52 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
bobbyholley: review-
Details | Diff | Splinter Review
Make ctypes function pointers callable via call and apply. (7.22 KB, patch)
2012-02-14 13:26 PST, Josh Matthews [:jdm] (on vacation until Dec 5)
bobbyholley: review+
jorendorff: feedback+
Details | Diff | Splinter Review

Description Philipp Kewisch [:Fallen] 2011-11-21 07:51:46 PST
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] (on vacation until Dec 5) 2011-11-21 08:42:07 PST
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) (busy with Stylo) 2011-11-21 11:10:46 PST
(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] 2011-11-23 05:20:44 PST
(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) (busy with Stylo) 2011-11-23 17:27:22 PST
(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] (on vacation until Dec 5) 2011-11-24 14:33:59 PST
Created attachment 576826 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.

This turned out to be super duper easy.
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2011-11-24 16:31:52 PST
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] (on vacation until Dec 5) 2011-11-24 21:28:23 PST
Of course!
Comment 8 David Teller [:Yoric] (please use "needinfo") 2012-02-04 07:29:15 PST
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) (busy with Stylo) 2012-02-04 11:23:34 PST
jdm, you still interested?
Comment 10 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-04 11:58:29 PST
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) (busy with Stylo) 2012-02-04 15:11:45 PST
Yoric - interested?
Comment 12 David Teller [:Yoric] (please use "needinfo") 2012-02-05 03:19:02 PST
(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] (please use "needinfo") 2012-02-05 05:30:29 PST
Created attachment 594537 [details] [diff] [review]
Complete patch

Here is a cleaned-up patch. Working on the testsuite.
Comment 14 David Teller [:Yoric] (please use "needinfo") 2012-02-05 07:30:00 PST
Created attachment 594542 [details] [diff] [review]
Partial testsuite

Attaching a Unix testsuite. I need to investigate how I can write a Windows version.
Comment 15 Nathan Froyd [:froydnj] 2012-02-05 12:15:55 PST
I plan on poking at this in the coming week unless Yoric beats me to it.
Comment 16 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-05 18:42:53 PST
Created attachment 594614 [details] [diff] [review]
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) (busy with Stylo) 2012-02-10 15:30:20 PST
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] (on vacation until Dec 5) 2012-02-14 10:49:10 PST
Created attachment 597088 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.
Comment 19 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-02-14 10:52:47 PST
Created attachment 597091 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.
Comment 20 Bobby Holley (:bholley) (busy with Stylo) 2012-02-14 11:20:17 PST
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] (on vacation until Dec 5) 2012-02-14 13:26:43 PST
Created attachment 597159 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2012-02-14 13:31:43 PST
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] (please use "needinfo") 2012-03-20 01:59:40 PDT
Any word on this? I will need it if I want to ever land OS.File stuff.
Comment 24 Jason Orendorff [:jorendorff] 2012-04-04 13:48:38 PDT
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] (on vacation until Dec 5) 2012-04-04 14:00:01 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/fd4b5485c24f
Comment 26 Doug Turner (:dougt) 2012-04-04 14:34:41 PDT
Busted build. Backed out.  https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe4b3fbd351
Comment 28 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-04-05 12:50:56 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d3fb71fc5292
Comment 29 Matt Brubeck (:mbrubeck) 2012-04-06 11:33:59 PDT
https://hg.mozilla.org/mozilla-central/rev/d3fb71fc5292

Note You need to log in before you can comment on or make changes to this bug.