With profiling enabled, Gmail fails to load with a JS exception.
This was working in r37698, but starts crashing in r37764. By r38654, it is failing with an exception.
This happens even with the bytecode interpreter.
Oliver and I found the problem while debugging a visual difference in one of his crazy JS raytracers with profiling enabled: http://nerget.com/rayjs/rayjs.html The culprit is this code in BytecodeGenerator::emitCall(): if (m_shouldEmitProfileHooks) { // If codegen decided to recycle func as this call's destination register, // we need to undo that optimization here so that func will still be around // for the sake of op_profile_did_call. if (dst == func) { RefPtr<RegisterID> protect = thisRegister; RefPtr<RegisterID> movedThisRegister = emitMove(newTemporary(), thisRegister); RefPtr<RegisterID> movedFunc = emitMove(thisRegister, func); thisRegister = movedThisRegister.release().releaseRef(); func = movedFunc.release().releaseRef(); } } I am not quite sure why this goes wrong yet, but changing all of the callers of emitCall so that dst is always distinct from func fixes both bugs. It seems to me that the only sane way to fix this is to ensure that dsti is distinct from func when profiling is enabled. One could also make the Node::emitCode() member function emit the profiler hook.
The problematic code was introduced in r38349: http://trac.webkit.org/changeset/38349 Geoff, do you have a suggestion about how to fix this?
Created attachment 26264 [details] Patch in progress Here is a fix. The problem is that the original patch was 'leaking' temporary registers, breaking things that rely on the fact that they can get sequential ranges of temporaries. Here is a simple example: function g(a, b) { return [a.toString(), b]; } In addition to this fix, I will add assertions that assert the sequentiality of sequential ranges of temporaries, and make tests for all node types that call emitCall() or emitConstruct().
Created attachment 26265 [details] Patch in progress Here is a patch with more assertions. Tests are coming.
<rdar://problem/6468077>
Created attachment 26273 [details] Proposed patch
Comment on attachment 26273 [details] Proposed patch r=me
Landed in r39488.
I am getting run time crash with RVCT compiler v3.1 but if the below 2 functions are compiled with RVCT compiler v2.2 it runs fine. Functions: ---------- JSC::BytecodeGenerator::emitCall JSC::BytecodeGenerator::emitConstruct It looks me the run time stack gets corrupted. With both the RVCT compiler v2.2 and v3.1 I am getting ASSERTS => ASSERT(argv[argv.size() - 1]->index() == argv[argv.size() - 2]->index() + 1); in aforesaid functions. Please let me know the followings 1. "op_construct/op_call requires the arguments to be a sequential range of registers" Is it specific to particular compilers or platform dependent(like gcc compiler or window platform)?. On window-x86 ia not getting this assert. 2. Is there any corner case or some particular scenario(s) where the requirements stated above may fails? Please note that the profiling is not enabled, the command line used for compilation is armcpp --cpp --thumb --bss_threshold=0 --debug -c --cpu=Cortex-A8 --fpu=VFPv3 --fpmode=fast -O1 BytecodeGenerator.cpp Thanks in advance. Regards, Shafi
(In reply to comment #11) > I am getting run time crash with RVCT compiler v3.1 but if the below 2 > functions are compiled with RVCT compiler v2.2 it runs fine. what does this have to do with this bug?
(In reply to comment #12) > (In reply to comment #11) > > I am getting run time crash with RVCT compiler v3.1 but if the below 2 > > functions are compiled with RVCT compiler v2.2 it runs fine. > what does this have to do with this bug? Hi Oliver, Thanks! for your quick response. I am just curious to know why I am getting this assert ASSERT(argv[argv.size() - 1]->index() == argv[argv.size() - 2]->index() + 1); Regards, Shafi
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > I am getting run time crash with RVCT compiler v3.1 but if the below 2 > > > functions are compiled with RVCT compiler v2.2 it runs fine. > > what does this have to do with this bug? > > Hi Oliver, > Thanks! for your quick response. > > I am just curious to know why I am getting this assert > ASSERT(argv[argv.size() - 1]->index() == argv[argv.size() - 2]->index() + > 1); > > Regards, > Shafi This bug is about loading gmail under the profiler -- what does a RVCT failure have to do with it?
> This bug is about loading gmail under the profiler -- what does a RVCT failure > have to do with it? http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/WebKit/JavaScriptCore/ChangeLog?view=markup&pathrev=8041 2008-12-28 Cameron Zwarich <cwzwarich@uwaterloo.ca> Reviewed by Oliver Hunt. Bug 22840: REGRESSION (r38349): Gmail doesn't load with profiling enabled <https://bugs.webkit.org/show_bug.cgi?id=22840> <rdar://problem/6468077> * bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::emitNewArray): Add an assertion that the range of registers passed to op_new_array is sequential. (JSC::BytecodeGenerator::emitCall): Correct the relocation of registers when emitting profiler hooks so that registers aren't leaked. Also, add an assertion that the 'this' register is always ref'd (because it is), remove the needless protection of the 'this' register when relocating, and add an assertion that the range of registers passed to op_call for function call arguments is sequential. (JSC::BytecodeGenerator::emitConstruct): Correct the relocation of registers when emitting profiler hooks so that registers aren't leaked. Also, add an assertion that the range of registers passed to op_construct for function call arguments is sequential. In the above comment you have posted comments to add an assertion that the range of registers passed to op_call/op_construct for function call arguments is sequential. So, I want to know what are the scenarios where this assert will come. Thanks and regards, Shafi