518721 - Implement jsctypes with raw JSAPI

Status: RESOLVED FIXED

(Core :: js-ctypes, defect, P1)

Description

[[e-mail]]

We don't need the xpconnect layer. If we want to keep the Cu.import syntax, we could have an interface "nsIGetCtypes" or somesuch, whose sole purpose is to set up the ctypes object and attach it to the toplevel scope.

The alternative is to do it entirely from C, and attach the ctypes object (with lazy construction) during init of a new system global scope: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/nsXPConnect.cpp#1109. Then you wouldn't even need to import it.

Comment 1

[Benjamin Smedberg]

Unless you're going to fix this before 3.6, let's keep the Cu.import bit. It also gives consumers more control about the names being imported.

Comment 2

[(not currently active) Ted Mielczarek]

Long-term, it would be nice to have a decent way to import JS modules like this, to better support phasing out XPCOM. Might be able to take something from the ServerJS effort.

Comment 3

[[e-mail]]

Attachment: patch v1

Here goes. I hope I've struck the right balance between JS_ASSERT and JS_ReportError; hopefully jorendorff will tell me if I've made incorrect assumptions about what's invariant in JS.

I opted to make all the type constants JSObjects with unique identifiers stuck in their reserved slot; this way, it prevents people from doing silly things like

library.declare("foo", 1, 2, 3); // what types are those?

and instead makes them use our type objects. (The intent being that they could say something like |const mytype = ctypes.types.INT32; print(mytype);| and get something more descriptive than "2" - but maybe I need to implement some toString() magic to make that happen.)

I might add some more unit tests to cover these API minutiae, but this patch is the gist of it.

Also, while this keeps the Cu.import() syntax, it'd be trivial to stuff this on nsXPConnect::InitClassesWithNewWrappedGlobal() as previously mentioned.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 403425 [details] [diff] [review]
patch v1

>+JSObject*
>+Function::Create(JSContext* aContext,
>+                 JSObject* aLibrary,
>+                 PRFuncPtr aFunc,
>+                 jsval aCallType,
>+                 jsval aResultType,
>+                 jsval* aArgTypes,
>+                 uintN aArgLength)
>+{
>+  JSObject* fn = JS_NewObject(aContext, &sFunctionClass, NULL, NULL);
>+  if (!fn)
>+    return NULL;
> 
>+  // root the Library object
>+  if (!JS_SetReservedSlot(aContext, fn, 0, OBJECT_TO_JSVAL(aLibrary)))
>+    return NULL;
>+
>+  // create new Function instance
>+  Function* self = new Function();
>+  if (!self)
>+    return NULL;
>+
>+  // deduce and check the ABI and parameter types
>+  if (!self->Init(aContext, aFunc, aCallType, aResultType, aArgTypes, aArgLength)) {
>+    delete self;
>+    return NULL;
>   }
> 
>+  // attach the Function instance and give ownership to the object
>+  if (!JS_SetPrivate(aContext, fn, self)) {
>+    delete self;
>+    return NULL;
>   }
> 
>+  return fn;
>+}

How about this instead:

{
  // create new Function instance
  Function* self = new Function();
  if (!self)
    return NULL;
 
  // deduce and check the ABI and parameter types
  if (!self->Init(aContext, aFunc, aCallType, aResultType, aArgTypes, aArgLength)) {
    delete self;
    return NULL;
  }

  // create and root the new JS function object
  JSFunction *f = JS_NewFunction(cx, (JSNative) &Call, aArgLength,
                                 JSFUN_FAST_NATIVE, NULL, name);
  if (!f)
    return NULL;
  JSObject *funobj = JS_GetFunctionObject(f);
  JSAutoTempValueRooter funRoot(cx, funobj);

  // stash a pointer to self, which Function::Call will need at call time
  if (!JS_SetReservedSlot(cx, funobj, 0, PRIVATE_TO_JSVAL(self)))
    return NULL;

  // make a strong reference to the library for GC-safety
  if (!JS_SetReservedSlot(cx, funobj, 1, OBJECT_TO_JSVAL(aLibrary)))
    return NULL;

  return funobj;
}

This way you don't have to have your own Function class; ctypes-declared
functions will just be regular JavaScript functions with native code.  Aside
from simplifying your code, this makes ctypes-declared functions more like
native JS functions in various subtle ways. For example, methods like
`fn.apply(thisobj, args)` will be present and work as expected; and `typeof fn`
will return "function".

Also, you can use JSFUN_FAST_NATIVE as the code above does, so JSFunction::Call
can be a JSFastNative.

GetFunction would need to use GetReservedSlot and JSVAL_TO_PRIVATE rather than
JS_GetInstancePrivate.

The drawback is that JavaScript functions don't have finalizers. Since somebody
has to delete the Functions, you can make the Library do it from its finalizer.
The Library needs a pointer to each Function though (linked list, maybe).

If you would rather tackle this change in a separate bug, possibly at a much
later date, that's fine with me.

>+JSBool
>+Function::Call(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+  JS_ASSERT(JSVAL_IS_OBJECT(JS_ARGV_CALLEE(argv)));
>+
>+  JSObject* callee = JSVAL_TO_OBJECT(JS_ARGV_CALLEE(argv));

No need to assert here -- JSVAL_TO_OBJECT asserts exactly that precondition
for you.

>+  JS_ASSERT(JSVAL_IS_OBJECT(slot));
>+
>+  PRLibrary* library = Library::GetLibrary(cx, JSVAL_TO_OBJECT(slot));

Same here.

>+JSBool
>+Library::Open(JSContext* cx, uintN argc, jsval *vp)
>+{
>+  jsval arg = JS_ARGV(cx, vp)[0];
>+  if (argc != 1 || JSVAL_IS_VOID(arg)) {
>+    JS_ReportError(cx, "open requires a single argument");
>+    return JS_FALSE;
>   }

This accesses argv[0] when argc might be 0. Believe it or not I think there's
an off chance this could segfault.

Library::Create() is going to check the type of arg anyway, so you can skip the
JSVAL_IS_VOID test here.

>+JSBool
>+Library::Close(JSContext* cx, uintN argc, jsval* vp)
>+{
>+  if (argc != 0) {
>+    JS_ReportError(cx, "close doesn't take any arguments");
>+    return JS_FALSE;
>+  }
>+
>+  JSObject* obj = JS_THIS_OBJECT(cx, vp);
>+  JS_ASSERT(obj);
>+
>+  // delete the internal Library object
>+  Finalize(cx, obj);
>+  JS_SetPrivate(cx, obj, NULL);
>+  return JS_TRUE;
>+}

You need to explicitly JS_SET_RVAL(cx, vp, JSVAL_VOID), I think. Otherwise your
function returns itself, bizarrely enough.

>+  PRLibrary* library = GetLibrary(cx, obj);
>+  JS_ASSERT(library);

Native functions can easily be applied to objects of the wrong type:
  obj = {};
  obj.close = libc.close;
  obj.close();

Or just:
  libc.close.apply({}, []);

So you can't just assert that GetLibrary will succeed.

Unfortunately the abstraction layer here prevents you from passing
`JS_ARGV(cx, vp)` to JS_GetInstancePrivate, which would have it report the
error for you.

In Module.cpp:
>+static JSClass sCtypesClass = {
>+  "CtypesClass",

The name here should be "CType", I think.

>+#define DEFINE_ABI(name)                                                       \
>+  typeconst = JS_DefineObject(cx, types, #name, &sCtypesClass, NULL,           \
>+                JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);        \
>+  if (!typeconst)                                                              \
>+    return false;                                                              \
>+  if (!JS_SetReservedSlot(cx, typeconst, 0, INT_TO_JSVAL(ABI_##name)))         \
>+    return false;
>+#define DEFINE_TYPE(name)                                                      \
>+  typeconst = JS_DefineObject(cx, types, #name, &sCtypesClass, NULL,           \
>+                JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT);        \
>+  if (!typeconst)                                                              \
>+    return false;                                                              \
>+  if (!JS_SetReservedSlot(cx, typeconst, 0, INT_TO_JSVAL(TYPE_##name)))        \
>+    return false;
>+#include "types.h"
>+#undef DEFINE_ABI
>+#undef DEFINE_TYPE

Here it would be nice to have less actual code in the macro:

  #define DEFINE_ABI(name) \
    if (!defineABI(cx, types, name)) \
        return false;

and the real meat in a separate function.

>+// Each internal ctypes class (representing ABI and type constants) has a
>+// unique ClassType identifier, stored in a reserved slot on the JSObject.
>+enum ClassType {
>+#define DEFINE_ABI(name) ABI_##name,
>+#define DEFINE_TYPE(name) TYPE_##name,
>+#include "types.h"
>+#undef DEFINE_ABI
>+#undef DEFINE_TYPE
>+  INVALID_TYPE
>+};

My two cents is, the name ClassType is confusing and it should be TypeCode or
something.

Having these in separate enums would be nice:

  enum ABI {
  #define DEFINE_ABI(name) ABI_##name,
  #define DEFINE_TYPE(name)
  ...
  };

  enum TypeCode {
  #define DEFINE_ABI(name)
  #define DEFINE_TYPE(name) TYPE_##name,
  ...
  };

r=me with the requested changes and fixes.

Comment 5

[[e-mail]]

Attachment: patch v2

Thanks for the quick review. Nice suggestion re making Function an actual function (and a JSFastNative). I went ahead and did that.

This patch adds in renaming of the types (bug 519474), since we want to do that ASAP:

ctypes.types.DEFAULT -> ctypes.default_abi
ctypes.types.STDCALL -> ctypes.stdcall_abi

ctypes.types.VOID -> ctypes.void_t
ctypes.types.{U}INT{8,16,32,64} -> ctypes.{u}int{8,16,32.64}_t
ctypes.types.BOOL -> ctypes.bool
ctypes.types.{U}STRING -> ctypes.{u}string

For the ABI constants, I'm torn between doing the above, or |ctypes.abi.{default,stdcall}|. Also, bool worries me a bit - people might confuse it with the C++ type, which is quite different to the C type _Bool that we're implementing. We might want to name it _Bool. (pyctypes doesn't appear to have it.)

Could both of you please sign off on these changes?

Comment 6

[[e-mail]]

Attachment: patch v2.1

Ugh, made a mistake in the API docs. Fixed.

Comment 7

[Jason Orendorff [:jorendorff]]

Wow. ISTM if we call something ctypes.bool, no matter which one it is, someone somewhere is going to get crashes and have a tough time figuring out why. Am I wrong?

I guess ultimately we'll want to provide both. In that case, maybe cbool and cppbool.

Comment 8

[Jason Orendorff [:jorendorff]]

Wait a minute -- what's the case where C _Bool is different (in the ABI sense) from C++ bool?

Comment 9

[Benjamin Smedberg]

p1 blocker, this changes the API significantly and we actually do want people using it.

Comment 10

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/a6e59476b690

Will land on branch later today, if there aren't any snags.

I'll take a look at the bool thing and we can get a short, sweet followup if it's needed.

Comment 11

[[e-mail]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6b59b89ebefc

Tryserver's chewing on the bool bits now.

Comment 12

[[e-mail]]

bool results:

* The MSVC C FE doesn't have _Bool or bool. Nothing to see here. The C++ FE has bool, which is always 1 byte on MSVC5+.
* gcc provides _Bool, and also bool via the C99 stdbool.h (which the standard says is a typedef for _Bool). g++ provides both as well. They're 1 byte on all the tryserver plats.

So, it's a pretty safe bet that C++ bool == C bool on everything we care about. We can keep the ctypes.bool name, and probably leave it hardcoded as 1 byte.

Comment 13

[Jacek Caban]

Attachment: Fixed compilation on MinGW.

Comment 14

[Jacek Caban]

On mingw jschar != PRUnichar so we need an explicit cast.

Comment 15

[[e-mail]]

Comment on attachment 405476 [details] [diff] [review]
Fixed compilation on MinGW.

Thanks. We want a trivial build fix like this on 1.9.2 as well.

Could you please run the jsctypes unit tests and let me know if they pass for you? |cd $OBJDIR/js/ctypes/tests; make xpcshell-tests| will do the trick.

Comment 16

[Jacek Caban]

I'm crosscompiling on Linux using mingw, so my test results are a bit tricky as I've modified runxpcshelltests.py to run tests using Wine. They pass for me:

$ make xpcshell-tests
/usr/bin/python -u /home/jacek/mozilla-build/wine-gecko-git/config/pythonpath.py \
          -I/home/jacek/mozilla-build/wine-gecko-git/build \
          /home/jacek/mozilla-build/wine-gecko-git/testing/xpcshell/runxpcshelltests.py \
          --symbols-path=../../../dist/crashreporter-symbols \
          ../../../dist/bin/xpcshell \
          ../../../_tests/xpcshell/jsctypes-test/unit
TEST-PASS | /home/jacek/mozilla-build/mozilla-build/_tests/xpcshell/jsctypes-test/unit/test_jsctypes.js | test passed
INFO | Result summary:
INFO | Passed: 1
INFO | Failed: 0

Comment 17

[[e-mail]]

That's awesome, thanks!

Comment 18

[[e-mail]]

Comment on attachment 405476 [details] [diff] [review]
Fixed compilation on MinGW.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ac3eed6df1fe
http://hg.mozilla.org/mozilla-central/rev/70fd120e7664