RFR 8171971: Fix timing bug in JVM management of package export lists

[Karen Kinnear]

Harold,

Thank you for doing this. And for the comments on the states.

Looks good. I believe that the changes you have made have fixed the timing holes. Thank you.

1) packageEntry.hpp: 145: set_unqual_exported()
It would be helpful to have a comment that this explicitly sets PKG_EXP_UNQUALIFIED and clears PKG_EXP_ALLUNNAMED.

I realized my concept of states is slightly different,
with AllUnnamed, NamedExports and AllUnnamed+NamedExports being different states. In future, when we rework this
for performance, let’s revisit the states.

> On Jan 19, 2017, at 10:14 AM, Lois Foltan <lois.foltan at oracle.com> wrote:
> 
> 
> On 1/18/2017 10:05 AM, harold seigel wrote:
>> Hi,
>> 
>> Please review this fix for the package export timing holes discussed in JDK-8171971.  The fix reduces the number of PackageEntry fields that are used to maintain a package's export state and uses the Module_lock to protect all access to these fields.
>> 
>> Also, in cases where a package transitions from having qualified exports to being unqualifiedly exported, it fixes the cleanup of its qualified export list by removing the _exported_pending_delete field and using just is_unqual_exported() to determine when the qualified exports list can be purged (at a safepoint).
>> 
>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8171971/webrev/
>> 
>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8171971
>> 
>> The fix was tested with the hotspot, java/lang, java/util, java/io, JFR, and other JTReg tests, the JCK lang and VM tests, RBT tier2 - tier5 tests on LinuxX64, and the colocated and non-colocated NSK tests.
> 
> Hi Harold,
> 
> Looks good! A couple of comments in src/share/vm/classfile/packageEntry.hpp:
> 
> - line #133, the method has_qual_exports_list().  A comment should be added ahead of this method to indicate that even if the _qualified_exports list is empty the package is still considered qualifiedly exported and a similar comment should be added to PackageEntry::purge_qualified_exports().  During a GC safepoint, it could be the scenario that all modules on a given package's qualified export list die, thus leaving an allocated but empty _qualified_exports field.  If the _qualified_exports GrowableArray was deleted in that scenario, then according to the new _exports_flag and a NULL value for _qualified_exports, the package would erroneously not be considered qualifiedly exported anymore.
So not deleting the GrowableArray today is deliberate, because the transition from qualifiedly exported to not exported is not currently valid.
Agreed, we may want to revisit that in future.

2) Harold, could you possibly add that state change to the comment to the “A package cannot transition from”, i.e. after line 58?
> This can be reworked in the future to have the _exports_flag contain definitions for all possible states.  Relying on the state of whether _qualified_exports field is NULL or not can be problematic, but I think your changes do improve upon ensuring a PackageEntry maintains a valid state.
> 
> - line #86 - please add a blank line for readability
> - line #139 & #143 please out the return expression in parentheses.
> 
> Thanks,
> Lois
> 
>> 
>> Thanks, Harold

thanks,
Karen