538324 - Move ctypes into js/src

Status: RESOLVED FIXED

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

Description

[Andreas Gal :gal]

The attached patch adds libffi support for SM. Its modeled very closely after jsctypes, borrowing from the code as appropriate. The code sits directly in the JS engine and can be used in the regular shell.

A couple of reasons why I did this. I would like to have libffi/jsctypes support in the shell (not xpcshell, js shell). Also, I think a lot of embeddings might like this, so having this as an engine function might make sense. Last but not least, I think the given machinery is sufficient to implement jsctypes on top of this, completely in JS, with a few native code helpers. Note that the ffi code here doesn't support structs or anything else fancy jsctypes supports. However, all of that should be pretty easy to implement on top of this.

Lets assume we want to print a string with puts. jsctypes offers us machinery to tell the call that we want to take the string apart and hand in its bytes. ffi doesn't really know anything about strings, it just understands pointers. The only thing this code knows about objects and strings is that if we want a pointer and give it an object/string, it will pass the address of it to the call.

How do we convert the string to a c-string? We can simply dlopen the js-engine and use JS_GetStringBytes:

var self = ffi.open("");

var JS_GetStringBytes = self.declare("JS_GetStringBytes", ffi.default_abi, ffi.type_pointer, ffi.type_pointer);
var cstr = JS_GetStringBytes.call("Hello World!");

cstr is a pointer. On 32-bit systems its stored in a jsval as int or double. On 64-bit systems we store it as a special object that can hold 64-bit values, since we don't have the usual PRIVATE_TO_JSVAL alignment guarantees. Int64 is also used for 64-bit integers, obviously. Its a pretty opaque type. I might add toSource(), but thats about it.

Once we have that address, we can hand it to puts:

var puts = self.declare("puts", ffi.default_abi, ffi.type_void, ffi.type_pointer);

puts.call(cstr);

Return values are never parsed back into objects or strings right now. This may or may not be a limitation. I might give the ffi object some additional functions that convert opaque pointers back to JSObject or JSString.

If there are plans to move jsctypes into the JS engine, I think we can bury this quickly. If not, it might make sense to land it. Even then I am not sure jsctypes would really want to use this. Note that once basic ffi functionality is in place, everything else can be done with simple native helpers, i.e. to peek and poke memory (to access structs and such).

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

Attachment: dropped long double, unlikely that we ever need that

Comment 4

[Brendan Eich [:brendan]]

Don't redo stuff already done -- if this is in the tree for jsctypes we should consider moving it.

Now that we have hg can we reorganize js a bit? Instead of everything oddly under js/src, including subdirs, channel the zen of python a bit (flat is better than nested):

js/src (or a new name but we can stick with src for now)
js/xpcom (used to be js/src/xpconnect)
js/ctypes (yay!)

etc.

/be

Comment 5

[Brendan Eich [:brendan]]

js/threads (better than js/src/threads?)

/be

Comment 6

[Andreas Gal :gal]

Yeah, this isn't intended to replace ctypes, its not even intended to land, as
long ctypes moves into regular js shell eventually. I needed this today, I have
it today. Its here in the bug so I don't lose it. I will chat with dwitte and jorendorff to see if there is interest in the code, or what the ctypes timeline is. In the meantime I can use this for my little project.

Comment 7

[Jeff Walden [:Waldo]]

Using JS_GetStringBytes as you suggest seems like a potentially dangerous pattern, given that the "ctypes" function call you'd use it with might GC and, if you're not careful about references, kill the string whose bytes you'd retrieved.  I'd prefer a more integral API than requiring manual dlopening for this sort of use case.

Comment 8

[Andreas Gal :gal]

You have access the the entire JSAPI, including JS_LockGCThing. The machinery isn't any different from a C implementation and its associated problems. C code can trigger a GC just the same. The only difference is that you can do all of it in JS.

Comment 9

[Jason Orendorff [:jorendorff]]

Yeah, fwiw I want to move ctypes into js/src as soon as possible. There are some uses of TArray and so forth but it won't be hard.

Comment 10

[[e-mail]]

Yep, moving ctypes into js/src will be easy. The question is, how do we make it available? Andreas' patch has it exposed as a global object in jsshell only, but if we want to make it available to embeddings, then we might want logic to attach a 'ctypes' object to every global scope. Or this may not be necessary, we can just provide the sources and embeddings can call the ctypes constructor function how they want, like Andreas' patch.

If we attach it ourselves, we'd need a security check (I forget the details but, as I recall, it was a trivial case of asking xpconnect about the scope). Not sure how this would work in an embedding, though. In any case, we'll need something for Firefox.

Comment 11

[Jeff Walden [:Waldo]]

JS_InitCTypesClass, I would assume.

Comment 12

[[e-mail]]

Sounds good. I'll take a stab when I get a chance.

We do use nsILocalFile in one API method. That method can also take a string arg, so I guess we'll have to #ifdef out the nsILocalFile bits. Unless there's a better way...

Comment 13

[[e-mail]]

Another issue is going to be nsIScriptableUnicodeConverter, which we want to use internally in readString() and writeString() methods.

Maybe we can have JS_InitCTypesClass provide a basic implementation, and Firefox can override those methods. We could do this by providing these implementations in js/src/xpconnect. If nsILocalFile & nsIScriptableUnicodeConverter aren't accessible from there, we could create a new interface that lives in js/src/xpconnect (let's call it nsICTypesGoo for now) that wraps this functionality, and whose implementation lives somewhere higher up in the Firefox build tiers.

Comment 14

[Wesley W. Garland]

Andreas:

You're right that embedders would find libffi functionality in the engine useful. I put together a JS FFI module last summer, and I have to say -- I figure I am about 10x more productive in JavaScript+FFI than I am in C alone. It's a great a way to write code, especially when said code is going to be I/O bound anyhow!

Two Quick observations
 - It might be worthwhile adding an option to suspend the current request while the foreign function is running  (I do the opposite, but don't normally call into JSAPI)
 - Rather than JS_GetStringBytes.call() to convert strings, I find doing some type coersion useful - i.e. if I'm expecting a C pointer and receive a JSString, I convert automatically.  (Yes, I can still use the JSString * if I really want to)

If you're curious about the type of code I'm writing in JS with FFI, here is a pretty good example: http://code.google.com/p/gpsee/source/browse/modules/fs-base/

Incidentally, I see you also used a .call property to invoke your functions -- any particular reason for that? I'm considering deprecating it over here; I did it that way originally so that I could construct an object with a private slot to hide my handle, but I figure I could switch to a JSFunction * and use reserved slot without much ado.

Comment 15

[[e-mail]]

Moving this into js-ctypes component, and resetting assignee (since I assume gal isn't working on it; I may be wrong!).

P1, need this for 1.9.3, since it'll be useful and we want to make sure it doesn't present ctypes API snags.

Comment 16

[[e-mail]]

brendan says we should move it into js/src/ctypes.

Comment 17

[Andreas Gal :gal]

Yeah, all yours. I am glad there is movement on this. Can we get shell support :)

Comment 18

[[e-mail]]

Attachment: part 1 - move ctypes into js/src/ctypes

Adds build fu on the js/src side to handle ctypes, and moves the requisite files into js/src/ctypes.

--enable-ctypes must be passed to get it; it's off by default since it requires NSPR, much like JS_THREADSAFE.

Comment 19

[[e-mail]]

Attachment: part 2 - use Vector classes instead of nsTArray/ns*String

This adds some convenience classes to CTypes.h that (mostly) derive from js::Vector, and switches all our nsTArray/ns*String logic over to use them.

I've tried to keep all the semantics the same here, with no functional change. In other words, allocation failures during string operations do not report an error, and are ignored, just like with ns*String. (The string just won't grow, but otherwise nothing bad will happen.)

Most of this is trivial replacement.

Comment 20

[[e-mail]]

Attachment: part 3 - split out js string conversion functions

This splits out jsstr.cpp's inflation/deflation routines, so that there are consistent UTF8 <--> UTF16 functions we can call that don't depend on js_CStringsAreUTF8.

Comment 21

[[e-mail]]

Attachment: part 4 - use js string conversion functions

Use aforementioned conversion functions.

Comment 22

[[e-mail]]

Attachment: part 5 - remove nsILocalFile dependency

Remove the nsILocalFile library.declare() API variant, and thus our dependency on it.

Comment 23

[[e-mail]]

Attachment: part 6 - add ctypes to JSAPI

Adds a JS_InitCTypesClass() function, and implements it.

Comment 24

[[e-mail]]

Attachment: part 7 - remove NSPRisms

Removes all unnecessary references to NSPR. This is all trivial.

Comment 25

[[e-mail]]

Attachment: part 8 - add gecko ctypes module

Adds a ctypes module to toolkit/components/ctypes, which provides ctypes.jsm and the XPCOM goo to hook it up to JS_InitCTypesClass.

The charset conversion hookups will also live here eventually.

Comment 26

[[e-mail]]

Attachment: part 9 - add ctypes to jsshell

Here you go. :)

Comment 27

[[e-mail]]

A note about tests -- I left all the existing tests in their xpcshell form, and moved them to toolkit/components/ctypes. The platform coverage we get on m-c is a superset of TM, and it'll be a bunch of monkeywork to convert the tests into jsshell form. If we get jsreftests running on m-c as part of 'make check', then we could look at converting them...

Comment 28

[[e-mail]]

These patches pass tryserver.

Comment 29

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 436346 [details] [diff] [review]
part 2 - use Vector classes instead of nsTArray/ns*String

>-      result.Insert('*', 0);
>+      InsertString(result, "*");

Maybe call this PrependString?

>-    result.Append(BuildTypeSource(cx, baseType, makeShort));
>-    result.Append(NS_LITERAL_STRING(".array("));
>+    BuildTypeSource(cx, baseType, makeShort, result);\
>+    AppendString(result, ".array(");

Stray '\', looks like.

>+  explicit AutoPtr(T* ptr) : mPtr(ptr) { }
>+  ~AutoPtr() { DeleteTraits::destroy(mPtr); }
>+
>+  T*   operator->()         { return mPtr; }
>+  bool operator!()          { return mPtr == NULL; }
>+  T&   operator[](size_t i) { return *(mPtr + i); }
>+  // Note: we cannot safely provide an 'operator T*()', since this would allow
>+  // the compiler to perform implicit conversion from one AutoPtr to another
>+  // via the constructor AutoPtr(T*).

Oh but you can!  Check out this trick:
http://hg.mozilla.org/mozilla-central/file/89e2b92f0b2a/xpcom/base/nsAutoPtr.h#l74

>+// Container class for Vector, using SystemAllocPolicy. (Unfortunately, one
>+// cannot use 'typedef' to partially specify default template parameters in C++.)
>+template<class T, size_t N = 0>
>+class Array {
>+public:
>+  typedef Vector<T, N, SystemAllocPolicy> Type;
>+};

Since Vector doesn't have any interesting constructors to inherit, you could just let Array<T, N> publicly inherit from Vector<T, N, SystemAllocPolicy>, and then you could just use Array<T> instead of Array<T>::Type.

>+// String and AutoString classes, based on Vector.
>+typedef Vector<jschar,  0, SystemAllocPolicy> String;
>+typedef Vector<jschar, 64, SystemAllocPolicy> AutoString;
>+
>+// Convenience functions to append, insert, and compare Strings.
>+template <class T, size_t N, class AP, size_t ArrayLength>
>+void
>+AppendString(Vector<T, N, AP> &v, const char (&array)[ArrayLength])

Holy static length inference!  (This blew my mind.)

See also
http://stackoverflow.com/questions/2563737/in-the-following-implementation-of-static-strlen-why-are-the-and-parentheses-a

>+template <class T, size_t N, class AP, size_t ArrayLength>
>+void
>+InsertString(Vector<T, N, AP> &v, const char (&array)[ArrayLength])

>+template <size_t N, class AP>
>+void
>+InsertString(Vector<jschar, N, AP> &v, JSString* str)

Yep, just call it PrependString.

r=me if you revisit those points.

Comment 30

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 436348 [details] [diff] [review]
part 4 - use js string conversion functions

>@@ -1693,30 +1679,32 @@ ImplicitConvert(JSContext* cx,
---8<---
>+        size_t nbytes =
>+          js_GetDeflatedUTF8StringLength(cx, sourceChars, sourceLength);
>+        if (nbytes == (size_t) -1)
>+            return false;

I hate that (size_t) -1 == 4294967295 == 2^32-1 isn't a symbolic constant, but that appears to be the convention.  Don't fix it if the cool kids broke it.

You probably do want to de-indent that |return false;| by two spaces, though.

Comment 31

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 436352 [details] [diff] [review]
part 7 - remove NSPRisms

I believe you could have written a one-line sed script to generate this patch :)

Comment 32

[Robert Sayre]

I reviewed the build bits and the add CTypes patch to get us out of a build pickle. jimb should follow up.

Comment 33

[[e-mail]]

Landed on TM. Will follow up if jimb has review comments.