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

RESOLVED FIXED

Status

()

Core
Rewriting and Analysis
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: (dormant account), Assigned: (dormant account))

Tracking

(Blocks: 1 bug)

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

9 years ago
 
(Assignee)

Comment 1

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

Comment 2

9 years ago
(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;
}
(Assignee)

Comment 3

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

Comment 4

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

Updated

9 years ago
Depends on: 441362
(Assignee)

Updated

9 years ago
Depends on: 442358
(Assignee)

Comment 5

9 years ago
Created attachment 327464 [details] [diff] [review]
Initial attempt at integration into moz

Would like to commit something like this in the near future
Assignee: nobody → tglek
Status: NEW → ASSIGNED
(Assignee)

Comment 6

9 years ago
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=...
Attachment #327464 - Attachment is obsolete: true
(Assignee)

Comment 7

9 years ago
Created attachment 327721 [details] [diff] [review]
added missing file
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])
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.
(Assignee)

Updated

9 years ago
Blocks: 443286
(Assignee)

Updated

9 years ago
No longer blocks: 443286
Depends on: 443286
(Assignee)

Comment 10

9 years ago
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.
Attachment #327721 - Attachment is obsolete: true
Attachment #327893 - Flags: review?(igor)

Comment 11

9 years ago
(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.

(Assignee)

Comment 12

9 years ago
Created attachment 327988 [details] [diff] [review]
SM annotations + header 1/2
Attachment #327893 - Attachment is obsolete: true
Attachment #327988 - Flags: review?(igor)
Attachment #327893 - Flags: review?(igor)
(Assignee)

Comment 13

9 years ago
Created attachment 327989 [details] [diff] [review]
analysis script integration + 1 fix
Attachment #327989 - Flags: review?(igor)

Comment 14

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

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

Updated

9 years ago
Attachment #327988 - Flags: superreview?
Attachment #327988 - Flags: review?(benjamin)
(Assignee)

Updated

9 years ago
Attachment #327988 - Flags: superreview?
(Assignee)

Updated

9 years ago
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 17

9 years ago
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 18

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

Comment 20

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

Comment 21

9 years ago
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.
Attachment #327988 - Attachment is obsolete: true
Attachment #337124 - Flags: review?(igor)
Attachment #327988 - Flags: review?(igor)
(Assignee)

Comment 22

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

Updated

9 years ago
Attachment #337124 - Flags: review?(igor) → review+

Comment 23

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

Comment 24

9 years ago
pushed patch 1, rev 9aa9d1a57edd
(Assignee)

Comment 25

9 years ago
pushed patch 2, rev f574044a8f49

Redundant comments were removed in patch 1.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.