Bug 142534 - [Win64] Not all parameters are set up for slow path call.
Summary: [Win64] Not all parameters are set up for slow path call.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-03-10 08:39 PDT by peavo
Modified: 2017-04-24 19:05 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.48 KB, patch)
2015-03-10 09:38 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (3.56 KB, patch)
2015-03-10 09:54 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (9.54 KB, patch)
2015-03-16 12:24 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (3.65 KB, patch)
2015-03-16 12:35 PDT, peavo
beidson: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.