Closed
Bug 554790
Opened 14 years ago
Closed 14 years ago
Implement variadic ctypes functions
Categories
(Core :: js-ctypes, defect, P2)
Core
js-ctypes
Tracking
()
RESOLVED
FIXED
People
(Reporter: dwitte, Assigned: mozilla+ben)
References
Details
(Keywords: dev-doc-needed)
Attachments
(3 files, 3 obsolete files)
26.31 KB,
patch
|
dwitte
:
review+
|
Details | Diff | Splinter Review |
14.14 KB,
patch
|
Details | Diff | Splinter Review | |
30.23 KB,
patch
|
dwitte
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
If I'ma be a js-ctypes peer, I might as well get my feet wet.
Assignee: nobody → bnewman
Assignee | ||
Comment 2•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
Another possibility is to use the string "..." for this: let f = ctypes.FunctionType(ctypes.default_abi, ctypes.void_t, [ ctypes.int, ctypes.double, "..." ]);
Reporter | ||
Comment 5•14 years ago
|
||
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...)
Assignee | ||
Comment 7•14 years ago
|
||
Existing xpcshell tests all pass, at least.
Reporter | ||
Comment 8•14 years ago
|
||
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 :)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #435745 -
Attachment is obsolete: true
Attachment #436117 -
Flags: review?(dwitte)
Assignee | ||
Comment 10•14 years ago
|
||
(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.
Assignee | ||
Comment 11•14 years ago
|
||
Reporter | ||
Comment 12•14 years ago
|
||
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.
Attachment #436117 -
Flags: review?(dwitte) → review+
Assignee | ||
Comment 13•14 years ago
|
||
Running this past the tryserver as I type.
Attachment #436118 -
Attachment is obsolete: true
Assignee | ||
Comment 14•14 years ago
|
||
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?
Assignee | ||
Comment 15•14 years ago
|
||
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.
Reporter | ||
Comment 16•14 years ago
|
||
(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!
Assignee | ||
Comment 18•14 years ago
|
||
This time without the BS that snuck in just before the license at the top of jsctypes-test.cpp.
Attachment #436397 -
Attachment is obsolete: true
Attachment #436408 -
Flags: review?(dwitte)
Attachment #436397 -
Flags: review?(dwitte)
Reporter | ||
Comment 19•14 years ago
|
||
Comment on attachment 436408 [details] [diff] [review] Tryserver-approved patch v2. Land it!
Attachment #436408 -
Flags: review?(dwitte) → review+
Assignee | ||
Comment 20•14 years ago
|
||
(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
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Doesn't seem documented.
Keywords: dev-doc-needed
Comment 22•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•