You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tapestry.apache.org by hl...@apache.org on 2011/04/30 00:56:00 UTC
svn commit: r1098002 - in /tapestry/tapestry5/trunk/plastic/src:
main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
test/groovy/org/apache/tapestry5/plastic/FieldHandleTests.groovy
test/java/testsubjects/FieldHandleAccessOnly.java
Author: hlship
Date: Fri Apr 29 22:55:59 2011
New Revision: 1098002
URL: http://svn.apache.org/viewvc?rev=1098002&view=rev
Log:
TAP5-853: Fix a bug where field access methods needed by the Shim were incorrectly optimized away
Added:
tapestry/tapestry5/trunk/plastic/src/test/java/testsubjects/FieldHandleAccessOnly.java
Modified:
tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
tapestry/tapestry5/trunk/plastic/src/test/groovy/org/apache/tapestry5/plastic/FieldHandleTests.groovy
Modified: tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java?rev=1098002&r1=1098001&r2=1098002&view=diff
==============================================================================
--- tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java (original)
+++ tapestry/tapestry5/trunk/plastic/src/main/java/org/apache/tapestry5/internal/plastic/PlasticClassImpl.java Fri Apr 29 22:55:59 2011
@@ -504,7 +504,7 @@ public class PlasticClassImpl extends Lo
// these also represent the names of the methods that replace field access
// in non-introduced methods
- private String getAccessName, setAccessName;
+ private MethodNode getAccess, setAccess;
private FieldState state = FieldState.INITIAL;
@@ -788,11 +788,11 @@ public class PlasticClassImpl extends Lo
private void replaceFieldWriteAccess(String conduitFieldName)
{
- setAccessName = makeUnique(methodNames, "set_" + node.name);
+ String setAccessName = makeUnique(methodNames, "set_" + node.name);
- MethodNode mn = new MethodNode(ACC_SYNTHETIC | ACC_FINAL, setAccessName, "(" + node.desc + ")V", null, null);
+ setAccess = new MethodNode(ACC_SYNTHETIC | ACC_FINAL, setAccessName, "(" + node.desc + ")V", null, null);
- InstructionBuilder builder = newBuilder(mn);
+ InstructionBuilder builder = newBuilder(setAccess);
pushFieldConduitOntoStack(conduitFieldName, builder);
@@ -809,18 +809,18 @@ public class PlasticClassImpl extends Lo
builder.returnResult();
- addMethod(mn);
+ addMethod(setAccess);
- fieldToWriteMethod.put(node.name, mn);
+ fieldToWriteMethod.put(node.name, setAccess);
}
private void replaceFieldReadAccess(String conduitFieldName)
{
- getAccessName = makeUnique(methodNames, "getfieldvalue_" + node.name);
+ String getAccessName = makeUnique(methodNames, "getfieldvalue_" + node.name);
- MethodNode mn = new MethodNode(ACC_SYNTHETIC | ACC_FINAL, getAccessName, "()" + node.desc, null, null);
+ getAccess = new MethodNode(ACC_SYNTHETIC | ACC_FINAL, getAccessName, "()" + node.desc, null, null);
- InstructionBuilder builder = newBuilder(mn);
+ InstructionBuilder builder = newBuilder(getAccess);
// Get the correct FieldConduit object on the stack
@@ -837,9 +837,9 @@ public class PlasticClassImpl extends Lo
builder.returnResult();
- addMethod(mn);
+ addMethod(getAccess);
- fieldToReadMethod.put(node.name, mn);
+ fieldToReadMethod.put(node.name, getAccess);
}
private void pushFieldConduitOntoStack(String conduitFileName, InstructionBuilder builder)
@@ -850,17 +850,17 @@ public class PlasticClassImpl extends Lo
private void makeReadOnly()
{
- setAccessName = makeUnique(methodNames, "setfieldvalue_" + node.name);
+ String setAccessName = makeUnique(methodNames, "setfieldvalue_" + node.name);
- MethodNode mn = new MethodNode(ACC_SYNTHETIC | ACC_FINAL, setAccessName, "(" + node.desc + ")V", null, null);
+ setAccess = new MethodNode(ACC_SYNTHETIC | ACC_FINAL, setAccessName, "(" + node.desc + ")V", null, null);
String message = String.format("Field %s of class %s is read-only.", node.name, className);
- newBuilder(mn).throwException(IllegalStateException.class, message);
+ newBuilder(setAccess).throwException(IllegalStateException.class, message);
- addMethod(mn);
+ addMethod(setAccess);
- fieldToWriteMethod.put(node.name, mn);
+ fieldToWriteMethod.put(node.name, setAccess);
node.access |= ACC_FINAL;
}
@@ -869,9 +869,9 @@ public class PlasticClassImpl extends Lo
* Adds a static setter method, allowing an external FieldAccess implementation
* to directly set the value of the field.
*/
- private MethodNode addSetAccessMethod()
+ private MethodNode addShimSetAccessMethod()
{
- String name = makeUnique(methodNames, "directset_" + node.name);
+ String name = makeUnique(methodNames, "shimset_" + node.name);
// Takes two Object parameters (instance, and value) and returns void.
@@ -884,12 +884,14 @@ public class PlasticClassImpl extends Lo
addMethod(mn);
+ fieldTransformMethods.add(mn);
+
return mn;
}
- private MethodNode addGetAccessMethod()
+ private MethodNode addShimGetAccessMethod()
{
- String name = makeUnique(methodNames, "directget_" + node.name);
+ String name = makeUnique(methodNames, "shimget_" + node.name);
MethodNode mn = new MethodNode(ACC_SYNTHETIC | ACC_FINAL, name, "()" + node.desc, null, null);
@@ -899,6 +901,8 @@ public class PlasticClassImpl extends Lo
addMethod(mn);
+ fieldTransformMethods.add(mn);
+
return mn;
}
@@ -913,16 +917,14 @@ public class PlasticClassImpl extends Lo
/** Invoked with the object instance on the stack and cast to the right type. */
void extendShimGet(SwitchBlock switchBlock)
{
- String accessMethodName = getAccessName;
-
- if (accessMethodName == null)
+ if (getAccess == null)
{
- MethodNode method = addGetAccessMethod();
- fieldTransformMethods.add(method);
- accessMethodName = method.name;
+ getAccess = addShimGetAccessMethod();
}
- final String methodToInvoke = accessMethodName;
+ final String methodToInvoke = getAccess.name;
+
+ shimInvokedMethods.add(getAccess);
switchBlock.addCase(fieldIndex, false, new InstructionBuilderCallback()
{
@@ -939,18 +941,16 @@ public class PlasticClassImpl extends Lo
*/
void extendShimSet(SwitchBlock switchBlock)
{
- String accessMethodName = setAccessName;
-
// If no conduit has yet been specified, then we need a set access method for the shim to invoke.
- if (accessMethodName == null)
+ if (setAccess == null)
{
- MethodNode method = addSetAccessMethod();
- fieldTransformMethods.add(method);
- accessMethodName = method.name;
+ setAccess = addShimSetAccessMethod();
}
- final String methodToInvoke = accessMethodName;
+ shimInvokedMethods.add(setAccess);
+
+ final String methodToInvoke = setAccess.name;
switchBlock.addCase(fieldIndex, true, new InstructionBuilderCallback()
{
@@ -1416,6 +1416,10 @@ public class PlasticClassImpl extends Lo
private final Set<MethodNode> fieldTransformMethods = PlasticInternalUtils.newSet();
+ // Tracks any methods that the Shim class uses to gain access to fields; used to ensure that
+ // such methods are not optimized away incorrectly.
+ private final Set<MethodNode> shimInvokedMethods = PlasticInternalUtils.newSet();
+
/**
* Maps a field name to a replacement method that should be invoked instead of reading the
* field.
@@ -1925,9 +1929,9 @@ public class PlasticClassImpl extends Lo
/**
* Iterates over all non-introduced methods, including the original constructor. For each
* method, the bytecode is scanned for field reads and writes. When a match is found against an intercepted field,
- * the operation is replaced with a method invocation. This is invoked only after THE {@link PlasticClassHandleShim}
- * for the class has been created, as the shim may create methods
- * that contain references to fields that may be subject to field access interception.
+ * the operation is replaced with a method invocation. This is invoked only after the {@link PlasticClassHandleShim}
+ * for the class has been created, as the shim may create methods that contain references to fields that may be
+ * subject to field access interception.
*/
private void interceptFieldAccess()
{
@@ -1938,6 +1942,9 @@ public class PlasticClassImpl extends Lo
unusedAccessMethods.addAll(fieldToReadMethod.values());
unusedAccessMethods.addAll(fieldToWriteMethod.values());
+ // Keep any field access methods invoked from the shim
+ unusedAccessMethods.removeAll(shimInvokedMethods);
+
for (MethodNode node : fieldTransformMethods)
{
// Intercept field access inside the method, tracking which access methods
Modified: tapestry/tapestry5/trunk/plastic/src/test/groovy/org/apache/tapestry5/plastic/FieldHandleTests.groovy
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/plastic/src/test/groovy/org/apache/tapestry5/plastic/FieldHandleTests.groovy?rev=1098002&r1=1098001&r2=1098002&view=diff
==============================================================================
--- tapestry/tapestry5/trunk/plastic/src/test/groovy/org/apache/tapestry5/plastic/FieldHandleTests.groovy (original)
+++ tapestry/tapestry5/trunk/plastic/src/test/groovy/org/apache/tapestry5/plastic/FieldHandleTests.groovy Fri Apr 29 22:55:59 2011
@@ -162,4 +162,30 @@ class FieldHandleTests extends Specifica
def handleByName(pc, name) {
pc.allFields.find({ it.name == name}).handle
}
+
+ /**
+ * Hit a bug where if a field has a conduit (and therefore, has added methods to access that conduit
+ * when the field is read or written) and it has a FieldHandle, the methods needed by the shim
+ * (for the FieldHandle) were incorrectly optimized out of existence.
+ */
+ def "handle to field keeps synthetic access methods from being removed"() {
+
+ def fc = Mock(FieldConduit)
+
+ def pc = mgr.getPlasticClass("testsubjects.FieldHandleAccessOnly")
+
+ def field = pc.allFields.first()
+
+ def handle = field.setConduit(fc).handle
+
+ def instance = pc.createInstantiator().newInstance()
+
+ when:
+
+ handle.set(instance, "ok")
+
+ then:
+
+ 1 * fc.set(_, _, "ok")
+ }
}
Added: tapestry/tapestry5/trunk/plastic/src/test/java/testsubjects/FieldHandleAccessOnly.java
URL: http://svn.apache.org/viewvc/tapestry/tapestry5/trunk/plastic/src/test/java/testsubjects/FieldHandleAccessOnly.java?rev=1098002&view=auto
==============================================================================
--- tapestry/tapestry5/trunk/plastic/src/test/java/testsubjects/FieldHandleAccessOnly.java (added)
+++ tapestry/tapestry5/trunk/plastic/src/test/java/testsubjects/FieldHandleAccessOnly.java Fri Apr 29 22:55:59 2011
@@ -0,0 +1,6 @@
+package testsubjects;
+
+public class FieldHandleAccessOnly
+{
+ private String unreachable;
+}