Bug 82013 – CALLFRAME_OFFSET and EXCEPTION_OFFSET are same in ctiTrampoline on ARM Thumb2

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[SangGyu Lee]

CALLFRAME_OFFSET and EXCEPTION_OFFSET have same value in ctiTrampoline on ARM Thumb2 like followings:

In JITStubs.cpp

#elif (COMPILER(GCC) || COMPILER(RVCT)) && CPU(ARM_THUMB2)

#define THUNK_RETURN_ADDRESS_OFFSET      0x38
#define PRESERVED_RETURN_ADDRESS_OFFSET  0x3C
...
#define REGISTER_FILE_OFFSET             0x60
#define CALLFRAME_OFFSET                 0x64
#define EXCEPTION_OFFSET                 0x64
#define ENABLE_PROFILER_REFERENCE_OFFSET 0x68

I wonder it is intentional or not.

I think they should have different offset and back up register value in separate stack location.
( like on MIPS or other platforms )

#define CALLFRAME_OFFSET                 0x64
#define EXCEPTION_OFFSET                 0x68
#define ENABLE_PROFILER_REFERENCE_OFFSET 0x6C

Currently, since CALLFRAME and EXCEPTION have same offset, in ARM_THUMB2 ctiTrampoline code,

    "str r2, [sp, #" STRINGIZE_VALUE_OF(CALLFRAME_OFFSET) "]" "\n"

seems to be dead code.

The value in stack location is replaced by r3, with the immediately following statement 

    "str r3, [sp, #" STRINGIZE_VALUE_OF(EXCEPTION_OFFSET) "]" "\n"

[Gavin Barraclough]

Uggh, so, the ctiTrampoline is now no longer passed an exception out pointer.  Rather than re-jig the arguments in all asm (which we really should do at some point), we left an unused argument in for now.  So the 'exception' argument no longer really exists, and is completely unused - other that the fact some entry/exit thunks still preserve it around JIT code.  This code should really be deleted (along with all reference to EXCEPTION_OFFSET).

The unused exception value is trampling over the call frame, why does this not crash?

Well, when we wrote the THUMB2 entry thunk we dumbly spilled all arguments to the stack, since that's where they were on x86 - and the CTI callbacks access these values.  But since the call frame may move (it is the JS stack pointer, and updated by JIT code), we cannot rely on the value upon entry to to JIT code, and instead must save a fresh copy each time a call out to C code is made.  As such, I think the store of the call frame is also probably completely redundant, and should likely also be removed.  I'd need to look closer to determine if this is true.

So, my guess is, no actually bug here, just some stupid, wasteful, and potentially confusing & bug prone code. :-/

[Gavin Barraclough]

Created attachment 162923 [details]
Fix

[Gavin Barraclough]

Fixed in r127944

[Geoffrey Garen]

Comment on attachment 162923 [details]
Fix

r=me