-
Notifications
You must be signed in to change notification settings - Fork 133
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.
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
Patch: jacoco LabelFlowAnalyzer should ignore jazzer data flow trace calls #707
Conversation
Nice catch! We should definitely fix this. Since we don't just apply a limited set of hooks before performing coverage instrumentation but also let the user add custom hooks, patching an exhaustive list into JaCoCo doesn't seem feasible. Could you check whether making coverage instrumentation the first rather than the last pass in jazzer/src/main/java/com/code_intelligence/jazzer/agent/RuntimeInstrumentor.kt Lines 221 to 232 in 0420f57
fixes the problem for your reproducer? Background: Our coverage instrumentation used to emit relatively complicated byte code that would trigger the tracing instrumentation, but this is no longer the case - we emit a single method call that shouldn't trigger any of the other instrumentation passes. |
It seems to me that all the user custom hooks are to intercept calls of some methods. |
I'll check the reverse of the hooks/coverage application and inform you. |
In my configuration for the exact project, from the first look, the proposed change of instrumentation order causes the same effect as the proposed patch. private fun instrument(internalClassName: String, bytecode: ByteArray, fullInstrumentation: Boolean): ByteArray {
val classWithHooksEnabledField = if (Opt.conditionalHooks) {
// Let the hook instrumentation emit additional logic that checks the value of the
// hooksEnabled field on this class and skips the hook if it is false.
"com/code_intelligence/jazzer/runtime/JazzerInternal"
} else {
null
}
return ClassInstrumentor(internalClassName, bytecode).run {
if (fullInstrumentation) {
coverageIdSynchronizer.withIdForClass(internalClassName) { firstId ->
coverage(firstId).also { actualNumEdgeIds ->
CoverageRecorder.recordInstrumentedClass(
internalClassName,
bytecode,
firstId,
actualNumEdgeIds,
)
}
}
// Hook instrumentation must be performed after data flow tracing as the injected
// bytecode would trigger the GEP callbacks for byte[]. Coverage instrumentation
// must be performed after hook instrumentation as the injected bytecode would
// trigger the GEP callbacks for ByteBuffer.
traceDataFlow(instrumentationTypes)
hooks(includedHooks + customHooks, classWithHooksEnabledField)
} else {
hooks(customHooks, classWithHooksEnabledField)
}
instrumentedBytecode
}
} |
@kmnls That looks promising. Could you submit a PR with this alternative approach? It does seem simpler and not having to patch JaCoCo more than absolutely necessary will help us maintain our dependencies. |
See #711 |
Reason:
JaCoCo produces wrong coverage because the coverage instrumentation happens after the hooks are set.
This instrumentation should follow the JaCoCo algorithm but in fact they produce different results.
Both algorithms inject control points into the end of each BB where the INVOKE happens.
BBs where there are no INVOKEs are not marked.
Let's check the case: BBs originally did not have any INVOKE but hooks are injected traceCmp callback.
CoverageRecorder happily adds a control point into such a BB but JaCoCo will never know about this.
As the result, all points after the mentioned will be misplaced in JaCoCo coverage.
Sample for test:
All the lines after
if (earlyReturn)
will be colored wrongly because this BB will have an additional coverage point after traceCmp.