Closed Bug 538324 Opened 10 years ago Closed 10 years ago

Move ctypes into js/src


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






(Reporter: gal, Assigned: dwitte)


(Blocks 2 open bugs)



(10 files, 2 obsolete files)

26.86 KB, patch
Details | Diff | Splinter Review
69.58 KB, patch
: review+
Details | Diff | Splinter Review
64.93 KB, patch
: review+
Details | Diff | Splinter Review
6.33 KB, patch
: review+
Details | Diff | Splinter Review
6.25 KB, patch
: review+
Details | Diff | Splinter Review
5.83 KB, patch
: review+
Details | Diff | Splinter Review
3.11 KB, patch
: review+
Details | Diff | Splinter Review
29.58 KB, patch
: review+
Details | Diff | Splinter Review
13.23 KB, patch
: review+
Details | Diff | Splinter Review
683 bytes, patch
: review+
Details | Diff | Splinter Review
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 ="");

var JS_GetStringBytes = self.declare("JS_GetStringBytes", ffi.default_abi, ffi.type_pointer, ffi.type_pointer);
var cstr ="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);;

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).
Attached patch patch (obsolete) — Splinter Review
Assignee: general → gal
Attached patch patch (obsolete) — Splinter Review
Attachment #420484 - Attachment is obsolete: true
Attachment #420485 - Attachment is obsolete: true
Severity: normal → enhancement
OS: Mac OS X → All
Priority: -- → P5
Hardware: x86 → All
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!)


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

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.
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.
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.
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.
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.
JS_InitCTypesClass, I would assume.
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...
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.

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

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.
Blocks: 552548
Blocks: 552551
Depends on: 552554
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.
Assignee: gal → nobody
Severity: enhancement → normal
Component: JavaScript Engine → js-ctypes
Priority: P5 → P1
QA Contact: general → js-ctypes
Summary: TM: libffi for JavaScript → Move ctypes into js/src
brendan says we should move it into js/src/ctypes.
Yeah, all yours. I am glad there is movement on this. Can we get shell support :)
Depends on: 552525
Depends on: 555644
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.
Assignee: nobody → dwitte
Attachment #436344 - Flags: review?(jim)
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.
Attachment #436346 - Flags: review?(bnewman)
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.
Attachment #436347 - Flags: review?(jorendorff)
Use aforementioned conversion functions.
Attachment #436348 - Flags: review?(bnewman)
Remove the nsILocalFile library.declare() API variant, and thus our dependency on it.
Attachment #436350 - Flags: review?(jorendorff)
Adds a JS_InitCTypesClass() function, and implements it.
Attachment #436351 - Flags: review?(jim)
Removes all unnecessary references to NSPR. This is all trivial.
Attachment #436352 - Flags: review?(bnewman)
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.
Attachment #436353 - Flags: review?(benjamin)
Here you go. :)
Attachment #436354 - Flags: review?(gal)
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...
Attachment #436347 - Flags: review?(jorendorff) → review+
Attachment #436350 - Flags: review?(jorendorff) → review+
These patches pass tryserver.
Attachment #436353 - Flags: review?(benjamin) → review+
Attachment #436354 - Flags: review?(gal) → review+
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:

>+// 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 {
>+  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>
>+AppendString(Vector<T, N, AP> &v, const char (&array)[ArrayLength])

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

See also

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

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

Yep, just call it PrependString.

r=me if you revisit those points.
Attachment #436346 - Flags: review?(bnewman) → review+
Comment on attachment 436348 [details] [diff] [review]
part 4 - use js string conversion functions

>@@ -1693,30 +1679,32 @@ ImplicitConvert(JSContext* cx,
>+        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.
Attachment #436348 - Flags: review?(bnewman) → review+
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 :)
Attachment #436352 - Flags: review?(bnewman) → review+
Attachment #436351 - Flags: review?(jim) → review+
Attachment #436344 - Flags: review?(jim) → review+
Attachment #436344 - Flags: review?(jim)
Attachment #436351 - Flags: review?(jim)
I reviewed the build bits and the add CTypes patch to get us out of a build pickle. jimb should follow up.
Landed on TM. Will follow up if jimb has review comments.
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 556902
Blocks: 556903
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a4
Depends on: 551724
Attachment #436351 - Flags: review?(jimb)
Attachment #436344 - Flags: review?(jimb)
You need to log in before you can comment on or make changes to this bug.