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

[harold seigel]

Hi,

Here is an updated webrev containing the changes that you suggested.

    http://cr.openjdk.java.net/~hseigel/bug_8171971.2/webrev/index.html

Could you take another look?

Thanks, Harold

On 1/19/2017 1:37 PM, harold seigel wrote:
> Hi Karen, Lois,
>
> Thanks for the review!  Please see comments in-line.
>
> Harold
>
>
> 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.
> Done
>>
>> 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.
> Agreed
>>
>>> 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.
> Good point.  I added the comments.
>> 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?
> Done
>>> 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
> okay
>>> - line #139 & #143 please out the return expression in parentheses.
> (okay)
>>>
>>> Thanks,
>>> Lois
>>>
>>>>
>>>> Thanks, Harold
>>
>> thanks,
>> Karen
>>
>