Bug 117281 - Crash in V8 benchmarks set in ARM,softfp,EABI
Summary: Crash in V8 benchmarks set in ARM,softfp,EABI
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Major
Assignee: Nobody
URL:
Keywords: PlatformOnly
: 116315 (view as bug list)
Depends on: 121287
Blocks: 108645
  Show dependency treegraph
 
Reported: 2013-06-05 21:42 PDT by Youngho Yoo
Modified: 2013-11-19 04:57 PST (History)
24 users (show)

See Also:


Attachments
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function. (6.51 KB, patch)
2013-06-05 22:05 PDT, Youngho Yoo
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
2nd_patch (6.27 KB, patch)
2013-06-06 09:45 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
3rd_patch(just changing ChaneLog) (6.27 KB, patch)
2013-06-19 18:33 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
4th_patch(add tests) (10.74 KB, patch)
2013-06-20 19:24 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
add tests and modify ChangeLog (10.74 KB, patch)
2013-06-21 09:19 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
add tests and modify ChangeLog (10.74 KB, patch)
2013-06-21 09:21 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
move the js test. (10.98 KB, patch)
2013-07-08 23:24 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
move the js test (10.98 KB, patch)
2013-07-08 23:47 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (623.43 KB, application/zip)
2013-07-09 01:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (473.81 KB, application/zip)
2013-07-10 20:37 PDT, Build Bot
no flags Details
build verification again. (10.98 KB, patch)
2013-07-14 17:10 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (550.68 KB, application/zip)
2013-07-14 19:16 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (492.55 KB, application/zip)
2013-07-15 14:59 PDT, Build Bot
no flags Details
Patch (10.98 KB, patch)
2013-07-25 18:18 PDT, Youngho Yoo
ggaren: review-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (551.73 KB, application/zip)
2013-07-26 13:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (530.86 KB, application/zip)
2013-07-27 06:28 PDT, Build Bot
no flags Details
verification test (10.98 KB, patch)
2013-09-10 22:03 PDT, Youngho Yoo
ggaren: review-
Details | Formatted Diff | Diff
verification test2 (10.98 KB, text/plain)
2013-09-10 22:07 PDT, Youngho Yoo
no flags Details
Patch_verification (10.98 KB, patch)
2013-09-10 22:35 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
add UNUSED_PARAM (11.04 KB, patch)
2013-09-11 18:44 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
omit_arg1 (10.97 KB, patch)
2013-09-11 23:20 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff
omit_arg1_and_add_line (10.67 KB, patch)
2013-09-12 17:57 PDT, Youngho Yoo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Youngho Yoo 2013-06-05 21:42:00 PDT
Overview :

In Qt, ARM platform, -mfloat-abi=softfp and EABI compile option,
There is crash in V8 benchmark, splay.js 

Steps to Reproduce:

1. http://v8.googlecode.com/svn/data/benchmarks/v7/run.html
2. Automately run V8 Benchmark suite - version 7

Actual Results:

When running Splay benchmarks it crashed and rerun the web page.

Expected Results:

Successfully running benchmarks, and then < Score : xxxx > showed up.

Build Date & Platform:

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

Additional Builds and Platforms:

Doesn't Occur on Linux, Qt, WebKit2, ARM_THUMB2 with LLInt + JIT (rev 149600)

Additional Information:

i) w/o DFG_JIT It doesn't occur.

ii) in splay.js, there is the push function

-----
/**
 * @return {Array<*>} An array containing all the keys of tree's nodes.
 */
SplayTree.prototype.exportKeys = function() {
  var result = [];
  if (!this.isEmpty()) {
    this.root_.traverse_(function(node) { result.push(node.key); });
  }
  return result;
};
-----

That is the crash site in JSC with running DFG-JIT.

iii) and http://www.smashcat.org/av/canvas_test is crash too.


Solution :

With -mfloat-abi=softfp and EABI compile option,
ii) and iii), call 
ALWAYS_INLINE void setupArgumentsWithExecState(FPRReg arg1, GPRReg arg2)<line 564, revision 151251>
and 
ALWAYS_INLINE void setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, FPRReg arg3)<line 571, revision 151251>
in dfg/DFGCCallHelpers.h

if it uses no hardfp, which means that it uses softfp,
And when being compiled in ARM EABI, it must be aligned even-numbered register (r0, r2 or [sp]).
 To avoid assemblies from using wrong registers, let's occupy r1 or r3 with a dummy argument when necessary.

In JSC, it uses EABI_32BIT_DUMMY_ARG for that, but in softfp option, it misses using EABI_32BIT_DUMMY_ARG for FPRReg, 
even if it uses like this below(don't aligned even-numbered register)
assembler().vmov(GPRInfo::argumentGPR1, GPRInfo::argumentGPR2, arg1);

So change the code with EABI_32BIT_DUMMY_ARG will fix it.
Comment 1 Youngho Yoo 2013-06-05 22:05:23 PDT
Created attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Initial patch
Comment 2 EFL EWS Bot 2013-06-06 01:31:16 PDT
Comment on attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Attachment 203904 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/745415
Comment 3 EFL EWS Bot 2013-06-06 01:33:04 PDT
Comment on attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Attachment 203904 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/745416
Comment 4 Early Warning System Bot 2013-06-06 01:33:33 PDT
Comment on attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Attachment 203904 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/697457
Comment 5 Early Warning System Bot 2013-06-06 01:34:53 PDT
Comment on attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Attachment 203904 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/745417
Comment 6 kov's GTK+ EWS bot 2013-06-06 01:43:52 PDT
Comment on attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Attachment 203904 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/695524
Comment 7 Build Bot 2013-06-06 02:03:35 PDT
Comment on attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Attachment 203904 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/716435
Comment 8 Build Bot 2013-06-06 03:38:14 PDT
Comment on attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Attachment 203904 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/758785
Comment 9 Youngho Yoo 2013-06-06 09:45:06 PDT
Created attachment 203943 [details]
2nd_patch

For considerating 64bit, seperating calloperation JITCompiler::Call callOperation(V_DFGOperation_EOZD operation, GPRReg arg1, GPRReg arg2, FPRReg arg3)
Comment 10 Youngho Yoo 2013-06-10 18:43:31 PDT
I uploaded patch.

review please.
Comment 11 Csaba Osztrogonác 2013-06-10 21:38:29 PDT
(In reply to comment #10)
> I uploaded patch.
> 
> review please.

In this case why did you close the bug and why 
didn't you upload the proposed patch with r? flag?
Comment 12 Youngho Yoo 2013-06-10 22:58:19 PDT
I do some mistake, change status again.
Comment 13 Youngho Yoo 2013-06-10 23:12:05 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > I uploaded patch.
> > 
> > review please.
> 
> In this case why did you close the bug and why 
> didn't you upload the proposed patch with r? flag?

I re-uploaded the proposed patch with r? flags. Thanks.
Comment 14 Youngho Yoo 2013-06-19 13:35:44 PDT
Should I re-uploaded patch? or not?

What Should I do now?
Comment 15 Mark Lam 2013-06-19 15:02:30 PDT
Comment on attachment 203943 [details]
2nd_patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        Crash fixex in V8 benchmark suite in ARM,softp,EABI environment.

Did you mean to say "Fixed crash in V8 benchmark suite for ARM, softfp, EABI environment"?

> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
> +
> +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> +    {
> +        moveDouble(arg2, FPRInfo::argumentFPR0);
> +        move(arg3, GPRInfo::argumentGPR1);
> +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> +    }
> +
> +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> +    {
> +        moveDouble(arg4, FPRInfo::argumentFPR0);
> +        setupStubArguments(arg2, arg3);
> +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> +    }

It seems these are not use in the ARM_HARDFP port.  I see no reason to add them.

> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:615
> +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> +    {
> +        poke(arg3, POKE_ARGUMENT_OFFSET);
> +        move(arg1, GPRInfo::argumentGPR1);
> +        assembler().vmov(GPRInfo::argumentGPR2, GPRInfo::argumentGPR3, arg2);
> +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> +    }
> +
> +    ALWAYS_INLINE void setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, TrustedImm32 arg3, FPRReg arg4)
> +    {
> +        setupStubArguments(arg1, arg2);
> +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> +        move(arg3, GPRInfo::argumentGPR3);
> +        assembler().vmov(GPRInfo::nonArgGPR0, GPRInfo::nonArgGPR1, arg4);
> +        poke(GPRInfo::nonArgGPR0, POKE_ARGUMENT_OFFSET);
> +        poke(GPRInfo::nonArgGPR1, POKE_ARGUMENT_OFFSET + 1);
> +    }

In the !ARM_HARDFP port, these replaces "setupArgumentsWithExecState(FPRReg arg1, GPRReg arg2)" and "setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, FPRReg arg3)".  Remove the now unused functions.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:-1041
> -    JITCompiler::Call callOperation(V_DFGOperation_EOZD operation, GPRReg arg1, GPRReg arg2, FPRReg arg3)
> -    {
> -        m_jit.setupArgumentsWithExecState(arg1, arg2, arg3);
> -        return appendCallWithExceptionCheck(operation);
> -    }
> -

Is there a reason that you move this function below?
Comment 16 Youngho Yoo 2013-06-19 16:30:26 PDT
Comment on attachment 203943 [details]
2nd_patch

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

>> Source/JavaScriptCore/ChangeLog:3
>> +        Crash fixex in V8 benchmark suite in ARM,softp,EABI environment.
> 
> Did you mean to say "Fixed crash in V8 benchmark suite for ARM, softfp, EABI environment"?

Yes, I will change that phrase.

>> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
>> +    }
> 
> It seems these are not use in the ARM_HARDFP port.  I see no reason to add them.

But this is for EABI, ARM_HARDFP port. (relate with @1396,@1494)
Is ARM_HARDFP port doesn't using EABI?
If or not, I will remove them.

>> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:615
>> +    }
> 
> In the !ARM_HARDFP port, these replaces "setupArgumentsWithExecState(FPRReg arg1, GPRReg arg2)" and "setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, FPRReg arg3)".  Remove the now unused functions.

Similar reason like upper things.
But "setupArgumentsWithExecState(FPRReg arg1, GPRReg arg2)" and "setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, FPRReg arg3)" is for nonEABI, !ARM_HARDFP port. (relate with @1396,@1494)
If there is no nonEABI and !ARM_HARDFP port, I will remove them.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:-1041
>> -
> 
> Is there a reason that you move this function below?

Yes.
first of all, "#define EABI_32BIT_DUMMY_ARG      TrustedImm32(0)"(@1303), only exists in "#else // USE(JSVALUE32_64)"(@1298).
So, because of that, I need to seperate "#if USE(JSVALUE64)"(@1089) and "#else // USE(JSVALUE32_64)"(@1298).
Comment 17 Youngho Yoo 2013-06-19 17:04:19 PDT
(In reply to comment #15)
In summary, there are 4 cases in there,

i) EABI, ARM_HARDFP

ii) !EABI, ARM_HARDFP

iii) EABI, !ARM_HARDFP

iv) !EABI, !ARM_HARDFP

If there is no ii) and iv) I will modified code.

If or not, I will re-upload with just changinglog
Comment 18 Mark Lam 2013-06-19 17:53:35 PDT
Comment on attachment 203943 [details]
2nd_patch

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

LGTM after you fix the ChangeLog, but need a reviewer to r+.

>>> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
>>> +    }
>> 
>> It seems these are not use in the ARM_HARDFP port.  I see no reason to add them.
> 
> But this is for EABI, ARM_HARDFP port. (relate with @1396,@1494)
> Is ARM_HARDFP port doesn't using EABI?
> If or not, I will remove them.

OK, I was mistaken.  Please disregard my comment.

>>> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:615
>>> +    }
>> 
>> In the !ARM_HARDFP port, these replaces "setupArgumentsWithExecState(FPRReg arg1, GPRReg arg2)" and "setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, FPRReg arg3)".  Remove the now unused functions.
> 
> Similar reason like upper things.
> But "setupArgumentsWithExecState(FPRReg arg1, GPRReg arg2)" and "setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, FPRReg arg3)" is for nonEABI, !ARM_HARDFP port. (relate with @1396,@1494)
> If there is no nonEABI and !ARM_HARDFP port, I will remove them.

I was mistaken.  Please disregard my comment.
Comment 19 Youngho Yoo 2013-06-19 18:33:59 PDT
Created attachment 205047 [details]
3rd_patch(just changing ChaneLog)

following Mark Lam's review, modifying the ChangeLog.
Comment 20 Youngho Yoo 2013-06-19 18:34:57 PDT
(In reply to comment #18)
Should I put in r? or r+ ?

I put in r? now.

Sorry for bothering you.
Comment 21 Mark Lam 2013-06-19 18:36:35 PDT
(In reply to comment #20)
> (In reply to comment #18)
> Should I put in r? or r+ ?

r?.  Only a reviewer has the privilege to r+ a patch (I'm not a reviewer).
Comment 22 Gyuyoung Kim 2013-06-19 22:43:14 PDT
Generally, WebKit prefers to land a patch per a bug. So, "2nd patch" needs to be set as "obsolete".


> Actual Results:
> When running Splay benchmarks it crashed and rerun the web page.

It looks you need to add a test case to reproduce the crash.
Comment 23 Roman Zhuykov 2013-06-20 05:55:23 PDT
This bug is a duplicate of https://bugs.webkit.org/show_bug.cgi?id=116315
There I created a smaller patch which suits ARM EABI (hardfp and softfp), but I didn't take non-EABI ARM case into account. The patch presented here doesn't have such a drawback and in general looks correct, but my approach seems to be more straightforward. It can be modified to support non-EABI using one #if section (are there actually any ARM targets that don't use EABI?).
My patch contains two layout tests, it is possible to use them for this patch.
Comment 24 Youngho Yoo 2013-06-20 09:32:53 PDT
(In reply to comment #22)
> Generally, WebKit prefers to land a patch per a bug. So, "2nd patch" needs to be set as "obsolete".
> 
> 
> > Actual Results:
> > When running Splay benchmarks it crashed and rerun the web page.
> 
> It looks you need to add a test case to reproduce the crash.

Ok. I will do that. Thanks.
Comment 25 Youngho Yoo 2013-06-20 10:05:25 PDT
(In reply to comment #23)
> This bug is a duplicate of https://bugs.webkit.org/show_bug.cgi?id=116315
> There I created a smaller patch which suits ARM EABI (hardfp and softfp), but I didn't take non-EABI ARM case into account. The patch presented here doesn't have such a drawback and in general looks correct, but my approach seems to be more straightforward. It can be modified to support non-EABI using one #if section (are there actually any ARM targets that don't use EABI?).
> My patch contains two layout tests, it is possible to use them for this patch.

I think your patch is straightforward.
But if you add #if DFG DFGCCallHelpers.h,
It needs two maintencance points for EABI things.
That's why I just follow DFGSpeculativeJIT.h's convention.
And your regression tests is LGTM.
Comment 26 Youngho Yoo 2013-06-20 19:24:56 PDT
Created attachment 205138 [details]
4th_patch(add tests)

added regession tests for this crashes.
this will cover two crashes below:
i)  splay.js in V8, Octane benchmarks(ALWAYS_INLINE void setupArgumentsWithExecState(FPRReg arg1, GPRReg arg2)<line 564, revision 151251>)
ii) http://www.smashcat.org/av/canvas_test(ALWAYS_INLINE void setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, FPRReg arg3)<line 571, revision 151251>)
Comment 27 Youngho Yoo 2013-06-20 19:28:50 PDT
(In reply to comment #22)
> Generally, WebKit prefers to land a patch per a bug. So, "2nd patch" needs to be set as "obsolete".
> 
> 
> > Actual Results:
> > When running Splay benchmarks it crashed and rerun the web page.
> 
> It looks you need to add a test case to reproduce the crash.

I attach the new patch with tests. Thanks.
Comment 28 Youngho Yoo 2013-06-21 09:19:54 PDT
Created attachment 205191 [details]
add tests and modify ChangeLog

I omit modifying ChangeLog, upload again.
Comment 29 Youngho Yoo 2013-06-21 09:21:55 PDT
Created attachment 205192 [details]
 add tests and modify ChangeLog
Comment 30 Youngho Yoo 2013-07-02 01:16:48 PDT
Now, What should I do?
Do I need more things to do?
Thanks.
Comment 31 Csaba Osztrogonác 2013-07-02 01:22:23 PDT
(In reply to comment #30)
> Now, What should I do?
> Do I need more things to do?
> Thanks.

Ping the reviewers again and again if they don't review your patch. :)
Comment 32 Gyuyoung Kim 2013-07-02 01:27:03 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > Now, What should I do?
> > Do I need more things to do?
> > Thanks.
> 
> Ping the reviewers again and again if they don't review your patch. :)

It looks Geoffrey Garen can review this patch. :)
Comment 33 Geoffrey Garen 2013-07-08 12:51:42 PDT
It looks like the test you've added is for correctness, and not performance. The js/regress directory is dedicated to performance regression tests. js/ is a better place for correctness tests.
Comment 34 Youngho Yoo 2013-07-08 23:24:15 PDT
Created attachment 206294 [details]
move the js test.

follow Geoffrey Garen's comment, move the js test.
Comment 35 Youngho Yoo 2013-07-08 23:27:11 PDT
(In reply to comment #34)
> Created an attachment (id=206294) [details]
> move the js test.
> 
> follow Geoffrey Garen's comment, move the js test.

(In reply to comment #33)
> It looks like the test you've added is for correctness, and not performance. The js/regress directory is dedicated to performance regression tests. js/ is a better place for correctness tests.

I moved the js tests. Thanks.
Comment 36 Youngho Yoo 2013-07-08 23:47:47 PDT
Created attachment 206295 [details]
move the js test

Follwing the Geoffrey Garen's comment, move the js tests and fix the wrong spell in tests.
Comment 37 Build Bot 2013-07-09 01:21:35 PDT
Comment on attachment 206295 [details]
move the js test

Attachment 206295 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/954586

New failing tests:
fast/js/array-with-double-push.html
fast/js/array-with-double-assign.html
Comment 38 Build Bot 2013-07-09 01:21:39 PDT
Created attachment 206299 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 39 Youngho Yoo 2013-07-09 19:23:12 PDT
(In reply to comment #33)
> It looks like the test you've added is for correctness, and not performance. The js/regress directory is dedicated to performance regression tests. js/ is a better place for correctness tests.

In the lastest patch, test failure in mac isn't my fault.
Other bug report has shown same issue.
Comment 40 Build Bot 2013-07-10 20:37:15 PDT
Comment on attachment 206295 [details]
move the js test

Attachment 206295 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/881983

New failing tests:
fast/js/array-with-double-push.html
fast/js/array-with-double-assign.html
Comment 41 Build Bot 2013-07-10 20:37:23 PDT
Created attachment 206424 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 42 Youngho Yoo 2013-07-14 17:10:05 PDT
Created attachment 206635 [details]
build verification again.

same as previous patch(move the js test) but build verification again.
Comment 43 Build Bot 2013-07-14 19:16:39 PDT
Comment on attachment 206635 [details]
build verification again.

Attachment 206635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/962959

New failing tests:
fast/js/array-with-double-push.html
fast/js/array-with-double-assign.html
Comment 44 Build Bot 2013-07-14 19:16:44 PDT
Created attachment 206636 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 45 Build Bot 2013-07-15 14:59:06 PDT
Comment on attachment 206635 [details]
build verification again.

Attachment 206635 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1076201

New failing tests:
fast/js/array-with-double-push.html
fast/js/array-with-double-assign.html
Comment 46 Build Bot 2013-07-15 14:59:12 PDT
Created attachment 206689 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 47 Youngho Yoo 2013-07-21 17:44:23 PDT
Should I pass mac and mac-wk2 test?
If or not, Is there any wrong in my patch set?
Comment 48 Youngho Yoo 2013-07-25 18:18:16 PDT
Created attachment 207501 [details]
Patch
Comment 49 Build Bot 2013-07-26 13:49:24 PDT
Comment on attachment 207501 [details]
Patch

Attachment 207501 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1242317

New failing tests:
fast/js/array-with-double-push.html
fast/js/array-with-double-assign.html
Comment 50 Build Bot 2013-07-26 13:49:30 PDT
Created attachment 207551 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 51 Build Bot 2013-07-27 06:28:32 PDT
Comment on attachment 207501 [details]
Patch

Attachment 207501 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1250467

New failing tests:
http/tests/security/cross-origin-plugin-private-browsing-toggled.html
fast/js/array-with-double-push.html
fast/js/array-with-double-assign.html
Comment 52 Build Bot 2013-07-27 06:28:38 PDT
Created attachment 207584 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 53 Peng Xinchao 2013-08-05 00:38:48 PDT
why nobody review the patch? I think the patch is ok .Does it have other issue?
Comment 54 Geoffrey Garen 2013-09-10 08:41:03 PDT
Comment on attachment 206635 [details]
build verification again.

Clearing review flag on obsolete patch.
Comment 55 Geoffrey Garen 2013-09-10 08:41:40 PDT
Comment on attachment 207501 [details]
Patch

It looks like the tests you added are failing on the mac-wk2 builder:

  fast/js/array-with-double-assign.html [ Failure ]
  fast/js/array-with-double-push.html [ Failure ]
Comment 56 Youngho Yoo 2013-09-10 22:03:49 PDT
Created attachment 211279 [details]
verification test

verification test
Comment 57 Youngho Yoo 2013-09-10 22:07:25 PDT
Created attachment 211280 [details]
verification test2

verification test2 - use ' insteadof "
Comment 58 Youngho Yoo 2013-09-10 22:35:49 PDT
Created attachment 211281 [details]
Patch_verification

make same text, actual and expected file.
Comment 59 Youngho Yoo 2013-09-10 23:14:30 PDT
(In reply to comment #55)
> (From update of attachment 207501 [details])
> It looks like the tests you added are failing on the mac-wk2 builder:
> 
>   fast/js/array-with-double-assign.html [ Failure ]
>   fast/js/array-with-double-push.html [ Failure ]

I passed the test by using same patch. could you review please?
Comment 60 Geoffrey Garen 2013-09-11 12:21:55 PDT
Comment on attachment 211279 [details]
verification test

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

> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
> +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> +    {
> +        moveDouble(arg2, FPRInfo::argumentFPR0);
> +        move(arg3, GPRInfo::argumentGPR1);
> +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> +    }
> +
> +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> +    {
> +        moveDouble(arg4, FPRInfo::argumentFPR0);
> +        setupStubArguments(arg2, arg3);
> +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> +    }

It looks like arg1 is unused in these functions. Is that a bug? I believe that shouldn't even build, due to the unused argument warning. Did you test this code on ARM_HARDFP?
Comment 61 Youngho Yoo 2013-09-11 17:18:21 PDT
(In reply to comment #60)
> (From update of attachment 211279 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211279&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
> > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> > +    {
> > +        moveDouble(arg2, FPRInfo::argumentFPR0);
> > +        move(arg3, GPRInfo::argumentGPR1);
> > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > +    }
> > +
> > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> > +    {
> > +        moveDouble(arg4, FPRInfo::argumentFPR0);
> > +        setupStubArguments(arg2, arg3);
> > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > +    }
> 
> It looks like arg1 is unused in these functions. Is that a bug? I believe that shouldn't even build, due to the unused argument warning. Did you test this code on ARM_HARDFP?

You are right, these two functions are the EABI_32BIT_DUMMY_ARG, ARM_HARDFP case.Then, How about use UNUSED_PARAM(arg1)?
Comment 62 Michael Saboff 2013-09-11 17:35:40 PDT
I tried the last patch on mac 64 bit and it passed tests fine.
Comment 63 Michael Saboff 2013-09-11 17:48:52 PDT
(In reply to comment #61)
> (In reply to comment #60)
> > (From update of attachment 211279 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=211279&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
> > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> > > +    {
> > > +        moveDouble(arg2, FPRInfo::argumentFPR0);
> > > +        move(arg3, GPRInfo::argumentGPR1);
> > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > +    }
> > > +
> > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> > > +    {
> > > +        moveDouble(arg4, FPRInfo::argumentFPR0);
> > > +        setupStubArguments(arg2, arg3);
> > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > +    }
> > 
> > It looks like arg1 is unused in these functions. Is that a bug? I believe that shouldn't even build, due to the unused argument warning. Did you test this code on ARM_HARDFP?
> 
> You are right, these two functions are the EABI_32BIT_DUMMY_ARG, ARM_HARDFP case.Then, How about use UNUSED_PARAM(arg1)?

Use UNUSED_PARAM(arg1).

Is there any concern that the argument registers are the same as the argumentGPRN?  Seems like this could be the case since ARM has a limited number of registers.
Comment 64 Youngho Yoo 2013-09-11 18:15:34 PDT
(In reply to comment #63)
> (In reply to comment #61)
> > (In reply to comment #60)
> > > (From update of attachment 211279 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=211279&action=review
> > > 
> > > > Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
> > > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> > > > +    {
> > > > +        moveDouble(arg2, FPRInfo::argumentFPR0);
> > > > +        move(arg3, GPRInfo::argumentGPR1);
> > > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > > +    }
> > > > +
> > > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> > > > +    {
> > > > +        moveDouble(arg4, FPRInfo::argumentFPR0);
> > > > +        setupStubArguments(arg2, arg3);
> > > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > > +    }
> > > 
> > > It looks like arg1 is unused in these functions. Is that a bug? I believe that shouldn't even build, due to the unused argument warning. Did you test this code on ARM_HARDFP?
> > 
> > You are right, these two functions are the EABI_32BIT_DUMMY_ARG, ARM_HARDFP case.Then, How about use UNUSED_PARAM(arg1)?
> 
> Use UNUSED_PARAM(arg1).
> 
> Is there any concern that the argument registers are the same as the argumentGPRN?  Seems like this could be the case since ARM has a limited number of registers.

In ARM, r0-r3 are the argument and scratch registers. r0-r1 are also the result registers. So there are only 4 argumentGPRN and others use poke.

See the http://www.scribd.com/doc/6546078/ARM-Architecture-Procedure-Call-Standard#page=15.
Comment 65 Youngho Yoo 2013-09-11 18:44:26 PDT
Created attachment 211378 [details]
add UNUSED_PARAM

add UNUSED_PARAM
Comment 66 Michael Saboff 2013-09-11 22:56:45 PDT
(In reply to comment #64)
> (In reply to comment #63)
> > (In reply to comment #61)
> > > (In reply to comment #60)
> > > > (From update of attachment 211279 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=211279&action=review
> > > > 
> > > > > Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
> > > > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> > > > > +    {
> > > > > +        moveDouble(arg2, FPRInfo::argumentFPR0);
> > > > > +        move(arg3, GPRInfo::argumentGPR1);
> > > > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > > > +    }
> > > > > +
> > > > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> > > > > +    {
> > > > > +        moveDouble(arg4, FPRInfo::argumentFPR0);
> > > > > +        setupStubArguments(arg2, arg3);
> > > > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > > > +    }
> > > > 
> > > > It looks like arg1 is unused in these functions. Is that a bug? I believe that shouldn't even build, due to the unused argument warning. Did you test this code on ARM_HARDFP?
> > > 
> > > You are right, these two functions are the EABI_32BIT_DUMMY_ARG, ARM_HARDFP case.Then, How about use UNUSED_PARAM(arg1)?
> > 
> > Use UNUSED_PARAM(arg1).
> > 
> > Is there any concern that the argument registers are the same as the argumentGPRN?  Seems like this could be the case since ARM has a limited number of registers.
> 
> In ARM, r0-r3 are the argument and scratch registers. r0-r1 are also the result registers. So there are only 4 argumentGPRN and others use poke.
> 
> See the http://www.scribd.com/doc/6546078/ARM-Architecture-Procedure-Call-Standard#page=15.

I'm aware of the calling convention.  My concern was if there is every a case where the arguments and the calling convention arg registers could step on each other.  Given the order that the argument registers are moved into the calling convention arg registers AND that you use setupStubArguments() which is aware of the cases where the input registers collide with the arg registers, I think everything is fine.
Comment 67 Filip Pizlo 2013-09-11 22:59:18 PDT
(In reply to comment #63)
> (In reply to comment #61)
> > (In reply to comment #60)
> > > (From update of attachment 211279 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=211279&action=review
> > > 
> > > > Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
> > > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> > > > +    {
> > > > +        moveDouble(arg2, FPRInfo::argumentFPR0);
> > > > +        move(arg3, GPRInfo::argumentGPR1);
> > > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > > +    }
> > > > +
> > > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> > > > +    {
> > > > +        moveDouble(arg4, FPRInfo::argumentFPR0);
> > > > +        setupStubArguments(arg2, arg3);
> > > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > > +    }
> > > 
> > > It looks like arg1 is unused in these functions. Is that a bug? I believe that shouldn't even build, due to the unused argument warning. Did you test this code on ARM_HARDFP?
> > 
> > You are right, these two functions are the EABI_32BIT_DUMMY_ARG, ARM_HARDFP case.Then, How about use UNUSED_PARAM(arg1)?
> 
> Use UNUSED_PARAM(arg1).

Better yet just omit the 'arg1' name.  Say:

ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32, GPRReg arg2, GPRReg arg3, FPRReg arg4)

Instead of:

ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)

> 
> Is there any concern that the argument registers are the same as the argumentGPRN?  Seems like this could be the case since ARM has a limited number of registers.
Comment 68 Youngho Yoo 2013-09-11 23:10:49 PDT
(In reply to comment #67)
> (In reply to comment #63)
> > (In reply to comment #61)
> > > (In reply to comment #60)
> > > > (From update of attachment 211279 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=211279&action=review
> > > > 
> > > > > Source/JavaScriptCore/dfg/DFGCCallHelpers.h:571
> > > > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, FPRReg arg2, GPRReg arg3)
> > > > > +    {
> > > > > +        moveDouble(arg2, FPRInfo::argumentFPR0);
> > > > > +        move(arg3, GPRInfo::argumentGPR1);
> > > > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > > > +    }
> > > > > +
> > > > > +    ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> > > > > +    {
> > > > > +        moveDouble(arg4, FPRInfo::argumentFPR0);
> > > > > +        setupStubArguments(arg2, arg3);
> > > > > +        move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
> > > > > +    }
> > > > 
> > > > It looks like arg1 is unused in these functions. Is that a bug? I believe that shouldn't even build, due to the unused argument warning. Did you test this code on ARM_HARDFP?
> > > 
> > > You are right, these two functions are the EABI_32BIT_DUMMY_ARG, ARM_HARDFP case.Then, How about use UNUSED_PARAM(arg1)?
> > 
> > Use UNUSED_PARAM(arg1).
> 
> Better yet just omit the 'arg1' name.  Say:
> 
> ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> 
> Instead of:
> 
> ALWAYS_INLINE void setupArgumentsWithExecState(TrustedImm32 arg1, GPRReg arg2, GPRReg arg3, FPRReg arg4)
> 
> > 
> > Is there any concern that the argument registers are the same as the argumentGPRN?  Seems like this could be the case since ARM has a limited number of registers.

OK, It's much better. I Think.
Comment 69 Youngho Yoo 2013-09-11 23:20:46 PDT
Created attachment 211401 [details]
omit_arg1

Follow Filip's advice, omit the 'arg1'
Comment 70 Michael Saboff 2013-09-12 08:23:28 PDT
Comment on attachment 211401 [details]
omit_arg1

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

r=me with suggested empty line format changes.

Sorry it took so long to get this reviewed.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1260
> +    }

Add an empty line below this method.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1291
>      }

Put the empty line below this method back in.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1498
> +    }

Add an empty line below this method.
Comment 71 Youngho Yoo 2013-09-12 17:57:49 PDT
Created attachment 211495 [details]
omit_arg1_and_add_line

omit_arg1_and_add_line
Comment 72 Michael Saboff 2013-09-12 18:00:02 PDT
Comment on attachment 211495 [details]
omit_arg1_and_add_line

r=me

You didn't need to post the patch with the lines added back in.  I figured you'd make those changes locally before checking the change in.
Comment 73 Youngho Yoo 2013-09-12 18:03:59 PDT
(In reply to comment #72)
> (From update of attachment 211495 [details])
> r=me
> 
> You didn't need to post the patch with the lines added back in.  I figured you'd make those changes locally before checking the change in.

Oh. I see. Anyway, Thanks a lot. :)
Comment 74 Youngho Yoo 2013-09-12 18:28:24 PDT
(In reply to comment #72)
> (From update of attachment 211495 [details])
> r=me
> 
> You didn't need to post the patch with the lines added back in.  I figured you'd make those changes locally before checking the change in.

I have a question for this.
How can I change locally without using commit-bot?
I set a commit-queue? flag in omit_arg1_and_add_line, first.
What can I do else?
Comment 75 Gyuyoung Kim 2013-09-12 19:35:12 PDT
(In reply to comment #72)
> (From update of attachment 211495 [details])
> r=me
> 
> You didn't need to post the patch with the lines added back in.  I figured you'd make those changes locally before checking the change in.

It seems to me he is not committer. So, he can't land it by himself.
Comment 76 WebKit Commit Bot 2013-09-13 00:33:16 PDT
Comment on attachment 211495 [details]
omit_arg1_and_add_line

Clearing flags on attachment: 211495

Committed r155675: <http://trac.webkit.org/changeset/155675>
Comment 77 Csaba Osztrogonác 2013-09-13 07:38:09 PDT
(In reply to comment #76)
> (From update of attachment 211495 [details])
> Clearing flags on attachment: 211495
> 
> Committed r155675: <http://trac.webkit.org/changeset/155675>

Test fixes landed in
 - https://trac.webkit.org/changeset/155695
 - https://trac.webkit.org/changeset/155695

and it broke the hardfp build: https://bugs.webkit.org/show_bug.cgi?id=121287
Comment 78 Brent Fulgham 2013-10-30 11:19:02 PDT
Comment on attachment 211378 [details]
add UNUSED_PARAM

Clearing flags since patch has been landed.
Comment 79 Brent Fulgham 2013-10-30 11:19:31 PDT
Comment on attachment 211401 [details]
omit_arg1

Clearing flags since patch has been landed.
Comment 80 Csaba Osztrogonác 2013-11-19 04:57:01 PST
*** Bug 116315 has been marked as a duplicate of this bug. ***