Skip to content

Commit

Permalink
Closer to replacing all uninitialized copies of an object in the stac…
Browse files Browse the repository at this point in the history 
…k frame.
  • Loading branch information
@DanielSperry
DanielSperry committed May 24, 2015
1 parent 007cfff commit 0cb06a9
Show file tree
Hide file tree
Showing 2 changed files with 201 additions and 16 deletions.
82 changes: 72 additions & 10 deletions async/src/main/java/com/ea/orbit/async/instrumentation/Transformer.java
View file
Expand Up @@ -385,16 +385,19 @@ protected String getCommonSuperClass(final String type1, final String type2)
classNode.accept(cw); classNode.accept(cw);
byte[] bytes = cw.toByteArray(); byte[] bytes = cw.toByteArray();
// for development use: new ClassReader(bytes).accept(new TraceClassVisitor(new PrintWriter(System.out)), ClassReader.EXPAND_FRAMES); // for development use: new ClassReader(bytes).accept(new TraceClassVisitor(new PrintWriter(System.out)), ClassReader.EXPAND_FRAMES);
// for development use: debugSaveTrace(classNode.name + ".3", classNode); // for development use: DevDebug.debugSaveTrace(classNode.name + ".3", classNode);
// for development use: debugSave(classNode, bytes); // for development use: DevDebug.debugSave(classNode, bytes);
// for development use: debugSaveTrace(classNode.name + ".1", bytes); // for development use: DevDebug.debugSaveTrace(classNode.name + ".1", bytes);
return bytes; return bytes;
} }


private void replaceObjectInitialization( private void replaceObjectInitialization(
ClassNode classNode, final MethodNode methodNode, ClassNode classNode, final MethodNode methodNode,
final Map<String, Integer> nameUseCount, final Frame<BasicValue>[] frames) final Map<String, Integer> nameUseCount, final Frame<BasicValue>[] frames)
{ {
int originalLocals = methodNode.maxLocals;
int originalStack = methodNode.maxStack;

// since we can't store uninitialized objects they have to be removed or replaced. // since we can't store uninitialized objects they have to be removed or replaced.
// this works for bytecodes where the initialization is implemented like: // this works for bytecodes where the initialization is implemented like:
// NEW T // NEW T
Expand Down Expand Up @@ -447,17 +450,35 @@ else if (insnNode.getOpcode() == INVOKESPECIAL)
methodInsnNode.name = "new$async$" + cType.getInternalName().replace('/', '_'); methodInsnNode.name = "new$async$" + cType.getInternalName().replace('/', '_');
methodInsnNode.owner = classNode.name; methodInsnNode.owner = classNode.name;
Type[] oldArguments = Type.getArgumentTypes(methodInsnNode.desc); Type[] oldArguments = Type.getArgumentTypes(methodInsnNode.desc);
Type[] argumentTypes = new Type[oldArguments.length + 2];
argumentTypes[0] = OBJECT_TYPE; Frame<BasicValue> frameBefore = frames[index];
argumentTypes[1] = OBJECT_TYPE; int targetStackIndex = frameBefore.getStackSize() - (1 + oldArguments.length);
System.arraycopy(oldArguments, 0, argumentTypes, 2, oldArguments.length); final Value target = frameBefore.getStack(targetStackIndex);
int extraConsumed = 0;
// how many more to remove from the stack (normally exactly 1)
for (int i = targetStackIndex; --i >= 0 && target.equals(frameBefore.getStack(i)); )
{
extraConsumed++;
}

// test extraConsumed = 0 and extraConsumed = 2,3,4
// done at #uninitializedInTheStackSingle and #uninitializedInTheStackMultipleExtraCopies

Type[] argumentTypes = new Type[oldArguments.length + 1 + extraConsumed];
for (int i = 0; i <= extraConsumed; i++)
{
argumentTypes[i] = OBJECT_TYPE;
}
System.arraycopy(oldArguments, 0, argumentTypes, 1 + extraConsumed, oldArguments.length);
methodInsnNode.desc = Type.getMethodDescriptor(cType, argumentTypes); methodInsnNode.desc = Type.getMethodDescriptor(cType, argumentTypes);
final String key = methodInsnNode.name + methodInsnNode.desc; final String key = methodInsnNode.name + methodInsnNode.desc;


Frame<BasicValue> frameBefore = frames[index]; int stackSizeAfter = targetStackIndex;
final Value target = frameBefore.getStack(frameBefore.getStackSize() - (1 + oldArguments.length)); // accounting for the extra pops
stackSizeAfter -= extraConsumed;


// must test with double in the locals: { Ljava/lang/Object; D Uninitialized } // must test with double in the locals: { Ljava/lang/Object; D Uninitialized }
// done at #uninitializedStoreWithWideVarsAndGaps
for (int j = 0; j < frameBefore.getLocals(); ) for (int j = 0; j < frameBefore.getLocals(); )
{ {
// replaces all locals that used to reference the old value // replaces all locals that used to reference the old value
Expand All @@ -470,6 +491,47 @@ else if (insnNode.getOpcode() == INVOKESPECIAL)
j += local.getSize(); j += local.getSize();
} }


// find first stack occurrence that needs to be replaced
// int firstOccurrence = -1;
// for (int j = 0; j < stackSizeAfter; j++)
// {
// // replaces all locals that used to reference the old value
// BasicValue local = frameBefore.getStack(j);
// if (target.equals(local))
// {
// firstOccurrence = j;
// break;
// }
// }
// if (firstOccurrence > 0)
// {
// // replaces it in the stack
// // must test with double and long
// int bufferSpaceNeeded = 1;
//
// for (int j = stackSizeAfter; --j >= firstOccurrence; )
// {
// BasicValue value = frameBefore.getLocal(j);
// if (target.equals(value))
// {
// break;
// }
//
// }
// }

if (extraConsumed == 0)
{
methodNode.instructions.insert(insnNode, insnNode = new InsnNode(POP));
}
else
{
for (int i = 1; i < extraConsumed; i++)
{
methodNode.instructions.insert(insnNode, insnNode = new InsnNode(DUP));
}
}

if (nameUseCount.get(key) == null) if (nameUseCount.get(key) == null)
{ {
nameUseCount.put(key, 1); nameUseCount.put(key, 1);
Expand All @@ -482,7 +544,7 @@ else if (insnNode.getOpcode() == INVOKESPECIAL)
mv.visitInsn(DUP); mv.visitInsn(DUP);
int argSizes = 2; int argSizes = 2;
// must test with long and double // must test with long and double
for (int i = 2; i < argumentTypes.length; i++) for (int i = extraConsumed + 1; i < argumentTypes.length; i++)
{ {
mv.visitVarInsn((argumentTypes[i].getOpcode(ILOAD)), i); mv.visitVarInsn((argumentTypes[i].getOpcode(ILOAD)), i);
argSizes += argumentTypes[i].getSize(); argSizes += argumentTypes[i].getSize();
Expand Down
135 changes: 129 additions & 6 deletions async/src/test/java/com/ea/orbit/async/test/UnorthodoxFrameTest.java
View file
Expand Up @@ -30,6 +30,7 @@


import com.ea.orbit.concurrent.Task; import com.ea.orbit.concurrent.Task;
import com.ea.orbit.exception.UncheckedException; import com.ea.orbit.exception.UncheckedException;
import com.ea.orbit.util.StringUtils;


import org.junit.Ignore; import org.junit.Ignore;
import org.junit.Test; import org.junit.Test;
Expand Down Expand Up @@ -68,7 +69,10 @@ public <T> T createClass(Class<T> superClass, Consumer<ClassVisitor> populate)
superType = Type.getType(superClass); superType = Type.getType(superClass);
} }
String superName = superType.getInternalName(); String superName = superType.getInternalName();
cw.visit(52, ACC_PUBLIC, "com/ea/orbit/async/test/Experiment", null, superName, interfaces); StackTraceElement caller = new Exception().getStackTrace()[1];
final String name = "Experiment" + StringUtils.capitalize(caller.getMethodName()) + caller.getLineNumber();

cw.visit(52, ACC_PUBLIC, Type.getType(getClass()).getInternalName() + "$" + name, null, superName, interfaces);
populate.accept(cw); populate.accept(cw);
MethodVisitor mv; MethodVisitor mv;
{ {
Expand Down Expand Up @@ -210,7 +214,7 @@ public void regularConstructorCall() throws Exception
mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/async/Await", "await", "(Ljava/util/concurrent/CompletableFuture;)Ljava/lang/Object;", false); mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/async/Await", "await", "(Ljava/util/concurrent/CompletableFuture;)Ljava/lang/Object;", false);
mv.visitTypeInsn(CHECKCAST, "java/lang/String"); mv.visitTypeInsn(CHECKCAST, "java/lang/String");
mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Integer", "<init>", "(Ljava/lang/String;)V", false); mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Integer", "<init>", "(Ljava/lang/String;)V", false);
mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/concurrent/Task", "fromValue", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", false); mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/concurrent/Task", "fromValue", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", false);
mv.visitInsn(ARETURN); mv.visitInsn(ARETURN);
mv.visitMaxs(3, 2); mv.visitMaxs(3, 2);
mv.visitEnd(); mv.visitEnd();
Expand Down Expand Up @@ -261,6 +265,7 @@ public void uninitializedStore() throws Exception
public void uninitializedStoreWithWideVarsAndGaps() throws Exception public void uninitializedStoreWithWideVarsAndGaps() throws Exception
{ {
// check what happens when the uninitialized object is stored in a local variable // check what happens when the uninitialized object is stored in a local variable
// and with gaps and wide var (long and double)
final Task task = createClass(AsyncFunction.class, cw -> { final Task task = createClass(AsyncFunction.class, cw -> {
MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, "apply", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", null, new String[]{ "java/lang/Exception" }); MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, "apply", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", null, new String[]{ "java/lang/Exception" });
mv.visitCode(); mv.visitCode();
Expand Down Expand Up @@ -300,11 +305,42 @@ public void uninitializedStoreWithWideVarsAndGaps() throws Exception


@Test @Test
@SuppressWarnings("unchecked") @SuppressWarnings("unchecked")
@Ignore public void uninitializedInTheStackSingle() throws Exception
// TODO: create a fix for this condition
public void uninitializedInTheStack() throws Exception
{ {
// check what happens when the uninitialized object is stored in a local variable // check what happens if there only one copy of the uninitialized object in the stack.
// the java compiler usually leaves two copies
final Task task = createClass(AsyncFunction.class, cw -> {
MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, "apply", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", null, new String[]{ "java/lang/Exception" });
mv.visitCode();
mv.visitTypeInsn(NEW, "java/lang/Integer");
// note a missing DUP here
// the java compiler usually puts a dup here.

mv.visitVarInsn(ALOAD, 1);
mv.visitTypeInsn(CHECKCAST, "com/ea/orbit/concurrent/Task");
mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/async/Await", "await", "(Ljava/util/concurrent/CompletableFuture;)Ljava/lang/Object;", false);
mv.visitTypeInsn(CHECKCAST, "java/lang/String");
mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Integer", "<init>", "(Ljava/lang/String;)V", false);
// no pop need as the constructor consumes the one copy

mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/concurrent/Task", "done", "()Lcom/ea/orbit/concurrent/Task;", false);

mv.visitInsn(ARETURN);
mv.visitMaxs(4, 3);
mv.visitEnd();
}).apply(getBlockedTask("101"));
assertFalse(task.isDone());
completeFutures();
// return should be null as we are using task.done()
assertEquals(null, task.join());
}

@Test
@SuppressWarnings("unchecked")
public void uninitializedInTheStackOneExtraCopy() throws Exception
{
// check what happens when the uninitialized object appears 3 consecutive times in the stack
// the java compiler usually puts two copies.
final Task task = createClass(AsyncFunction.class, cw -> { final Task task = createClass(AsyncFunction.class, cw -> {
MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, "apply", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", null, new String[]{ "java/lang/Exception" }); MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, "apply", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", null, new String[]{ "java/lang/Exception" });
mv.visitCode(); mv.visitCode();
Expand Down Expand Up @@ -333,4 +369,91 @@ public void uninitializedInTheStack() throws Exception
completeFutures(); completeFutures();
assertEquals(101, task.join()); assertEquals(101, task.join());
} }

@Test
@SuppressWarnings("unchecked")
public void uninitializedInTheStackMultipleExtraCopies() throws Exception
{
// check what happens when the uninitialized object appears several times in the stack
// the java compiler usually puts two copies.
for (int extraCopies = 0; extraCopies < 10; extraCopies++)
{
int extra = extraCopies;
// check what happens when the uninitialized object is stored in a local variable
// without proper stack management this will fail in the jvm verifier
final Task task = createClass(AsyncFunction.class, cw -> {
MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, "apply", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", null, new String[]{ "java/lang/Exception" });
mv.visitCode();
mv.visitTypeInsn(NEW, "java/lang/Integer");
mv.visitInsn(DUP);

// this is valid bytecode: creating a 3rd copy of the uninitialized object
for (int i = 0; i < extra; i++)
{
mv.visitInsn(DUP);
}

mv.visitVarInsn(ALOAD, 1);
mv.visitTypeInsn(CHECKCAST, "com/ea/orbit/concurrent/Task");
mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/async/Await", "await", "(Ljava/util/concurrent/CompletableFuture;)Ljava/lang/Object;", false);
mv.visitTypeInsn(CHECKCAST, "java/lang/String");
mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Integer", "<init>", "(Ljava/lang/String;)V", false);

// discarding the object and getting the one that is in the stack (without instrumentation they are the same)
for (int i = 0; i < extra; i++)
{
mv.visitInsn(POP);
}

mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/concurrent/Task", "fromValue", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", false);

mv.visitInsn(ARETURN);
mv.visitMaxs(4 + extra, 3);
mv.visitEnd();
}).apply(getBlockedTask("101"));
assertFalse(task.isDone());
completeFutures();
assertEquals(101, task.join());
}
}


@Test
@SuppressWarnings("unchecked")
@Ignore
// TODO implement this
public void uninitializedInTheStackWithUnterleavedCopies() throws Exception
{
// check that the constructor replacement is able to replace interleaved elements in the stack
// obs.: the java compiler doesn't usually produce code like this
// regular java compiler will do { new dup dup ... <init> }
// this tests what happends if { new dup push_1 swap ... <init> pop }
final Task task = createClass(AsyncFunction.class, cw -> {
MethodVisitor mv = cw.visitMethod(ACC_PUBLIC, "apply", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", null, new String[]{ "java/lang/Exception" });
mv.visitCode();
mv.visitTypeInsn(NEW, "java/lang/Integer");

mv.visitInsn(DUP);
// interleaving copies in the stack
mv.visitIntInsn(SIPUSH, 1);
mv.visitInsn(SWAP);
// stack: { uobj, int, uobj }
mv.visitVarInsn(ALOAD, 1);
mv.visitTypeInsn(CHECKCAST, "com/ea/orbit/concurrent/Task");
mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/async/Await", "await", "(Ljava/util/concurrent/CompletableFuture;)Ljava/lang/Object;", false);
mv.visitTypeInsn(CHECKCAST, "java/lang/String");
mv.visitMethodInsn(INVOKESPECIAL, "java/lang/Integer", "<init>", "(Ljava/lang/String;)V", false);
// stack: { uobj, int }
mv.visitInsn(POP);
// stack: { uobj }

mv.visitMethodInsn(INVOKESTATIC, "com/ea/orbit/concurrent/Task", "fromValue", "(Ljava/lang/Object;)Lcom/ea/orbit/concurrent/Task;", false);
mv.visitInsn(ARETURN);
mv.visitMaxs(16, 3);
mv.visitEnd();
}).apply(getBlockedTask("101"));
assertFalse(task.isDone());
completeFutures();
assertEquals(101, task.join());
}
} }

0 comments on commit 0cb06a9

Please sign in to comment.