Closed Bug 584219 Opened 10 years ago Closed 10 years ago
nanojit failed on Solaris with fatval
After fatval patch landed on mozilla-central, make check failed on Solaris. Compiler is Sun Studio. For debug build, it gets: TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/arguments/args8.js: /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/arguments/args8.js:14: Error: Assertion failed: got "[object Object],[object Object],[object Object],[object Object],[object Object],", expected "[object Arguments],[object Arguments],[object Arguments],[object Arguments],[object Arguments]," TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/arguments/argsx-4.js: /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/arguments/argsx-4.js:23: Error: Assertion failed: got "[object Object] undefined undefined,[object Object] undefined undefined,", expected "[object Arguments] undefined undefined,[object Arguments] undefined undefined," TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/bug561279.js: Assertion failure: obj->getPrivate() == js_FloatingFrameIfGenerator(cx, fp), at ../../../js/src/jstracer.cpp:14039 TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/bug566637.js: /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/bug566637.js:5: Error: Assertion failed: got 3, expected 8 TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/parseIntTests.js: /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/parseIntTests.js:31: Error: Assertion failed: got 0, expected 8 TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/perf-smoketest.js: /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/perf-smoketest.js:15: ReferenceError: PerfMeasurement is not defined TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/strictParseIntOctal.js: /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/strictParseIntOctal.js:3: Error: Assertion failed: got 0, expected 8 TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/testXMLPropertyNames.js: Assertion failure: JSVAL_IS_INT(id) || JSVAL_IS_STRING(id), at ../../../js/src/jsiter.cpp:157 For release build, it has TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/arguments/bug503772.js: TEST-UNEXPECTED-FAIL | trace-test.py | /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/bigLoadStoreDisp.js: /export/home/ginn/work/mozilla-central/js/src/trace-test/tests/basic/bigLoadStoreDisp.js:24: Error: Assertion failed: got 1094, expected 1099 and it takes a long time for trace-test/tests/basic/outerline.js then finally hangs at trace-test/tests/basic/testMethodInitUneval.js if I run the js without nanojit, it passed. On tracemonkey, failures start after fatval patch is landed, however the assertions are different at that time. It is compiler related. Compiling with gcc-4.3 on Solaris passed all the tests except math-trace-tests.js (it is another bug)
Sun Studio doesn't support pack attribute for enum.
Assignee: general → ginn.chen
Status: NEW → ASSIGNED
Attachment #462710 - Flags: review?(lw)
Comment on attachment 462710 [details] [diff] [review] patch part1 Thanks! Not sure how those duplicate #defines got in there.
Attachment #462710 - Flags: review?(lw) → review+
Part 1 pushed: http://hg.mozilla.org/tracemonkey/rev/87ddaf82dbd0
It looks like we have problem is sPayloadOffset is not 0. (on big endian machine) Sometimes we use sPayloadOffset, but sometimes we don't. e.g. we don't use sPayloadOffset for values on stack *(JSObject **)mStack = *p; Thoughts?
OS: Solaris → Android
Removed + sPayloadOffset from "TraceRecorder::importImpl" Added +sPayloadOffset to stobj_get_fslot_ptr Removed + sPayloadOffset in the line of return lir->insLoad(LIR_ldd, vaddr_ins, offset + sPayloadOffset, accSet); Now most of the cases passed except 2 cases: check-math-partial-sums.js jsspi-tests/bug528645 I'm not sure if I'm on the right track.
Ginn, will you be trying to get fatvals running on sparc v9 -m64? I ask because IIUC both fatvals and JaegerMonkey depend on the memory allocator returning addresses which use only the lower 48 bits, as the upper 16 bits of the jsval is now used for tagging. This is "free" on x86_64 due to its addressing scheme, and obviously not a problem on 32-bit platforms. I have done some rudimentary work here, but I haven't yet figured out how to make it work on sparc without writing a new allocator based on mmap scanning usable regions and doling out arenas from there. Do you know any solaris-10 kernel magic? My first thought was to sneak a syscall() to the 32-bit implementation of mmap() -- but that has two drawbacks. One, I couldn't get it to work, two it would leave NetBSD/sparc and Linux/sparc users (all ten of them?) without a solution.
(In reply to comment #9) Yes, it sounds like you are on the right track. I'm sorry all these sPayloadOffset bugs remain -- initially the trace+fatval integration didn't even include the sPayloadOffset; they were only added (incompletely) after the fact.
(In reply to comment #10) This is not a sparc only problem. Solaris x64 also has the same problem. I filed the bug 577056 to address this problem. We have a rough idea to add a tag to identify if the address start with 0 or 0xF.
Fix the use of sPayloadOffset for big endian machine
Attachment #464612 - Flags: review?(lw)
Comment on attachment 464612 [details] [diff] [review] patch part3 Nice, thanks!
Attachment #464612 - Flags: review?(lw) → review+
Part3 pushed to t-m http://hg.mozilla.org/tracemonkey/rev/ba24f1f29feb
Do you still need review for part 2? I don't like that you've changed "int32_t" to "signed int", my understanding is that int32_t is guaranteed to be signed.
Attachment #462712 - Flags: review?(nnethercote) → review-
Nicholas, Yes, int32_t is guaranteed to signed. But I have to use "signed int" instead of "int" or "int32_t" here. This is what the standard said. http://en.wikipedia.org/wiki/C_syntax#Bit_fields "As a special exception to the usual C syntax rules, it is implementation-defined whether a bit field declared as type int, without specifying signed or unsigned, is signed or unsigned. Thus, it is recommended to explicitly specify signed or unsigned on all structure members for portability." The footnote on C99 184.108.40.206p9 says: 104) As specified in 6.7.2 above, if the actual type specifier used is int or a typedef-name defined as int, then it is implementation-defined whether the bit-field is signed or unsigned.
Attachment #462712 - Flags: review- → review?(nnethercote)
Comment on attachment 462712 [details] [diff] [review] patch part2 What a grotty corner of the language! r=me. Can you add a reference to this bug number in the comment? Thanks.
Attachment #462712 - Flags: review?(nnethercote) → review+
Part 2 pushed to nanojit-central http://hg.mozilla.org/projects/nanojit-central/rev/2f6b1e2a50de
Whiteboard: fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey
grotty indeed. TR: http://hg.mozilla.org/tamarin-redux/rev/25e05d728f36
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.