883171 - Remove JSContext::fp() and JSContext::regs()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

cx->fp(), cx->hasfp(), cx->regs() et al return information about the topmost interpreter frame. Calling them is not safe/correct when we are running code in Ion or Baseline.

Atm the remaining calls to these functions are not causing problems (that we know of) but once bug 882111 is fixed we will be able to run JIT code without having an interpreter frame on the stack and these functions are causing NULL pointer crashes in various places.

The attached patch removes these functions and fixes their callers. We're still using cx->stack.regs() instead of cx->regs() in a few places, but only when we know it's correct and these calls will be removed very soon.

The patch adds cx->interpreterFrame() and cx->interpreterRegs(). These will assert when we're not currently running in the interpreter so they are a lot safer.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 762710 [details] [diff] [review]
Patch

Review of attachment 762710 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

::: js/src/vm/Interpreter-inl.h
@@ -252,5 @@
>  {
>      JSOp op = JSOp(*pc);
>  
>      if (op == JSOP_LENGTH) {
> -        if (IsOptimizedArguments(cx->fp(), lval.address())) {

I was going to ask "what about the baseline jit?" and ask you to JS_ASSERT(!magic) etc, but it would appear that GetPropertyOperation is only called from JSOP_GETPROP.  In that case, can you avoid the whole future hazard and question and move this function into vm/Interpreter.cpp as a static function and then have it take a StackFrame* so that it can keep IsOptimizedArguments where it is?

::: js/src/vm/Interpreter.cpp
@@ +739,5 @@
>   * Enter the new with scope using an object at sp[-1] and associate the depth
>   * of the with block with sp + stackIndex.
>   */
>  static bool
> +EnterWith(JSContext *cx, AbstractFramePtr frame, HandleValue val, uint32_t stackDepth)

Since EnterWith is banished to the interpreter, it seems fine to take a StackFrame* here and avoid adding AbstractFramePtr::pushOnScopeChain.

Comment 3

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36fb664f9101

(In reply to Luke Wagner [:luke] from comment #2)
> but it would appear that GetPropertyOperation is only
> called from JSOP_GETPROP.  In that case, can you avoid the whole future
> hazard and question and move this function into vm/Interpreter.cpp as a
> static function and then have it take a StackFrame* so that it can keep
> IsOptimizedArguments where it is?

Good catch. GetElementOperation is used by the JITs but GetPropertyOperation is not.

> Since EnterWith is banished to the interpreter, it seems fine to take a
> StackFrame* here and avoid adding AbstractFramePtr::pushOnScopeChain.

I want to add "with" support to the baseline compiler soon, to get complete op coverage. When we do that the JIT can just call EnterWith as well.

Comment 4

[Luke Wagner [:luke]]

Wow, *complete* op coverage; very nice.  What about JSOP_QNAMEPART... oh that's right ;)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36fb664f9101