Expose libffi thiscall to js-ctypes

RESOLVED FIXED in Firefox 48

Status

()

Core
js-ctypes
P3
normal
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: dwitte@gmail.com, Assigned: m_kato)

Tracking

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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". :)
(Reporter)

Updated

9 years ago
Blocks: 505907

Comment 1

9 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

8 years ago
P3, nice to have. Can wait til we get serious about C++ support.
(Reporter)

Updated

8 years ago
Priority: -- → P3
(Reporter)

Comment 3

8 years ago
Created attachment 444547 [details] [diff] [review]
add FFI_THISCALL

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

8 years ago
Created attachment 444549 [details] [diff] [review]
fix FFI_LAST_ABI

Required to fix a bug in libffi where the enum values for FFI_LAST_ABI are whacked.

Comment 5

8 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

8 years ago
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
(Assignee)

Updated

5 years ago
Depends on: 810631
Fixed by updating to libffi 3.1.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

Comment 9

4 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 → ---
Target Milestone: mozilla32 → ---
Attachment #444549 - Attachment is obsolete: true
Summary: libffi support for thiscall → Expose libffi thiscall to js-ctypes
(Assignee)

Comment 10

3 years ago
Created attachment 8600858 [details] [diff] [review]
bug552533
Attachment #444547 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
Comment on attachment 8600858 [details] [diff] [review]
bug552533

add thiscall abi support
Attachment #8600858 - Flags: review?(jorendorff)
(Assignee)

Updated

3 years ago
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)

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a4835fe862f
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Comment 16

2 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.