432917 - Treehydra: SpiderMonkey: replace "control flow must flow through here"

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(dormant account)]

Comment 1

[(dormant account)]

Igor do you still want this, given that we have push/pop stuff?

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> Igor do you still want this, given that we have push/pop stuff?
> 

For sure: push/pop does not cover everything. It would be really nice to have something like MUST_FLOW(label_name) to replace all those comments. 

Another thing is that SM has a few of /* FALL THROUGH */ comments to indicate that the case block is really want to continue with the next one like in:

switch (a) {
  case 1:
    do_something();
    /* FALL THROUGH */
  case 2:
    do_more();
    break;
}

Ideally it would be nice to have a macro like js_fallthrough with the same semantic as the fallthrough keyword in C#: the code after the case label must either ends with control transfer like break/goto/return/continue or with explicit js_fallthrough. The exception is empty code block between consecutive case labels so one would not need to change the following switch 

switch (a) {
  case 1:
  case 2:
    common_code;
    break;
}

into

switch (a) {
  case 1:
    js_fallthrough;
  case 2:
    common_code;
    break;
}

Comment 3

[(dormant account)]

Igor,
ok found my first bug. I added a magic func
MUST_FLOW_THROUGH("label") and labeled most of the "must flow through out:" cases i found.

I'm using mozilla-central for lines and filenames are a little different:
js/src/jsemit.cpp:2582: error: EmitSwitch: control flow doesn't go through out because CHECK_AND_SET_JUMP_OFFSET_AT macro contains a return statement.

Comment 4

[(dormant account)]

Another weird one
js/src/jsopcode.cpp:1624: error:Decompile: Control did not flow through enterblock_out

cause is that LOCAL_ASSERT for some reason does a return statement. 

Comment 5

[(dormant account)]

Attachment: Initial attempt at integration into moz

Would like to commit something like this in the near future

Comment 6

[(dormant account)]

Attachment: complete patch

Could someone(Igor?) start reviewing this? 
Note: The commented out MUST_FLOW_THROUGH labels are correct code that the checker isn't smart enough to check.

I added testcases. To run, configure mozilla with --with-static-checking=...

Comment 7

[(dormant account)]

Attachment: added missing file

Comment 8

[David Mandelin [:dmandelin]]

Can you post an update that runs against the latest treehydra, with the boilerplate reduction? I'm trying to check out your perf issues but:

/home/dmandelin/gcc-dehydra/installed/libexec/gcc/i686-pc-linux-gnu/4.3.0/cc1plus -fpreprocessed -fshort-wchar -quiet -o /dev/null -fplugin=/home/dmandelin/gcc-dehydra/dehydra-gcc/gcc_treehydra.so -fplugin-arg=/home/dmandelin/sources/mozilla-xpidl/xpcom/analysis/flow.js /home/dmandelin/sources/mozilla-xpidl/xpcom/tests/static-checker/flow_through_pass.cpp
/home/dmandelin/gcc-dehydra/dehydra-gcc/libs/treehydra.js:12: JS Exception: No vbl in this lazy object
:0:	#0: Error("No vbl in this lazy object")
/home/dmandelin/gcc-dehydra/dehydra-gcc/libs/treehydra.js:12:	#1: unhandledLazyProperty("vbl")
/home/dmandelin/gcc-dehydra/dehydra-gcc/libs/unstable/esp.js:329:	#2: ([object GCCNode],[object Array],meet,0)
/home/dmandelin/sources/mozilla-xpidl/xpcom/analysis/flow.js:137:	#3: FlowCheck([object GCCNode],[object Array],0)
/home/dmandelin/sources/mozilla-xpidl/xpcom/analysis/flow.js:53:	#4: process_tree([object GCCNode])

Comment 9

[David Mandelin [:dmandelin]]

Attachment: Perf improvement for ESP

This patch to ESP seems to fix the perf problem. jsinterp.ii takes 35 seconds to analyze with this patch.

I diagnosed the problem by setting the pass_limit to 4, just on a guess that the problem was too many passes. It turned out to be right. BB31 was getting analyzed way too many times.

1. BB31 has a huge number of inedges, and got scheduled for reanalysis every time one of the incoming blocks got updated, even though the in state for BB31 only changes once. I fixed ESP to compare the in states before actually analyzing a block.

2. But it still got analyzed too many times. This time, the problem was that lots of entries like "foobar: T" were appearing in the state map. This is bad because states "foobar: T" and "" are equivalent (both represent foobar as having TOP), but don't compare as equal. There are clearly several fixes available here, but I chose to fix assignValue so that instead of putting in an explicit TOP, it deletes the entry, because this saves space, FWIW. 

With #2 in place, #1 may not actually be needed: BB31 is a small BB so analyzing it a lot of times shouldn't be that bad. And you have to trade that off against the fact that in #1, it does more state equality comparisons.

Comment 10

[(dormant account)]

Attachment: updated patch

This disabled outparams so flow.js can use the new esp api. Use this with Dave's perf patch for awesome perf.

Igor, please review I'd like to land this soon.

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> Created an attachment (id=327893) [details]
> updated patch
> 
> This disabled outparams so flow.js can use the new esp api. Use this with
> Dave's perf patch for awesome perf.
> 
> Igor, please review I'd like to land this soon.

It would be nice to split the patch and bug in 2: The first part is is just SM source annotations together with jsstaticchecks.h header without any bug fixes and the second is the script itself with fixes.

Comment 12

[(dormant account)]

Attachment: SM annotations + header 1/2

Comment 13

[(dormant account)]

Attachment: analysis script integration + 1 fix

Comment 14

[Igor Bukanov]

Comment on attachment 327988 [details] [diff] [review]
SM annotations + header 1/2

>+++ b/js/src/jsstaticcheck.h
>+#ifdef NS_STATIC_CHECKING
>+/*
>+ * Trigger a control flow check to make sure that code flows through label
>+ */
>+static __attribute__ ((unused)) void MUST_FLOW_THROUGH(const char *label) {
>+}
>+#else
>+#define MUST_FLOW_THROUGH(label) 

Nit: use ((void) 0) as the body for MUST_FLOW_THROUGH to avoid extra-semicolon issues with the empty body.

r+ with this fixed.

Comment 15

[Igor Bukanov]

Comment on attachment 327989 [details] [diff] [review]
analysis script integration + 1 fix

My r+ is very weak: I just barely grasped the idea of the script.

Comment 16

[David Mandelin [:dmandelin]]

ESP perf improvements pushed to Treehydra. I verified both aspects were needed to get acceptable perf on this application.

Comment 17

[Benjamin Smedberg]

Comment on attachment 327988 [details] [diff] [review]
SM annotations + header 1/2

not sure about the "static" on the inline function... why is it marked static? Other than that, this is fine... I didn't review any of the actual code changes, of course.

Comment 18

[Benjamin Smedberg]

Comment on attachment 327989 [details] [diff] [review]
analysis script integration + 1 fix

>--- a/config/config.mk

> TREEHYDRA_MODULES = \
>-  $(topsrcdir)/xpcom/analysis/outparams.js \

Make sure this gets reverted before checkin ;-)

Comment 19

[Brendan Eich [:brendan]]

static is required on non-inline functions to avoid multiple definition link errors. But that function MUST_FLOW_THROUGH -- should it be explicitly inline too? static inline seems best to me, but I defer to you C++ gurus.

/be

Comment 20

[(dormant account)]

It was static just to link. Changed it to inline now.

Will land with outparams.js enabled once bug 433939 is resolved.

Comment 21

[(dormant account)]

Attachment: SM annotations + header 1/2

been sitting on this patch too long, had to update it. Please re-review. The only thing that changed is addition of MUST_FLOW_LABEL(label) macro to be able to define "fake" labels for analysis when there aren't existing ones in the code.

Comment 22

[(dormant account)]

Attachment: analysis script integration 2/2

Removed stuff that was no longer needed from this patch, otherwise same as before

Comment 23

[Igor Bukanov]

Comment on attachment 337124 [details] [diff] [review]
SM annotations + header 1/2

>     /* After this, control must flow through label out: to exit. */
>+    MUST_FLOW_THROUGH("out");
>     JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr);

MUST_FLOW_THROUGH is nice and loud comment in itself. So we can remove all those "after this..." comments as they just bring extra noise. But I am fine with doing that in a separated cleanup bug.

Comment 24

[(dormant account)]

pushed patch 1, rev 9aa9d1a57edd

Comment 25

[(dormant account)]

pushed patch 2, rev f574044a8f49

Redundant comments were removed in patch 1.