Remove need for MUST_FLOW static analysis from SpiderMonkey

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
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).
(Assignee)

Comment 2

5 years ago
Created attachment 639543 [details] [diff] [review]
(part 0) - Add ScopedFreePtr to Utility.h.

This patch adds ScopedFreePtr, to match ScopedDeletePtr.
Attachment #639543 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

5 years ago
Created attachment 639545 [details] [diff] [review]
(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|.
Attachment #639545 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

5 years ago
Created attachment 639546 [details] [diff] [review]
(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.
Attachment #639546 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

5 years ago
Created attachment 639547 [details] [diff] [review]
(part 3) - Remove MUST_FLOW_THROUGH and related things.

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+
(Assignee)

Comment 8

5 years ago
> 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?
(Assignee)

Comment 9

5 years ago
> 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

5 years ago
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?
(Assignee)

Comment 11

5 years ago
> You know there's AutoReleasePtr, right?

Sigh.  How did we end up with a mish-mash of Auto* and Scoped* classes?
Fix it!
(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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Assignee)

Comment 15

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

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