8156871: Possible concurrency issue with JVM_AddModuleExports

[Christian Tornqvist]

Hi Lois,

Ok, I see why @compile won't work for you. We should probably look at moving the CompilerUtils code into one of the shared testlibrary classes at some point, I don't think your test is the last that will need this functionality.

As for testng, you can avoid using testng by simply changing the testExportModuleStress() method to be the main method and calling compileAll() from that.

Thanks,
Christian

-----Original Message-----
From: Lois Foltan [mailto:lois.foltan at oracle.com]
Sent: Wednesday, June 15, 2016 11:39 AM
To: Christian Tornqvist <christian.tornqvist at oracle.com>
Cc: 'hotspot-runtime-dev' <hotspot-runtime-dev at openjdk.java.net>
Subject: Re: 8156871: Possible concurrency issue with JVM_AddModuleExports


On 6/15/2016 9:32 AM, Christian Tornqvist wrote:
> Hi Lois,
>
> I briefly looked at the test code and I have some concerns. I see the 
> CompilerUtils class, what prevents you from using @compile in jtreg?
>
> Also, it's a testng test, in Runtime we've decided to not use testng to 
> write our tests, is there a reason for using testng here?
Hi Christian,

Thanks for reviewing the test.  CompilerUtils provides a very convenient way 
prior to the test being run, to compile a module declaration from an 
exploded build.  All that is needed is to provide the module name and the 
directory location of the top of the exploded directory that contains that 
module declaration to the CompilerUtils::compile method.
And it allows for providing the directory where the module definition once 
compiled, should be located.  As we develop more and more extensive module 
tests, I think it would be unfortunate to have to manually go through and 
need to specify for every java file that makes up that module declaration:

     @compile src/jdk.test/module-info.java
     @compile src/jdk.test/test/Main.java
     @compile src/jdk.translet/module-info.java
     @compile src/jdk.translet/translet/Main.java

And these are just simple module declarations!  If there is a better way to 
do this without the CompilerUtils functionality please let me know, I would 
be happy to change it.  testng provided a way to run CompilerUtils::compile 
prior to the actual test run.  There is precedence for this in other areas 
of the hotspot/test directory.

Thanks,
Lois

>
> Thanks,
> Christian
>
> -----Original Message-----
> From: hotspot-runtime-dev
> [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of
> Lois Foltan
> Sent: Friday, June 10, 2016 10:25 AM
> To: hotspot-runtime-dev <hotspot-runtime-dev at openjdk.java.net>
> Subject: RFR: 8156871: Possible concurrency issue with
> JVM_AddModuleExports
>
> Hello,
>
> Please review the work to finalize a concurrency issue when setting
> the exported state of a PackageEntry.  The work completed in bug
> https://bugs.openjdk.java.net/browse/JDK-8152404 actually had the side
> effect of fixing this bug as well.  Prior to JDK-8152404, each
> PackageEntry determined its exported state via two flags, one of which
> was a general _is_exported flag followed by a another more specific
> state of exportability flag that determined if the package was
> exported qualifiedly or not to a given module.  Checking these two
> flags, as the
> PackageEntry::is_* methods used to do without the Module_lock, was 
> problematic and yielded a situation where a call to add a module to a 
> PackageEntry's qualified exported entry list failed because it was 
> determined that the package was unqualifiedly exported when it really was 
> not.
>
> To complete this fix, I have removed the is_unqual_exported call prior to 
> setting a PackageEntry's exported list.  The method 
> PackageEntry::set_exported determines what the current package export 
> state is and acts correctly.  Also, the test case has been added in this 
> webrev since it is a good stress test case for JVM module support.
>
> Passes JPRT, java/lang, java/util RBT hotspot nightly.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8156871
> Open webrev:
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8156871/webrev/
>
> Thanks,
> Lois
>