Closed Bug 771378 Opened 9 years ago Closed 9 years ago

Remove need for MUST_FLOW static analysis from SpiderMonkey

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

(Whiteboard: [js:t])

Attachments

(4 files)

jcranmer is working on getting static analysis working again:

https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.platform/qURgwb2XxeA

He mentioned that SpiderMonkey has only two uses of the MUST_FLOW analysis.  Let's replace them with RAII.
<http://mxr.mozilla.org/mozilla-central/ident?i=MUST_FLOW_THROUGH> lists all of them (BytecodeEmitter's EmitSwitch and E4X's PutProperty, plus some tests that don't matter).
This patch adds ScopedFreePtr, to match ScopedDeletePtr.
Attachment #639543 - Flags: review?(jwalden+bmo)
This patch removes the MUST_FLOW_THROUGH from EmitSwitch().  It's an awkward
case at first glance, because the MUST_FLOW_THROUGH occurs within an else 
branch, one level of nesting deeper than the target |out| label.

Beneath the |out| label two things happen:

- |table| is freed.  We can use ScopedFreePtr instead to guarantee this.  
  (This calls Foreground::free_(), which probably is good enough for this
  case.)

- If |ok| is true, some extra stuff is done.  Now, we only ever leave this
  function early if something goes wrong (i.e. returning JS_FALSE).  So it's
  ok to remove the |out| label and just have control flow fall through here 
  naturally in the normal (non-failing) case.

Finally, the |bad| case, because it sets |ok| to JS_FALSE, only needs to go
through |out| in order to free table.  So the |goto bad|s can be replaced 
with |return JS_FALSE|.
Attachment #639545 - Flags: review?(jwalden+bmo)
This patch removes js_EnterLocalRootScope(), js_LeaveLocalRootScope(), and
js_LeaveLocalRootScopeWithResult(), because they're all no-ops.  (Luke says
they were there "to support slow natives in a multi-threaded
many-global-per-compartment doubles-on-the-heap world". :)

With them gone, there's no need for the MUST_FLOW_THROUGH("out") in 
PutProperty().  I didn't bother rearranging PutProperty() beyond that, 
because jsxml.cpp is on death row.
Attachment #639546 - Flags: review?(jwalden+bmo)
Now we can actually remove MUST_FLOW_*.
Attachment #639547 - Flags: review?(jwalden+bmo)
Attachment #639543 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 639545 [details] [diff] [review]
(part 1) - Remove use of MUST_FLOW_THROUGH from BytecodeEmitter.cpp.

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

This function's big enough, as I recall, that nobody's going to think their change is big enough to change it over.  Your change is at least plausibly close, so maybe switch it over now so we don't have to stare at this for years?

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2416,5 @@
>          if (switchOp == JSOP_CONDSWITCH && !pn3->isKind(PNK_DEFAULT))
>              SetJumpOffsetAt(bce, pn3->pn_offset);
>          pn4 = pn3->pn_right;
> +        if (!EmitTree(cx, bce, pn4))
> +            return JS_FALSE;

false

@@ +2444,5 @@
>  
>      /* Set the SRC_SWITCH note's offset operand to tell end of switch. */
>      off = bce->offset() - top;
> +    if (!SetSrcNoteOffset(cx, bce, (unsigned)noteIndex, 0, off))
> +        return JS_FALSE;

false

@@ +2465,5 @@
>          for (pn3 = pn2->pn_head; pn3; pn3 = pn3->pn_next) {
>              if (pn3->isKind(PNK_DEFAULT))
>                  continue;
>              if (!bce->constList.append(*pn3->pn_pval))
> +                return JS_FALSE;

false

@@ +2476,5 @@
>          }
>      }
>  
> +    if (!PopStatementBCE(cx, bce))
> +        return JS_FALSE;

false

@@ +2485,2 @@
>  #endif
> +    return JS_TRUE;

true
Attachment #639545 - Flags: review?(jwalden+bmo) → review+
Attachment #639546 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 639547 [details] [diff] [review]
(part 3) - Remove MUST_FLOW_THROUGH and related things.

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

\o/
Attachment #639547 - Flags: review?(jwalden+bmo) → review+
> This function's big enough, as I recall, that nobody's going to think their
> change is big enough to change it over.  Your change is at least plausibly
> close, so maybe switch it over now so we don't have to stare at this for
> years?

I'm happy to, if you explain what you mean by "change it over" :)  Change the return value from JSBool to bool, maybe?
Comment on attachment 639543 [details] [diff] [review]
(part 0) - Add ScopedFreePtr to Utility.h.

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

You know there's AutoReleasePtr, right?
> You know there's AutoReleasePtr, right?

Sigh.  How did we end up with a mish-mash of Auto* and Scoped* classes?
(In reply to Nicholas Nethercote [:njn] from comment #8)
> I'm happy to, if you explain what you mean by "change it over" :)  Change
> the return value from JSBool to bool, maybe?

That plus changing all the return JS_TRUE/JS_FALSE to return true/false.
A trivial follow-up to avoid an uninitialized variable in jsxml.cpp:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a711846e73d
You need to log in before you can comment on or make changes to this bug.