Closed Bug 759433 Opened 9 years ago Closed 9 years ago

IonMonkey: (ARM) Assertion failure: TODO: implement bigger offsets :(, at arm/MacroAssembler-arm.cpp:1029 or Opt Crash [@ js::ion::MacroAssemblerARM::ma_dtr]

Categories

(Core :: JavaScript Engine, defect)

Other Branch
ARM
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr10 --- unaffected

People

(Reporter: decoder, Unassigned)

References

Details

(Keywords: assertion, testcase)

Attachments

(1 file)

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 :)
Summary: IonMonkey: Assertion failure: TODO: implement bigger offsets :(, at arm/MacroAssembler-arm.cpp:1029 or Opt Crash [@ js::ion::MacroAssemblerARM::ma_dtr] → IonMonkey: (ARM) Assertion failure: TODO: implement bigger offsets :(, at arm/MacroAssembler-arm.cpp:1029 or Opt Crash [@ js::ion::MacroAssemblerARM::ma_dtr]
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.
Attachment #629096 - Flags: review?(Jacob.Bramley)
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?
Attachment #629096 - Flags: review?(Jacob.Bramley) → review+
landed: http://hg.mozilla.org/projects/ionmonkey/rev/9e1ad66c4e85
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.