Closed Bug 552533 Opened 10 years ago Closed 4 years ago

Expose libffi thiscall to js-ctypes

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: dwitte, Assigned: m_kato)

References

Details

Attachments

(1 file, 2 obsolete files)

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". :)
Blocks: 505907
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.
P3, nice to have. Can wait til we get serious about C++ support.
Priority: -- → P3
Attached patch add FFI_THISCALL (obsolete) — Splinter Review
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
Attached patch fix FFI_LAST_ABI (obsolete) — Splinter Review
Required to fix a bug in libffi where the enum values for FFI_LAST_ABI are whacked.
Yeah, I think that if we want the pretty class syntax we should implement it in JS, not in C++.
Reassigning to nobody. If anyone wants to work on this, feel free!
Assignee: dwitte → nobody
Status: ASSIGNED → NEW
FWIW, upstream libffi seems to have thiscall support already: https://github.com/atgreen/libffi/blob/master/src/x86/ffitarget.h#L71
Depends on: 810631
Fixed by updating to libffi 3.1.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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 → ---
Target Milestone: mozilla32 → ---
Attachment #444549 - Attachment is obsolete: true
Summary: libffi support for thiscall → Expose libffi thiscall to js-ctypes
Attached patch bug552533Splinter Review
Attachment #444547 - Attachment is obsolete: true
Comment on attachment 8600858 [details] [diff] [review]
bug552533

add thiscall abi support
Attachment #8600858 - Flags: review?(jorendorff)
Assignee: nobody → m_kato
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+
Flags: needinfo?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/3a4835fe862f
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.