Bug 153186

Summary: FTL doesn't do proper spilling for exception handling when GetById/Snippets go to slow path
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fpizlo, ggaren, gskachkov, keith_miller, mark.lam, msaboff, oliver, sukolsak, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch msaboff: review+

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