Bug 142534

Summary:

[Win64] Not all parameters are set up for slow path call.

Product:

WebKit

Reporter:

peavo

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

NEW

Severity:

Normal

CC:

achristensen, beidson, bfulgham, ggaren, mark.lam, msaboff, ossy

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	beidson: review-

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