Bug 129314 - JIT Engines use the wrong stack limit for stack checks
Summary: JIT Engines use the wrong stack limit for stack checks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-25 09:36 PST by Michael Saboff
Modified: 2014-02-25 10:50 PST (History)
2 users (show)

See Also:


Attachments
Patch (4.78 KB, patch)
2014-02-25 09:38 PST, Michael Saboff
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-02-25 09:36:05 PST
The VM has a few stack limit values, m_stackLimit for assembly LLInt, baseline and DFG limit checks, m_jsStackLimit for C Loop LLInt checks and m_ftlStackLimit for FTL limit checks.  When we compile using the assembly LLInt, m_stackLimit and m_jsStackLimit are declared in a union and therefore are aliases.  When compiling with the C Loop LLInt, they are separate values.  The LLInt, baseline and DFG all refer to m_jsStackLimit.  This is fine for the LLInt as it's behavior will change depending on how it is compiled while the baseline and DFG engines should always use m_stackLimit.
Comment 1 Michael Saboff 2014-02-25 09:38:12 PST
Created attachment 225158 [details]
Patch
Comment 2 Michael Saboff 2014-02-25 09:48:15 PST
Committed r164653: <http://trac.webkit.org/changeset/164653>
Comment 3 Mark Lam 2014-02-25 10:12:45 PST
Comment on attachment 225158 [details]
Patch

Why is this an issue at all?  m_jsStackLimit and m_stackLimit should be the same field for JIT builds because they are in a union.  Are the compilers actually storing them in different fields?  My concern is that your fix is not actually doing what you think it is.
Comment 4 Geoffrey Garen 2014-02-25 10:22:27 PST
Yeah, this looks wrong. What bug does this fix?
Comment 5 Michael Saboff 2014-02-25 10:28:29 PST
(In reply to comment #4)
> Yeah, this looks wrong. What bug does this fix?

The doesn't change correctness, in fact it shouldn't change anything at the machine code level.

This is primarily a fix for clarity / readability and eliminating future problems if we change it so that m_stackLimit and m_jsStackLimit are not aliases.  We set m_stackLimit for ASM LLInt + JIT + DFG builds, but we check using m_jsStackLimit.  Not the setting and use is consistent.
Comment 6 Michael Saboff 2014-02-25 10:33:20 PST
(In reply to comment #5)
> (In reply to comment #4)
> ....   Not the setting and use is consistent.

Now the setting and use are consistent.
Comment 7 Geoffrey Garen 2014-02-25 10:35:03 PST
I think this patch reduced future flexibility instead of increasing it because it introduced a hard dependency that JIT code must run on the machine stack. The point of m_jsStackLimit was to abstract out where the JS stack lived. All JS code checks m_jsStackLimit, and it's the VM that decides where m_jsStackLimit points.

In other words, if we change it so that m_stackLimit and m_jsStackLimit are not aliases, it will be a bug that the JIT checks m_stackLimit instead of m_jsStackLimit.
Comment 8 Mark Lam 2014-02-25 10:37:40 PST
(In reply to comment #7)
> I think this patch reduced future flexibility instead of increasing it because it introduced a hard dependency that JIT code must run on the machine stack. The point of m_jsStackLimit was to abstract out where the JS stack lived. All JS code checks m_jsStackLimit, and it's the VM that decides where m_jsStackLimit points.
> 
> In other words, if we change it so that m_stackLimit and m_jsStackLimit are not aliases, it will be a bug that the JIT checks m_stackLimit instead of m_jsStackLimit.

I agree.  Note: the LLINT still checks m_jsStackLimit by necessity (for C loop LLINT compatibility).  Hence, changing only the JIT side to use m_stackLimit is no clearer than it was before.
Comment 9 Michael Saboff 2014-02-25 10:50:40 PST
(In reply to comment #7)
> I think this patch reduced future flexibility instead of increasing it because it introduced a hard dependency that JIT code must run on the machine stack. The point of m_jsStackLimit was to abstract out where the JS stack lived. All JS code checks m_jsStackLimit, and it's the VM that decides where m_jsStackLimit points.

We only set m_jsStackLimit when compiling for the C Loop.  The rest of the code sets m_stackLimit. 
If we roll out this patch then we should also change all the occurrences of setting m_stackLimit to set m_sjStackLimit.

I believe there are other places the code would need to change in order to use a stack besides the machine stack.

> In other words, if we change it so that m_stackLimit and m_jsStackLimit are not aliases, it will be a bug that the JIT checks m_stackLimit instead of m_jsStackLimit.

Concerning the LLInt using m_jsStackLimit, that was covered in the description.  It needs to switch based on how it is built.