Closed
Bug 550271
Opened 14 years ago
Closed 14 years ago
Inline assembler code assumes 32bit compilation on sparc
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: martin, Assigned: martin)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
710 bytes,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
The inline assembler implementing NativeCompareAndSwap for sparc machines erroneously assumes 32bit compilation. In 64bit compilations the upper 32 bit are never changed, breaking (for example) string atomization (due to the flag geting lost).
Assignee | ||
Updated•14 years ago
|
Attachment #430393 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #430393 -
Flags: review? → review?(bzbarsky)
Comment 1•14 years ago
|
||
Comment on attachment 430393 [details] [diff] [review] Proposed fix Jason, are you the right person to review it? Or is Wes or someone? Pass this on as needed? It looks like this code has been around "forever" (at least since hg import)...
Attachment #430393 -
Flags: review?(bzbarsky) → review?(jorendorff)
Comment 2•14 years ago
|
||
Adding sparc-y people...
Comment 3•14 years ago
|
||
Best guy for review is probably Ginn Chen Oddly, ISTR seeing a patch very much like this relatively recently. Is there a way to search bugzilla for attachments using the casx instruction? IIRC that bug author also complained about the use of membar or stbar because there are cooler instructions for that now.
Comment 4•14 years ago
|
||
Found it: Bug 521302 Not sure which bug should be duped, solutions are different. Author of 521302 claims his is superior; I have no actual experience that could be of use in evaluating that claim. At issue is the membar vs. stbar instruction; Sun's website says The SPARC Version 8 (SPARC v8) stbar instruction is a subset of membar, equivalent to a membar with an ordering relationship of #StoreStore. There is no longer a need to support v8, only v8+ and v9 arches. (v8+ is 32-bit words but v9 instructions). Both authors use casx instead of cas instruction for 64-bit compare-and-swap. This probably didn't come out of the wood work until relatively recently, as this time last year, Mozilla was shipping a jslock.cpp that used PRLock() (pthread mutexes) instead of CAS on sparc -- even though the code was there. Feature detection was broken IIRC, and had been since sometime shortly after jsapi 1.7.0.
Assignee | ||
Comment 5•14 years ago
|
||
I'm fine with using Andreas' patch instead (I like the reduced locality of the ifdef), though I think the ifdef triggering on #if JS_BITS_PER_WORD == 32 is cleaner - who knows, we could end up building a sparc64 version using (due to configure bugs?) only 32 bit words for JS. However, this is obviously close to irrelevant. For the membar vs. stbar: yes, stbar is deprecated, but it still works in all environments, so this is only a cosmetical change. Could be done while touching this code, or in a separate commit.
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #430393 -
Attachment is obsolete: true
Attachment #431959 -
Flags: review?(jorendorff)
Attachment #430393 -
Flags: review?(jorendorff)
Comment 8•14 years ago
|
||
Comment on attachment 431959 [details] [diff] [review] Merged patch, very similar to the one in Bug 521302 Great. Do you have the necessary privileges to commit this to hg.mozilla.org/tracemonkey, or shall I?
Attachment #431959 -
Flags: review?(jorendorff) → review+
Comment 9•14 years ago
|
||
...I'm going to take that as a no. Apologies for the delay: http://hg.mozilla.org/tracemonkey/rev/263c63005ce8
Assignee: general → martin
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla1.9.3a5
Comment 10•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/263c63005ce8
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•