Fixes an inconsistency that resulted in a bad AST.
In some cases the Pruner would leave the AST in an incosistent form
where there is a JNewInstance poiting to a JConstructor that is has
been already removed from the correspoinding class.
Suprisingly most passes seem not to care as, in fact, this constructor
is actually unreachable. However, under the correct (or should I say
wrong) circumstances MethodInliner fails due to inconsistent
expectations about the parameters of the method it inlines.
This patch also adds a verification stage after each transformation
in the optimization loop that is only run when assertions are enabled.
Change-Id: Iabcc6d4247205f6097e514d33a14bd4a1e9924a9
diff --git a/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java b/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
index 40dfd46..3417fbc 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/JavaToJavaScriptCompiler.java
@@ -70,6 +70,7 @@
import com.google.gwt.dev.jjs.impl.FixAssignmentsToUnboxOrCast;
import com.google.gwt.dev.jjs.impl.GenerateJavaScriptAST;
import com.google.gwt.dev.jjs.impl.ImplementClassLiteralsAsFields;
+import com.google.gwt.dev.jjs.impl.JavaAstVerifier;
import com.google.gwt.dev.jjs.impl.JavaToJavaScriptMap;
import com.google.gwt.dev.jjs.impl.JsAbstractTextTransformer;
import com.google.gwt.dev.jjs.impl.JsFunctionClusterer;
@@ -1501,6 +1502,7 @@
// Clinits might have become empty become empty.
jprogram.typeOracle.recomputeAfterOptimizations(jprogram.getDeclaredTypes());
OptimizerStats stats = new OptimizerStats(passName);
+ JavaAstVerifier.assertProgramIsConsistent(jprogram);
stats.add(Pruner.exec(jprogram, true, optimizerCtx).recordVisits(numNodes));
stats.add(Finalizer.exec(jprogram, optimizerCtx).recordVisits(numNodes));
stats.add(MakeCallsStatic.exec(jprogram, options.shouldAddRuntimeChecks(), optimizerCtx)
diff --git a/dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java b/dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
index 1629395..67606d2 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/ast/JTypeOracle.java
@@ -284,6 +284,12 @@
*
* This is used to remove "dead clinit cycles" where self-referential cycles
* of empty clinits can keep each other alive.
+ * <p>
+ * IMPORTANT: do not optimize clinit visitor to do a better job in determining if the clinit
+ * contains useful code (like by doing implicit DeadCodeEliminination). Passes like
+ * ControlFlowAnalyzer and Pruner will produce inconsistent ASTs.
+ *
+ * @see ControlFlowAnalyzer.visit(JClassType class, Context ctx)
*/
private static final class CheckClinitVisitor extends JVisitor {
@@ -294,8 +300,8 @@
* because we explicitly visit all AST structures that might contain
* non-clinit-calling code.
*
- * @see #mightBeDeadCode(JExpression)
- * @see #mightBeDeadCode(JStatement)
+ * @see #mightContainOnlyClinitCalls(JExpression)
+ * @see #mightContainOnlyClinitCallsOrDeclarationStatements(JStatement)
*/
private boolean hasLiveCode = false;
@@ -310,7 +316,7 @@
@Override
public boolean visit(JBlock x, Context ctx) {
for (JStatement stmt : x.getStatements()) {
- if (mightBeDeadCode(stmt)) {
+ if (mightContainOnlyClinitCallsOrDeclarationStatements(stmt)) {
accept(stmt);
} else {
hasLiveCode = true;
@@ -324,8 +330,11 @@
JVariable target = x.getVariableRef().getTarget();
if (target instanceof JField) {
JField field = (JField) target;
- if (field.getLiteralInitializer() != null) {
- // Top level initializations generate no code.
+ // {@See ControlFlowAnalizer.rescue(JVariable var)
+ if (field.getLiteralInitializer() != null && field.isStatic()) {
+ // Literal initializers for static fields, even though they appear in the clinit they are
+ // not considered part of it; instead they are normally considered part of the fields they
+ // initialize.
return false;
}
}
@@ -336,7 +345,7 @@
@Override
public boolean visit(JExpressionStatement x, Context ctx) {
JExpression expr = x.getExpr();
- if (mightBeDeadCode(expr)) {
+ if (mightContainOnlyClinitCalls(expr)) {
accept(expr);
} else {
hasLiveCode = true;
@@ -359,7 +368,7 @@
public boolean visit(JMultiExpression x, Context ctx) {
for (JExpression expr : x.getExpressions()) {
// Only a JMultiExpression or JMethodCall can contain clinit calls.
- if (mightBeDeadCode(expr)) {
+ if (mightContainOnlyClinitCalls(expr)) {
accept(expr);
} else {
hasLiveCode = true;
@@ -368,22 +377,13 @@
return false;
}
- @Override
- public boolean visit(JNewInstance x, Context ctx) {
- if (x.hasSideEffects()) {
- hasLiveCode = true;
- }
- return false;
+ private boolean mightContainOnlyClinitCalls(JExpression expr) {
+ // Must have a visit method for every subtype that might answer yes!
+ return expr instanceof JMultiExpression || expr instanceof JMethodCall;
}
- private boolean mightBeDeadCode(JExpression expr) {
- // Must have a visit method for every subtype that answers yes!
- return expr instanceof JMultiExpression || expr instanceof JMethodCall
- || expr instanceof JNewInstance;
- }
-
- private boolean mightBeDeadCode(JStatement stmt) {
- // Must have a visit method for every subtype that answers yes!
+ private boolean mightContainOnlyClinitCallsOrDeclarationStatements(JStatement stmt) {
+ // Must have a visit method for every subtype that might answer yes!
return stmt instanceof JBlock || stmt instanceof JExpressionStatement
|| stmt instanceof JDeclarationStatement;
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
index ca82868..59c860e 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/ControlFlowAnalyzer.java
@@ -741,6 +741,8 @@
* the static initializer runs. This allows fields initialized to
* string literals to only need the string literals when the field
* itself becomes live.
+ *
+ * NOTE: needs to be in sync with {@link JTypeOracle.CheckClinitVistior}.
*/
accept(((JField) var).getLiteralInitializer());
} else if (var instanceof JField
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java b/dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java
index 0b5af10..d80c4a4 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/DeadCodeElimination.java
@@ -2027,6 +2027,7 @@
DeadCodeVisitor deadCodeVisitor = new DeadCodeVisitor(optimizerCtx);
deadCodeVisitor.accept(node);
stats.recordModified(deadCodeVisitor.getNumMods());
+ JavaAstVerifier.assertProgramIsConsistent(program);
optimizeEvent.end("didChange", "" + stats.didChange());
return stats;
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
index 040f709..95ba6c3 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/EnumOrdinalizer.java
@@ -835,6 +835,7 @@
type.setOrdinalized();
}
}
+ JavaAstVerifier.assertProgramIsConsistent(program);
return stats;
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java
index c09c475..5ee90b8 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/Finalizer.java
@@ -240,6 +240,8 @@
FinalizeVisitor finalizer = new FinalizeVisitor(optimizerCtx);
finalizer.accept(program);
+ JavaAstVerifier.assertProgramIsConsistent(program);
+
return new OptimizerStats(NAME).recordModified(finalizer.getNumMods());
}
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/JavaAstVerifier.java b/dev/core/src/com/google/gwt/dev/jjs/impl/JavaAstVerifier.java
new file mode 100644
index 0000000..9a2be14
--- /dev/null
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/JavaAstVerifier.java
@@ -0,0 +1,93 @@
+/*
+ * Copyright 2014 Google Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+
+package com.google.gwt.dev.jjs.impl;
+
+import com.google.gwt.dev.jjs.ast.Context;
+import com.google.gwt.dev.jjs.ast.JDeclaredType;
+import com.google.gwt.dev.jjs.ast.JField;
+import com.google.gwt.dev.jjs.ast.JFieldRef;
+import com.google.gwt.dev.jjs.ast.JMethod;
+import com.google.gwt.dev.jjs.ast.JMethodCall;
+import com.google.gwt.dev.jjs.ast.JNode;
+import com.google.gwt.dev.jjs.ast.JProgram;
+import com.google.gwt.dev.jjs.ast.JVisitor;
+import com.google.gwt.dev.jjs.ast.js.JsniFieldRef;
+import com.google.gwt.dev.jjs.ast.js.JsniMethodRef;
+import com.google.gwt.thirdparty.guava.common.collect.HashMultimap;
+import com.google.gwt.thirdparty.guava.common.collect.Multimap;
+
+/**
+ * Verifies that all the references from AST nodes to AST nodes are reachable from the
+ * top of the AST.
+ * <p>
+ * The purpose fo this pass is to verify the consistency of the AST after a specific pass has
+ * run.
+ */
+public class JavaAstVerifier extends JVisitor {
+
+ private Multimap<JDeclaredType, JNode> membersByType = HashMultimap.create();
+
+ JavaAstVerifier(JProgram program) {
+ for (JDeclaredType type :program.getModuleDeclaredTypes()) {
+ membersByType.putAll(type, type.getMethods());
+ membersByType.putAll(type, type.getFields());
+ }
+ }
+
+ /**
+ * Throws an assertion error if the AST for a program is not consistent.
+ */
+ public static void assertProgramIsConsistent(JProgram program) {
+ if (JavaAstVerifier.class.desiredAssertionStatus()) {
+ new JavaAstVerifier(program).accept(program);
+ }
+ }
+
+ @Override
+ public void endVisit(JFieldRef x, Context ctx) {
+ assertReferencedFieldIsInAst(x);
+ }
+
+ @Override
+ public void endVisit(JMethodCall x, Context ctx) {
+ assertCalledMethodIsInAst(x);
+ }
+
+ @Override
+ public void endVisit(JsniFieldRef x, Context ctx) {
+ assertReferencedFieldIsInAst(x);
+ }
+
+ @Override
+ public void endVisit(JsniMethodRef x, Context ctx) {
+ assertCalledMethodIsInAst(x);
+ }
+
+ private void assertCalledMethodIsInAst(JMethodCall x) {
+ if (x.getTarget() == JMethod.NULL_METHOD) {
+ return;
+ }
+ assert membersByType.containsEntry(x.getTarget().getEnclosingType(), x.getTarget());
+ }
+
+ private void assertReferencedFieldIsInAst(JFieldRef x) {
+ if (x.getField() == JField.NULL_FIELD) {
+ return;
+ }
+ assert membersByType.containsEntry(x.getField().getEnclosingType(), x.getField());
+ }
+}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java b/dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java
index d140c55..feaff7c 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/MakeCallsStatic.java
@@ -503,6 +503,7 @@
rewriter.accept(program);
stats.recordModified(rewriter.getNumMods());
assert (rewriter.didChange() || toBeMadeStatic.isEmpty());
+ JavaAstVerifier.assertProgramIsConsistent(program);
return stats;
}
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallSpecializer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallSpecializer.java
index 1e5b005..e75d89d 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallSpecializer.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallSpecializer.java
@@ -111,6 +111,7 @@
private OptimizerStats execImpl(OptimizerContext optimizerCtx) {
MethodCallSpecializingVisitor specializer = new MethodCallSpecializingVisitor(optimizerCtx);
specializer.accept(program);
+ JavaAstVerifier.assertProgramIsConsistent(program);
return new OptimizerStats(NAME).recordModified(specializer.getNumMods());
}
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java
index d72d0da..54e01cc 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodCallTightener.java
@@ -130,6 +130,7 @@
OptimizerStats stats = new MethodCallTightener(program).execImpl(optimizerCtx);
optimizerCtx.incOptimizationStep();
optimizeEvent.end("didChange", "" + stats.didChange());
+ JavaAstVerifier.assertProgramIsConsistent(program);
return stats;
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
index 8d3296e..5ac8509 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
@@ -586,6 +586,7 @@
stats.recordModified(dceStats.getNumMods());
}
optimizerCtx.setLastStepFor(NAME, optimizerCtx.getOptimizationStep());
+ JavaAstVerifier.assertProgramIsConsistent(program);
return stats;
}
@@ -593,7 +594,8 @@
* Return the set of methods affected (because they are or callers of) by the modifications to the
* given set functions.
*/
- private Set<JMethod> affectedMethods(Set<JMethod> modifiedMethods, OptimizerContext optimizerCtx) {
+ private Set<JMethod> affectedMethods(Set<JMethod> modifiedMethods,
+ OptimizerContext optimizerCtx) {
Set<JMethod> affectedMethods = Sets.newLinkedHashSet();
if (modifiedMethods == null || modifiedMethods.size() == 0) {
return affectedMethods;
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java b/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
index 5dddc53..903b153 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/Pruner.java
@@ -55,6 +55,8 @@
import com.google.gwt.dev.util.log.speedtracer.CompilerEventType;
import com.google.gwt.dev.util.log.speedtracer.SpeedTracerLogger;
import com.google.gwt.dev.util.log.speedtracer.SpeedTracerLogger.Event;
+import com.google.gwt.thirdparty.guava.common.base.Predicate;
+import com.google.gwt.thirdparty.guava.common.base.Predicates;
import java.util.ArrayList;
import java.util.HashMap;
@@ -356,31 +358,12 @@
@Override
public boolean visit(JClassType type, Context ctx) {
assert (referencedTypes.contains(type));
- for (int i = 0; i < type.getFields().size(); ++i) {
- JField field = type.getFields().get(i);
- if (!referencedNonTypes.contains(field)) {
- fieldWasRemoved(field);
- type.removeField(i);
- madeChanges();
- --i;
- }
- }
+ Predicate<JNode> notReferenced = Predicates.not(Predicates.in(referencedNonTypes));
+ removeFields(notReferenced, type);
+ removeMethods(notReferenced, type);
- for (int i = 0; i < type.getMethods().size(); ++i) {
- JMethod method = type.getMethods().get(i);
- if (!referencedNonTypes.contains(method)) {
- // Never prune clinit directly out of the class.
- if (i > 0) {
- methodWasRemoved(method);
- type.removeMethod(i);
- methodWasRemoved(program.getStaticImpl(method));
- program.removeStaticImplMapping(method);
- madeChanges();
- --i;
- }
- } else {
- accept(method);
- }
+ for (JMethod method : type.getMethods()) {
+ accept(method);
}
return false;
@@ -388,32 +371,16 @@
@Override
public boolean visit(JInterfaceType type, Context ctx) {
- boolean isReferenced = referencedTypes.contains(type);
- boolean isInstantiated = program.typeOracle.isInstantiatedType(type);
+ Predicate<JNode> notReferenced = Predicates.not(Predicates.in(referencedNonTypes));
+ boolean typeReferenced = referencedTypes.contains(type);
+ boolean typeInstantiated = program.typeOracle.isInstantiatedType(type);
- for (int i = 0; i < type.getFields().size(); ++i) {
- JField field = type.getFields().get(i);
- // all interface fields are static and final
- if (!isReferenced || !referencedNonTypes.contains(field)) {
- fieldWasRemoved(field);
- type.removeField(i);
- madeChanges();
- --i;
- }
- }
-
- // Start at index 1; never prune clinit directly out of the interface.
- for (int i = 1; i < type.getMethods().size(); ++i) {
- JMethod method = type.getMethods().get(i);
- // all other interface methods are instance and abstract
- if (!isInstantiated || !referencedNonTypes.contains(method)) {
- methodWasRemoved(method);
- type.removeMethod(i);
- program.removeStaticImplMapping(method);
- madeChanges();
- --i;
- }
- }
+ Predicate<JNode> notReferencedField =
+ typeReferenced ? notReferenced : Predicates.<JNode>alwaysTrue();
+ Predicate<JNode> notReferencedMethod =
+ typeInstantiated ? notReferenced : Predicates.<JNode>alwaysTrue();
+ removeFields(notReferencedField, type);
+ removeMethods(notReferencedMethod, type);
return false;
}
@@ -500,6 +467,35 @@
}
return false;
}
+
+ private void removeFields(Predicate<JNode> shouldRemove, JDeclaredType type) {
+ for (int i = 0; i < type.getFields().size(); ++i) {
+ JField field = type.getFields().get(i);
+ if (!shouldRemove.apply(field)) {
+ continue;
+ }
+ fieldWasRemoved(field);
+ type.removeField(i);
+ madeChanges();
+ --i;
+ }
+ }
+
+ private void removeMethods(Predicate<JNode> shouldRemove, JDeclaredType type) {
+ // Skip method 0 which is clinit and is assumed to exist.
+ assert type.getMethods().get(0) == type.getClinitMethod();
+ for (int i = 1; i < type.getMethods().size(); ++i) {
+ JMethod method = type.getMethods().get(i);
+ if (!shouldRemove.apply(method)) {
+ continue;
+ }
+ methodWasRemoved(method);
+ type.removeMethod(i);
+ program.removeStaticImplMapping(method);
+ madeChanges();
+ --i;
+ }
+ }
}
private static final String NAME = Pruner.class.getSimpleName();
@@ -667,6 +663,8 @@
new CleanupRefsVisitor(livenessAnalyzer.getLiveFieldsAndMethods(), pruner
.getMethodToOriginalParamsMap(), optimizerCtx);
cleaner.accept(program.getDeclaredTypes());
+
+ JavaAstVerifier.assertProgramIsConsistent(program);
return stats;
}
@@ -685,5 +683,4 @@
}
}
}
-
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/SameParameterValueOptimizer.java b/dev/core/src/com/google/gwt/dev/jjs/impl/SameParameterValueOptimizer.java
index fcd27be..525f26d 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/SameParameterValueOptimizer.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/SameParameterValueOptimizer.java
@@ -243,6 +243,7 @@
stats.recordModified(substituteParameterVisitor.getNumMods());
}
}
+ JavaAstVerifier.assertProgramIsConsistent(program);
return stats;
}
}
diff --git a/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java b/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
index a531e5b..9b9b88f 100644
--- a/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
+++ b/dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
@@ -906,6 +906,7 @@
if (stats.didChange()) {
FixDanglingRefsVisitor fixer = new FixDanglingRefsVisitor(optimizerCtx);
fixer.accept(program);
+ JavaAstVerifier.assertProgramIsConsistent(program);
}
return stats;
