Tracemonkey will crash if the compiler doesn't have FASTCALL

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Ginn Chen, Assigned: Ginn Chen)

Tracking

Trunk
x86
OpenSolaris
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Microsoft or GCC __fastcall convention (aka __msfastcall) passes the first two arguments (evaluated left to right) that fit into ECX and EDX.

If I'm using another compiler which doesn't have FASTCALL, the function generated by Tracemonkey still tries to get value from ECX and EDX.

IMHO, we should either disable JIT or deal with it in nanojit.
There are seriously compilers that don't give you a way to do gcc/msvc fastcall?  I'm pretty surprised!

I would recommend patching nanojit to use another optimized calling convention, then, and submitting it upstream with tests.
(Assignee)

Comment 2

10 years ago
I hacked nanojit. It's not too hard.

Now it works for me on Solaris x86.
The SunSpider result is 1.76x as fast.

I'll submit patches soon.
(Assignee)

Updated

10 years ago
Blocks: 452588
(Assignee)

Comment 3

10 years ago
Created attachment 336341 [details] [diff] [review]
patch v3

I used a GCC style inline asm to simulate a fastcall to u.func.
Also push ECX, EDX when calling back to jsbuiltin functions.

I also tested it on Mac OS X. (modified jstypes.h to turn off fastcall)
trace-test.js passed with sse2 features.
Assignee: general → ginn.chen
Status: NEW → ASSIGNED
Attachment #336341 - Flags: review?(shaver)
(Assignee)

Comment 4

10 years ago
Created attachment 336342 [details] [diff] [review]
patch

Same patch, a few Nit changes.

Sorry for my spam.
BTW: Please ignore "v3". This is the initial version.
Attachment #336341 - Attachment is obsolete: true
Attachment #336342 - Flags: review?(shaver)
Attachment #336341 - Flags: review?(shaver)
Comment on attachment 336342 [details] [diff] [review]
patch

I'm not a good choice for review here; since the majority of the changes appear to be in nanojit, I'd recommend David Anderson to start.  (I'm not generally a good choice for review these days without some previous communication about the patch, due to my workload.)
Attachment #336342 - Flags: review?(shaver) → review?(danderson)
Attachment #336342 - Flags: review?(danderson) → review+
Cool patch, pushed to tracemonkey repo as changeset de2d26b3c902.  I made a few little style changes to keep the nanojit/TraceMonkey separation.

Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
(Assignee)

Comment 7

10 years ago
It's not working any longer.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

10 years ago
Created attachment 347500 [details] [diff] [review]
use nanojit::ABI_CDECL
Attachment #347500 - Flags: review?(danderson)

Updated

10 years ago
Blocks: 449757
Attachment #347500 - Flags: review?(danderson) → review+
(Assignee)

Comment 9

10 years ago
pushed to tracemonkey
http://hg.mozilla.org/tracemonkey/rev/9caa6acaeee0
(Assignee)

Updated

10 years ago
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.