Bug 103707 - AssemblyHelpers::decodedCodeMapFor crash
Summary: AssemblyHelpers::decodedCodeMapFor crash
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Critical
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-29 19:48 PST by linzj
Modified: 2014-02-05 11:41 PST (History)
6 users (show)

See Also:


Attachments
patch to this crash (537 bytes, patch)
2013-01-03 21:41 PST, linzj
no flags Details | Formatted Diff | Diff
patch to this crash (701 bytes, patch)
2013-01-03 21:44 PST, linzj
no flags Details | Formatted Diff | Diff
patch to this crash (837 bytes, patch)
2013-01-04 02:43 PST, linzj
msaboff: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description linzj 2012-11-29 19:48:33 PST
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.
Comment 1 Alexey Proskuryakov 2012-11-30 12:02:50 PST
Is this seen with the latest version of JavaScriptCore from webkit.org? I'm confused because of the platform being Android.
Comment 2 linzj 2012-11-30 18:26:30 PST
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.
Comment 3 linzj 2012-11-30 19:26:29 PST
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
Comment 4 linzj 2013-01-03 19:12:30 PST
wired.nobody is assigned,after such a long time.
Comment 5 Filip Pizlo 2013-01-03 19:18:07 PST
(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.
Comment 6 linzj 2013-01-03 20:36:23 PST
(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.
Comment 7 Filip Pizlo 2013-01-03 21:31:42 PST
(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.
Comment 8 linzj 2013-01-03 21:41:38 PST
Created attachment 181273 [details]
patch to this crash

All comments are in the patch.
Comment 9 linzj 2013-01-03 21:42:50 PST
sorry! wrong patch!
Comment 10 WebKit Review Bot 2013-01-03 21:42:52 PST
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.
Comment 11 linzj 2013-01-03 21:44:30 PST
Created attachment 181274 [details]
patch to this crash

All comments are in this patch.
Comment 12 Filip Pizlo 2013-01-03 22:29:10 PST
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.
Comment 13 linzj 2013-01-03 22:52:30 PST
(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.
Comment 14 linzj 2013-01-03 22:53:37 PST
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
Comment 15 Filip Pizlo 2013-01-03 23:01:39 PST
Comment on attachment 181274 [details]
patch to this crash

The patch doesn't apply. Can you rebate it?
Comment 16 linzj 2013-01-04 00:24:58 PST
(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.
Comment 17 linzj 2013-01-04 02:43:12 PST
Created attachment 181285 [details]
patch to this crash
Comment 18 linzj 2013-01-04 18:09:23 PST
Can you apply it?Or should I checkout a trunk tree to recreate a new patch?
Comment 19 Filip Pizlo 2013-01-04 18:11:52 PST
(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.
Comment 20 Michael Saboff 2013-10-31 13:57:40 PDT
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 '.'
Comment 21 Filip Pizlo 2013-12-02 10:54:51 PST
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.