I forgot to edit the log description before I submitted, this isn't a partial fix, it's a complete fix, although it is more conservative than it needs to be (an inliner could substitute and propagate expressions if it did better dataflow analysis). It can be made more optimal later.
-Ray On Mon, Dec 6, 2010 at 2:52 PM, <[email protected]> wrote: > Revision: 9362 > Author: [email protected] > Date: Mon Dec 6 14:51:53 2010 > Log: Partial fix for > http://code.google.com/p/google-web-toolkit/issues/detail?id=5707 > > Modify JsInliner to avoid propagating expressions incorrectly. > > in EvaluationOrderVisitor, two changes were made. First of all, a clinit() > call > is treated as violating order, the same as any other invocation. > > Secondly, a JsNameRef that can be affected by side effects (a global, > non-param, > non-local) that is visited when there are still expressions that need > evaluation > first, is treated as violating order. > > This fixes the showcase issue, but is still insufficient. Side-effect > inducing > expressions (assignment, post/prefix increment ops) are still not handled. > The > exemption of JsNameRefs that are locals is also insufficient, since a side > effect inducing parameter value may be aliased to the same object as this > local. > After your review, I may remove it and just ban any non-param reference from > occuring while there are still params to be evaluated. > > I tested this and it seems to fix the problem. > > Review by: [email protected] > http://code.google.com/p/google-web-toolkit/source/detail?r=9362 > > Modified: > /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java > /trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java > > ======================================= > --- /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Mon Sep 20 > 06:50:17 2010 > +++ /trunk/dev/core/src/com/google/gwt/dev/js/JsInliner.java Mon Dec 6 > 14:51:53 2010 > @@ -611,12 +611,37 @@ > private boolean maintainsOrder = true; > private final List<JsName> toEvaluate; > private final List<JsName> unevaluated; > - > - public EvaluationOrderVisitor(List<JsName> toEvaluate) { > + private final Set<JsName> paramsOrLocals = new HashSet<JsName>(); > + > + public EvaluationOrderVisitor(List<JsName> toEvaluate, JsFunction > callee) { > this.toEvaluate = toEvaluate; > this.unevaluated = new ArrayList<JsName>(toEvaluate); > + // collect params and locals from callee function > + new JsVisitor() { > + �...@override > + public void endVisit(JsParameter x, JsContext<JsParameter> ctx) { > + paramsOrLocals.add(x.getName()); > + } > + > + �...@override > + public boolean visit(JsVar x, JsContext<JsVar> ctx) { > + // record this before visiting initializer > + paramsOrLocals.add(x.getName()); > + return true; > + } > + }.accept(callee); > } > > + /** > + * Referencing an array breaks order if there are unevaluated > arguments. > + */ > + �...@override > + public void endVisit(JsArrayAccess x, JsContext<JsExpression> ctx) { > + if (unevaluated.size() > 0) { > + maintainsOrder = false; > + } > + } > + > @Override > public void endVisit(JsBinaryOperation x, JsContext<JsExpression> ctx) { > JsBinaryOperator op = x.getOperator(); > @@ -669,12 +694,7 @@ > */ > @Override > public void endVisit(JsInvocation x, JsContext<JsExpression> ctx) { > - /* > - * The check for isExecuteOnce() is potentially incorrect here, > however > - * the original Java semantics of the clinit would have made the code > - * incorrect anyway. > - */ > - if ((isExecuteOnce(x) == null) && unevaluated.size() > 0) { > + if (unevaluated.size() > 0) { > maintainsOrder = false; > } > } > @@ -710,11 +730,35 @@ > return maintainsOrder && unevaluated.size() == 0; > } > > + /** > + * Check to see if the evaluation of this JsName will break program > order assumptions given > + * the parameters left to be substituted. > + * > + * The cases are as follows: > + * 1) JsName is a function parameter name which has side effects or is > affected by side effects > + * (hereafter called 'volatile'), so it will be in 'toEvaluate' > + * 2) JsName is a function parameter which is not volatile (not in > toEvaluate) > + * 3) JsName is a reference to a global variable > + * 4) JsName is a reference to a local variable > + * > + * A reference to a global while there are still parameters left to > evaluate / substitute > + * implies an order volation. > + * > + * A reference to a volatile parameter is ok if it if is the next > parameter in sequence to > + * be evaluated (beginning of unevaluated list). Else, it is either > being evaluated out of > + * order with respect to other parameters, or it is being evaluated > more than once. > + */ > private void checkName(JsName name) { > if (!toEvaluate.contains(name)) { > + // if the name is a non-local/non-parameter (e.g. global) and there > are params left to eval > + if (!paramsOrLocals.contains(name) && unevaluated.size() > 0) { > + maintainsOrder = false; > + } > + // else this may be a local, or all volatile params have already > been evaluated, so it's ok. > return; > } > > + // either this param is being evaled twice, or out of order > if (unevaluated.size() == 0 || !unevaluated.remove(0).equals(name)) { > maintainsOrder = false; > } > @@ -1945,7 +1989,7 @@ > * order. > */ > EvaluationOrderVisitor orderVisitor = new EvaluationOrderVisitor( > - requiredOrder); > + requiredOrder, callee); > orderVisitor.accept(toInline); > if (!orderVisitor.maintainsOrder()) { > return false; > ======================================= > --- /trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java Tue > Apr 13 06:08:53 2010 > +++ /trunk/dev/core/test/com/google/gwt/dev/js/JsInlinerTest.java Mon > Dec 6 14:51:53 2010 > @@ -79,6 +79,94 @@ > + "function c1() { return ex2 ? a1() :c1(); } c1()"; > compare(expected, input); > } > + > + /** > + * Test that an array reference breaks argument ordering. > + */ > + public void testOrderingArray() throws Exception { > + StringBuffer code = new StringBuffer(); > + > + code.append("function clinit() { clinit = null; }"); > + > + // callee references array[0] before evaluating argument > + code.append("function callee(arg) { var array; return array[0] + arg; > }"); > + > + // caller invokes callee with a multi that runs clinit() > + code.append("function caller() { callee((clinit(),2)); }"); > + > + // bootstrap the program > + code.append("caller();"); > + > + compare(code.toString(), code.toString()); > + } > + > + /** > + * Test that a field reference breaks argument ordering. > + */ > + public void testOrderingField() throws Exception { > + StringBuffer code = new StringBuffer(); > + > + code.append("function clinit() { clinit = null; }"); > + > + // callee references field.x before evaluating argument > + code.append("function callee(arg) { var field; return field.x + arg; > }"); > + > + // caller invokes callee with a multi that runs clinit() > + code.append("function caller() { callee((clinit(),2)); }"); > + > + // bootstrap the program > + code.append("caller();"); > + > + compare(code.toString(), code.toString()); > + } > + > + /** > + * Test that a global variable breaks argument ordering. > + */ > + public void testOrderingGlobal() throws Exception { > + StringBuffer code = new StringBuffer(); > + // A global variable x > + code.append("var x;"); > + > + // clinit() sets up x > + code.append("function clinit() { x = 1; clinit = null; }"); > + > + // callee references x before evaluating argument > + code.append("function callee(arg) { alert(x); return arg; }"); > + > + // caller invokes callee with a multi that runs clinit() > + code.append("function caller() { callee((clinit(),2)); }"); > + > + // bootstrap the program > + code.append("caller();"); > + > + compare(code.toString(), code.toString()); > + } > + > + /** > + * Test that a local variable does not break argument ordering. > + */ > + public void testOrderingLocal() throws Exception { > + StringBuffer code = new StringBuffer(); > + > + code.append("function clinit() { clinit = null; }"); > + > + // callee references y before evaluating argument > + code.append("function callee(arg) { var y; y=2; return arg; }"); > + > + // caller invokes callee with a multi that runs clinit() > + code.append("function caller() { return callee((clinit(),3)); }"); > + > + // bootstrap the program > + code.append("caller();"); > + > + StringBuffer expected = new StringBuffer(); > + > + expected.append("function clinit() { clinit = null; }"); > + expected.append("function caller() {var y; return y=2,clinit(),3;}"); > + expected.append("caller();"); > + compare(expected.toString(), code.toString()); > + } > > /** > * Test that a new expression breaks argument ordering. > > -- > http://groups.google.com/group/Google-Web-Toolkit-Contributors > -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
