Closed
Bug 583532
Opened 14 years ago
Closed 14 years ago
JM: Assert PIC patching correctness.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sstangl, Unassigned)
Details
Attachments
(1 file, 1 obsolete file)
37.97 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
PICs currently patch bytecode by offsetting a saved location in the bytestream by a hardcoded constant determined experimentally. This logic is derived from JSC. However, JaegerMonkey incorporates a register allocator, and the length of opcodes can differ based on the registers involved. So determining validity experimentally for a few cases is insufficient to assert correctness. Additionally, a failure in PIC patching is currently reported as SIGSEGV, which is profoundly unpleasant to work with. I have also seen cases where patching memory with nonsensical offsets does not cause any errors in our trace-test suite at all! So a bad situation all-around. The attached patch asserts the correctness of every constant involved in PIC patching. It also incorporates fixes to the constants -- two constants were swapped in one case, which would not have triggered a SIGSEGV, and which would have been irritating to debug. This patch currently fails 12 trace-tests because it reveals that GETPROP_STUB_SHAPE_JUMP, hardcoded as 12, becomes 13 in certain circumstances. The fix for that is pending and will be attached in this bug. This patch also makes x86_64 PIC porting very easy, because offsets vary wildly depending on which registers are used. So it's nice to have a helpful message rather than signal 11.
Attachment #461841 -
Flags: review?(dvander)
Reporter | ||
Comment 1•14 years ago
|
||
Fixes the tripped asserts caused by the previous patch: in GetPropCompiler::generateStub(), if obj->isDenseArray(), then it is acceptable for the offset to be incorrect, since the IC is immediately disabled. So the easiest fix is to just not assert in that case. I moved the assert to only the !obj->isDenseArray() path, and left a comment in the other.
Attachment #461841 -
Attachment is obsolete: true
Attachment #461867 -
Flags: review?(dvander)
Attachment #461841 -
Flags: review?(dvander)
Comment on attachment 461867 [details] [diff] [review] Previous patch + fix dense array assert >+ void cmpManual(RegisterID left, Imm32 right) >+ { >+ m_assembler.cmpl_ir(right.m_value, left); >+ } >+ >+ Jump branchManual(Condition cond) >+ { >+ return Jump(m_assembler.jCC(x86Condition(cond))); >+ } The macro assembler design doesn't like splitting these things up. For consistency, how about: Jump branch32WithPatch(Condition cond, RegisterID left, Imm32 right, Whatever ...) And fill in an outparam? We should be doing stuff like this anyway IMO, instead of using large bogus values. >+#ifndef DEBUG >+ Jump jmpShapeGuard = masm.branch32(Assembler::NotEqual, shapeReg, >+ Imm32(int32(JSObjectMap::INVALID_SHAPE))); >+#else >+ /* Same as above, but decompose branch32 to assert validity. */ >+ masm.cmpManual(shapeReg, Imm32(int32(JSObjectMap::INVALID_SHAPE))); >+ DBGLABEL(dbgInlineShapeOffset); >+ Jump jmpShapeGuard = masm.branchManual(Assembler::NotEqual); >+ DBGLABEL(dbgInlineShapeJump); >+#endif ^ We should be able to get rid of these #ifdef DEBUG patterns too, which carry the risk of having diverging implementations. Unused labels should be okay, compiler shouldn't warn if we make them ou >+#ifndef DEBUG > masm.storeValue(Valueify(vr.u.v), slot); >+#else >+ /* Do the same as above, but get labels after type and data store. */ >+ dbgInlineStoreType = masm.storeValueDbgTypeLabel(Valueify(vr.u.v), slot); >+ DBGLABEL_ASSIGN(dbgInlineStoreData); >+#endif Different pattern but prefer removing DEBUG split here too. >+#ifdef DEBUG >+ /* >+ * Same as storeValue, but emits a Label after the type store. >+ * Storing the data label is assumed to be the last operation. >+ * Used for offset verification. >+ */ >+ JSC::MacroAssembler::Label >+ storeValueDbgTypeLabel(const Value &v, Address address) { Just replace storeValue(), okay to return label always r=me w/ those fixed. Nice work, awesome that it caught a potential bug.
Attachment #461867 -
Flags: review?(dvander) → review+
Reporter | ||
Comment 3•14 years ago
|
||
http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/2ee92d697741
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 4•14 years ago
|
||
Hotness. /be
You need to log in
before you can comment on or make changes to this bug.
Description
•