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

Let JacocoCoverageRunner identify the classpath on JDK 16+ #15081

Closed
wants to merge 1 commit into from
Closed

Let JacocoCoverageRunner identify the classpath on JDK 16+ #15081

Show file tree
Hide file tree
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/java_tools/junitrunner/java/com/google/testing/coverage/JacocoCoverageRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,8 +398,17 @@ private static URL[] getClassLoaderUrls(ClassLoader classLoader) {
field.setAccessible(true);
Unsafe unsafe = (Unsafe) field.get(null);

// jdk.internal.loader.ClassLoaders.AppClassLoader.ucp
Field ucpField = classLoader.getClass().getDeclaredField("ucp");
Field ucpField;
try {
// Java 9-15:
// jdk.internal.loader.ClassLoaders.AppClassLoader.ucp
ucpField = classLoader.getClass().getDeclaredField("ucp");
Copy link
Contributor

@comius comius Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@fmeum fmeum Mar 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@fmeum fmeum Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

@fmeum fmeum Apr 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

} catch (NoSuchFieldException e) {
// Java 16+:
// jdk.internal.loader.BuiltinClassLoader.ucp
// https://github.com/openjdk/jdk/commit/03a4df0acd103702e52dcd01c3f03fda4d7b04f5#diff-32cc12c0e3172fe5f2da1f65a75fa1cb920c39040d06323c83ad2c4d84e095aaL147
ucpField = classLoader.getClass().getSuperclass().getDeclaredField("ucp");
}
long ucpFieldOffset = unsafe.objectFieldOffset(ucpField);
Object ucpObject = unsafe.getObject(classLoader, ucpFieldOffset);

Expand Down
3 changes: 2 additions & 1 deletion src/test/shell/bazel/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,8 @@ sh_test(
],
tags = ["no_windows"],
)
for java_version in JAVA_VERSIONS_COVERAGE
# FIXME: Update JAVA_VERSIONS_COVERAGE on the next java_tools release.
for java_version in JAVA_VERSIONS_COVERAGE + ("17",)
]

sh_test(
Expand Down