This will track the work needed to get js-ctypes on trunk for 1.9.2. I'll comment with more details here once I've got a better idea of what needs to be done...
Note that mozilla-central is currently 1.9.3, we already branched 1.9.2. If you really do want this on 1.9.2 you'll have to land it there as well.
Yeah, I meant trunk and 1.9.2. ;) Changing summary.
This is not blocking but is desperately wanted.
Okay. I've got js-ctypes building on trunk, got the x86_64 ABI working, and generally started cleaning things up. A few todo's: * Tidy up struct support. This looks really easy to do so I'll just do it now. * Pull libffi 3.0.8. We're currently on a release from 2007 (pre-2.0 I think?); the newer one has improved ABI support, is better optimized, works on many more platforms, and (more importantly) has been tested on them. (According to the docs, anyway.) * Ditch the XPCOM impl and move to JSAPI. This shouldn't be hard either, so I may start that now. * Tweak the API. * Add more unit tests and get platform coverage. WRT landing, it's currently on its own little svn repo. Since it's never been reviewed (I think?), what I'll do is just finish my work, get it to a committable state, and then get review on the whole thing. It's not a whole lot of code, so this should be tractable, but if anyone has a better plan let me know!
How much of that additional work is *necessary*? In particular "ditch the XPCOM impl and move to JSAPI" sounds like schedule risk that I wary of assuming we want to get this into 3.6. Still not a blocker, though.
Er, sorry about the blocking flag change, for some reason that got twiddled... I'd consider everything except JSAPI necessary for landing, given that they're easy to do. ("Necessary" in the sense that "we want to ship something good", not just "it works".) I'll leave the JSAPI part til after the rest gets landed, because I think you're right... I don't know if we'll hit snags or not.
Created attachment 400236 [details] [diff] [review] land libffi 3.0.8 on trunk Alright. This adds libffi to the trunk, under toolkit/components/js-ctypes/src/libffi. I've made this patch separate since it's huge, and not very useful to read. (Makefile changes are in the next patch.) I've pruned it down pretty well, but it's still huge - their configure script is almost a meg alone. It's ridiculous for such a small library; I'm sure we can pare it down much further, but we can always do that later. Per the next patch, we run their configure script to generate their header files, then we build the source using our own build system.
Created attachment 400237 [details] [diff] [review] land js-ctypes, v1 Here's the meat. I've hacked this up considerably from mfinkle's svn repo. It's still got some rough edges, but is getting close. Some highlights: * Strong typing. We figure out the function argument types at declaration time, and store these for later use. This means we can throw TypeErrors if a wrong type gets passed in at call time. * Struct support. This works for nested structs as arguments (but not return vals yet - easy enough to do, but see later.) See xpcshell test for examples. Mardak, jorendorff, myk and I had a chat today about a struct/pointer API. I'll file a separate bug for this, but it's a bit more elegant than the _fields_ thing we do now. I'll get working on it next, and I didn't want to spend time getting struct return vals working since the new API will make it redundant. The JSAPI type mangling needs a bit of polishing - jorendorff posted a patch in bug 516077 just now, so I'll incorporate those on Monday. I'll work on de-XPCOMifying the API a bit, too.
Comment on attachment 400237 [details] [diff] [review] land js-ctypes, v1 As for the API, I still want the API to be: var ctypes = Components.utils.import("resource://gre/modules/ctypes.js"); var types = ctypes.types; var library = ctypes.open(file); No Ci.nsINativeTypes. No Cc["@developer.mozilla.org/js-ctypes;1"] Although under the hood that's what it would map) The pointer test is all wrong. When you pass in a pointer, you really need to create a wrapper object because in general the point of pointers is to modify the underlying value: var p = new ctypes.pointer(ctypes.types.INT16); p.value = 50; test_s_echo(p); // p is an outparam, test_s_echo will set it to 42 do_check_eq(p.value, 42);
I'd like this to live in js/ctypes and js/ctypes/libffi. cc'ing brendan for confirmation. I'm concerned about "I've pruned it down". It sounds like you're only importing pieces of libffi, or actually altering the sources. I'd prefer to take the entirety of libffi from a known release/revision/tag rather than do any patching at all. This is what we do with breakpad and other projects. For bonus points, add the import script to client.py (which is where the NSPR/NSS import scripts live). Then I only need to review the script, and not the external code. Why do we run their configure script for headers but then build with our own build system? Is there some reason we can't build using their system? It sounds easy for us to get variables or other important details out of sync.
js/ctypes, righteous. Will move js/src/xpconnect to js/xpcom so we can study old vs. new in a more parallel structure. /be
(In reply to comment #9) > As for the API, I still want the API to be: On) I think I tried using nsIXULRuntime.OS but the interface wasn't available from xpcshell, or somesuch. Will try the preprocessor. Since consumers will face the same kinds of problem, I think I'll add a convenience function similar to what pyctypes does - e.g. ctypes.open("foo") would open libfoo.so on linux, foo.dl on windows, and foo.dylib or somesuch on mac. (Phase 2 though!) > The pointer test is all wrong. Yup, just a holdover from the svn repo. They're commented out, but I'll just remove them. (In reply to comment #10) > I'm concerned about "I've pruned it down". I omitted files that we don't need (it comes with a whole bunch of libtool goo, and docs), and then made a one-line patch to libffi/configure to remove those Makefiles references. I figured the lib was enormous enough as it is, but if that's not a problem we can land the thing wholesale, for sure. > Why do we run their configure script for headers but then build with our own > build system? Is there some reason we can't build using their system? It sounds > easy for us to get variables or other important details out of sync. The short answer is that I could use some help here... if we build with their system, we need to map some of our make targets (clean, etc) onto theirs. We also end up with a bunch of unused symbols linked in - the raw & closure API's, and some stuff from libffi/src/debug.c. (My linker didn't discard them, even when #including gcc-hidden.h, but I stopped investigating there.) Other than that, "configure --with-pic --enable-static --disable-shared" will get us a static lib we can link against - they have an "--enable-debug", too. (We might also want to fiddle with CFLAGS to get hidden vis or other stuff in there.) If you or someone else can help out with this part it would rock!
(In reply to comment #12) > I think I tried using nsIXULRuntime.OS but the interface wasn't available from > xpcshell, or somesuch. Will try the preprocessor. Yeah, that singleton isn't available in xpcshell in non-libxul builds. I filed bug 475965 on fixing that a while ago.
Created attachment 400907 [details] [diff] [review] js-ctypes, v2 Addresses review comments. I tweaked client.py per suggestion, so you can pull via |client.py libffi_v3_0_8|. Also incorporates jorendorff's strict-value-checking awesomeness (bug 516077) with a tweak or two, and adds in unsigned integer types. I ripped out struct support in this patch, somewhat to my dismay, but the interface is wrong and we really don't want to ship like that. Plenty of work for a followup!.) Ready for review!
>. We are going to need to support the standard calling conventions (cdecl, stdcall/WINAPI). I presume cdecl is the current default? We'll probably want stdcall for 3.6 if possible since all the Windows (WINAPI) functions are stdcall. >.) Right now we should land whatever we have. Yes, we should provide int/long types which match the standard compiler version so that code which is written with C types doesn't need to know the width on each platform.
. From libffi/src/x86/ffitarget.h: #ifdef X86_WIN32 FFI_SYSV, FFI_STDCALL, /* TODO: Add fastcall support for the sake of completeness */ FFI_DEFAULT_ABI = FFI_SYSV, #endif I'm guessing FFI_SYSV is cdecl, but I'm not sure. > Right now we should land whatever we have. Yes, we should provide int/long > types which match the standard compiler version so that code which is written > with C types doesn't need to know the width on each platform. Sounds good. I'll tweak those bits in a followup.
(In reply to comment #14) > I tweaked client.py per suggestion, so you can pull via |client.py libffi_v3_0_8| ./client.py update_libffi libffi_v3_0_8, that is.
. It has support for stdcall. I had defaulted WIN32 to sdtcall and everything else to cdecl
Comment on attachment 400907 [details] [diff] [review] js-ctypes, v2 Probably makes no difference at all, but I wanted to let you know the XPCOM contract is "@developer.mozilla.org.jsctypes" and not "@mozilla.org/jsctypes" Since the XPCOM is going away, this really isn't a concern, right?
"@developer.mozilla.org/jsctypes" and not "@mozilla.org/jsctypes"
Comment on attachment 400907 [details] [diff] [review] js-ctypes, v2 I don't think we need js/ctypes/Makefile.in. Just build the public/src/test directories directly. This means less makefile recursion, good for build speed. Also, is there a particular reason you used public/src directories? The new standard is to just use a single directory unless you really need to separate the headers for some reason. >diff --git a/js/ctypes/build.mk b/js/ctypes/build.mk >+TIERS += libffi >+ >+tier_libffi_staticdirs = js/ctypes/libffi I know I said to do this, but do we really need a *new* tier? Could we just make this part of tier_gecko? (tier_gecko_staticdirs). Also, since you've made this file, you might as well put js/ctypes/public and js/ctypes/src into tier_whatever_dirs instead of having that logic somewhere else. >diff --git a/js/ctypes/public/Makefile.in b/js/ctypes/public/Makefile.in >+MODULE = jsctypes >+XPIDL_MODULE = jsctypes You're adding a new XPT file: you need to make sure it ends up in browser/installer/package-manifest.in >diff --git a/js/ctypes/public/nsINativeTypes.idl b/js/ctypes/public/nsINativeTypes.idl >+/** >+ * Magical interface that implements nsIXPCScriptable >+ * so it can act as a callable object in JS >+ */ >+[scriptable, uuid(a54a5c6c-9529-418c-9d9f-b8111c6e247d)] >+interface nsINativeMethod : nsISupports >+{ >+}; Why is this interface necessary? Can't we just return an nsISupports? >+[scriptable, uuid(352a72c1-5b99-451e-a330-c45079d8f087)] >+interface nsINativeTypes : nsISupports >+{ >+ /** >+ * ABI constants that specify the calling convention to use. >+ * In almost all cases, DEFAULT is the correct choice, and will >+ * map to the common calling convention on each platform. >+ */ "common" is a relative term. The compiler-default calling convention on Windows is typically cdecl, even though Windows uses stdcall. I think we should default to cdecl uniformly. >+ const PRUint16 DEFAULT = 0; >+ const PRUint16 STDCALL = 1; >+ const PRUint16 SYSV = 2; Please rename this to CDECL. >+ const PRUint16 UNIX64 = 3; This either needs to be removed or needs some documentation (a link to a doc describing the calling convention would be sufficient). >+ const PRUint16 WSTRING = 13; // UTF16 string (PRUnichar *).. >diff --git a/js/ctypes/src/ctypes.jsm b/js/ctypes/src/ctypes.jsm >+ * The Initial Developer of the Original Code is >+ * Mozilla Corporation. Technically it's "The Mozilla Founcation <>" because they own the copyright for all MoCo-produced stuff. >diff --git a/js/ctypes/src/nsNativeMethod.cpp b/js/ctypes/src/nsNativeMethod.cpp >+template<class IntegerType> >+static bool >+jsvalToIntStrict(jsval aValue, IntegerType *aResult) >+{ >+ if (JSVAL_IS_INT(aValue)) { ... >+ if (JSVAL_IS_DOUBLE(aValue)) { ... >+ if (JSVAL_IS_BOOLEAN(aValue)) { ... >+ } >+ // Don't silently convert null to an integer. It's probably a mistake. >+ return false; >+} Here and below you aren't dealing with JSVAL_IS_OBJECT: what happens if you pass `new Number(16)` through this? It seems like some ToPrimitive step needs to happen, although I'm not sure the JSAPI >+static bool >+jsvalToDoubleStrict(jsval aValue, jsdouble *dp) >+{ >+ // Don't silently convert true to 1.0 or false to 0.0, even though C/C++ >+ // does it. It's likely to be a mistake. Hrm, strict-typing in JS seems weird. Normally if you passed a string as "3.14159" it would be auto-converted to a double... are you saying you don't want that? How un-JSey... >+static nsresult >+GetABI(PRUint16 aCallType, ffi_abi& aResult) >+{ >+ // determine the ABI from the subset of those available on the >+ // given platform. we provide some common ones; otherwise, >+ // the consumer can specify nsINativeTypes::DEFAULT, for which >+ // libffi will pick something sane for the given platform. Boy, this makes me nervous. What default does libffi pick for Windows, where windows and C code have different defaults? >+#elif defined(__i386__) || defined(__x86_64__) >+ case nsINativeTypes::SYSV: >+ aResult = FFI_SYSV; >+ return NS_OK; >+ case nsINativeTypes::UNIX64: >+ aResult = FFI_UNIX64; >+ return NS_OK; Why no stdcall here? We certainly *can* have stdcall functions on Linux, although they are unusual. >+ case nsINativeTypes::STRING: >+ // Don't implicitly convert to string. Users can implicitly convert >+ // with `String(x)` or `""+x`. >+ if (!JSVAL_IS_STRING(aValue)) >+ return TypeError(aContext, "Expected string"); >+ >+ aResult.mValue.mPointer = JS_GetStringBytes(JSVAL_TO_STRING(aValue)); Hrmph... JS is an auto-converting language! This doesn't make me happy. However, what makes me even more unhappy is that JSVAL_VOID is not handled here: we should handle that correctly by passing null in for mValue.mPointer >+nsresult >+nsNativeMethod::Init(JSContext* aContext, >+ nsNativeTypes* aLibrary, >+ PRFuncPtr aFunc, >+ PRUint16 aCallType, >+ jsval aResultType, >+ const nsTArray<jsval>& aArgTypes) >+{ >+ nsresult rv; >+ JSAutoRequest ar(aContext); This method is always called from inside a request. I don't think the extra request is good. Is there a method to assert that we're currently in a request? >+ ffi_status status = ffi_prep_cif(&mCIF, mCallType, mFFITypes.Length(), >+ &mResultType.mType, mFFITypes.Elements()); >+ switch (status) { >+ case FFI_OK: >+ return NS_OK; >+ case FFI_BAD_ABI: >+ JS_ReportError(aContext, "Invalid ABI specification"); >+ return NS_ERROR_INVALID_ARG; >+ case FFI_BAD_TYPEDEF: >+ JS_ReportError(aContext, "Invalid type specification"); >+ return NS_ERROR_INVALID_ARG; >+ default: >+ return NS_ERROR_INVALID_ARG; >+ } The default case should report an error also, if only "unknown libffi error". >+nsresult >+nsNativeMethod::Execute(JSContext* aContext, PRUint32 aArgc, jsval* aArgv, jsval* aValue) >+{ >+ nsresult rv; >+ JSAutoRequest ar(aContext); ... >+ ffi_call(&mCIF, mFunc, resultValue.mData, values.Elements()); Does the Call method have a request when it's entered (I would think so, but I'm not sure). If so, the JSAutoRequest shouldn't be repeated. The request should be suspended during the ffi_call, as it's potentially a blocking method. >+/* readonly attribute string className; */ >+NS_IMETHODIMP nsNativeMethod::GetClassName(char * *aClassName) >+{ >+ NS_ENSURE_ARG_POINTER(aClassName); >+ *aClassName = (char *) nsMemory::Clone("nsNativeMethod", 12); Use NS_strdup >+/* PRBool call (in nsIXPConnectWrappedNative wrapper, in JSContextPtr cx, in JSObjectPtr obj, in PRUint32 argc, in JSValPtr argv, in JSValPtr vp); */ >+NS_IMETHODIMP nsNativeMethod::Call(nsIXPConnectWrappedNative *wrapper, JSContext * cx, >+ JSObject * obj, PRUint32 argc, jsval * argv, jsval * vp, PRBool *_retval) >+ if (!mFunc) { >+ JS_ReportError(cx, "Function is null"); >+ *_retval = PR_FALSE; >+ return NS_ERROR_FAILURE; >+ } This seems like an unnecessary check, especially because this is a common codepath. Clients should never be able to reach a nsNativeMethod if mFunc is null, since mFunc is set in Init. >+ nsresult rv = Execute(cx, argc, argv, vp); Does the Call method have a request when it's entered? >+ if (NS_FAILED(rv)) { >+ *_retval = PR_FALSE; >+ return NS_ERROR_FAILURE; Why does Execute() return a nsresult if you're not actually going to use it like one? Better make it a bool. >+/* PRBool getProperty (in nsIXPConnectWrappedNative wrapper, in JSContextPtr cx, in JSObjectPtr obj, in JSVal id, in JSValPtr vp); */ >+NS_IMETHODIMP nsNativeMethod::GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext * cx, >+ JSObject * obj, jsval id, jsval * vp, PRBool *_retval) >+{ >+ *_retval = PR_FALSE; Assert that the nsIXPCScriptable methods you haven't asked for are not actually called. >+/* PRBool newResolve (in nsIXPConnectWrappedNative wrapper, in JSContextPtr cx, in JSObjectPtr obj, in JSVal id, in PRUint32 flags, out JSObjectPtr objp); */ >+NS_IMETHODIMP nsNativeMethod::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext * cx, >+ JSObject * obj, jsval id, PRUint32 flags, JSObject * *objp, PRBool *_retval) >+{ >+ *_retval = PR_TRUE; Why did you WANT_NEWRESOLVE? Just to allow clients to set custom properties on the function objects? >diff --git a/js/ctypes/src/nsNativeTypes.cpp b/js/ctypes/src/nsNativeTypes.cpp >+/* void open (in nsILocalFile file); */ >+NS_IMETHODIMP nsNativeTypes::Open(nsILocalFile *aFile) >+{ >+ NS_ENSURE_ARG(aFile); >+ NS_ENSURE_TRUE(!mLibrary, NS_ERROR_ALREADY_INITIALIZED); >+ >+ return aFile->Load(&mLibrary);. >+/* void close (); */ >+NS_IMETHODIMP nsNativeTypes::Close() >+{ >+ if (mLibrary) { >+ PRStatus rv = PR_UnloadLibrary(mLibrary); >+ >+ mLibrary = nsnull; >+ if (rv != PR_SUCCESS) >+ return NS_ERROR_FAILURE; >+ } I don't see any reason to propagate errors here: just return NS_OK in all cases. >+ nsRefPtr<nsNativeMethod> call = new nsNativeMethod; >+ rv = call->Init(ctx, this, func, callType, argv[2], argTypes); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ NS_ADDREF(*aResult = call); call.forget(aResult) >diff --git a/js/ctypes/tests/Makefile.in b/js/ctypes/tests/Makefile.in you need FORCE_SHARED_LIB=1. You do not want EXPORT_LIBRARY=1. You also want NO_DIST_INSTALL=1. I think you want to install the test library into the testing directory. Have you tested this on mac (tryserver)? I'm a little worried that this will create a .dylib, not a .bundle, and we won't be able to load it, but I'll be happen to be proven wrong. >+libs:: unit/test_jsctypes.js.in >+ $(PYTHON) $(MOZILLA_DIR)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) \ >+ $^ > $(xpctestdir)/test_jsctypes.js >+ $(RM) $(xpctestdir)/test_jsctypes.js.in The RM shouldn't be necessary... we don't ever put that file there... >diff --git a/js/ctypes/tests/jsctypes-test.cpp b/js/ctypes/tests/jsctypes-test.cpp >+const PRUnichar * >+test_wide_ret() >+{ >+ return NS_LITERAL_STRING("success").get(); This is not safe on systems where we don't have literal wchar equivalents. Use static const PRUnichar kSuccess[] = {'o', 'k', '\0'}; >diff --git a/js/ctypes/tests/unit/test_jsctypes.js.in b/js/ctypes/tests/unit/test_jsctypes.js.in >+function run_test() >+{ >+ var libfile = Cc["@mozilla.org/file/directory_service;1"] >+ .getService(Ci.nsIProperties) >+ .get("CurProcD", Ci.nsIFile); Once you've installed the test library into the testing directory, use do_get_file. >+ var library = ctypes.open(libfile); I'l like a test for the string version of .open as well. >+function run_struct_tests(library) { vestigial? I'd like tests for the Number(16) case I mentioned earlier.? I'm ok with this with nits fixed: please coordinate with jorendorff to fix the specific issue of jsvalToIntStrict accepting Number objects.
New patch forthcoming! (In reply to comment #21) > "common" is a relative term. The compiler-default calling convention on Windows > is typically cdecl, even though Windows uses stdcall. I think we should default > to cdecl uniformly. Done. We now have DEFAULT, which maps to libffi's idea of what cdecl is on each plat; and STDCALL, provided only on Win32. >. Good call. I added a USTRING for 16 bit strings, and removed WSTRING for now. To add it back, methinks we'll need a configure test to figure out wchar_t's size, which I can write later. > Here and below you aren't dealing with JSVAL_IS_OBJECT: what happens if you > pass `new Number(16)` through this? It seems like some ToPrimitive step needs > to happen, although I'm not sure the JSAPI So, in our discussions of pointer APIs, we've been leaning toward making it strict because otherwise things get pretty hairy. I think we should be careful about what we convert, and I'd like to postpone Number() and toString() autoconversion until after we have a better idea of pointer implementation. We can always loosen it up later... > This method is always called from inside a request. I don't think the extra > request is good. Is there a method to assert that we're currently in a request? Hmm. Not without #including jscntxt.h, which would probably make the JS guys cringe. I removed one superfluous JSAutoRequest, but kept the one in Call(). (Of course, if xpconnect guarantees that it takes out a request before invoking us, I could remove them.) AFAICT, taking out extra requests just increases the request refcount, which (other than lock price) doesn't sound too bad. > Why did you WANT_NEWRESOLVE? Just to allow clients to set custom properties on > the function objects? No idea, it was like that when I got here :) I removed NEWRESOLVE and made all the methods, other than Call(), NOTREACHED. >. Do you want the fix in nsLocalFile, or ctypes? (I couldn't find anywhere that loads .dylibs, so I'm not sure what the fix would look like yet. Will test.) >? Not sure. I'll follow up on it. libffi doesn't have any shared stuff AFAICT, apart from statically initialized globals.
Created attachment 401152 [details] [diff] [review] js-ctypes, v3 Carrying over r+, but let me know if you'd like further changes based on my comment above.
Comment on attachment 401152 [details] [diff] [review] js-ctypes, v3 Linux needs stdcall too! All of the Mozilla functions (NS_*) are stdcall on all x86 platforms. I don't understand the concern about Number() objects: it seems pretty important that number objects behave the exact same way as number literals.
(In reply to comment #24) > I don't understand the concern about Number() objects: it seems pretty > important that number objects behave the exact same way as number literals. Why? Number wrappers are extremely rare. Anyone creating one on purpose is either hanging ad-hoc properties off of the instance for obscure reasons, or possibly trying to amortize auto-wrapping overhead on old engines. I haven't seen code do anything like this in years. /be.
(In reply to comment #24) > Linux needs stdcall too! All of the Mozilla functions (NS_*) are stdcall on all > x86 platforms. libffi only supports SYSV on linux x86 :( But per discussion on irc, this doesn't appear to be a big deal since the NS_*'s are indeed cdecl on linux. (Those that aren't fastcall, anyway.)
Created attachment 401249 [details] [diff] [review] js-ctypes, v4 With a few more improvements. Tested on linux i386, x86-64, and OSX i386. Switching bsmedberg's r+ to sr+ per suggestion; jorendorff, could you please review? I'm thinking of changing the type constants in nsINativeTypes.idl to more JS-ey capitalization - ctypes.types.Int32, ctypes.types.String etc - since we'll be making them full JS classes soon. We should put them on the ctypes object itself when we do so, too. On OSX, the test lib gets linked as a .dylib, and everything works. So nothing more to do there. Re workers, bent says that Components isn't accessible to them - so it's a moot point for now, until we get extra machinery.
We should either match python ctypes as closely as possible or use standard _t names: int32_t etc...
I wasn't talking about DOM workers, BTW: I was talking about chrome code using the thread manager, creating a new thread and calling this from there.
(In reply to comment #26) >. You want (new Number(42)) and 42 to be acceptable for an int param -- what about "42"? If yes, I missed it above. If no, why not? Whatever "the normal JS" model is it implicitly converts in many contexts, and numeric ones (not + but all other dyadic numerical operators, e.g.) will convert "42" to 42. I don't think we should have implicit conversions interfacing to native code. It's better to be strict and avoid mistakes and attack surface. /be
Yes, I thought "42" should auto-convert also, but I wasn't going to press that as much. Since I'm in the minority, I'll cave.
This is great, nice work. Though I have a question regarding contributor status. I did some work in a couple of bugs and had some patches applied to SVN by mfinkle last year, including the the initial embryonic xpcshell test stuff (that admittedly lacked a proper license header). The whole thing was a bit ad-hoc with it being outside the main tree. (Bug references #416119, #416229, #427286, #430922). I'm not trying to be a pain, I just wonder what the policy is for getting your name listed as a contributor, however inconsequential the contribution is. If not doing it myself or adding a license header nullifies this, then that's fine too, I had fun doing it at least.
(In reply to comment #33) > I'm not trying to be a pain, I just wonder what the policy is for getting your > name listed as a contributor, however inconsequential the contribution is. Not a problem! I noticed your name in one license header (see patch), I think - just let me know which ones you'd like your name added to, and I'll do it. (Where I added license headers, I've basically listed mfinkle first and myself second, for lack of better knowledge as to who's done what.)
In my escapades with tryserver, it turns out libffi doesn't build with msvc, and has some trouble on darwin/ppc and linux/arm. (And possibly others, but that covers the ones we really care about, I think.) In the msvc case, both the build system and the asm source fail (the msvc assembler is, of course, incompatible with the gnu one). pyctypes has their own minimal msvc hack of libffi, which they keep alongside it in a libffi_msvc dir. This was hacked up in 2004 and was not (and probably never will be) upstreamed. Since they ship it, it's obviously tested, so I'll import that. We can pull from them if they ever update it, or remove it if libffi adds msvc win32 support. (Apparently, libffi head does have msvc win64 cygwin support.) For darwin/ppc, libffi head has a fix, so I'll pull that in. For linux/arm, I'll disable ctypes for now, because the fix isn't clear and will probably involve a libffi configure fix. For wince/arm, pyctypes has another minimal fork, which we could look at pulling... but for now I'll just disable. I have these build fixes in a patch against attachment 401249 [details] [diff] [review] which I'm testing on tryserver; I'll post on Monday. (That shouldn't hold up review of attachment 401249 [details] [diff] [review], though.)
Created attachment 401949 [details] [diff] [review] add msvc support, v1 This adds msvc support and makes some tweaks for other platforms, per previous comment. (It's diffed against the previous attachment; it's big because it includes the libffi_msvc sources, which you may or may not want to read. See js/ctypes/libffi_msvc/README.mozilla for details on how I pulled and patched them.) Tryserver seems happy with it. It should, in theory, work on win64 as well - but I haven't tested. I think review should be sufficient for this one, but if I'm wrong please feel free to sr. ;)
Four very general points: 1. Who will own this code? It seems like some of it belongs in the SpiderMonkey engine under Brendan's ownership. Other parts don't. Either way it seems like this might as well have a Bugzilla component and an official owner though. 2. I am not the right person to review these files, so I didn't: browser/installer/package-manifest.in configure.in client.py js/ctypes/Makefile.in js/ctypes/tests/Makefile.in toolkit/library/libxul-config.mk toolkit/library/nsStaticXULComponents.cpp toolkit/toolkit-makefiles.sh toolkit/toolkit-tiers.mk ...except for this question: The plan is to check in a copy of libffi, not for every developer to have to run client.py and fetch it out of redhat CVS every time--just like NSPR, right? 3. The use of the word "native" throughout is somewhat unfortunate, since it already has at least two *radically* different meanings in the JS source ("native function", meaning one that's implemented in C/C++, and "native object", meaning one that *doesn't* have custom ops implemented in C/C++). Each meaning of the word is present in many, many identifiers, all of which require extra mental effort to read as a result (JSNativeEnumerator, JSNativeTraceInfo, JSFastNative, js_ArgsPrivateNative, js_NativeGet, map_is_native, js_DefineNativeProperty, js_generic_native_method_dispatcher, etc.) In addition the ECMAScript standard uses the word "native" to mean something yet again completely different. I wouldn't mind seeing "Foreign" or "FFI" here instead, but it's a big change. Your call. 4. The dependency on XPConnect is a real shame. This would perform better if it were coded directly against the JSAPI. We can mop that up later though ...unless XPConnect exposes magical features that applications are going to start depending on (like the ability to call QueryInterface on an nsNativeTypes object). (nervous look) Maybe we can document in big letters that users pretty much shouldn't depend on anything but open(), declare(), and the type constants. :-| In nsNativeMethod.cpp: >+static nsresult >+TypeError(JSContext *cx, const char *message) >+{ >+ JS_ReportError(cx, message); >+ return NS_ERROR_FAILURE; >+} I wrote this, and sure enough there's a bug. It should say: JS_ReportError(cx, "%s", message); But actually the error reporing here just kinda sucks. We should throw a TypeError, and it should contain the value that caused the error. You can do that by creating a jsctypes.msg file that looks like js/src/js.msg, including at least one entry with JSEXN_TYPEERR, like this: MSG_DEF(JSCTYPESMSG_TYPE_ERROR, 1, 2, JSEXN_TYPEERR, "expected {0}, got {1}") And then adding some code like this (cribbed from jspubtd.h and jscntxt.cpp): // header file enum JSCTypesErrNum { #define MSG_DEF(name, number, count, exception, format) \ name = number, #include "jsctypes.msg" #undef MSG_DEF JSCTYPESERR_LIMIT }; const JSErrorFormatString * jsctypes_GetErrorMessage(void *userRef, const char *locale, const uintN errorNumber); // .cpp file JSErrorFormatString js_ErrorFormatString[JSCTYPESERR_LIMIT] = { #define MSG_DEF(name, number, count, exception, format) \ { format, count, exception } , #include "jsctypes.msg" #undef MSG_DEF }; const JSErrorFormatString * jsctypes_GetErrorMessage(void *userRef, const char *locale, const uintN errorNumber) { if (0 < errorNumber && errorNumber < JSCTYPESERR_LIMIT) return &js_ErrorFormatString[errorNumber]; return NULL; } With all that, TypeError could do this (ToSource is stolen from js/src/shell/js.cpp): static const char * ToSource(JSContext *cx, jsval *vp) { JSString *str = JS_ValueToSource(cx, *vp); if (str) { *vp = STRING_TO_JSVAL(str); return JS_GetStringBytes(str); } JS_ClearPendingException(cx); return "<<error converting value to string>>"; } static nsresult TypeError(JSContext *cx, const char *expected, jsval *actual) { const char *src = ToSource(cx, actual); JS_ReportErrorNumber(cx, jsctypes_GetErrorMessage, NULL, JSCTYPESMSG_TYPE_ERROR, expected, src); return NS_ERROR_FAILURE; } And you could call it like this: if (!jsvalToIntStrict(aValue, &aResult.mValue.mUint8) || aResult.mValue.mUint8 > 1) + return TypeError(aContext, "boolean", &aValue); All this code is untested, but I mostly copied it from existing code. With this you could tighten up the tests that call do_check_throws to expect TypeErrors specifically (not just any old Error). >+ case nsINativeTypes::USTRING: >+ if (JSVAL_IS_NULL(aValue)) { >+ // Allow passing a null pointer. >+ aResult.mValue.mPointer = nsnull; >+ } else if (JSVAL_IS_STRING(aValue)) { >+ aResult.mValue.mPointer = JS_GetStringChars(JSVAL_TO_STRING(aValue)); JS_GetStringChars is a funny API. It tries really hard to return a pointer to a null-terminated jschar string. However, if you're out of memory (i.e. malloc fails), instead of returning NULL, which we could at least detect, it returns a pointer into a jschar buffer that *isn't* null-terminated. JS_GetStringBytes, which is used in `case nsINativeTypes::STRING`, handles OOM rather idiosyncratically too. If you care about this, I'll add better-behaved APIs for you to use instead of these two. It's long overdue. Follow-up bug perhaps. In ConvertReturnValue(): >+ case nsINativeTypes::STRING: { >+ if (!aResultValue.mValue.mPointer) { >+ // Allow returning a null pointer. >+ *aValue = JSVAL_VOID; >+ } else { Returning NULL from C++ should produce JSVAL_NULL (that is, the JS value `null` rather than `undefined`). Compare the corresponding case in PrepareValue, where we accept JSVAL_IS_NULL(aValue) but not JSVAL_IS_VOID. Oddly enough, the test for this feature already says do_check_eq(test_ansi_echo(null), null); but apparently do_check_eq is lenient about null vs. undefined! >+ case nsINativeTypes::USTRING: { >+ if (!aResultValue.mValue.mPointer) { >+ // Allow returning a null pointer. >+ *aValue = JSVAL_VOID; Same change here. In nsNativeMethod::Call: >+ JSAutoRequest ar(cx); Is this JSAutoRequest necessary? nsIXPCScriptable::Call()'s caller doesn't promise to provide a request? I think it probably does-- JSNative provides a request. >+static inline nsresult >+jsvalToUint16(JSContext* aContext, jsval aVal, PRUint16& aResult) >+{ >+ if (JSVAL_IS_INT(aVal)) { >+ PRUint32 i = JSVAL_TO_INT(aVal); >+ if (i <= PR_UINT16_MAX) { >+ aResult = i; >+ return NS_OK; >+ } >+ } >+ >+ JS_ReportError(aContext, "Parameter must be an integer"); >+ return NS_ERROR_INVALID_ARG; >+} >+ >+static inline nsresult >+jsvalToCString(JSContext* aContext, jsval aVal, const char*& aResult) >+{ >+ if (JSVAL_IS_STRING(aVal)) { >+ aResult = JS_GetStringBytes(JSVAL_TO_STRING(aVal)); >+ return NS_OK; >+ } >+ >+ JS_ReportError(aContext, "Parameter must be a string"); >+ return NS_ERROR_INVALID_ARG; >+} It's a shame that we have here code that's so similar to what's in nsNativeMethod.cpp. But in both cases the behavior is different for good reasons. (Maybe ctypes's users will run into similar cases: for any given API, js-ctypes won't wrap it *quite* the way they would like. They can easily fix it by wrapping the js-ctypes NativeMethod in another JS function though. It's good.) >+class nsNativeTypes This seems like a weird name. Objects of this class represent libraries we've loaded, right? In nsNativeTypes::Declare: >+ nsresult rv; >+ >+ nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID()); >+ >+ nsAXPCNativeCallContext* ncc; >+ rv = xpc->GetCurrentNativeCallContext(&ncc); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ JSContext *ctx; >+ rv = ncc->GetJSContext(&ctx); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ JSAutoRequest ar(ctx); >+ >+ PRUint32 argc; >+ jsval *argv; >+ ncc->GetArgc(&argc); >+ ncc->GetArgvPtr(&argv); Bluh! You don't have to do any of that if you code to the JSAPI instead of XPIDL/XPConnect. We should get this fixed up in a follow-up bug. In jsctypes-test.cpp: >+int >+test_i_if_floor(int number1, float number2) >+{ >+ return floor(float(number1) + number2); >+} It's kind of weird that gcc doesn't warn about this implicit conversion from float (or maybe double -- the return type of floor, anyway) to int, since if number2 is large it can overflow. I think an explicit cast is appropriate, but whatever you prefer is fine with me. In js/ctypes/tests/unit/test_jsctypes.js.in: >+function POINT(x, y) { >+ this.x = x; this.y = y; >+} >[...] >+ _fields_ : [{"n1" : Types.INT32}, {"n2" : Types.INT16}, {"inner" : INNER}, {"n3" : Types.INT64}, {"n4" : Types.INT32}] >+} I don't think we want structs to work like this. If you agree, please just delete this part of the test. If not, it would still be nice to have all the struct-related stuff together, and block-commented out for now. Likewise in jsctypes-test.{h,cpp}. >+function do_check_throws(f, type, stack) >+{ >+ if (!stack) >+ stack = Components.stack.caller; >+ >+ try { >+ f(); >+ } catch (exc) { >+ if (exc instanceof type) >+ return; >+ do_throw("expected " + type.name + " exception, caught " + exc, stack); >+ } >+ do_throw("expected " + type.name + " exception, none thrown", stack); >+} Ted said he would take a patch adding this to the xpcshell harness. If you're not interested in doing it right now, I can file the follow-up bug. >+ // test the range of unsigned. (we can reuse the signed C function >+ // here, since it's binary-compatible.) >+ var test_us_us = library.declare("test_s_s", Types.DEFAULT, Types.UINT16, Types.UINT16); >+ do_check_eq(test_us_us(0xffff), 0xffff); >+ do_check_throws(function () { test_us_us(0x10000); }, Error); Cool. Please add this test: do_check_throws(function () { test_us_us(-1); }, Error); >+ var test_ui_ui = library.declare("test_i_i", Types.DEFAULT, Types.UINT32, Types.UINT32); >+ do_check_eq(test_ui_ui(0xffffffff), 0xffffffff); And again for test_ui_ui here. >+ var test_f_f = library.declare("test_f_f", Types.DEFAULT, Types.FLOAT, Types.FLOAT); >+ do_check_eq(test_f_f(5), 5); >+ do_check_eq(test_f_f(5.25), 5.25); >[...] >+ do_check_eq(test_f_f(Number(16.5)), 16.5); What is this last test intended to test? `Number(16.5)` is the same value as `16.5`. >+ do_check_eq(test_d_d(Number(16.5)), 16.5); Same here.
Comment on attachment 401249 [details] [diff] [review] js-ctypes, v4 OK, r+=me with fixes. Can't wait to see this land.
dwitte will own this code and I'll continue to provide API srs, I think... dwitte, do you have a CVS account I can use in despot?
I do - should be dwitte@mozilla.com. I'll get to work updating the patch now, and land it first thing tomorrow.
(In reply to comment #34) I think the files in tests/ would be in order, I created those originally and Mark added to them before landing. Looking back at the other patches, I contributed some bug fixes and additions in nsNativeMethod.cpp and nsNativeTypes.cpp as well. Not much but I wouldn't mind being added to those as well (though for sure lowest on the pecking order since not much of the code, if any, is left, heh).
(In reply to comment #37) > 1. Who will own this code? It seems like some of it belongs in the SpiderMonkey > engine under Brendan's ownership. Other parts don't. Either way it seems like > this might as well have a Bugzilla component and an official owner though. Bug filed to get the bugzilla component moved. > ...except for this question: The plan is to check in a copy of libffi, not for > every developer to have to run client.py and fetch it out of redhat CVS every > time--just like NSPR, right? Yup, I'll land libffi along with it. > 3. The use of the word "native" throughout is somewhat unfortunate Agree... I didn't worry too much about it, but we should still change it now. s/nsNativeMethod/mozilla::ctypes::Function/, s/nsNativeTypes/mozilla::ctypes::Library/, and s/nsINativeTypes/nsIForeignLibrary/. > I wrote this, and sure enough there's a bug. It should say: > JS_ReportError(cx, "%s", message); Thanks for the lesson in JS error messages :) I'll make the JS_ReportError fix above, and try to roll/land a followup patch today for the rest. (Want to get this on trunk ASAP in case tinderbox gets indigestion.) > JS_GetStringChars is a funny API. Ugh. A new or fixed API would be good. (Could we not just change it? Would people really be upset if it started returning NULL in OOM conditions?) As it stands, we'd have to get the string length and check for termination, which sucks. (Especially since that memory read might be off the end of the allocated space, unless it intentionally overallocs.) So let's get this in a followup. > Returning NULL from C++ should produce JSVAL_NULL Nice catch, thanks. > I don't think we want structs to work like this. Yeah... removed the code. > Ted said he would take a patch adding this to the xpcshell harness. If you're > not interested in doing it right now, I can file the follow-up bug. A followup would be nice - thanks. >. > What is this last test intended to test? `Number(16.5)` is the same value as > `16.5`. Mistake on my part... removed. I'll land this puppy then post an updated patch. In the meantime, let me know if you'd like any changes based on the above.
(In reply to comment #41) > I think the files in tests/ would be in order Done. > Looking back at the other patches, I contributed some bug fixes and additions > in nsNativeMethod.cpp and nsNativeTypes.cpp as well. Also done. You contributed the struct code, which - while not in yet - is certainly worthy of mention.
Created attachment 402525 [details] [diff] [review] final trunk patch (landed) Landed on mozilla-central. This is the whole thing (minus libffi), including various fixes to make tinderbox happy, and jorendorff's TypeError suggestion. Leaving bug open so we can look at branch landing in a few days.
(In reply to comment #42) > (In reply to comment #37) > > JS_GetStringChars is a funny API. > > Ugh. A new or fixed API would be good. Filed bug 518463. > > Ted said he would take a patch adding this to the xpcshell harness. If you're > > not interested in doing it right now, I can file the follow-up bug. > > A followup would be nice - thanks. Filed bug 518436. > >. That wasn't obviously correct to me at first, but OK. I think you could just: if (JSVAL_IS_NUMBER(v)) { double d = JSVAL_IS_INT(v) ? jsdouble(JSVAL_TO_INT(v)) : *JSVAL_TO_DOUBLE(v); *aResult = IntegerType(d); return jsdouble(*aResult) == d; } ... This would work for all the cases, I think. Not a big deal though. Oh but this reminds me: we need tests for 64-bit integer types too, right?
How does this affect front-end js performance? Does this provide us a route around the zillion xpconnect-traversals we do for any? (eg bug 412531)
Yes, it will provide a fast way around xpconnect, but not for C++ or virtual method calls (yet). We can look at adding these features at some point, but for now, it's C only.
Created attachment 402675 [details] [diff] [review] add int8 and int64 tests Also, improved error reporting from within Function and Library methods - turns out if you JS_ReportError, you don't want to also return NS_ERROR_*, because that makes xpconnect angry. This lets us remove a whole bunch of nsresult error handling, and go with bool instead.
Comment on attachment 402675 [details] [diff] [review] add int8 and int64 tests Great. >+ do_check_eq(test_i64_i64(0x7ffffffffffff000), 0x7ffffffffffff000); ... >+ do_check_eq(test_ui64_ui64(0xfffffffffffff000), 0xfffffffffffff000); On these two lines you can push that to 0x7ffffffffffffc00 and 0xfffffffffffff800 respectively, because in this range, a 64-bit double can hold up to 52 bits after the first (most significant) 1 bit.
Created attachment 402912 [details] patch for 1.9.2 Here's the rolled-up patch for 1.9.2, including libffi. Requesting approval!
Comment on attachment 402912 [details] patch for 1.9.2 a=me pending tryserver happiness
This landed today as: [ <-- merge ] One of the tests included with this patch was particularly finicky on Windows. dwitte pushed two orange fixes: First: Second: ...and then a comment-out-failing-test-pending-investigation changeset: For the record, a Firefox-Unittest box just finished running a build from changeset aae349de03be, which is the child-revision of the second orange fix, and it's still got the test-failure: So, dwitte's final "comment-out-the-failing-test" changeset (cf9dca577e60) was indeed necessary. Presumably once we get a cycle with that included, the Firefox-Unittest boxes will go green again.
Yeah, something's up with the uint64 test on win32: >+ do_check_eq(test_ui64_ui64(0xfffffffffffff000), 0xfffffffffffff000); I'm guessing either jsdouble has different precision on win32 (highly unlikely) or MSVC handles unsigned int64 strangely. I'll debug on tryserver.
Looks like MSVC can't convert a double to an unsigned __int64 correctly, when the double is larger than the maximum signed value of 9223372036854775807. unsigned __int64( 1.644600e+019 ) = 9223372036854775808 (as printed by printf("%I64u", ...))
The following says it all (from): /* The largest double value that can be cast to uint64 in windows is the * signed int64 max, which is 2^63-1. The macro below provides * a workaround for casting large double values to uint64 in windows. */ # define double_to_uint64(d) ( ((d) > 0xffffffffffffffffu) ? \ (unsigned __int64) 0xffffffffffffffffu : \ ((d) < 0) ? (unsigned __int64) 0 : \ ((d) > _I64_MAX) ? \ (unsigned __int64) ((d) - _I64_MAX) - 1 + (unsigned __int64)_I64_MAX + 1: \ (unsigned __int64)(d) ) Go MSVC.
Landed on 1.9.2, including the build fixes in bug 519146 and bug 518741. Filed bug 519235 for the uint64 thing; we can land it separately after I get a patch up later today. Thanks for the help, everyone!
Is it possible to pass by reference into js-ctypes? In other words, if you have a C function: unsigned short foo(char *s, short *l); Where l is used to return a value, is there a way to do that?
Not yet, but I've almost got a patch ready to do that. See bug 513788.
I have initial documentation for js-ctypes here (with the understanding that these will need revision when bug 513788 lands, but I was nearly done with the article when I found out about that, so figured I'd wrap it up).
I'm marking this bug as documentation complete; I know the API is changing, but I'll use that bug to track its doc status from here on.
(In reply to comment #44) > Landed on mozilla-central. Land jsctypes. + Add libffi_msvc fork for msvc x86 builds, and other build fixes for various platforms. (In reply to comment #56) > Landed on 1.9.2 Land jsctypes.