The following testcase asserts on ionmonkey-arm (private branch) revision (run with --ion -n -m --ion-eager): var a = ['a','test string',456,9.34,new String("string object"),[],['h','i','j','k']]; exhaustiveSpliceTestWithArgs("exhaustive splice w/2 optional args 1",a); function mySplice(testArray, splicedArray, first, len, elements) { removedArray.push(testArray[( 346645 ) ]); } function exhaustiveSpliceTestWithArgs(testname, testArray) { for (var first = -(testArray.length+2); first <= 2 + testArray.length; first++) { var expectedSpliced = []; for (var len = 0; len < testArray.length + 2; len++) { expectedRemoved = mySplice(testArray,expectedSpliced,first,len,[-97,new String("test arg"),[],9.8]); } } }

Crash trace: Program received signal SIGSEGV, Segmentation fault. js::ion::MacroAssemblerARM::ma_dtr (this=0x80, ls=2147483647, rt=..., addr=..., mode=20971520, cc=420) at js/src/ion/arm/MacroAssembler-arm.cpp:835 835 Register::FromCode(addr.base()), Imm32(addr.disp()), (gdb) bt #0 js::ion::MacroAssemblerARM::ma_dtr (this=0x80, ls=2147483647, rt=..., addr=..., mode=20971520, cc=420) at js/src/ion/arm/MacroAssembler-arm.cpp:835 #1 0x0000000e in ?? () #2 0x0000000e in ?? () Backtrace stopped: previous frame identical to this frame (corrupt stack?) (gdb) x /i $pc => 0x1f8388 <js::ion::MacroAssemblerARM::ma_dtr(js::ion::LoadStore, js::ion::Register, js::ion::Operand const&, js::ion::Index, js::ion::Assembler::Condition)+4>: ldrb r5, [r4, #0] (gdb) info reg r5 r4 r5 0x94 148 r4 0xba 186 Not sure if this needs to be s-s, but Marty should probably look at it first :)

I was in the middle of doing some work on the load code in the macro assembler, when I came across this bug, so I just rolled the fixes for this bug into the improvements to the macro assembler.

Comment on attachment 629096 [details] [diff] [review] /home/mrosenberg/patches/macroassembler_fixes-r0.patch Review of attachment 629096 [details] [diff] [review]: ----------------------------------------------------------------- r+, though the load-store code could be made somewhat clearer, and given the likelihood of bugs in that kind of code (in my experience) and the complexity involved in calculating the offsets, I think it should be made as clear as possible. (Refer to my comment for one suggestion.) It could wait for a future patch, though. ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +911,5 @@ > } > +/* > +void > +MacroAssemblerARM::ma_offsetLoad(LoadStore ls, int size, boolIsSig) > +*/ Delete it, don't leave it commented out. @@ +922,5 @@ > // we can encode this as a standard ldr... MAKE IT SO > if (size == 32 || (size == 8 && !IsSigned) ) { > if (off < 4096 && off > -4096) { > + // This encodes as a single instruction, passing mode through > + // does what the caller expects.. What does the caller expect? This _is_ the caller, so the comment doesn't help. Assuming I've understood what the caller expects, this should be clearer (to my mind): // This encodes as a single instruction. The 'mode' parameter // has equivalent meaning in as_dtr. Alternatively, just omit the bit about 'mode' entirely. I would have assumed that behaviour without the comment, I think. @@ +934,5 @@ > + // a PreIndex'ed load. PostIndexed loads can be tricky. Normally, doing the load with > + // an index of 0, then doing an add would work, but if the destination is the PC, > + // you don't get to execute the instruction after the branch, which will lead to > + // the base register not being updated correctly. Explicitly handle this case, without > + // doing anything fancy, then handle all of the other cases. It'd be better to list instructions for each variant, since this will be easier to read than text: // mode == Offset // add scratch, base, offset_hi // ldr dest, [scratch, +offset_lo] // // mode == PreIndex // add base, base, offset_hi // ldr dest, [base, +offset_lo]! // // mode == PostIndex, dest == pc // ldr scratch, [base] // add base, base, offset_hi // add base, base, offset_lo // mov dest, scratch // [...] (Note about why a special case is needed for dest == pc.) // // mode == PostIndex, dest != pc // ldr dest, [base], offset_lo // add base, base, offset_hi Splitting the code by variant would also greatly improve the clarity and readability of this code. @@ +935,5 @@ > + // an index of 0, then doing an add would work, but if the destination is the PC, > + // you don't get to execute the instruction after the branch, which will lead to > + // the base register not being updated correctly. Explicitly handle this case, without > + // doing anything fancy, then handle all of the other cases. > + if (rt == pc && mode == PostIndex && ls == IsLoad) { Parentheses would make that clearer. @@ +950,5 @@ > + // For the preindex case, we want to just re-use rn as the base register, so when > + // the base register is updated *before* the load, rn is updated. > + if (mode == PreIndex) > + base = rn; > + JS_ASSERT(mode != PostIndex); Why assert that? Do you only support PostIndex into the PC? (The previous special case handled that.) ::: js/src/ion/arm/MacroAssembler-arm.h @@ +474,5 @@ > } > void push(const Register &reg) { > ma_push(reg); > } > + void pushN(const Register &reg, Imm32 extraSpace) { These (push|pop)N methods aren't used in this patch. Are they required by something else? The naming is also misleading in that pushN sounds like it will push 'reg' N times. Something like "pushWithSpace" would be better. @@ +873,5 @@ > return pushWithPatch(word); > } > > + > + void PushN(const Register &reg, const Imm32 extraSpace) { The 'const Imm32' here is fine, by why is it not repeated in pushN?

landed: http://hg.mozilla.org/projects/ionmonkey/rev/9e1ad66c4e85