Re: RFR: 8256430: add linux-x64-optimized to regular testing [v2]

[Igor Ignatyev]

> Hi all,
> 
> 
> [8256414](https://bugs.openjdk.java.net/browse/JDK-8256414) /  #1233 added 
> similar profile to submit workflow, this patch defines `linux-x64-optimized` 
> profile in jib-profile so it can be used by mach5 and added to tier1? 
> 
> Thanks
> -- Igor
> 
> cc-ing @dcubed-ojdk

Igor Ignatyev has updated the pull request incrementally with one additional 
commit since the last revision:

  build only hotspot for optimized

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1244/files
  - new: https://git.openjdk.java.net/jdk/pull/1244/files/05c735be..2d861d14

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1244&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1244&range=00-01

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1244.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1244/head:pull/1244

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

Re: RFR: 8256430: add linux-x64-optimized to regular testing [v2]

[Igor Ignatyev]

On Tue, 17 Nov 2020 19:52:49 GMT, Erik Joelsson  wrote:

>> Igor Ignatyev has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   build only hotspot for optimized
>
> Marked as reviewed by erikj (Reviewer).

folks, thanks for your reviews.

-

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

Integrated: 8256430: add linux-x64-optimized to regular testing

[Igor Ignatyev]

On Tue, 17 Nov 2020 00:31:24 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> 
> [8256414](https://bugs.openjdk.java.net/browse/JDK-8256414) /  #1233 added 
> similar profile to submit workflow, this patch defines `linux-x64-optimized` 
> profile in jib-profile so it can be used by mach5 and added to tier1? 
> 
> Thanks
> -- Igor
> 
> cc-ing @dcubed-ojdk

This pull request has now been integrated.

Changeset: d9dbd5de
Author:Igor Ignatyev 
URL:   https://git.openjdk.java.net/jdk/commit/d9dbd5de
Stats: 13 lines in 1 file changed: 13 ins; 0 del; 0 mod

8256430: add linux-x64-optimized to regular testing

Reviewed-by: kvn, dcubed, vlivanov, erikj

-

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

Re: [jdk16] RFR: 8259794: Remove EA from JDK 16 version string starting with Initial RC promotion on Feb 04, 2021(B35)

[Igor Ignatyev]

On Tue, 2 Feb 2021 18:17:55 GMT, Jesper Wilhelmsson  
wrote:

> We have our (first) RC candidate build for JDK 16 on Feb 04, 2021. We need to 
> remove the EA from version string for this build (b35) and going forward.
> 
> Pushing this for @pashh

Marked as reviewed by iignatyev (Reviewer).

-

PR: https://git.openjdk.java.net/jdk16/pull/146

Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]

[Igor Ignatyev]

On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Ahead-of-Time Compiler from JDK: 
>> 
>> - `jdk.aot` module — the `jaotc` tool 
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution 
>> - related HotSpot code guarded by `#if INCLUDE_AOT` 
>> 
>> Additionally, remove tests as well as code in makefiles related to AoT 
>> compilation.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove exports from Graal module to jdk.aot

Test changes look good to me.

-

Marked as reviewed by iignatyev (Reviewer).

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

Re: RFR: 8264806: Remove the experimental JIT compiler

[Igor Ignatyev]

On Fri, 9 Apr 2021 22:30:32 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Java-based JIT compiler (Graal) from JDK:
>> 
>> - `jdk.internal.vm.compiler` — the Graal compiler 
>> - `jdk.internal.vm.compiler.management` — Graal's `MBean`
>> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests
>> 
>> Remove Graal related code in makefiles.
>> 
>> Note, next two `module-info.java` files are preserved so that the JVMCI 
>> module `jdk.internal.vm.ci` continues to build:
>> src/jdk.internal.vm.compiler/share/classes/module-info.java
>> src/jdk.internal.vm.compiler.management/share/classes/module-info.java
>> 
>> @AlanBateman suggested that we can avoid it by using Module API to export 
>> packages at runtime . It requires changes in GraalVM's JVMCI too so I will 
>> file followup RFE to implement it.
>> 
>> Tested hs-tier1-4
>
> Thankyou, @erikj79, for review. I restored code as you asked.
> For some reasons incremental webrev shows update only in Cdiffs.

none of the full webrevs seem to render even the list of changed files? is it 
just me?

-

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

Re: RFR: 8264806: Remove the experimental JIT compiler [v2]

[Igor Ignatyev]

On Fri, 9 Apr 2021 22:26:40 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Java-based JIT compiler (Graal) from JDK:
>> 
>> - `jdk.internal.vm.compiler` — the Graal compiler 
>> - `jdk.internal.vm.compiler.management` — Graal's `MBean`
>> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests
>> 
>> Remove Graal related code in makefiles.
>> 
>> Note, next two `module-info.java` files are preserved so that the JVMCI 
>> module `jdk.internal.vm.ci` continues to build:
>> src/jdk.internal.vm.compiler/share/classes/module-info.java
>> src/jdk.internal.vm.compiler.management/share/classes/module-info.java
>> 
>> @AlanBateman suggested that we can avoid it by using Module API to export 
>> packages at runtime . It requires changes in GraalVM's JVMCI too so I will 
>> file followup RFE to implement it.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - Restore Graal Builder image makefile
>  - Merge latest changes from branch 'JDK-8264805' into JDK-8264806
>  - 8264806: Remove the experimental JIT compiler

should we remove `sun.hotspot.code.Compiler::isGraalEnabled` method and update 
a few of its users accordingly?
what about `vm.graal.enabled` `@requires` property?

-

Changes requested by iignatyev (Reviewer).

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

Re: RFR: 8264806: Remove the experimental JIT compiler [v2]

[Igor Ignatyev]

On Fri, 9 Apr 2021 22:26:40 GMT, Vladimir Kozlov  wrote:

>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related 
>> to Java-based JIT compiler (Graal) from JDK:
>> 
>> - `jdk.internal.vm.compiler` — the Graal compiler 
>> - `jdk.internal.vm.compiler.management` — Graal's `MBean`
>> - `test/hotspot/jtreg/compiler/graalunit` — Graal's unit tests
>> 
>> Remove Graal related code in makefiles.
>> 
>> Note, next two `module-info.java` files are preserved so that the JVMCI 
>> module `jdk.internal.vm.ci` continues to build:
>> src/jdk.internal.vm.compiler/share/classes/module-info.java
>> src/jdk.internal.vm.compiler.management/share/classes/module-info.java
>> 
>> @AlanBateman suggested that we can avoid it by using Module API to export 
>> packages at runtime . It requires changes in GraalVM's JVMCI too so I will 
>> file followup RFE to implement it.
>> 
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains three additional 
> commits since the last revision:
> 
>  - Restore Graal Builder image makefile
>  - Merge latest changes from branch 'JDK-8264805' into JDK-8264806
>  - 8264806: Remove the experimental JIT compiler

Marked as reviewed by iignatyev (Reviewer).

-

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

Re: RFR: 8264806: Remove the experimental JIT compiler [v2]

[Igor Ignatyev]

On Sat, 10 Apr 2021 16:36:54 GMT, Vladimir Kozlov  wrote:

>> should we remove `sun.hotspot.code.Compiler::isGraalEnabled` method and 
>> update a few of its users accordingly?
>> what about `vm.graal.enabled` `@requires` property?
>
> @iignatev  If you think that I should clean tests anyway I will file follow 
> up RFE to do that.

> > should we remove `sun.hotspot.code.Compiler::isGraalEnabled` method and 
> > update a few of its users accordingly?
> > what about `vm.graal.enabled` `@requires` property?
> 
> Thank you, @iignatev for looking on changes.
> 
> I forgot to mention that `Compiler::isGraalEnabled()` returns always false 
> now. Because 94 tests uses `@requires !vm.graal.enabled` I don't want to 
> include them in these changes which are already very big. I am not sure if I 
> should modify tests if GraalVM group wants to run all these tests.

> If you think that I should clean tests anyway I will file follow up RFE to do 
> that.

changing  `Compiler::isGraalEnabled()` to always return false effectively makes 
these tests unrunnable for GraalVM group (unless they are keep the modified 
`sun/hotspot/code/Compiler` and/or `requires/VMProps` in their forks). on top 
of that, I foresee that there will be more tests incompatible w/ Graal yet 
given it won't be run/tested in OpenJDK, these tests won't be marked and hence 
will fail when run w/ Graal. so Graal people will have to either do marking 
themselves (I guess in both upstream and their fork) or maintain a list of 
inapplicable tests in a format that works best for their setup.

that's to say, I think we should clean up the tests, yet I totally agree there 
is no need to do it as part of this PR. we can discuss how to do it better for 
both OpenJDK and GraalVM folks in the follow-up RFE.

-

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

Re: RFR: 8265782: Bump bootjdk to jdk-17+19 on macosx-aarch64 at Oracle

[Igor Ignatyev]

On Thu, 22 Apr 2021 18:15:58 GMT, Mikael Vidstedt  wrote:

> The bootjdk used for macosx-aarch64 at Oracle is a custom build of JDK 16 
> from early December, so does not include all the JDK 16 GA functionality. 
> This leads to build issues after JDK-8263621 which uses a method not included 
> in the custom version. Let's bump the bootjdk used for macosx-aarch64 to 
> jdk-17+19 instead.

Marked as reviewed by iignatyev (Reviewer).

-

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

Re: RFR: 8264806: Remove the experimental JIT compiler [v2]

[Igor Ignatyev]

On Tue, 13 Apr 2021 09:30:23 GMT, Doug Simon  wrote:

> I guess this should really be named `isJVMCICompilerEnabled` now and the 
> `vm.graal.enabled` predicate renamed to `vm.jvmcicompiler.enabled` but maybe 
> that's too big a change (or can be done later).

@dougxc, I don't think that we should (or even can) rename it to 
`vm.jvmcicompiler.enabled`. although the value of the property is indeed `true` 
whenever a jvmci compiler is enabled, it is used to filter out tests that are 
incompatible with a particular compiler -- graal. so what we can do is to 
change `requires/VMProps.java` to set `vm.graal.enabled` only if the jvmci 
compiler is indeed graal, but on the other hand, we haven't heard anyone 
complaining that the test coverage for their jvmci compiler has been reduced, 
so I don't see much of the benefit here.

-- Igor

-

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

[jdk17] RFR: 8269037: jsig/Testjsig.java doesn't have to be restricted to linux only

[Igor Ignatyev]

Hi all,

could you please review this small patch that enables 
`runtime/jsig/Testjsig.java` test and compilation of its native library on all 
platforms but windows?
from JBS:
> `runtime/jsig/Testjsig.java` test currently `@requires (os.family == 
> "linux")` and `test/hotspot/jtreg/runtime/jsig/libTestJNI.c` compilation is 
> resticted to linux only, howere there seems to be nothing in the test code 
> that prevents it from execution on a system that supports POSIX.

testing:  `runtime/jsig/Testjsig.java` on `{linux,windows,macosx}-x64`


Thanks,
-- Igor

-

Commit messages:
 - 8269037

Changes: https://git.openjdk.java.net/jdk17/pull/105/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=105&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269037
  Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/105.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/105/head:pull/105

PR: https://git.openjdk.java.net/jdk17/pull/105

[jdk17] Withdrawn: 8269037: jsig/Testjsig.java doesn't have to be restricted to linux only

[Igor Ignatyev]

On Sun, 20 Jun 2021 11:08:21 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review this small patch that enables 
> `runtime/jsig/Testjsig.java` test and compilation of its native library on 
> all platforms but windows?
> from JBS:
>> `runtime/jsig/Testjsig.java` test currently `@requires (os.family == 
>> "linux")` and `test/hotspot/jtreg/runtime/jsig/libTestJNI.c` compilation is 
>> resticted to linux only, howere there seems to be nothing in the test code 
>> that prevents it from execution on a system that supports POSIX.
> 
> testing:  `runtime/jsig/Testjsig.java` on `{linux,windows,macosx}-x64`
> 
> 
> Thanks,
> -- Igor

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk17/pull/105

Re: [jdk17] RFR: 8269037: jsig/Testjsig.java doesn't have to be restricted to linux only

[Igor Ignatyev]

On Sun, 20 Jun 2021 11:08:21 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review this small patch that enables 
> `runtime/jsig/Testjsig.java` test and compilation of its native library on 
> all platforms but windows?
> from JBS:
>> `runtime/jsig/Testjsig.java` test currently `@requires (os.family == 
>> "linux")` and `test/hotspot/jtreg/runtime/jsig/libTestJNI.c` compilation is 
>> resticted to linux only, howere there seems to be nothing in the test code 
>> that prevents it from execution on a system that supports POSIX.
> 
> testing:  `runtime/jsig/Testjsig.java` on `{linux,windows,macosx}-x64`
> 
> 
> Thanks,
> -- Igor

closed in favor of openjdk/jdk#4975

-

PR: https://git.openjdk.java.net/jdk17/pull/105

RFR: 8269037: jsig/Testjsig.java doesn't have to be restricted to linux only

[Igor Ignatyev]

Hi all,

could you please review this small patch that enables 
`runtime/jsig/Testjsig.java` test and compilation of its native library on all 
platforms but windows?
from JBS:
> `runtime/jsig/Testjsig.java` test currently `@requires (os.family == 
> "linux")` and `test/hotspot/jtreg/runtime/jsig/libTestJNI.c` compilation is 
> restricted to linux only, however there seems to be nothing in the test code 
> that prevents it from execution on a system that supports POSIX.

testing:  `runtime/jsig/Testjsig.java` on `{linux,windows,macosx}-x64`

Thanks,
-- Igor

-

Commit messages:
 - 8269037

Changes: https://git.openjdk.java.net/jdk/pull/4975/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4975&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269037
  Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4975.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4975/head:pull/4975

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

Re: RFR: 8269037: jsig/Testjsig.java doesn't have to be restricted to linux only

[Igor Ignatyev]

On Tue, 3 Aug 2021 20:18:11 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review this small patch that enables 
> `runtime/jsig/Testjsig.java` test and compilation of its native library on 
> all platforms but windows?
> from JBS:
>> `runtime/jsig/Testjsig.java` test currently `@requires (os.family == 
>> "linux")` and `test/hotspot/jtreg/runtime/jsig/libTestJNI.c` compilation is 
>> restricted to linux only, however there seems to be nothing in the test code 
>> that prevents it from execution on a system that supports POSIX.
> 
> testing:  `runtime/jsig/Testjsig.java` on `{linux,windows,macosx}-x64`
> 
> Thanks,
> -- Igor

David, Misha,

thank you for your reviews.

-

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

Integrated: 8269037: jsig/Testjsig.java doesn't have to be restricted to linux only

[Igor Ignatyev]

On Tue, 3 Aug 2021 20:18:11 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review this small patch that enables 
> `runtime/jsig/Testjsig.java` test and compilation of its native library on 
> all platforms but windows?
> from JBS:
>> `runtime/jsig/Testjsig.java` test currently `@requires (os.family == 
>> "linux")` and `test/hotspot/jtreg/runtime/jsig/libTestJNI.c` compilation is 
>> restricted to linux only, however there seems to be nothing in the test code 
>> that prevents it from execution on a system that supports POSIX.
> 
> testing:  `runtime/jsig/Testjsig.java` on `{linux,windows,macosx}-x64`
> 
> Thanks,
> -- Igor

This pull request has now been integrated.

Changeset: 34ba70a7
Author:Igor Ignatyev 
URL:   
https://git.openjdk.java.net/jdk/commit/34ba70a71ba414a6d8cfc5c667d556d4d6072793
Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod

8269037: jsig/Testjsig.java doesn't have to be restricted to linux only

Reviewed-by: mseledtsov, dholmes

-

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

Re: RFR: 8275512: Upgrade required version of jtreg to 6.1

[Igor Ignatyev]

On Tue, 19 Oct 2021 13:51:45 GMT, Weijun Wang  wrote:

> As a follow up of JEP 411, we will soon disallow security manager by default. 
> jtreg 6.1 does not set its own security manager if JDK version is >= 18.

LGTM

-

Marked as reviewed by iignatyev (Reviewer).

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

Re: RFR(S): 8197906: Enable CDS mode execution of jtreg tests via make

[Igor Ignatyev]

[1] and [2] are equivalent for cygwin, but it doesn't mean they are equivalent 
JVM. I recall there were some problems when cygwin paths passed to JVM. so to 
be on the safe side, I'd use mixed or windows path to the archive in JVM flags.

Thanks,
-- Igor 

> On Feb 14, 2018, at 7:26 AM, Mikhailo Seledtsov 
>  wrote:
> 
> Hi Igor,
> 
>  Thank you for review. This is what I see from the test execution logs on 
> Windows:
> 
> // At archive creation time:
> >  -XX:SharedArchiveFile=T:/testoutput/jtreg/cds_archive.jsa -Xshare:dump
> 
> 
> // At archive use time:
> 
> >-vmoption:-XX:+UnlockDiagnosticVMOptions 
> > -vmoption:-XX:SharedArchiveFile=/cygdrive/t/testoutput/jtreg/cds_archive.jsa
> >  -javaoptions:-Xshare:auto -javaoptions:-Xlog:class+path=trace 
> > -javaoptions:-showversion
> 
> 
> I am not a Windows expert, and especially not a wiz in Cygwin. Do you know if 
> these 2 paths are equivalent?
> [1] T:/testoutput/jtreg/cds_archive.jsa
> [2] /cygdrive/t/testoutput/jtreg/cds_archive.jsa
> 
> If these are equivalent, then the code is OK as is. If not, I will change it 
> and rerun the tests. Please let me know.
> 
> 
> Thank you,
> Misha
> 
> 
> On 2/13/18, 8:41 PM, Igor Ignatyev wrote:
>> Hi Misha,
>> 
>> have you check that it works on windows w/ GENERATE_CDS_ARCHIVE=true? it 
>> seems that you should called GETMIXEDPATH in CDS_VM_ARGS:
>>> CDS_VM_ARGS := -XX:+UnlockDiagnosticVMOptions 
>>> -XX:SharedArchiveFile=$(CDS_ARCHIVE_FILE)
>> vs
>>> CDS_VM_ARGS := -XX:+UnlockDiagnosticVMOptions -XX:SharedArchiveFile=$(shell 
>>> $(GETMIXEDPATH) "$(CDS_ARCHIVE_FILE)")
>> otherwise, tests won't be able to find cds archive during test execution.
>> 
>> -- Igor
>> 
>>> On Feb 13, 2018, at 8:17 PM, Mikhailo 
>>> Seledtsov  wrote:
>>> 
>>> Please review this small change that enables execution of any jtreg test(s) 
>>> in CDS mode
>>> via make. Please see bug description for details.
>>> 
>>> Thanks to Erik and Igor for their help with this change.
>>> 
>>>JBS: https://bugs.openjdk.java.net/browse/JDK-8197906
>>>WebRev: http://cr.openjdk.java.net/~mseledtsov/8197906.01/
>>> 
>>>Testing:
>>>  - ran hotspot_runtime in CDS mode with this change - Linux-x64 - no 
>>> new failures
>>>  - running several test sets in CDS mode via distributed test system - 
>>> pass
>>>  - running pre-integration sanity testing (HS tier1, tier2) - in 
>>> progress
>>> 
>>> Thank you,
>>> Misha
>>> 

Re: RFR(S) 8197453 : Add support of extra problem list

[Igor Ignatyev]

adding build-dev alias

-- Igor

> On Feb 8, 2018, at 3:08 PM, Ekaterina Pavlova  
> wrote:
> 
> Hi all,
> 
> ProblemList.txt files used by makefiles for jtreg testing allow to specify 
> list of tests to be excluded
> from execution on all or specific platforms. However to test such features 
> like Graal we want to be able
> to specify list of failed tests which fail in particular JVM mode only.
> Please review this change which adds support of extra problem list and 
> introduces 2 Graal specific problem list files.
> - test/hotspot/jtreg/ProblemList-graal.txt
> - test/jdk/ProblemList-graal.txt
> 
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8197453
>  webrev: http://cr.openjdk.java.net/~epavlova//8197453/webrev.00/
> testing: precheckin, tier1 and tier2 with empty EXTRA_PROBLEM_LISTS.
>  testing in Graal mode with EXTRA_PROBLEM_LISTS=ProblemList-graal.txt
> 
> thanks,
> -katya
> 
> p.s.
> Igor Ignatyev volunteered to sponsor this change.

Re: RFR(S) 8197453 : Add support of extra problem list

[Igor Ignatyev]

Hi Jon,

yes, they are. we will  generic-all to each line.

do you think it makes sense to file an RFE for jtreg to report a missed 
platform specifier as an error?

Thanks,
-- Igor 

> On Feb 26, 2018, at 12:02 PM, Jonathan Gibbons  
> wrote:
> 
> If these new problem-list files are destined for use by jtreg, I would 
> encourage adding a platform specifier on each line, after the bug number. If 
> you want to mark the test as excluded on all platforms, the convention is to 
> use "generic-all".
> 
> -- Jon
> 
> 
> On 2/26/18 11:47 AM, Igor Ignatyev wrote:
>> adding build-dev alias
>> 
>> -- Igor
>> 
>>> On Feb 8, 2018, at 3:08 PM, Ekaterina Pavlova 
>>>  wrote:
>>> 
>>> Hi all,
>>> 
>>> ProblemList.txt files used by makefiles for jtreg testing allow to specify 
>>> list of tests to be excluded
>>> from execution on all or specific platforms. However to test such features 
>>> like Graal we want to be able
>>> to specify list of failed tests which fail in particular JVM mode only.
>>> Please review this change which adds support of extra problem list and 
>>> introduces 2 Graal specific problem list files.
>>> - test/hotspot/jtreg/ProblemList-graal.txt
>>> - test/jdk/ProblemList-graal.txt
>>> 
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8197453
>>>  webrev: http://cr.openjdk.java.net/~epavlova//8197453/webrev.00/
>>> testing: precheckin, tier1 and tier2 with empty EXTRA_PROBLEM_LISTS.
>>>  testing in Graal mode with 
>>> EXTRA_PROBLEM_LISTS=ProblemList-graal.txt
>>> 
>>> thanks,
>>> -katya
>>> 
>>> p.s.
>>> Igor Ignatyev volunteered to sponsor this change.
> 

Re: RFR(S) 8197453 : Add support of extra problem list

[Igor Ignatyev]

Hi Katya,

the fix looks good to me.

-- Igor

> On Feb 27, 2018, at 9:45 AM, Ekaterina Pavlova  
> wrote:
> 
> Jon,
> 
> thanks for the review.
> I have updated the webrev.
> 
> thanks,
> -katya
> 
> 
> On 2/26/18 12:02 PM, Jonathan Gibbons wrote:
>> If these new problem-list files are destined for use by jtreg, I would 
>> encourage adding a platform specifier on each line, after the bug number. If 
>> you want to mark the test as excluded on all platforms, the convention is to 
>> use "generic-all".
>> -- Jon
>> On 2/26/18 11:47 AM, Igor Ignatyev wrote:
>>> adding build-dev alias
>>> 
>>> -- Igor
>>> 
>>>> On Feb 8, 2018, at 3:08 PM, Ekaterina Pavlova 
>>>>  wrote:
>>>> 
>>>> Hi all,
>>>> 
>>>> ProblemList.txt files used by makefiles for jtreg testing allow to specify 
>>>> list of tests to be excluded
>>>> from execution on all or specific platforms. However to test such features 
>>>> like Graal we want to be able
>>>> to specify list of failed tests which fail in particular JVM mode only.
>>>> Please review this change which adds support of extra problem list and 
>>>> introduces 2 Graal specific problem list files.
>>>> - test/hotspot/jtreg/ProblemList-graal.txt
>>>> - test/jdk/ProblemList-graal.txt
>>>> 
>>>> 
>>>>  JBS: https://bugs.openjdk.java.net/browse/JDK-8197453
>>>>   webrev: http://cr.openjdk.java.net/~epavlova//8197453/webrev.00/
>>>> testing: precheckin, tier1 and tier2 with empty EXTRA_PROBLEM_LISTS.
>>>>   testing in Graal mode with 
>>>> EXTRA_PROBLEM_LISTS=ProblemList-graal.txt
>>>> 
>>>> thanks,
>>>> -katya
>>>> 
>>>> p.s.
>>>> Igor Ignatyev volunteered to sponsor this change.
> 

Re: RFR: JDK-8199352: The Jib artifact resolver in test lib needs to print better error messages

[Igor Ignatyev]

Hi Erik,

to avoid incompatibility, you could have just made ArtifactResolverException a 
subclass of java.io.FileNotFoundException.

it seems you forgot to add ArtifactResolverException.java file to the repo.

a minor nit: in JibArtifactManager::newInstance, you pass "Could not resolve " 
+ JIB_SERVICE_FACTORY to ClassNotFoundException constructor. by the convention, 
the message in CNFE is the classname.

-- Igor

> On Mar 8, 2018, at 2:08 PM, Erik Joelsson  wrote:
> 
> The Jib artifact resolver is not very good at telling us why things go wrong. 
> The reason is that it swallows exceptions. This patch changes the API from 
> throwing a FileNotFoundException, which I don't really think fits correctly 
> in all cases, to a new API specific exception.
> 
> I have greped for all uses of this API in the tests and changed the exception 
> type caught at the caller location. I verified that I didn't break anything 
> by compiling all the affected test classes and by running some of them for a 
> bit.
> 
> With these changes it should be easier to diagnose problems with resolving 
> artifacts in the future.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8199352
> 
> Webrev: http://cr.openjdk.java.net/~erikj/8199352/webrev.01/index.html
> 
> /Erik
> 

Re: RFR: JDK-8199352: The Jib artifact resolver in test lib needs to print better error messages

[Igor Ignatyev]

Looks good to me.

-- Igor

> On Mar 8, 2018, at 4:06 PM, Erik Joelsson  wrote:
> 
> On 2018-03-08 15:24, Igor Ignatyev wrote:
>> Hi Erik,
> Thanks for looking at this!
>> to avoid incompatibility, you could have just made ArtifactResolverException 
>> a subclass of java.io.FileNotFoundException.
> This is correct, but I don't think the exception should be of type 
> FileNotFoundException. IMO, this is something different and trying to 
> re-purpose an existing exception type is rarely a good idea.
>> it seems you forgot to add ArtifactResolverException.java file to the repo.
> Doh! Added.
>> a minor nit: in JibArtifactManager::newInstance, you pass "Could not resolve 
>> " + JIB_SERVICE_FACTORY to ClassNotFoundException constructor. by the 
>> convention, the message in CNFE is the classname.
> Right, good point, fixed.
> 
> New Webrev: http://cr.openjdk.java.net/~erikj/8199352/webrev.02/
> 
> /Erik
>> -- Igor
>> 
>>> On Mar 8, 2018, at 2:08 PM, Erik Joelsson  wrote:
>>> 
>>> The Jib artifact resolver is not very good at telling us why things go 
>>> wrong. The reason is that it swallows exceptions. This patch changes the 
>>> API from throwing a FileNotFoundException, which I don't really think fits 
>>> correctly in all cases, to a new API specific exception.
>>> 
>>> I have greped for all uses of this API in the tests and changed the 
>>> exception type caught at the caller location. I verified that I didn't 
>>> break anything by compiling all the affected test classes and by running 
>>> some of them for a bit.
>>> 
>>> With these changes it should be easier to diagnose problems with resolving 
>>> artifacts in the future.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8199352
>>> 
>>> Webrev: http://cr.openjdk.java.net/~erikj/8199352/webrev.01/index.html
>>> 
>>> /Erik
>>> 
> 

RFR(XXS) : 8200180 : fix a typo in run-test framework documentation

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8200180/webrev.00/index.html
> 3 lines changed: 0 ins; 0 del; 3 mod; 


Hi all,

could you please review this small fix for run-test framework documentation? 
VM_OTIONS was used instead of VM_OPTIONS at several places, the fix is obvious 
s/VM_OTIONS/VM_OPTIONS/ .

webrev: http://cr.openjdk.java.net/~iignatyev//8200180/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8200180

Thanks,
-- Igor
 

Re: RFR(XXS) : 8200180 : fix a typo in run-test framework documentation

[Igor Ignatyev]

Magnus, Erik,

thanks for your review.

-- Igor

> On Mar 23, 2018, at 8:30 AM, Magnus Ihse Bursie 
>  wrote:
> 
> 
>> 23 mars 2018 kl. 16:25 skrev Erik Joelsson :
>> 
>> Hello Igor,
>> 
>> This looks good, but please also run "make update-build-docs" so that the 
>> html file also gets regenerated before pushing.
> 
> Looks good to me too. 
> 
> /Magnus
> 
>> 
>> /Erik
>> 
>> 
>>> On 2018-03-23 08:13, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev//8200180/webrev.00/index.html
>>>> 3 lines changed: 0 ins; 0 del; 3 mod;
>>> 
>>> Hi all,
>>> 
>>> could you please review this small fix for run-test framework 
>>> documentation? VM_OTIONS was used instead of VM_OPTIONS at several places, 
>>> the fix is obvious s/VM_OTIONS/VM_OPTIONS/ .
>>> 
>>> webrev: http://cr.openjdk.java.net/~iignatyev//8200180/webrev.00/index.html
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8200180
>>> 
>>> Thanks,
>>> -- Igor
>>> 
>> 
> 

RFR(XXS) : 8200538 : cl : Command line warning D9014 : invalid value '2220' for '/wd'

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev/8200538/webrev.00/
> 1 line changed: 0 ins; 0 del; 1 mod; 

Hi all,

could you please review this one-liner change which removes C2220 from disabled 
warning on windows? C2220 is "warning treated as error" compiler error, which 
isn't a real compiler warning/error, so it can't be used in /wd as a result, we 
get warning D9014.

webrev: http://cr.openjdk.java.net/~iignatyev/8200538/webrev.00/
jbs: https://bugs.openjdk.java.net/browse/JDK-8200538
testing: built windows, windows-debug (including open only) and grepped for 
D9014 in log  

Thanks,
-- Igor 

Re: RFR(L) : 8199643 : [TESTBUG] Open source common VM testbase code

[Igor Ignatyev]

Jerry, Misha,

thank you for your review. 

adding build-dev alias as the patch includes makefiles changes.

-- Igor

> On Apr 30, 2018, at 8:39 AM, mikhailo  wrote:
> 
> Changes look good to me,
> 
> Thank you,
> 
> Misha
> 
> 
> On 04/28/2018 07:34 AM, Gerald Thornbrugh wrote:
>> Hi Igor,
>> 
>> Your changes look good to me.
>> 
>> Thanks!
>> 
>> Jerry
>> 
>>> http://cr.openjdk.java.net/~iignatyev//8199375/webrev.00/index.html
 58155 lines changed: 58155 ins; 0 del; 0 mod;
>>> Hi all,
>>> 
>>> could you please review this webrev which open sources code shared by many 
>>> tests from so-called VM testbase?
>>> 
>>> this patch doesn't include any tests, it's rather a preparation step to 
>>> simplify actual open sourcing of the tests, which will be done later by 
>>> separate RFEs[*]. the files are intentionally put into a separate directory 
>>> (test/hotspot/jtreg/vmTestbase) to emphasize that these tests were a part 
>>> of one "product", most of the code doesn't meet openjdk coding guidelines 
>>> (or any other coding guidelines for that matter), might be highly coupled 
>>> and duplicate some existing test and/or test libraries from jtreg test 
>>> bases. in a long term, we are planning to rework all these tests and make 
>>> them more like other regular jtreg tests.
>>> 
>>> I'd like to highlight that the code in this webrev isn't new, VM testbase 
>>> tests have been using it for a long time and these tests have been by 
>>> Oracle for internal hotspot testing for a long period if time. however as 
>>> this patch adds "new" native code, I'd really like platforms' maintainers 
>>> to closely review all .h/.c files (esp. libProcessUtils.c and all the files 
>>> used by it) as they were never built/executed on platforms other than the 
>>> ones supported by Oracle.
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8199643
>>> webrev: http://cr.openjdk.java.net/~iignatyev//8199375/webrev.00/index.html
>>> testing:
>>>   - all tests which depend on this code
>>>   - build linux-x64, windows-x64, mac-x64, solaris-sparcv9 including 
>>> open-only variants
>>> 
>>> [*] JBS:(labels = test-opensource and component = hotspot)
>>> https://bugs.openjdk.java.net/issues/?jql=labels%20%3D%20test-opensource%20and%20component%20%3D%20hotspot
>>>  
>>> 
>>> Thanks,
>>> -- Igor
>> 
> 

RFR(L) : 8199375 : [TESTBUG] Open source vm testbase monitoring tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8199375/webrev.00/index.html
> 41276 lines changed: 41274 ins; 1 del; 1 mod; 

Hi all,

could you please review the patch which open sources monitoring tests from vm 
testbase?

The tests were developed to test hotspot related JMX functionality. as w/ 
common VM testbase code, these tests are old, they have been run from hotspot 
testing for a long period of time. originally, these tests were run by a test 
harness different from jtreg and had different build and execution schemes, 
some parts couldn't be easily translated to jtreg, so tests might have actions 
or pieces of code which look weird. in a long term, we are planning to rework 
them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199375
webrev:  http://cr.openjdk.java.net/~iignatyev/8199375/webrev.00/index.html
testing: vmTestbase_nsk_monitoring test group

Thanks,
-- Igor

Re: RFR(L) : 8199375 : [TESTBUG] Open source vm testbase monitoring tests

[Igor Ignatyev]

Vladimir,

we can introduce 'quick' keyword, mark these tests w/ them and use this keyword 
in test selection. I personally don't like this way either, as it uses a 
loosely defined property. it also might be possible to create a separate test 
group file and use it to define only _quick groups. I'll look into these and 
other possible options.

in any case, although it will create a temporary mess, I'd prefer not to change 
how we define these *_quick test groups or how we do test selection, until all 
vm testbase tests are open sourced as it might create unneeded complications w/ 
test execution. I'll file an RFE to not forget about it.

Thanks,
-- Igor  

> On May 2, 2018, at 10:59 AM, Vladimir Kozlov  
> wrote:
> 
> I wish we have ability to include other files with definitions into 
> TEST.group file. It is very ugly to double size of TEST.group file just for 
> that purpose.
> 
> Thanks,
> Vladimir
> 
> On 5/1/18 9:39 PM, Igor Ignatev wrote:
>> Vladimir,
>> Tests are listed only in _quick test group b/c it doesn’t include all tests 
>> from the directory. We use this group in some of our test configurations, 
>> and :vmTestbase_nsk_monitoring in others. vmTestbase_nsk_monitoring is 
>> defined by the directory as other groups.
>> Thanks,
>> — Igor
>>> On May 1, 2018, at 7:54 PM, Vladimir Kozlov  
>>> wrote:
>>> 
>>> Igor,
>>> 
>>> Why you need to list each test in TEST.groups and not just directory as we 
>>> do in other cases?
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>>> On 5/1/18 7:10 PM, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8199375/webrev.00/index.html
>>>>> 41276 lines changed: 41274 ins; 1 del; 1 mod;
>>>> Hi all,
>>>> could you please review the patch which open sources monitoring tests from 
>>>> vm testbase?
>>>> The tests were developed to test hotspot related JMX functionality. as w/ 
>>>> common VM testbase code, these tests are old, they have been run from 
>>>> hotspot testing for a long period of time. originally, these tests were 
>>>> run by a test harness different from jtreg and had different build and 
>>>> execution schemes, some parts couldn't be easily translated to jtreg, so 
>>>> tests might have actions or pieces of code which look weird. in a long 
>>>> term, we are planning to rework them.
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8199375
>>>> webrev:  http://cr.openjdk.java.net/~iignatyev/8199375/webrev.00/index.html
>>>> testing: vmTestbase_nsk_monitoring test group
>>>> Thanks,
>>>> -- Igor

RFR(L) : 8199382 : [TESTBUG] Open source VM testbase JDI tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev/8199382/webrev.00/index.html
> 577169 lines changed: 577169 ins; 0 del; 0 mod; 

Hi all,

could you please review the patch which open sources JDI tests from vm 
testbase? These tests cover different aspects of JDI implementation.

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199382
webrev:  http://cr.openjdk.java.net/~iignatyev/8199382/webrev.00/index.html
testing: vmTestbase_nsk_jdi test group

Thanks,
-- Igor

RFR(L) : 8199370: [TESTBUG] Open source vm testbase GC tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev/8199370/webrev.00/index.html
> 45710 lines changed: 45710 ins; 0 del; 0 mod;

Hi all,

could you please review the patch which open sources GC tests from vm testbase? 
it introduces the following test groups:
- vmTestbase_vm_g1classunloading
- vmTestbase_vm_gc_compact
- vmTestbase_vm_gc_concurrent
- vmTestbase_vm_gc_container
- vmTestbase_vm_gc_juggle
- vmTestbase_vm_gc_locker
- vmTestbase_vm_gc_ref
- vmTestbase_vm_gc_misc

besides these test groups, which split tests by functionality under test, the 
patch also adds test groups used only for test selection purposes:
- vmTestbase_vm_gc which includes all vmTestbase_vm_gc_* test groups
- vmTestbase_vm_gc_quick -- "quick" tests from vmTestbase_vm_gc test group
- vmTestbase_largepages which consists of tests which are believed to be good 
candidate to test largepage. this group is used in our testing w/ external vm 
flags and/or on pre-configured hosts. 

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199370
webrev: http://cr.openjdk.java.net/~iignatyev/8199370/webrev.00/index.html

Thanks,
-- Igor

Re: RFR(L) : 8199370: [TESTBUG] Open source vm testbase GC tests

[Igor Ignatyev]

Erik, Magnus, thank you for reviewing build change.

can someone from GC people Review the rest?

-- Igor

> On May 8, 2018, at 2:06 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2018-05-08 00:35, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev/8199370/webrev.00/index.html
>>> 45710 lines changed: 45710 ins; 0 del; 0 mod;
>> Hi all,
>> 
>> could you please review the patch which open sources GC tests from vm 
>> testbase? it introduces the following test groups:
>> - vmTestbase_vm_g1classunloading
>> - vmTestbase_vm_gc_compact
>> - vmTestbase_vm_gc_concurrent
>> - vmTestbase_vm_gc_container
>> - vmTestbase_vm_gc_juggle
>> - vmTestbase_vm_gc_locker
>> - vmTestbase_vm_gc_ref
>> - vmTestbase_vm_gc_misc
>> 
>> besides these test groups, which split tests by functionality under test, 
>> the patch also adds test groups used only for test selection purposes:
>> - vmTestbase_vm_gc which includes all vmTestbase_vm_gc_* test groups
>> - vmTestbase_vm_gc_quick -- "quick" tests from vmTestbase_vm_gc test group
>> - vmTestbase_largepages which consists of tests which are believed to be 
>> good candidate to test largepage. this group is used in our testing w/ 
>> external vm flags and/or on pre-configured hosts.
>> 
>> As usually w/ VM testbase code, these tests are old, they have been run in 
>> hotspot testing for a long period of time. Originally, these tests were run 
>> by a test harness different from jtreg and had different build and execution 
>> schemes, some parts couldn't be easily translated to jtreg, so tests might 
>> have actions or pieces of code which look weird. In a long term, we are 
>> planning to rework them.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8199370
>> webrev: http://cr.openjdk.java.net/~iignatyev/8199370/webrev.00/index.html
> Build changes look good.
> 
> /Magnus
> 
>> 
>> Thanks,
>> -- Igor

RFR(L) : 8199384 : [TESTBUG] Open source VM testbase MLVM tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8199384/webrev.00/index.html
> 61414 lines changed: 61414 ins; 0 del; 0 mod; 

Hi all,

could you please review this patch which open sources MLVM tests from VM 
testbase?

these tests were developed in early days of JSR292 to test different aspects of 
MethodHandles and invokedynamic.

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199384
webrev: http://cr.openjdk.java.net/~iignatyev//8199384/webrev.00/index.html
testing: :vmTestbase_vm_mlvm test group

Thanks,
-- Igor

Re: RFR(L) : 8199370: [TESTBUG] Open source vm testbase GC tests

[Igor Ignatyev]

Hi Erik,

please see my answers inline.

can I consider this RFR as reviewed by you? 

Thanks,
-- Igor

> On May 14, 2018, at 4:23 AM, Erik Helin  wrote:
> 
> On 05/08/2018 12:35 AM, Igor Ignatyev wrote:
>> Hi all,
> 
> Hi Igor,
> 
> On 05/08/2018 12:35 AM, Igor Ignatyev wrote:
>> could you please review the patch which open sources GC tests from vm 
>> testbase? it introduces the following test groups:
>> - vmTestbase_vm_g1classunloading
>> - vmTestbase_vm_gc_compact
>> - vmTestbase_vm_gc_concurrent
>> - vmTestbase_vm_gc_container
>> - vmTestbase_vm_gc_juggle
>> - vmTestbase_vm_gc_locker
>> - vmTestbase_vm_gc_ref
>> - vmTestbase_vm_gc_misc
> 
> This is a very welcome, and pretty massive, change :) I won't be able to read 
> through each test, there are simple too many, but I can sample a few of them 
> and have a look.
> 
> On 05/08/2018 12:35 AM, Igor Ignatyev wrote:
>> As usually w/ VM testbase code, these tests are old, they have been run in 
>> hotspot testing for a long period of time. Originally, these tests were run 
>> by a test harness different from jtreg and had different build and execution 
>> schemes, some parts couldn't be easily translated to jtreg, so tests might 
>> have actions or pieces of code which look weird. In a long term, we are 
>> planning to rework them.
> 
> I'm also assuming that to help the open sourcing of these tests, most 
> comments will likely be deferred until later? If so, that is fine with me.
y, unless it is something very important and cost of delaying changes is high, 
I'd prefer to defer all comments/improvements till later. 
> 
> On 05/08/2018 12:35 AM, Igor Ignatyev wrote:
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8199370
>> webrev: http://cr.openjdk.java.net/~iignatyev/8199370/webrev.00/index.html
> 
> Many of the tests are a bit cryptic, but that can be refactored later. Could 
> you please file bugs for the two tests written in shell? Particularly 
> parOld/test.sh should be trivial to rewrite in Java.
sure, I've filed 8203239 and 8203238. 
> 
> It seems like a lot of tests contains a TEST.properties file with the content 
> `exclusiveAccess.dirs=.`. Could this become the default value somewhere, so 
> we don't need all those TEST.properties files?
y, but this will require finding a most top directory whose all tests have this 
TEST.properties file and it will also make it harder to understand how a test 
is executed. there was/is ongoing discussing w/ Jon on moving 'exclusiveAccess' 
to test description. anyhow I've filed an RFE to clean that up -- 8203241.

> 
> Thanks,
> Erik
> 
>> Thanks,
>> -- Igor

Re: RFR(L) : 8199370: [TESTBUG] Open source vm testbase GC tests

[Igor Ignatyev]

Hi Martin,

as it breaks build, it definitely has to be fixed. I have filed 8203437. the 
obvious fix for this warning would be:
> diff -r 3af6ed2513aa 
> test/hotspot/jtreg/vmTestbase/gc/gctests/nativeGC05/libnativeGC05.c
> --- a/test/hotspot/jtreg/vmTestbase/gc/gctests/nativeGC05/libnativeGC05.c 
> Thu May 17 21:05:43 2018 -0700
> +++ b/test/hotspot/jtreg/vmTestbase/gc/gctests/nativeGC05/libnativeGC05.c 
> Fri May 18 10:02:11 2018 -0700
> @@ -27,7 +27,7 @@
>  Java_gc_gctests_nativeGC05_nativeGC05_kickOffRefillers
>  (JNIEnv *env, jobject obj, jobject matrix, jobject stack) {
>  jclass matrixClass, stackClass, pairClass = 0;
> -jmethodID stack_pop_mid, stack_empty_mid, matrix_repopulate_mid, 
> pair_geti_mid, pair_getj_mid;
> +jmethodID stack_pop_mid, stack_empty_mid, matrix_repopulate_mid, 
> pair_geti_mid, pair_getj_mid = 0;
>  jobject pair;
>  jint i, j;
>  jboolean b;

while I'm in progress of setting up a linux w/ gcc4.8.5 (which due to different 
reasons might take some time), could you please verify whether it fixes this 
warning and if there are other warnings from your compilers? if there are, you  
can get the whole list by disable warnings as error (configure  
--disable-warnings-as-errors) and then grep for ': warning:' in 
'build/*/build.log'.

Thanks,
-- Igor

> On May 18, 2018, at 5:03 AM, Doerr, Martin  wrote:
> 
> Hi Igor,
> 
> we get compiler warnings on linux ppc64le (GCC 4.8.5):
> 
> libnativeGC05.c:80:19: error: 'pair_getj_mid' may be used uninitialized in 
> this function [-Werror=maybe-uninitialized]
> j = (*env)->CallIntMethod(env, pair, pair_getj_mid);
>   ^
> 
> libnativeGC05.c:78:19: error: 'pair_geti_mid' may be used uninitialized in 
> this function [-Werror=maybe-uninitialized]
> i = (*env)->CallIntMethod(env, pair, pair_geti_mid);
>   ^
> 
> Unfortunately, the files are compiled with warnings as errors. We think this 
> should get fixed.
> 
> Best regards,
> Martin
> 
> 
> -Original Message-
> From: hotspot-gc-dev [mailto:hotspot-gc-dev-boun...@openjdk.java.net] On 
> Behalf Of Igor Ignatyev
> Sent: Dienstag, 15. Mai 2018 20:59
> To: Erik Helin 
> Cc: hotspot-gc-dev ; build-dev 
> ; hotspot-dev developers 
> 
> Subject: Re: RFR(L) : 8199370: [TESTBUG] Open source vm testbase GC tests
> 
> Hi Erik,
> 
> please see my answers inline.
> 
> can I consider this RFR as reviewed by you? 
> 
> Thanks,
> -- Igor
> 
>> On May 14, 2018, at 4:23 AM, Erik Helin  wrote:
>> 
>> On 05/08/2018 12:35 AM, Igor Ignatyev wrote:
>>> Hi all,
>> 
>> Hi Igor,
>> 
>> On 05/08/2018 12:35 AM, Igor Ignatyev wrote:
>>> could you please review the patch which open sources GC tests from vm 
>>> testbase? it introduces the following test groups:
>>> - vmTestbase_vm_g1classunloading
>>> - vmTestbase_vm_gc_compact
>>> - vmTestbase_vm_gc_concurrent
>>> - vmTestbase_vm_gc_container
>>> - vmTestbase_vm_gc_juggle
>>> - vmTestbase_vm_gc_locker
>>> - vmTestbase_vm_gc_ref
>>> - vmTestbase_vm_gc_misc
>> 
>> This is a very welcome, and pretty massive, change :) I won't be able to 
>> read through each test, there are simple too many, but I can sample a few of 
>> them and have a look.
>> 
>> On 05/08/2018 12:35 AM, Igor Ignatyev wrote:
>>> As usually w/ VM testbase code, these tests are old, they have been run in 
>>> hotspot testing for a long period of time. Originally, these tests were run 
>>> by a test harness different from jtreg and had different build and 
>>> execution schemes, some parts couldn't be easily translated to jtreg, so 
>>> tests might have actions or pieces of code which look weird. In a long 
>>> term, we are planning to rework them.
>> 
>> I'm also assuming that to help the open sourcing of these tests, most 
>> comments will likely be deferred until later? If so, that is fine with me.
> y, unless it is something very important and cost of delaying changes is 
> high, I'd prefer to defer all comments/improvements till later. 
>> 
>> On 05/08/2018 12:35 AM, Igor Ignatyev wrote:
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8199370
>>> webrev: http://cr.openjdk.java.net/~iignatyev/8199370/webrev.00/index.html
>> 
>> Many of the tests are a bit cryptic, but that can be refactored later. Could 
>> you please file bugs for the two tests written in shell? Particularly 
>> parOld/test.sh should be trivial to rewrite in Jav

RFR(XL) : 8199383 : [TESTBUG] Open source VM testbase JVMTI tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8199383/webrev.00/index.html
> 308253 lines changed: 308253 ins; 0 del; 0 mod;

Hi all,

could you please review this patch which open sources JVMTI tests from VM 
testbase?

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

webrev: http://cr.openjdk.java.net/~iignatyev//8199383/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8199383
testing: :vmTestbase_nsk_jvmti test group

Thanks,
-- Igor

RFR(M) : 8199380 : [TESTBUG] Open source VM testbase AOD tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev/8199380/webrev.00/
> 2370 lines changed: 2370 ins; 0 del; 0 mod;

Hi all,

could you please review this patch which open sources AOD tests from VM 
testbase? these tests test hotspot's attach-on-demand. 

As usually w/ VM testbase code, these tests are old, they have been run in 
hotspot testing for a long period of time. Originally, these tests were run by 
a test harness different from jtreg and had different build and execution 
schemes, some parts couldn't be easily translated to jtreg, so tests might have 
actions or pieces of code which look weird. In a long term, we are planning to 
rework them.

JBS: https://bugs.openjdk.java.net/browse/JDK-8199380
webrev: http://cr.openjdk.java.net/~iignatyev//8199380/webrev.00/index.html
testing: :vmTestbase_vm_aod test group

Thanks,
-- Igor

Re: RFR(L): 8199255: [TESTBUG] Open source VM testbase default methods tests

[Igor Ignatyev]

Hi Misha,

looks good to me.

-- Igor

> On May 23, 2018, at 4:13 PM, mikhailo  wrote:
> 
> Hi Calvin,
> 
>   Thank you for review. I will fix the issue with vmTestbase_nsk_stress 
> (merge issue) and will remove the blank line at line 1273 prior to check in.
> 
> 
> Misha
> 
> 
> On 05/23/2018 03:15 PM, Calvin Cheung wrote:
>> Hi Misha,
>> 
>> I've compared the file.list from your closed webrev with the one from this 
>> open webrev and didn't see any missing files.
>> Also spot checked a few copyright headers and they look good.
>> 
>> Regarding TEST.groups, why was the following removed?
>> 1160 vmTestbase_nsk_stress = \
>> 1161   vmTestbase/nsk/stress
>> 
>> Could you also remove the extra blank line added at line 1273?
>> 
>> thanks,
>> Calvin
>> 
>> On 5/21/18, 11:34 AM, Mikhailo Seledtsov wrote:
>>> Please review this change that will open source VM default method tests.
>>> These tests have been used internally for a while, and are now being open 
>>> sourced. Since this is not an creation of new tests, we would like to keep 
>>> the changes during this review to a minimum required for open sourcing 
>>> these tests, such as major issues and integration blockers. If you have 
>>> other feedback regarding improvements to these tests, please file RFE(s) 
>>> that will be addressed later in order of priority.
>>> 
>>> Here is what was done for this change:
>>>   1. Moved the tests to OpenJDK repository to the specified directory 
>>> location and structure.
>>>   3. Updated Copyright statements accordingly.
>>>   4. Updated "@library" statements accordingly.
>>>   5. Updated TEST.groups and a HotSpot test make file
>>> 
>>>   JBS:https://bugs.openjdk.java.net/browse/JDK-8199255
>>>   Webrev: http://cr.openjdk.java.net/~mseledtsov/8199255.01/
>>> 
>>>   Testing:
>>>   1. Ran the following tests on open-only repository and build, using 
>>> "make run-test" (Linux-x64)
>>>  vmTestbase_vm_defmeth
>>>  All PASS
>>> 
>>>   2. Automated multip-platform test system (usual 4 platforms):
>>>  - vmTestbase_vm_defmeth
>>>  - hs-tier{1,2}
>>>  In progress
>>> 
>>> 
>>> Thank you,
>>> Misha
>>> 
> 

RFR(XXS) : 8206088 : 8205207 broke builds

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8206088/webrev.00/index.html
> 1 line changed: 0 ins; 0 del; 1 mod; 

Hi all,

could you please review this one liner fix? 

webrev: http://cr.openjdk.java.net/~iignatyev//8206088/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8206088
testing:  make build-test-hotspot-jtreg-graal w/ empty GRAALUNIT_LIB

Thanks,
-- Igor

Re: RFR(XXS) : 8206088 : 8205207 broke builds

[Igor Ignatyev]

Hi Erik,

actually, I have a redundant quotation mark at the begging, removed. thanks for 
spotting this.

Thanks,
-- Igor

> On Jun 28, 2018, at 9:50 PM, Erik Helin  wrote:
> 
> On 06/29/2018 06:42 AM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8206088/webrev.00/index.html
>>> 1 line changed: 0 ins; 0 del; 1 mod;
>> Hi all,
>> could you please review this one liner fix?
>> webrev: http://cr.openjdk.java.net/~iignatyev//8206088/webrev.00/index.html
> 
> Hmmm, it seems like you are missing an end-of-string quotation mark in
> 
> + $(info "Skip building of Graal unit tests because 3rd party libraries 
> directory is not specified)
> 
> Or did I misunderstand the patch?
> Thanks,
> Erik
> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8206088
>> testing:  make build-test-hotspot-jtreg-graal w/ empty GRAALUNIT_LIB
>> Thanks,
>> -- Igor

Re: RFR(XXS) : 8206088 : 8205207 broke builds

[Igor Ignatyev]

Erik, Katya,

thank you for such fast reviews.

Cheers,
-- Igor

> On Jun 28, 2018, at 9:56 PM, Erik Helin  wrote:
> 
> On 06/29/2018 06:55 AM, Igor Ignatyev wrote:
>> Hi Erik,
>> actually, I have a redundant quotation mark at the begging, removed. thanks 
>> for spotting this.
> 
> Yes, I realized that just as I had sent my email :) Anyways, patch looks good 
> now, Reviewed.
> 
> Thanks,
> Erik
> 
>> Thanks,
>> -- Igor
>>> On Jun 28, 2018, at 9:50 PM, Erik Helin  wrote:
>>> 
>>> On 06/29/2018 06:42 AM, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8206088/webrev.00/index.html
>>>>> 1 line changed: 0 ins; 0 del; 1 mod;
>>>> Hi all,
>>>> could you please review this one liner fix?
>>>> webrev: http://cr.openjdk.java.net/~iignatyev//8206088/webrev.00/index.html
>>> 
>>> Hmmm, it seems like you are missing an end-of-string quotation mark in
>>> 
>>> + $(info "Skip building of Graal unit tests because 3rd party libraries 
>>> directory is not specified)
>>> 
>>> Or did I misunderstand the patch?
>>> Thanks,
>>> Erik
>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8206088
>>>> testing:  make build-test-hotspot-jtreg-graal w/ empty GRAALUNIT_LIB
>>>> Thanks,
>>>> -- Igor

Re: RFR(XXS): 8206113: Troubles configuring graal tests

[Igor Ignatyev]

Vladimir,

there are two different configure-time problems:
- 1st is ability to get unbuildable configuration w/ AOT but w/o JVMCI
 - 2nd is not getting configure time warnings when Graal is enabled, but 3rd 
party libs aren't available for tests.

AFAIU,  8206135 is about 1st problem, and I, as David, expected 2nd problem to 
be addressed by 8206113. if you are planning to change the scope of 8206135, 
its title and descriptions should be changed correspondingly. 

Thanks,
-- Igor

> On Jul 2, 2018, at 2:19 PM, Vladimir Kozlov  
> wrote:
> 
> On 7/2/18 2:17 PM, David Holmes wrote:
>> Hi Katya,
>> On 3/07/2018 5:10 AM, Ekaterina Pavlova wrote:
>>> Hi All,
>>> 
>>> please review this extra small change which makes make more silent by
>>> removing info message in case graal unit tests are not built.
>> Happy to see the build-time warning go, but I thought it was going to be 
>> replaced by some kind of configure-time warning?
> 
> I will fix it in jdk12:
> 
> JDK-8206135: Building jvm with AOT but without JVMCI should fail at configure 
> time.
> 
> https://bugs.openjdk.java.net/browse/JDK-8206135
> 
> Vladimir
> 
>> Thanks,
>> David
>>>  JBS: https://bugs.openjdk.java.net/browse/JDK-8206113
>>>   webrev: http://cr.openjdk.java.net/~epavlova//8206113/webrev.00/index.html
>>> 
>>> thanks,
>>> -katya
>>> 
>>> p.s.
>>>   Igor Ignatyev volunteered to sponsor this change.

Re: RFR(XXS): 8206113: Troubles configuring graal tests

[Igor Ignatyev]

> On Jul 2, 2018, at 3:52 PM, Vladimir Kozlov  
> wrote:
> 
> On 7/2/18 3:28 PM, Igor Ignatyev wrote:
>> Vladimir,
>> there are two different configure-time problems:
>> - 1st is ability to get unbuildable configuration w/ AOT but w/o JVMCI
>>  - 2nd is not getting configure time warnings when Graal is enabled, but 3rd 
>> party libs aren't available for tests.
>> AFAIU,  8206135 is about 1st problem, and I, as David, expected 2nd problem 
>> to be addressed by 8206113. if you are planning to change the scope of 
>> 8206135, its title and descriptions should be changed correspondingly.
> 
> Yes, I talked only about 1st issue and I will fix only it.
> 
> What 3rd party libs you are you talking about?

Ones which are needed to compile/run graal unit tests[1]. JDK-8205207[2] added 
make/configure GRAALUNIT_LIB variable which is expected to point to the 
directory w/ these libraries and added compilations of graal unit tests part of 
'test-image' target.

-- Igor

[1] test/hotspot/jtreg/compiler/graalunit/README.md:
 asm-5.0.4.jar
 asm-tree-5.0.4.jar
 junit-4.12.jar
 hamcrest-core-1.3.jar
 java-allocation-instrumenter.jar
[2] https://bugs.openjdk.java.net/browse/JDK-8205207 
<https://bugs.openjdk.java.net/browse/JDK-8205207>

> 
> Thanks
> Vladimir
> 
>> Thanks,
>> -- Igor
>>> On Jul 2, 2018, at 2:19 PM, Vladimir Kozlov  
>>> wrote:
>>> 
>>> On 7/2/18 2:17 PM, David Holmes wrote:
>>>> Hi Katya,
>>>> On 3/07/2018 5:10 AM, Ekaterina Pavlova wrote:
>>>>> Hi All,
>>>>> 
>>>>> please review this extra small change which makes make more silent by
>>>>> removing info message in case graal unit tests are not built.
>>>> Happy to see the build-time warning go, but I thought it was going to be 
>>>> replaced by some kind of configure-time warning?
>>> 
>>> I will fix it in jdk12:
>>> 
>>> JDK-8206135: Building jvm with AOT but without JVMCI should fail at 
>>> configure time.
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8206135
>>> 
>>> Vladimir
>>> 
>>>> Thanks,
>>>> David
>>>>>  JBS: https://bugs.openjdk.java.net/browse/JDK-8206113
>>>>>   webrev: 
>>>>> http://cr.openjdk.java.net/~epavlova//8206113/webrev.00/index.html
>>>>> 
>>>>> thanks,
>>>>> -katya
>>>>> 
>>>>> p.s.
>>>>>   Igor Ignatyev volunteered to sponsor this change.

RFR(S/M) [trivial] 8206429 : [REDO] 8202561 clean up TEST.groups

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8206429/webrev.00/index.html
> 243 lines changed: 0 ins; 124 del; 119 mod; 

Hi all,

could you please review this redo of 8202561? 8202561 was backed out b/c our CI 
infra didn't support multiple test group files. now such configurations are 
supported, so 8202561 can be re-integrated.
the patch is the exact 8202561 changeset and it has applied clearly.
 
webrev: http://cr.openjdk.java.net/~iignatyev//8206429/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8206429
testing: successfully submitted adhoc jobs which use a "quick" test group in 
the same way CI system does

Thanks,
-- Igor

Re: RFR(S/M) [trivial] 8206429 : [REDO] 8202561 clean up TEST.groups

[Igor Ignatyev]

well, I did test it last time, I just didn't expect that some parts of infra do 
submission a bit weirdly. now (at least as far as I can tell) I tested 
submission in the exact same way.

-- Igor

> On Jul 5, 2018, at 2:18 PM, Vladimir Kozlov  
> wrote:
> 
> I am fine with re-do if you were able re-test it to verify that it actually 
> works. Not like last time.
> 
> Thanks,
> Vladimir
> 
> On 7/5/18 1:02 PM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8206429/webrev.00/index.html
>>> 243 lines changed: 0 ins; 124 del; 119 mod;
>> Hi all,
>> could you please review this redo of 8202561? 8202561 was backed out b/c our 
>> CI infra didn't support multiple test group files. now such configurations 
>> are supported, so 8202561 can be re-integrated.
>> the patch is the exact 8202561 changeset and it has applied clearly.
>>  webrev: http://cr.openjdk.java.net/~iignatyev//8206429/webrev.00/index.html
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8206429
>> testing: successfully submitted adhoc jobs which use a "quick" test group in 
>> the same way CI system does
>> Thanks,
>> -- Igor

Re: RFR: 8208157: requires.VMProps throws NPE for missing properties in "release" file

[Igor Ignatyev]

looks good to me.

-- Igor

> On Jul 24, 2018, at 3:48 PM, Alexandre (Shura) Iline 
>  wrote:
> 
> Hi,
> 
> Could you please tale a quick look on this simple fix?
> 
> diff --git a/test/jtreg-ext/requires/VMProps.java 
> b/test/jtreg-ext/requires/VMProps.java
> --- a/test/jtreg-ext/requires/VMProps.java
> +++ b/test/jtreg-ext/requires/VMProps.java
> @@ -432,7 +432,8 @@
> System.getProperty("java.home") + "/release"))) {
> Properties properties = new Properties();
> properties.load(in);
> -return properties.getProperty("IMPLEMENTOR").replace("\"", "");
> +String implementorProperty = 
> properties.getProperty("IMPLEMENTOR");
> +return (implementorProperty == null) ? "null" : 
> implementorProperty.replace("\"", "");
> } catch (IOException e) {
> e.printStackTrace();
> }
> 
> 
> Thank you.
> 
> Shura

RFR(XXS) : 8208647 : switch jtreg to 4.2b13

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev/8208647/webrev.00/index.html
> 5 lines changed: 0 ins; 0 del; 5 mod;

Hi all,

could you please review this small fix which switches jtreg to the latest 
available build? due to changes in javatest API, failure_handler had to be 
slightly updated.

webrev: http://cr.openjdk.java.net/~iignatyev//8208647/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8208647
testing: tier1-2

Thanks,
-- Igor

Re: RFR(XXS) : 8208647 : switch jtreg to 4.2b13

[Igor Ignatyev]

> On Aug 1, 2018, at 10:33 PM, David Holmes  wrote:
> 
> Hi Igor,
> 
> On 2/08/2018 5:42 AM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev/8208647/webrev.00/index.html
>>> 5 lines changed: 0 ins; 0 del; 5 mod;
>> Hi all,
>> could you please review this small fix which switches jtreg to the latest 
>> available build? 
> 
> Okay.
> 
>> due to changes in javatest API, failure_handler had to be slightly updated.
> 
> Don't understand how anything in javatest affected the code you changed, but 
> it's cleaner to specify its a Map and avoid the casts.

com.sun.javatest.InterviewParameters::save used to be 'save(Map)', and now it's 
'save(Map)', so w/o changing 'map' local, we would get 
compile-time error.

-- Igor 
> Thanks,
> David
> 
>> webrev: http://cr.openjdk.java.net/~iignatyev//8208647/webrev.00/index.html
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8208647
>> testing: tier1-2
>> Thanks,
>> -- Igor

RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html
> 11160 lines changed: 879 ins; 61 del; 10220 mod;

Hi all,

could you please review the patch which moves all hotspot native test code to 
C++? this will guarantee that we always use C++ compilers for them (as an 
opposite to either C or C++ compiler depending on configuration), as a result 
we will be able to get rid of JNI_ENV_ARG[1] macros, perform other clean ups 
and improve overall quality of the test code.

the patch consists of two parts:
 - automatic: renaming .c files to .cpp, updating #include, changing JNI/JVMTI 
calls
 - semi-manual: adding extern "C" , fixing a number of compiler warnings 
(mostly types inconsistency), updating makefiles

JBS: https://bugs.openjdk.java.net/browse/JDK-8209611
webrevs:
 - automatic: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.00/index.html
> 9394 lines changed: 0 ins; 0 del; 9394 mod; 

 - semi-manual: 
http://cr.openjdk.java.net/~iignatyev//8209611/webrev.0-1/index.html
> 1899 lines changed: 879 ins; 61 del; 959 mod

 - whole: http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html
> 11160 lines changed: 879 ins; 61 del; 10220 mod;

testing: all hotspot tests + tier[1-3]

[1] https://bugs.openjdk.java.net/browse/JDK-8209547

Thanks,
-- Igor

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

> On Aug 20, 2018, at 10:06 PM, David Holmes  wrote:
> 
> Hi Igor,
> 
> On 21/08/2018 1:58 PM, Igor Ignatev wrote:
>> Hi David,
>> It is necessary for vmTestbase tests (if we of course want to clean them up) 
>> as they are coupled, and moving only part of them will require (non trivial) 
>> updates in the rest of code (or at least in the shared part) to properly 
>> serve both compilers.
> 
> Can you elaborate on the problem please as I don't see why the vmTestbase 
> tests are special here.
> 
> I would expect a .c file to be compiled as C and a .cpp to be compiled as C++.

currently our build system supports only 1-1 relation b/w tests lib/exec and 
source file; most of native libraries in vmTestbase have more than one source 
file. to work that around, we introduced a .c file for each library and theses 
.c files include all other required .c files.

yes, a .c file will be compiled as C and a .cpp file as C++, but as we have .c 
files which include .c files, moving only part of .c files, we will end up w/ 
.c files being included into both .c and .cpp files and thus compiled as both C 
and C++. let's take 
test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS301/hs301t004, if 
we want its native part to be compiled as C++, we rename libhs301t004.c to 
libhs301t004.cpp, but libhs301t004.c includes 11 other files, there only one is 
a test specific fill and all the other are used by other tests and included by 
their .c files.  so we have 10 files which now will be compiled by both C and 
C++ compilers.

> 
>> It absolutely not necessary for other tests, but I’d prefer to have some 
>> level of unification in the test base. That being said, I agree I might went 
>> too far and moved all the tests which might or might not compromise their 
>> purpose. I personally don’t think we should relay on our jtreg testbase to 
>> verify if C linked libraries can be used with hotspot. it must be verified 
>> by JCK which should be compiled/linked with carefully chosen 
>> compilers/linkers and their flags.
> 
> Sure but JCK comes into play much later and I'd rather spot issues during 
> developer testing than promotion testing.

agree, but if we want to verify how C/C++ linked libraries work w/ hotspot, it 
should be done by the tests specifically written for that purpose rather that 
relaying on indirect coverage from other tests, and I guess it hasn't been done 
as we always relied on JCK to test that.

> 
>> It has been discussed (not widely enough and I accept that) in 8209547 and 
>> the related email thread b/w JC(cc'ed) and myself.
>> as I said, I might went a way too far, so I'll revert changes in the 
>> non-vmTestbase tests and made appropriate changes in makefiles. what do you 
>> think?
> 
> I think I need to see what you mean exactly :)
sure, it will take some time for me to do that, hopefully will upload new 
webrevs tomorrow morning PT. but the basic idea is to leave files in 
test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, 
testlibrary as .c files, exactly as they were before, and restore corresponding 
filenames in make/test/JtregNativeHotspot.gmk.

Cheers,
-- Igor

> 
> Thanks,
> David
> 
>> Thanks,
>> — Igor
>> On Aug 20, 2018, at 6:43 PM, David Holmes > <mailto:david.hol...@oracle.com>> wrote:
>>> Hi Igor,
>>> 
>>> On 21/08/2018 8:59 AM, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html 
>>>> <http://cr.openjdk.java.net/%7Eiignatyev//8209611/webrev.01/index.html>
>>>>> 11160 lines changed: 879 ins; 61 del; 10220 mod;
>>>> Hi all,
>>>> could you please review the patch which moves all hotspot native test code 
>>>> to C++? this will guarantee that we always use C++ compilers for them (as 
>>>> an opposite to either C or C++ compiler depending on configuration), as a 
>>>> result we will be able to get rid of JNI_ENV_ARG[1] macros, perform other 
>>>> clean ups and improve overall quality of the test code.
>>> 
>>> Sorry but I don't see why this is necessary. If people want to be able to 
>>> write C++ tests then we should enable that if not currently enabled, but I 
>>> don't see why everything should be forced to C++. What if we did something 
>>> that broke the C linkage and we didn't detect it because we only ever 
>>> tested C++?
>>> 
>>> Was the motivation previously discussed somewhere?
>>> 
>>> Thanks,
>>> David
>>> 
>>>> the patch consists of two parts:
>>>>  - automatic: renaming .c files t

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html 
<http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html> is a new 
version of patch, which moves only vmTestbase tests.

Thanks,
-- Igor 

> On Aug 20, 2018, at 11:07 PM, Igor Ignatyev  wrote:
> 
>>> It has been discussed (not widely enough and I accept that) in 8209547 and 
>>> the related email thread b/w JC(cc'ed) and myself.
>>> as I said, I might went a way too far, so I'll revert changes in the 
>>> non-vmTestbase tests and made appropriate changes in makefiles. what do you 
>>> think?
>> 
>> I think I need to see what you mean exactly :)
> sure, it will take some time for me to do that, hopefully will upload new 
> webrevs tomorrow morning PT. but the basic idea is to leave files in 
> test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, 
> testlibrary as .c files, exactly as they were before, and restore 
> corresponding filenames in make/test/JtregNativeHotspot.gmk.

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

Hi David,

thanks for looking at the webrev and all your comments. my answers are inlined.

enjoy your vacation!

-- Igor 

> On Aug 21, 2018, at 9:28 PM, David Holmes  wrote:
> 
> Hi Igor,
> 
> On 22/08/2018 9:04 AM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html 
>> <http://cr.openjdk.java.net/%7Eiignatyev//8209611/webrev.02/index.html> is a 
>> new version of patch, which moves only vmTestbase tests.
> 
> This seems okay in principle. I didn't verify all the mechanical changes 
> individually.
> 
> make/common/TestFilesCompilation.gmk
> 
> I suspect you can just find all .c and .cpp files and process them together, 
> rather than duplicate all the logic. But I'll leave that to Magnus to comment 
> on.
I had to duplicate this foreach cycle to specify a correct TOOLCHAIN, w/o it 
linking fails on linux-slowdebug. 
> 
> make/test/JtregNativeHotspot.gmk
> 
> + BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS := -D__STDC_FORMAT_MACROS 
> -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
> 
> Just a note that defining these is obsolete once we use C11/C++11 - which I 
> think is an intent for JDK 12 if possible.
initially I wanted to use  instead of  + 
-D__STDC_FORMAT_MACROS, but solaris studio apparently doesn't have cinttypes 
header file.

hotspot makefiles also use -D__STDC_, so I hope both places will be fixed 
together.

> 
> test/hotspot/jtreg/vmTestbase/gc/g1/unloading/libdefine.cpp
> 
>  #ifdef __cplusplus
> ! #define JNI_ENV_ARG(x, y) y
>  #define JNI_ENV_PTR(x) x
>  #else
>  #define JNI_ENV_ARG(x, y) x , y
>  #define JNI_ENV_PTR(x) (*x)
>  #endif
> 
> If this is now a C++ source file why do you a still have the code that allows 
> for either C or C++? Is this stuff that will be cleaned up in the later RFE?
yes, 8209547 will clean up JNI_ENV_ARG macros.

> 
> ---
> 
> The switch to C++ means a lot of code now needs:
> 
> + #ifdef __cplusplus
> + extern "C" {
> + #endif

that's true, and this patch adds 'extern "C"' very conservatively (basically to 
everywhere). this, if needed, can be cleaned up later.

> 
> I still have to wonder whether it better to leave these as C tests and only 
> convert to C++ as and when needed ... seems like so much work.
> 
> Anyway I'm off on vacation after today so I'll leave it to others to take 
> this up.
> 
> Thanks,
> David
> 
>> Thanks,
>> -- Igor
>>> On Aug 20, 2018, at 11:07 PM, Igor Ignatyev >> <mailto:igor.ignat...@oracle.com>> wrote:
>>> 
>>>>> It has been discussed (not widely enough and I accept that) in 8209547 
>>>>> and the related email thread b/w JC(cc'ed) and myself.
>>>>> as I said, I might went a way too far, so I'll revert changes in the 
>>>>> non-vmTestbase tests and made appropriate changes in makefiles. what do 
>>>>> you think?
>>>> 
>>>> I think I need to see what you mean exactly :)
>>> sure, it will take some time for me to do that, hopefully will upload new 
>>> webrevs tomorrow morning PT. but the basic idea is to leave files in 
>>> test/hotspot/jtreg/compiler, runtime, gc, native_sanity, serviceability, 
>>> testlibrary as .c files, exactly as they were before, and restore 
>>> corresponding filenames in make/test/JtregNativeHotspot.gmk.

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

Hi JC,

thanks for looking at it and even paying attention to the changes;)

regarding your nits. a space before comma is straightforward and must be done 
for test code to be aligned w/ regular hotspot code style, static_case<> is 
more controversial, it's not used much by hotspot. I do recall some discussion 
around it 3-4 years ago, but I can't recall the outcome. anyhow, I'd prefer to 
clean it up later, as these editorial/style fixes can be done more gradually, 
and this patch kinda blocks your work on 8209547.

Thanks,
-- Igor


> On Aug 22, 2018, at 11:53 AM, JC Beyler  wrote:
> 
> Hi Igor,
> 
> I looked over the changes and tried to pay attention to all the mechanical 
> changes done. They look good to me as they are the step in the direction of 
> getting it all in C++. I will be happy to start on 8209547 once this goes in. 
> 
> (As always, not an official reviewer)
> 
> Two nits:
> 
> - We could use static_cast?
> +*new_bytes = (u1*) realloc(gen, *new_length);
> 
> - Add a space since we are doing the change (this is one example)?
> -arrayOrig[i]=env->GetObjectArrayElement(orig,i); CE
> -arrayClone[i]=env->GetObjectArrayElement(clone,i); CE
> +arrayOrig[i]=(jarray) env->GetObjectArrayElement(orig,i); CE
> +arrayClone[i]=(jarray) env->GetObjectArrayElement(clone,i); CE
> 
> On the other hand, we could just clean this up later once this big move to 
> C++ happens, so I'd be happy to see us tackle it in a second/third step.
> 
> So looks good to me and thanks for doing it!
> Jc
> 
> On Tue, Aug 21, 2018 at 9:58 PM Igor Ignatyev  <mailto:igor.ignat...@oracle.com>> wrote:
> Hi David,
> 
> thanks for looking at the webrev and all your comments. my answers are 
> inlined.
> 
> enjoy your vacation!
> 
> -- Igor 
> 
> > On Aug 21, 2018, at 9:28 PM, David Holmes  > <mailto:david.hol...@oracle.com>> wrote:
> > 
> > Hi Igor,
> > 
> > On 22/08/2018 9:04 AM, Igor Ignatyev wrote:
> >> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html 
> >> <http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/index.html> 
> >> <http://cr.openjdk.java.net/%7Eiignatyev//8209611/webrev.02/index.html 
> >> <http://cr.openjdk.java.net/%7Eiignatyev//8209611/webrev.02/index.html>> 
> >> is a new version of patch, which moves only vmTestbase tests.
> > 
> > This seems okay in principle. I didn't verify all the mechanical changes 
> > individually.
> > 
> > make/common/TestFilesCompilation.gmk
> > 
> > I suspect you can just find all .c and .cpp files and process them 
> > together, rather than duplicate all the logic. But I'll leave that to 
> > Magnus to comment on.
> I had to duplicate this foreach cycle to specify a correct TOOLCHAIN, w/o it 
> linking fails on linux-slowdebug. 
> > 
> > make/test/JtregNativeHotspot.gmk
> > 
> > + BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS := -D__STDC_FORMAT_MACROS 
> > -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS
> > 
> > Just a note that defining these is obsolete once we use C11/C++11 - which I 
> > think is an intent for JDK 12 if possible.
> initially I wanted to use  instead of  + 
> -D__STDC_FORMAT_MACROS, but solaris studio apparently doesn't have cinttypes 
> header file.
> 
> hotspot makefiles also use -D__STDC_, so I hope both places will be fixed 
> together.
> 
> > 
> > test/hotspot/jtreg/vmTestbase/gc/g1/unloading/libdefine.cpp
> > 
> >  #ifdef __cplusplus
> > ! #define JNI_ENV_ARG(x, y) y
> >  #define JNI_ENV_PTR(x) x
> >  #else
> >  #define JNI_ENV_ARG(x, y) x , y
> >  #define JNI_ENV_PTR(x) (*x)
> >  #endif
> > 
> > If this is now a C++ source file why do you a still have the code that 
> > allows for either C or C++? Is this stuff that will be cleaned up in the 
> > later RFE?
> yes, 8209547 will clean up JNI_ENV_ARG macros.
> 
> > 
> > ---
> > 
> > The switch to C++ means a lot of code now needs:
> > 
> > + #ifdef __cplusplus
> > + extern "C" {
> > + #endif
> 
> that's true, and this patch adds 'extern "C"' very conservatively (basically 
> to everywhere). this, if needed, can be cleaned up later.
> 
> > 
> > I still have to wonder whether it better to leave these as C tests and only 
> > convert to C++ as and when needed ... seems like so much work.
> > 
> > Anyway I'm off on vacation after today so I'll leave it to others to take 
> > this up.
> > 
> > Thanks,
> > David
> > 
> 

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

ok, it worked just fine, here is the final diff for TestFilesCompilation.gmk:

> diff -r 028076b2832a -r caca95a834e3 make/common/TestFilesCompilation.gmk
> --- a/make/common/TestFilesCompilation.gmkThu Aug 16 16:28:03 2018 -0700
> +++ b/make/common/TestFilesCompilation.gmkFri Aug 24 14:12:37 2018 -0700
> @@ -60,13 +60,13 @@
>ifeq ($$($1_TYPE), LIBRARY)
>  $1_PREFIX = lib
>  $1_OUTPUT_SUBDIR := lib
> -$1_CFLAGS := $(CFLAGS_TESTLIB)
> +$1_CFLAGS += $(CFLAGS_TESTLIB)
>  $1_LDFLAGS := $(LDFLAGS_TESTLIB) $(call SET_SHARED_LIBRARY_ORIGIN)
>  $1_COMPILATION_TYPE := LIBRARY
>else ifeq ($$($1_TYPE), PROGRAM)
>  $1_PREFIX = exe
>  $1_OUTPUT_SUBDIR := bin
> -$1_CFLAGS := $(CFLAGS_TESTEXE)
> +$1_CFLAGS += $(CFLAGS_TESTEXE)
>  $1_LDFLAGS := $(LDFLAGS_TESTEXE)
>  $1_COMPILATION_TYPE := EXECUTABLE
>else
> @@ -75,12 +75,12 @@
>  
># Locate all files with the matching prefix
>$1_FILE_LIST := \
> -  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f -name "$$($1_PREFIX)*.c")
> +  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
> "$$($1_PREFIX)*.c" \
> +   -o -name 
> "$$($1_PREFIX)*.cpp" \))
>  
>$1_EXCLUDE_PATTERN := $$(addprefix %/, $$($1_EXCLUDE))
>$1_FILTERED_FILE_LIST := $$(filter-out $$($1_EXCLUDE_PATTERN), 
> $$($1_FILE_LIST))
>  
> -  # Setup a compilation for each and every one of them
>$$(foreach file, $$($1_FILTERED_FILE_LIST),\
>  $$(eval name := $$(strip $$(basename $$(notdir $$(file) \
>  $$(eval unprefixed_name := $$(patsubst $$($1_PREFIX)%, %, $$(name))) \
> @@ -94,6 +94,7 @@
>  CFLAGS := $$($1_CFLAGS) $$($1_CFLAGS_$$(name)), \
>  LDFLAGS := $$($1_LDFLAGS) $$($1_LDFLAGS_$$(name)), \
>  LIBS := $$($1_LIBS_$$(name)), \
> +TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
> TOOLCHAIN_DEFAULT), \
>  OPTIMIZATION := LOW, \
>  COPY_DEBUG_SYMBOLS := false, \
>  STRIP_SYMBOLS := false, \

Thanks,
-- Igor


> On Aug 24, 2018, at 10:36 AM, Igor Ignatev  wrote:
> 
> Hi Erik,
> 
> Unfortunately, just adding .cpp files to file list isn’t enough, as I 
> mentioned in one of my previous emails[1], initially I did exactly that and 
> linux-slowdebug build fails b/c c-linker was be used for .o files produced by 
> cpp-compiler.
> 
> I guess something like [2] might work. I'll play w/ this idea and send final 
> patch latch.
> 
> — Igor
> 
> [1] http://mail.openjdk.java.net/pipermail/build-dev/2018-August/022922.html
> [2] TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
> TOOLCHAIN_DEFAULT)
> 
>> On Aug 23, 2018, at 11:31 PM, Erik Joelsson  wrote:
>> 
>> Hello Igor,
>> 
>> In TestFilesCompilation.gmk, there is no need to duplicate the whole macro 
>> call. If you want to find .cpp as well as .c files, just add that to the one 
>> list of files. The SetupNativeCompilation macro will automatically treat .c 
>> and .cpp correctly.
>> 
>> Regarding the .c/.cpp files for your vmtestbase tests that include all the 
>> other files, this is an unfortunate solution, but I guess OK if it works. We 
>> certainly didn't intend it that way. The macro SetupTestFilesCompilation was 
>> intended for easily writing single file native exe and lib binaries without 
>> having to modify any makefile. The expectation was that if anything more 
>> complicated was needed (like multiple files per binary), we would just write 
>> explicit SetupNativeCompilation calls for them in JtregNativeHotspot.gmk. It 
>> now looks like we have a new pattern of source files/directories that turns 
>> into native libraries, and we could of course create a new macro that 
>> automatically generates compilation setups for them as well (given that file 
>> or directory names makes it possible to automatically determine everything 
>> needed). Of course, that change would be a separate cleanup possible if you 
>> want to.
>> 
>> /Erik
>> 
>>> On 2018-08-20 15:59, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev//8209611/webrev.01/index.html
>>>> 11160 lines changed: 879 ins; 61 del; 10220 mod;
>>> Hi all,
>>> 
>>> could you please review the patch which moves all hotspot native test code 
>>> to C++? this will guarantee that we always use C++ compilers for them (as 
>>> an opposite to either C or C++ compiler depending on configuration), as a 
>>> result we will be able to get rid of JNI_ENV_ARG[1] macros, perform other 
>>> clean ups a

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

Hi Magnus,

thanks for reviewing this.

please see my answers inline.

Cheers,
-- Igor

> On Aug 28, 2018, at 4:14 AM, Magnus Ihse Bursie 
>  wrote:
> 
> Hi Igor,
> 
> Sorry for my late arrival to this discussion.
> 
> I tried to figure out what the latest version of your patch is. My guess is 
> that it is http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/ + the 
> inlined patch from this mail?
yes, that's right
> 
> In JtregNativeHotspot.gmk, you are removing:
> ifeq ($(TOOLCHAIN_TYPE), solstudio)
>BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS_libji06t001 += 
> -erroff=E_END_OF_LOOP_CODE_NOT_REACHED
> endif
> 
> Is this related to the current fix?

y, C++ compiler of SS doesn't recognize E_END_OF_LOOP_CODE_NOT_REACHED as a 
valid tag, it seems this tag is only defined for C compiler

> 
> In the patch below, why are you removing the comment to the "$$(foreach 
> file..." loop? I think it's still relevant.
no reason, will put it back.

> In this code:
>   $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
> "$$($1_PREFIX)*.c" \
>-o -name 
> "$$($1_PREFIX)*.cpp" \))
> 
> While I appreciate your concerns of aligning the "-name" arguments, this kind 
> of padding is something we try to avoid in the build system. There are never 
> any logical places to break the arguments, and this just leads to big 
> reshufflings whenever a command line changes, with little gain in 
> readability. Instead, just indent the line following the \ with four more 
> spaces (and no tabs!). (See 
> http://openjdk.java.net/groups/build/doc/code-conventions.html)
sure thing, I'll remove all redundant spaces. 
> 
> Otherwise, this looks fine.
> 
> /Magnus
> 
> 
> On 2018-08-24 23:20, Igor Ignatyev wrote:
>> ok, it worked just fine, here is the final diff for TestFilesCompilation.gmk:
>> 
>>> diff -r 028076b2832a -r caca95a834e3 make/common/TestFilesCompilation.gmk
>>> --- a/make/common/TestFilesCompilation.gmk  Thu Aug 16 16:28:03 2018 -0700
>>> +++ b/make/common/TestFilesCompilation.gmk  Fri Aug 24 14:12:37 2018 -0700
>>> @@ -60,13 +60,13 @@
>>>ifeq ($$($1_TYPE), LIBRARY)
>>>  $1_PREFIX = lib
>>>  $1_OUTPUT_SUBDIR := lib
>>> -$1_CFLAGS := $(CFLAGS_TESTLIB)
>>> +$1_CFLAGS += $(CFLAGS_TESTLIB)
>>>  $1_LDFLAGS := $(LDFLAGS_TESTLIB) $(call SET_SHARED_LIBRARY_ORIGIN)
>>>  $1_COMPILATION_TYPE := LIBRARY
>>>else ifeq ($$($1_TYPE), PROGRAM)
>>>  $1_PREFIX = exe
>>>  $1_OUTPUT_SUBDIR := bin
>>> -$1_CFLAGS := $(CFLAGS_TESTEXE)
>>> +$1_CFLAGS += $(CFLAGS_TESTEXE)
>>>  $1_LDFLAGS := $(LDFLAGS_TESTEXE)
>>>  $1_COMPILATION_TYPE := EXECUTABLE
>>>else
>>> @@ -75,12 +75,12 @@
>>>  # Locate all files with the matching prefix
>>>$1_FILE_LIST := \
>>> -  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f -name 
>>> "$$($1_PREFIX)*.c")
>>> +  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
>>> "$$($1_PREFIX)*.c" \
>>> +   -o -name 
>>> "$$($1_PREFIX)*.cpp" \))
>>>  $1_EXCLUDE_PATTERN := $$(addprefix %/, $$($1_EXCLUDE))
>>>$1_FILTERED_FILE_LIST := $$(filter-out $$($1_EXCLUDE_PATTERN), 
>>> $$($1_FILE_LIST))
>>>  -  # Setup a compilation for each and every one of them
>>>$$(foreach file, $$($1_FILTERED_FILE_LIST),\
>>>  $$(eval name := $$(strip $$(basename $$(notdir $$(file) \
>>>  $$(eval unprefixed_name := $$(patsubst $$($1_PREFIX)%, %, $$(name))) \
>>> @@ -94,6 +94,7 @@
>>>  CFLAGS := $$($1_CFLAGS) $$($1_CFLAGS_$$(name)), \
>>>  LDFLAGS := $$($1_LDFLAGS) $$($1_LDFLAGS_$$(name)), \
>>>  LIBS := $$($1_LIBS_$$(name)), \
>>> +TOOLCHAIN := $(if $$(filter %.cpp, $$(file)), TOOLCHAIN_LINK_CXX, 
>>> TOOLCHAIN_DEFAULT), \
>>>  OPTIMIZATION := LOW, \
>>>  COPY_DEBUG_SYMBOLS := false, \
>>>  STRIP_SYMBOLS := false, \
>> Thanks,
>> -- Igor
>> 
>> 
>>> On Aug 24, 2018, at 10:36 AM, Igor Ignatev  wrote:
>>> 
>>> Hi Erik,
>>> 
>>> Unfortunately, just adding .cpp files to file list isn’t enough, as I 
>>> mentioned in one of my previous emails[1], initially I did exactly that and 
>>> linux-slowdebug build fails b/c c-linker was be used for .o files produced 
>>> by cpp-compiler.
>>> 
>&

Re: RFR(M/L) : 8209611 : use C++ compiler for hotspot tests

[Igor Ignatyev]

Hi Erik,

thanks for your review!

-- Igor

> On Aug 28, 2018, at 9:01 AM, Erik Joelsson  wrote:
> 
> Hello,
> 
> I have nothing to add in addition to Magnus' comments. If that is fixed, this 
> looks good from a build point of view.
> 
> /Erik
> 
> 
> On 2018-08-28 07:51, Igor Ignatyev wrote:
>> Hi Magnus,
>> 
>> thanks for reviewing this.
>> 
>> please see my answers inline.
>> 
>> Cheers,
>> -- Igor
>> 
>>> On Aug 28, 2018, at 4:14 AM, Magnus Ihse Bursie 
>>>  wrote:
>>> 
>>> Hi Igor,
>>> 
>>> Sorry for my late arrival to this discussion.
>>> 
>>> I tried to figure out what the latest version of your patch is. My guess is 
>>> that it is http://cr.openjdk.java.net/~iignatyev//8209611/webrev.02/ + the 
>>> inlined patch from this mail?
>> yes, that's right
>>> In JtregNativeHotspot.gmk, you are removing:
>>> ifeq ($(TOOLCHAIN_TYPE), solstudio)
>>>BUILD_HOTSPOT_JTREG_LIBRARIES_CFLAGS_libji06t001 += 
>>> -erroff=E_END_OF_LOOP_CODE_NOT_REACHED
>>> endif
>>> 
>>> Is this related to the current fix?
>> y, C++ compiler of SS doesn't recognize E_END_OF_LOOP_CODE_NOT_REACHED as a 
>> valid tag, it seems this tag is only defined for C compiler
>> 
>>> In the patch below, why are you removing the comment to the "$$(foreach 
>>> file..." loop? I think it's still relevant.
>> no reason, will put it back.
>> 
>>> In this code:
>>>   $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
>>> "$$($1_PREFIX)*.c" \
>>>-o -name 
>>> "$$($1_PREFIX)*.cpp" \))
>>> 
>>> While I appreciate your concerns of aligning the "-name" arguments, this 
>>> kind of padding is something we try to avoid in the build system. There are 
>>> never any logical places to break the arguments, and this just leads to big 
>>> reshufflings whenever a command line changes, with little gain in 
>>> readability. Instead, just indent the line following the \ with four more 
>>> spaces (and no tabs!). (See 
>>> http://openjdk.java.net/groups/build/doc/code-conventions.html)
>> sure thing, I'll remove all redundant spaces.
>>> Otherwise, this looks fine.
>>> 
>>> /Magnus
>>> 
>>> 
>>> On 2018-08-24 23:20, Igor Ignatyev wrote:
>>>> ok, it worked just fine, here is the final diff for 
>>>> TestFilesCompilation.gmk:
>>>> 
>>>>> diff -r 028076b2832a -r caca95a834e3 make/common/TestFilesCompilation.gmk
>>>>> --- a/make/common/TestFilesCompilation.gmkThu Aug 16 16:28:03 
>>>>> 2018 -0700
>>>>> +++ b/make/common/TestFilesCompilation.gmkFri Aug 24 14:12:37 
>>>>> 2018 -0700
>>>>> @@ -60,13 +60,13 @@
>>>>>ifeq ($$($1_TYPE), LIBRARY)
>>>>>  $1_PREFIX = lib
>>>>>  $1_OUTPUT_SUBDIR := lib
>>>>> -$1_CFLAGS := $(CFLAGS_TESTLIB)
>>>>> +$1_CFLAGS += $(CFLAGS_TESTLIB)
>>>>>  $1_LDFLAGS := $(LDFLAGS_TESTLIB) $(call SET_SHARED_LIBRARY_ORIGIN)
>>>>>  $1_COMPILATION_TYPE := LIBRARY
>>>>>else ifeq ($$($1_TYPE), PROGRAM)
>>>>>  $1_PREFIX = exe
>>>>>  $1_OUTPUT_SUBDIR := bin
>>>>> -$1_CFLAGS := $(CFLAGS_TESTEXE)
>>>>> +$1_CFLAGS += $(CFLAGS_TESTEXE)
>>>>>  $1_LDFLAGS := $(LDFLAGS_TESTEXE)
>>>>>  $1_COMPILATION_TYPE := EXECUTABLE
>>>>>else
>>>>> @@ -75,12 +75,12 @@
>>>>>  # Locate all files with the matching prefix
>>>>>$1_FILE_LIST := \
>>>>> -  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f -name 
>>>>> "$$($1_PREFIX)*.c")
>>>>> +  $$(shell $$(FIND) $$($1_SOURCE_DIRS) -type f \( -name 
>>>>> "$$($1_PREFIX)*.c" \
>>>>> +   -o -name 
>>>>> "$$($1_PREFIX)*.cpp" \))
>>>>>  $1_EXCLUDE_PATTERN := $$(addprefix %/, $$($1_EXCLUDE))
>>>>>$1_FILTERED_FILE_LIST := $$(filter-out $$($1_EXCLUDE_PATTERN), 
>>>>> $$($1_FILE_LIST))
>>>>>  -  # Setup a compilation for each and every one of them
>>>>>$$(foreach file, $$($1_FILTERED_FILE_LIST),\
>>>>>  $$

Re: 8209520: Build fails when native code coverage is enabled

[Igor Ignatyev]

Hi,

my claim was based on the warnings which we were getting when we just 
introduced code coverage builds in JDK 9, e.g. 8130790[1] (clobbered warning in 
libt2k). these warnings haven't been seen w/o code coverage enabled, and 
enabling coverage changes code path, so I don't think we should care much about 
these warnings.

I ain't saying that Leonid's fixes are wrong, I definitely support his changes 
in macroAssembler_x86 and genCollectedHeap (can't comment on splashscreen_png.c 
as I don't really understand what gcc complains there), I'm saying that b/c 
coverage-enabled builds aren't built often enough and warnings from these 
builds will always be low-priority bugs, I believe that such instrumented must 
be produced w/ warnings-as-errors disabled.

[1] https://bugs.openjdk.java.net/browse/JDK-8130790

Thanks,
-- Igor


> On Aug 30, 2018, at 6:26 AM, Erik Joelsson  wrote:
> 
> Hello,
> 
> On 2018-08-30 02:22, Magnus Ihse Bursie wrote:
>> 
>> 
>> On 2018-08-24 00:44, Igor Ignatev wrote:
>>> Hi Leonid,
>>> 
>>> We have never supported native code coverage builds with warnings enabled 
>>> as errors. There are bugs in gcc which cause false positive warnings, so it 
>>> was decided to ignore all warnings from instrumented builds. It’d be much 
>>> better and reliable to fix makefiles to always use 
>>> ‘disable-warning-as-errors’ when ‘enable-native-coverage’ is used. It 
>>> should be pretty straightforward to do.
>> I disagree.
>> 
>> While it is simple to change the build to disable warnings as error when 
>> building with code coverage, I think hard-coding this into the build system 
>> is a step backwards :-( and not something I want to support.
>> 
> I shared your opinion at first while discussing this offline with Leonid. 
> What changed my mind was the claim that the warnings cannot be truly trusted 
> when GCC is generating code coverage data. If that is true, then having 
> warnings as errors turned on then serves no purpose. The majority of builds 
> will be without code coverage enabled, so we will still get ample protection 
> against warnings in the code.
>> The code changes suggested by Leonid seems trivial and benign, and helps 
>> readability, even apart from fixing compiler warnings.
>> 
>> If this is deemed unacceptable, it's better to disable those few warnings 
>> specifically, for the files/libraries they occur in.
>> 
> If the claim on the warnings produced by GCC while generating code coverage 
> is false, then I certainly agree that the warnings should be fixed instead.
> 
> /Erik
>> /Magnus
>> 
>> 
>>> 
>>> cc’ing build alias.
>>> 
>>> Cheers,
>>> — Igor
>>> 
 On Aug 23, 2018, at 2:37 PM, Vladimir Kozlov  
 wrote:
 
 macroassembler changes are good.
 
 Thanks,
 Vladimir
 
> On 8/23/18 1:51 PM, Leonid Mesnik wrote:
> Hi
> Could you please review following fix which fix code so gcc doesn't 
> complain when JDK is build with enabled native code coverage.
> webrev: http://cr.openjdk.java.net/~lmesnik/8209520/webrev.00/ 
> 
> bug: https://bugs.openjdk.java.net/browse/JDK-8209520
> These warning appeared because of change optimization settings used for 
> getting code coverage.
> 1) src/hotspot/cpu/x86/macroAssembler_x86.cpp, 
> src/hotspot/share/gc/shared/genCollectedHeap.cpp
> gcc complained about uninitialized variables, like
> * For target hotspot_variant-server_libjvm_objs_macroAssembler_x86.o:
> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:
>  In member function 'void ControlWord::print() const':
> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:5769:11:
>  error: 'pc' may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
>   printf("%04x  masks = %s, %s, %s", _value & 0x, f, rc, pc);
> ~~^~~~
> /home/lmesnik/ws/jdk-8209520/open/src/hotspot/cpu/x86/macroAssembler_x86.cpp:5769:11:
>  error: 'rc' may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
> So I just fixed codepath to show more explicitly that variables are 
> initialized before usage.
> 2) src/java.desktop/share/native/libsplashscreen/splashscreen_png.c:
> The changes to prevent waning about clobbering in splashscreen_png.c are 
> similar to fix in:
> 1. JDK-8080695 
> splashscreen_png.c compile error with gcc 4.9.2
> The another approach would be to fix build to ignore these warnings for 
> code coverage build. While I think it makes build system even more 
> complicated.
> Leonid
>> 
> 

Re: 8209520: Build fails when native code coverage is enabled

[Igor Ignatyev]

Hi Magnus,

herein I will be talking only about 1st two warnings.
 
although your analyze is correct, it doesn't take into account the fact that 
the warnings report situations that can't happen in current codebase, and gcc 
doesn't report them in our "regular" builds b/c it can proof that
- in macroAssembler_x86.cpp both rounding_control() and precision_control() are 
>= 0 and <= 3, so switch statements do cover all possible cases
- in genCollectedHeap.cpp, removeSmallestScratch is called only from one place 
w/ prev_ptr being an address of local variable, therefore we will always visit 
while loop, so smallest_ptr and smallest will be inited.

so these warnings are false-possitive b/c they can never happen. one can argue 
that if we change the code, these issues might become real. that's true, but 
when it happens, gcc will report them as warnings for non-instrumented builds, 
as the compiler won't be able to proof that prev_ptr is always not null, or 
precision_control is from [0;3], etc. this is one of the reasons why these bugs 
won't be ever treated the same as other compiler errors. they are 
false-possitive from practical point of view and fixing them not always improve 
code quality, it some case we might end up w/ more clumsy code just to fix 
"potential" issues w/ zero probability to happen. if we want to use compiler 
warnings to improve quality, I'd suggest us to start w/ removing currently 
disabled warnings and/or start using clang static analyzer and clang tidy 
rather than fixing issues which will never happen.

disabling warnings in instrumented builds isn't the same as disabling all 
warnings in a single stroke, it's just disabling warnings in a build 
configuration we don't treat as important as other configurations.

as for the bug I posted, it was "fixed" by passing 
'--disable-warnings-as-errors' to jprt if native-coverage is enabled.

Thanks,
-- Igor

> On Aug 31, 2018, at 9:21 AM, Erik Joelsson  wrote:
> 
> On 2018-08-31 01:20, Magnus Ihse Bursie wrote:
>> 
>> These are two different arguments for turning off warnings for code coverage:
>> 1) gcc is producing incorrect warnings
>> 2) the warnings might be correct, but we are going to treat such bugs as low 
>> priority
>> 
>> 
>> I understand and accept 1, but I do not accept 2 as a valid reason for 
>> turning off warnings. I tried to dig through history in the 
>> (Oracle-internal) bug Igor posted, but in the end I could find no changes 
>> was made to any compiler flags..?
>> 
>> I have googled around but found no discussions at all that indicate that 
>> gcov should emit invalid warnings.
>> 
>> As for argument 2: we're using code coverage as a way to increase code 
>> quality, after all. If we get additional warnings, that indicate ways to 
>> improve the code, then we should use this to improve the code, not hide it 
>> away. And there's no reason not to treat such bugs as severe as any other 
>> compile errors.
>> 
> I agree and it seems I was assuming too much when I claimed 1.
>> Let's get more concrete: In this case, we have three warnings. To solve 
>> this, Leonid fixed:
>> 
>> * In macroAssembler_x86.cpp, he added ShouldNotReachHere(), which definitely 
>> improves code quality.
>> 
>> * In genCollectedHeap.cpp, he he initalized two local variables to NULL. 
>> After studying the code, I see that this is strictly not needed, and gcc is 
>> apparently losing some information here that could have been used to 
>> determine that the code is correct. However, my initial reaction when 
>> reading the code was that it was indeed broken, and it took me some time to 
>> prove that it was not. This is not good code readability. It's fragile, and 
>> some future code change might end up with a non-initialized local variable. 
>> So I think this is good, defensive programming, no matter what.
>> 
>> * In splashscreen_png.c, I'll honestly say that I don't know wether this fix 
>> is correct or not. But it does seem reasonable. The SplashDecodePng() 
>> function uses setjmp() *shudder* and for me, that's a big Here Be Dragons! 
>> sign. I also note that the success variable is read in the done: label, just 
>> below access to row_pointers and image_data -- which are both, already, 
>> declared volatile. So I'm guessing that declaring success volatile is in 
>> fact the correct thing to do. (I challenge anyone with a greater 
>> understanding of setjmp to prove me wrong...)
>> 
>> So I say we should be happy we got those warnings, and fix the code.
>> 
>> I don't see this as an argument that gcc emits invalid warnings. If that 
>> happens in the future, then we can discuss eliminating those warning. But if 
>> we do, we should eliminate them selectively, only the incorrect warnings and 
>> only on the files/libraries where they occur erroneously. Not disable all 
>> warnings in a single stroke.
>> 
> Thank you for the detailed analysis. With this I definitely agree that it's 
> better to fix the code.
> 
> /Erik
>> /Magnus

RFR(S): remove ExecuteInternalVMTests and VerboseInternalVMTests flags

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8213058/webrev.00/index.html
> 174 lines changed: 0 ins; 170 del; 4 mod; 

Hi all,

could you please review this small clean up which removes 
ExecuteInternalVMTests and VerboseInternalVMTests flags and related make 
targets and tests?

8177708[1-3] is to convert the last of internal vm tests, so the whole 
InternalVMTests can be removed.

JBS: https://bugs.openjdk.java.net/browse/JDK-8213058
webrev: http://cr.openjdk.java.net/~iignatyev//8213058/webrev.00/index.html

testing: tier1, build all regular platforms
[1] https://bugs.openjdk.java.net/browse/JDK-8177708
[2] http://cr.openjdk.java.net/~iignatyev//8177708/webrev.00/index.html
[3] 
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-October/030633.html

Thanks,
-- Igor

Re: RFR(S): remove ExecuteInternalVMTests and VerboseInternalVMTests flags

[Igor Ignatyev]

Hi David,

removing usage of test_log will just mix "unneeded" changes w/ this clean up. 
as TestReservedSpace_test, TestReserveMemorySpecial_test, 
TestVirtualSpace_test, and TestMetaspaceUtils_test are to be removed by 
8213269[*], I don't think we need to pay much attention to their code.

[*] https://bugs.openjdk.java.net/browse/JDK-8213269 
<https://bugs.openjdk.java.net/browse/JDK-8213269>

-- Igor

> On Nov 1, 2018, at 4:16 PM, David Holmes  wrote:
> 
> Hi Igor,
> 
> There's no point having empty test_log macros that do nothing. The macro and 
> all uses should just be deleted ... unless you plan on adding some other form 
> of logging for this?
> 
> Thanks,
> David
> 
> On 2/11/2018 7:15 AM, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8213058/webrev.00/index.html
>>> 174 lines changed: 0 ins; 170 del; 4 mod;
>> Hi all,
>> could you please review this small clean up which removes 
>> ExecuteInternalVMTests and VerboseInternalVMTests flags and related make 
>> targets and tests?
>> 8177708[1-3] is to convert the last of internal vm tests, so the whole 
>> InternalVMTests can be removed.
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8213058
>> webrev: http://cr.openjdk.java.net/~iignatyev//8213058/webrev.00/index.html
>> testing: tier1, build all regular platforms
>> [1] https://bugs.openjdk.java.net/browse/JDK-8177708
>> [2] http://cr.openjdk.java.net/~iignatyev//8177708/webrev.00/index.html
>> [3] 
>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-October/030633.html
>> Thanks,
>> -- Igor

Re: RFR(S): remove ExecuteInternalVMTests and VerboseInternalVMTests flags

[Igor Ignatyev]

on a second though, removing these macros isn't that big, here is an 
incremental webrev: http://cr.openjdk.java.net/~iignatyev//8213058/webrev.0-1 
<http://cr.openjdk.java.net/~iignatyev//8213058/webrev.0-1/index.html>. besides 
removing test_log, it also include fixes in build and doc needed due to 
rebasing.

Erik, could you please re-review build changes?

http://cr.openjdk.java.net/~iignatyev//8213058/webrev.01/ 
<http://cr.openjdk.java.net/~iignatyev//8213058/webrev.01/> is the whole webrev.

Thanks,
-- Igor


> On Nov 1, 2018, at 4:23 PM, Igor Ignatyev  wrote:
> 
> Hi David,
> 
> removing usage of test_log will just mix "unneeded" changes w/ this clean up. 
> as TestReservedSpace_test, TestReserveMemorySpecial_test, 
> TestVirtualSpace_test, and TestMetaspaceUtils_test are to be removed by 
> 8213269[*], I don't think we need to pay much attention to their code.
> 
> [*] https://bugs.openjdk.java.net/browse/JDK-8213269 
> <https://bugs.openjdk.java.net/browse/JDK-8213269>
> 
> -- Igor
> 
>> On Nov 1, 2018, at 4:16 PM, David Holmes  wrote:
>> 
>> Hi Igor,
>> 
>> There's no point having empty test_log macros that do nothing. The macro and 
>> all uses should just be deleted ... unless you plan on adding some other 
>> form of logging for this?
>> 
>> Thanks,
>> David
>> 
>> On 2/11/2018 7:15 AM, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev//8213058/webrev.00/index.html
>>>> 174 lines changed: 0 ins; 170 del; 4 mod;
>>> Hi all,
>>> could you please review this small clean up which removes 
>>> ExecuteInternalVMTests and VerboseInternalVMTests flags and related make 
>>> targets and tests?
>>> 8177708[1-3] is to convert the last of internal vm tests, so the whole 
>>> InternalVMTests can be removed.
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8213058
>>> webrev: http://cr.openjdk.java.net/~iignatyev//8213058/webrev.00/index.html
>>> testing: tier1, build all regular platforms
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8177708
>>> [2] http://cr.openjdk.java.net/~iignatyev//8177708/webrev.00/index.html
>>> [3] 
>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-October/030633.html
>>> Thanks,
>>> -- Igor
> 

Re: RFR(S): remove ExecuteInternalVMTests and VerboseInternalVMTests flags

[Igor Ignatyev]

Hi Magnus,

> You still need to fix FindTests.gmk, though.
thanks for spotting that. is [*] enough, or did I miss something else?

-- Igor

[*]
> diff -r c6d128f60997 make/common/FindTests.gmk
> --- a/make/common/FindTests.gmk Thu Nov 01 16:33:43 2018 -0700
> +++ b/make/common/FindTests.gmk Fri Nov 02 10:02:22 2018 -0700
> @@ -79,7 +79,7 @@
>  ALL_NAMED_TESTS += $(addprefix make-, $(MAKE_TEST_TARGETS))
>  
>  # Add special tests
> -ALL_NAMED_TESTS += hotspot-internal failure-handler make
> +ALL_NAMED_TESTS += failure-handler make
>  
>  
> 


> On Nov 2, 2018, at 1:27 AM, Magnus Ihse Bursie 
>  wrote:
> 
> 
> On 2018-11-02 00:37, Igor Ignatyev wrote:
>> on a second though, removing these macros isn't that big, here is an 
>> incremental webrev: 
>> http://cr.openjdk.java.net/~iignatyev//8213058/webrev.0-1 
>> <http://cr.openjdk.java.net/~iignatyev//8213058/webrev.0-1/index.html>. 
>> besides removing test_log, it also include fixes in build and doc needed due 
>> to rebasing.
>> 
>> Erik, could you please re-review build changes?
>> 
>> http://cr.openjdk.java.net/~iignatyev//8213058/webrev.01/ 
>> <http://cr.openjdk.java.net/~iignatyev//8213058/webrev.01/> is the whole 
>> webrev.
> 
> Sorry, I missed your updated webrev. You still need to fix FindTests.gmk, 
> though.
> 
> /Magnus
> 
> 
> 
>> 
>> Thanks,
>> -- Igor
>> 
>> 
>>> On Nov 1, 2018, at 4:23 PM, Igor Ignatyev  wrote:
>>> 
>>> Hi David,
>>> 
>>> removing usage of test_log will just mix "unneeded" changes w/ this clean 
>>> up. as TestReservedSpace_test, TestReserveMemorySpecial_test, 
>>> TestVirtualSpace_test, and TestMetaspaceUtils_test are to be removed by 
>>> 8213269[*], I don't think we need to pay much attention to their code.
>>> 
>>> [*] https://bugs.openjdk.java.net/browse/JDK-8213269 
>>> <https://bugs.openjdk.java.net/browse/JDK-8213269>
>>> 
>>> -- Igor
>>> 
>>>> On Nov 1, 2018, at 4:16 PM, David Holmes  wrote:
>>>> 
>>>> Hi Igor,
>>>> 
>>>> There's no point having empty test_log macros that do nothing. The macro 
>>>> and all uses should just be deleted ... unless you plan on adding some 
>>>> other form of logging for this?
>>>> 
>>>> Thanks,
>>>> David
>>>> 
>>>> On 2/11/2018 7:15 AM, Igor Ignatyev wrote:
>>>>> http://cr.openjdk.java.net/~iignatyev//8213058/webrev.00/index.html
>>>>>> 174 lines changed: 0 ins; 170 del; 4 mod;
>>>>> Hi all,
>>>>> could you please review this small clean up which removes 
>>>>> ExecuteInternalVMTests and VerboseInternalVMTests flags and related make 
>>>>> targets and tests?
>>>>> 8177708[1-3] is to convert the last of internal vm tests, so the whole 
>>>>> InternalVMTests can be removed.
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8213058
>>>>> webrev: 
>>>>> http://cr.openjdk.java.net/~iignatyev//8213058/webrev.00/index.html
>>>>> testing: tier1, build all regular platforms
>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8177708
>>>>> [2] http://cr.openjdk.java.net/~iignatyev//8177708/webrev.00/index.html
>>>>> [3] 
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-October/030633.html
>>>>> Thanks,
>>>>> -- Igor
> 

Re: [13] RFR (S): 8217404: --with-jvm-features doesn't work when multiple features are explicitly disabled

[Igor Ignatyev]

Hi Vladimir,

overall your fix looks reasonable, but w/ it we can get unintentionally 
disabled features (b/c grep doesn't do full word match). although this problem 
wasn't really introduced by your fix, I think it's be better to fix it as a 
part of your patch. I see two possible solutions:
 -  add "-w" to grep, but I am not sure if "-w" is supported by all grep 
implementations
 - use $XARGS instead of $ECHO when we get DISABLE_X. in this case you will 
need to revert your changes in 'if test ...' lines

Thanks,
-- Igor

> On Jan 18, 2019, at 3:33 PM, Vladimir Ivanov  
> wrote:
> 
> http://cr.openjdk.java.net/~vlivanov/8217404/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8217404
> 
> --with-jvm-features doesn't work properly when multiple features are 
> explicitly disabled:
> 
> $ bash configure --with-jvm-features="-aot -jvmci -graal"
> ...
> checking if jvmci module jdk.internal.vm.ci should be built... yes
> checking if graal module jdk.internal.vm.compiler should be built... yes
> checking if aot should be enabled... yes
> ...
> 
> The problem in the following code:
> 
>  DISABLE_AOT=`$ECHO $DISABLED_JVM_FEATURES | $GREP aot`
>  if test "x$DISABLE_AOT" = "xaot"; then
>ENABLE_AOT="false"
>  fi
> 
> Since DISABLED_JVM_FEATURES ("aot jvmci graal") contains the list of 
> explicitly disabled features, grep over it returns the whole list when 
> there's a match. The subsequent check fails because there's no exact match, 
> though DISABLE_AOT contains "aot" .
> 
> Proposed fix is to check there's no match instead.
> 
> After the fix it works as expected:
> 
> $ bash configure --with-jvm-features="-aot -jvmci -graal"
> ...
> checking if jvmci module jdk.internal.vm.ci should be built... no, forced
> checking if graal module jdk.internal.vm.compiler should be built... no, 
> forced
> checking if aot should be enabled... no, forced
> ...
> 
> (The fix doesn't address the case when one feature has a name which is a 
> proper substring of another feature, but there are no such cases at the 
> moment.)
> 
> Best regards,
> Vladimir Ivanov

Re: RFR: JDK-8217613: [AOT] TEST_OPTS_AOT_MODULES doesn't work on mac

[Igor Ignatyev]

Hi Erik,

I don't like that it's based on the assumption that ld and clang/gcc are in the 
same directory, but this assumption seems to be always true for now. so unless 
there is an easy way to get ld path, I'm fine w/ this fix.

-- Igor

> On Jan 23, 2019, at 1:18 PM, Vladimir Kozlov  
> wrote:
> 
> Looks good.
> 
> Thanks,
> Vladimir
> 
> On 1/23/19 12:03 PM, Erik Joelsson wrote:
>> The TEST_OPTS=AOT_MODULES option does not work when running tests locally on 
>> Macosx. This is because jaot expects the linker to be "ld", while we define 
>> the linker (LD) to be clang on Macosx. When running tests on already built 
>> JDKs using run-test-prebuilt, we already setup LD to point to "ld" rather 
>> than "clang", so the same should be done when running local tests.
>> This patch rewrites LD for jaotc, on Macosx and Linux. Though we did not 
>> have a problem on Linux, the run-test-prebuilt case does set LD to "ld" on 
>> Linux so better have it consistent.
>> I also added -XX:+UnlockDiagnosticVMOptions to the verification command to 
>> make it work on release builds where this flag is default off, as well as 
>> cleaned up some whitespace.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217613
>> Webrev: http://cr.openjdk.java.net/~erikj/8217613/webrev.01/
>> /Erik

Re: RFR: JDK-8217613: [AOT] TEST_OPTS_AOT_MODULES doesn't work on mac

[Igor Ignatyev]

so far it has affected only my local runs, so we are in no hurry to get this 
fixed ;) you can work on a better fix, I, meanwhile, will apply your patch to 
my local ws as a workaround.

-- Igor  

> On Jan 23, 2019, at 3:10 PM, Erik Joelsson  wrote:
> 
> On 2019-01-23 13:38, Igor Ignatyev wrote:
>> Hi Erik,
>> 
>> I don't like that it's based on the assumption that ld and clang/gcc are in 
>> the same directory, but this assumption seems to be always true for now. so 
>> unless there is an easy way to get ld path, I'm fine w/ this fix.
> 
> I don't either, and I was in a bit of a hurry so didn't think further. It 
> would of course be cleaner to have configure find ld for us in a separate 
> variable since this is only a problem in the local/configured case.
> 
> /Erik
> 
>> -- Igor
>> 
>>> On Jan 23, 2019, at 1:18 PM, Vladimir Kozlov  
>>> wrote:
>>> 
>>> Looks good.
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>> On 1/23/19 12:03 PM, Erik Joelsson wrote:
>>>> The TEST_OPTS=AOT_MODULES option does not work when running tests locally 
>>>> on Macosx. This is because jaot expects the linker to be "ld", while we 
>>>> define the linker (LD) to be clang on Macosx. When running tests on 
>>>> already built JDKs using run-test-prebuilt, we already setup LD to point 
>>>> to "ld" rather than "clang", so the same should be done when running local 
>>>> tests.
>>>> This patch rewrites LD for jaotc, on Macosx and Linux. Though we did not 
>>>> have a problem on Linux, the run-test-prebuilt case does set LD to "ld" on 
>>>> Linux so better have it consistent.
>>>> I also added -XX:+UnlockDiagnosticVMOptions to the verification command to 
>>>> make it work on release builds where this flag is default off, as well as 
>>>> cleaned up some whitespace.
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217613
>>>> Webrev: http://cr.openjdk.java.net/~erikj/8217613/webrev.01/
>>>> /Erik

Re: RFR: JDK-8217613: [AOT] TEST_OPTS_AOT_MODULES doesn't work on mac

[Igor Ignatyev]

LGTM!

-- Igor

> On Jan 23, 2019, at 4:16 PM, Erik Joelsson  wrote:
> 
> Hello again,
> 
> Here is a better fix as described below. Configure finds and sets LD_JAOTC 
> specifically tailored for the needs of jaotc for each platform.
> 
> http://cr.openjdk.java.net/~erikj/8217613/webrev.02/index.html
> 
> Verified with local test using TEST_OPTS=AOT_MODULES=java.base on Macosx, 
> Linux and Windows, as well as mach5 run of the same.
> 
> /Erik
> 
> On 2019-01-23 15:55, Igor Ignatyev wrote:
>> so far it has affected only my local runs, so we are in no hurry to get this 
>> fixed ;) you can work on a better fix, I, meanwhile, will apply your patch 
>> to my local ws as a workaround.
>> 
>> -- Igor
>> 
>>> On Jan 23, 2019, at 3:10 PM, Erik Joelsson  wrote:
>>> 
>>> On 2019-01-23 13:38, Igor Ignatyev wrote:
>>>> Hi Erik,
>>>> 
>>>> I don't like that it's based on the assumption that ld and clang/gcc are 
>>>> in the same directory, but this assumption seems to be always true for 
>>>> now. so unless there is an easy way to get ld path, I'm fine w/ this fix.
>>> I don't either, and I was in a bit of a hurry so didn't think further. It 
>>> would of course be cleaner to have configure find ld for us in a separate 
>>> variable since this is only a problem in the local/configured case.
>>> 
>>> /Erik
>>> 
>>>> -- Igor
>>>> 
>>>>> On Jan 23, 2019, at 1:18 PM, Vladimir Kozlov  
>>>>> wrote:
>>>>> 
>>>>> Looks good.
>>>>> 
>>>>> Thanks,
>>>>> Vladimir
>>>>> 
>>>>> On 1/23/19 12:03 PM, Erik Joelsson wrote:
>>>>>> The TEST_OPTS=AOT_MODULES option does not work when running tests 
>>>>>> locally on Macosx. This is because jaot expects the linker to be "ld", 
>>>>>> while we define the linker (LD) to be clang on Macosx. When running 
>>>>>> tests on already built JDKs using run-test-prebuilt, we already setup LD 
>>>>>> to point to "ld" rather than "clang", so the same should be done when 
>>>>>> running local tests.
>>>>>> This patch rewrites LD for jaotc, on Macosx and Linux. Though we did not 
>>>>>> have a problem on Linux, the run-test-prebuilt case does set LD to "ld" 
>>>>>> on Linux so better have it consistent.
>>>>>> I also added -XX:+UnlockDiagnosticVMOptions to the verification command 
>>>>>> to make it work on release builds where this flag is default off, as 
>>>>>> well as cleaned up some whitespace.
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8217613
>>>>>> Webrev: http://cr.openjdk.java.net/~erikj/8217613/webrev.01/
>>>>>> /Erik

RFR(S) : 8219391 : extend gcov support to llvm/clang

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8219391/webrev.00
> 21 lines changed: 3 ins; 0 del; 18 mod;

Hi all,

could you please review this small and trivial patch which allows usage of 
--enable-native-coverage configure option w/ clang devkit?

> llvm/clang supports gcov in the same way gcc does, it has the same 
> compiler/linker options to enable code coverage information, produces the 
> same .gcda files; the produced binaries generate .gcdo files and treat env. 
> variables in the same way.

webrev: http://cr.openjdk.java.net/~iignatyev//8219391/webrev.00
JBS: https://bugs.openjdk.java.net/browse/JDK-8219391
testing: built on mac w/ --enable-native-coverage, run some tests and 
spot-checked coverage

Thanks,
-- Igor
  

Re: RFR(S) : 8219391 : extend gcov support to llvm/clang

[Igor Ignatyev]

thanks Erik, pushed.

y, this means exactly that.

-- Igor

> On Feb 19, 2019, at 12:35 PM, Erik Joelsson  wrote:
> 
> Looks good.
> 
> Does this mean it works on Macos?
> 
> /Erik
> 
> On 2019-02-19 11:46, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8219391/webrev.00
>>> 21 lines changed: 3 ins; 0 del; 18 mod;
>> Hi all,
>> 
>> could you please review this small and trivial patch which allows usage of 
>> --enable-native-coverage configure option w/ clang devkit?
>> 
>>> llvm/clang supports gcov in the same way gcc does, it has the same 
>>> compiler/linker options to enable code coverage information, produces the 
>>> same .gcda files; the produced binaries generate .gcdo files and treat env. 
>>> variables in the same way.
>> webrev: http://cr.openjdk.java.net/~iignatyev//8219391/webrev.00
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219391
>> testing: built on mac w/ --enable-native-coverage, run some tests and 
>> spot-checked coverage
>> 
>> Thanks,
>> -- Igor
>>   

RFR(T) : 8219132 : switch to jtreg4.2-b14

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8219132/webrev.00/index.html
> 1 line changed: 0 ins; 0 del; 1 mod;

Hi all,

could you please review this one-liner which switch jtreg version to 
jtreg4.2-b14?

webrev: http://cr.openjdk.java.net/~iignatyev//8219132/webrev.00/index.html
JBS:  https://bugs.openjdk.java.net/browse/JDK-8219132
testing: tier[1-5], client tests and jcstress test group testing showed no new 
failures; spot checking of .jtr files also didn't find any anomalies

Thanks,
-- Igor

Re: RFR(T) : 8219132 : switch to jtreg4.2-b14

[Igor Ignatyev]

Hi Erik,

thanks for your review. 

we can't just bump the required version as it can introduce changes in tests' 
behavior. requiredVersion >= 4.2 b14 will enable allowSmartActionArgs[1], this 
will affect all the tests which have ${} in their @run directives, for 
example it will affect many vmTestbase tests which use 
PropertyResolvingWrapper. therefore, I suggest we bump requiredVersion after we 
clean up such tests and have filed 8219140[2] to deal w/ vmTestbase tests. I'll 
check if there are other tests which will be affected and will file RFE(s) to 
cover them as well.

[1] https://bugs.openjdk.java.net/browse/CODETOOLS-7902352 
<https://bugs.openjdk.java.net/browse/CODETOOLS-7902352>
[2] https://bugs.openjdk.java.net/browse/JDK-8219140 
<https://bugs.openjdk.java.net/browse/JDK-8219140>

Thanks,
-- Igor

> On Feb 19, 2019, at 5:01 PM, Erik Joelsson  wrote:
> 
> Looks good.
> 
> Do we need to also bump the required version in the TEST.ROOT files?
> 
> /Erik
> 
> On 2019-02-19 16:34, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8219132/webrev.00/index.html
>>> 1 line changed: 0 ins; 0 del; 1 mod;
>> Hi all,
>> 
>> could you please review this one-liner which switch jtreg version to 
>> jtreg4.2-b14?
>> 
>> webrev: http://cr.openjdk.java.net/~iignatyev//8219132/webrev.00/index.html
>> JBS:  https://bugs.openjdk.java.net/browse/JDK-8219132
>> testing: tier[1-5], client tests and jcstress test group testing showed no 
>> new failures; spot checking of .jtr files also didn't find any anomalies
>> 
>> Thanks,
>> -- Igor

RFR(S) : 8219395 : integrate gcov w/ run-test

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8219395/webrev.00/index.html
> 65 lines changed: 59 ins; 0 del; 6 mod;

Hi all,

could you please review the patch which makes it easy to run tests on the 
builds w/ native-code-coverage enabled? to do so the patch
 - sets GCOV_PREFIX env. variable, so .gcda files will be stored in 
build/*/test-results/gcov-output directory, and makes jtreg to propagate this 
env. variable to JDK under test
 - adds linux-x64-gcov and macosx-x64-gcov jib profiles
 - changes 'run-test-prebuilt' profile to set GCOV_ENABLED=true if it's the 
tested profile is -gcov profile

and also fixes comment for JDKOPT_SETUP_CODE_COVERAGE in jdk-options.m4.

webrev: http://cr.openjdk.java.net/~iignatyev//8219395/webrev.00/index.html
JBS: https://bugs.openjdk.java.net/browse/JDK-8219395
testing: 
 - :tier1 on {linux,macos}-x64 and {linux,macos}-x64-gcov
 - checked that *-gcov builds have .gcno files generated and stored in symbols 
bundle; and regular builds don't 
 - checked that *-gcov runs have .gcda files generated in 
test-results/gcov-output; and runs on regular builds don't

Thanks,
-- Igor

Re: RFR(T) : 8219132 : switch to jtreg4.2-b14

[Igor Ignatyev]

Joe,

allowSmartActionArgs is opt-in if requiredVersion < b14, and opt-out if 
requiredVersion >= b14, please see 
http://hg.openjdk.java.net/code-tools/jtreg/rev/36c592d2f544 
<http://hg.openjdk.java.net/code-tools/jtreg/rev/36c592d2f544> . so you don't 
have to have allowSmartActionArgs=true in 8219254.

Thanks,
-- Igor

> On Feb 19, 2019, at 5:49 PM, Joseph D. Darcy  wrote:
> 
> Hello,
> 
> On 2/19/2019 5:06 PM, Igor Ignatyev wrote:
>> Hi Erik,
>> 
>> thanks for your review.
>> 
>> we can't just bump the required version as it can introduce changes in 
>> tests' behavior. requiredVersion >= 4.2 b14 will enable 
>> allowSmartActionArgs[1],
> 
> From what I understand from Jon, allowSmartActionArgs is opt-in and has to be 
> enabled in the TEST.ROOT file with
> 
>allowSmartActionArgs=true
> 
> Therefore, IIRC, the required version could be bumped without enabling smart 
> action args.
> 
> (I have a patch in progress to use smart action args for langtools tests: 
> https://mail.openjdk.java.net/pipermail/compiler-dev/2019-February/012959.html)
> 
> HTH,
> 
> -Joe
> 
>>  this will affect all the tests which have ${} in their @run directives, 
>> for example it will affect many vmTestbase tests which use 
>> PropertyResolvingWrapper. therefore, I suggest we bump requiredVersion after 
>> we clean up such tests and have filed 8219140[2] to deal w/ vmTestbase 
>> tests. I'll check if there are other tests which will be affected and will 
>> file RFE(s) to cover them as well.
>> 
>> [1] https://bugs.openjdk.java.net/browse/CODETOOLS-7902352 
>> <https://bugs.openjdk.java.net/browse/CODETOOLS-7902352>
>> [2] https://bugs.openjdk.java.net/browse/JDK-8219140 
>> <https://bugs.openjdk.java.net/browse/JDK-8219140>
>> 
>> Thanks,
>> -- Igor
>> 
>>> On Feb 19, 2019, at 5:01 PM, Erik Joelsson  wrote:
>>> 
>>> Looks good.
>>> 
>>> Do we need to also bump the required version in the TEST.ROOT files?
>>> 
>>> /Erik
>>> 
>>> On 2019-02-19 16:34, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8219132/webrev.00/index.html
>>>>> 1 line changed: 0 ins; 0 del; 1 mod;
>>>> Hi all,
>>>> 
>>>> could you please review this one-liner which switch jtreg version to 
>>>> jtreg4.2-b14?
>>>> 
>>>> webrev: http://cr.openjdk.java.net/~iignatyev//8219132/webrev.00/index.html
>>>> JBS:  https://bugs.openjdk.java.net/browse/JDK-8219132
>>>> testing: tier[1-5], client tests and jcstress test group testing showed no 
>>>> new failures; spot checking of .jtr files also didn't find any anomalies
>>>> 
>>>> Thanks,
>>>> -- Igor
> 

Re: RFR(T) : 8219132 : switch to jtreg4.2-b14

[Igor Ignatyev]

I've pushed the patch. 

for the reference, 8219408 
<https://bugs.openjdk.java.net/browse/JDK-8219408%5B*> [1] was filed to handle 
jdk tests which have ${; the only affected hotspot tests are vmTestbase tests, 
so 8219140 <https://bugs.openjdk.java.net/browse/JDK-8219140> [2] covers all 
hotspot tests.

[1] https://bugs.openjdk.java.net/browse/JDK-8219408 
<https://bugs.openjdk.java.net/browse/JDK-8219408>
[2] https://bugs.openjdk.java.net/browse/JDK-8219140 
<https://bugs.openjdk.java.net/browse/JDK-8219140>

Thanks,
-- Igor


> On Feb 19, 2019, at 5:06 PM, Igor Ignatyev  wrote:
> 
> Hi Erik,
> 
> thanks for your review. 
> 
> we can't just bump the required version as it can introduce changes in tests' 
> behavior. requiredVersion >= 4.2 b14 will enable allowSmartActionArgs[1], 
> this will affect all the tests which have ${} in their @run directives, 
> for example it will affect many vmTestbase tests which use 
> PropertyResolvingWrapper. therefore, I suggest we bump requiredVersion after 
> we clean up such tests and have filed 8219140[2] to deal w/ vmTestbase tests. 
> I'll check if there are other tests which will be affected and will file 
> RFE(s) to cover them as well.
> 
> [1] https://bugs.openjdk.java.net/browse/CODETOOLS-7902352 
> <https://bugs.openjdk.java.net/browse/CODETOOLS-7902352>
> [2] https://bugs.openjdk.java.net/browse/JDK-8219140 
> <https://bugs.openjdk.java.net/browse/JDK-8219140>
> 
> Thanks,
> -- Igor
> 
>> On Feb 19, 2019, at 5:01 PM, Erik Joelsson  wrote:
>> 
>> Looks good.
>> 
>> Do we need to also bump the required version in the TEST.ROOT files?
>> 
>> /Erik
>> 
>> On 2019-02-19 16:34, Igor Ignatyev wrote:
>>> http://cr.openjdk.java.net/~iignatyev//8219132/webrev.00/index.html
>>>> 1 line changed: 0 ins; 0 del; 1 mod;
>>> Hi all,
>>> 
>>> could you please review this one-liner which switch jtreg version to 
>>> jtreg4.2-b14?
>>> 
>>> webrev: http://cr.openjdk.java.net/~iignatyev//8219132/webrev.00/index.html
>>> JBS:  https://bugs.openjdk.java.net/browse/JDK-8219132
>>> testing: tier[1-5], client tests and jcstress test group testing showed no 
>>> new failures; spot checking of .jtr files also didn't find any anomalies
>>> 
>>> Thanks,
>>> -- Igor
> 

Re: RFR(S) : 8219395 : integrate gcov w/ run-test

[Igor Ignatyev]

Hi Erik,

thanks for your review, I've removed the commented lines and changed 
default_make_targets, 
http://cr.openjdk.java.net/~iignatyev//8219395/webrev.0-1/index.html 
<http://cr.openjdk.java.net/~iignatyev//8219395/webrev.0-1/index.html> is 
incremental webrev.

Thanks,
-- Igor

> On Feb 20, 2019, at 6:59 AM, Erik Joelsson  wrote:
> 
> Hello Igor,
> 
> This looks pretty good. Just a few comments.
> 
> In jib-profiles.js, the linux-x64 profile also builds docs-bundles, so if you 
> base linux-x64-gcov on a clone of that there is some extra build work being 
> done unnecessarily. I would recommend explicitly setting the 
> default_make_targets (which would be product-bundles and test-bundles) for 
> the new *-gcov profiles.
> 
> On lines 795, 804 and 812 you seem to have left commented out code that 
> should probably be removed.
> 
> /Erik
> 
> On 2019-02-19 17:26, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8219395/webrev.00/index.html
>>> 65 lines changed: 59 ins; 0 del; 6 mod;
>> Hi all,
>> 
>> could you please review the patch which makes it easy to run tests on the 
>> builds w/ native-code-coverage enabled? to do so the patch
>>  - sets GCOV_PREFIX env. variable, so .gcda files will be stored in 
>> build/*/test-results/gcov-output directory, and makes jtreg to propagate 
>> this env. variable to JDK under test
>>  - adds linux-x64-gcov and macosx-x64-gcov jib profiles
>>  - changes 'run-test-prebuilt' profile to set GCOV_ENABLED=true if it's the 
>> tested profile is -gcov profile
>> 
>> and also fixes comment for JDKOPT_SETUP_CODE_COVERAGE in jdk-options.m4.
>> 
>> webrev: http://cr.openjdk.java.net/~iignatyev//8219395/webrev.00/index.html
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219395
>> testing:
>>  - :tier1 on {linux,macos}-x64 and {linux,macos}-x64-gcov
>>  - checked that *-gcov builds have .gcno files generated and stored in 
>> symbols bundle; and regular builds don't
>>  - checked that *-gcov runs have .gcda files generated in 
>> test-results/gcov-output; and runs on regular builds don't
>> 
>> Thanks,
>> -- Igor

RFR(L/XS) : 8222414 : bring googlemock v1.8.1

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
> 21638 lines changed: 21628 ins; 0 del; 10 mod;

Hi all,

could you please review this patch which makes mocking framework from google 
test available for hotspot tests?

JBS: https://bugs.openjdk.java.net/browse/JDK-8222414
testing: tier1
webrevs:
 - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.00
> 21605 lines changed: 21605 ins; 0 del; 0 mod; 
* moves gtest from test/fmw/gtest to test/fmw/gtest/googletest;
* adds only required (w/o build-aux, docs, make, msvc, scripts, tests 
directories and make related files) source of gmock to 
test/fmw/gtest/googlemock;

- http://cr.openjdk.java.net/~iignatyev//8222414/webrev.0-1
> 33 lines changed: 23 ins; 0 del; 10 mod; 

* makes necessary changes in makefiles
* calls gmock framework init function
* works around the conflict b/w :testing::internal::Log function defined by 
gmock and Log macro defined by hotspot 

- http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
> 21638 lines changed: 21628 ins; 0 del; 10 mod;

all the changes together


thanks,
-- Igor

Re: RFR(L/XS) : 8222414 : bring googlemock v1.8.1

[Igor Ignatyev]

Hi Erik,

thanks for your review. I've updated CompileGtest.gmk as you advised[1], which 
gives [2].

-- Igor

[1]
> diff -r 3443e083223d make/hotspot/lib/CompileGtest.gmk
> --- a/make/hotspot/lib/CompileGtest.gmk Thu May 23 19:03:32 2019 -0700
> +++ b/make/hotspot/lib/CompileGtest.gmk Thu May 23 19:04:09 2019 -0700
> @@ -64,8 +64,9 @@
>  EXCLUDES := $(JVM_EXCLUDES), \
>  EXCLUDE_FILES := gtestLauncher.cpp, \
>  EXCLUDE_PATTERNS := $(JVM_EXCLUDE_PATTERNS), \
> -EXTRA_FILES := $(GTEST_FRAMEWORK_SRC)/googletest/src/gtest-all.cc \
> -   $(GTEST_FRAMEWORK_SRC)/googlemock/src/gmock-all.cc, \
> +EXTRA_FILES := \
> +$(GTEST_FRAMEWORK_SRC)/googletest/src/gtest-all.cc \
> +$(GTEST_FRAMEWORK_SRC)/googlemock/src/gmock-all.cc, \
>  EXTRA_OBJECT_FILES := $(filter-out %/operator_new$(OBJ_SUFFIX), \
>  $(BUILD_LIBJVM_ALL_OBJS)), \
>  CFLAGS := $(JVM_CFLAGS) \
[2] http://cr.openjdk.java.net/~iignatyev/8222414/webrev.1-2/

> On May 23, 2019, at 6:57 AM, Erik Joelsson  wrote:
> 
> Hello Igor,
> 
> Build changes look ok, with a (very) minor style suggestion [1] (point 18). 
> We try to avoid padding continuation line breaks, so I would appreciate if 
> CompileGtest.gmk:68 could be indented just 4 extra spaces compared to line 
> 67. If you want the file names to line up, it's acceptable to break after the 
> := on 67.
> 
> [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
> 
> /Erik
> 
> On 2019-05-22 22:20, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
>>> 21638 lines changed: 21628 ins; 0 del; 10 mod;
>> Hi all,
>> 
>> could you please review this patch which makes mocking framework from google 
>> test available for hotspot tests?
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8222414
>> testing: tier1
>> webrevs:
>>  - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.00
>>> 21605 lines changed: 21605 ins; 0 del; 0 mod;
>> * moves gtest from test/fmw/gtest to test/fmw/gtest/googletest;
>> * adds only required (w/o build-aux, docs, make, msvc, scripts, tests 
>> directories and make related files) source of gmock to 
>> test/fmw/gtest/googlemock;
>> 
>> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.0-1
>>> 33 lines changed: 23 ins; 0 del; 10 mod;
>> * makes necessary changes in makefiles
>> * calls gmock framework init function
>> * works around the conflict b/w :testing::internal::Log function defined by 
>> gmock and Log macro defined by hotspot
>> 
>> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
>>> 21638 lines changed: 21628 ins; 0 del; 10 mod;
>> all the changes together
>> 
>> 
>> thanks,
>> -- Igor

Re: RFR(L/XS) : 8222414 : bring googlemock v1.8.1

[Igor Ignatyev]

@Erik, Thanks!

@hotspot (looking at Robin), can I get another review? 

-- Igor

> On May 24, 2019, at 8:28 AM, Erik Joelsson  wrote:
> 
> Thanks, looks good!
> 
> /Erik
> 
> On 2019-05-23 19:07, Igor Ignatyev wrote:
>> Hi Erik,
>> 
>> thanks for your review. I've updated CompileGtest.gmk as you advised[1], 
>> which gives [2].
>> 
>> -- Igor
>> 
>> [1]
>>> diff -r 3443e083223d make/hotspot/lib/CompileGtest.gmk
>>> --- a/make/hotspot/lib/CompileGtest.gmk Thu May 23 19:03:32 2019 -0700
>>> +++ b/make/hotspot/lib/CompileGtest.gmk Thu May 23 19:04:09 2019 -0700
>>> @@ -64,8 +64,9 @@
>>>  EXCLUDES := $(JVM_EXCLUDES), \
>>>  EXCLUDE_FILES := gtestLauncher.cpp, \
>>>  EXCLUDE_PATTERNS := $(JVM_EXCLUDE_PATTERNS), \
>>> -EXTRA_FILES := $(GTEST_FRAMEWORK_SRC)/googletest/src/gtest-all.cc \
>>> -   $(GTEST_FRAMEWORK_SRC)/googlemock/src/gmock-all.cc, \
>>> +EXTRA_FILES := \
>>> +$(GTEST_FRAMEWORK_SRC)/googletest/src/gtest-all.cc \
>>> +$(GTEST_FRAMEWORK_SRC)/googlemock/src/gmock-all.cc, \
>>>  EXTRA_OBJECT_FILES := $(filter-out %/operator_new$(OBJ_SUFFIX), \
>>>  $(BUILD_LIBJVM_ALL_OBJS)), \
>>>  CFLAGS := $(JVM_CFLAGS) \
>> [2] http://cr.openjdk.java.net/~iignatyev/8222414/webrev.1-2/
>> 
>>> On May 23, 2019, at 6:57 AM, Erik Joelsson  wrote:
>>> 
>>> Hello Igor,
>>> 
>>> Build changes look ok, with a (very) minor style suggestion [1] (point 18). 
>>> We try to avoid padding continuation line breaks, so I would appreciate if 
>>> CompileGtest.gmk:68 could be indented just 4 extra spaces compared to line 
>>> 67. If you want the file names to line up, it's acceptable to break after 
>>> the := on 67.
>>> 
>>> [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
>>> 
>>> /Erik
>>> 
>>> On 2019-05-22 22:20, Igor Ignatyev wrote:
>>>> http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
>>>>> 21638 lines changed: 21628 ins; 0 del; 10 mod;
>>>> Hi all,
>>>> 
>>>> could you please review this patch which makes mocking framework from 
>>>> google test available for hotspot tests?
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8222414
>>>> testing: tier1
>>>> webrevs:
>>>>  - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.00
>>>>> 21605 lines changed: 21605 ins; 0 del; 0 mod;
>>>> * moves gtest from test/fmw/gtest to test/fmw/gtest/googletest;
>>>> * adds only required (w/o build-aux, docs, make, msvc, scripts, tests 
>>>> directories and make related files) source of gmock to 
>>>> test/fmw/gtest/googlemock;
>>>> 
>>>> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.0-1
>>>>> 33 lines changed: 23 ins; 0 del; 10 mod;
>>>> * makes necessary changes in makefiles
>>>> * calls gmock framework init function
>>>> * works around the conflict b/w :testing::internal::Log function defined 
>>>> by gmock and Log macro defined by hotspot
>>>> 
>>>> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
>>>>> 21638 lines changed: 21628 ins; 0 del; 10 mod;
>>>> all the changes together
>>>> 
>>>> 
>>>> thanks,
>>>> -- Igor

Re: RFR(L/XS) : 8222414 : bring googlemock v1.8.1

[Igor Ignatyev]

thanks Robin! pushed.
-- Igor

> On May 27, 2019, at 12:54 AM, Robin Westberg  
> wrote:
> 
> Hi Igor,
> 
> Looks good to me, I tried rewriting parts of an existing test to use gmock 
> instead of a handcrafted mock, and things worked as expected!
> 
> Best regards,
> Robin
> 
>> On 24 May 2019, at 23:33, Igor Ignatyev  wrote:
>> 
>> @Erik, Thanks!
>> 
>> @hotspot (looking at Robin), can I get another review? 
>> 
>> -- Igor
>> 
>>> On May 24, 2019, at 8:28 AM, Erik Joelsson  wrote:
>>> 
>>> Thanks, looks good!
>>> 
>>> /Erik
>>> 
>>> On 2019-05-23 19:07, Igor Ignatyev wrote:
>>>> Hi Erik,
>>>> 
>>>> thanks for your review. I've updated CompileGtest.gmk as you advised[1], 
>>>> which gives [2].
>>>> 
>>>> -- Igor
>>>> 
>>>> [1]
>>>>> diff -r 3443e083223d make/hotspot/lib/CompileGtest.gmk
>>>>> --- a/make/hotspot/lib/CompileGtest.gmk Thu May 23 19:03:32 2019 -0700
>>>>> +++ b/make/hotspot/lib/CompileGtest.gmk Thu May 23 19:04:09 2019 -0700
>>>>> @@ -64,8 +64,9 @@
>>>>>EXCLUDES := $(JVM_EXCLUDES), \
>>>>>EXCLUDE_FILES := gtestLauncher.cpp, \
>>>>>EXCLUDE_PATTERNS := $(JVM_EXCLUDE_PATTERNS), \
>>>>> -EXTRA_FILES := $(GTEST_FRAMEWORK_SRC)/googletest/src/gtest-all.cc \
>>>>> -   $(GTEST_FRAMEWORK_SRC)/googlemock/src/gmock-all.cc, \
>>>>> +EXTRA_FILES := \
>>>>> +$(GTEST_FRAMEWORK_SRC)/googletest/src/gtest-all.cc \
>>>>> +$(GTEST_FRAMEWORK_SRC)/googlemock/src/gmock-all.cc, \
>>>>>EXTRA_OBJECT_FILES := $(filter-out %/operator_new$(OBJ_SUFFIX), \
>>>>>$(BUILD_LIBJVM_ALL_OBJS)), \
>>>>>CFLAGS := $(JVM_CFLAGS) \
>>>> [2] http://cr.openjdk.java.net/~iignatyev/8222414/webrev.1-2/
>>>> 
>>>>> On May 23, 2019, at 6:57 AM, Erik Joelsson  
>>>>> wrote:
>>>>> 
>>>>> Hello Igor,
>>>>> 
>>>>> Build changes look ok, with a (very) minor style suggestion [1] (point 
>>>>> 18). We try to avoid padding continuation line breaks, so I would 
>>>>> appreciate if CompileGtest.gmk:68 could be indented just 4 extra spaces 
>>>>> compared to line 67. If you want the file names to line up, it's 
>>>>> acceptable to break after the := on 67.
>>>>> 
>>>>> [1] http://openjdk.java.net/groups/build/doc/code-conventions.html
>>>>> 
>>>>> /Erik
>>>>> 
>>>>> On 2019-05-22 22:20, Igor Ignatyev wrote:
>>>>>> http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
>>>>>>> 21638 lines changed: 21628 ins; 0 del; 10 mod;
>>>>>> Hi all,
>>>>>> 
>>>>>> could you please review this patch which makes mocking framework from 
>>>>>> google test available for hotspot tests?
>>>>>> 
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8222414
>>>>>> testing: tier1
>>>>>> webrevs:
>>>>>> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.00
>>>>>>> 21605 lines changed: 21605 ins; 0 del; 0 mod;
>>>>>> * moves gtest from test/fmw/gtest to test/fmw/gtest/googletest;
>>>>>> * adds only required (w/o build-aux, docs, make, msvc, scripts, tests 
>>>>>> directories and make related files) source of gmock to 
>>>>>> test/fmw/gtest/googlemock;
>>>>>> 
>>>>>> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.0-1
>>>>>>> 33 lines changed: 23 ins; 0 del; 10 mod;
>>>>>> * makes necessary changes in makefiles
>>>>>> * calls gmock framework init function
>>>>>> * works around the conflict b/w :testing::internal::Log function defined 
>>>>>> by gmock and Log macro defined by hotspot
>>>>>> 
>>>>>> - http://cr.openjdk.java.net/~iignatyev//8222414/webrev.01
>>>>>>> 21638 lines changed: 21628 ins; 0 del; 10 mod;
>>>>>> all the changes together
>>>>>> 
>>>>>> 
>>>>>> thanks,
>>>>>> -- Igor
>> 
> 

RFR(S) [13] : 8226910 : make it possible to use jtreg's -match via run-test framework

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html
> 25 lines changed: 18 ins; 3 del; 4 mod;

Hi all,

could you please review this small patch which adds JTREG_RUN_PROBLEM_LISTS 
options to run-test framework? when JTREG_RUN_PROBLEM_LISTS is set to true, 
jtreg will use problem lists as values of -match: instead of -exclude, which 
effectively means it will run only problem listed tests.

doc/building.html got changed when I ran update-build-docs, I can exclude it 
from the patch, but it seems it will keep changing every time we run 
update-build-docs, so I decided to at least bring it up.

JBS: https://bugs.openjdk.java.net/browse/JDK-8226910
webrev: http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html

Thanks,
-- Igor

Re: RFR(S) [13] : 8226910 : make it possible to use jtreg's -match via run-test framework

[Igor Ignatyev]

ping?

-- Igor

> On Jun 27, 2019, at 3:25 PM, Igor Ignatyev  wrote:
> 
> http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html
>> 25 lines changed: 18 ins; 3 del; 4 mod;
> 
> Hi all,
> 
> could you please review this small patch which adds JTREG_RUN_PROBLEM_LISTS 
> options to run-test framework? when JTREG_RUN_PROBLEM_LISTS is set to true, 
> jtreg will use problem lists as values of -match: instead of -exclude, which 
> effectively means it will run only problem listed tests.
> 
> doc/building.html got changed when I ran update-build-docs, I can exclude it 
> from the patch, but it seems it will keep changing every time we run 
> update-build-docs, so I decided to at least bring it up.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8226910
> webrev: http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html
> 
> Thanks,
> -- Igor

Re: RFR(S) [13] : 8226910 : make it possible to use jtreg's -match via run-test framework

[Igor Ignatyev]

Hi David,

> On Jul 6, 2019, at 1:58 AM, David Holmes  wrote:
> 
> Hi Igor,
> 
> On 6/07/2019 1:09 pm, Igor Ignatyev wrote:
>> ping?
>> -- Igor
>>> On Jun 27, 2019, at 3:25 PM, Igor Ignatyev  wrote:
>>> 
>>> http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html
>>>> 25 lines changed: 18 ins; 3 del; 4 mod;
>>> 
>>> Hi all,
>>> 
>>> could you please review this small patch which adds JTREG_RUN_PROBLEM_LISTS 
>>> options to run-test framework? when JTREG_RUN_PROBLEM_LISTS is set to true, 
>>> jtreg will use problem lists as values of -match: instead of -exclude, 
>>> which effectively means it will run only problem listed tests.
> 
> doc/testing.md
> 
> + Set to `true` of `false`.
> 
> typo: s/of/or/
fixed .md, regenerated .html.
> 
> Build changes seem okay - I can't attest to the operation of the flag.

here is how I verified that it does that it supposed to:

$ make test "JTREG=OPTIONS=-l;RUN_PROBLEM_LISTS=true" 
TEST=open/test/hotspot/jtreg/:hotspot_all
lists 53 tests, the same command w/o RUN_PROBLEM_LISTS (or w/ 
RUN_PROBLEM_LISTS=false) lists 6698 tests.

$ make test 
"JTREG=OPTIONS=-l;RUN_PROBLEM_LISTS=true;EXTRA_PROBLEM_LISTS=ProblemList-aot.txt
lists 81 tests, the same command w/o RUN_PROBLEM_LISTS lists 6670 tests.

> 
>>> doc/building.html got changed when I ran update-build-docs, I can exclude 
>>> it from the patch, but it seems it will keep changing every time we run 
>>> update-build-docs, so I decided to at least bring it up.
> 
> Weird it seems to have removed line-breaks in that paragraph. What platform 
> did you build on?
I built on macos. now when I wrote that, I remember pandoc used to produce 
different results on macos. so I've rerun it on linux on the source w/o my 
change, and doc/building.html still got changed in the exact same way.

> David
> -
> 
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8226910
>>> webrev: http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html
>>> 
>>> Thanks,
>>> -- Igor

Re: RFR(S) [13] : 8226910 : make it possible to use jtreg's -match via run-test framework

[Igor Ignatyev]

can I get a review for this patch? 
http://cr.openjdk.java.net/~iignatyev//8226910/webrev.01/index.html 
<http://cr.openjdk.java.net/~iignatyev//8226910/webrev.01/index.html> 

Thanks,
-- Igor

> On Jul 6, 2019, at 11:50 AM, Igor Ignatyev  wrote:
> 
> Hi David,
> 
>> On Jul 6, 2019, at 1:58 AM, David Holmes  wrote:
>> 
>> Hi Igor,
>> 
>> On 6/07/2019 1:09 pm, Igor Ignatyev wrote:
>>> ping?
>>> -- Igor
>>>> On Jun 27, 2019, at 3:25 PM, Igor Ignatyev  
>>>> wrote:
>>>> 
>>>> http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html
>>>>> 25 lines changed: 18 ins; 3 del; 4 mod;
>>>> 
>>>> Hi all,
>>>> 
>>>> could you please review this small patch which adds 
>>>> JTREG_RUN_PROBLEM_LISTS options to run-test framework? when 
>>>> JTREG_RUN_PROBLEM_LISTS is set to true, jtreg will use problem lists as 
>>>> values of -match: instead of -exclude, which effectively means it will run 
>>>> only problem listed tests.
>> 
>> doc/testing.md
>> 
>> + Set to `true` of `false`.
>> 
>> typo: s/of/or/
> fixed .md, regenerated .html.
>> 
>> Build changes seem okay - I can't attest to the operation of the flag.
> 
> here is how I verified that it does that it supposed to:
> 
> $ make test "JTREG=OPTIONS=-l;RUN_PROBLEM_LISTS=true" 
> TEST=open/test/hotspot/jtreg/:hotspot_all
> lists 53 tests, the same command w/o RUN_PROBLEM_LISTS (or w/ 
> RUN_PROBLEM_LISTS=false) lists 6698 tests.
> 
> $ make test 
> "JTREG=OPTIONS=-l;RUN_PROBLEM_LISTS=true;EXTRA_PROBLEM_LISTS=ProblemList-aot.txt
> lists 81 tests, the same command w/o RUN_PROBLEM_LISTS lists 6670 tests.
> 
>> 
>>>> doc/building.html got changed when I ran update-build-docs, I can exclude 
>>>> it from the patch, but it seems it will keep changing every time we run 
>>>> update-build-docs, so I decided to at least bring it up.
>> 
>> Weird it seems to have removed line-breaks in that paragraph. What platform 
>> did you build on?
> I built on macos. now when I wrote that, I remember pandoc used to produce 
> different results on macos. so I've rerun it on linux on the source w/o my 
> change, and doc/building.html still got changed in the exact same way.
> 
>> David
>> -
>> 
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8226910 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8226910>
>>>> webrev: 
>>>> http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html 
>>>> <http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html>
>>>> 
>>>> Thanks,
>>>> -- Igor

Re: RFR(S) [13] : 8226910 : make it possible to use jtreg's -match via run-test framework

[Igor Ignatyev]

David, Misha,

thanks for your review, pushed.

-- Igor

> On Jul 18, 2019, at 10:14 AM, mikhailo.seledt...@oracle.com wrote:
> 
> +1
> 
> On 7/17/19 9:43 PM, David Holmes wrote:
>> Hi Igor,
>> 
>> This seems fine to me.
>> 
>> Thanks,
>> David
>> 
>> On 17/07/2019 7:35 am, Igor Ignatyev wrote:
>>> can I get a review for this patch? 
>>> http://cr.openjdk.java.net/~iignatyev//8226910/webrev.01/index.html
>>> 
>>> Thanks,
>>> -- Igor
>>> 
>>>> On Jul 6, 2019, at 11:50 AM, Igor Ignatyev >>> <mailto:igor.ignat...@oracle.com>> wrote:
>>>> 
>>>> Hi David,
>>>> 
>>>>> On Jul 6, 2019, at 1:58 AM, David Holmes >>>> <mailto:david.hol...@oracle.com>> wrote:
>>>>> 
>>>>> Hi Igor,
>>>>> 
>>>>> On 6/07/2019 1:09 pm, Igor Ignatyev wrote:
>>>>>> ping?
>>>>>> -- Igor
>>>>>>> On Jun 27, 2019, at 3:25 PM, Igor Ignatyev >>>>>> <mailto:igor.ignat...@oracle.com>> wrote:
>>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html
>>>>>>>> 25 lines changed: 18 ins; 3 del; 4 mod;
>>>>>>> 
>>>>>>> Hi all,
>>>>>>> 
>>>>>>> could you please review this small patch which adds 
>>>>>>> JTREG_RUN_PROBLEM_LISTS options to run-test framework? when 
>>>>>>> JTREG_RUN_PROBLEM_LISTS is set to true, jtreg will use problem lists as 
>>>>>>> values of -match: instead of -exclude, which effectively means it will 
>>>>>>> run only problem listed tests.
>>>>> 
>>>>> doc/testing.md
>>>>> 
>>>>> + Set to `true` of `false`.
>>>>> 
>>>>> typo: s/of/or/
>>>> fixed .md, regenerated .html.
>>>>> 
>>>>> Build changes seem okay - I can't attest to the operation of the flag.
>>>> 
>>>> here is how I verified that it does that it supposed to:
>>>> 
>>>> $ make test "JTREG=OPTIONS=-l;RUN_PROBLEM_LISTS=true" 
>>>> TEST=open/test/hotspot/jtreg/:hotspot_all
>>>> lists 53 tests, the same command w/o RUN_PROBLEM_LISTS (or w/ 
>>>> RUN_PROBLEM_LISTS=false) lists 6698 tests.
>>>> 
>>>> $ make test 
>>>> "JTREG=OPTIONS=-l;RUN_PROBLEM_LISTS=true;EXTRA_PROBLEM_LISTS=ProblemList-aot.txt
>>>> lists 81 tests, the same command w/o RUN_PROBLEM_LISTS lists 6670 tests.
>>>> 
>>>>> 
>>>>>>> doc/building.html got changed when I ran update-build-docs, I can 
>>>>>>> exclude it from the patch, but it seems it will keep changing every 
>>>>>>> time we run update-build-docs, so I decided to at least bring it up.
>>>>> 
>>>>> Weird it seems to have removed line-breaks in that paragraph. What 
>>>>> platform did you build on?
>>>> I built on macos. now when I wrote that, I remember pandoc used to produce 
>>>> different results on macos. so I've rerun it on linux on the source w/o my 
>>>> change, and doc/building.html still got changed in the exact same way.
>>>> 
>>>>> David
>>>>> -
>>>>> 
>>>>>>> 
>>>>>>> JBS:https://bugs.openjdk.java.net/browse/JDK-8226910
>>>>>>> webrev:http://cr.openjdk.java.net/~iignatyev//8226910/webrev.00/index.html
>>>>>>>  
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> -- Igor
>>> 

RFR(S) : 8238943: switch to jtreg 5.0

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8238943/webrev.00
> 10 lines changed: 1 ins; 0 del; 9 mod;

Hi all,

could you please review the patch which changes jtreg version used in jdk/jdk 
to the latest and greatest -- jtreg 5.0? and as (recently became) usually, this 
patch also bumps requiredVersion in all the jtreg test suites in order to 
reduce possible discrepancy in results.

JBS: https://bugs.openjdk.java.net/browse/JDK-8238943
webrev: http://cr.openjdk.java.net/~iignatyev/8238943/webrev.00
testing: tier1-4 

Thanks,
-- Igor
 

Re: RFR: JDK-8242463: ProcessTools.createNativeTestProcessBuilder() in testlib needs jdk/bin on PATH on Windows

[Igor Ignatyev]

Hi Erik,

looks good to me.

-- Igor

> On Apr 9, 2020, at 8:15 AM, Erik Joelsson  wrote:
> 
> The test 
> open/test/hotspot/jtreg/testlibrary_tests/process/TestNativeProcessBuilder.java
>  fails when building the JDK with VS2019. More specifically, it fails if the 
> JDK under test is built with a different version of VS than the boot JDK.
> 
> The cause of this is in how the PATH is setup. ProcessTools adds the 
> jdk/bin/server dir to PATH so that jvm.dll can be loaded, but it omits the 
> jdk/bin dir where the VS runtime dlls are located. If the boot JDK happens to 
> have the same runtime dlls (or if the OS has them installed) those may be on 
> the path and save the situation accidentally, which is what regularly happens 
> in Oracle automated testing today.
> 
> I think the proper fix to this is to add jdk/bin to the PATH on Windows, and 
> also to change the order so that jdk/bin and jdk/bin/server are prepended 
> rather than appended to the PATH. This way we minimize the risk of the 
> environment affecting tests in any way.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8242463
> 
> Webrev: http://cr.openjdk.java.net/~erikj/8242463/webrev.01/index.html
> 
> /Erik
> 

Re: RFR 15 8242462: Residual Cleanup of rmic removal

[Igor Ignatyev]

Hi Roger,

removal of applications/ctw/modules/jdk_rmic.java and changes in doc (assuming 
.html was generated from .md) look good to me.

adding hotspot-runtime alias to bring attention of runtime team. I'm not sure 
who are the right people to review bin/unshuffle_list.txt though,.. build team 
(cc'ed them)?

Cheers,
-- Igor

> On Apr 9, 2020, at 6:48 AM, Roger Riggs  wrote:
> 
> Please review a few cleanups I missed on the removal of rmic.
> I didn't get a reply on the changes to the hotspot (cds) tests.
> (I did get an ok on the javadoc changes)
> 
> Webrev:
> http://cr.openjdk.java.net/~rriggs/webrev-remove-rmic-8225319-misc-1/
> 
> Issue:
>   https://bugs.openjdk.java.net/browse/JDK-8242462
> 
> Thanks, Roger
> 

RFR(S) : 8240904 : Screen flashes on test failures when running tests from make

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev//8240904/webrev.00
> 35 lines changed: 26 ins; 0 del; 10 mod


Hi all,

8233827[1] which added screenshots to so-called failure handler had an 
unexpected side-effect on linux, where users might observer flashes each time a 
screenshot is taken, which, to put it mildly, is annoying. the patch replaces 
gnome-screenshot app w/ calling java.awt API to make a screenshot. the patch 
also uses the same solution to make screenshots on windows and solaris (which 
previously didn't save screenshots on failures). we still use native app on mac 
as using java.awt API requires accessibility permissions, which might be as 
annoying as flashes on linux.

[1] https://bugs.openjdk.java.net/browse/JDK-8233827

JBS: https://bugs.openjdk.java.net/browse/JDK-8240904
webrev: http://cr.openjdk.java.net/~iignatyev//8240904/webrev.00
testing: verified that screenshots are successfully generated on headful 
systems, and headless systems ignore errors ('java.awt.AWTException: headless 
environment' from j.a.Robot::).

Thanks,
-- Igor

Re: RFR(S) : 8240904 : Screen flashes on test failures when running tests from make

[Igor Ignatyev]

Hi Erik,

sure, I've actually replaced one long statement w/ multiple shorter ones, which 
made the comment section redundant -- 
http://cr.openjdk.java.net/~iignatyev//8240904/webrev.01 
<http://cr.openjdk.java.net/~iignatyev//8240904/webrev.01>

Thanks,
-- Igor

> On Apr 16, 2020, at 6:17 AM, Erik Joelsson  wrote:
> 
> Looks ok to me. Would it be possible to break up the long lines a bit to 
> improve readability? Backslash escape for newlines should work in properties 
> files.
> 
> /Erik
> 
> On 2020-04-15 22:22, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8240904/webrev.00
>>> 35 lines changed: 26 ins; 0 del; 10 mod
>> 
>> Hi all,
>> 
>> 8233827[1] which added screenshots to so-called failure handler had an 
>> unexpected side-effect on linux, where users might observer flashes each 
>> time a screenshot is taken, which, to put it mildly, is annoying. the patch 
>> replaces gnome-screenshot app w/ calling java.awt API to make a screenshot. 
>> the patch also uses the same solution to make screenshots on windows and 
>> solaris (which previously didn't save screenshots on failures). we still use 
>> native app on mac as using java.awt API requires accessibility permissions, 
>> which might be as annoying as flashes on linux.
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8233827
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8240904
>> webrev: http://cr.openjdk.java.net/~iignatyev//8240904/webrev.00
>> testing: verified that screenshots are successfully generated on headful 
>> systems, and headless systems ignore errors ('java.awt.AWTException: 
>> headless environment' from j.a.Robot::).
>> 
>> Thanks,
>> -- Igor
>> 
>> 

Re: RFR(S) : 8240904 : Screen flashes on test failures when running tests from make

[Igor Ignatyev]

Thanks for your review, Sergey.

-- Igor

> On Apr 16, 2020, at 1:43 AM, Sergey Bylokhov  
> wrote:
> 
> Looks fine.
> 
> On 4/15/20 10:22 pm, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev//8240904/webrev.00
>>> 35 lines changed: 26 ins; 0 del; 10 mod
>> Hi all,
>> 8233827[1] which added screenshots to so-called failure handler had an 
>> unexpected side-effect on linux, where users might observer flashes each 
>> time a screenshot is taken, which, to put it mildly, is annoying. the patch 
>> replaces gnome-screenshot app w/ calling java.awt API to make a screenshot. 
>> the patch also uses the same solution to make screenshots on windows and 
>> solaris (which previously didn't save screenshots on failures). we still use 
>> native app on mac as using java.awt API requires accessibility permissions, 
>> which might be as annoying as flashes on linux.
>> [1] https://bugs.openjdk.java.net/browse/JDK-8233827
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8240904
>> webrev: http://cr.openjdk.java.net/~iignatyev//8240904/webrev.00
>> testing: verified that screenshots are successfully generated on headful 
>> systems, and headless systems ignore errors ('java.awt.AWTException: 
>> headless environment' from j.a.Robot::).
>> Thanks,
>> -- Igor
> 
> 
> -- 
> Best regards, Sergey.

Re: mistriggered "error: warnings found and -Werror specified" for java warnings

[Igor Ignatyev]

> On Apr 23, 2020, at 6:12 AM, Erik Joelsson  wrote:
> 
> Hello Matthias,
> 
> On 2020-04-23 05:51, Matthias Klose wrote:
>> jdk-15+20 fails to build with
>> 
>> * For target 
>> support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:
>> /packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
>> warning: [deprecation] finishedTesting() in Observer has been deprecated
>> public void finishedTesting() {
>> ^
>> error: warnings found and -Werror specified
>> 1 error
>> 1 warning
> That's strange. I assume this tool is built with the boot JDK, so that makes 
> me wonder what boot JDK you are using as we have not seen this warning/error?
I guess version of jtreg/jt-harness is more relevant here as deprecated 
finishedTesting is from com.sun.javatest.Harness.Observer.

--Igor

>> 
>> Apparently --disable-warnings-as-errors only has an effect on C/C++ files,
>> however the build diagnostics trigger on java warnings as well, and 
>> apparently
>> -Werror is hard-coded in various places for java options. Should the
>> documentation for this configure option be clarified, or should it trigger 
>> for
>> java warnings as well?
> 
> Correct. The reasoning is that OpenJDK is built on a wide variety of 
> environments with different compiler versions, so keeping the build warning 
> free on all of them isn't feasible. This option makes it possible to build 
> with all those different compiler versions while still maintaining a warning 
> free source for a core set of compiler versions. In contrast, the Java code 
> should only be compiled with a very small set of javac versions, which should 
> be easily controlled. The majority of the code is even compiled with the 
> Javac we are building. We have contemplated a similar option for Java code, 
> but concluded that it doesn't serve any purpose. The Java source should just 
> always be warning free.
> 
> /Erik
> 

Re: mistriggered "error: warnings found and -Werror specified" for java warnings

[Igor Ignatyev]

Hi Matthias,

> jtharness 6.0-b10 and jtreg 5.0-b1.

jtreg 5.0-b1 "officially" depends on jt6.0-b08, and the bundle which we use 
internally has jt6.0-b08 within it, and AFAICT jt6.0-b08 hasn't deprecated 
c.s.j.H.Observer::finishedTesting yet. so as a temp. solution to allow 
configurations like yours, we need to suppress deprecation warning in failure 
handler, that's if we decide that such configurations are "supported", which 
isn't that obvious as you might encounter some deviations in jtreg behavior   
b/c another version of its dependency is used, so I'd encourage you to use the 
exact same version as used by jtreg build script[1].

a proper solution would include switching jtreg to newer version of jt-harness 
(which implies adjustment of jtreg and subsequently testing), promotion/tagging 
of newer jtreg build, switching to newer jtreg in jdk and updating in 
failurehandler.

Thanks,
-- Igor 

[1] 
http://hg.openjdk.java.net/code-tools/jtreg/file/fc37a1d7f0ea/make/build-all.sh#l129
 
<http://hg.openjdk.java.net/code-tools/jtreg/file/fc37a1d7f0ea/make/build-all.sh#l129>

> On Apr 23, 2020, at 7:48 AM, Matthias Klose  wrote:
> 
> On 4/23/20 4:05 PM, Magnus Ihse Bursie wrote:
>> 
>>> 23 apr. 2020 kl. 15:50 skrev Igor Ignatyev :
>>> 
>>> 
>>> 
>>>> On Apr 23, 2020, at 6:12 AM, Erik Joelsson  
>>>> wrote:
>>>> 
>>>> Hello Matthias,
>>>> 
>>>>> On 2020-04-23 05:51, Matthias Klose wrote:
>>>>> jdk-15+20 fails to build with
>>>>> 
>>>>> * For target 
>>>>> support_test_failure_handler_classes__the.BUILD_FAILURE_HANDLER_batch:
>>>>> /packages/openjdk/15/openjdk-15-15~20/test/failure_handler/src/share/classes/jdk/test/failurehandler/jtreg/GatherDiagnosticInfoObserver.java:136:
>>>>> warning: [deprecation] finishedTesting() in Observer has been deprecated
>>>>>   public void finishedTesting() {
>>>>>   ^
>>>>> error: warnings found and -Werror specified
>>>>> 1 error
>>>>> 1 warning
>>>> That's strange. I assume this tool is built with the boot JDK, so that 
>>>> makes me wonder what boot JDK you are using as we have not seen this 
>>>> warning/error?
> 
> that's with 14.0.1+7.
> 
>>> I guess version of jtreg/jt-harness is more relevant here as deprecated 
>>> finishedTesting is from com.sun.javatest.Harness.Observer.
>> 
>> Aha, that’s probably the explanation. I recently removed deprecation as a 
>> disabled warning for the failure handler. If it depends on jtreg, and it has 
>> changed deprecation status, that might trigger a compilation warning.
>> 
>> I’m on mobile now and can’t check how this should be resolved. 
>> 
>> If a newer version of jtreg introduced the depreciation, we should fix our 
>> sources. If this is something only present in older sources (?) we might 
>> need to raise the minimum jtreg version. 
> 
> jtharness 6.0-b10 and jtreg 5.0-b1.
> 
>> 
>> /Magnus 
>> 
>>> 
>>> --Igor
>>> 
>>>>> 
>>>>> Apparently --disable-warnings-as-errors only has an effect on C/C++ files,
>>>>> however the build diagnostics trigger on java warnings as well, and 
>>>>> apparently
>>>>> -Werror is hard-coded in various places for java options. Should the
>>>>> documentation for this configure option be clarified, or should it 
>>>>> trigger for
>>>>> java warnings as well?
>>>> 
>>>> Correct. The reasoning is that OpenJDK is built on a wide variety of 
>>>> environments with different compiler versions, so keeping the build 
>>>> warning free on all of them isn't feasible. This option makes it possible 
>>>> to build with all those different compiler versions while still 
>>>> maintaining a warning free source for a core set of compiler versions. In 
>>>> contrast, the Java code should only be compiled with a very small set of 
>>>> javac versions, which should be easily controlled. The majority of the 
>>>> code is even compiled with the Javac we are building. We have contemplated 
>>>> a similar option for Java code, but concluded that it doesn't serve any 
>>>> purpose. The Java source should just always be warning free.
>>>> 
>>>> /Erik

RFR(S) : 8245610 : remove in-tree copy on gtest

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/
> 132 lines changed: 80 ins; 36 del; 16 mod
http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/
> 57482 lines changed: 80 ins; 57385 del; 17 mod;

Hi all,

could you please review this small (if you ignore removal part) patch which 
removes in-tree copy of gtest (test/fmw/gtest) and updates makefiles to use one 
provided thru an added configure option `--with-gtest`? the previously used 
configure option `--enable-hotspot-gtest` gets depricated.

the patch also compiles gtest and gmock source code into a static library so 
they can be compiled w/ all warnings disabled.

from JBS:
> w/ JEP 381 (JDK-8241787 / JDK-8244224) being integrated, all compilers used 
> by OpenJDK became supported by gtest out-of-box, so there is no need to have 
> our copy of gtest framework (including gmock) within OpenJDK source tree. 
> instead, it can be treated like any other external dependencies, and a 
> pointer to the directory w/ gtest code can be passed via configure options. 


JBS: https://bugs.openjdk.java.net/browse/JDK-8245610
webrevs:
 - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/ (meaningful changes)
 - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/ (all 
changes)
testing: 
- gtest tests on {linux,windows,macosx}-x64; 
- tier[1-5] builds which include but not limited to linux-aarch64, linux-arm32, 
linux-x64-zero

Thanks,
-- Igor

Re: RFR(S) : 8245610 : remove in-tree copy on gtest

[Igor Ignatyev]

somehow I forgot to include this although planned:
  this patch does *not* change the version of gtest we use in hotspot, that's 
to say if you want to build/run gtest tests, the source of gtest-1.8.1 should 
be provided to --with-gtest (unless you use jib which would download and pass 
source directory --with-gtest). one can download the source bundle from [1] or 
clone googletest, checkout release-1.8.1[2].
 
[1] https://github.com/google/googletest/releases/tag/release-1.8.1 
<https://github.com/google/googletest/releases/tag/release-1.8.1> 
[2] git clone https://github.com/google/googletest 
<https://github.com/google/googletest>
git checkout release-1.8.1

-- Igor


> On May 22, 2020, at 11:12 AM, Igor Ignatyev  wrote:
> 
> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/
>> 132 lines changed: 80 ins; 36 del; 16 mod
> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/
>> 57482 lines changed: 80 ins; 57385 del; 17 mod;
> 
> Hi all,
> 
> could you please review this small (if you ignore removal part) patch which 
> removes in-tree copy of gtest (test/fmw/gtest) and updates makefiles to use 
> one provided thru an added configure option `--with-gtest`? the previously 
> used configure option `--enable-hotspot-gtest` gets depricated.
> 
> the patch also compiles gtest and gmock source code into a static library so 
> they can be compiled w/ all warnings disabled.
> 
> from JBS:
>> w/ JEP 381 (JDK-8241787 / JDK-8244224) being integrated, all compilers used 
>> by OpenJDK became supported by gtest out-of-box, so there is no need to have 
>> our copy of gtest framework (including gmock) within OpenJDK source tree. 
>> instead, it can be treated like any other external dependencies, and a 
>> pointer to the directory w/ gtest code can be passed via configure options. 
> 
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8245610
> webrevs:
> - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/ (meaningful 
> changes)
> - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/ (all 
> changes)
> testing: 
> - gtest tests on {linux,windows,macosx}-x64; 
> - tier[1-5] builds which include but not limited to linux-aarch64, 
> linux-arm32, linux-x64-zero
> 
> Thanks,
> -- Igor
> 

Re: RFR(S) : 8245610 : remove in-tree copy on gtest

[Igor Ignatyev]

Hi Erik,

thanks for you review. sure, I'll hold up on pushing till Tue's evening, so 
Magnus and other will have a chance to look/comment.

-- Igor

> On May 22, 2020, at 11:48 AM, Erik Joelsson  wrote:
> 
> Hello Igor,
> 
> The changes look good to me, but I think we should give Magnus a chance to 
> look at this too.
> 
> /Erik
> 
> On 2020-05-22 11:12, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/
>>> 132 lines changed: 80 ins; 36 del; 16 mod
>> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/
>>> 57482 lines changed: 80 ins; 57385 del; 17 mod;
>> Hi all,
>> 
>> could you please review this small (if you ignore removal part) patch which 
>> removes in-tree copy of gtest (test/fmw/gtest) and updates makefiles to use 
>> one provided thru an added configure option `--with-gtest`? the previously 
>> used configure option `--enable-hotspot-gtest` gets depricated.
>> 
>> the patch also compiles gtest and gmock source code into a static library so 
>> they can be compiled w/ all warnings disabled.
>> 
>> from JBS:
>>> w/ JEP 381 (JDK-8241787 / JDK-8244224) being integrated, all compilers used 
>>> by OpenJDK became supported by gtest out-of-box, so there is no need to 
>>> have our copy of gtest framework (including gmock) within OpenJDK source 
>>> tree. instead, it can be treated like any other external dependencies, and 
>>> a pointer to the directory w/ gtest code can be passed via configure 
>>> options.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245610
>> webrevs:
>>  - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/ (meaningful 
>> changes)
>>  - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/ (all 
>> changes)
>> testing:
>> - gtest tests on {linux,windows,macosx}-x64;
>> - tier[1-5] builds which include but not limited to linux-aarch64, 
>> linux-arm32, linux-x64-zero
>> 
>> Thanks,
>> -- Igor
>> 

Re: RFR(S) : 8245610 : remove in-tree copy on gtest

[Igor Ignatyev]

Hi Magnus,

> On May 25, 2020, at 7:46 AM, Magnus Ihse Bursie 
>  wrote:
> 
> Looks basically good to me.
thanks for your review!
> 
> We need to document the dependency on gtest, and how to retrieve it. I 
> recommend you add a section next to the JTReg information (under the "## 
> Running Tests" heading) in doc/building.md. I think you should include 
> basically the same information as you did in your follow-up mail, that was 
> informative and clear.
that's a good suggestion, I've added a small paragraph to 'Running Tests' in 
doc/building.md and regenerated corresponding .html file -- 
http://cr.openjdk.java.net/~iignatyev/8245610/webrev.doc/ 
<http://cr.openjdk.java.net/~iignatyev/8245610/webrev.doc/>
please let me know if you think something should be added or reworded.

> 
> I assume the most suitable replacement for developers who are used to add a 
> '--disable-hotspot-gtest' to e.g. a pre-definied jib configuration is to now 
> use '--without-gtest'. This will need to be communicated, perhaps to both 
> hotspot-dev and jdk-dev.
sure, after this gets integrated, I'll let both hotspot-dev and jdk-dev aliases 
know which changes might be required to enable/disable hotspot gtest tests 
compilation. 

Thanks.
-- Igor
 
> 
> /Magnus
> 
> On 2020-05-22 20:12, Igor Ignatyev wrote:
>> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/
>>> 132 lines changed: 80 ins; 36 del; 16 mod
>> http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/
>>> 57482 lines changed: 80 ins; 57385 del; 17 mod;
>> Hi all,
>> 
>> could you please review this small (if you ignore removal part) patch which 
>> removes in-tree copy of gtest (test/fmw/gtest) and updates makefiles to use 
>> one provided thru an added configure option `--with-gtest`? the previously 
>> used configure option `--enable-hotspot-gtest` gets depricated.
>> 
>> the patch also compiles gtest and gmock source code into a static library so 
>> they can be compiled w/ all warnings disabled.
>> 
>> from JBS:
>>> w/ JEP 381 (JDK-8241787 / JDK-8244224) being integrated, all compilers used 
>>> by OpenJDK became supported by gtest out-of-box, so there is no need to 
>>> have our copy of gtest framework (including gmock) within OpenJDK source 
>>> tree. instead, it can be treated like any other external dependencies, and 
>>> a pointer to the directory w/ gtest code can be passed via configure 
>>> options.
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245610
>> webrevs:
>>  - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00/ (meaningful 
>> changes)
>>  - http://cr.openjdk.java.net/~iignatyev/8245610/webrev.00%2bremoval/ (all 
>> changes)
>> testing:
>> - gtest tests on {linux,windows,macosx}-x64;
>> - tier[1-5] builds which include but not limited to linux-aarch64, 
>> linux-arm32, linux-x64-zero
>> 
>> Thanks,
>> -- Igor
>> 
> 

RFR(T) : 8245870 : GTEST_FRAMEWORK_SRC should go thought UTIL_FIXUP_PATH

[Igor Ignatyev]

Hi all,

could you please review this trivial one-liner?

JBS: https://bugs.openjdk.java.net/browse/JDK-8245870
webrev: http://cr.openjdk.java.net/~iignatyev//8245870/webrev.00
testing: build on linux-x64,windows-x64 (in progress)

Thanks,
-- Igor

Re: RFR(T) : 8245870 : GTEST_FRAMEWORK_SRC should go thought UTIL_FIXUP_PATH

[Igor Ignatyev]

thanks Erik, pushed.

-- Igor

> On May 26, 2020, at 2:52 PM, Erik Joelsson  wrote:
> 
> Looks good.
> 
> /Erik
> 
> On 2020-05-26 14:48, Igor Ignatyev wrote:
>> Hi all,
>> 
>> could you please review this trivial one-liner?
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245870
>> webrev: http://cr.openjdk.java.net/~iignatyev//8245870/webrev.00
>> testing: build on linux-x64,windows-x64 (in progress)
>> 
>> Thanks,
>> -- Igor

Re: RFR(S) : 8245610 : remove in-tree copy on gtest

[Igor Ignatyev]

Hi Thomas,

I regret that this patch made you unhappier. I fully agree that all hotspot 
developers are to always build *and* run gtest tests, yet not all people who 
work on OpenJDK project are hotspot developers.

I also believe that all OpenJDK developers are to run tests related to the area 
they are changing. The vast majority of such tests are jtreg tests. And jtreg, 
for better or worse, is a counterexample to your last paragraph -- it's an 
essential part of OpenJDK, but it doesn't come in a form of well established 
library, and none of known to me platforms have jtreg in their package manager, 
so you do have to manually download jtreg and specify its location one way or 
another. I understand that there is a difference b/w gtest and jtreg, as jtreg 
isn't really need at build time. However the proper way to run any of OpenJDK 
tests is by `make test` and for it to work you need to either executed 
configure w/ --with-jtreg or specify JT_HOME on each invocation of `make test`, 
where the latter is a preferred way. From that point of view, gtest and jtreg 
aren't different, you need to download both manually and specify their location 
at configure-time. 

with that being said, I truly understand the inconvenience it causes to you, 
yet given you need to download gtest only once and it's fairly easy to hide 
`with-gtest` behind a script or an alias like configure_openjdk='bash 
./configure --with-gtest=$GTEST_HOME', I hope it won't cause problems for you 
and won't discourage you in anyways.

Thanks,
-- Igor

> On May 28, 2020, at 12:31 AM, Thomas Stüfe  wrote:
> 
> Hi,
> 
> I know the boat has sailed, I missed this one. But I am a bit unhappy about 
> this.
> 
> I always build with gtests; I think it makes no sense to not build with 
> gtest. Even if you do not want to run the gtests (and it makes sense to 
> always do since its a good first-base validity test), hotspot developers 
> should always build them since changes in the hotspot sources may break 
> hotspot gtests. hotspot source and gtests are a unity.
> 
> So if it makes no sense to not build gtest, I should not need a special 
> option to specify gtest location - I'd argue that the default case should not 
> require special options.
> 
> The JBS issue states that "it can be treated like any other external 
> dependencies" but this comparison does not hold - almost all other 
> dependencies come in the form of well established libraries with standard 
> headers and standard installation locations which can be coded as default 
> values. On a recent mainstream platform I do not have to specify any paths to 
> libraries to build OpenJDK. This is now different, I always have to manually 
> download gtests and specify gtest location. This is regrettable.
> 
> Thanks, Thomas
> 
> 
> On Tue, May 26, 2020 at 2:27 PM Magnus Ihse Bursie 
> mailto:magnus.ihse.bur...@oracle.com>> wrote:
> 
> 
> On 2020-05-25 19:53, Igor Ignatyev wrote:
> > Hi Magnus,
> >
> >> On May 25, 2020, at 7:46 AM, Magnus Ihse Bursie 
> >> mailto:magnus.ihse.bur...@oracle.com> 
> >> <mailto:magnus.ihse.bur...@oracle.com 
> >> <mailto:magnus.ihse.bur...@oracle.com>>> wrote:
> >>
> >> Looks basically good to me.
> > thanks for your review!
> >>
> >> We need to document the dependency on gtest, and how to retrieve it. 
> >> I recommend you add a section next to the JTReg information (under 
> >> the "## Running Tests" heading) in doc/building.md. I think you 
> >> should include basically the same information as you did in your 
> >> follow-up mail, that was informative and clear.
> > that's a good suggestion, I've added a small paragraph to 'Running 
> > Tests' in doc/building.md and regenerated corresponding .html file -- 
> > http://cr.openjdk.java.net/~iignatyev/8245610/webrev.doc/ 
> > <http://cr.openjdk.java.net/~iignatyev/8245610/webrev.doc/>
> > please let me know if you think something should be added or reworded.
> Looks great! Thank you.
> 
> /Magnus
> >
> >>
> >> I assume the most suitable replacement for developers who are used to 
> >> add a '--disable-hotspot-gtest' to e.g. a pre-definied jib 
> >> configuration is to now use '--without-gtest'. This will need to be 
> >> communicated, perhaps to both hotspot-dev and jdk-dev.
> > sure, after this gets integrated, I'll let both hotspot-dev and 
> > jdk-dev aliases know which changes might be required to enable/disable 
> > hotspot gtest tests compilation.
> >
> > Thanks.
> > -- Igor
> >
> >>
> >> /Magn

RFR(S) : 8246387 : switch to jtreg 5.1

[Igor Ignatyev]

http://cr.openjdk.java.net/~iignatyev/8246387/webrev.00/
> 110 lines changed: 6 ins; 89 del; 15 mod;

Hi all,

could you please review the patch which switches jtreg used in jdk/jdk to the 
newly promoted jtreg5.1? as before, the patch changes 'requiredVersion' 
property in all jtreg test suites to have more consistent results.

besides changing the version in jib-profiles and requiredVersion in all 
TEST.ROOT, the patch also 
 - removes no-op methods from GatherDiagnosticInfoObserver. in jtharness 
6.0-b11, and Harness.Observer::finishedTesting() method got deprecated, but 
instead of suppressing the warning, we can just remove it and other empty 
methods as Harness.Observer interface now provides no-op implementations for 
all its methods; 
 - removes j.t.lib.Utils::getTestName method. this method has a (now) wrong 
assumption that testng classes are available only if we are executing a testng 
test. the method was used only by appcds tests to get a test specific filename, 
and these tests were updated to use a new supported way to get a testname via 
`test.name` system property; 
 - updates test/hotspot/jtreg/runtime/condy/staticInit/TestInitException.java 
test to workaround CODETOOLS-7902686[1] . jasm from asmtools 7.0-b08 
incorrectly sets full file path on windows as SourceFile, so the test was 
temporarily updated to use regexp which matches both full path and just 
basename.
 

webrev: http://cr.openjdk.java.net/~iignatyev/8246387/webrev.00/
testing: tier1-4
JBS: https://bugs.openjdk.java.net/browse/JDK-8246387

[1] https://bugs.openjdk.java.net/browse/CODETOOLS-7902686
(fixed in asmtools7.0-b09, yet jtreg5.1 uses asmtools7.0-b08, so effectively 
can be fixed only w/ switch to a next version of jtreg) 

Thanks,
-- Igor