Closed
Bug 538324
Opened 15 years ago
Closed 15 years ago
Move ctypes into js/src
Categories
(Core :: js-ctypes, defect, P1)
Core
js-ctypes
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a4
People
(Reporter: gal, Assigned: dwitte)
References
(Blocks 2 open bugs)
Details
Attachments
(10 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
sayrer
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sayrer
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gal
:
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 = 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).
Reporter | ||
Comment 1•15 years ago
|
||
Assignee: general → gal
Reporter | ||
Comment 2•15 years ago
|
||
Attachment #420484 -
Attachment is obsolete: true
Reporter | ||
Comment 3•15 years ago
|
||
Reporter | ||
Updated•15 years ago
|
Attachment #420485 -
Attachment is obsolete: true
Reporter | ||
Updated•15 years ago
|
Severity: normal → enhancement
OS: Mac OS X → All
Priority: -- → P5
Hardware: x86 → All
Comment 4•15 years ago
|
||
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•15 years ago
|
||
js/threads (better than js/src/threads?)
/be
Reporter | ||
Comment 6•15 years ago
|
||
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•15 years ago
|
||
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.
Reporter | ||
Comment 8•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
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•15 years ago
|
||
JS_InitCTypesClass, I would assume.
Assignee | ||
Comment 12•15 years ago
|
||
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...
Assignee | ||
Comment 13•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 15•15 years ago
|
||
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
Assignee | ||
Comment 16•15 years ago
|
||
brendan says we should move it into js/src/ctypes.
Reporter | ||
Comment 17•15 years ago
|
||
Yeah, all yours. I am glad there is movement on this. Can we get shell support :)
Assignee | ||
Comment 18•15 years ago
|
||
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 | ||
Comment 19•15 years ago
|
||
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)
Assignee | ||
Comment 20•15 years ago
|
||
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)
Assignee | ||
Comment 21•15 years ago
|
||
Use aforementioned conversion functions.
Attachment #436348 -
Flags: review?(bnewman)
Assignee | ||
Comment 22•15 years ago
|
||
Remove the nsILocalFile library.declare() API variant, and thus our dependency on it.
Attachment #436350 -
Flags: review?(jorendorff)
Assignee | ||
Comment 23•15 years ago
|
||
Adds a JS_InitCTypesClass() function, and implements it.
Attachment #436351 -
Flags: review?(jim)
Assignee | ||
Comment 24•15 years ago
|
||
Removes all unnecessary references to NSPR. This is all trivial.
Attachment #436352 -
Flags: review?(bnewman)
Assignee | ||
Comment 25•15 years ago
|
||
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)
Assignee | ||
Comment 27•15 years ago
|
||
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...
Updated•15 years ago
|
Attachment #436347 -
Flags: review?(jorendorff) → review+
Updated•15 years ago
|
Attachment #436350 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 28•15 years ago
|
||
These patches pass tryserver.
Updated•15 years ago
|
Attachment #436353 -
Flags: review?(benjamin) → review+
Reporter | ||
Updated•15 years ago
|
Attachment #436354 -
Flags: review?(gal) → review+
Comment 29•15 years ago
|
||
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.
Attachment #436346 -
Flags: review?(bnewman) → review+
Comment 30•15 years ago
|
||
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.
Attachment #436348 -
Flags: review?(bnewman) → review+
Comment 31•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #436351 -
Flags: review?(jim) → review+
Updated•15 years ago
|
Attachment #436344 -
Flags: review?(jim) → review+
Updated•15 years ago
|
Attachment #436344 -
Flags: review?(jim)
Updated•15 years ago
|
Attachment #436351 -
Flags: review?(jim)
Comment 32•15 years ago
|
||
I reviewed the build bits and the add CTypes patch to get us out of a build pickle. jimb should follow up.
Assignee | ||
Comment 33•15 years ago
|
||
Landed on TM. Will follow up if jimb has review comments.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a4
Updated•13 years ago
|
Attachment #436351 -
Flags: review?(jimb)
Updated•13 years ago
|
Attachment #436344 -
Flags: review?(jimb)
You need to log in
before you can comment on or make changes to this bug.
Description
•