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

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #762706 - Flags: review?(luke)
Posted patch PatchSplinter Review
Attachment #762706 - Attachment is obsolete: true
Attachment #762706 - Flags: review?(luke)
Attachment #762710 - Flags: review?(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.
Attachment #762710 - Flags: review?(luke) → review+
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.
Wow, *complete* op coverage; very nice.  What about JSOP_QNAMEPART... oh that's right ;)
https://hg.mozilla.org/mozilla-central/rev/36fb664f9101
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.