Bug 142534

Summary: [Win64] Not all parameters are set up for slow path call.
Product: WebKit Reporter: peavo
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: achristensen, beidson, bfulgham, ggaren, mark.lam, msaboff, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch beidson: review-

Description peavo 2015-03-10 08:39:47 PDT
Only 6 parameters are set up, but a slow path call can have up to 8 parameters (including frame pointer).
Comment 1 peavo 2015-03-10 09:38:18 PDT
Created attachment 248332 [details]
Patch
Comment 2 peavo 2015-03-10 09:54:02 PDT
Created attachment 248334 [details]
Patch
Comment 3 Geoffrey Garen 2015-03-10 11:36:30 PDT
Is this covered by some kind of regression test?
Comment 4 peavo 2015-03-10 11:55:06 PDT
(In reply to comment #3)
> Is this covered by some kind of regression test?

I have run the stress tests, without any regressions.
Comment 5 Geoffrey Garen 2015-03-10 11:57:02 PDT
Sure, but is there a test that fails without this change and passes with it? If not, we need to add one.
Comment 6 peavo 2015-03-10 11:59:22 PDT
(In reply to comment #5)
> Sure, but is there a test that fails without this change and passes with it?
> If not, we need to add one.

I see, I'll look into it :)
Comment 7 Alex Christensen 2015-03-10 12:01:57 PDT
For the record, the "64-bit" WinCairo bot is building and running 32-bit tests.  We won't see the test failure or fix on that bot :(
Comment 8 Csaba Osztrogon√°c 2015-03-11 01:03:53 PDT
(In reply to comment #7)
> For the record, the "64-bit" WinCairo bot is building and running 32-bit
> tests.  We won't see the test failure or fix on that bot :(

Why do we call it 64 bit if it is a 32 bit bot? We should rename it.
Comment 9 Csaba Osztrogon√°c 2015-03-11 01:05:38 PDT
And what aboud the Apple Windows bots, aren't they 64 bit bots?
Comment 10 peavo 2015-03-16 12:24:25 PDT
Created attachment 248742 [details]
Patch
Comment 11 peavo 2015-03-16 12:28:53 PDT
It turns out nobody is currently using 8 parameters, but 6 parameters at most. I removed the unused overloads of setupArgumentsWithExecState in the last patch. I'm not quite sure how to test this, since nobody is using 8 parameters right now.
Comment 12 peavo 2015-03-16 12:35:24 PDT
Created attachment 248743 [details]
Patch
Comment 13 peavo 2015-03-16 12:43:37 PDT
(In reply to comment #11)
> It turns out nobody is currently using 8 parameters, but 6 parameters at
> most. I removed the unused overloads of setupArgumentsWithExecState in the
> last patch. I'm not quite sure how to test this, since nobody is using 8
> parameters right now.

Correction: Win64 is not using more than 6 parameters, but others are, so the overloads of setupArgumentsWithExecState I removed, needs to stay.
Comment 14 Alex Christensen 2015-06-15 12:13:22 PDT
Is this still needed?
Comment 15 peavo 2015-06-16 05:46:19 PDT
This is not really needed at the moment, since nobody is calling a slow path function with more than 6 parameters AFAIK. It might be needed in the future, though, if somebody were to add a slow path function with 7 or 8 parameters.
Comment 16 Brent Fulgham 2016-08-22 11:20:50 PDT
(In reply to comment #15)
> This is not really needed at the moment, since nobody is calling a slow path
> function with more than 6 parameters AFAIK. It might be needed in the
> future, though, if somebody were to add a slow path function with 7 or 8
> parameters.

If you don't think it's needed, we should close it.

If you think it's likely the 7 or 8-parameter case is useful we should get it approved and landed!
Comment 17 Brady Eidson 2017-04-24 19:05:55 PDT
Comment on attachment 248743 [details]
Patch

This patch has been pending review since 2015 with no recent activity.

Clearing from the review queue.

Feel free to update and resubmit if the patch is still relevant.