Closed
Bug 564966
Opened 15 years ago
Closed 14 years ago
ctypes stdcall tests borked on windows
Categories
(Core :: js-ctypes, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta4+ |
People
(Reporter: dwitte, Assigned: dwitte)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(2 files)
(deleted),
patch
|
mozilla+ben
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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 1•14 years ago
|
||
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•14 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.
Comment 3•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 444546 [details] [diff] [review]
fix tests
http://hg.mozilla.org/tracemonkey/rev/1d641507f763
Assignee | ||
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 5•14 years ago
|
||
Will file new bug for closure stdcall test fixage. (Requires libffi patches & hence new libffi pull.)
Assignee | ||
Comment 6•14 years ago
|
||
Backed out due to Win Xd fail. http://hg.mozilla.org/tracemonkey/rev/039bd5d263f9
Whiteboard: fixed-in-tracemonkey
Comment 7•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 8•14 years ago
|
||
Reopening since this got backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•14 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•14 years ago
|
blocking2.0: ? → beta4+
Assignee | ||
Comment 10•14 years ago
|
||
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•14 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•14 years ago
|
Attachment #462955 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 12•14 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•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•