RFR(S): 8166931: Do not include classes which are unusable during run time in the classlist file

[Lois Foltan]

On 10/11/2016 1:19 PM, Calvin Cheung wrote:
> Hi Lois,
>
> Thanks for your review.
>
> On 10/11/16, 4:38 AM, Lois Foltan wrote:
>>
>> On 10/10/2016 11:59 PM, Calvin Cheung wrote:
>>>
>>> Please review this small fix for not including classes in the 
>>> classlist file which are unusable during run time.
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8166931
>>>
>>> webrev: http://cr.openjdk.java.net/~ccheung/8166931/webrev.00/
>>
>> Hi Calvin,
>>
>> src/share/vm/classfile/classFileParser.cpp
>> - line #5781, I find the if statement logic to be somewhat 
>> confusing.  This check seems to be only for classes defined to the 
>> boot and platform class loader.  I am assuming it does not apply to 
>> the application class loader because there is no way to differentiate 
>> a class defined to the application class loader from being on the 
>> --patch-module list and the -classpath?  Is that why the if statement 
>> logic does not include the application class loader?
> Yes. We do want to include the classes defined to the app class loader.
Even if those classes defined to the app class loader are located in 
--patch-module entries?

>>   Maybe it is enough to improve the comment to something like:
>>
>>   // For the boot and platform class loaders, check if the class is 
>> not found in the java runtime image
>>   // or the boot loader's appended entries.  This indicates that the 
>> class must be located on the --patch-module list and
>>   // is not useable during run time, so should be skipped.
> I've modified it a little. How about the following?
> // For the boot and platform class loaders, check if the class is not 
> found in the java runtime image.
> // Additional check for the boot class loader is if the class is not 
> found in the boot loader’s appended
> // entries. This indicates that the class is not useable during run 
> time, such as the ones found in the
> // —patch-module entries, so it should not be included in the 
> classlist file.
Looks good, thanks for rewording!

>
>>
>> Then please indent the start of line #5782 by one space to show that 
>> the check for the platform class loader is part of that first || 
>> expression.
> I'll fix it.
>>
>> test/runtime/modules/PatchModule/PatchModuleClassList.java
>> - good test!
> Let me know if you want to see another webrev.

No, I'm all set.
Thanks,
Lois

>
> thanks,
> Calvin
>>
>> Thanks,
>> Lois
>>
>>>
>>> Testing:
>>>     JPRT with -testset hotspot
>>>     jtreg tests under hotspot/runtime on all supported platforms (in 
>>> progress)
>>>
>>> thanks,
>>> Calvin
>>