561977 - Extend asm_spill (on ARM) to accept arbitrary offsets.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jacob Bramley [:jbramley]]

In bug 556175 Edwin tried to raise NJ_MAX_STACK_ENTRY to be consistent with other architectures, but ARM's asm_spill implementation falls over with more than 1024 entries.

The asm_spill implementation for ARM should be extended to allow arbitrary offsets, and the NJ_MAX_STACK_ENTRY raised up as originally intended.

Comment 1

[Jacob Bramley [:jbramley]]

Attachment: Tests that trigger assertions with a large MAX_STACK_ENTRY on ARM.

I'm not sure if this is the best way to test this functionality, so please suggest alternatives if you know of any! The tests both work by storing, then immediately re-loading a few thousands values (to ensure that they are live and that spills occur), summing them, then returning the result. In the case of spill_fp.in, I also had to add "d2i" at the end as my Core2 gives rounding errors.

Comment 2

[Jacob Bramley [:jbramley]]

Attachment: Extend load/store range where necessary, and lift MAX_STACK_ENTRY.

I added "asm_str" to accept arbitrary offsets in the same way that "asm_ldr_chk" does. I then replaced most uses of "STR" with a call to this function. The impact on within-range stores will be minimal, and stores that might be out-of-range will no longer trigger assertions (or worse, on a release build).

I also had to edit the asm_mmq function as this starting breaking once I'd fixed asm_spill; it uses STR directly, and I left it that way because it should be more efficient in this case.

This patch should also fix bug 490439. (It no longer fails the test, at least.)

----

Vlad, I recall you talking about LDR/STR ranges a few months ago, so you might have some observations to make here.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 442346 [details] [diff] [review]
Tests that trigger assertions with a large MAX_STACK_ENTRY on ARM.

Wow.  I appreciate the effort you've taken, but I also wonder if having no test is better than this test.

Can you do something in JavaScript and put it in the TM trace-tests instead?

Failing that, if you really want to add this test, please add a very descriptive comment at the top :)

Comment 4

[Edwin Smith]

For what it's worth, a js test like this (on TR), does the trick:

print([ ... 1000's of values ])

that's because the actionscript compiler generates array literals as:

push x
push y
push z
push q
... // 1000's of times
newarray

which creates lots of stack pressure.  The situation might not stay like that forever, but its enough to ensure large stacks get tested on ARM on TR.

Comment 5

[Jacob Bramley [:jbramley]]

(In reply to comment #3)
> (From update of attachment 442346 [details] [diff] [review])
> Wow.  I appreciate the effort you've taken, but I also wonder if having no test
> is better than this test.

Actually the effort was minimal; I simply observed which LIR instructions caused spills to occur and wrote a short script to generate the test. I couldn't find a more concise way to do it using JavaScript, though Edwin's suggestion may work.

I'll go with Edwin's suggestion for now; even if it doesn't work in Trace Monkey, it'll be good for Tamarin.

Bug 490439 has some JavaScript tests which _do_ test stack usage, but they're as ugly as that LIR code!

Comment 6

[Rick Reitmaier]

Comment on attachment 442347 [details] [diff] [review]
Extend load/store range where necessary, and lift MAX_STACK_ENTRY.

Code looks good, assume that Jacob will test with Tamarin prior to landing, let me know otherwise.

Small nit: usage of int rather than int32_t for new code.

Comment 7

[Jacob Bramley [:jbramley]]

(In reply to comment #6)
> (From update of attachment 442347 [details] [diff] [review])
> Code looks good, assume that Jacob will test with Tamarin prior to landing, let
> me know otherwise.

Yep, I'll test with Tamarin, though I'm out of the office this week so I'll look at it on Monday.

> Small nit: usage of int rather than int32_t for new code.

Good spot. Thanks!

Comment 8

[Jacob Bramley [:jbramley]]

> Yep, I'll test with Tamarin, though I'm out of the office this week so I'll
> look at it on Monday.

I would, but Tamarin won't build on ARM at the moment. I'll investigate.

Comment 9

[Jacob Bramley [:jbramley]]

http://hg.mozilla.org/projects/nanojit-central/rev/09f682eb3100

Note that the extended-range store is rather slow and won't be merged into STMs, but it is emitted _very_ rarely so it's probably not worth the time to optimize it.

Comment 10

[Edwin Smith]

I'm seeing this failure  in tamarin-redux.  The test in question does a function call with 1000+ arguments, which stresses large stack frames.  

$ avmshell ../test/acceptance/ecma3/Array/e15_4_4_4_1.abc
15.4.4.4-1 Array.prototype.reverse()
Assertion failed: (((offset-adj) & 0xfff) == (offset-adj)) || (((-(offset-adj)) & 0xfff) == (-(offset-adj))) (/home/edwsmith/share/redux/nanojit/NativeARM.cpp:2045)
Trace/breakpoint trap

It happens in the negative-offset, extended-range store case, in asm_str():

        } else {
            int32_t adj = -((-offset) & 0xfff);
            asm_add_imm(IP, rr, adj);
=>          STR(rt, rr, offset-adj);
            asm_sub_imm(rr, rr, adj);
        }

I can't tell by looking whether there is a cut & paste error in this code, and i'm a little bit afraid to tweak it without fully groking the surrounding compensation code.  help?

Setting NJ_MAX_STACK_ENTRY back to 1024 seems to be fine as a workaround, no surprise.

Comment 11

[Rick Reitmaier]

BTW, I believe this issue was introduced with http://hg.mozilla.org/projects/nanojit-central/rev/6f625518afb6, which should have only tightened up the semantics (assuming the change is correct), but apparently uncovered a hidden bug.

Comment 12

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/b44daa2c0503

Comment 13

[Edwin Smith]

I changed NJ_MAX_STACK_ENTRY back to 1024 in TR as a temporary workaround, only good until the next time we merge from NC.  We need to either do the same change in nanojit-central, or fix the bug, asap before the bug spreads too far.

Comment 14

[Jacob Bramley [:jbramley]]

Hmm. The test was failing for me _before_ my commit too so I assumed there was no regression, but on further investigation it looks like it's the Python engine crashing in Lucid. On my old Jaunty board, it shows up as a regression.

I've also identified the bug. Yep, it was my fault and looks like a copy-paste error! I'll get a fix pushed into nanojit-central as soon as I've re-tested it to prevent the bug spreading.

Comment 15

[Jacob Bramley [:jbramley]]

http://hg.mozilla.org/projects/nanojit-central/rev/8dc1d2f7c397

Comment 16

[Edwin Smith]

(In reply to comment #10)

> $ avmshell ../test/acceptance/ecma3/Array/e15_4_4_4_1.abc

Test passes with this fix and NJ_MAX_STACK_ENTRY=4096.  Thanks!

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b44daa2c0503

Comment 18

[Edwin Smith]

TR: http://hg.mozilla.org/tamarin-redux/rev/15927a678dc6

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a6e75d5cf3f0