Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
Thank you for the suggestions, Mandy and David. I've pushed the change. -Brent
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
On 16/11/2019 4:38 am, Brent Christian wrote: On 11/14/19 4:46 PM, Mandy Chung wrote: On 11/14/19 4:42 PM, David Holmes wrote: If you really want to test both positive and negative cases from a clean slate then I would suggest modifying the test slightly and using two @run commands - one to try to initialize and one to not. Yes this is what I was thinking. Two separate @run commands with an argument to indicate if initialize is true or false would do it. That sounds good. Test updated here: http://cr.openjdk.java.net/~bchristi/8233272/webrev-04/ Minor optimisations: 35 * @compile MissingClass.java 36 * @compile Container.java 37 * 38 * @run main/othervm ClassFileInstaller -jar classes.jar Container Container$1 You can use a single @compile line to compile both classes. You can use "@run driver" for ClassfileInstaller. No need to see updated webrev. Thanks, David Thanks, -Brent
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
On 11/15/19 10:38 AM, Brent Christian wrote: That sounds good. Test updated here: http://cr.openjdk.java.net/~bchristi/8233272/webrev-04/ Looks good. Minor: an additional check to consider is to check if NCDFE's cause whose message contains "MissingClass" just to be sure. No new webrev needed. Mandy
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
On 11/14/19 4:46 PM, Mandy Chung wrote: On 11/14/19 4:42 PM, David Holmes wrote: If you really want to test both positive and negative cases from a clean slate then I would suggest modifying the test slightly and using two @run commands - one to try to initialize and one to not. Yes this is what I was thinking. Two separate @run commands with an argument to indicate if initialize is true or false would do it. That sounds good. Test updated here: http://cr.openjdk.java.net/~bchristi/8233272/webrev-04/ Thanks, -Brent
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
On 11/14/19 4:42 PM, David Holmes wrote: On 15/11/2019 10:33 am, Brent Christian wrote: On 11/14/19 4:12 PM, David Holmes wrote: On 15/11/2019 9:58 am, Brent Christian wrote: http://cr.openjdk.java.net/~bchristi/8233272/webrev-03/ Test is fine. Just one note/clarification: 63 // Loading (but not linking) Container will succeed. Container was already loaded as part of the failing forName call, so this second forName will just return it. Hmm. I could use a different classloader instance for the second Class.forName() call. If you really want to test both positive and negative cases from a clean slate then I would suggest modifying the test slightly and using two @run commands - one to try to initialize and one to not. Yes this is what I was thinking. Two separate @run commands with an argument to indicate if initialize is true or false would do it. Mandy
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
On 15/11/2019 10:33 am, Brent Christian wrote: On 11/14/19 4:12 PM, David Holmes wrote: On 15/11/2019 9:58 am, Brent Christian wrote: http://cr.openjdk.java.net/~bchristi/8233272/webrev-03/ Test is fine. Just one note/clarification: 63 // Loading (but not linking) Container will succeed. Container was already loaded as part of the failing forName call, so this second forName will just return it. Hmm. I could use a different classloader instance for the second Class.forName() call. If you really want to test both positive and negative cases from a clean slate then I would suggest modifying the test slightly and using two @run commands - one to try to initialize and one to not. Cheers, David (The test does fail as expected using a build with 8212117 but without 8233091.) -Brent
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
On 11/14/19 4:12 PM, David Holmes wrote: On 15/11/2019 9:58 am, Brent Christian wrote: http://cr.openjdk.java.net/~bchristi/8233272/webrev-03/ Test is fine. Just one note/clarification: 63 // Loading (but not linking) Container will succeed. Container was already loaded as part of the failing forName call, so this second forName will just return it. Hmm. I could use a different classloader instance for the second Class.forName() call. (The test does fail as expected using a build with 8212117 but without 8233091.) -Brent
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
Hi Brent,
On 15/11/2019 9:58 am, Brent Christian wrote:
On 11/14/19 8:22 AM, Mandy Chung wrote:
On 11/13/19 10:37 AM, Brent Christian wrote:
The spec change looks fine.
OK, thanks.
+1 from me on spec changes.
As for the test, I expect that it simply calls
Class.forName("Provider", false, ucl) and then should succeed.
Then calling Class.forName("Provider", true, ucl) should fail with an
error (I think it's EIIE with NCDFE?). This way it verifies that
initialization/linking does cause NCDFE during verification while
Class.forName does not do linking if initialize=false.
Yes, that works well, thanks for the idea (plus I can do it with one
fewer class):
http://cr.openjdk.java.net/~bchristi/8233272/webrev-03/
Test is fine. Just one note/clarification:
63 // Loading (but not linking) Container will succeed.
Container was already loaded as part of the failing forName call, so
this second forName will just return it.
Thanks,
David
-
-Brent
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
On 11/14/19 8:22 AM, Mandy Chung wrote:
On 11/13/19 10:37 AM, Brent Christian wrote:
The spec change looks fine.
OK, thanks.
As for the test, I expect that it simply calls Class.forName("Provider",
false, ucl) and then should succeed.
Then calling Class.forName("Provider", true, ucl) should fail with an
error (I think it's EIIE with NCDFE?). This way it verifies that
initialization/linking does cause NCDFE during verification while
Class.forName does not do linking if initialize=false.
Yes, that works well, thanks for the idea (plus I can do it with one
fewer class):
http://cr.openjdk.java.net/~bchristi/8233272/webrev-03/
-Brent
Re: RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
On 11/13/19 10:37 AM, Brent Christian wrote:
Hi,
Recently, the 2-arg and 3-arg Class.forName() methods were updated[1]
to perform class linking, per the specification. However this change
had to be reverted[2].
Instead, let's clarify the Class.forName() spec not to guarantee
linking (outside the case of also performing initialization, of
course). This is the long-standing behavior.
I also have a test of the non-linking behavior; it's based on the test
case[3] for JDK-8231924. It fails as of 14b14 (8212117) and passes as
of 14b22 (8233091).
Please review my webrev:
http://cr.openjdk.java.net/~bchristi/8233272/webrev-02/
If the wording looks good, I'll fill in the Specification for the
CSR[4] I've started.
The spec change looks fine.
As for the test, I expect that it simply calls Class.forName("Provider",
false, ucl) and then should succeed.
Then calling Class.forName("Provider", true, ucl) should fail with an
error (I think it's EIIE with NCDFE?). This way it verifies that
initialization/linking does cause NCDFE during verification while
Class.forName does not do linking if initialize=false.
Mandy
RFR 8233272 : The Class.forName specification should be updated to match the long-standing implementation with respect to class linking
Hi, Recently, the 2-arg and 3-arg Class.forName() methods were updated[1] to perform class linking, per the specification. However this change had to be reverted[2]. Instead, let's clarify the Class.forName() spec not to guarantee linking (outside the case of also performing initialization, of course). This is the long-standing behavior. I also have a test of the non-linking behavior; it's based on the test case[3] for JDK-8231924. It fails as of 14b14 (8212117) and passes as of 14b22 (8233091). Please review my webrev: http://cr.openjdk.java.net/~bchristi/8233272/webrev-02/ If the wording looks good, I'll fill in the Specification for the CSR[4] I've started. Thanks, -Brent 1. https://bugs.openjdk.java.net/browse/JDK-8212117 2. https://bugs.openjdk.java.net/browse/JDK-8233091 3. https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-October/062747.html 4. https://bugs.openjdk.java.net/browse/JDK-8233554
