Hi Brent,
The updated webrev looks good to me.
At least, I do not see any issues.
Thanks,
Serguei
On 9/5/19 17:09, Brent Christian wrote:
Hi, David
On 9/5/19 12:52 AM, David Holmes wrote:
Good to see this all come together :)
:)
So to clarify for others any current caller to
find_class_from_class_loader that passes true for "init" was already
implicitly asking for a linked class (as you must be linked before
you can be initialized). With that in mind I would suggest that in
find_class_from_class_loader you add an assert:
! jclass find_class_from_class_loader(JNIEnv* env, Symbol* name,
jboolean init, jboolean link,
Handle loader, Handle
protection_domain,
jboolean throwError, TRAPS) {
+assert((init && link) || !init, "incorrect use of init/link
arguments");
just to ensure the callers understand this.
I'm good with adding an assert to check for coherent arguments.
(Another interpretation is that if 'init' is set, then the 'link'
argument is ignored, since linking is implied).
Aside: in looking at this I've spotted another existing bug. JNI
FindClass is not specified to perform class initialization, but the
implementation passes init=true!
OK, thanks. I've noted this in JDK-8226540.
src/java.base/share/classes/java/lang/invoke/MethodHandles.java
I don't see the need for the new note given it already has
* @throws LinkageError if the linkage fails
(Discussed in the CSR)
src/java.base/share/classes/sun/launcher/LauncherHelper.java
... > Is AccessControlException the only exception, that is not a
LinkageError, that might be thrown when linking? I would think a
number of others are possible - in particular IllegalAccessError
might occur during verification. Other than the fact a test obviously
triggered this, it's not clear why ACE should be singled out here. ??
Also discussed in the CSR[1].
test/hotspot/jtreg/serviceability/jvmti/ClassStatus/ClassStatus.java
45 // public static void foo(Foo f) { }
Unclear why this exists. Also the test refers initially to class Foo
but then uses Foo2 and Foo3. ??
I'm guessing it's just a leftover from an earlier version of the test.
I've removed the comment, and updated the test @summary.
Serguei, anything to add?
There is also a CSR[2] in need of review.
I've added a couple of comments and will add myself as a reviewer
once things are near finalized.
Thanks for taking a look.
Updated webrev at:
http://cr.openjdk.java.net/~bchristi/8212117/webrev10/
-Brent
1.
https://bugs.openjdk.java.net/browse/JDK-8222071?focusedCommentId=14288303&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14288303
The spec for the 2-arg and 3-arg Class.forName() methods states they
will "locate, load, and link" the class, however the linking part is
not ensured to happen.
Although the VM spec allows flexibility WRT when classes are linked,
this is a corner where the Class.forName() spec is not being upheld.
While this is not an issue for most usages, 8181144 [3] demonstrates
how this can be a problem (with the debugging interface, in this case).
This fix ensures that linking happens during the course of
Class.forName(). Class.forName() already @throws LinkageError, so
no spec change is needed there. MethodHandles.Lookup.findClass()
needs a small spec update, due to calling Class.forName with
init=false,
Of course Errors are not required to be caught. It is therefore
possible that the new behavior could introduce previously unseen,
possibly unhandled LinkageErrors. A new VM flag
(-XX:+ClassForNameDeferLinking) is introduced to restore the
previous behavior (to keep such code running until it can be updated).
This change surfaced a couple new "A JNI error has occurred"
situations (see 8181033[5]) in the Launcher, by way of
test/jdk/tools/launcher/MainClassCantBeLoadedTest.java
(using the 3-arg Class::forName, detailed in the bug report[4]),
and
test/jdk/tools/launcher/modules/basic/LauncherErrors.java
(using the 2-arg Class::forName).
The Launcher is updated to maintain non-confusing error messages :).
The new test included with this fix ensures that 8181144[3] is
addressed. Thanks go to Serguei Spitsyn for writing the test.
Automated corelibs and hotspot tests pass cleanly.
Thanks,
-Brent
--
1. https://bugs.openjdk.java.net/browse/JDK-8212117
2. https://bugs.openjdk.java.net/browse/JDK-8222071
3. https://bugs.openjdk.java.net/browse/JDK-8181144
4.
https://bugs.openjdk.java.net/browse/JDK-8212117?focusedCommentId=14215986&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14215986
5. https://bugs.openjdk.java.net/browse/JDK-8181033
