Bug 22840 - REGRESSION (r38349): Gmail doesn't load with profiling enabled
Summary: REGRESSION (r38349): Gmail doesn't load with profiling enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Nobody
URL: http://www.gmail.com/
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-12-12 23:03 PST by Cameron Zwarich (cpst)
Modified: 2010-02-08 00:36 PST (History)
4 users (show)

See Also:


Attachments
Patch in progress (1.32 KB, patch)
2008-12-27 04:00 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Patch in progress (2.33 KB, patch)
2008-12-27 04:22 PST, Cameron Zwarich (cpst)
no flags Details | Formatted Diff | Diff
Proposed patch (8.66 KB, patch)
2008-12-28 00:44 PST, Cameron Zwarich (cpst)
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron Zwarich (cpst) 2008-12-12 23:03:45 PST
With profiling enabled, Gmail fails to load with a JS exception.
Comment 1 Cameron Zwarich (cpst) 2008-12-14 18:10:53 PST
This was working in r37698, but starts crashing in r37764. By r38654, it is failing with an exception.
Comment 2 Cameron Zwarich (cpst) 2008-12-15 00:36:10 PST
This happens even with the bytecode interpreter.
Comment 3 Cameron Zwarich (cpst) 2008-12-27 00:14:00 PST
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.
Comment 4 Cameron Zwarich (cpst) 2008-12-27 00:34:06 PST
The problematic code was introduced in r38349:

http://trac.webkit.org/changeset/38349

Geoff, do you have a suggestion about how to fix this?
Comment 5 Cameron Zwarich (cpst) 2008-12-27 04:00:01 PST
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().
Comment 6 Cameron Zwarich (cpst) 2008-12-27 04:22:07 PST
Created attachment 26265 [details]
Patch in progress

Here is a patch with more assertions. Tests are coming.
Comment 7 Oliver Hunt 2008-12-27 18:39:37 PST
<rdar://problem/6468077>
Comment 8 Cameron Zwarich (cpst) 2008-12-28 00:44:05 PST
Created attachment 26273 [details]
Proposed patch
Comment 9 Oliver Hunt 2008-12-28 00:46:38 PST
Comment on attachment 26273 [details]
Proposed patch

r=me
Comment 10 Cameron Zwarich (cpst) 2008-12-28 00:52:29 PST
Landed in r39488.
Comment 11 Shafi 2010-02-07 23:15:49 PST
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
Comment 12 Oliver Hunt 2010-02-07 23:18:28 PST
(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?
Comment 13 Shafi 2010-02-07 23:42:08 PST
(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
Comment 14 Oliver Hunt 2010-02-07 23:49:14 PST
(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?
Comment 15 Shafi 2010-02-08 00:36:50 PST
> 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