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

[Lois Foltan]

On 10/18/2017 10:32 PM, Ioi Lam wrote:

>
>
> On 10/18/17 6:26 PM, David Holmes wrote:
>> 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" ??

Hi Ioi,
I think it would be better to rename "is_jrt" to 
"is_java_runtime_image".  It has nothing to do with modules.  It has 
more to do with the JDK built as a "run-time" image vs. the JDK built as 
an exploded build.
Thanks,
Lois

>>
>> That aside your fix does what you needed, so looks fine.
>>
>
> The problem we have is the ClassFileParser::source()  returns 
> "<JAVA_HOME>/lib/modules" for the boot loader (C code), and 
> "jrt:/java.compiler" for the platform loader (Java code). This should 
> probably be fixed, but I don't want to deal with that now :-(
>
>
>>>
>>>
>>>>> 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
> - Ioi
>
>> Thanks,
>> David
>>
>>> Thanks
>>> - Ioi
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks
>>>>> - Ioi
>>>
>