Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split [v3]

[Ioi Lam]

On Sat, 13 Mar 2021 06:16:37 GMT, Ioi Lam  wrote:

> But I don't understand why this error can happen. It seems like jtreg would 
> allow two test cases to interfere with each other.

The root cause seems to be 
https://bugs.openjdk.java.net/browse/CODETOOLS-7902847

-

PR: https://git.openjdk.java.net/jdk/pull/2985

Re: RFR: 8263549: 8263412 can cause jtreg testlibrary split

[Ioi Lam]

On Sat, 13 Mar 2021 04:31:31 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review this dull patch that replaces `ClassFileInstaller` w/ 
> `jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to 
> ensure we won't get split testlibrary, and removes 
> `jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used).
> 
> from JBS:
>> after JDK-8263412, we might (again) encounter NCDFE b/c parts of 
>> testlibraries aren't on the classpath. this happens when jtreg builds 
>> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, 
>> but `ClassFileInstaller` as part of shared testibrary directory in one test, 
>> when in the following test, jtreg sees `ClassFileInstaller` in the shared 
>> directory, hence javac won't recompile it/its dependencies, but in runtime 
>> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we 
>> get NCDFE. 
> 
> testing:
> - [x]  `grep ' ClassFileInstaller[^.]`
> - [ ] tier1-3
> 
> Thanks,
> -- Igor

I did this and scanned the differences (with the diff file from the webrev) and 
it looks reasonable to me.

grep '^[+-]' diff.txt | grep -v Copyright | grep -v '^.[+-]' | less

It looks like most of the changes are mechanical. There were only a few cases 
where manual changes were made. I trusted that you have tested those cases 
individually.

But I don't understand why this error can happen. It seems like jtreg would 
allow two test cases to interfere with each other.

-

Marked as reviewed by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2985

Re: RFR: 8263412: ClassFileInstaller can't be used by classes outside of default package

[Ioi Lam]

On Thu, 11 Mar 2021 05:47:00 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review the patch which moves `ClassFileInstaller` class to 
> `jdk.test.lib.helpers` package? 
> to reduce changes in the tests, `ClassFileInstaller` in the default package 
> is kept w/ just `main` method that calls  `jdk.test.lib.helpers. 
> ClassFileInstaller::main`. 
> 
> from JBS:
>> ClassFileInstaller is in the default package hence it's impossible to use it 
>> directly by classes in any other package. although ClassFileInstaller is 
>> mainly used directly from jtreg test description, there are cases when one 
>> needs to use it in another "driver" class (e.g. to reduce boilerplate), yet 
>> to do. that these driver classes have to either be in a default package or 
>> use reflection, both approaches have drawbacks. 
>> 
>> to solve that, we can move ClassFileInstaller implementation to a package, 
>> and to avoid unneeded churn, keep package-less ClassFileInstaller class 
>> which will call package-full impl.
>>
>> the need for this patch has raised as part of 
>> [JDK-8254129](https://bugs.openjdk.java.net/browse/JDK-8254129).
> 
> testing:
> - [x] `:jdk_core` against `{macos,windows,linux}-x64`
> - [x] `:jdk_svc` against `{macos,windows,linux}-x64`
> - [x] `test/hotspot/jtreg` w/o `applications` and `vmTestbase` directories 
> against `{macos,windows,linux}-x64`
> 
> Thanks,
> -- Igor

The CDS test changes look good to me. It seems they are just adding a single 
line of import.

-

Marked as reviewed by iklam (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2932

Re: RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64

[Ioi Lam]

On Fri, 25 Sep 2020 17:15:01 GMT, Daniel D. Daugherty  
wrote:

> Reduce noise in CI Tier2 by ProblemListing this test.

Marked as reviewed by iklam (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/362

Re: RFR: 8253659: ProblemList sun/security/ec/TestEC.java on linux-aarch64

[Ioi Lam]

On Fri, 25 Sep 2020 19:31:36 GMT, Ioi Lam  wrote:

>> Reduce noise in CI Tier2 by ProblemListing this test.
>
> Marked as reviewed by iklam (Reviewer).

Looks like a trivial change to me.

-

PR: https://git.openjdk.java.net/jdk/pull/362

Re: RFR: 8253208: Move CDS related code to a separate class [v2]

[Ioi Lam]

On Mon, 21 Sep 2020 18:17:55 GMT, Yumin Qi  wrote:

>> With more CDS related code added to VM, it is time to move CDS code to a 
>> separate class. CDS is the new class which is
>> specific to CDS.
>> Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8253208: Move CDS related code to a separate class

Marked as reviewed by iklam (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/261

Re: RFR: 8253208: Move CDS related code to a separate class

[Ioi Lam]

On Fri, 18 Sep 2020 23:47:56 GMT, Yumin Qi  wrote:

> With more CDS related code added to VM, it is time to move CDS code to a 
> separate class. CDS is the new class which is
> specific to CDS.
> Tests: tier1-4

src/java.base/share/classes/jdk/internal/misc/CDS.java line 52:

> 50:  * Check if CDS sharing is enabled by via the UseSharedSpaces flag.
> 51:  */
> 52: public static native boolean isCDSSharingEnabled();

I think the word CDS is redundant in the method names. How about

getRandomSeedForCDSDump() -> getRandomSeedForDumping()
isCDSDumpingEnabled() -> isDynamicDumpingEnabled()  // doesn't return true if 
we're doing a static dump
isCDSSharingEnabled() -> isSharingEnabled()

src/java.base/share/native/libjava/CDS.c line 49:

> 47: JNIEXPORT jboolean JNICALL
> 48: Java_jdk_internal_misc_CDS_isCDSDumpingEnabled(JNIEnv *env, jclass jcls) {
> 49: return JVM_IsCDSDumpingEnabled(env);

Maybe: return JVM_IsCDSDynamicDumpingEnabled(env)

-

PR: https://git.openjdk.java.net/jdk/pull/261

Re: RFR: 8244778: Archive full module graph in CDS

[Ioi Lam]

On Tue, 8 Sep 2020 15:59:33 GMT, Ioi Lam  wrote:

> This is the same patch as
> [8244778-archive-full-module-graph.v03](http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v03/)
> published in
> [hotspot-runtime-...@openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041496.html).
> The rest of the review will continue on GitHub. I will add new commits to 
> respond to comments to the above e-mail.

In response to Lois Foltain's comments on
[hotspot-runtime-...@openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-September/041616.html):

> Minor nit in moduleEntry.cpp & packageEntry.cpp when dealing with the 
> ModuleEntry's reads list and a PackageEntry's
> exports list.  The names of the methods to write and read those arrays is 
> somewhat confusing.
>
> ModuleEntry::write_archived_entry_array
> ModuleEntry::read_archived_entry_array
>
> At first I thought you were reading/writing an array of archived entries, not 
> the array within an archived entry
> itself.  I was trying to think of a better name.  Please consider adding a 
> comment at line #400 & line #417 ahead of
> those methods in moduleEntry.cpp to indicate that they are used for both 
> reading/writing a ModuleEntry's reads list and
> a PackageEntry's exports list.

I renamed the functions to `ModuleEntry's::write_growable_array` and 
`ModuleEntry::restore_growable_array`, and added
comments as you suggested. See commit
[4f90e77](https://github.com/openjdk/jdk/pull/80/commits/4f90e77207de5fc7cf09a12fb89b75087be57225)

// This function is used to archive ModuleEntry::_reads and 
PackageEntry::_qualified_exports.
// GrowableArray cannot be directly archived, as it needs to be expandable at 
runtime.
// Write it out as an Array, and convert it back to GrowableArray at runtime.
Array* 
ModuleEntry::write_growable_array(GrowableArray* array) {

> A question about this because a user's program can define modules post module 
> initialization via
> ModuleDescriptor.newModule().  See for example, tests within 
> open/test/hotspot/jtreg/runtime/module/AccessCheck.  So
> all of these tests would trigger check_cds_restrictions() if -Xshare:dump was 
> turned on.  Is that a concern?

Arbitrary user code cannot be executed during -Xshare:dump. The only way to do 
it is to use a JVMTI agent, which
requires specifying `-XX:+AllowArchivingWithJavaAgent`. You can see an example 
in the
[GCDuringDump.java](https://github.com/openjdk/jdk/blob/704f784c88ee282837c980948167e741e7227f27/test/hotspot/jtreg/runtime/cds/appcds/javaldr/GCDuringDump.java#L65)
test. If the agent tries to define an extra module, it will get an 
`UnsupportedOperationException` thrown by
`check_cds_restrictions()`.

-

PR: https://git.openjdk.java.net/jdk/pull/80

RFR: 8244778: Archive full module graph in CDS

[Ioi Lam]

This is the same patch as
[8244778-archive-full-module-graph.v03](http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v03/)
published in
[hotspot-runtime-...@openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041496.html).

The rest of the review will continue on GitHub. I will add new commits to 
respond to comments to the above e-mail.

-

Commit messages:
 - fixed trailing spaces
 - Renamed ModuleEntry::write_growable_array
 - Update to latest repo (JDK-8251557); added comments
 - 8244778: Archive full module graph in CDS

Changes: https://git.openjdk.java.net/jdk/pull/80/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=80&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8244778
  Stats: 2039 lines in 59 files changed: 1887 ins; 26 del; 126 mod
  Patch: https://git.openjdk.java.net/jdk/pull/80.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/80/head:pull/80

PR: https://git.openjdk.java.net/jdk/pull/80

Re: RFR(M) 8212605: Pure-Java implementation of AccessController.doPrivileged

[Ioi Lam]

On 10/31/18 5:13 PM, dean.l...@oracle.com wrote:

On 10/31/18 4:06 PM, David Holmes wrote:

Hi Dean,

Looking only at the hotspot changes. The removal of the DoPrivileged 
and related privileged_stack code seems okay. I have a few related 
comments:


src/hotspot/share/classfile/systemDictionary.hpp

You added the java_security_AccessController class after 
java_security_AccessControlContext. Did you actually verify where in 
the load/initialization order the AccessController class appears 
today, and where it appears after your change? (Note the comments at 
the start of WK_KLASSES_DO). Changes to the initialization order 
would be my biggest concern with this patch.




No, I did not notice that comment and assumed alphabetical order was 
OK here.  However, these classes appear to be only resolved, not 
initialized (and AccessController does not have a static initializer), 
so could you explain how the order in this list can affect 
initialization order?


I only need this in JVM_GetStackAccessControlContext, which is a 
static JNI method, so I could get the klass* from the incoming jclass 
instead of using a well-known class entry.


Hi Dean,

You're correct that those classes are only resolved, and not 
initialized. So the order shouldn't matter too much. However, the order 
of the first few classes is important, as the creation of Object, 
Serializable, Cloneable, String, etc, has to be done in a certain order.


I'm not sure whether the order of the reference classes, 292 classes, 
and box classes are important. I'll experiment of getting rid of the 
separate phases after the call to Universe::fixup_mirrors(). This might 
be relics from old days where the classes were once indeed initialized 
in SystemDictionary::initialize_well_known_classes, which was the old 
name for SystemDictionary::resolve_well_known_classes.


(-XX:+TraceBytecodes shows no Java code is executed before 
resolve_well_known_classes returns).


I filed https://bugs.openjdk.java.net/browse/JDK-8213230

For the time being, I think as long as you put the new AccessController 
class near the existing class AccessControlContext, you should be OK.


Thanks
- Ioi



---

I'm unclear about the change to the test:

test/hotspot/jtreg/runtime/JVMDoPrivileged/DoPrivRunAbstract.jasm

as it still refers to the now non-existent JVM_DoPrivileged in the 
summary. Does this test still make sense? Should it be moved to the 
Java side now it doesn't actually test anything in the VM?




I think these tests still make sense, unless we have similar coverage 
somewhere else.  How about if I fix the reference to JVM_DoPrivileged 
for now and file a separate bug about moving or removing these tests?



---

There may be further dead code to remove now:

vmSymbols.hpp: codesource_permissioncollection_signature is now 
unreferenced and can be removed.


javaClasses.*:
  - java_lang_System::has_security_manager
  - java_security_AccessControlContext::is_authorized

./share/memory/universe.hpp:  static Method* 
protection_domain_implies_method()




Good catches!  I have uploaded a new webrev:

http://cr.openjdk.java.net/~dlong/8212605/webrev.2/
http://cr.openjdk.java.net/~dlong/8212605/webrev.2.incr/ (incremental 
diff)


dl


---

Thanks,
David


On 1/11/2018 8:23 AM, dean.l...@oracle.com wrote:

https://bugs.openjdk.java.net/browse/JDK-8212605
http://cr.openjdk.java.net/~dlong/8212605/webrev.1

This change implements AccessController.doPrivileged in Java. This 
gives a performance improvement while also being useful to Project 
Loom by removing the Java --> native --> Java transition.  One 
reason doPrivileged has historically been in native is because of 
the need to guarantee the cleanup of the privileged context when 
doPrivileged returns.  To do that in Java, the information that 
AccessController.getContext needs is pushed onto the Java stack as 
arguments to a method that getContext will recognize during its 
stack walk.  This allows us to remove the native privileged stack 
while guaranteeing that the privileged context goes away when the 
method returns.


Tested with tier1-tier3 hotspot and jdk tests and JCK 
api/java_security tests.  For the first few rounds of testing, I 
kept the old native privileged stack and compared the results of the 
old and new implementations for each getContext call, which did 
catch some early bugs.  The changes were also examined by internal 
security experts and run through additional internal security tests.


The improvement on this [1] doPrivileged microbenchmark is 
approximate 50x.


There is no attempt to optimize getContext() or security permission 
checks in this change, however, this is intended to be a first step 
towards other possible improvements, for example those proposed here 
[2].


dl

[1] 
http://hg.openjdk.java.net/code-tools/jmh-jdk-microbenchmarks/file/fc4783360f58/src/main/java/org/openjdk/bench/java/security/DoPrivileged.java 

[2] 
http://mail.openjdk.java.net/pipermail/security-dev/20

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Ioi Lam]

On 6/2/17 8:44 AM, Ioi Lam wrote:



On 6/2/17 6:40 AM, Chris Hegarty wrote:

On 02/06/17 00:14, Ioi Lam wrote:

...

The gem is hidden in the compile.0.jta file. It contains something 
like:


  -sourcepath :/jdk/foobar/test/lib:

So if my test refers to a class under /test/lib, such as
jdk.test.lib.process.ProcessTools, javac will be able to locate it 
under

/jdk/foobar/test/lib/jdk/test/lib/process/ProcessTools.java, and will
build it automatically.

So really, there's no reason why the test must explicitly do an @build
of the library classes that it uses.


Sure, you're relying on the implicit compilation of dependencies
by javac. Look at the output, where it compiles the library
classes to.  It is part of the classes directory for the
individual test. That means that the library classes will need
to be compiled many many times.  The @build tag will compile
the library classes to a common output directory, where they
can be reused ( unless I'm missing something ).

-Chris.
Yes, @build will compile classes so that they can be reused. But why 
should it be the responsibility of every test to do this?


To reuse my malloc metaphore -- is it reasonable for every program 
that uses malloc to explicitly build libc?


By the way, jtreg arranges the output directory of the test by the 
directory they sit in, so


  jdk/test/foo/bar/XTest.java
  jdk/test/foo/bar/YTest.java

will all output their .class files to the same directory. Therefore, 
the amount of duplicated classes is not as bad as you might think. 
We've been omitting the @build tags in the hotspot tests and we 
haven't seen any problems.


- Ioi
To avoid repeat compilation of the library classes, a more reasonable 
solution would be:


[1] Before test execution -- scan all the selected test to find all 
libraries specified by @library tags


[2] Fully compile all the libraries into their own output directories

[3] Then, start execution of the selected tests

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Ioi Lam]

I agree with what Daniel said. Even without explicit @build tags (as in 
the reproducer in CODETOOLS-790198), if you use something like


@run main RedefineClassHelper

that would cause an implicit invocation of "@build test/lib", because 
RedefineClassHelper.java is part of test/lib


So it's not possible to avoid @build altogether (even if you're not 
using reflection).


=

More explanation of the jtreg bug:

In the CODETOOLS-790198 reproducer's case, another test 
(ModifyAnonymous.java) uses jdk.test.lib.compiler.InMemoryJavaCompiler 
without an explicit @build.


Later, when RedefineRunningMethodsWithResolutionErrors.java is executed 
and runs "@run main RedefineClassHelper", the jtreg bug causes 
classes/test/lib to be partially compiled -- RedefineClassHelper.class 
is there, but InMemoryJavaCompiler.class is missing.


Sure, according to the jtreg docs, "@build 
jdk.test.lib.compiler.InMemoryJavaCompiler" should be added to 
ModifyAnonymous.java. However, when you test fails because ANOTHER TEST 
forgets to add an @build, and you are looking at a sea of over 1000 test 
cases, you're completely lost.


So what we have is a jtreg rule that says "you should ...", but there's 
no enforcement (every test runs perfectly fine by itself), and when 
failure happens there's no diagnostic that tells you who's to blame. 
Seems like a perfect recipe for anarchy.


- Ioi


On 6/2/17 2:19 AM, Daniel Fuchs wrote:

Hi guys,

The jtreg bug really needs to be fixed.
What I hear is that adding an explicit @build in one test
can make an unrelated test that depends on the same library
but doesn't have the explicit @build fail (and possibly
randomly and intermittently depending of the order in
which tests are run).

This is very unintuitive, and the 'obvious' (thouhj maybe
wrong) fix for anyone stumbling on the issue would be to fix
the failing test by adding the explicit @build - not grep
the whole test base in search for a test that might have an
explicit @build, which as pointed elsewhere might well be
legitimate if the test is using reflection.

So until the jtreg bug is fixed, I am not at all sure that
removing all the explicit @build is the correct thing to do,
as it's still bound to make existing unrelated tests fail
randomly if new tests with an explicit @build are added
later on...

my2c

-- daniel

On 01/06/2017 23:37, Ioi Lam wrote:



On 6/1/17 1:17 PM, Igor Ignatyev wrote:
On Jun 1, 2017, at 1:20 AM, Chris Hegarty 
 wrote:


Igor,

On 1 Jun 2017, at 04:32, Igor Ignatyev  
wrote:


Hi Felix,

I have suggested the exact opposite change[1-3] to fix the same 
problem.
I’m sorry, but this is all just too confusing. After your change, 
who, or what, is

responsible for building/compiling the test library dependencies?
jtreg is responsible, there is an implicit build for each @run, and 
jtreg will analyze a test class to get transitive closure for static 
dependencies, hence you have to have @build only for classes which 
are not in constant pool, e.g. used only by reflection or whose 
classnames are only used to spawn a new java instance.



I suspect the problem is caused by a long standing bug in jtreg that 
results in library classes being partially compiled. Please see my 
evaluation in


https://bugs.openjdk.java.net/browse/CODETOOLS-7901986

In the bug report, there is test case that can reliably reproduce the 
NoClassDefFoundError problem.


I think adding all the @build commands in the tests are just 
band-aids. Things will break unless every test explicitly uses @build 
to build every class in every library that they use, including all 
the private classes that are not directly accessible by the test cases.


For example: doing this may be enough for now:

  * @build jdk.test.lib.process.*

But what if in the future, jdk.test.lib.process is restructured to 
have a private package jdk.test.lib.process.hidden? To work around 
CODETOOLS-7901986, all the test cases that must be modified to the 
following, which unnecessarily exposes library implementation details 
to the library users:


  * @build jdk.test.lib.process.* jdk.test.lib.process.hidden.*

Just imagine this -- "in order to use malloc() you must explicitly 
build not only malloc(), but also sbrk() ... and every other function 
in libc". That seems unreasonable to me.


By the way, we made a fix in the HotSpot tests (see 
https://bugs.openjdk.java.net/browse/JDK-8157957) that got rid of 
many (but not all) of the NoClassDefFoundErrors by *removing* the 
@build lines .


My proposal is, instead of just adding @build for band-aid, we should 
fix CODETOOLS-7901986 instead.


Thanks
- Ioi




Test library code has no @modules tags, so does not explicitly 
declare its

module dependencies. Instead module dependencies, required by test
library code, are declared in the test using the library. If we 
wildcard, or

otherwise leave br

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Ioi Lam]

On 6/2/17 6:40 AM, Chris Hegarty wrote:

On 02/06/17 00:14, Ioi Lam wrote:

...

The gem is hidden in the compile.0.jta file. It contains something like:

  -sourcepath :/jdk/foobar/test/lib:

So if my test refers to a class under /test/lib, such as
jdk.test.lib.process.ProcessTools, javac will be able to locate it under
/jdk/foobar/test/lib/jdk/test/lib/process/ProcessTools.java, and will
build it automatically.

So really, there's no reason why the test must explicitly do an @build
of the library classes that it uses.


Sure, you're relying on the implicit compilation of dependencies
by javac. Look at the output, where it compiles the library
classes to.  It is part of the classes directory for the
individual test. That means that the library classes will need
to be compiled many many times.  The @build tag will compile
the library classes to a common output directory, where they
can be reused ( unless I'm missing something ).

-Chris.
Yes, @build will compile classes so that they can be reused. But why 
should it be the responsibility of every test to do this?


To reuse my malloc metaphore -- is it reasonable for every program that 
uses malloc to explicitly build libc?


By the way, jtreg arranges the output directory of the test by the 
directory they sit in, so


  jdk/test/foo/bar/XTest.java
  jdk/test/foo/bar/YTest.java

will all output their .class files to the same directory. Therefore, the 
amount of duplicated classes is not as bad as you might think. We've 
been omitting the @build tags in the hotspot tests and we haven't seen 
any problems.


- Ioi

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Ioi Lam]

On 6/1/17 1:17 PM, Igor Ignatyev wrote:

On Jun 1, 2017, at 1:20 AM, Chris Hegarty  wrote:

Igor,


On 1 Jun 2017, at 04:32, Igor Ignatyev  wrote:

Hi Felix,

I have suggested the exact opposite change[1-3] to fix the same problem.

I’m sorry, but this is all just too confusing. After your change, who, or what, 
is
responsible for building/compiling the test library dependencies?

jtreg is responsible, there is an implicit build for each @run, and jtreg will 
analyze a test class to get transitive closure for static dependencies, hence 
you have to have @build only for classes which are not in constant pool, e.g. 
used only by reflection or whose classnames are only used to spawn a new java 
instance.


Just to add to what Igor said:

In fact, what JTREG does is fairly simple, but kind of hidden so it's 
not obvious what it does with the @library tags.


Let's say your test uses "@library /test/lib"

After your test completes, open the .jtr file (JTREG Report). It should 
have a command-line for javac, like this:


DISPLAY=:2 \\
HOME=/home/iklam \\
JTREG_COMPILEJDK=/home/iklam/jdk/bld/foobar/images/jdk \\
LANG=en_US.UTF-8 \\
LD_LIBRARY_PATH=/home/iklam/jdk/bld/foobar/images/jdk/../../images/test/hotspot/jtreg/native 
\\

PATH=/bin:/usr/bin \\
/home/iklam/jdk/bld/foobar/images/jdk/bin/javac \\
-J-Dtest.src=/jdk/foobar/hotspot/test/runtime/SharedArchiveFile \\
-J-Dtest.src.path=/jdk/foobar/hotspot/test/runtime/SharedArchiveFile:/jdk/foobar/test/lib 
\\

-J-Dtest.classes=/jdk/tmp/jtreg/work/classes/14/runtime/SharedArchiveFile \\
-J-Dtest.class.path=/jdk/tmp/jtreg/work/classes/14/runtime/SharedArchiveFile:/jdk/tmp/jtreg/work/classes/14/test/lib 
\\

-J-Dtest.vm.opts= \\
-J-Dtest.tool.vm.opts= \\
-J-Dtest.compiler.opts= \\
-J-Dtest.java.opts= \\
-J-Dtest.jdk=/home/iklam/jdk/bld/foobar-fastdebug/images/jdk \\
-J-Dcompile.jdk=/home/iklam/jdk/bld/foobar/images/jdk \\
-J-Dtest.timeout.factor=4.0 \\
-J-Dtest.modules='java.base/jdk.internal.misc java.management' \\
-J-Dtest.nativepath=/home/iklam/jdk/bld/foobar/images/jdk/../../images/test/hotspot/jtreg/native 
\\

@/jdk/tmp/jtreg/work/runtime/SharedArchiveFile/SpaceUtilizationCheck.d/compile.0.jta

The gem is hidden in the compile.0.jta file. It contains something like:

  -sourcepath :/jdk/foobar/test/lib:

So if my test refers to a class under /test/lib, such as 
jdk.test.lib.process.ProcessTools, javac will be able to locate it under 
/jdk/foobar/test/lib/jdk/test/lib/process/ProcessTools.java, and will 
build it automatically.


So really, there's no reason why the test must explicitly do an @build 
of the library classes that it uses.


- Ioi


Test library code has no @modules tags, so does not explicitly declare its
module dependencies. Instead module dependencies, required by test
library code, are declared in the test using the library. If we wildcard, or
otherwise leave broad build dependencies, from tests then there is no
way to know what new module dependencies may be added in the future.
That is, one of, the reason(s) I asked Felix to be explicit about the build
dependencies.

having explicit builds does not really help w/ module dependency, if someone 
change a testlibrary class so it starts to depend on another testlibrary class, 
jtreg will implicitly build it and if this class has some module dependencies, 
you will have to reflect them in the test.

generally speaking, I don't like having explicit build actions because build 
actions themselves are implicit, so they don't really help, it's still will be 
hard to spot missed explicit builds. not having (unneeded) explicit builds is 
an easy rule to follow and we can easily find all places which don't follow 
this rule by grep.

-- Igor

-Chris.


[1] https://bugs.openjdk.java.net/browse/JDK-8181391
[2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-June/048012.html
[3] http://cr.openjdk.java.net/~iignatyev//8181391/webrev.00/index.html

Re: RFR 8181299/10, Several jdk tests fail with java.lang.NoClassDefFoundError: jdk/test/lib/process/StreamPumper

[Ioi Lam]

On 6/1/17 1:17 PM, Igor Ignatyev wrote:

On Jun 1, 2017, at 1:20 AM, Chris Hegarty  wrote:

Igor,


On 1 Jun 2017, at 04:32, Igor Ignatyev  wrote:

Hi Felix,

I have suggested the exact opposite change[1-3] to fix the same problem.

I’m sorry, but this is all just too confusing. After your change, who, or what, 
is
responsible for building/compiling the test library dependencies?

jtreg is responsible, there is an implicit build for each @run, and jtreg will 
analyze a test class to get transitive closure for static dependencies, hence 
you have to have @build only for classes which are not in constant pool, e.g. 
used only by reflection or whose classnames are only used to spawn a new java 
instance.



I suspect the problem is caused by a long standing bug in jtreg that 
results in library classes being partially compiled. Please see my 
evaluation in


https://bugs.openjdk.java.net/browse/CODETOOLS-7901986

In the bug report, there is test case that can reliably reproduce the 
NoClassDefFoundError problem.


I think adding all the @build commands in the tests are just band-aids. 
Things will break unless every test explicitly uses @build to build 
every class in every library that they use, including all the private 
classes that are not directly accessible by the test cases.


For example: doing this may be enough for now:

 * @build jdk.test.lib.process.*

But what if in the future, jdk.test.lib.process is restructured to have 
a private package jdk.test.lib.process.hidden? To work around 
CODETOOLS-7901986, all the test cases that must be modified to the 
following, which unnecessarily exposes library implementation details to 
the library users:


 * @build jdk.test.lib.process.* jdk.test.lib.process.hidden.*

Just imagine this -- "in order to use malloc() you must explicitly build 
not only malloc(), but also sbrk() ... and every other function in 
libc". That seems unreasonable to me.


By the way, we made a fix in the HotSpot tests (see 
https://bugs.openjdk.java.net/browse/JDK-8157957) that got rid of many 
(but not all) of the NoClassDefFoundErrors by *removing* the @build 
lines .


My proposal is, instead of just adding @build for band-aid, we should 
fix CODETOOLS-7901986 instead.


Thanks
- Ioi




Test library code has no @modules tags, so does not explicitly declare its
module dependencies. Instead module dependencies, required by test
library code, are declared in the test using the library. If we wildcard, or
otherwise leave broad build dependencies, from tests then there is no
way to know what new module dependencies may be added in the future.
That is, one of, the reason(s) I asked Felix to be explicit about the build
dependencies.

having explicit builds does not really help w/ module dependency, if someone 
change a testlibrary class so it starts to depend on another testlibrary class, 
jtreg will implicitly build it and if this class has some module dependencies, 
you will have to reflect them in the test.

generally speaking, I don't like having explicit build actions because build 
actions themselves are implicit, so they don't really help, it's still will be 
hard to spot missed explicit builds. not having (unneeded) explicit builds is 
an easy rule to follow and we can easily find all places which don't follow 
this rule by grep.

-- Igor

-Chris.


[1] https://bugs.openjdk.java.net/browse/JDK-8181391
[2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-June/048012.html
[3] http://cr.openjdk.java.net/~iignatyev//8181391/webrev.00/index.html