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

[harold seigel]

Thanks Karen!

I'll make the change before I push it.

Harold


On 1/20/2017 11:13 AM, Karen Kinnear wrote:
> Harold,
>
> Thank you for the review. Looks good.
>
> Can you possibly changed packageEntry.hpp lines 60->61
>    “Because transitions are only allowed from less exposure to greater exposure, the transition from
> qualifiedly exported to not exported would be considered a backward direction. Therefore the implementation
> considers a package as qualifiedly exported even if its export-list exists but is empty."
>
> I believe that the forward transition restriction will make it cleaner for us to make these into lock-free snapshots
> going forward.
>
> thanks,
> Karen
>
>> On Jan 20, 2017, at 8:56 AM, harold seigel <harold.seigel at oracle.com> wrote:
>>
>> Thanks!
>>
>> Harold
>>
>>
>> On 1/20/2017 8:55 AM, Lois Foltan wrote:
>>> Looks good.
>>> Lois
>>>
>>> On 1/19/2017 6:38 PM, harold seigel wrote:
>>>> 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
>>>>>>