Last Comment Bug 554790 - Implement variadic ctypes functions
: Implement variadic ctypes functions
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Ben Newman (:bnewman) (:benjamn)
:
Mentors:
Depends on: 513778 565598
Blocks: 1290170
  Show dependency treegraph
 
Reported: 2010-03-24 16:07 PDT by dwitte@gmail.com
Modified: 2016-07-28 10:31 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Untested WIP patch, theoretically complete. (8.77 KB, patch)
2010-03-29 16:13 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Patch addressing dwitte's comments, with tests. (26.31 KB, patch)
2010-03-30 21:11 PDT, Ben Newman (:bnewman) (:benjamn)
dwitte: review+
Details | Diff | Splinter Review
Interdiff between the previous two patches. (21.52 KB, patch)
2010-03-30 21:27 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Incremental patch addressing feedback in comment #12. (14.14 KB, patch)
2010-03-31 14:37 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Tryserver-approved patch. (30.25 KB, patch)
2010-03-31 19:02 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Tryserver-approved patch v2. (30.23 KB, patch)
2010-03-31 19:47 PDT, Ben Newman (:bnewman) (:benjamn)
dwitte: review+
Details | Diff | Splinter Review

Description dwitte@gmail.com 2010-03-24 16:07:12 PDT
Syntax would be something like

  let f = ctypes.FunctionType(ctypes.default_abi, ctypes.void_t, [ ctypes.int, ctypes.double, ctypes.varargs ]);

or something more imaginative. I *think* varargs implies cdecl on all platforms -- needs checking. So we could actually omit the ABI part, or -- crazy idea -- use ctypes.varargs_abi.

Now, contrary to varargs in C, we actually need to know all the types of the arguments in order for libffi to assemble them onto the stack correctly. Thus we would require, at call time, that each argument be a CData proper -- no ImplicitConvert.

So, once the syntax is nailed, the rest is easy.
Comment 1 Ben Newman (:bnewman) (:benjamn) 2010-03-24 18:19:54 PDT
If I'ma be a js-ctypes peer, I might as well get my feet wet.
Comment 2 Ben Newman (:bnewman) (:benjamn) 2010-03-24 19:23:34 PDT
(In reply to comment #0)
> I *think* varargs implies cdecl on all platforms
> -- needs checking.

Let's try to find out the good ol' lazy way:
http://stackoverflow.com/questions/2512746/in-c-do-variadic-functions-those-with-at-the-end-of-the-parameter-list-n
Comment 3 Jason Orendorff [:jorendorff] 2010-03-24 20:28:16 PDT
I like the first proposed syntax. What should ctypes.varargs be? I guess some kind of special otherwise-useless object, like the ABI objects but not an ABI.

Let's keep varargs and ABI separate. They're not quite orthogonal, but close enough. declare() and FunctionType() should throw if the specified calling convention doesn't support varargs.
Comment 4 Jason Orendorff [:jorendorff] 2010-03-25 10:49:19 PDT
Another possibility is to use the string "..." for this:

  let f = ctypes.FunctionType(ctypes.default_abi, ctypes.void_t, [ ctypes.int,
ctypes.double, "..." ]);
Comment 5 dwitte@gmail.com 2010-03-25 11:14:48 PDT
Let's do "...", it seems closer to C and a bit less silly than having a dummy ctypes.varargs object. (Although that's only slightly more silly than ctypes.{default,stdcall}_abi, but whatev...)
Comment 6 dwitte@gmail.com 2010-03-25 14:02:56 PDT
P2, nice to have for 1.9.3.
Comment 7 Ben Newman (:bnewman) (:benjamn) 2010-03-29 16:13:45 PDT
Created attachment 435745 [details] [diff] [review]
Untested WIP patch, theoretically complete.

Existing xpcshell tests all pass, at least.
Comment 8 dwitte@gmail.com 2010-03-29 17:25:35 PDT
Comment on attachment 435745 [details] [diff] [review]
Untested WIP patch, theoretically complete.

>+static bool
>+PrepareCIF(JSContext* cx,
>+           FunctionInfo* fninfo)

Convention in jsengine, and here, is for functions that fail and report an error to have a return type of JSBool. Functions that don't report an error can be bool. This one needs to be JSBool.

> static FunctionInfo*
> NewFunctionInfo(JSContext* cx,
>                 jsval abiType,
>                 jsval returnType,
>                 jsval* argTypes,
>                 uintN argLength)
> {

>   for (PRUint32 i = 0; i < argLength; ++i) {
>+    if (IsEllipsis(argTypes[i])) {

Should check here if i == argLength - 1; if it's not, throw an error "'...' must be the last parameter for a variadic function".

>+  FunctionInfo* forgotten = fninfo.forget();
>+  
>+  if (forgotten->mIsVariadic)
>+    // wait to PrepareCIF until function is called
>+    return forgotten;

Hm. How about 'return fninfo.forget()' here, and then

>+  if (!PrepareCIF(cx, forgotten)) {
>+    delete forgotten;

you don't need the 'delete', and

>+  return forgotten;

'return fninfo.forget()' here too.

>+typedef nsAutoTArray<AutoValue, 16> AutoValueAutoArray;

<grin>

>+static bool
>+ConvertArgument(JSContext* cx,
>+                jsval arg,
>+                JSObject* type,
>+                AutoValueAutoArray* values,
>+                AutoValueAutoArray* strings)
>+{

JSBool return type here too, please.

>@@ -4492,35 +4564,43 @@ FunctionType::Call(JSContext* cx,
>+  if (fninfo->mIsVariadic) {
>+    JS_ASSERT(fninfo->mFFITypes.Length() >= argcFixed);

It's slightly more work, but I think I'd prefer that we assert that 'fninfo->mFFITypes.Length() == argcFixed' here, and truncate it again after the call. Seems more foolproof.

>+    fninfo->mFFITypes.SetLength(argcFixed);

Hm. Can you SetCapacity(argc) here instead?

>+    JSObject* obj;  // Could reuse obj instead of declaring a second
>+    JSObject* type; // JSObject*, but readability would suffer.
>+
>+    for (PRUint32 i = argcFixed; i < argc; ++i)
>+      if (JSVAL_IS_PRIMITIVE(argv[i]) ||
>+          !CData::IsCData(cx, obj = JSVAL_TO_OBJECT(argv[i])) ||
>+          !(type = CData::GetCType(cx, obj)) ||
>+          !(type = PrepareType(cx, OBJECT_TO_JSVAL(type))) ||
>+          !ConvertArgument(cx, argv[i], type, &values, &strings) ||
>+          !fninfo->mFFITypes.AppendElement(CType::GetFFIType(cx, type)))
>+        return false;

Wow. Some crazy stuff going on here. :)

Sad to say, but this has to be split up. Some of those terms will throw, and some not, but this function must consistently throw on failure. See other uses of JSVAL_TO_PRIMITIVE in this file for examples. A comment about a) why we require a CData object and b) why ImplicitConvert is the right thing for these conversions would also be great.

This looks neat so far. You'll need to go through the following places and burn in knowledge of variadicity:

http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#1997 (serialize the '...')
http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#2084 (same)
http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#2645 (add variadicity as an equality condition)
http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#4590 (add '...' to the end of the array... or add an .isVariadic property to FunctionType.prototype? Hmm.)
http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#4452 (throw if you try to construct a variadic closure)
http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#4657 (assert the same)

And of course, a test or two wouldn't hurt :)
Comment 9 Ben Newman (:bnewman) (:benjamn) 2010-03-30 21:11:32 PDT
Created attachment 436117 [details] [diff] [review]
Patch addressing dwitte's comments, with tests.
Comment 10 Ben Newman (:bnewman) (:benjamn) 2010-03-30 21:22:38 PDT
(In reply to comment #8)
> Convention in jsengine, and here, is for functions that fail and report an
> error to have a return type of JSBool. Functions that don't report an error can
> be bool. This one needs to be JSBool.

Changed.

> Should check here if i == argLength - 1; if it's not, throw an error "'...'
> must be the last parameter for a variadic function".

Good idea, done.

> >+  FunctionInfo* forgotten = fninfo.forget();
> >+  
> >+  if (forgotten->mIsVariadic)
> >+    // wait to PrepareCIF until function is called
> >+    return forgotten;
> 
> Hm. How about 'return fninfo.forget()' here, and then
> 
> >+  if (!PrepareCIF(cx, forgotten)) {
> >+    delete forgotten;
> 
> you don't need the 'delete', and

I'd forgotten (heh) whether passing an nsAutoPtr by value transferred ownership or not.  Fortunately it does not.

> >+typedef nsAutoTArray<AutoValue, 16> AutoValueAutoArray;
> 
> <grin>

I call 'em like I see 'em.

> >+static bool
> >+ConvertArgument(JSCont
...
> JSBool return type here too, please.

Done.

> It's slightly more work, but I think I'd prefer that we assert that
> 'fninfo->mFFITypes.Length() == argcFixed' here, and truncate it again after the
> call. Seems more foolproof.

Wrote an AutoLengthTruncator stack class.  Overkill?

> Hm. Can you SetCapacity(argc) here instead?

Yes, now that I'm truncating the length back to argcFixed every time, I can.

> Sad to say, but this has to be split up. Some of those terms will throw, and
> some not, but this function must consistently throw on failure. See other uses
> of JSVAL_TO_PRIMITIVE in this file for examples. A comment about a) why we
> require a CData object and b) why ImplicitConvert is the right thing for these
> conversions would also be great.

No more Epic Disjunction of Negations.  See what you think.

> This looks neat so far. You'll need to go through the following places and burn
> in knowledge of variadicity:
> 
> http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#1997
> (serialize the '...')
> http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#2084 (same)
> http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#2645 (add
> variadicity as an equality condition)

Done, done, and done.

> http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#4590 (add
> '...' to the end of the array... or add an .isVariadic property to
> FunctionType.prototype? Hmm.)

Added the .isVariadic property instead.

> http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#4452 (throw
> if you try to construct a variadic closure)
> http://mxr.mozilla.org/mozilla-central/source/js/ctypes/CTypes.cpp#4657 (assert
> the same)

Throwing and asserting.

> And of course, a test or two wouldn't hurt :)

Added some basic tests, particularly to make sure I'm doing the right thing when arguments are promoted to larger types during the call.
Comment 11 Ben Newman (:bnewman) (:benjamn) 2010-03-30 21:27:54 PDT
Created attachment 436118 [details] [diff] [review]
Interdiff between the previous two patches.
Comment 12 dwitte@gmail.com 2010-03-31 10:43:10 PDT
Comment on attachment 436117 [details] [diff] [review]
Patch addressing dwitte's comments, with tests.

>@@ -2105,19 +2111,22 @@ BuildTypeSource(JSContext* cx, JSObject*

>+      if (fninfo->mIsVariadic)
>+        result.Append(NS_LITERAL_STRING("..."));

We want the '...' wrapped in quotes, here, just like a declaration would, so that it roundtrips through eval():

  result.Append(NS_LITERAL_STRING("\"...\""));

> static FunctionInfo*
> NewFunctionInfo(JSContext* cx,

>   for (PRUint32 i = 0; i < argLength; ++i) {
>+    if (IsEllipsis(argTypes[i])) {
>+      fninfo->mIsVariadic = true;
>+      if (i < argLength - 1) {
>+        JS_ReportError(cx, "'...' must be the last parameter type of a variadic "
>+                       "function declaration");

Come to think of it, it '...' may also not be the first (and only) argument, right? Or can one declare such a C function? Should error on that, too. And have a testcase, plz!

>@@ -4448,16 +4504,21 @@ FunctionType::ConstructData(JSContext* c

>+      FunctionInfo* fninfo = FunctionType::GetFunctionInfo(cx, obj);
>+      if (fninfo->mIsVariadic) {
>+        JS_ReportError(cx, "Can't pass a variadic function as a callback");

"Can't declare", perhaps?

>@@ -4510,52 +4615,78 @@ FunctionType::Call(JSContext* cx,

>-  if (argc != fninfo->mArgTypes.Length()) {
>+  if (!fninfo->mIsVariadic &&
>+      argc != fninfo->mArgTypes.Length()) {
>     JS_ReportError(cx, "Number of arguments does not match declaration");

Don't we also need to check mIsVariadic && argc > mArgTypes.Length()?

>+    fninfo->mFFITypes.SetCapacity(argc);

SetCapacity has an rv... can check it and report, and then...

>+      if (!fninfo->mFFITypes.AppendElement(CType::GetFFIType(cx, type))) {
>+        JS_ReportOutOfMemory(cx);

...no need to check the rv of AppendElement here; it's guaranteed to succeed.

>diff --git a/js/ctypes/CTypes.h b/js/ctypes/CTypes.h

>-// Descriptor of ABI, return type, and argument types for a FunctionType.
>+// Descriptor of ABI, return type, argument types, and variadicity for a
>+// FunctionType.
> struct FunctionInfo
> {
>   ffi_cif mCIF;
>   JSObject* mABI;
>   JSObject* mReturnType;
>   nsTArray<JSObject*> mArgTypes;
>   nsTArray<ffi_type*> mFFITypes;
>+  bool mIsVariadic;

Hmm, it's about time we commented this beast. Can you add comments to each field, a la FieldInfo, with a brief description? Also mention that mCIF is... variable... for variadic functions, and thus is meaningless outside the context of a particular call.

Come to think of it, I changed my mind about mFFITypes being truncated. Seems a bit silly given that mCIF isn't reset. So you can change that back, if you want (sorry about your AutoLengthTruncator, its life was so tragically cut short); and comment it here; something to the effect of "the state of mCIF and mFFITypes will not necessarily be consistent, and can change between function calls".

>diff --git a/js/ctypes/tests/jsctypes-test.cpp b/js/ctypes/tests/jsctypes-test.cpp

>+void
>+test_add_char_short_int_va_cdecl(PRUint32* result, ...)
>+{
>+  va_list list;
>+  va_start(list, result);
>+  *result += va_arg(list, PromotedTraits<char>::type);
>+  *result += va_arg(list, PromotedTraits<short>::type);
>+  *result += va_arg(list, PromotedTraits<int>::type);

Sooo... you discovered that the promotion happens in the callee, not the caller?

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

>+function run_variadic_tests(library) {

>+  let thrown = false;
>+  try {
>+    library.declare("test_bogus_va_signature", ctypes.default_abi, ctypes.bool,
>+                    ctypes.bool, "...", ctypes.bool);
>+    do_throw("should not have reached this line");
>+  } catch (x) { thrown = true }
>+  do_check_true(thrown);

Can just use do_check_throws for this :)

Also need to test the other cases you burnt in: that .toSource() on the type is right, that a variadic closure throws, that ImplicitConvert'ing between two identical variadic functions is OK but variadic/nonvariadic throws (aside: you can do this using .value:

  variadicFn.value = nonVariadicFn; // throws
  variadicFn.value = variadicFn; // OK

), and it's also worth checking that array variadic params are ImplicitConverted correctly to pointers -- have another binary function that takes 'n' and an array and adds their elements? -- and:

Ooh! I just remembered... we need to guard on the ABI being right, in NewFunctionInfo(). For now, that means cdecl only; throw if it's not. And then test that stdcall throws, #ifdef WIN32 && !WIN64 (see other stdcall tests).

If this works multiplat on tryserver, then that's a good sign the variadic promotion n'all is working right, and in the end trumps any docs that say how variadics should work.

r=me with fixes.
Comment 13 Ben Newman (:bnewman) (:benjamn) 2010-03-31 14:37:02 PDT
Created attachment 436303 [details] [diff] [review]
Incremental patch addressing feedback in comment #12.

Running this past the tryserver as I type.
Comment 14 Ben Newman (:bnewman) (:benjamn) 2010-03-31 14:48:27 PDT
Anything I didn't comment on below, I fixed.

(In reply to comment #12)
> Come to think of it, it '...' may also not be the first (and only) argument,
> right? Or can one declare such a C function? Should error on that, too. And
> have a testcase, plz!

There's actually nothing wrong with it, technically, but having no explicit parameters does make it impossible to use va_start (which requires the name of the last non-variadic parameter), so the function has to be pretty dumb; e.g.,

  int zero(...) { return 0; }

I made it an error anyway.

> Don't we also need to check mIsVariadic && argc > mArgTypes.Length()?

Not quite.  The new condition is

    if ((!fninfo->mIsVariadic && argc != argcFixed) ||
        (fninfo->mIsVariadic && argc < argcFixed)) {
      JS_ReportError(cx, ...);
      return false;
    }

> Hmm, it's about time we commented this beast. Can you add comments to each
> field, a la FieldInfo, with a brief description? Also mention that mCIF is...
> variable... for variadic functions, and thus is meaningless outside the context
> of a particular call.

You'll want to proofread the comments I added, regardless of how the tryserver feels about them ;)

> Come to think of it, I changed my mind about mFFITypes being truncated. Seems a
> bit silly given that mCIF isn't reset. So you can change that back, if you want
> (sorry about your AutoLengthTruncator, its life was so tragically cut short);
> and comment it here; something to the effect of "the state of mCIF and
> mFFITypes will not necessarily be consistent, and can change between function
> calls".

Reverted.

> >diff --git a/js/ctypes/tests/jsctypes-test.cpp b/js/ctypes/tests/jsctypes-test.cpp
> 
> >+void
> >+test_add_char_short_int_va_cdecl(PRUint32* result, ...)
> >+{
> >+  va_list list;
> >+  va_start(list, result);
> >+  *result += va_arg(list, PromotedTraits<char>::type);
> >+  *result += va_arg(list, PromotedTraits<short>::type);
> >+  *result += va_arg(list, PromotedTraits<int>::type);
> 
> Sooo... you discovered that the promotion happens in the callee, not the
> caller?

Yeah, it looks that way.  Can you think of a more thorough test?
Comment 15 Ben Newman (:bnewman) (:benjamn) 2010-03-31 14:53:41 PDT
Comment on attachment 436303 [details] [diff] [review]
Incremental patch addressing feedback in comment #12.

>diff --git a/js/ctypes/tests/unit/test_jsctypes.js.in b/js/ctypes/tests/unit/test_jsctypes.js.in
>--- a/js/ctypes/tests/unit/test_jsctypes.js.in
>+++ b/js/ctypes/tests/unit/test_jsctypes.js.in
>@@ -48,18 +48,20 @@ const Ci = Components.interfaces;
> function do_check_throws(f, type, stack)
> {
>   if (!stack)
>     stack = Components.stack.caller;
> 
>   try {
>     f();
>   } catch (exc) {
>-    if (exc instanceof type)
>+    if (exc instanceof type) {
>+      do_check_true(true);
>       return;
>+    }
>     do_throw("expected " + type.name + " exception, caught " + exc, stack);
>   }
>   do_throw("expected " + type.name + " exception, none thrown", stack);
> }

I also made this unsolicited change, so that do_check_throws prints a message (instead of nothing) when it passes:

    TEST-PASS | ... | [do_check_throws : 57] true == true

When you have several do_check_throws in a row, this change helps track down which one is failing.
Comment 16 dwitte@gmail.com 2010-03-31 15:17:12 PDT
(In reply to comment #14)
> Yeah, it looks that way.  Can you think of a more thorough test?

Pass in a bunch of different sizes in different orders, such that if they were required to be word-aligned on the stack in the caller, things would break... for instance: char, short, int, char, double, short, int... basically like what your char, short, int test does. :)

> When you have several do_check_throws in a row, this change helps track down
> which one is failing.

Nice.

> You'll want to proofread the comments I added, regardless of how the tryserver
> feels about them ;)

Looks OK!
Comment 17 Ben Newman (:bnewman) (:benjamn) 2010-03-31 19:02:21 PDT
Created attachment 436397 [details] [diff] [review]
Tryserver-approved patch.

Ready to land?
Comment 18 Ben Newman (:bnewman) (:benjamn) 2010-03-31 19:47:28 PDT
Created attachment 436408 [details] [diff] [review]
Tryserver-approved patch v2.

This time without the BS that snuck in just before the license at the top of jsctypes-test.cpp.
Comment 19 dwitte@gmail.com 2010-04-01 10:26:01 PDT
Comment on attachment 436408 [details] [diff] [review]
Tryserver-approved patch v2.

Land it!
Comment 20 Ben Newman (:bnewman) (:benjamn) 2010-04-01 10:51:36 PDT
(In reply to comment #19)
> (From update of attachment 436408 [details] [diff] [review])
> Land it!

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/faf4147ffe7f
Comment 21 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-06 06:51:15 PDT
Doesn't seem documented.
Comment 22 Eric Shepherd [:sheppy] 2014-11-24 06:08:31 PST
No, this isn't documented yet (says Sheppy, 2.5 years or so later). It's been pointed out on dev-extensions that the tests included in the push in comment 20 can be helpful. Here's a link to the JS itself: http://hg.mozilla.org/mozilla-central/diff/faf4147ffe7f/js/ctypes/tests/unit/test_jsctypes.js.in

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