Bug 153186 - FTL doesn't do proper spilling for exception handling when GetById/Snippets go to slow path
Summary: FTL doesn't do proper spilling for exception handling when GetById/Snippets g...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-16 11:04 PST by Saam Barati
Modified: 2016-01-18 14:16 PST (History)
10 users (show)

See Also:


Attachments
patch (10.42 KB, patch)
2016-01-16 11:28 PST, Saam Barati
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-01-16 11:04:15 PST
Michael was investigating a bug he found while doing the new JSC calling convention work
and it turns out to be a latent bug in FTL try/catch machinery.
After I looked at the code again, I realized that what I had previously
written is wrong in a subtle way. The FTL callOperation machinery will remove
its result register from the set of registers it needs to spill. This is not
correct when we have try/catch. We may want to do value recovery on the value
of the result prior to the call. The case that we were solving before was
when the resultRegister == baseRegister in a GetById, or left/rightRegister == resultRegister
in a Snippet. This code is correct in wanting to spill in that case, but that case is
just a partial subset of the rule saying we should spill anytime the result is a register
we might do value recovery on.
Comment 1 Saam Barati 2016-01-16 11:28:00 PST
Created attachment 269176 [details]
patch
Comment 2 Saam Barati 2016-01-16 11:33:37 PST
Comment on attachment 269176 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269176&action=review

> Source/JavaScriptCore/ftl/FTLCompile.cpp:666
> +                // on it in the OSR exit. This is because the callOperation(.) machinery doesn't
> +                // ever spill the result value. It actually takes care to never spill the result
> +                // because it overwrites it with the result of the call. But, with exceptions and
> +                // OSR exits, we may need the result value during OSR exit, so we take care to spill
> +                // it now.

changed this locally to:
                // We take care to always spill the result whenever we need to do value recovery
                // on it in the OSR exit. This is because the callOperation(.) machinery doesn't
                // ever spill the result value. It actually takes care to never spill the result
                // because it overwrites it with the result of the call. But, with exceptions and
                // OSR exits, we may need the result value prior to the call during OSR exit.
                // We take care to mark it for spillage now.
Comment 3 Michael Saboff 2016-01-16 17:16:19 PST
Comment on attachment 269176 [details]
patch

r=me
Comment 4 Saam Barati 2016-01-18 14:16:01 PST
landed in:
http://trac.webkit.org/changeset/195238