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

(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; }

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.

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.

Would like to commit something like this in the near future

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

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

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.

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.

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

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

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

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

It was static just to link. Changed it to inline now. Will land with outparams.js enabled once bug 433939 is resolved.

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.

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

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.

pushed patch 1, rev 9aa9d1a57edd

pushed patch 2, rev f574044a8f49 Redundant comments were removed in patch 1.