Last Comment Bug 432917 - Treehydra: SpiderMonkey: replace "control flow must flow through here"
: Treehydra: SpiderMonkey: replace "control flow must flow through here"
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Rewriting and Analysis (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: (dormant account)
:
: Michael Layzell [:mystor] [:mrl]
Mentors:
Depends on: 433939 441362 442358 443286
Blocks: analyses
  Show dependency treegraph
 
Reported: 2008-05-08 16:52 PDT by (dormant account)
Modified: 2008-09-08 11:03 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial attempt at integration into moz (17.60 KB, patch)
2008-06-30 13:25 PDT, (dormant account)
no flags Details | Diff | Splinter Review
complete patch (17.57 KB, patch)
2008-07-01 16:37 PDT, (dormant account)
no flags Details | Diff | Splinter Review
added missing file (19.85 KB, patch)
2008-07-01 16:52 PDT, (dormant account)
no flags Details | Diff | Splinter Review
Perf improvement for ESP (2.07 KB, patch)
2008-07-01 18:51 PDT, David Mandelin [:dmandelin]
no flags Details | Diff | Splinter Review
updated patch (19.93 KB, patch)
2008-07-02 17:19 PDT, (dormant account)
no flags Details | Diff | Splinter Review
SM annotations + header 1/2 (12.78 KB, patch)
2008-07-03 10:27 PDT, (dormant account)
benjamin: review+
Details | Diff | Splinter Review
analysis script integration + 1 fix (7.44 KB, patch)
2008-07-03 10:28 PDT, (dormant account)
igor: review+
benjamin: review+
Details | Diff | Splinter Review
SM annotations + header 1/2 (14.15 KB, patch)
2008-09-05 13:59 PDT, (dormant account)
igor: review+
Details | Diff | Splinter Review
analysis script integration 2/2 (6.68 KB, patch)
2008-09-05 14:02 PDT, (dormant account)
no flags Details | Diff | Splinter Review

Description (dormant account) 2008-05-08 16:52:26 PDT
 
Comment 1 (dormant account) 2008-06-11 10:29:17 PDT
Igor do you still want this, given that we have push/pop stuff?
Comment 2 Igor Bukanov 2008-06-11 11:53:10 PDT
(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) 2008-06-13 11:02:02 PDT
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) 2008-06-13 11:27:51 PDT
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) 2008-06-30 13:25:11 PDT
Created attachment 327464 [details] [diff] [review]
Initial attempt at integration into moz

Would like to commit something like this in the near future
Comment 6 (dormant account) 2008-07-01 16:37:56 PDT
Created attachment 327717 [details] [diff] [review]
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) 2008-07-01 16:52:22 PDT
Created attachment 327721 [details] [diff] [review]
added missing file
Comment 8 David Mandelin [:dmandelin] 2008-07-01 17:09:10 PDT
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] 2008-07-01 18:51:56 PDT
Created attachment 327727 [details] [diff] [review]
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) 2008-07-02 17:19:57 PDT
Created attachment 327893 [details] [diff] [review]
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 2008-07-02 18:06:55 PDT
(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) 2008-07-03 10:27:22 PDT
Created attachment 327988 [details] [diff] [review]
SM annotations + header 1/2
Comment 13 (dormant account) 2008-07-03 10:28:24 PDT
Created attachment 327989 [details] [diff] [review]
analysis script integration + 1 fix
Comment 14 Igor Bukanov 2008-07-03 10:46:59 PDT
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 2008-07-03 11:20:19 PDT
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] 2008-07-08 13:40:46 PDT
ESP perf improvements pushed to Treehydra. I verified both aspects were needed to get acceptable perf on this application.
Comment 17 Benjamin Smedberg [:bsmedberg] 2008-07-08 14:04:01 PDT
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 [:bsmedberg] 2008-07-08 14:09:09 PDT
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] 2008-07-08 14:20:46 PDT
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) 2008-07-08 17:21:14 PDT
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) 2008-09-05 13:59:19 PDT
Created attachment 337124 [details] [diff] [review]
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) 2008-09-05 14:02:02 PDT
Created attachment 337125 [details] [diff] [review]
analysis script integration 2/2

Removed stuff that was no longer needed from this patch, otherwise same as before
Comment 23 Igor Bukanov 2008-09-06 00:22:31 PDT
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) 2008-09-08 10:51:50 PDT
pushed patch 1, rev 9aa9d1a57edd
Comment 25 (dormant account) 2008-09-08 11:03:02 PDT
pushed patch 2, rev f574044a8f49

Redundant comments were removed in patch 1.

Note You need to log in before you can comment on or make changes to this bug.