Bug 103707 – AssemblyHelpers::decodedCodeMapFor crash

• Component: JavaScriptCore
• Status: UNCONFIRMED

[linzj]

when jit compiles the following function ,
function f(){this.init.apply(this,arguments)}
CodeBlock::canCompileWithDFG says its DFG::CapabilityLevel should be DFG::ShouldProfile.But when it comes to jit code map info collection phase,which looks like
#if ENABLE(DFG_JIT) || ENABLE(LLINT)
    if (canBeOptimized()
#if ENABLE(LLINT)
        || true
#endif
        ) {
        CompactJITCodeMap::Encoder jitCodeMapEncoder;
        for (unsigned bytecodeOffset = 0; bytecodeOffset < m_labels.size(); ++bytecodeOffset) {
            if (m_labels[bytecodeOffset].isSet())
                jitCodeMapEncoder.append(bytecodeOffset, patchBuffer.offsetOf(m_labels[bytecodeOffset]));
        }
        m_codeBlock->setJITCodeMap(jitCodeMapEncoder.finish());
    }
#endif

it refuse to collect these info,because current jit state is mark to cannot be optimize but should be profiled.
So when it comes to osr compilation phase,
    // 17) Jump into the corresponding baseline JIT code.
    
    CodeBlock* baselineCodeBlock = m_jit.baselineCodeBlockFor(exit.m_codeOrigin);
    Vector<BytecodeAndMachineOffset>& decodedCodeMap = m_jit.decodedCodeMapFor(baselineCodeBlock);
m_jit.decodedCodeMapFor(baselineCodeBlock); will cause crash.

[Alexey Proskuryakov]

Is this seen with the latest version of JavaScriptCore from webkit.org? I'm confused because of the platform being Android.

[linzj]

Well,I found this bug in r120658.Reviewed r134433,I believed it was still there.
I am working for a proprietary mobile browser company named UC Browser.It is using jsc as the javascript engine on the android platform.
(In reply to comment #1)
> Is this seen with the latest version of JavaScriptCore from webkit.org? I'm confused because of the platform being Android.

[linzj]

We configure the jsc as
#define ENABLE_JIT 1
#define WTF_CPU_ARM_TRADITIONAL 0
#define WTF_CPU_ARM_THUMB2 1
#define ENABLE_CLASSIC_INTERPRETER 1
#define ENABLE_DFG_JIT 1

[linzj]

wired.nobody is assigned,after such a long time.

[Filip Pizlo]

(In reply to comment #4)
> wired.nobody is assigned,after such a long time.

You could put up a patch fixing the issue, along with a test case.

[linzj]

(In reply to comment #5)
> (In reply to comment #4)
> > wired.nobody is assigned,after such a long time.
> 
> You could put up a patch fixing the issue, along with a test case.
Are you sure there would be a reviewer?I have seen bug 40300 hanging there for half a year without anyone reviewed.

[Filip Pizlo]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > wired.nobody is assigned,after such a long time.
> > 
> > You could put up a patch fixing the issue, along with a test case.
> Are you sure there would be a reviewer?I have seen bug 40300 hanging there for half a year without anyone reviewed.

Yes there will be a reviewer.

[linzj]

Created attachment 181273 [details]
patch to this crash

All comments are in the patch.

[linzj]

sorry! wrong patch!

[WebKit Review Bot]

Attachment 181273 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[linzj]

Created attachment 181274 [details]
patch to this crash

All comments are in this patch.

[Filip Pizlo]

Comment on attachment 181274 [details]
patch to this crash

Did you mean to mark as 'r?'?  We usually only review patches that are marked for review since it is common and acceptable to post work-in-progress patches.

[linzj]

(In reply to comment #12)
> (From update of attachment 181274 [details])
> Did you mean to mark as 'r?'?  We usually only review patches that are marked for review since it is common and acceptable to post work-in-progress patches.
Sorry.This is the first time I upload a patch.

[linzj]

Here is the crash stack.
#2  0x5f9d0fcc in JSC::DFG::AssemblyHelpers::decodedCodeMapFor (this=<optimized out>, codeBlock=0xdeadbaad)
    at /media/linzj/aad16a33-290e-488f-8339-29310a47571e/src/u3/android-cn-java-2/JavaScriptCore/dfg/DFGAssemblyHelpers.cpp:50
#3  0x5f9dafb6 in JSC::DFG::OSRExitCompiler::compileExit (this=0x5fd7d7a4, exit=..., recovery=<optimized out>)
    at /media/linzj/aad16a33-290e-488f-8339-29310a47571e/src/u3/android-cn-java-2/JavaScriptCore/dfg/DFGOSRExitCompiler32_64.cpp:689
#4  0x5f9da070 in JSC::DFG::compileOSRExit (exec=<optimized out>) at /media/linzj/aad16a33-290e-488f-8339-29310a47571e/src/u3/android-cn-java-2/JavaScriptCore/dfg/DFGOSRExitCompiler.cpp:79

[Filip Pizlo]

Comment on attachment 181274 [details]
patch to this crash

The patch doesn't apply. Can you rebate it?

[linzj]

(In reply to comment #15)
> (From update of attachment 181274 [details])
> The patch doesn't apply. Can you rebate it?

you may try 
patch -p N <patch
to apply it.If it doesn't work in your directory,please try in Source/JavaScriptCore/jit directory.

[linzj]

Created attachment 181285 [details]
patch to this crash

[linzj]

Can you apply it?Or should I checkout a trunk tree to recreate a new patch?

[Filip Pizlo]

(In reply to comment #18)
> Can you apply it?Or should I checkout a trunk tree to recreate a new patch?

Do you see how EWS is purple?  And is claiming that it cannot apply the patch?  This is what I'm referring to.

[Michael Saboff]

Comment on attachment 181285 [details]
patch to this crash

View in context: https://bugs.webkit.org/attachment.cgi?id=181285&action=review

Rebase patch, make sure it builds and then resubmit for review.

> Source/JavaScriptCore/jit/JIT.cpp:754
> +/*!! begin change by linzj !!*/
> +    if (true
> +// when compiling osrExit ,the jit code map is needed for jumping.The dfg optimized code is able to jump to
> +// any base line code no matter if  it is able to be optimized.So this fix will allow jit to collect jit code map
> +// for current code block in all conditions.
> +/*!! end change by linzj !!*/

Eliminate the /*!! … !!*/ comments.
Not sure how many of the other comments are needed.  If you still think some/all of these comment need to be present, make these complete sentences beginning with a capitalized word and with a space after the '.'

[Filip Pizlo]

Comment on attachment 181285 [details]
patch to this crash

View in context: https://bugs.webkit.org/attachment.cgi?id=181285&action=review

>> Source/JavaScriptCore/jit/JIT.cpp:754
>> +/*!! end change by linzj !!*/
> 
> Eliminate the /*!! … !!*/ comments.
> Not sure how many of the other comments are needed.  If you still think some/all of these comment need to be present, make these complete sentences beginning with a capitalized word and with a space after the '.'

Yeah, I mean, this change needs a lot of work.  You shouldn't be adding comments that say that it's your change; this is a bad style.  It doesn't scale.  Imagine if everyone who changed the code added such a comment; pretty soon you would have a giant mess.  The comment isn't accurate.  The change itself has some sloppiness.  First, it's bad because you're introducing an "if (true) ..." into the code.  Please don't do that.  If you have something that is unconditional and must always be executed, then you should remove the "if".  The change also assumes that the DFG can jump to any baseline code block; this is not true - it can only jump to those code blocks that can be inlined.  There is already machinery in this file for detecting whether a code block is inlineable.

I wouldn't be opposed to just making the CompactJITCodeMap creation unconditional on the grounds that we don't care about optimizing !ENABLE(LLINT) builds.  This change accomplishes this functionally but stylistically it needs a lot of work before it can be accepted into the repository.