Bug 119117 – Fix the wrong refining ArrayMode

• Component: JavaScriptCore
• Status: UNCONFIRMED

[Youngho Yoo]

Overview :

I tested on X86 and ARM arch,
There is crash in Google Translate(http://translate.google.com)

Steps to Reproduce:

1. http://translate.google.com

Actual Results:

When go into the address,it crashed.

Expected Results:

Successfully loading page, and then setting into idle condition.

Build Date & Platform:

Linux, Qt, WebKit2, ARM_THUMB2 with LLInt + JIT + DFG_JIT (rev 151088)
with -mfloat-abi=softfp and EABI compile option.

Additional Builds and Platforms:

It occurs on Linux, Qt, WebKit2, ARM_THUMB2 with LLInt + JIT (rev 151088) too.

Additional Information:

w/o DFG_JIT It doesn't occur.

Solution :

If we go to translate.google.com, QtWebProcess crash.
That is because of wrong refining array-mode in JSC
In speculating array, change refining array-mode rule in JSC, DFG-JIT mode.
In speculating array(refine func), Regardless of int32 or not in index, 
If base is int32, It shoulde be return ArrayMode(Array::Int32Array);

[Youngho Yoo]

Created attachment 207499 [details]
1st patch

[Mark Hahnenberg]

Comment on attachment 207499 [details]
1st patch

I'm not sure this is the correct fix. When would the base ever be an int? Do you have a reduced test case that reproduces the bug?

[Youngho Yoo]

Created attachment 207500 [details]
Patch

[Youngho Yoo]

(In reply to comment #2)
> (From update of attachment 207499 [details])
> I'm not sure this is the correct fix. When would the base ever be an int? Do you have a reduced test case that reproduces the bug?

Let me find it. I will make test case. Thanks.

[Filip Pizlo]

Comment on attachment 207500 [details]
Patch

Yeah, this makes no sense whatsoever.  The base is the array.  Your fix makes it so that every array access becomes a generic access (i.e. function call).  It defeats the whole purpose of having an optimizing compiler.

[Youngho Yoo]

(In reply to comment #5)
> (From update of attachment 207500 [details])
> Yeah, this makes no sense whatsoever.  The base is the array.  Your fix makes it so that every array access becomes a generic access (i.e. function call).  It defeats the whole purpose of having an optimizing compiler.

Ok, then I will try another approach for crash in Google Translate.

[Youngho Yoo]

(In reply to comment #5)
> (From update of attachment 207500 [details])
> Yeah, this makes no sense whatsoever.  The base is the array.  Your fix makes it so that every array access becomes a generic access (i.e. function call).  It defeats the whole purpose of having an optimizing compiler.

But.. there is already if (!isInt32Speculation(index)), so if any index is not int32, it becomes a generic access, I think it is wrong.
I hope that I have misunderstood about that. But I am curious abot that. Plz teach me.

And there is another review requests on https://bugs.webkit.org/show_bug.cgi?id=117281, Does anyone can review that? Thanks.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 207500 [details] [details])
> > Yeah, this makes no sense whatsoever.  The base is the array.  Your fix makes it so that every array access becomes a generic access (i.e. function call).  It defeats the whole purpose of having an optimizing compiler.
> 
> But.. there is already if (!isInt32Speculation(index)), so if any index is not int32, it becomes a generic access, I think it is wrong.

Why is that wrong?  The purpose of GetByVal optimizations is just to handle the case where the index is predicted to be an int32.

> I hope that I have misunderstood about that. But I am curious abot that. Plz teach me.
> 
> And there is another review requests on https://bugs.webkit.org/show_bug.cgi?id=117281, Does anyone can review that? Thanks.

[Youngho Yoo]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (From update of attachment 207500 [details] [details] [details])
> > > Yeah, this makes no sense whatsoever.  The base is the array.  Your fix makes it so that every array access becomes a generic access (i.e. function call).  It defeats the whole purpose of having an optimizing compiler.
> > 
> > But.. there is already if (!isInt32Speculation(index)), so if any index is not int32, it becomes a generic access, I think it is wrong.
> Why is that wrong?  The purpose of GetByVal optimizations is just to handle the case where the index is predicted to be an int32.

Yes, you are right. I appreciate your quick and correct reply, I will see this problem more in detail.
And here is the information, in translate.google.com :
some refine func arguments is this : SpeculatedType base = "<Int32>", index = "<String>", value = ""(SpecNone, because it called by case GetByVal: in DFGFixupPhase.cpp)
(I use static const char* speculationToAbbreviatedString(SpeculatedType prediction) func in SpeculatedType.cpp)
they return ArrayMode(Array::Generic);
funcc SpeculativeJIT::compile(Node* node) in "dfg/DFGSpeculativeJIT32_64.cpp" in case of GetByVal:
because of upper thing, it switched Array::Generic:, and call operationGetByValCell, but crash in here.

> > I hope that I have misunderstood about that. But I am curious abot that. Plz teach me.
> > 
> > And there is another review requests on https://bugs.webkit.org/show_bug.cgi?id=117281, Does anyone can review that? Thanks.

[Youngho Yoo]

Created attachment 207513 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGArrayMode.cpp']" exit_code: 1
Source/JavaScriptCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


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

[Youngho Yoo]

Created attachment 207514 [details]
Patch

[Youngho Yoo]

(In reply to comment #5)
> (From update of attachment 207500 [details])
> Yeah, this makes no sense whatsoever.  The base is the array.  Your fix makes it so that every array access becomes a generic access (i.e. function call).  It defeats the whole purpose of having an optimizing compiler.

If it's own type is Array::Unprofiled and index is Int32SpeculatedType, It will crash.
So I changed order of returning ArrayMode(Array::ForceExit) when type() is Array::Unprofiled.
Regardless index is Int32Speculation or not, Array::Unprofiled type has the highest priority.

[Mark Hahnenberg]

Comment on attachment 207514 [details]
Patch

This seems more correct. I'll let Phil do the final review though.

[Filip Pizlo]

Comment on attachment 207514 [details]
Patch

This is totally wrong.

[Filip Pizlo]

Comment on attachment 207514 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        If type() is Array::Unprofiled, refine func should return ArrayMode(Array::ForceExit)
> +        If refine func return ArrayMode(Array::Generic) in upper condition, it will crash.

It's incorrect to return ForceExit for Unprofiled.  The array profile may report Unprofiled if the base was not a cell, and in some cases if the index is not an integer.  That doesn't mean that the DFG should always exit.

Also, your changelog comment saying that if you return Generic it will crash reveals that you haven't really thought this through.  Why does it crash?  What is the test case?  I don't believe that randomly inserting crap into our code base because it averts a crash is the kind of development strategy we want to pursue.

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:130
> -    if (!base || !index) {
> +    if (!base || !index || type() == Array::Unprofiled) {

type() == Array::Unprofiled means that the array profile didn't have any information.  But the array profile will not have any information if the array access had a weird base.  In that case, we do want the access to go generic.

[Youngho Yoo]

(In reply to comment #16)
> (From update of attachment 207514 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207514&action=review
> > Source/JavaScriptCore/ChangeLog:11
> > +        If type() is Array::Unprofiled, refine func should return ArrayMode(Array::ForceExit)
> > +        If refine func return ArrayMode(Array::Generic) in upper condition, it will crash.
> It's incorrect to return ForceExit for Unprofiled.  The array profile may report Unprofiled if the base was not a cell, and in some cases if the index is not an integer.  That doesn't mean that the DFG should always exit.
> Also, your changelog comment saying that if you return Generic it will crash reveals that you haven't really thought this through.  Why does it crash?  What is the test case?  I don't believe that randomly inserting crap into our code base because it averts a crash is the kind of development strategy we want to pursue.
> > Source/JavaScriptCore/dfg/DFGArrayMode.cpp:130
> > -    if (!base || !index) {
> > +    if (!base || !index || type() == Array::Unprofiled) {
> type() == Array::Unprofiled means that the array profile didn't have any information.  But the array profile will not have any information if the array access had a weird base.  In that case, we do want the access to go generic.

OK, I will find the test case for this and I will see in more detail things.