Bug 125849 – Change JSStack::topOfStack() (and peers) to point to the last slot instead of past it.

• Component: JavaScriptCore
• Status: ASSIGNED

[Mark Lam]

Currently, the JSStack assumes a stack pointer (topOfStack) that points past the end of the top CallFrame.  This is not consistent with the C stack convention of the stack pointer pointing to the last allocated slot on the stack i.e. last slot in the top frame.  Will change the JSStack to match the C stack convention.  This has a side effect of simplifying code that interact with the stack (whether we use the C stack of a separate stack for the JSStack).

[Mark Lam]

Created attachment 219408 [details]
the patch.

[EFL EWS Bot]

Comment on attachment 219408 [details]
the patch.

Attachment 219408 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/44318280

[Filip Pizlo]

Comment on attachment 219408 [details]
the patch.

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3389
> +size_t CodeBlock::frameExtentSizeInRegisters()

Does this return exactly The same thing as frameRegisterCount?

> Source/JavaScriptCore/interpreter/JSStackInlines.h:71
> +        neededRegisters += codeBlock->frameExtentSizeInRegisters();

Isn't this wrong for DFG code?

[Mark Lam]

Comment on attachment 219408 [details]
the patch.

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

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3389
>> +size_t CodeBlock::frameExtentSizeInRegisters()
> 
> Does this return exactly The same thing as frameRegisterCount?

Yes.  Exactly the same.  I would prefer to return frameRegisterCount(), but I’ve heard a differing opinion that we should route it through virtualRegisterForLocal() for the reason that if we change the layout of locals (e.g. pag with something else that we don’t consider locals), then virtualRegisterForLocal() will encapsulate that change.  I would argue that if we were to add some other fields besides locals, we should update the frameRegisterCount() as well, or encapsulate it here in this CodeBlock function.  Any preferences given the options?  I’d be happy to change it to return frameRegisterCount() directly (which I consider to be more intuitive).

>> Source/JavaScriptCore/interpreter/JSStackInlines.h:71
>> +        neededRegisters += codeBlock->frameExtentSizeInRegisters();
> 
> Isn't this wrong for DFG code?

How so?  CodeBlock::frameExtentSizeInRegisters() is computed from CodeBlock::frameRegisterCount(), which in turn is computed from jitCode()->dfgCommon()->frameRegisterCount if jitType() is DFGJIT.  Or are you referring to something else?

[Mark Lam]

(In reply to comment #2)
> (From update of attachment 219408 [details])
> Attachment 219408 [details] did not pass efl-wk2-ews (efl-wk2):
> Output: http://webkit-queues.appspot.com/results/44318280

This efl-wk2-ews failure is due to an internal compiler error on the bot.  It’s not an issue with the patch.

[Filip Pizlo]

(In reply to comment #4)
> (From update of attachment 219408 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219408&action=review
> 
> >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3389
> >> +size_t CodeBlock::frameExtentSizeInRegisters()
> > 
> > Does this return exactly The same thing as frameRegisterCount?
> 
> Yes.  Exactly the same.  I would prefer to return frameRegisterCount(), but I’ve heard a differing opinion that we should route it through virtualRegisterForLocal() for the reason that if we change the layout of locals (e.g. pag with something else that we don’t consider locals), then virtualRegisterForLocal() will encapsulate that change.

OK, got it.  In which case this change is an obvious violation of YAGNI and is based on false premises.

1) We have no plans to pad locals.

2) Even if we had such plans, they would be very far off and so having a bunch of crufty code where you've got two methods that return the same exact value is just going to be hurting the project.  We don't land refactorings that make code more complex in order to support hypothetical far-off-in-the-future functionality.

3) Even if we padded locals, we would absolutely change frameRegisterCount() to account for that padding.  That is the meaning of frameRegisterCount().  It has nothing to do with the number of locals.  It is the amount of the stack that a code block uses.  A code block's stack usage is measured from wherever FP points.

So, please, lets not do this.

> I would argue that if we were to add some other fields besides locals, we should update the frameRegisterCount() as well

Yes.  We would obviously do that.  Anything else would be insane.

> , or encapsulate it here in this CodeBlock function.  Any preferences given the options?  I’d be happy to change it to return frameRegisterCount() directly (which I consider to be more intuitive).

Yes, please call it directly.

Sorry that I didn't catch this in our previous discussions.

> 
> >> Source/JavaScriptCore/interpreter/JSStackInlines.h:71
> >> +        neededRegisters += codeBlock->frameExtentSizeInRegisters();
> > 
> > Isn't this wrong for DFG code?
> 
> How so?  CodeBlock::frameExtentSizeInRegisters() is computed from CodeBlock::frameRegisterCount(), which in turn is computed from jitCode()->dfgCommon()->frameRegisterCount if jitType() is DFGJIT.  Or are you referring to something else?

Aren't you doing a stack check?

There are at least two notions of frame register count:

A) The amount you need to check.

B) The place where you put SP.

In DFG, (A) may be greater than (B).

frameRegisterCount() returns the second one (where to put SP).  Therefore, it's not appropriate for stack checks.

As an aside, why does this do a stack check?  Maybe the callee should always do the stack check and then you won't have this problem?

[Mark Lam]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 219408 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=219408&action=review
> > 
> > >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3389
> > >> +size_t CodeBlock::frameExtentSizeInRegisters()
> > > 
> > > Does this return exactly The same thing as frameRegisterCount?
> > 
> > Yes.  Exactly the same.  ...
> 
> Yes, please call it directly.

Just to make sure I’m understanding you correctly, we’ll still have a CodeBlock::frameExtentSizeInRegisters().  For this patch, it returns CodeBlock::frameRegisterCount().  But shortly thereafter, I will be changing it to factor in maxFrameExtentForSlowPathCall.


> > >> Source/JavaScriptCore/interpreter/JSStackInlines.h:71
> > >> +        neededRegisters += codeBlock->frameExtentSizeInRegisters();
> > > 
> > > Isn't this wrong for DFG code?
> > 
> > How so?  CodeBlock::frameExtentSizeInRegisters() is computed from CodeBlock::frameRegisterCount(), which in turn is computed from jitCode()->dfgCommon()->frameRegisterCount if jitType() is DFGJIT.  Or are you referring to something else?
> 
> Aren't you doing a stack check?
> 
> There are at least two notions of frame register count:
> 
> A) The amount you need to check.
> 
> B) The place where you put SP.
> 
> In DFG, (A) may be greater than (B).
> 
> frameRegisterCount() returns the second one (where to put SP).  Therefore, it's not appropriate for stack checks.

Ok, this is interesting.  Why is it not OK to set the SP to the same value as the first one (the worst case)?  Is it for space usage efficiency, or is there some other reason?

> As an aside, why does this do a stack check?  Maybe the callee should always do the stack check and then you won't have this problem?

My understanding is that this entryCheck() simplifies the work for the callToJavaScript() thunk.  It ensures that we have enough stack capacity so that callToJavaScript() is guaranteed to be successful and need not do its own checks.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (From update of attachment 219408 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=219408&action=review
> > > 
> > > >> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3389
> > > >> +size_t CodeBlock::frameExtentSizeInRegisters()
> > > > 
> > > > Does this return exactly The same thing as frameRegisterCount?
> > > 
> > > Yes.  Exactly the same.  ...
> > 
> > Yes, please call it directly.
> 
> Just to make sure I’m understanding you correctly, we’ll still have a CodeBlock::frameExtentSizeInRegisters().  For this patch, it returns CodeBlock::frameRegisterCount().  But shortly thereafter, I will be changing it to factor in maxFrameExtentForSlowPathCall.

The *entire point* of frameRegisterCount() is that it should already include maxFrameExtentForSlowPathCall.

Also, you're fooling yourself if you think that you can compute maxFrameExtentForSlowPathCall for all of our engines.  You certainly can't compute it for the FTL.

maxFrameExtentForSlowPathCall cannot be part of CodeBlock and cannot be added by CodeBlock methods to any other value because CodeBlock must be able to encapsulate things for which maxFrameExtentForSlowPathCall is not defined (like FTL).

maxFrameExtentForSlowPathCall should be part of the JIT's and DFG's calculation of frameRegisterCount().  I think we've now made this point multiple times.  The *entire point* of frameRegisterCount() is to encapsulate maxFrameExtentForSlowPathCall for those engines where such a thing is well-defined (namely JIT and DFG).

So please, please, please don't introduce yet another method for the purpose of adding maxFrameExtentForSlowPathCall to frameRegisterCount.  The *whole point* of adding a thing called "frameRegisterCount" was to make a placeholder for encapsulating maxFrameExtentForSlowPathCall.  If you then start adding yet another thing, with yet another name, to encapsulate maxFrameExtentForSlowPathCall then you're just making the code unnecessarily complex.

That, and what you're doing is *just plain wrong*.  There is no such thing as maxFrameExtentForSlowPathCall for the FTL and there never will be.  CodeBlocks may encapsulate FTL compilations.  Therefore, CodeBlock should not refer to maxFrameExtentForSlowPathCall.  Any mention of maxFrameExtentForSlowPathCall anywhere in CodeBlock.h or CodeBlock.cpp will smell fishy.

> 
> 
> > > >> Source/JavaScriptCore/interpreter/JSStackInlines.h:71
> > > >> +        neededRegisters += codeBlock->frameExtentSizeInRegisters();
> > > > 
> > > > Isn't this wrong for DFG code?
> > > 
> > > How so?  CodeBlock::frameExtentSizeInRegisters() is computed from CodeBlock::frameRegisterCount(), which in turn is computed from jitCode()->dfgCommon()->frameRegisterCount if jitType() is DFGJIT.  Or are you referring to something else?
> > 
> > Aren't you doing a stack check?
> > 
> > There are at least two notions of frame register count:
> > 
> > A) The amount you need to check.
> > 
> > B) The place where you put SP.
> > 
> > In DFG, (A) may be greater than (B).
> > 
> > frameRegisterCount() returns the second one (where to put SP).  Therefore, it's not appropriate for stack checks.
> 
> Ok, this is interesting.  Why is it not OK to set the SP to the same value as the first one (the worst case)?  Is it for space usage efficiency, or is there some other reason?

Because that would be inefficient. An optimizing compiler should set SP once - at the top of the prologue - and never change it. It should make calls from wherever it had set SP in the prologue. If it sets SP to some pessimistically large value then you're wasting stack space, leading to degraded cache locality and higher likelihood of conservative GC pathologies.

> 
> > As an aside, why does this do a stack check?  Maybe the callee should always do the stack check and then you won't have this problem?
> 
> My understanding is that this entryCheck() simplifies the work for the callToJavaScript() thunk.  It ensures that we have enough stack capacity so that callToJavaScript() is guaranteed to be successful and need not do its own checks.

Then why can't it just do the minimum amount of checking needed to prove that callToJavaScript() won't access garbage memory?  I don't understand why it has to know anything about the callee codeBlock's stack usage.

[Mark Lam]

(In reply to comment #8)
> The *entire point* of frameRegisterCount() is that it should already include maxFrameExtentForSlowPathCall.

*This* is the piece of detail I was lacking, and I was getting differing info from other discussions with different folks.  I’m proceed with this being the plan.


> > > frameRegisterCount() returns the second one (where to put SP).  Therefore, it's not appropriate for stack checks.
> > 
> > Ok, this is interesting.  Why is it not OK to set the SP to the same value as the first one (the worst case)?  Is it for space usage efficiency, or is there some other reason?
> 
> Because that would be inefficient. An optimizing compiler should set SP once - at the top of the prologue - and never change it. It should make calls from wherever it had set SP in the prologue. If it sets SP to some pessimistically large value then you're wasting stack space, leading to degraded cache locality and higher likelihood of conservative GC pathologies.

I thought as much but didn’t want to assume just in case there are other reasons (maybe DFG implementation idiosyncrasies).  Good to know that there is no non-obvious reasons.


> > > As an aside, why does this do a stack check?  Maybe the callee should always do the stack check and then you won't have this problem?
> > 
> > My understanding is that this entryCheck() simplifies the work for the callToJavaScript() thunk.  It ensures that we have enough stack capacity so that callToJavaScript() is guaranteed to be successful and need not do its own checks.
> 
> Then why can't it just do the minimum amount of checking needed to prove that callToJavaScript() won't access garbage memory?  I don't understand why it has to know anything about the callee codeBlock's stack usage.

I was just implementing the equivalence of the pre-existing code.  I will dig deeper and see if I can rework this code.

[Geoffrey Garen]

> > Then why can't it just do the minimum amount of checking needed to prove that callToJavaScript() won't access garbage memory?  I don't understand why it has to know anything about the callee codeBlock's stack usage.

> I was just implementing the equivalence of the pre-existing code.  I will dig deeper and see if I can rework this code.

Currently, only function code emits a stack check. If we want to remove the stack check from callToJavaScript (and I think we do), then we need to add stack checks to program and eval code. This is an easy refactoring, and I think we should do it.

We don't need to do a stack check for the callToJavaScript frame. It's a simple stack frame with a fixed size. If calling callToJavaScript overflows the stack, we have already lost, because calling any other C/C++ function would have done the same.

[Mark Lam]

Because it has expressed that fixing this bug all in one shot results in a patch that is difficult to follow, I will proceed with the fix in small incremental fixes to make it easier for the reviewer.  The work starts now.