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

[Lois Foltan]

On 10/19/2017 2:19 PM, Lois Foltan wrote:

> 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

On 2nd look, I actually think is_modules_image() is probably a more 
accurate name.  So I'm good with your changes.  We are using 
ClassLoader::has_jrt_entry() to distinguish between a java runtime 
images vs. a exploded builds.  Sorry for the confusion.
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
>>>>
>>
>