Closed
Bug 552533
Opened 14 years ago
Closed 8 years ago
Expose libffi thiscall to js-ctypes
Categories
(Core :: js-ctypes, defect, P3)
Core
js-ctypes
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: dwitte, Assigned: m_kato)
References
Details
Attachments
(1 file, 2 obsolete files)
9.04 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
On some plats (x86/linux/gcc), thiscall is just cdecl + 'this' as the first parameter. On x86/win32, 'this' is passed in ecx -- a poor man's fastcall. To begin contemplating C++ support in ctypes, even for virtual methods or raw fnptrs, we need thiscall support in libffi. The implementation doesn't have to be exhaustive; it can be for the big three plats, and other plats just throw FFI_BAD_ABI or whatever. (And two of those plats will probably just have FFI_THISCALL === FFI_SYSV or equivalent, which is a one or two liner.) Writing assembly is a ton of fun; I'd say "good first bug", but what I really mean is "good first bug for someone who enjoys assembly". :)
Comment 1•14 years ago
|
||
FWIW, we found that XPCOM and MS-COM interfaces use stdcall declarations, so this is not necessary for those. It's only really necessary for C++ classes which don't explicitly pick a calling convention and therefore get thiscall.
Reporter | ||
Comment 2•14 years ago
|
||
P3, nice to have. Can wait til we get serious about C++ support.
Reporter | ||
Updated•14 years ago
|
Priority: -- → P3
Reporter | ||
Comment 3•14 years ago
|
||
Hacks up FFI_THISCALL on Windows, adds the bits to ctypes, and tests it. This is basically what we want. Could use some tweaking. Ideally we'd have a nice class declaration syntax in JS so you don't need to manually pass 'this', e.g. let class_t = ctypes.ClassType("MyClass"); let method_t = ctypes.MethodType(class_t, "add", ...); let c = new class_t(); // class_t.prototype gets an 'add' function let result = c.add(5); but I think we can still provide the down-and-dirty syntax used here: let add = library.declare("_ZN9TestClass3addEi", ctypes.thiscall_abi, ...); let c = ...; // instantiate a blob of appropriate size add(c.address(), 5);
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•14 years ago
|
||
Required to fix a bug in libffi where the enum values for FFI_LAST_ABI are whacked.
Comment 5•14 years ago
|
||
Yeah, I think that if we want the pretty class syntax we should implement it in JS, not in C++.
Reporter | ||
Comment 6•13 years ago
|
||
Reassigning to nobody. If anyone wants to work on this, feel free!
Assignee: dwitte → nobody
Status: ASSIGNED → NEW
Comment 7•12 years ago
|
||
FWIW, upstream libffi seems to have thiscall support already: https://github.com/atgreen/libffi/blob/master/src/x86/ffitarget.h#L71
Comment 8•10 years ago
|
||
Fixed by updating to libffi 3.1.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 9•10 years ago
|
||
libffi landed but not the patches to actually expose it to ctypes. Reopening to track that. Jorendorff or anyone else interested in mentoring this to done?
Status: RESOLVED → REOPENED
Flags: needinfo?(jorendorff)
Resolution: FIXED → ---
Updated•10 years ago
|
Target Milestone: mozilla32 → ---
Updated•10 years ago
|
Attachment #444549 -
Attachment is obsolete: true
Updated•10 years ago
|
Summary: libffi support for thiscall → Expose libffi thiscall to js-ctypes
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #444547 -
Attachment is obsolete: true
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8600858 [details] [diff] [review] bug552533 add thiscall abi support
Attachment #8600858 -
Flags: review?(jorendorff)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → m_kato
Comment 12•8 years ago
|
||
Comment on attachment 8600858 [details] [diff] [review] bug552533 Review of attachment 8600858 [details] [diff] [review]: ----------------------------------------------------------------- Sorry I never reviewed this. Looks fine, r=me assuming the test passes on all platforms.
Attachment #8600858 -
Flags: review?(jorendorff) → review+
Updated•8 years ago
|
Flags: needinfo?(jorendorff)
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a4835fe862f
Status: REOPENED → RESOLVED
Closed: 10 years ago → 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 15•8 years ago
|
||
Docs updated - https://developer.mozilla.org/en-US/docs/Mozilla/js-ctypes/js-ctypes_reference/ABI
Comment 16•8 years ago
|
||
One example usage is in COM APIs on Windows - we can use ctypes.thiscall_abi instead and avoid having to pass the ptr to self on each call - https://github.com/Noitidart/ostypes/blob/master/ostypes_win.jsm#L686 - *I think* I havent tested it yet.
You need to log in
before you can comment on or make changes to this bug.
Description
•