RFR 8170870: https://bugs.openjdk.java.net/browse/JDK-8170870

[serguei.spitsyn at oracle.com]

Hi Harold,

I have a couple of comments so far.

http://cr.openjdk.java.net/~hseigel/bug_8170870/webrev/src/share/vm/classfile/packageEntry.cpp.udiff.html

  void PackageEntry::package_exports_do(ModuleClosure* const f) {
- assert_locked_or_safepoint(Module_lock);
    assert(f != NULL, "invariant");
-
+ assert(Module_lock->owned_by_self(), "should have the Module_lock");


The function is used here:
   closed/share/vm/jfr/jfrModules.cpp: 
package->package_exports_do(&qexports);

static void module_export_event_callback(PackageEntry*const package) {
   assert_locked_or_safepoint(Module_lock);
   assert(package != NULL,"invariant");

   if (package->is_exported()) {
     if (package->has_qual_exports_list()) {
       . . .
       package->package_exports_do(&qexports);
       return;
     }


It seems, the module_export_event_callback() is the only context where 
the package_exports_do is called.
But that context has the assert:
assert_locked_or_safepoint(Module_lock);


It means that it can be at a safepoint with the unlocked Module_lock.
In such a case, the new assert in the package_exports_do() would fire.
(I do not see that your closed webrev updates the 
closed/share/vm/jfr/jfrModules.cpp).


And one question:

  void PackageEntryTable::purge_all_package_exports() {
    assert(SafepointSynchronize::is_at_safepoint(), "must be at safepoint");
+ if (table_size() > 0) {
+ MutexLocker module_lock(Module_lock);


The purge_all_package_exports() is called at a safepoint and it holds 
the Module_lock.
This is probably Ok because the critical section does not escape the 
safepoint
and also no thread is allowed to hold the Module_lock over a safepoint.
Could you confirm that my understanding is correct here?


Thanks,
Serguei


On 12/7/16 10:43, harold seigel wrote:
> Hi,
>
> Please review this fix for bug JDK-8170870.  The fix synchronizes 
> access to the class loaders' PackageEntryTable and ModuleEntryTable 
> structures by requiring callers of ClassLoaderData::modules_do() and 
> ClassLoaderData::packages_do() to lock the Module_lock.
>
> The lock is needed because even during a safepoint one thread could be 
> removing items from the tables while another thread is reading the 
> tables.  For example, unloading a class could cause an event 
> collection framework's JVM support to read the tables during a 
> ClassUnload event while the VM is removing entries from these 
> structures for modules and packages whose class loader got unloaded.
>
> The fix also cleans up management of package qualified export lists.
>
> Open Webrev: 
> http://cr.openjdk.java.net/~hseigel/bug_8170870/webrev/index.html
>
> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8170870
>
> The fix was tested with the JCK Lang and vm tests, the JTreg hotspot, 
> java/io, java/lang, and java/util tests and the nsk cololocated and 
> the non-colocated tests.
>
> Thanks, Harold
>