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

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: 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

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: [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

[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

[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

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

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 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: 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

[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: 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: [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

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: 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

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

RFR: 8256430: add linux-x64-optimized to tier1

[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

-

Commit messages:
 - 8256430: add linux-x64-optimized to tier1

Changes: https://git.openjdk.java.net/jdk/pull/1244/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1244&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256430
  Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 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: 8256414: add optimized build to submit workflow [v2]

[igor . ignatyev]

Hi Thomas,

There is indeed was a discussion about the future of optimized flavor, and the 
consensus was to remove it. We, however, haven’t done that yet (8183287[*] is 
filed for that work). meanwhile, people still use optimized builds from time to 
time so we need to ensure they are at least buildable.

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

— Igor

> On Nov 16, 2020, at 12:43 PM, Thomas Stüfe  wrote:
> 
> 
> Hi,
> 
> just curious, was the optimized build not earmarked for removal? I dimly 
> remember reading discussions about this.
> 
> Thanks, Thomas
> 
> On Mon, Nov 16, 2020 at 9:35 PM Daniel D.Daugherty  <mailto:dcu...@openjdk.java.net>> wrote:
> On Mon, 16 Nov 2020 19:29:59 GMT, Igor Ignatyev  <mailto:iignat...@openjdk.org>> wrote:
> 
> >> Marked as reviewed by shade (Reviewer).
> >
> > Thanks for the reviews, folks. the build looks green, integrating.
> 
> @iignatev - did you also change Mach5? I don't have workflow builds enabled
> by default since I typically do Mach5 builds...
> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/1233 
> <https://git.openjdk.java.net/jdk/pull/1233>

Re: RFR: 8256414: add optimized build to submit workflow [v2]

[Igor Ignatyev]

On Mon, 16 Nov 2020 19:29:59 GMT, Igor Ignatyev  wrote:

>> Marked as reviewed by shade (Reviewer).
>
> Thanks for the reviews, folks. the build looks green, integrating.

> @iignatev - did you also change Mach5? I don't have workflow builds enabled
> by default since I typically do Mach5 builds...

Hi Dan, no, not yet. I’m going to change jib profile and tier definitions by a 
separate RFE.

-

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

Re: RFR: 8256414: add optimized build to submit workflow [v2]

[Igor Ignatyev]

On Mon, 16 Nov 2020 18:58:37 GMT, Aleksey Shipilev  wrote:

>> Igor Ignatyev has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   added --disable-precompiled-headers
>
> Marked as reviewed by shade (Reviewer).

Thanks for the reviews, folks. the build looks green, integrating.

-

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

Integrated: 8256414: add optimized build to submit workflow

[Igor Ignatyev]

On Mon, 16 Nov 2020 18:26:18 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> Could you please review this small and trivial patch which adds 
> `linux-x64-optimized` build to submit workflow so breakages of this build 
> flavor would be easier to spot?
> 
> Thanks,
> -- Igor

This pull request has now been integrated.

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

8256414: add optimized build to submit workflow

add linux-x64-optimized to submit workflow

Reviewed-by: vlivanov, shade, kvn

-

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

Re: RFR: 8256414: add optimized build to submit workflow [v2]

[Igor Ignatyev]

On Mon, 16 Nov 2020 18:40:27 GMT, Aleksey Shipilev  wrote:

> Looks fine, but wouldn't you like to add `--disable-precompiled-headers` as 
> well?

sure, make sense.

-

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

Re: RFR: 8256414: add optimized build to submit workflow [v2]

[Igor Ignatyev]

> Hi all,
> 
> Could you please review this small and trivial patch which adds 
> `linux-x64-optimized` build to submit workflow so breakages of this build 
> flavor would be easier to spot?
> 
> Thanks,
> -- Igor

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

  added --disable-precompiled-headers

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1233/files
  - new: https://git.openjdk.java.net/jdk/pull/1233/files/bad0582f..f8189b0a

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

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

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

RFR: 8256414: add optimized build to submit workflow

[Igor Ignatyev]

Hi all,

Could you please review this small and trivial patch which adds 
`linux-x64-optimized` build to submit workflow so breakages of this build 
flavor would be easier to spot?

Thanks,
-- Igor

-

Commit messages:
 - add linux-x64-optimized build

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

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

Re: RFR: 8255616: Disable AOT and Graal in Oracle OpenJDK

[Igor Ignatyev]

On Fri, 30 Oct 2020 17:40:51 GMT, Vladimir Kozlov  wrote:

> We shipped Ahead-of-Time compilation (the jaotc tool) in JDK 9, as an 
> experimental feature. We shipped Graal as an experimental JIT compiler in JDK 
> 10. We haven't seen much use of these features, and the effort required to 
> support and enhance them is significant. We therefore intend to disable these 
> features in Oracle builds as of JDK 16. 
> 
> We'll leave the sources for these features in the repository, in case any one 
> else is interested in building them. But we will not update or test them.
> 
> We'll continue to build and ship JVMCI as an experimental feature in Oracle 
> builds.
> 
> Tested changes in all tiers.
> 
> I verified that with these changes I still able to build Graal in open repo 
> and run graalunit testing: 
> 
> `open$ bash test/hotspot/jtreg/compiler/graalunit/downloadLibs.sh 
> /mydir/graalunit_lib/`
> `open$ bash configure --with-debug-level=fastdebug 
> --with-graalunit-lib=/mydir/graalunit_lib/ --with-jtreg=/mydir/jtreg`
> `open$ make jdk-image`
> `open$ make test-image`
> `open$ make run-test TEST=compiler/graalunit/HotspotTest.java`

LGTM

-

Marked as reviewed by iignatyev (Reviewer).

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

Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]

[Igor Ignatyev]

On Thu, 8 Oct 2020 11:00:41 GMT, Aleksei Voitylov  wrote:

> @iignatev I resolved the conflict in whitebox.cpp and fixed a minor style nit 
> on the way. Could you take a look?

LGTM

-

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

Integrated: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests

[Igor Ignatyev]

On Tue, 6 Oct 2020 23:08:40 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review this small cleanup which replaces
> `ManagementFactory.getRuntimeMXBean().getName().split("@")[0]` w/ 
> `ProcessHandle.current().pid()` to get current
> process pid?  Thanks,
> -- Igor

This pull request has now been integrated.

Changeset: 5a9bd41e
Author:Igor Ignatyev 
URL:   https://git.openjdk.java.net/jdk/commit/5a9bd41e
Stats: 57 lines in 8 files changed: 0 ins; 41 del; 16 mod

8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean 
to get pid in tests

Reviewed-by: rriggs, shade

-

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

Re: RFR: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests [v2]

[Igor Ignatyev]

On Wed, 7 Oct 2020 18:38:07 GMT, Roger Riggs  wrote:

>> Igor Ignatyev has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - use Long.toString instead of String.valueOf
>>  - remove explicit coversion to String in Suicide.java
>
> Marked as reviewed by rriggs (Reviewer).

Roger, Aleksey, thanks for your reviews.

-

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

Re: RFR: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests [v2]

[Igor Ignatyev]

On Wed, 7 Oct 2020 06:30:43 GMT, Aleksey Shipilev  wrote:

>> test/failure_handler/test/sanity/Suicide.java line 36:
>> 
>>> 34: String osName = System.getProperty("os.name");
>>> 35: if (osName.contains("Windows")) {
>>> 36: cmd = "taskkill.exe /F /PID " + pidStr;
>> 
>> This can be simplified to ProcessHandle.current().toString().  It returns 
>> the pid of the process as a string.
>> 
>> Explicitly converting it to a string is not necessary.  The "+" 
>> concatenation would convert the number to a string.
>
> Yes, can just have `long pid` in this case.
> 
> I don't see that `ProcessHandle.toString` is *specified* to return the string 
> with pid, so relying on that is brittle.
> We might call `Long.toString` here directly, to avoid jumping through a few 
> calls. But seeing how all this is a test
> code, that does not seem necessary.

@RogerRiggs , as Aleksey pointed out `ProcessHandle::toString` isn't specified 
to return the string which contains only
`pid` (in fact it's not specified at all), so I don't think we should rely on 
the current implementation.

although it doesn't matter much, I've removed explicit conversion here and 
replaced `String::valueOf` with
`Long::toString` in other places.

-

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

Re: RFR: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests [v2]

[Igor Ignatyev]

> Hi all,
> 
> could you please review this small cleanup which replaces
> `ManagementFactory.getRuntimeMXBean().getName().split("@")[0]` w/ 
> `ProcessHandle.current().pid()` to get current
> process pid?  Thanks,
> -- Igor

Igor Ignatyev has updated the pull request incrementally with two additional 
commits since the last revision:

 - use Long.toString instead of String.valueOf
 - remove explicit coversion to String in Suicide.java

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/534/files
  - new: https://git.openjdk.java.net/jdk/pull/534/files/b6f4b94a..bc6e3515

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

  Stats: 8 lines in 6 files changed: 0 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/534.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/534/head:pull/534

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

RFR: 8254102: use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get pid in tests

[Igor Ignatyev]

Hi all,

could you please review this small cleanup which replaces
`ManagementFactory.getRuntimeMXBean().getName().split("@")[0]` w/ 
`ProcessHandle.current().pid()` to get current
process pid?

Thanks,
-- Igor

-

Commit messages:
 - update copyright
 - use ProcessHandle::pid instead of ManagementFactory::getRuntimeMXBean to get 
pid

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

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

Integrated: 8253207: enable problemlists jcheck's check

[Igor Ignatyev]

On Wed, 16 Sep 2020 00:15:54 GMT, Igor Ignatyev  wrote:

> problemlists[[1]] check verifies that there are no problem-list entries w/ 
> the bug-id used in the commit message.
> 
> [1]: https://github.com/openjdk/skara/pull/518

This pull request has now been integrated.

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

8253207: enable problemlists jcheck's check

Reviewed-by: erikj

-

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

Re: RFR: 8253207: enable problemlists jcheck's check

[Igor Ignatyev]

On Wed, 16 Sep 2020 13:08:17 GMT, Erik Joelsson  wrote:

>> problemlists[[1]] check verifies that there are no problem-list entries w/ 
>> the bug-id used in the commit message.
>> 
>> [1]: https://github.com/openjdk/skara/pull/518
>
> Marked as reviewed by erikj (Reviewer).

thanks for your review, Erik.

-

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

RFR: 8253207: enable problemlists jcheck's check

[Igor Ignatyev]

problemlists[[1]] check verifies that there are no problem-list entries w/ the 
bug-id used in the commit message.

[1]: https://github.com/openjdk/skara/pull/518

-

Commit messages:
 - 8253207: enable problemlists jcheck's check

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

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

Re: [15] RFR(T) : 8250688 : missed open parenthesis for GTEST_FRAMEWORK_SRC var in Main.gmk

[Igor Ignatyev]

thanks Erik, pushed to jdk15.

-- Igor

> On Jul 28, 2020, at 8:15 AM, Erik Joelsson  wrote:
> 
> Looks good.
> 
> /Erik
> 
> On 2020-07-28 07:51, Igor Ignatyev wrote:
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>> Hi all,
>> 
>> could you please review the one-liner which fixes the typo introduced by 
>> 8245610?
>> 
>> patch:
>>> diff -r 31a8f79a7dfe make/Main.gmk
>>> --- a/make/Main.gmk Tue Jul 28 10:32:57 2020 -0400
>>> +++ b/make/Main.gmk Tue Jul 28 07:50:42 2020 -0700
>>> @@ -664,7 +664,7 @@
>>>  DEPS := build-test-hotspot-jtreg-graal, \
>>>  ))
>>>  -ifneq ($GTEST_FRAMEWORK_SRC), )
>>> +ifneq ($(GTEST_FRAMEWORK_SRC), )
>>>$(eval $(call SetupTarget, test-image-hotspot-gtest, \
>>>MAKEFILE := hotspot/test/GtestImage, \
>>>DEPS := hotspot, \
>>> 
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8250688
>> 8245610: https://bugs.openjdk.java.net/browse/JDK-8245610
>> 
>> Thanks,
>> -- Igor

[15] RFR(T) : 8250688 : missed open parenthesis for GTEST_FRAMEWORK_SRC var in Main.gmk

[Igor Ignatyev]

> 1 files changed, 1 insertions(+), 1 deletions(-)

Hi all,

could you please review the one-liner which fixes the typo introduced by 
8245610?

patch:
> diff -r 31a8f79a7dfe make/Main.gmk
> --- a/make/Main.gmk   Tue Jul 28 10:32:57 2020 -0400
> +++ b/make/Main.gmk   Tue Jul 28 07:50:42 2020 -0700
> @@ -664,7 +664,7 @@
>  DEPS := build-test-hotspot-jtreg-graal, \
>  ))
>  
> -ifneq ($GTEST_FRAMEWORK_SRC), )
> +ifneq ($(GTEST_FRAMEWORK_SRC), )
>$(eval $(call SetupTarget, test-image-hotspot-gtest, \
>MAKEFILE := hotspot/test/GtestImage, \
>DEPS := hotspot, \
> 


JBS: https://bugs.openjdk.java.net/browse/JDK-8250688
8245610: https://bugs.openjdk.java.net/browse/JDK-8245610

Thanks,
-- Igor

Re: RFR: JDK-8247573 gtest/GTestWrapper.java is not helpful if gtest framework is missing

[igor . ignatyev]

LGTM

— Igor

> On Jun 23, 2020, at 11:13 AM, Erik Joelsson  wrote:
> 
> That looks much better! :)
> 
> /Erik
> 
>> On 2020-06-23 09:53, Magnus Ihse Bursie wrote:
>>> On 2020-06-23 17:05, Erik Joelsson wrote:
>>> Looks good, but that was the worst way of posting a patch I've seen to date.
>> The mail you quoted looked awful, yes! :-( I tried a new way of formatting 
>> the mail so the patch and log should be fixed space. Apparently it failed 
>> horribly.
>> 
>> Also, I realized that since the fix is technically in hotspot code, I should 
>> have a hotspot review on this as well. cc:ing them.
>> 
>> Here is the original mail again, sans the formatting:
>> ---
>> 
>> If you run make test TEST=jtreg:gtest/GTestWrapper.java but had not built 
>> using the gtest framework, you would just get a result like this:
>> 
>> STDERR:
>> java.lang.Error: TESTBUG: the library has not been found in 
>> /home/shade/trunks/jdk-jdk/build/linux-x86_64-server-fastdebug/images/test/hotspot/jtreg/native
>> at GTestWrapper.main(GTestWrapper.java:60)
>> at 
>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
>> Method)
>> at 
>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
>> at 
>> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>> at java.base/java.lang.reflect.Method.invoke(Method.java:564)
>> at 
>> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
>> at java.base/java.lang.Thread.run(Thread.java:832)
>> 
>> JavaTest Message: Test threw exception: java.lang.Error
>> JavaTest Message: shutting down test
>> 
>> This is not very helpful in analyzing the problem. I have added a helpful 
>> suggestion to the exception.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8247573
>> Patch inline:
>> 
>> diff --git a/test/hotspot/jtreg/gtest/GTestWrapper.java 
>> b/test/hotspot/jtreg/gtest/GTestWrapper.java
>> --- a/test/hotspot/jtreg/gtest/GTestWrapper.java
>> +++ b/test/hotspot/jtreg/gtest/GTestWrapper.java
>> @@ -1,5 +1,5 @@
>>  /*
>> - * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights 
>> reserved.
>> + * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights 
>> reserved.
>>   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>   *
>>   * This code is free software; you can redistribute it and/or modify it
>> @@ -57,7 +57,7 @@
>>   .resolve(jvmVariantDir);
>>  }
>>  if (!path.toFile().exists()) {
>> -throw new Error("TESTBUG: the library has not been found in " + 
>> nativePath);
>> +throw new Error("TESTBUG: the library has not been found in " + 
>> nativePath + ". Did you forget to use --with-gtest to configure?");
>>  }
>> 
>>  Path execPath = path.resolve("gtestLauncher" + 
>> (Platform.isWindows() ? ".exe" : ""));
>> 
>> /Magnus
>>> 
>>> /Erik
>>> 
>>> On 2020-06-23 07:37, Magnus Ihse Bursie wrote:
 If you run |make test TEST=jtreg:gtest/GTestWrapper.java| but had not 
 built using the gtest framework, you would just get a result like this:
 
 |STDERR: java.lang.Error: TESTBUG: the library has not been found in 
 /home/shade/trunks/jdk-jdk/build/linux-x86_64-server-fastdebug/images/test/hotspot/jtreg/native
  at GTestWrapper.main(GTestWrapper.java:60) at 
 java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native 
 Method) at 
 java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
  at 
 java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
  at java.base/java.lang.reflect.Method.invoke(Method.java:564) at 
 com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
  at java.base/java.lang.Thread.run(Thread.java:832) JavaTest Message: Test 
 threw exception: java.lang.Error JavaTest Message: shutting down test |
 
 This is not very helpful in analyzing the problem. I have added a helpful 
 suggestion to the exception.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8247573
 Patch inline:
 
 |diff --git a/test/hotspot/jtreg/gtest/GTestWrapper.java 
 b/test/hotspot/jtreg/gtest/GTestWrapper.java --- 
 a/test/hotspot/jtreg/gtest/GTestWrapper.java +++ 
 b/test/hotspot/jtreg/gtest/GTestWrapper.java @@ -1,5 +1,5 @@ /* - * 
 Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights 
 reserved. + * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All 
 rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE 
 HEADER. * * This code is free software; you can redistribute it and/or 
 modify it @@ -57,7 +57,7 @@ .resolve(jvmVariantDir); } if 
 (!path.toFile().exists()) { - throw new Error("TESTBUG: th

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Igor Ignatyev]

Magnus, Erik,

thanks for your reviews, pushed w/ a newline being added at L#654 of 
make/Main.gmk.

Cheers,
-- Igor

> On Jun 16, 2020, at 7:03 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-06-16 15:06, Erik Joelsson wrote:
>> 
>> On 2020-06-15 17:39, Igor Ignatyev wrote:
>>> @David, Erik, Magnus,
>>> 
>>> please find the answers to your comments at the bottom of this email.
>>> 
>>> @all,
>>> 
>>> David's and Erik's comments made me realize that some parts of the original 
>>> patch were stashed away and didn't make it to webrev.00. I'm truly sorry 
>>> for the confusion and inconvenience. I also agree w/ David that it's hard 
>>> to follow/review, and my gaffe just made it worse, therefore I'd like to 
>>> start it over again from a clean sate
>>> 
>>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02 
>>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02>Build changes 
>>> look good to me now, except for a missing newline in Main.gmk. (No need for 
>>> new review.)
>> 
> 
> Ditto.
> 
> /Magnus
>> 
>> /Erik
>> 
>>>> 833 lines changed: 228 ins; 591 del; 14 mod; 
>>> 
>>> could you please review the patch which puts all tests for common 
>>> testlibrary classes (which is ones located in /test/lib) into one location 
>>> under /test/lib-test? please note this intentionally doesn't move all 
>>> "testlibrary" tests, e.g. tests for CTW, hotspot-specific testlibrary, are 
>>> left in /test/hotspot/jtreg/testlibrary_tests/ctw .
>>> 
>>> to simplify review, the patch has been split into several webrevs, with 
>>> each of them accomplishes a more-or-less distinct thing, but is not 
>>> necessary self-contained:
>>> 
>>> 0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites 
>>> to test/lib-test:
>>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/ 
>>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/>
>>> 
>>> 1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and 
>>> "merge" of hotspot's and jdk's OutputAnalyzerTest:
>>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/ 
>>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/>
>>> 
>>> 2. simplification of TestNativeProcessBuilder tests: converts Test class 
>>> used by TestNativeProcessBuilder into a static nested class:
>>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder
>>>  
>>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder>
>>> 
>>> 3. makefiles changes to build native parts of lib-test tests. in past, 
>>> there were no tests w/ native parts in this test suite, 
>>> TestNativeProcessBuilder is the 1st such test; this part also removes now 
>>> unneeded lines from hotspot test suite makefile:
>>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest 
>>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest>
>>> 
>>> 4. clean ups in hotspot test suite: adjusted location of 
>>> SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test 
>>> library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should 
>>> not be moved to /test/lib-test; removed 
>>> TestMutuallyExclusivePlatformPredicates from TEST.groups:
>>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/ 
>>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/>
>>> 
>>> 5. LingeredAppTest fix (which apparently has never been run before):
>>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/ 
>>> <http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/>
>>> 
>>> 6. changes to make test/lib-test a fully supported and working test suite, 
>>> and add in into tier1,  includes adding ProblemList, TEST.groups file, and 
>>> 'randomness' k/w:
>>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/ 
>>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/>
>>> 
>>> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02 
>>> <http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02>
>&g

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Igor Ignatyev]

Hi David,

thanks for your review. re: LingeredAppTest, I agree that the test doesn't look 
very useful, yet I'd remind that the goal of this (and other test in 
/test/lib-test) is to (sanity) test testlibrary in order to easily spot bugs in 
testlibrary in a clear manner so one would not have to investigate failures of 
actual "product" tests and go thru their sometimes convoluted logic just to 
find out that the problem was in LingeredApp. w/ that being said, this test can 
do a better job in testing LingeredApp, so I'll file a JBS ticket against 
hotspo/svc to evaluate/improve/discuss the fate of the test.

Thanks,
-- Igor

> On Jun 16, 2020, at 12:14 AM, David Holmes  wrote:
> 
> Hi Igor,
> 
> On 16/06/2020 10:39 am, Igor Ignatyev wrote:
>> @David, Erik, Magnus,
>> please find the answers to your comments at the bottom of this email.
>> @all,
>> David's and Erik's comments made me realize that some parts of the original 
>> patch were stashed away and didn't make it to webrev.00. I'm truly sorry for 
>> the confusion and inconvenience. I also agree w/ David that it's hard to 
>> follow/review, and my gaffe just made it worse, therefore I'd like to start 
>> it over again from a clean sate
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
>>> 833 lines changed: 228 ins; 591 del; 14 mod; 
>> could you please review the patch which puts all tests for common 
>> testlibrary classes (which is ones located in /test/lib) into one location 
>> under /test/lib-test? please note this intentionally doesn't move all 
>> "testlibrary" tests, e.g. tests for CTW, hotspot-specific testlibrary, are 
>> left in /test/hotspot/jtreg/testlibrary_tests/ctw .
> 
> Ok.
> 
>> to simplify review, the patch has been split into several webrevs, with each 
>> of them accomplishes a more-or-less distinct thing, but is not necessary 
>> self-contained:
> 
> Many thanks for doing this!
> 
>> 0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites to 
>> test/lib-test:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/
> 
> Looks good.
> 
>> 1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and "merge" 
>> of hotspot's and jdk's OutputAnalyzerTest:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/
> 
> Looks good.
> 
>> 2. simplification of TestNativeProcessBuilder tests: converts Test class 
>> used by TestNativeProcessBuilder into a static nested class:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder
> 
> Looks good.
> 
>> 3. makefiles changes to build native parts of lib-test tests. in past, there 
>> were no tests w/ native parts in this test suite, TestNativeProcessBuilder 
>> is the 1st such test; this part also removes now unneeded lines from hotspot 
>> test suite makefile:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest
> 
> Hmm okay. Not sure this needed its own category of build logic but ...
> 
> Aside: Makefiles should not have the classpath exception version of the 
> license header. But they all do for some reason. I'll check if we need to do 
> a separate cleanup of that.
> 
>> 4. clean ups in hotspot test suite: adjusted location of 
>> SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test 
>> library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should 
>> not be moved to /test/lib-test; removed 
>> TestMutuallyExclusivePlatformPredicates from TEST.groups:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/
> 
> Looks good. Took me a while to understand the connection to the test library 
> here.
> 
>> 5. LingeredAppTest fix (which apparently has never been run before):
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/
> 
> Someone from serviceability should evaluate this test. It may not be needed.
> 
>> 6. changes to make test/lib-test a fully supported and working test suite, 
>> and add in into tier1,  includes adding ProblemList, TEST.groups file, and 
>> 'randomness' k/w:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/
> 
> Seems okay.
> 
> Thanks,
> David
> -
> 
>> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
>> testing:
>>  - make test TEST=tier1 locally on macosx
>>  - test/lib-test on  {windows,linux,macosx}-x64
>>  - tier1 job (in pro

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Igor Ignatyev]

@David, Erik, Magnus,

please find the answers to your comments at the bottom of this email.

@all,

David's and Erik's comments made me realize that some parts of the original 
patch were stashed away and didn't make it to webrev.00. I'm truly sorry for 
the confusion and inconvenience. I also agree w/ David that it's hard to 
follow/review, and my gaffe just made it worse, therefore I'd like to start it 
over again from a clean sate

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
> 833 lines changed: 228 ins; 591 del; 14 mod; 

could you please review the patch which puts all tests for common testlibrary 
classes (which is ones located in /test/lib) into one location under 
/test/lib-test? please note this intentionally doesn't move all "testlibrary" 
tests, e.g. tests for CTW, hotspot-specific testlibrary, are left in 
/test/hotspot/jtreg/testlibrary_tests/ctw .

to simplify review, the patch has been split into several webrevs, with each of 
them accomplishes a more-or-less distinct thing, but is not necessary 
self-contained:

0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites to 
test/lib-test:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/

1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and "merge" of 
hotspot's and jdk's OutputAnalyzerTest:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/

2. simplification of TestNativeProcessBuilder tests: converts Test class used 
by TestNativeProcessBuilder into a static nested class:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder

3. makefiles changes to build native parts of lib-test tests. in past, there 
were no tests w/ native parts in this test suite, TestNativeProcessBuilder is 
the 1st such test; this part also removes now unneeded lines from hotspot test 
suite makefile:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest

4. clean ups in hotspot test suite: adjusted location of 
SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test 
library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should not 
be moved to /test/lib-test; removed TestMutuallyExclusivePlatformPredicates 
from TEST.groups:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/

5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/

6. changes to make test/lib-test a fully supported and working test suite, and 
add in into tier1,  includes adding ProblemList, TEST.groups file, and 
'randomness' k/w:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/

webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing: 
 - make test TEST=tier1 locally on macosx
 - test/lib-test on  {windows,linux,macosx}-x64
 - tier1 job (in progress)

Thanks,
-- Igor


> On Jun 14, 2020, at 11:23 PM, David Holmes  wrote:
> <...>

> This doesn't seem to move everything under 
> test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
this is intentionally, ctw test-library is hotspot specific, hence its tests 
are to reside in hotspot test suite (until we decide to move the library to 
/test/lib), the same is true for SimpleClassFileLoadHookTest.
> 
> You did not modify hotspot/jtreg/TEST.groups but it refers to:
> testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \
> Plus it should probably refer to the new native_sanity tests somewhere.
the actual version of the patch removed reference to 
TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups and had 
TestNativeProcessBuilder moved to /test/test-lib, so no updates w.r.t. 
native_sanity are needed

> The newly added copyright notice needs to have the correct initial copyright 
> year based on when the file was first added to the repo.

 /test/lib-test/TEST.ROOT file was created by JDK-8243010 on 2020-04-16, hence 
the added copyright has 2020 as the initial copyright year.

> You used the wrong license header - makefiles don't use the classpath 
> exception.
JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which also have 
classpath exception. quick grep showed that make directory has 794 files which 
has '"Classpath" exception', out of 850 which contain 'GNU General Public 
License version 2' string. And although I agree that makefiles shouldn't have 
the classpath exception, I'd prefer to keep JtregNativeLibTest.gmk in sync w/ 
the its siblings and would leave it up to build team to decide how it's best to 
handle.


> On Jun 15, 2020, at 8:12 AM, Magnus Ihse Bursie 
>  wrote:
> 
> A few comments:
> 
> This seems like code copied from elsewhere:
>   57 # This evaluation is expensive and should only be done if this target was
>   58 # explicitly called.
>   59 ifneq ($(filter build-test-libtest-jtreg-native, $(MAKECMDGOALS)), )
> I don't agree that th

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Igor Ignatyev]

testing revealed that LingeredAppTest.java required some love, incremental 
webrev w/ the fixes for LingeredAppTest -- 
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.0-1

-- Igor

> On Jun 12, 2020, at 8:38 PM, Igor Ignatyev  wrote:
> 
> adding build-dev
> 
>> On Jun 12, 2020, at 8:36 PM, Igor Ignatyev  wrote:
>> 
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.00/
>>> 796 lines changed: 200 ins; 588 del; 8 mod;
>> 
>> Hi all,
>> 
>> could you please review this small patch which puts all tests for 
>> testlibrary classes into one location under /test/lib-test?
>> 
>> besides moving tests from test/jdk/lib/testlibrary and 
>> test/hotspot/jtreg/testlibrary_tests to test/lib-test the patch also
>> - problem lists HexPrinterTest.java on windows due to JDK-8247521
>> - introduces make targets to build native parts for the tests in 
>> test/lib-test (needed b/c one test has a native part)
>> - adds randomness k/w to test/lib-test (as it's used by 
>> RandomGeneratorTest.java)
>> - makes Test class used by TestNativeProcessBuilder a static nested class of 
>> TestNativeProcessBuilder
>> - updates LingeredAppTest to use @build instead of @compile and adds 
>> necessary @library tag
>> - removes AssertsTest.java, OutputAnalyzerTest.java from 
>> test/hotspot/jtreg/testlibrary_tests as they are either identical or lesser 
>> that the same tests from test/jdk/lib/testlibrary/
>> - merges test/hotspot/jtreg/testlibrary_tests/OutputAnalyzerTest.java and  
>> test/jdk/lib/testlibrary/OutputAnalyzerTest.java (effectively adds test 
>> cases for `firstMatch` to the superier copy from test/jdk/lib/testlibrary)
>> 
>> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.00/
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
>> testing: test/lib-test on {windows,linux,macosx}-x64
>> 
>> Thanks,
>> -- Igor
>> 
>> 
> 

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Igor Ignatyev]

adding build-dev

> On Jun 12, 2020, at 8:36 PM, Igor Ignatyev  wrote:
> 
> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.00/
>> 796 lines changed: 200 ins; 588 del; 8 mod;
> 
> Hi all,
> 
> could you please review this small patch which puts all tests for testlibrary 
> classes into one location under /test/lib-test?
> 
> besides moving tests from test/jdk/lib/testlibrary and 
> test/hotspot/jtreg/testlibrary_tests to test/lib-test the patch also
> - problem lists HexPrinterTest.java on windows due to JDK-8247521
> - introduces make targets to build native parts for the tests in 
> test/lib-test (needed b/c one test has a native part)
> - adds randomness k/w to test/lib-test (as it's used by 
> RandomGeneratorTest.java)
> - makes Test class used by TestNativeProcessBuilder a static nested class of 
> TestNativeProcessBuilder
> - updates LingeredAppTest to use @build instead of @compile and adds 
> necessary @library tag
> - removes AssertsTest.java, OutputAnalyzerTest.java from 
> test/hotspot/jtreg/testlibrary_tests as they are either identical or lesser 
> that the same tests from test/jdk/lib/testlibrary/
> - merges test/hotspot/jtreg/testlibrary_tests/OutputAnalyzerTest.java and  
> test/jdk/lib/testlibrary/OutputAnalyzerTest.java (effectively adds test cases 
> for `firstMatch` to the superier copy from test/jdk/lib/testlibrary)
> 
> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.00/
> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
> testing: test/lib-test on {windows,linux,macosx}-x64
> 
> Thanks,
> -- Igor
> 
> 

Re: RFR(S) : 8246387 : switch to jtreg 5.1

[Igor Ignatyev]

Erik, David,

thank you for your reviews, pushed.

Cheers,
-- Igor

> On Jun 9, 2020, at 7:06 PM, David Holmes  wrote:
> 
> Hi Igor,
> 
> All of these changes seem okay to me.
> 
> Thanks,
> David

> On Jun 9, 2020, at 9:10 AM, Erik Joelsson  wrote:
> 
> Looks good to me.
> 
> /Erik

> 
> On 10/06/2020 1:56 am, Igor Ignatyev wrote:
>> 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

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

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

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

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(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
>> 
> 

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]

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
> 

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: 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

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: 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: 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
>> 
>> 

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 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
> 

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
> 

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(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
>>> 

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]

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]

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

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(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
>> 
> 

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]

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(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(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

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(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
> 

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]

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(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(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(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: 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

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]

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: [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(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: 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 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

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: 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

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: 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: 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]

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 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]

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]

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]

> 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

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(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(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: 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

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