Closed
Bug 721280
Opened 12 years ago
Closed 12 years ago
IonMonkey: Don't bailout on all out-of-bounds array accesses
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
References
Details
Attachments
(3 files)
29.08 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
6.88 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
19.47 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
ai-astar's neighbor() function tests out-of-bounds array accesses sort of frequently, using index -1. We currently bailout every time. We should stay in JIT for this case, like JM+TI.
Assignee | ||
Updated•12 years ago
|
Assignee: general → dvander
Status: NEW → ASSIGNED
Comment 1•12 years ago
|
||
FWIW, bug 706328 handles this for the SETELEM case, which I'll rebase today.
Assignee | ||
Comment 2•12 years ago
|
||
I think useRegisterAtStart() should be possible for at least one of the parameters, but doing so for the initLength gives me errors in crypto-sha1. I see stuff like: 277 loadelementhole ([t:277 (=ecx)], [d:278 (=eax)]) (=esi), (=edi), (=edi) <|@ So it seems like two different inputs are getting the same physical register. So for now I just did useRegister.
Attachment #592024 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #592030 -
Flags: review?(sstangl)
Assignee | ||
Comment 5•12 years ago
|
||
(Note, most of these patches are refactoring to use the new ToOutValue.)
Comment 6•12 years ago
|
||
Comment on attachment 592024 [details] [diff] [review] part 1: x86 version Review of attachment 592024 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, the BaseIndex and ToOutValue abstractions are good to avoid platform-specific code. ::: js/src/ion/CodeGenerator.cpp @@ +1202,5 @@ > return callVM(ThrowInfo, lir); > } > > +bool > +CodeGenerator::visitLoadElementHole(LLoadElementHole *lir) Really nice and concise. Would you mind rewriting LoadElementV like this? @@ +1213,5 @@ > + // value. > + Label undefined, done; > + if (lir->index()->isConstant()) { > + masm.branch32(Assembler::BelowOrEqual, initLength, Imm32(ToInt32(lir->index())), &undefined); > + masm.loadValue(Address(elements, ToInt32(lir->index())), out); Shouldn't this be ToInt32(lir->index()) * sizeof(js::Value)? ::: js/src/ion/arm/MacroAssembler-arm.h @@ +536,5 @@ > ma_cmp(ScratchRegister, imm); > ma_b(label, InvertCondition(cond)); > } > void branchPtr(Condition cond, Register lhs, Register rhs, Label *label) { > + return branch32(cond, lhs, rhs, label); Nit: remove the "return"? @@ +543,5 @@ > movePtr(ptr, ScratchRegister); > branchPtr(cond, lhs, ScratchRegister, label); > } > void branchPtr(Condition cond, Register lhs, ImmWord imm, Label *label) { > + return branch32(cond, lhs, Imm32(imm.value), label); Ditto.
Attachment #592024 -
Flags: review?(jdemooij) → review+
Comment 7•12 years ago
|
||
Comment on attachment 592033 [details] [diff] [review] part 3: ARM support Review of attachment 592033 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/arm/CodeGenerator-arm.cpp @@ +880,5 @@ > + Register typeReg = ToRegister(ins->getDef(TYPE_INDEX)); > + Register payloadReg = ToRegister(ins->getDef(PAYLOAD_INDEX)); > + return ValueOperand(typeReg, payloadReg); > +} > + Wow, that cleans things up a lot more than I would have expected. @@ +1110,5 @@ > Register base = ToRegister(load->elements()); > + if (load->index()->isConstant()) > + masm.loadValue(Address(base, ToInt32(load->index()) * sizeof(Value)), out); > + else > + masm.loadValue(base, ToRegister(load->index()), out); Do we want to bundle base and ToRegister(load->index()) into a single BaseIndex? ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +1238,5 @@ > : AboveOrEqual; > ma_cmp(value.typeReg(), ImmTag(JSVAL_TAG_CLEAR)); > return actual; > } > + Initially, I'd left out all of the spaces because all of these functions formed a cohesive unit (at least in my head). Without putting these in a separate class, is there some nice way to visually distinguish this block of functions as a single unit? I suspect a divider-comment is the solution. @@ +1539,5 @@ > { > if (isValueDTRDCandidate(val)) { > Register tmpIdx; > + ma_lsl(Imm32(TimesEight), index, ScratchRegister); > + tmpIdx = ScratchRegister; Since there is only one case here, tmpIdx isn't needed, hard-coding ScratchRegister on the next line should be fine.
Attachment #592033 -
Flags: review?(mrosenberg) → review+
Updated•12 years ago
|
Attachment #592030 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 8•12 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/fd1bdc763619 http://hg.mozilla.org/projects/ionmonkey/rev/f8ea86249dda http://hg.mozilla.org/projects/ionmonkey/rev/0818792fd461
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•