Skip to content

Commit

Permalink
Fix incorrect optimization in TypeTightener with @jstype.
Browse files Browse the repository at this point in the history 
Typetigheter was replacing class super.m() with this.m() (resulting
in a runtime StackOverflowError if m() is called) win a specific case
when the following conditions happened:
  (1) an abstract JsType A with a single concrete JsType subclass B
  (2) an explicit call in B.m() to super.m().

Bug: issue 9153.
Change-Id: I1e5e3b99010d859440efa4abf70cfd0889b423fc
  • Loading branch information
@rluble
rluble committed Mar 16, 2015
1 parent b9e9d55 commit 69ffcb4
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 31 deletions.
6 changes: 4 additions & 2 deletions dev/core/src/com/google/gwt/dev/jjs/impl/TypeTightener.java
View file
Expand Up @@ -624,13 +624,14 @@ public void exit(JMethod x, Context ctx) {
*/
@Override
public void endVisit(JMethodCall x, Context ctx) {
if (x.isVolatile()) {
if (!x.canBePolymorphic() || x.isVolatile()) {
return;
}
JMethod target = x.getTarget();
JMethod concreteMethod = getSingleConcreteMethodOverride(target);
assert concreteMethod != target;
if (concreteMethod != null) {
assert !x.isStaticDispatchOnly();
JMethodCall newCall = new JMethodCall(x.getSourceInfo(), x.getInstance(), concreteMethod);
newCall.addArgs(x.getArgs());
ctx.replaceMe(newCall);
Expand All @@ -642,7 +643,7 @@ public void endVisit(JMethodCall x, Context ctx) {
* Mark a call as non-polymorphic if the targeted method is the only
* possible dispatch, given the qualifying instance type.
*/
if (x.canBePolymorphic() && !target.isAbstract()) {
if (!target.isAbstract()) {
JExpression instance = x.getInstance();
assert (instance != null);
JReferenceType instanceType = (JReferenceType) instance.getType();
Expand All @@ -658,6 +659,7 @@ public void endVisit(JMethodCall x, Context ctx) {
}
// The instance type is incompatible with all overrides.
}
assert !x.isStaticDispatchOnly();
x.setCannotBePolymorphic();
madeChanges();
}
Expand Down
22 changes: 22 additions & 0 deletions dev/core/test/com/google/gwt/dev/jjs/impl/OptimizerTestBase.java
View file
Expand Up @@ -37,6 +37,7 @@
import com.google.gwt.dev.jjs.ast.JReturnStatement;
import com.google.gwt.dev.jjs.ast.JStatement;
import com.google.gwt.dev.jjs.ast.JType;
import com.google.gwt.dev.jjs.ast.JVisitor;
import com.google.gwt.dev.jjs.ast.js.JMultiExpression;
import com.google.gwt.thirdparty.guava.common.base.Function;
import com.google.gwt.thirdparty.guava.common.base.Joiner;
Expand Down Expand Up @@ -157,6 +158,27 @@ public JProgram getOptimizedProgram() {
}
}

/**
* Asserts that there {@code method} calls all and only {@code expectedTargets}.
*/
protected static void assertCallsAndOnlyCalls(JMethod method, JMethod... expectedTargets) {
final Set<JMethod> actualTargets = Sets.newHashSet();
new JVisitor() {
@Override
public void endVisit(JMethodCall x, Context ctx) {
actualTargets.add(x.getTarget());
}
}.accept(method);
assertEquals(ImmutableSet.copyOf(expectedTargets), actualTargets);
}

/**
* Asserts that there {@code method} only calls {@code forwardsToMethod}.
*/
protected static void assertForwardsTo(JMethod method, JMethod forwardsToMethod) {
assertCallsAndOnlyCalls(method, forwardsToMethod);
}

protected static void assertOverrides(
Result result, String fullMethodSignature, String... overriddenMethodSignatures) {
assertEquals(ImmutableSet.copyOf(overriddenMethodSignatures),
Expand Down
11 changes: 11 additions & 0 deletions dev/core/test/com/google/gwt/dev/jjs/impl/TypeTightenerTest.java
View file
Expand Up @@ -170,6 +170,17 @@ public void testTightenDependsOnTightenedMethods() throws Exception {
OptimizerTestBase.findField(result.findClass("EntryPoint$C"), "a").toString());
}

public void testDoNotTighten_staticDispatch() throws Exception {
addSnippetClassDecl("abstract static class A { public void m() {} }");
addSnippetClassDecl("static class B extends A { public void m() { super.m(); }}");

Result result = optimize("void",
"new B().m();");

assertForwardsTo(result.findMethod("test.EntryPoint$B.m()V"),
result.findMethod("test.EntryPoint$A.m()V"));
}

@Override
protected boolean optimizeMethod(JProgram program, JMethod method) {
program.addEntryMethod(findMainMethod(program));
Expand Down
37 changes: 8 additions & 29 deletions dev/core/test/com/google/gwt/dev/jjs/impl/UnifyAstTest.java
View file
Expand Up @@ -16,16 +16,9 @@
import com.google.gwt.core.ext.UnableToCompleteException;
import com.google.gwt.dev.javac.testing.impl.JavaResourceBase;
import com.google.gwt.dev.javac.testing.impl.MockJavaResource;
import com.google.gwt.dev.jjs.ast.Context;
import com.google.gwt.dev.jjs.ast.JMethod;
import com.google.gwt.dev.jjs.ast.JMethodCall;
import com.google.gwt.dev.jjs.ast.JProgram;
import com.google.gwt.dev.jjs.ast.JVisitor;
import com.google.gwt.dev.util.arg.SourceLevel;
import com.google.gwt.thirdparty.guava.common.collect.ImmutableSet;
import com.google.gwt.thirdparty.guava.common.collect.Sets;

import java.util.Set;

/**
* Test for {@link UnifyAst}.
Expand Down Expand Up @@ -272,7 +265,7 @@ public void testAccidentalOverrides_concreteImplementationCase()

JMethod C_m = findMethod(result, "C.m()V");
assertFalse(C_m.isAbstract());
assertIsForwardingMethod(C_m, findMethod(result, "A.m()V"));
assertForwardsTo(C_m, findMethod(result, "A.m()V"));
assertOverrides(result, "C.m()V", "A.m()V", "I1.m()V");
}

Expand Down Expand Up @@ -311,7 +304,7 @@ public void testAccidentalOverrides_concreteImplementationInterfaceMethodDefault

JMethod C_m = findMethod(result, "C.m()V");
assertFalse(C_m.isAbstract());
assertIsForwardingMethod(C_m, findMethod(result, "A.m()V"));
assertForwardsTo(C_m, findMethod(result, "A.m()V"));
assertOverrides(result, "C.m()V", "A.m()V", "I1.m()V");
}

Expand Down Expand Up @@ -339,7 +332,7 @@ public void testDefaults_simpleCase()

JMethod A_m = findMethod(result, "A.m()V");
assertFalse(A_m.isAbstract());
assertIsForwardingMethod(A_m, findMethod(result, "I1.m()V"));
assertForwardsTo(A_m, findMethod(result, "I1.m()V"));
assertOverrides(result, "A.m()V", "I1.m()V");
}

Expand Down Expand Up @@ -378,12 +371,12 @@ public void testDefaults_diamond()

JMethod A1_m = findMethod(result, "A1.m()V");
assertFalse(A1_m.isAbstract());
assertIsForwardingMethod(A1_m, findMethod(result, "I12.m()V"));
assertForwardsTo(A1_m, findMethod(result, "I12.m()V"));
assertOverrides(result, "A1.m()V", "I1.m()V", "I12.m()V");

JMethod A2_m = findMethod(result, "A2.m()V");
assertFalse(A2_m.isAbstract());
assertIsForwardingMethod(A2_m, findMethod(result, "I12.m()V"));
assertForwardsTo(A2_m, findMethod(result, "I12.m()V"));
assertOverrides(result, "A2.m()V", "I1.m()V", "I12.m()V");
}

Expand Down Expand Up @@ -427,17 +420,17 @@ public void testDefaults_diamondAbstractBaseClass()

JMethod A_m = findMethod(result, "A.m()V");
assertFalse(A_m.isAbstract());
assertIsForwardingMethod(A_m, findMethod(result, "I1.m()V"));
assertForwardsTo(A_m, findMethod(result, "I1.m()V"));
assertOverrides(result, "A.m()V", "I1.m()V");

JMethod A1_m = findMethod(result, "A1.m()V");
assertFalse(A1_m.isAbstract());
assertIsForwardingMethod(A1_m, findMethod(result, "I12.m()V"));
assertForwardsTo(A1_m, findMethod(result, "I12.m()V"));
assertOverrides(result, "A1.m()V", "A.m()V", "I1.m()V", "I12.m()V");

JMethod A2_m = findMethod(result, "A2.m()V");
assertFalse(A2_m.isAbstract());
assertIsForwardingMethod(A2_m, findMethod(result, "I12.m()V"));
assertForwardsTo(A2_m, findMethod(result, "I12.m()V"));
assertOverrides(result, "A2.m()V", "A.m()V", "I1.m()V", "I12.m()V");
}

Expand All @@ -447,20 +440,6 @@ protected boolean optimizeMethod(JProgram program, JMethod method) {
return false;
}

private void assertIsForwardingMethod(JMethod method, JMethod forwardsToMethod) {
assertAllCallTargets(method, forwardsToMethod);
}

private static void assertAllCallTargets(JMethod method, JMethod... expectedTargets) {
final Set<JMethod> actualTargets = Sets.newHashSet();
new JVisitor() {
@Override
public void endVisit(JMethodCall x, Context ctx) {
actualTargets.add(x.getTarget());
}
}.accept(method);
assertEquals(ImmutableSet.copyOf(expectedTargets), actualTargets);
}
private static final MockJavaResource A_A =
JavaResourceBase.createMockJavaResource("a.A",
"package a;",
Expand Down
27 changes: 27 additions & 0 deletions user/test/com/google/gwt/dev/jjs/test/CompilerMiscRegressionTest.java
View file
Expand Up @@ -16,6 +16,7 @@
package com.google.gwt.dev.jjs.test;

import com.google.gwt.core.client.impl.DoNotInline;
import com.google.gwt.core.client.js.JsType;
import com.google.gwt.dev.jjs.test.overrides.package1.Caller;
import com.google.gwt.dev.jjs.test.overrides.package1.ClassExposingM;
import com.google.gwt.dev.jjs.test.overrides.package1.SomeParent;
Expand Down Expand Up @@ -307,6 +308,32 @@ public native void testMultipleClassLiteralReferences() /*-{
var b = @com.google.gwt.dev.jjs.test.CompilerMiscRegressionTest::class;
}-*/;

/**
* Test for issue 9153.
* <p>
* Typetightener used to incorrectly tighten method calls marked with STATIC_DISPATCH_ONLY.
*/
public void testIncorrectDispatch() {
final int[] state = new int[1];

@JsType
abstract class A {
public void m() {
state[0] = 1;
}
}

@JsType
class B extends A {
public void m() {
super.m();
}
}

new B().m();
assertEquals(1, state[0]);
}

private static void assertEqualContents(float[] expected, float[] actual) {

assertEquals("Array length mismatch", expected.length, actual.length);
Expand Down

0 comments on commit 69ffcb4

Please sign in to comment.