Bug 117281 – Crash in V8 benchmarks set in ARM,softfp,EABI

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Youngho Yoo]

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.

[Youngho Yoo]

Created attachment 203904 [details]
Initial patch for fixing the missing EABI_32BIT_DUMMY_ARG in FPRReg using callOperation function.

Initial patch

[EFL EWS Bot]

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

[EFL EWS Bot]

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

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[kov's GTK+ EWS bot]

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

[Build Bot]

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

[Build Bot]

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

[Youngho Yoo]

Created attachment 203943 [details]
2nd_patch

For considerating 64bit, seperating calloperation JITCompiler::Call callOperation(V_DFGOperation_EOZD operation, GPRReg arg1, GPRReg arg2, FPRReg arg3)

[Youngho Yoo]

I uploaded patch.

review please.

[Csaba Osztrogonác]

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

[Youngho Yoo]

I do some mistake, change status again.

[Youngho Yoo]

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

[Youngho Yoo]

Should I re-uploaded patch? or not?

What Should I do now?

[Mark Lam]

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?

[Youngho Yoo]

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

[Youngho Yoo]

(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

[Mark Lam]

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.

[Youngho Yoo]

Created attachment 205047 [details]
3rd_patch(just changing ChaneLog)

following Mark Lam's review, modifying the ChangeLog.

[Youngho Yoo]

(In reply to comment #18)
Should I put in r? or r+ ?

I put in r? now.

Sorry for bothering you.

[Mark Lam]

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

[Gyuyoung Kim]

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.

[Roman Zhuykov]

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.

[Youngho Yoo]

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

[Youngho Yoo]

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

[Youngho Yoo]

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

[Youngho Yoo]

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

[Youngho Yoo]

Created attachment 205191 [details]
add tests and modify ChangeLog

I omit modifying ChangeLog, upload again.

[Youngho Yoo]

Created attachment 205192 [details]
 add tests and modify ChangeLog

[Youngho Yoo]

Now, What should I do?
Do I need more things to do?
Thanks.

[Csaba Osztrogonác]

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

[Gyuyoung Kim]

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

[Geoffrey Garen]

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.

[Youngho Yoo]

Created attachment 206294 [details]
move the js test.

follow Geoffrey Garen's comment, move the js test.

[Youngho Yoo]

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

[Youngho Yoo]

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.

[Build Bot]

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

[Build Bot]

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

[Youngho Yoo]

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

[Build Bot]

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

[Build Bot]

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

[Youngho Yoo]

Created attachment 206635 [details]
build verification again.

same as previous patch(move the js test) but build verification again.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Youngho Yoo]

Should I pass mac and mac-wk2 test?
If or not, Is there any wrong in my patch set?

[Youngho Yoo]

Created attachment 207501 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Peng Xinchao]

why nobody review the patch? I think the patch is ok .Does it have other issue?

[Geoffrey Garen]

Comment on attachment 206635 [details]
build verification again.

Clearing review flag on obsolete patch.

[Geoffrey Garen]

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 ]

[Youngho Yoo]

Created attachment 211279 [details]
verification test

verification test

[Youngho Yoo]

Created attachment 211280 [details]
verification test2

verification test2 - use ' insteadof "

[Youngho Yoo]

Created attachment 211281 [details]
Patch_verification

make same text, actual and expected file.

[Youngho Yoo]

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

[Geoffrey Garen]

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?

[Youngho Yoo]

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

[Michael Saboff]

I tried the last patch on mac 64 bit and it passed tests fine.

[Michael Saboff]

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

[Youngho Yoo]

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

[Youngho Yoo]

Created attachment 211378 [details]
add UNUSED_PARAM

add UNUSED_PARAM

[Michael Saboff]

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

[Filip Pizlo]

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

[Youngho Yoo]

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

[Youngho Yoo]

Created attachment 211401 [details]
omit_arg1

Follow Filip's advice, omit the 'arg1'

[Michael Saboff]

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.

[Youngho Yoo]

Created attachment 211495 [details]
omit_arg1_and_add_line

omit_arg1_and_add_line

[Michael Saboff]

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.

[Youngho Yoo]

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

[Youngho Yoo]

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

[Gyuyoung Kim]

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

[WebKit Commit Bot]

Comment on attachment 211495 [details]
omit_arg1_and_add_line

Clearing flags on attachment: 211495

Committed r155675: <http://trac.webkit.org/changeset/155675>

[Csaba Osztrogonác]

(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

[Brent Fulgham]

Comment on attachment 211378 [details]
add UNUSED_PARAM

Clearing flags since patch has been landed.

[Brent Fulgham]

Comment on attachment 211401 [details]
omit_arg1

Clearing flags since patch has been landed.

[Csaba Osztrogonác]

*** Bug 116315 has been marked as a duplicate of this bug. ***