Closed Bug 550271 Opened 14 years ago Closed 14 years ago

Inline assembler code assumes 32bit compilation on sparc

Categories

(Core :: JavaScript Engine, defect)

Sun
NetBSD
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: martin, Assigned: martin)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Attached patch Proposed fix (obsolete) — 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).
Attachment #430393 - Flags: review?
Attachment #430393 - Flags: review? → review?(bzbarsky)
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)
Adding sparc-y people...
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.
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.
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.
Attachment #430393 - Attachment is obsolete: true
Attachment #431959 - Flags: review?(jorendorff)
Attachment #430393 - Flags: review?(jorendorff)
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+
...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
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.