Summary: | JIT Engines use the wrong stack limit for stack checks | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ggaren, mark.lam | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Michael Saboff
2014-02-25 09:36:05 PST
Created attachment 225158 [details]
Patch
Committed r164653: <http://trac.webkit.org/changeset/164653> 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.
Yeah, this looks wrong. What bug does this fix? (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. (In reply to comment #5) > (In reply to comment #4) > .... Not the setting and use is consistent. Now the setting and use are consistent. 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. (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. (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. |