ctypes stdcall tests borked on windows

RESOLVED FIXED

Status

()

Core
js-ctypes
RESOLVED FIXED
8 years ago
7 years ago

People

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

Tracking

unspecified
x86
Windows 7
Points:
---

Firefox Tracking Flags

(blocking2.0 beta4+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
Created attachment 444546 [details] [diff] [review]
fix tests

The #ifdef tests in test_jsctypes.js.in aren't getting tickled properly, which means the stdcall tests aren't even getting run -- fixing that revealed a bunch of other papercuts that needed fixing.

Most notably, one of the stdcall closure tests is crashing -- jumping to a garbage address on return from one of the libffi functions. Probably stack corruption/overpushing/overpopping. I commented it out for now.
Attachment #444546 - Flags: review?(mozilla+ben)
Comment on attachment 444546 [details] [diff] [review]
fix tests

> #if defined(_WIN32) && !defined(_WIN64)
> PRInt32
>-test_closure_cdecl(PRInt8 i, PRInt32 (NS_STDCALL *f)(PRInt8))
>+test_closure_stdcall(PRInt8 i, PRInt32 (NS_STDCALL *f)(PRInt8))

That'll teach you to copy/paste! ;p

>+#ifdef WIN32
>+#ifndef HAVE_64BIT_OS
>+  // Disable this test for now -- it crashes.
>+  //run_single_closure_tests(library, ctypes.stdcall_abi, "stdcall");
> #endif
> #endif

When would WIN32 be defined simultaneously with HAVE_64BIT_OS?  Just curious.
Attachment #444546 - Flags: review?(mozilla+ben) → review+
(Assignee)

Comment 2

8 years ago
(In reply to comment #1)
> When would WIN32 be defined simultaneously with HAVE_64BIT_OS?  Just curious.

For Windows x64. I *think* our configure does it that way. If it doesn't, I'm sure Makoto will pounce on it.
WIN32 and _WIN32 are defined on Windows x86 and x64 for Mozilla code.  About _WIN32, please read http://msdn.microsoft.com/en-us/library/aa384267%28VS.85%29.aspx.  This is by compiler.  WIN32 macro is used some 3rd party libraries such as zlib and Mozilla for windows x86 and x64.
(Assignee)

Comment 4

7 years ago
Comment on attachment 444546 [details] [diff] [review]
fix tests

http://hg.mozilla.org/tracemonkey/rev/1d641507f763
(Assignee)

Updated

7 years ago
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 5

7 years ago
Will file new bug for closure stdcall test fixage. (Requires libffi patches & hence new libffi pull.)
(Assignee)

Comment 6

7 years ago
Backed out due to Win Xd fail. http://hg.mozilla.org/tracemonkey/rev/039bd5d263f9
Whiteboard: fixed-in-tracemonkey

Comment 7

7 years ago
http://hg.mozilla.org/mozilla-central/rev/1d641507f763
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

7 years ago
Reopening since this got backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

7 years ago
This should block, since having stdcall work on Win32 is kinda important.

The patch already here is sufficient, we just need a libffi patch as well. I've pushed it upstream; once it's merged I'll pull it into trunk. Shouldn't take long...
blocking2.0: --- → ?

Updated

7 years ago
blocking2.0: ? → beta4+
(Assignee)

Comment 10

7 years ago
Created attachment 462955 [details] [diff] [review]
add FFI_LAST_ABI

This is the libffi fix required here. I've pushed it upstream, but it hasn't been applied yet. Depending on when that happens, we might need to pull it in locally first.
(Assignee)

Comment 11

7 years ago
Comment on attachment 462955 [details] [diff] [review]
add FFI_LAST_ABI

Looking for rs= here. (Though if it doesn't get upstreamed soon, we'll carry it locally.)
Attachment #462955 - Flags: review?(benjamin)

Updated

7 years ago
Attachment #462955 - Flags: review?(benjamin) → review+
(Assignee)

Comment 12

7 years ago
http://hg.mozilla.org/tracemonkey/rev/9912fe3c7935

And pulled in a new libffi:
http://hg.mozilla.org/tracemonkey/rev/e2265b714a02
Whiteboard: fixed-in-tracemonkey

Comment 13

7 years ago
http://hg.mozilla.org/mozilla-central/rev/9912fe3c7935
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.