FunctionType.ptr doesn't expose call/apply

RESOLVED FIXED in mozilla14

Status

()

Core
js-ctypes
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Fallen, Assigned: jdm)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

6 years ago
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"))
(Assignee)

Comment 1

6 years ago
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! :-)
(Reporter)

Comment 3

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 576826 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.

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-
(Assignee)

Comment 7

6 years ago
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?
(Assignee)

Comment 10

5 years ago
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.
Created attachment 594537 [details] [diff] [review]
Complete patch

Here is a cleaned-up patch. Working on the testsuite.
Attachment #594537 - Flags: review?(jorendorff)
Created attachment 594542 [details] [diff] [review]
Partial testsuite

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

Comment 16

5 years ago
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?
Attachment #594614 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 18

5 years ago
Created attachment 597088 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.
Attachment #597088 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
Attachment #594614 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 597091 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.
Attachment #597091 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 21

5 years ago
Created attachment 597159 [details] [diff] [review]
Make ctypes function pointers callable via call and apply.
Attachment #597159 - Flags: review?(bobbyholley+bmo)
(Assignee)

Updated

5 years ago
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)
Blocks: 563742
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+
(Assignee)

Comment 25

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/fd4b5485c24f
Target Milestone: --- → mozilla14
Busted build. Backed out.  https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe4b3fbd351
Landing: https://hg.mozilla.org/mozilla-central/rev/fd4b5485c24f
Backout: https://hg.mozilla.org/mozilla-central/rev/2fe4b3fbd351
(Assignee)

Comment 28

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/d3fb71fc5292
https://hg.mozilla.org/mozilla-central/rev/d3fb71fc5292
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.