On 9/5/19 1:36 AM, David Holmes wrote:
Hi Dan,
With my CSR Group member hat on ....
On 5/09/2019 8:06 am, Daniel D. Daugherty wrote:
Brent,
You currently have '-XX:+ClassForNameDeferLinking' as a 'product'
option, but
product options are harder to remove down the road. Would it be
better as a
diagnostic option? A diagnostic option requires
Whether a flag is product or diagnostic (or experimental) should be a
basic property of the flag based on its purpose. I would not want to
see a trend of making new flags diagnostic just because it is easier
to get rid of them later. The expectation with this fix and flag
(which I've been heavily involved in) is that production code may be
impacted by the change and need to use the flag to restore previous
behaviour. So it really is a product flag that end users should be
comfortable in using if they need
it.
This is the key phrase:
> production code may be impacted by the change and need to
> use the flag to restore previous behaviour.
Thanks for making that clear. Maybe I missed it in what I read,
but it is now clear that this should be a product flag.
Dan
The removal process for a product flag is phased (deprecate, obsolete,
expire) but not particularly onerous. There is an expectation that
this flag may be deprecated in 15 as it is intended as a transitional
flag.
Thanks,
David
-----
'-XX:+UnlockDiagnosticVMOptions'
to be specified before it can be used, e.g.:
java -XX:+UnlockDiagnosticVMOptions
-XX:+ClassForNameDeferLinking Foo
so it is a bit harder to use, but maybe that's a Good Thing (TM).
Dan
On 9/4/19 5:12 PM, Brent Christian wrote:
Hi,
Please review my fix for JDK-8212117[1]. The webrev is here:
http://cr.openjdk.java.net/~bchristi/8212117/webrev09/
There is also a CSR[2] in need of review.
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