Last Comment Bug 513783 - Land js-ctypes on trunk and 1.9.2
: Land js-ctypes on trunk and 1.9.2
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.3a1
Assigned To: dwitte@gmail.com
: js-ctypes
Mentors:
https://wiki.mozilla.org/Jctypes
: 474129 (view as bug list)
Depends on: 516077 533671
Blocks: 513798 518130 519146 535231
  Show dependency treegraph
 
Reported: 2009-08-31 15:33 PDT by dwitte@gmail.com
Modified: 2010-01-26 09:31 PST (History)
26 users (show)
benjamin: blocking1.9.2-
benjamin: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed


Attachments
land libffi 3.0.8 on trunk (1.55 MB, patch)
2009-09-11 22:22 PDT, dwitte@gmail.com
no flags Details | Diff | Splinter Review
land js-ctypes, v1 (66.70 KB, patch)
2009-09-11 22:34 PDT, dwitte@gmail.com
benjamin: review-
Details | Diff | Splinter Review
js-ctypes, v2 (84.09 KB, patch)
2009-09-15 16:44 PDT, dwitte@gmail.com
benjamin: review+
Details | Diff | Splinter Review
js-ctypes, v3 (81.61 KB, patch)
2009-09-16 19:29 PDT, dwitte@gmail.com
dwitte: review+
Details | Diff | Splinter Review
js-ctypes, v4 (72.14 KB, patch)
2009-09-17 12:04 PDT, dwitte@gmail.com
jorendorff: review+
dwitte: superreview+
Details | Diff | Splinter Review
add msvc support, v1 (83.72 KB, patch)
2009-09-21 15:02 PDT, dwitte@gmail.com
benjamin: review+
Details | Diff | Splinter Review
final trunk patch (landed) (144.27 KB, patch)
2009-09-23 20:06 PDT, dwitte@gmail.com
no flags Details | Diff | Splinter Review
add int8 and int64 tests (31.83 KB, patch)
2009-09-24 14:51 PDT, dwitte@gmail.com
jorendorff: review+
Details | Diff | Splinter Review
patch for 1.9.2 (759.32 KB, application/x-gzip)
2009-09-25 13:23 PDT, dwitte@gmail.com
benjamin: approval1.9.2+
Details

Description dwitte@gmail.com 2009-08-31 15:33:13 PDT
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...
Comment 1 Ted Mielczarek [:ted.mielczarek] 2009-08-31 15:52:52 PDT
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.
Comment 2 dwitte@gmail.com 2009-08-31 15:56:23 PDT
Yeah, I meant trunk and 1.9.2. ;)

Changing summary.
Comment 3 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-01 11:33:45 PDT
This is not blocking but is desperately wanted.
Comment 4 dwitte@gmail.com 2009-09-04 13:51:56 PDT
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!
Comment 5 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-04 14:01:20 PDT
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.
Comment 6 dwitte@gmail.com 2009-09-04 14:11:45 PDT
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.
Comment 7 dwitte@gmail.com 2009-09-11 22:22:25 PDT
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.
Comment 8 dwitte@gmail.com 2009-09-11 22:34:00 PDT
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 9 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-14 07:29:38 PDT
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 to.

Also .open should accept an nsIFile in addition to a string path. And in the test script you can/should use build-time #ifdefs for the filename, instead of checking for windows-registry-key or other bits which are incidental. (Or if preprocessing is somehow impractical, use nsIXULRuntime.OS)

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);
Comment 10 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-14 07:35:33 PDT
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.
Comment 11 Brendan Eich [:brendan] 2009-09-14 08:14:02 PDT
js/ctypes, righteous. Will move js/src/xpconnect to js/xpcom so we can study old vs. new in a more parallel structure.

/be
Comment 12 dwitte@gmail.com 2009-09-14 10:10:54 PDT
(In reply to comment #9)
> As for the API, I still want the API to be:

On it!

> Also .open should accept an nsIFile in addition to a string path. And in the
> test script you can/should use build-time #ifdefs for the filename, instead of
> checking for windows-registry-key or other bits which are incidental. (Or if
> preprocessing is somehow impractical, use nsIXULRuntime.OS)

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!
Comment 13 Ted Mielczarek [:ted.mielczarek] 2009-09-14 10:37:36 PDT
(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.
Comment 14 dwitte@gmail.com 2009-09-15 16:44:21 PDT
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!

I have a couple of questions I'd like to bring up. First, ABI support. 99% of the time, people are going to want the system default ABI, and probably don't even know what ABI means. Perhaps we should have declare() just assume that, and provide a separate method like declareWithABI.

Secondly, types. We currently specify by width - int8_t, int16_t, etc. Is this the right approach - should we also provide the traditional char, short, int, long, long long? The lack of binary-compatible int and long in particular could cause pain for people. long double may also be of use (we only provide float and double currently). I also assume _Bool is 8 bits, but I'm not sure if that's generally true. (It wouldn't be hard to #ifdef it to map to the right width.)

Ready for review!
Comment 15 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-15 16:50:52 PDT
> I have a couple of questions I'd like to bring up. First, ABI support. 99% of
> the time, people are going to want the system default ABI, and probably don't
> even know what ABI means. Perhaps we should have declare() just assume that,
> and provide a separate method like declareWithABI.

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.

> Secondly, types. We currently specify by width - int8_t, int16_t, etc. Is this
> the right approach - should we also provide the traditional char, short, int,
> long, long long? The lack of binary-compatible int and long in particular could
> cause pain for people. long double may also be of use (we only provide float
> and double currently). I also assume _Bool is 8 bits, but I'm not sure if
> that's generally true. (It wouldn't be hard to #ifdef it to map to the right
> width.)

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.
Comment 16 dwitte@gmail.com 2009-09-15 17:01:36 PDT
(In reply to comment #15)
> 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.

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.
Comment 17 dwitte@gmail.com 2009-09-15 17:04:27 PDT
(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.
Comment 18 Mark Finkle (:mfinkle) (use needinfo?) 2009-09-15 17:55:39 PDT
(In reply to comment #15)

> 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.

It has support for stdcall. I had defaulted WIN32 to sdtcall and everything else to cdecl
Comment 19 Mark Finkle (:mfinkle) (use needinfo?) 2009-09-15 18:19:45 PDT
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?
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2009-09-15 18:20:19 PDT
"@developer.mozilla.org/jsctypes" and not "@mozilla.org/jsctypes"
Comment 21 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-16 08:01:21 PDT
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 *).

Unfortunately, WSTRING is not a good choice here: Mozilla compiles with 2-byte wchar currently, thought 4-byte is the normal default on many *nix systems. I'd use STRING16 for PRUnichar*, and either implement now or later a realy WSTRING type which uses the system-standard wchar.

>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 <http://www.mozilla.org/>" 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);

Please file a followup: MacOSX has two different types of shared libraries (.dylib and .bundle) which are loaded differently. nsILocalFile.load only loads the .bundle type, which is used by XPCOM components. This means however that jsctypes won't be able to script most system libraries.

>+/* 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.

If extension create worker threads, will ctypes be usable in those (single-threaded, but off the main thread)? Currently it seems that you at least don't have the correct classinfo flags to make that work. That would be really nice, but can be a followup. Are there any shared caches/data structures within libffi itself that might cause synchronization issues?

I'm ok with this with nits fixed: please coordinate with jorendorff to fix the specific issue of jsvalToIntStrict accepting Number objects.
Comment 22 dwitte@gmail.com 2009-09-16 19:27:28 PDT
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.

> Unfortunately, WSTRING is not a good choice here: Mozilla compiles with 2-byte
> wchar currently, thought 4-byte is the normal default on many *nix systems. I'd
> use STRING16 for PRUnichar*, and either implement now or later a realy WSTRING
> type which uses the system-standard wchar.

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.

> Please file a followup: MacOSX has two different types of shared libraries
> (.dylib and .bundle) which are loaded differently. nsILocalFile.load only loads
> the .bundle type, which is used by XPCOM components. This means however that
> jsctypes won't be able to script most system libraries.

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

> If extension create worker threads, will ctypes be usable in those
> (single-threaded, but off the main thread)? Currently it seems that you at
> least don't have the correct classinfo flags to make that work. That would be
> really nice, but can be a followup. Are there any shared caches/data structures
> within libffi itself that might cause synchronization issues?

Not sure. I'll follow up on it. libffi doesn't have any shared stuff AFAICT, apart from statically initialized globals.
Comment 23 dwitte@gmail.com 2009-09-16 19:29:02 PDT
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 24 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-17 05:03:56 PDT
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.
Comment 25 Brendan Eich [:brendan] 2009-09-17 07:25:18 PDT
(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
Comment 26 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-17 09:10:05 PDT
It just seems that the normal JS programming model is not to worry about the exact type of thing you have (a number literal or a number object, same with string literals or string objects) when passing it in to functions which expect a number or a string. That makes this API behave differently from most others.
Comment 27 dwitte@gmail.com 2009-09-17 10:12:16 PDT
(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.)
Comment 28 dwitte@gmail.com 2009-09-17 12:04:39 PDT
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.
Comment 29 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-17 12:06:16 PDT
We should either match python ctypes as closely as possible or use standard _t names: int32_t etc...
Comment 30 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-17 12:11:20 PDT
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.
Comment 31 Brendan Eich [:brendan] 2009-09-17 13:52:52 PDT
(In reply to comment #26)
> It just seems that the normal JS programming model is not to worry about the
> exact type of thing you have (a number literal or a number object, same with
> string literals or string objects) when passing it in to functions which expect
> a number or a string. That makes this API behave differently from most others.

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
Comment 32 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-18 06:58:19 PDT
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.
Comment 33 Fredrik Larsson (:nossralf) 2009-09-18 12:29:59 PDT
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.
Comment 34 dwitte@gmail.com 2009-09-18 13:17:13 PDT
(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.)
Comment 35 dwitte@gmail.com 2009-09-19 14:09:07 PDT
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.)
Comment 36 dwitte@gmail.com 2009-09-21 15:02:33 PDT
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. ;)
Comment 37 Jason Orendorff [:jorendorff] 2009-09-22 16:32:01 PDT
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 38 Jason Orendorff [:jorendorff] 2009-09-22 16:41:00 PDT
Comment on attachment 401249 [details] [diff] [review]
js-ctypes, v4

OK, r+=me with fixes. Can't wait to see this land.
Comment 39 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-22 16:42:53 PDT
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?
Comment 40 dwitte@gmail.com 2009-09-22 16:47:36 PDT
I do - should be dwitte@mozilla.com.

I'll get to work updating the patch now, and land it first thing tomorrow.
Comment 41 Fredrik Larsson (:nossralf) 2009-09-23 10:37:06 PDT
(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).
Comment 42 dwitte@gmail.com 2009-09-23 10:43:28 PDT
(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.

> Cool. Please add this test:
>   do_check_throws(function () { test_us_us(-1); }, Error);

Woah! Nice catch. We failed this test. :/

I changed jsvalToIntStrict() to do

    // Make sure the integer fits in the alotted precision, and has the right sign.
    return jsint(*aResult) == i &&
           (i < 0) == (*aResult < 0);

and likewise for the double conversion.

> 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.
Comment 43 dwitte@gmail.com 2009-09-23 10:46:24 PDT
(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.
Comment 44 dwitte@gmail.com 2009-09-23 20:06:22 PDT
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.
Comment 45 Jason Orendorff [:jorendorff] 2009-09-24 09:02:12 PDT
(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.

> > Cool. Please add this test:
> >   do_check_throws(function () { test_us_us(-1); }, Error);
> 
> Woah! Nice catch. We failed this test. :/
> 
> I changed jsvalToIntStrict() to do
> 
>     // Make sure the integer fits in the alotted precision, and has the right
> sign.
>     return jsint(*aResult) == i &&
>            (i < 0) == (*aResult < 0);
> 
> and likewise for the double conversion.

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?
Comment 46 Dietrich Ayala (:dietrich) 2009-09-24 14:34:46 PDT
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)
Comment 47 dwitte@gmail.com 2009-09-24 14:41:17 PDT
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.
Comment 48 dwitte@gmail.com 2009-09-24 14:51:28 PDT
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 49 Jason Orendorff [:jorendorff] 2009-09-25 06:45:01 PDT
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.
Comment 50 dwitte@gmail.com 2009-09-25 13:23:34 PDT
Created attachment 402912 [details]
patch for 1.9.2

Here's the rolled-up patch for 1.9.2, including libffi. Requesting approval!
Comment 51 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2009-09-25 14:04:13 PDT
Comment on attachment 402912 [details]
patch for 1.9.2

a=me pending tryserver happiness
Comment 52 Daniel Holbert [:dholbert] 2009-09-25 20:39:09 PDT
This landed today as:
 http://hg.mozilla.org/mozilla-central/rev/21c1f047ca90
 http://hg.mozilla.org/mozilla-central/rev/4b153a70bfcf
 http://hg.mozilla.org/mozilla-central/rev/54ec92df6902  [ <-- merge ]

One of the tests included with this patch was particularly finicky on Windows.  dwitte pushed two orange fixes:
  First: http://hg.mozilla.org/mozilla-central/rev/19e35ffbe519
  Second: http://hg.mozilla.org/mozilla-central/rev/159fabf915a4

...and then a comment-out-failing-test-pending-investigation changeset:
  http://hg.mozilla.org/mozilla-central/rev/cf9dca577e60

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:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1253932344.1253934218.31042.gz

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.
Comment 53 dwitte@gmail.com 2009-09-26 00:09:10 PDT
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.
Comment 54 dwitte@gmail.com 2009-09-27 11:58:58 PDT
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", ...))
Comment 55 dwitte@gmail.com 2009-09-27 17:40:32 PDT
The following says it all (from http://icculus.org/projects/rollercoaster/ctl/src/ctl4j/matlab/mini-mcr/extern/include/tmwtypes.h):

/* 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.
Comment 56 dwitte@gmail.com 2009-09-28 10:09:54 PDT
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!
Comment 57 Henrik Skupin (:whimboo) 2009-10-14 15:07:17 PDT
*** Bug 474129 has been marked as a duplicate of this bug. ***
Comment 58 Eric Shepherd [:sheppy] 2009-10-19 14:08:48 PDT
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?
Comment 59 dwitte@gmail.com 2009-10-19 14:14:39 PDT
Not yet, but I've almost got a patch ready to do that. See bug 513788.
Comment 60 Eric Shepherd [:sheppy] 2009-10-19 15:20:05 PDT
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).

https://developer.mozilla.org/en/JavaScript_code_modules/ctypes.jsm
Comment 61 Eric Shepherd [:sheppy] 2009-10-20 09:10:09 PDT
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.
Comment 62 Serge Gautherie (:sgautherie) 2010-01-25 18:39:08 PST
(In reply to comment #44)
> Landed on mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/eb97628a701b
Land jsctypes.
+
http://hg.mozilla.org/mozilla-central/rev/2a88b4c3eb91
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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8dcb04076a94
Land jsctypes.

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