RESOLVED FIXED 123421
ARM/ARMv7: 4th argument register gets clobbered during storePtr call in JIT::updateTopCallFrame
https://bugs.webkit.org/show_bug.cgi?id=123421
Summary ARM/ARMv7: 4th argument register gets clobbered during storePtr call in JIT::...
Mandeep Singh Baines
Reported 2013-10-28 14:15:11 PDT
R3 is used as the third argument register. R3 is also used as the addressTempRegister. JIT::callOperation() first setups up the arguments and then appends the call: ALWAYS_INLINE MacroAssembler::Call JIT::callOperation(C_JITOperation_E operation) { setupArgumentsExecState(); return appendCallWithExceptionCheck(operation); } appendCall calls updateTopCallFrame: ALWAYS_INLINE MacroAssembler::Call JIT::appendCallWithExceptionCheck(const FunctionPtr& function) { updateTopCallFrame(); MacroAssembler::Call call = appendCall(function); exceptionCheck(); return call; } updateTopCallFrame then does a storePtr which uses addressTempRegister and clobbers R3, corrupting the third argument. ALWAYS_INLINE void JIT::updateTopCallFrame() { ASSERT(static_cast<int>(m_bytecodeOffset) >= 0); #if USE(JSVALUE32_64) Instruction* instruction = m_codeBlock->instructions().begin() + m_bytecodeOffset + 1; uint32_t locationBits = CallFrame::Location::encodeAsBytecodeInstruction(instruction); #else uint32_t locationBits = CallFrame::Location::encodeAsBytecodeOffset(m_bytecodeOffset + 1); #endif store32(TrustedImm32(locationBits), intTagFor(JSStack::ArgumentCount)); storePtr(callFrameRegister, &m_vm->topCallFrame); } There is a comment describing this potential bug (now real) in GPRInfo.h: // FIXME: r3 is currently used be the MacroAssembler as a temporary - it seems // This could threoretically be a problem if this is used in code generation // between the arguments being set up, and the call being made. That said, // any change introducing a problem here is likely to be immediately apparent! static const GPRReg argumentGPR3 = ARMRegisters::r3; // FIXME! One potential fix would be to use something like the claimScratch() technique used in SH4Assembler.h or update topCallFrame before setting up the argument registers.
Attachments
Fix 4th argument register trampling for ARM architecture. (7.02 KB, patch)
2013-10-29 06:00 PDT, Julien Brianceau
no flags
Fix 4th argument register trampling for ARM architecture (with ChangeLog) (8.41 KB, patch)
2013-10-29 06:04 PDT, Julien Brianceau
jbriance: review-
Fix 4th argument register trampling for ARM architecture (with RegExpJIT fix) (10.26 KB, patch)
2013-10-29 11:31 PDT, Julien Brianceau
no flags
Julien Brianceau
Comment 1 2013-10-29 05:41:27 PDT
Hi, I have a fix for this, I'm just waiting that https://bugs.webkit.org/show_bug.cgi?id=123247 lands before
Julien Brianceau
Comment 2 2013-10-29 06:00:39 PDT
Created attachment 215380 [details] Fix 4th argument register trampling for ARM architecture. This patch solves a lot of crashes for ARM_TRADITIONAL. Could you test it for ARMv7 and give me your feedback please ?
Julien Brianceau
Comment 3 2013-10-29 06:04:20 PDT
Created attachment 215381 [details] Fix 4th argument register trampling for ARM architecture (with ChangeLog) Better with the ChangeLog
Julien Brianceau
Comment 4 2013-10-29 10:06:57 PDT
Comment on attachment 215381 [details] Fix 4th argument register trampling for ARM architecture (with ChangeLog) I've seen a regression in the RegExpJIT for ARM, and after reading the code, it must be fixed too. I'll submit a new patch soon
Julien Brianceau
Comment 5 2013-10-29 11:31:06 PDT
Created attachment 215403 [details] Fix 4th argument register trampling for ARM architecture (with RegExpJIT fix)
Michael Saboff
Comment 6 2013-10-29 11:45:57 PDT
Comment on attachment 215403 [details] Fix 4th argument register trampling for ARM architecture (with RegExpJIT fix) r=me. Thanks for cleaning this up.
WebKit Commit Bot
Comment 7 2013-10-29 12:32:36 PDT
Comment on attachment 215403 [details] Fix 4th argument register trampling for ARM architecture (with RegExpJIT fix) Clearing flags on attachment: 215403 Committed r158208: <http://trac.webkit.org/changeset/158208>
WebKit Commit Bot
Comment 8 2013-10-29 12:32:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.