Bug 129314 – JIT Engines use the wrong stack limit for stack checks

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

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.

[Michael Saboff]

Created attachment 225158 [details]
Patch

[Michael Saboff]

Committed r164653: <http://trac.webkit.org/changeset/164653>

[Mark Lam]

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.

[Geoffrey Garen]

Yeah, this looks wrong. What bug does this fix?

[Michael Saboff]

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

[Michael Saboff]

(In reply to comment #5)
> (In reply to comment #4)
> ....   Not the setting and use is consistent.

Now the setting and use are consistent.

[Geoffrey Garen]

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.

[Mark Lam]

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

[Michael Saboff]

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