Bug 123421 - ARM/ARMv7: 4th argument register gets clobbered during storePtr call in JIT::updateTopCallFrame
Summary: ARM/ARMv7: 4th argument register gets clobbered during storePtr call in JIT::...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 108645
  Show dependency treegraph
 
Reported: 2013-10-28 14:15 PDT by Mandeep Singh Baines
Modified: 2013-10-29 12:34 PDT (History)
10 users (show)

See Also:


Attachments
Fix 4th argument register trampling for ARM architecture. (7.02 KB, patch)
2013-10-29 06:00 PDT, Julien Brianceau
no flags Details | Formatted Diff | Diff
Fix 4th argument register trampling for ARM architecture (with ChangeLog) (8.41 KB, patch)
2013-10-29 06:04 PDT, Julien Brianceau
jbriance: review-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mandeep Singh Baines 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.
Comment 1 Julien Brianceau 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
Comment 2 Julien Brianceau 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 ?
Comment 3 Julien Brianceau 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
Comment 4 Julien Brianceau 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
Comment 5 Julien Brianceau 2013-10-29 11:31:06 PDT
Created attachment 215403 [details]
Fix 4th argument register trampling for ARM architecture (with RegExpJIT fix)
Comment 6 Michael Saboff 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.
Comment 7 WebKit Commit Bot 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>
Comment 8 WebKit Commit Bot 2013-10-29 12:32:38 PDT
All reviewed patches have been landed.  Closing bug.