Let JacocoCoverageRunner identify the classpath on JDK 16+

[fmeum]

The Unsafe-backed extraction of the classpath in
JacocoCoverageRunner#getClassLoaderUrls has to be adapted for JDK 16+
as the private ucp field on AppClassLoader has been moved to its
superclass.

With this commit, the existing coverage integration tests pass for the
JDK 17 toolchain.

[comius]

cc @c-mita

[comius]

I thinkucpField = classLoader.getClass().getField("ucp"); looks also into super class. Using it would be nicer than catching an exception an retrying.

[fmeum]

It does look into super classes, but only finds public fields: https://docs.oracle.com/javase/8/docs/api/java/lang/Class.html#getField-java.lang.String-. I don't see a way around using getDeclaredField on the super class, but we could of course try to detect Java 16+ directly instead of by the absence of the field.

[cushon]

I wonder if getClassLoaderUrls has requirements that aren't met by Guava's ClassPath or ClassGraph?

It would be nice to avoid layering on reflection here, especially since it's getting harder to reflect on internal APIs with JEP-403 etc.

[fmeum]

I wonder if getClassLoaderUrls has requirements that aren't met by Guava's ClassPath or ClassGraph?

I think that either of those two could be used here. Most of the complexity ClassGraph deals with, in particular special class loaders offeres by frameworks such as Spring, isn't relevant here, but of course that's not a reason not to use it.

The function in ClassGraph that implements the equivalent of getClassLoaderUrls is here. Note that it does use a native library as a helper to work around restrictions on reflection usage.

It would be nice to avoid layering on reflection here, especially since it's getting harder to reflect on internal APIs with JEP-403 etc.

I think that at least JEP-403 is not something we need to worry about here: AFAIU, strong encapsulation means that setAccessible(true) calls on private or protected fields will fail, but getClassLoaderUrls already circumvents this by using Unsafe to directly read the field contents from memory. This strategy should be future-proof for as long as sun.misc.Unsafe itself remains accessible.

[cushon]

Thanks. I realized we had a separate internal fix for a related issue that's doing something similar to what ClassPath is doing here: https://github.com/google/guava/blob/2a268346329c3ce17ad1269c5c0206ddd2747a17/guava/src/com/google/common/reflect/ClassPath.java#L642

Porting that fix to JacocoCoverageRunner would look like this: https://bazel-review.googlesource.com/c/bazel/+/194693

What do you think about that as an alternative to the reflection here?

I agree that this approach is going to work for existing releases, but I'd still prefer to avoid Unsafe if possible.

[cushon]

I see, thanks. Would you be willing to investigate the ClassPath or ClassGraph-based approaches?

[fmeum]

Yes, I will go over the possible approaches and let you know about their pros and cons.

[fmeum]

I looked into the different approaches in detail and found the following.

Guava's ClassPath: Parses java.class.path and returns a list of URLs via ClassPath.parseJavaClassPath, but doesn't expose this functionality as public API. Efforts to change that are tracked in b/65488446, but given that the class doc recommends using ClassGraph instead, I don't know how likely this is to be fixed.
A general downside of this approach is that java.class.path doesn't reflect the full class path of the system class loader: Jars specified via the -javaagent JVM option and those added via Instrumentation#appendToSystemClassLoaderSearch are not added to the properties value. Going with this approach would thus mean that coverage wouldn't be collected correctly for Java agents without workarounds.

ClassGraph: Reads the ucp field of the system class loader. To do this on JDK 17, it delegates to Narcissus, which ultimately relies on a JNI library to circumvent strong encapsulation. Narcissus doesn't seem to support M1 Macs or Windows on ARM yet, but that could be worked around by building the dependency from source with Bazel instead of importing a jar. I do not know whether there are any caveats here. New versions of the JDK that change the relevant internals would still require an update of this dependency.
An advantage of ClassGraph is that it could handle many custom class loaders if those are used as the system class loader via the java.system.class.loader property. I don't know how common that is though.

https://bazel-review.googlesource.com/c/bazel/+/194693: Essentially does what Guava does, sharing the pros (only uses public APIs) and cons (doesn't handle Java agents).

This PR: Uses the same approach as ClassGraph. It doesn't require a JNI library or third-party dependency.

Based on my analysis, I would say that neither ClassGraph nor Guava's ClassPath are viable alternatives at this point (although this may change as part of b/65488446). Given that I have a concrete use case for Java agent coverage collection, I would prefer a solution that reads ucp of the system class loader. Given that this part of the code has only required changes twice since Java 8 and would only break fundamentally if Unsafe were to be removed without replacement, I would personally find the maintenance burden acceptable. But I can understand if you arrive at a different conclusion and would, in that case, propose to go with your Bazel CL, possibly with the wrapper jar logic added back.

@cushon What do you think?

[cushon]

Thanks for the thorough investigation here, I agree there don't seem to be great alternatives that would provide support for instrumenting agents.

For my understanding, is the Jazzer use-case to get Java coverage support when running Jazzer's own unit tests using the java_test rule?

[fmeum]

For my understanding, is the Jazzer use-case to get Java coverage support when running Jazzer's own unit tests using the java_test rule?

Yes. Jazzer itself is a cc_binary that starts a JVM with a java_binary deploy jar attached as an agent. We have a java_test-based macro that runs Jazzer on a number of example projects as an end-to-end test and would like to use these tests to collect coverage reports for Jazzer itself. rules_jni recently gained the ability to do this, but it requires JacocoCoverageRunner to see java agent jars.

[fmeum]

@comius Friendly ping. Is the import the only remaining step?

[fmeum]

@cushon How does the process for a new java_tools release look like? Do I have to do anything else to get the change in now that it has been merged to master?

[comius]

@cushon How does the process for a new java_tools release look like? Do I have to do anything else to get the change in now that it has been merged to master?

cc @hvadehra

[gergelytraveltime]

As a workaround you could build jacocorunner yourself (from the fixed version), and override it in the java_toolchain.