552533 - Expose libffi thiscall to js-ctypes

Status: RESOLVED FIXED

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

Description

[[e-mail]]

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

[Benjamin Smedberg]

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.

Comment 2

[[e-mail]]

P3, nice to have. Can wait til we get serious about C++ support.

Comment 3

[[e-mail]]

Attachment: 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);

Comment 4

[[e-mail]]

Attachment: fix FFI_LAST_ABI

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

Comment 5

[Benjamin Smedberg]

Yeah, I think that if we want the pretty class syntax we should implement it in JS, not in C++.

Comment 6

[[e-mail]]

Reassigning to nobody. If anyone wants to work on this, feel free!

Comment 7

[Nathan Froyd [:froydnj]]

FWIW, upstream libffi seems to have thiscall support already: https://github.com/atgreen/libffi/blob/master/src/x86/ffitarget.h#L71

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Fixed by updating to libffi 3.1.

Comment 9

[Benjamin Smedberg]

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?

Comment 10

[Makoto Kato [:m_kato]]

Attachment: bug552533

Comment 11

[Makoto Kato [:m_kato]]

Comment on attachment 8600858 [details] [diff] [review]
bug552533

add thiscall abi support

Comment 12

[Jason Orendorff [:jorendorff]]

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.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a4835fe862f

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a4835fe862f

Comment 15

[noitidart]

Docs updated - https://developer.mozilla.org/en-US/docs/Mozilla/js-ctypes/js-ctypes_reference/ABI

Comment 16

[noitidart]

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.