Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix JaCoCo instrumented constructor skipped. #8057

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix JaCoCo instrumented constructor skipped. #8057

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 56 additions & 42 deletions sandbox/src/main/java/org/robolectric/internal/bytecode/ClassInstrumentor.java
View file
Open in desktop
Expand Up @@ -36,7 +36,6 @@
import org.objectweb.asm.tree.MethodInsnNode;
import org.objectweb.asm.tree.MethodNode;
import org.objectweb.asm.tree.TypeInsnNode;
import org.objectweb.asm.tree.VarInsnNode;
import org.robolectric.util.PerfStatsCollector;

/**
Expand Down Expand Up @@ -212,12 +211,13 @@ private void addSetToSparseArray(MutableClass mutableClass) {
}

/**
* Checks if the first or second instruction is a Jacoco load instruction. Robolectric is not
* capable at the moment of re-instrumenting Jacoco-instrumented constructors, so these are
* currently skipped.
* Checks if the first or second instruction is a Jacoco load instruction.
*
* <p>Note that in most of cases, JaCoCo load instructions should locate at the beginning of the
* constructor.
*
* @param ctor constructor method node
* @return whether or not the constructor can be instrumented
* @return whether or not the constructor was instrumented by JaCoCo
*/
private boolean isJacocoInstrumented(MethodNode ctor) {
AbstractInsnNode[] insns = ctor.instructions.toArray();
Expand All @@ -229,42 +229,13 @@ private boolean isJacocoInstrumented(MethodNode ctor) {
if ((node instanceof LdcInsnNode && ((LdcInsnNode) node).cst instanceof ConstantDynamic)) {
ConstantDynamic cst = (ConstantDynamic) ((LdcInsnNode) node).cst;
return cst.getName().equals("$jacocoData");
} else if (insns.length > 1
&& insns[0] instanceof LabelNode
&& insns[1] instanceof MethodInsnNode) {
return "$jacocoInit".equals(((MethodInsnNode) insns[1]).name);
} else if (node instanceof MethodInsnNode) {
return "$jacocoInit".equals(((MethodInsnNode) node).name);
}
}
return false;
}

/**
* Adds a call $$robo$init, which instantiates a shadow object if required. This is to support
* custom shadows for Jacoco-instrumented classes (except cnstructor shadows).
*/
protected void addCallToRoboInit(MutableClass mutableClass, MethodNode ctor) {
AbstractInsnNode returnNode =
Iterables.find(
ctor.instructions,
node -> {
if (node.getOpcode() == Opcodes.INVOKESPECIAL) {
MethodInsnNode mNode = (MethodInsnNode) node;
return (mNode.owner.equals(mutableClass.internalClassName)
|| mNode.owner.equals(mutableClass.classNode.superName));
}
return false;
},
null);
ctor.instructions.insert(
returnNode,
new MethodInsnNode(
Opcodes.INVOKEVIRTUAL,
mutableClass.classType.getInternalName(),
ROBO_INIT_METHOD_NAME,
"()V"));
ctor.instructions.insert(returnNode, new VarInsnNode(Opcodes.ALOAD, 0));
}

private void instrumentMethods(MutableClass mutableClass) {
if (mutableClass.isInterface()) {
for (MethodNode method : mutableClass.getMethods()) {
Expand All @@ -278,11 +249,7 @@ private void instrumentMethods(MutableClass mutableClass) {
method.name = ShadowConstants.STATIC_INITIALIZER_METHOD_NAME;
mutableClass.addMethod(generateStaticInitializerNotifierMethod(mutableClass));
} else if (method.name.equals("<init>")) {
if (isJacocoInstrumented(method)) {
addCallToRoboInit(mutableClass, method);
} else {
instrumentConstructor(mutableClass, method);
}
instrumentConstructor(mutableClass, method);
} else if (!isSyntheticAccessorMethod(method) && !Modifier.isAbstract(method.access)) {
instrumentNormalMethod(mutableClass, method);
}
Expand Down Expand Up @@ -366,7 +333,8 @@ private static boolean isSyntheticAccessorMethod(MethodNode method) {
*
* <ul>
* <li>The original constructor will be stripped of its instructions leading up to, and
* including, the call to super() or this(). It is also renamed to $$robo$$__constructor__
* including, possible instrumented calls (like jacoco) before the call to super() or this()
* and the call to super() or this(). It is also renamed to $$robo$$__constructor__
* <li>A method called __constructor__ is created and its job is to call
* $$robo$$__constructor__. The __constructor__ method is what gets shadowed if a Shadow
* wants to shadow a constructor.
Expand All @@ -383,7 +351,15 @@ protected void instrumentConstructor(MutableClass mutableClass, MethodNode metho
int methodAccess = method.access;
makeMethodPrivate(method);

boolean isJacocoInstrumented = isJacocoInstrumented(method);

InsnList callSuper = extractCallToSuperConstructor(mutableClass, method);

if (isJacocoInstrumented) {
InsnList initJaCoCo = extractInitJaCoCoFromConstructorCallSuper(callSuper);
method.instructions.insert(initJaCoCo);
}

method.name = directMethodName(mutableClass, ShadowConstants.CONSTRUCTOR_METHOD_NAME);
mutableClass.addMethod(
redirectorMethod(mutableClass, method, ShadowConstants.CONSTRUCTOR_METHOD_NAME));
Expand All @@ -407,6 +383,44 @@ protected void instrumentConstructor(MutableClass mutableClass, MethodNode metho
mutableClass.addMethod(initMethodNode);
}

/**
* JaCoCo instrumentation initializes its probes before constructor to collect the coverage.
*
* <p>Those JaCoCo instrumented instructions are supported to be stripped from call super
* instructions and insert to the original constructor in case of breaking the following
* instructions.
*/
private InsnList extractInitJaCoCoFromConstructorCallSuper(InsnList callSuper) {
InsnList removedInstructions = new InsnList();

AbstractInsnNode[] insns = callSuper.toArray();
for (int i = 0; i < insns.length; i++) {
AbstractInsnNode node = insns[i];

if (node.getOpcode() == Opcodes.INVOKESTATIC) {
MethodInsnNode mnode = (MethodInsnNode) node;
if ("$jacocoInit".equals(mnode.name)) {
if (i + 1 >= insns.length) {
throw new RuntimeException("huh? JaCoCo instrumented instructions completed?!");
}

if (insns[i + 1].getOpcode() != Opcodes.ASTORE) {
throw new RuntimeException("huh? JaCoCo instrumented instructions no astore?!");
}

callSuper.remove(insns[i]);
callSuper.remove(insns[i + 1]);
removedInstructions.add(insns[i]);
removedInstructions.add(insns[i + 1]);

return removedInstructions;
}
}
}

throw new RuntimeException("huh? JaCoCo instrumented?!");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it fine to throw RuntimeException here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be a violation between robolectric's JaCoCo detection and JaCoCo instructions strip and split when it comes to line 421.
I think it would be better to fail early to avoid the previous issue when we has assumed that all the JaCoCo detection and JaCoCo instructions strip and split are breaked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But some users can accept no-code-coverage or other things, but they can't accept exceptions when running tests.

}

/**
* Checks to see if there are instructions after RETURN. If there are, it will check to see if
* they belong in the call-to-super, or the shadowable part of the constructor.
Expand Down