Closed Bug 561977 Opened 11 years ago Closed 11 years ago

Extend asm_spill (on ARM) to accept arbitrary offsets.

Categories

(Core :: JavaScript Engine, defect, P1)

ARM
All
defect

Tracking

()

RESOLVED FIXED
flash10.2

People

(Reporter: jbramley, Assigned: jbramley)

References

Details

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

Attachments

(2 files)

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.
Summary: Extend asm_spill to accept arbitrary offsets. → Extend asm_spill (on ARM) to accept arbitrary offsets.
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.
Attachment #442346 - Flags: review?(nnethercote)
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.
Attachment #442347 - Flags: review?(edwsmith)
Attachment #442347 - Flags: feedback?(vladimir)
Blocks: 490439
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 :)
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.
(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!
Attachment #442347 - Flags: review?(edwsmith) → review?(rreitmai)
Attachment #442347 - Flags: review?(rreitmai) → review+
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.
(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!
> 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.
Depends on: 565026
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.
Whiteboard: fixed-in-nanojit
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.
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.
http://hg.mozilla.org/tracemonkey/rev/b44daa2c0503
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
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.
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → flash10.2
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.
(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!
Blocks: 536609
http://hg.mozilla.org/mozilla-central/rev/b44daa2c0503
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
TR: http://hg.mozilla.org/tamarin-redux/rev/15927a678dc6
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Attachment #442347 - Flags: feedback?(vladimir)
Duplicate of this bug: 490439
You need to log in before you can comment on or make changes to this bug.