RFR (XS) 8185160 -XX:DumpLoadedClassList omits graal classes

[David Holmes]

Hi Ioi,

On 19/10/2017 9:54 AM, Ioi Lam wrote:
> 
> 
> On 10/17/17 9:04 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> On 18/10/2017 1:52 PM, Ioi Lam wrote:
>>> http://cr.openjdk.java.net/~iklam/jdk10/8185160-dump-class-list-omits-graal.v01/ 
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8185160
>>>
>>> Summary:
>>>
>>> DumpLoadedClassList skips classes that can't be stored into the CDS
>>> archive. For boot and platform loaders, only classes from the JRT image
>>> are listed.
>>>
>>> The test for "is this class from JRT image" was insufficient. It
>>> checked only classes whose source ends with "...../modules". For the
>>> platform loader, which loads the graal classes, the source is in the
>>> form "jrt:/jdk.internal.vm.compiler".
>>>
>>> The fix is to also check for a "jrt:" prefix.
>>
>> So ./share/classfile/classLoader.hpp:
>>
>> static bool is_jrt(const char* name) { return string_ends_with(name, 
>> MODULES_IMAGE_NAME); }
>>
>> is a badly named function or just plain broken?
>>
> 
> I've renamed all the is_jrt() functions to is_modules_image(), so it's 
> clear as to what it does.
> 
> See 
> http://cr.openjdk.java.net/~iklam/jdk10/8185160-dump-class-list-omits-graal.v02/ 

Okay. But I'm still a bit confused about the expectations of 
is_modules_image() - where are these names defined? What does jrt: mean 
if not "in a module image" ??

That aside your fix does what you needed, so looks fine.

> 
> 
>>> I also restructured the code to make it easier to read -- the nesting 
>>> of || and && is hard to follow.
>>>
>>> The original code had a subtle bug with the "!" operator:
>>>
>>> 5938   if (((class_loader == NULL && 
>>> !ClassLoader::contains_append_entry(stream->source())) ||
>>> 5939 SystemDictionary::is_platform_class_loader(class_loader)) &&
>>> 5940       !ClassLoader::is_jrt(stream->source())) {
>>>
>>> So if (class_loader == NULL && 
>>> ClassLoader::contains_append_entry(stream->source()) was true,
>>> the class would NOT be skipped.
>>>
>>> The new version removes the "!".
>>
>> So there should be an updated test to trigger the original bug.
>>
> 
> I added a test case, but it's in the closed repo for now (since the test 
> case uses utilities in the closed AppCDS tests). This test will be 
> opened as part of JDK-8185996

Ok.

Thanks,
David

> Thanks
> - Ioi
> 
>> Thanks,
>> David
>>
>>> Thanks
>>> - Ioi
>