Closed
Bug 507089
Opened 14 years ago
Closed 13 years ago
TM/nanojit: prepare to add get/set methods for CallInfo::_argtypes
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)
Attachments
(3 files, 1 obsolete file)
54.96 KB,
patch
|
edwsmith
:
review+
|
Details | Diff | Splinter Review |
20.04 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
2.79 KB,
patch
|
Details | Diff | Splinter Review |
CallInfo::_argtypes is a bitfield containing 9 x 3-bit fields (it was 9 x 2-bits until recently). There are no get/set methods for this bitfield, so it's manipulated directly with shifting+masking in a number of places. This is very error-prone, especially given that it's not obvious the order of the arguments (right-to-left, left-to-right?) nor where the return value is represented (first or last?) I plan to rectify this.
![]() |
Assignee | |
Comment 1•14 years ago
|
||
The patch to lirasm in bug 519873 introduces some set functions. That's a good start for this bug.
![]() |
Assignee | |
Comment 2•13 years ago
|
||
This patch: - Renames various things to talk about "types" rather than "sizes", because it's a more accurate description. Eg. ArgSize -> ArgType, ARGSIZE_* -> ARGTYPE_*. Similarly, CallInfo::_argtypes as CallInfo::_typesig. - Introduces CallInfo::typeSig[012345678]() and typeSigN() for creating _typesigs, much less error-prone than explicit bit manipulations. In lirasm, these replace the inferior pre-existing ones. - Gets rid of the too-clever-by-half ARGSIZE_MASK_INT. In the few places where it was used, it now just checks the integer possibilities in turn. Got rid of _count_args() accordingly. This means the ArgType enum values can be sequential. - Replaces the (unchecked) table-base opcode selection in LirBufWriter::insCall() with a (checked) switch. - Moved (and renamed) get_sizes() and count_args() from Assembler.cpp into LIR.cpp, with the rest of the CallInfo stuff. - Removed the (AFAICT) unused '...' args from the _nvprof macro because they cause lots of warnings like this from GCC: ../nanojit/CodeAlloc.cpp:466:39: warning: ISO C99 requires rest arguments to be used
Attachment #423455 -
Flags: review?(edwsmith)
![]() |
Assignee | |
Comment 3•13 years ago
|
||
This patch updates TM for the above NJ changes. It also reformats JS_DEFINE_CALLINFO_* to be easier to read.
Attachment #423456 -
Flags: review?(jorendorff)
![]() |
Assignee | |
Comment 4•13 years ago
|
||
This patch updates TR for the above NJ changes. It keeps the SIG0..SIG8 macros, but defines them in terms of typeSig0()..typeSig8().
Attachment #423457 -
Flags: review?(edwsmith)
Updated•13 years ago
|
Attachment #423456 -
Flags: review?(jorendorff) → review+
Updated•13 years ago
|
Attachment #423457 -
Flags: review?(edwsmith) → review+
Updated•13 years ago
|
Attachment #423455 -
Flags: review?(edwsmith) → review+
![]() |
Assignee | |
Comment 5•13 years ago
|
||
http://hg.mozilla.org/projects/nanojit-central/rev/8075a19e1156
Whiteboard: fixed-in-nanojit
![]() |
Assignee | |
Comment 6•13 years ago
|
||
NJ part: http://hg.mozilla.org/tracemonkey/rev/adc7e42ecbe3 TM part: http://hg.mozilla.org/tracemonkey/rev/3c673457c90b
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
![]() |
Assignee | |
Comment 7•13 years ago
|
||
These got backed out due to mysterious windows bustage. http://hg.mozilla.org/projects/nanojit-central/rev/8a1ae643bab0 http://hg.mozilla.org/tracemonkey/rev/636964f611b3
Comment 8•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/adc7e42ecbe3
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
![]() |
Assignee | |
Comment 9•13 years ago
|
||
This one was backed out... Sayre, I hope you merged the backouts as well!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
![]() |
Assignee | |
Comment 10•13 years ago
|
||
The problem that required the backout was mysterious Window bustage that was, as far as I could tell (with Julian's help), an MSVC bug. To keep things moving, I'm going to reland a subset of this bug: - Renames various things to talk about "types" rather than "sizes", because it's a more accurate description. Eg. ArgSize -> ArgType, ARGSIZE_* -> ARGTYPE_*. Similarly, CallInfo::_argtypes as CallInfo::_typesig. - Gets rid of the too-clever-by-half ARGSIZE_MASK_INT. In the few places where it was used, it now just checks the integer possibilities in turn. Got rid of _count_args() accordingly. This means the ArgType enum values can be sequential. - Moved (and renamed) get_sizes() and count_args() from Assembler.cpp into LIR.cpp, with the rest of the CallInfo stuff. It seemed to the the typeSigN functions that caused the MSVC bug, and the other pieces removed ended up landing in other patches. I've filed bug 553962 as a follow-up for the rest of it.
Blocks: 553962
Summary: TM/nanojit: introduce get/set methods for CallInfo::_argtypes → TM/nanojit: prepare to add get/set methods for CallInfo::_argtypes
![]() |
Assignee | |
Comment 11•13 years ago
|
||
http://hg.mozilla.org/projects/nanojit-central/rev/55f02d797675 http://hg.mozilla.org/projects/nanojit-central/rev/2ad8e20152c9 http://hg.mozilla.org/tracemonkey/rev/cc5f301065a7 http://hg.mozilla.org/tracemonkey/rev/d64155596297 http://hg.mozilla.org/tracemonkey/rev/f4e6161afc35 (That includes a Windows bustage fix-up patch. Who knew that "VOID" was #defined on Windows as "void"? Not me.)
![]() |
Assignee | |
Comment 12•13 years ago
|
||
Attachment #423457 -
Attachment is obsolete: true
Comment 13•13 years ago
|
||
TR patches http://hg.mozilla.org/tamarin-redux/rev/c53c3642bbeb http://hg.mozilla.org/tamarin-redux/rev/7970a79e183a
Updated•13 years ago
|
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
![]() |
Assignee | |
Comment 14•13 years ago
|
||
I'm 99.99% certain this has been merged to mozilla-central.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•