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

[Volker Simonis]

OK.

Thanks,
Volker

harold seigel <harold.seigel at oracle.com> schrieb am Mi. 7. Feb. 2018 um
18:14:

> Hi Volker,
>
> We did look at the uses of is_builtin_class_loader_data() and decided
> its callers were okay with the slight behavior change.  I also ran lots
> of tests hoping to catch any issues that may have been overlooked.
>
> Also, is_builtin_class_loader_data()'s old behavior was inconsistent.
> It returned TRUE for anonymous platform and system CLD's, but FALSE for
> anonymous boot loader CLD's.  It now treats all three loaders the same.
>
> Thanks, Harold
>
>
> On 2/7/2018 11:16 AM, Volker Simonis wrote:
> > 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
> >>>>>>
>
>