NEW 142534
[Win64] Not all parameters are set up for slow path call. 
https://bugs.webkit.org/show_bug.cgi?id=142534
Summary [Win64] Not all parameters are set up for slow path call.
peavo
Reported 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).
Attachments
Patch (3.48 KB, patch)
2015-03-10 09:38 PDT, peavo 		no flags
DetailsFormatted DiffDiff
Patch (3.56 KB, patch)
2015-03-10 09:54 PDT, peavo 		no flags
DetailsFormatted DiffDiff
Patch (9.54 KB, patch)
2015-03-16 12:24 PDT, peavo 		no flags
DetailsFormatted DiffDiff
Patch (3.65 KB, patch)
2015-03-16 12:35 PDT, peavo 		beidson: review-
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
peavo
Comment 1 2015-03-10 09:38:18 PDT
Created attachment 248332 [details] Patch
peavo
Comment 2 2015-03-10 09:54:02 PDT
Created attachment 248334 [details] Patch
Geoffrey Garen
Comment 3 2015-03-10 11:36:30 PDT
Is this covered by some kind of regression test?
peavo
Comment 4 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.
Geoffrey Garen
Comment 5 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.
peavo
Comment 6 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 :)
Alex Christensen
Comment 7 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 :(
Csaba Osztrogonác
Comment 8 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.
Csaba Osztrogonác
Comment 9 2015-03-11 01:05:38 PDT
And what aboud the Apple Windows bots, aren't they 64 bit bots?
peavo
Comment 10 2015-03-16 12:24:25 PDT
Created attachment 248742 [details] Patch
peavo
Comment 11 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.
peavo
Comment 12 2015-03-16 12:35:24 PDT
Created attachment 248743 [details] Patch
peavo
Comment 13 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.
Alex Christensen
Comment 14 2015-06-15 12:13:22 PDT
Is this still needed?
peavo
Comment 15 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.
Brent Fulgham
Comment 16 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!
Brady Eidson
Comment 17 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.
Note You need to log in before you can comment on or make changes to this bug.