Closed Bug 584219 Opened 10 years ago Closed 10 years ago

nanojit failed on Solaris with fatval

Categories

(Core :: JavaScript Engine, defect)

x86
Android
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(3 files)

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)
Attached patch patch part1Splinter Review
Sun Studio doesn't support pack attribute for enum.
Assignee: general → ginn.chen
Status: NEW → ASSIGNED
Attachment #462710 - Flags: review?(lw)
Attached patch patch part2Splinter Review
Attachment #462712 - Flags: review?(nnethercote)
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
Whiteboard: fixed-in-tracemonkey
Latest nightly cores at startup on OpenSolaris snv_134 SPARC (Mozilla/5.0 (X11; SunOS sun4u; rv:2.0b4pre) Gecko/20100806 Minefield/4.0b4pre).

setting javascript.options.jit.chrome to false prevents coring.
(In reply to comment #5)
> Latest nightly cores at startup on OpenSolaris snv_134 SPARC (Mozilla/5.0 (X11;
> SunOS sun4u; rv:2.0b4pre) Gecko/20100806 Minefield/4.0b4pre).
> 
> setting javascript.options.jit.chrome to false prevents coring.

right, we still have endian issue with fatval + nanojit.
This bug is not fixed on SPARC yet.
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.
I've confirmed the 2 fails have nothing to do with fatval.
Filed Bug 585925 and Bug 585926.
Attached patch patch part3Splinter Review
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 6.7.2.1p9 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
http://hg.mozilla.org/mozilla-central/rev/077c0836d5db
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.