Closed Bug 432917 Opened 12 years ago Closed 12 years ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect)

x86
All
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

Attachments

(4 files, 5 obsolete files)

 
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. 
Depends on: 441362
Depends on: 442358
Would like to commit something like this in the near future
Assignee: nobody → tglek
Status: NEW → ASSIGNED
Attached patch complete patch (obsolete) — Splinter Review
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=...
Attachment #327464 - Attachment is obsolete: true
Attached patch added missing file (obsolete) — Splinter Review
Attachment #327717 - Attachment is obsolete: true
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.
Blocks: 443286
No longer blocks: 443286
Depends on: 443286
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #327721 - Attachment is obsolete: true
Attachment #327893 - Flags: review?(igor)
(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.

Attached patch SM annotations + header 1/2 (obsolete) — Splinter Review
Attachment #327893 - Attachment is obsolete: true
Attachment #327988 - Flags: review?(igor)
Attachment #327893 - Flags: review?(igor)
Attachment #327989 - Flags: review?(igor)
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.
Attachment #327989 - Flags: review?(igor) → review+
Attachment #327988 - Flags: superreview?
Attachment #327988 - Flags: review?(benjamin)
Attachment #327988 - Flags: superreview?
Attachment #327989 - Flags: review?(benjamin)
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.
Attachment #327988 - Flags: review?(benjamin) → review+
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 ;-)
Attachment #327989 - Flags: review?(benjamin) → review+
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.
Depends on: 433939
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.
Attachment #327988 - Attachment is obsolete: true
Attachment #337124 - Flags: review?(igor)
Attachment #327988 - Flags: review?(igor)
Removed stuff that was no longer needed from this patch, otherwise same as before
Attachment #337124 - Flags: review?(igor) → review+
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.