RFR 8190235: Clarify ClassLoaderData::is_*_class_loader_data() method implementations

[Volker Simonis]

Hi Harold,

thanks for the detailed explanation. But then
ClassLoaderData::is_builtin_class_loader_data() will now return "true"
for CLDs created as a result of calling Unsafe.defineAnonymousClass()
(which it didn't do before). So this slightly changes its behavior.
Have the consequences been looked at (i.e. is it OK for all the
callers of ClassLoaderData::is_builtin_class_loader_data() to
additionally handle (or not handle anymore) the CLDs created by
Unsafe.defineAnonymousClass()) ?

Thanks,
Volker


On Tue, Feb 6, 2018 at 8:16 PM, harold seigel <harold.seigel at oracle.com> wrote:
> Hi Volker,
>
> Thanks for looking at this.  Unfortunately, I have already pushed the
> change.
>
> A class loader, including the boot loader, can have multiple class loader
> data's.  These CLD's include the original CLD that is used normally used by
> the class loader and the CLD's that get created by calls to
> Unsafe.defineAnonymousClass().  Every successful call to
> Unsafe.defineAnonymousClass() creates another CLD for the loader that loads
> the anonymous class, even if the loader is the boot loader.
>
> The difference between is_the_null_class_loader_data() and
> is_boot_class_loader_data() is that function is_the_null_class_loader_data()
> only returns TRUE if the CLD is the boot loader's original CLD.  Function
> is_boot_class_loader_data() returns TRUE, if it is a boot loader CLD, even
> if the CLD was created as a result of a call to
> Unsafe.defineAnonymousClass().  So, the two functions are not the same.
> Perhaps, in retrospect, they could have been given better names.
>
> Thanks, Harold
>
>
>
> On 2/6/2018 1:49 PM, Volker Simonis wrote:
>>
>> Hi Harold,
>>
>> the change looks good, but why we now need two methods (i.e.
>> is_the_null_class_loader_data() and is_boot_class_loader_data()) for
>> the same thing?
>>
>> In general, I more like the name is_boot_class_loader_data(), but
>> then, shouldn't we replace all the occurrences of
>> is_the_null_class_loader_data() with is_boot_class_loader_data().
>> Introducing is_boot_class_loader_data() just for using it a single
>> time seem weird to me, or did I missed something?
>>
>> Regards,
>> Volker
>>
>>
>> On Tue, Feb 6, 2018 at 7:38 PM, harold seigel <harold.seigel at oracle.com>
>> wrote:
>>>
>>> Hi Coleen,
>>>
>>> Thanks for reviewing this.
>>>
>>> Note that the rfr adds an assert to ModuleEntry::set_loader_data() to
>>> help
>>> ensure that a module's class loader data is not anonymous.
>>>
>>> Harold
>>>
>>>
>>>
>>> On 2/6/2018 1:35 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> This looks good.   I don't see any code that assumes
>>>> is_builtin_class_loader_data means the same thing as
>>>> is_permanent_class_loader_data() but there are a couple of instances in
>>>> ModuleEntry::set_read_walk_required and
>>>> PackageEntry::set_export_walk_required, but these might be okay because
>>>> there isn't a module reads list or exports list on anonymous CLD.
>>>> thanks,
>>>> Coleen
>>>>
>>>> On 2/5/18 3:37 PM, harold seigel wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Please review this small JDK-11 RFR to clarify and clean up the
>>>>> implementations of the ClassLoaderData::is_*_class_loader_data()
>>>>> implementations.  The changes primarily consist of adding some
>>>>> comments.
>>>>>
>>>>> Open Webrev:
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8190235/webrev/index.html
>>>>>
>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8190235
>>>>>
>>>>> The changes were tested with Mach 5 tier1 - tier5 tests and JTReg JDK
>>>>> tests.
>>>>>
>>>>> Thanks, Harold
>>>>>
>