Closed
Bug 759433
Opened 13 years ago
Closed 12 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)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox-esr10 | --- | unaffected |
People
(Reporter: decoder, Unassigned)
References
Details
(Keywords: assertion, testcase)
Attachments
(1 file)
9.12 KB,
patch
|
jbramley
:
review+
|
Details | Diff | Splinter Review |
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]);
}
}
}
Reporter | ||
Comment 1•13 years ago
|
||
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 :)
Reporter | ||
Updated•13 years ago
|
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]
Comment 2•13 years ago
|
||
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 3•13 years ago
|
||
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 ®) {
> ma_push(reg);
> }
> + void pushN(const Register ®, 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 ®, const Imm32 extraSpace) {
The 'const Imm32' here is fine, by why is it not repeated in pushN?
Attachment #629096 -
Flags: review?(Jacob.Bramley) → review+
Comment 4•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox-esr10:
--- → unaffected
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•