Bug 22840 – REGRESSION (r38349): Gmail doesn't load with profiling enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Cameron Zwarich (cpst)]

With profiling enabled, Gmail fails to load with a JS exception.

[Cameron Zwarich (cpst)]

This was working in r37698, but starts crashing in r37764. By r38654, it is failing with an exception.

[Cameron Zwarich (cpst)]

This happens even with the bytecode interpreter.

[Cameron Zwarich (cpst)]

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.

[Cameron Zwarich (cpst)]

The problematic code was introduced in r38349:

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

Geoff, do you have a suggestion about how to fix this?

[Cameron Zwarich (cpst)]

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().

[Cameron Zwarich (cpst)]

Created attachment 26265 [details]
Patch in progress

Here is a patch with more assertions. Tests are coming.

[Oliver Hunt]

<rdar://problem/6468077>

[Cameron Zwarich (cpst)]

Created attachment 26273 [details]
Proposed patch

[Oliver Hunt]

Comment on attachment 26273 [details]
Proposed patch

r=me

[Cameron Zwarich (cpst)]

Landed in r39488.

[Shafi]

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

[Oliver Hunt]

(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?

[Shafi]

(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

[Oliver Hunt]

(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?

[Shafi]

> 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  <[e-mail]>

        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