771378 - Remove need for MUST_FLOW static analysis from SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Joshua Cranmer [:jcranmer]]

<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).

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 0) - Add ScopedFreePtr to Utility.h.

This patch adds ScopedFreePtr, to match ScopedDeletePtr.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove use of MUST_FLOW_THROUGH from BytecodeEmitter.cpp.

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|.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove use of MUST_FLOW_THROUGH from jsxml.cpp.

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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove MUST_FLOW_THROUGH and related things.

Now we can actually remove MUST_FLOW_*.

Comment 6

[Jeff Walden [:Waldo]]

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

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 639547 [details] [diff] [review]
(part 3) - Remove MUST_FLOW_THROUGH and related things.

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

\o/

Comment 8

[Nicholas Nethercote [inactive]]

> 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 9

[Nicholas Nethercote [inactive]]

> I'm happy to, if you explain what you mean by "change it over" :)  Change
> the return value from JSBool to bool, maybe?

Silence implies consent! :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcba8ae674df
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d030947dffe
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b273f6a04af
https://hg.mozilla.org/integration/mozilla-inbound/rev/02416efff4af

Comment 10

[:Benjamin Peterson]

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?

Comment 11

[Nicholas Nethercote [inactive]]

> You know there's AutoReleasePtr, right?

Sigh.  How did we end up with a mish-mash of Auto* and Scoped* classes?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fix it!

Comment 13

[Jeff Walden [:Waldo]]

(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.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fcba8ae674df
https://hg.mozilla.org/mozilla-central/rev/6d030947dffe
https://hg.mozilla.org/mozilla-central/rev/0b273f6a04af
https://hg.mozilla.org/mozilla-central/rev/02416efff4af

Comment 15

[Nicholas Nethercote [inactive]]

A trivial follow-up to avoid an uninitialized variable in jsxml.cpp:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a711846e73d

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5a711846e73d