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

[Lois Foltan]

On 1/19/2017 11:54 AM, Karen Kinnear wrote:
> 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 
>> <mailto: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/ 
>>> <http://cr.openjdk.java.net/%7Ehseigel/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.

Yes it is deliberate and 
hotspot/test/runtime/modules/ModuleStress/ExportModuleStressTest.java is 
a good test of exactly this scenario.
Lois

> 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
>