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

[Ioi Lam]

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

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

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

-

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

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

[Igor Ignatyev]

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

>> Igor Ignatyev has refreshed the contents of this pull request, and previous 
>> commits have been removed. The incremental views will show differences 
>> compared to the previous content of the PR. The pull request contains one 
>> new commit since the last revision:
>> 
>>   fix compilation error in IncorrectAOTLibraryTest test
>
> I did this and scanned the differences (with the diff file from the webrev) 
> and it looks reasonable to me.
> 
> grep '^[+-]' diff.txt | grep -v Copyright | grep -v '^.[+-]' | less
> 
> It looks like most of the changes are mechanical. There were only a few cases 
> where manual changes were made. I trusted that you have tested those cases 
> individually.
> 
> But I don't understand why this error can happen. It seems like jtreg would 
> allow two test cases to interfere with each other.

Hi Ioi,

thanks for review this, I ran the whole tier1-3 jobs which should provide 
enough coverage. as oracle builds don't have AOT feature enabled, I missed a 
compilation error in `IncorrectAOTLibraryTest` test. the test failed in GitHub 
action and should be fixed by 
[3a3b7a8](https://github.com/openjdk/jdk/pull/2985/commits/3a3b7a846289181b466b3c1eb478a0a571d9468b).

-- Igor

-

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

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

[Igor Ignatyev]

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

Igor Ignatyev has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  fix compilation error in IncorrectAOTLibraryTest test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2985/files
  - new: https://git.openjdk.java.net/jdk/pull/2985/files/ff6d4f91..3a3b7a84

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2985=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2985=01-02

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

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

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

[Igor Ignatyev]

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

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

  fix compilation error in IncorrectAOTLibraryTest test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2985/files
  - new: https://git.openjdk.java.net/jdk/pull/2985/files/6e53ad97..ff6d4f91

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2985=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2985=00-01

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

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

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

[Ioi Lam]

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

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

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

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

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

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

-

Marked as reviewed by iklam (Reviewer).

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v27]

[David Holmes]

On Fri, 12 Mar 2021 16:44:38 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix most of issues in java/foreign/ tests
>   
>   Failures related to va_args are tracked in JDK-8263512.

src/hotspot/share/runtime/safefetch.inline.hpp line 35:

> 33: inline int SafeFetch32(int* adr, int errValue) {
> 34:   assert(StubRoutines::SafeFetch32_stub(), "stub not yet generated");
> 35:   Thread* thread = Thread::current_or_null_safe();

Sorry but this should be MACOS_AARCH64 only. All three lines need to be ifdef'd 
if you are going to include the assertion.

Thanks,
David

-

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

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

[Igor Ignatyev]

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

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

note for reviewers: the big part of the patch is just `sed -i  's/ 
ClassFileInstaller / jdk.test.lib.helpers.ClassFileInstaller /g'`

-

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

RFR: 8263549: 8263412 can cause jtreg testlibrary split

[Igor Ignatyev]

Hi all,

could you please review this dull patch that replaces `ClassFileInstaller` w/ 
`jdk.test.lib.helpers.ClassFileInstaller` in all jtreg test descriptions to 
ensure we won't get split testlibrary, and removes 
`jdk/test/lib/ClassFileInstaller.java` (so it won't be accidentally used).

from JBS:
> after JDK-8263412, we might (again) encounter NCDFE b/c parts of 
> testlibraries aren't on the classpath. this happens when jtreg builds 
> `jdk.test.lib.helpers.ClassFileInstaller` as a part of test-specific code, 
> but `ClassFileInstaller` as part of shared testibrary directory in one test, 
> when in the following test, jtreg sees `ClassFileInstaller` in the shared 
> directory, hence javac won't recompile it/its dependencies, but in runtime 
> `jdk.test.lib.helpers.ClassFileInstaller` is nowhere to be found, hence we 
> get NCDFE. 

testing:
- [x]  `grep ' ClassFileInstaller[^.]`
- [ ] tier1-3

Thanks,
-- Igor

-

Commit messages:
 - fixup
 - update copyright year
 - rm test/lib/ClassFileInstaller.java
 - 's/ ClassFileInstaller / jdk.test.lib.helpers.ClassFileInstaller /g'

Changes: https://git.openjdk.java.net/jdk/pull/2985/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2985=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263549
  Stats: 1736 lines in 867 files changed: 0 ins; 67 del; 1669 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2985.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2985/head:pull/2985

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

Re: RFR: 8263536: Add missing @compile tags to jpackage tests [v2]

[Victor Dyakov]

On Sat, 13 Mar 2021 00:52:44 GMT, Kevin Rushforth  wrote:

>> Marked as reviewed by almatvee (Committer).
>
> Since you are removing the problem listing, would it be better to use the 
> parent bug ID -- 8263474 -- rather than a sub-task? How do you plan to 
> resolve the parent bug? Manually as "Delivered"? Something else?

..and deproblemlist mach5 failed tests

-

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

Re: RFR: 8263536: Add missing @compile tags to jpackage tests [v2]

[Kevin Rushforth]

On Sat, 13 Mar 2021 00:42:13 GMT, Alexander Matveev  
wrote:

>> Alexey Semenyuk 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 one additional 
>> commit since the last revision:
>> 
>>   8263536: Add missing @compile tags to jpackage tests
>
> Marked as reviewed by almatvee (Committer).

Since you are removing the problem listing, would it be better to use the 
parent bug ID -- 8263474 -- rather than a sub-task? How do you plan to resolve 
the parent bug? Manually as "Delivered"? Something else?

-

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

Re: RFR: 8263536: Add missing @compile tags to jpackage tests [v2]

[Alexey Semenyuk]

> 8263536: Add missing @compile tags to jpackage tests

Alexey Semenyuk has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains one commit:

  8263536: Add missing @compile tags to jpackage tests

-

Changes: https://git.openjdk.java.net/jdk/pull/2975/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2975=01
  Stats: 15 lines in 14 files changed: 13 ins; 2 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2975.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2975/head:pull/2975

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

Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations

[Alexander Matveev]

On Wed, 10 Mar 2021 18:33:37 GMT, Andy Herrick  wrote:

> implementation of
> JDK-8256145: JEP 398: Deprecate the Applet API for Removal

Marked as reviewed by almatvee (Committer).

-

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

Re: RFR: 8263536: Add missing @compile tags to jpackage tests

[Alexander Matveev]

On Fri, 12 Mar 2021 17:52:30 GMT, Alexey Semenyuk  wrote:

> 8263536: Add missing @compile tags to jpackage tests

Marked as reviewed by almatvee (Committer).

-

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

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

[Igor Ignatyev]

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

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

This pull request has now been integrated.

Changeset: e834f99d
Author:Igor Ignatyev 
URL:   https://git.openjdk.java.net/jdk/commit/e834f99d
Stats: 472 lines in 114 files changed: 145 ins; 229 del; 98 mod

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

Reviewed-by: iklam, coleenp, mseledtsov

-

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

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

[Igor Ignatyev]

On Fri, 12 Mar 2021 02:08:09 GMT, Mikhailo Seledtsov  
wrote:

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

Ioi, Coleen, Misha, thanks for your reviews.

-- Igor

-

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

RFR: 8263536: Add missing @compile tags to jpackage tests

[Alexey Semenyuk]

8263536: Add missing @compile tags to jpackage tests

-

Commit messages:
 - 8263536: Add missing @compile tags to jpackage tests

Changes: https://git.openjdk.java.net/jdk/pull/2975/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2975=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263536
  Stats: 15 lines in 14 files changed: 13 ins; 2 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2975.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2975/head:pull/2975

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

Re: RFR: 8263508: Remove dead code in MethodHandleImpl

[Johannes Kuhn]

On Fri, 12 Mar 2021 13:27:39 GMT, Claes Redestad  wrote:

> Remove unused methods.

LGTM

-

Marked as reviewed by jkuhn (Author).

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

Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v29]

[Jim Laskey]

> This PR is to introduce a new random number API for the JDK. The primary API 
> is found in RandomGenerator and RandomGeneratorFactory. Further description 
> can be found in the JEP https://openjdk.java.net/jeps/356 .
> 
> javadoc can be found at 
> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
> 
> old PR:  https://github.com/openjdk/jdk/pull/1273

Jim Laskey has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 63 commits:

 - Review requested changes
 - Merge branch 'master' into 8248862
 - Remove conflicts
 - Use isAnnotationPresent
 - Introduce isDeprecated
 - Update javadoc
 - Merge branch 'master' into 8248862
 - Adjust ThreadLocalRandom javadoc inheritence
 - L32X64StarStarRandom -> L32X64MixRandom
 - Various corrects
 - ... and 53 more: https://git.openjdk.java.net/jdk/compare/273f8bdf...e5b87227

-

Changes: https://git.openjdk.java.net/jdk/pull/1292/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1292=28
  Stats: 13827 lines in 26 files changed: 11646 ins; 1849 del; 332 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1292.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1292/head:pull/1292

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

Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v3]

[Chris Hegarty]

On Thu, 11 Mar 2021 16:42:24 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.lang` 
>> package to make use of the `instanceof` pattern variable?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8263358: Refactored equals methods

src/java.base/share/classes/java/lang/ProcessHandleImpl.java line 525:

> 523: return (pid == other.pid) &&
> 524: (startTime == other.startTime
> 525: || startTime == 0

This one can be a single expression. Otherwise, looks good.

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v21]

[Andrew Haley]

On Tue, 9 Mar 2021 18:01:11 GMT, Anton Kozlov  wrote:

>> src/hotspot/cpu/aarch64/globalDefinitions_aarch64.hpp line 62:
>> 
>>> 60: 
>>> 61: #if defined(__APPLE__) || defined(_WIN64)
>>> 62: #define R18_RESERVED
>> 
>> #define R18_RESERVED true```
>
> We always check for `R18_RESERVED` with `#if(n)def`, is there any reason to 
> define the value for the macro?

Robustness, clarity, maintainability, convention. Why not?

-

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

Re: RFR: 8189198: Add "forRemoval = true" to Applet API deprecations

[Iris Clark]

On Wed, 10 Mar 2021 18:33:37 GMT, Andy Herrick  wrote:

> implementation of
> JDK-8256145: JEP 398: Deprecate the Applet API for Removal

Marked as reviewed by iris (Reviewer).

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v27]

[Anton Kozlov]

> Please review the implementation of JEP 391: macOS/AArch64 Port.
> 
> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
> windows/aarch64. 
> 
> Major changes are in:
> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
> JDK-8253817, JDK-8253818)
> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary 
> adjustments (JDK-8253819)
> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
> required on macOS/AArch64 platform. It's implemented with 
> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
> thread, so W^X mode change relates to the java thread state change (for java 
> threads). In most cases, JVM executes in write-only mode, except when calling 
> a generated stub like SafeFetch, which requires a temporary switch to 
> execute-only mode. The same execute-only mode is enabled when a java thread 
> executes in java or native states. This approach of managing W^X mode turned 
> out to be simple and efficient enough.
> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)

Anton Kozlov has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix most of issues in java/foreign/ tests
  
  Failures related to va_args are tracked in JDK-8263512.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2200/files
  - new: https://git.openjdk.java.net/jdk/pull/2200/files/29991c92..5bfe0f08

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2200=26
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2200=25-26

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

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

Re: RFR: 8263509: LdapSchemaParser.readNextTag checks array length incorrectly

[Thomas Stuefe]

On Fri, 12 Mar 2021 13:25:31 GMT, Aleksey Shipilev  wrote:

> SonarCloud rightfully says:
>   The length of "values" is always ">=0", so update this test to either "==0" 
> or ">0".
> 
> // make sure at least one value was returned
> if(values.length < 0) { // <--- here
> throw new InvalidAttributeValueException("no values for " +
>  "attribute "" +
>  tagName + """);
> }
> 
> There is a subsequent access to values[0], which means the failure would 
> throw `AIOOB`, not `InvalidAttributeValueException`.
> 
> Additional testing:
>  - [x] Linux x86_64 fastdebug, `com/sun/jndi`

Seems fine. Lets hope no caller relies on this throwing AIOOBE. 

..Thomas

-

Marked as reviewed by stuefe (Reviewer).

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v24]

[Andrew Haley]

On Tue, 9 Mar 2021 16:12:36 GMT, Anton Kozlov  wrote:

>> Please review the implementation of JEP 391: macOS/AArch64 Port.
>> 
>> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
>> windows/aarch64. 
>> 
>> Major changes are in:
>> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
>> JDK-8253817, JDK-8253818)
>> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with 
>> necessary adjustments (JDK-8253819)
>> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
>> required on macOS/AArch64 platform. It's implemented with 
>> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
>> thread, so W^X mode change relates to the java thread state change (for java 
>> threads). In most cases, JVM executes in write-only mode, except when 
>> calling a generated stub like SafeFetch, which requires a temporary switch 
>> to execute-only mode. The same execute-only mode is enabled when a java 
>> thread executes in java or native states. This approach of managing W^X mode 
>> turned out to be simple and efficient enough.
>> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)
>
> Anton Kozlov has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 105 commits:
> 
>  - Merge commit 'refs/pull/11/head' of https://github.com/AntonKozlov/jdk 
> into jdk-macos
>  - workaround JDK-8262895 by disabling subtest
>  - Fix typo
>  - Rename threadWXSetters.hpp -> threadWXSetters.inline.hpp
>  - JDK-8259937: bsd_aarch64 part
>  - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
>  - Fix after JDK-8259539, partially revert preconditions
>  - JDK-8260471: bsd_aarch64 part
>  - JDK-8259539: bsd_aarch64 part
>  - JDK-8257828: bsd_aarch64 part
>  - ... and 95 more: 
> https://git.openjdk.java.net/jdk/compare/a6e34b3d...a72f6834

> @theRealAph, could you elaborate on what is need to be done for [#2200 
> (review)](https://github.com/openjdk/jdk/pull/2200#pullrequestreview-600597066).

I think that what you've got now is fine.

-

Marked as reviewed by aph (Reviewer).

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

RFR: 8189198: Add "forRemoval = true" to Applet API deprecations

[Andy Herrick]

implementation of
JDK-8256145: JEP 398: Deprecate the Applet API for Removal

-

Commit messages:
 - 8189198: Add "forRemoval = true" to Applet API deprecations

Changes: https://git.openjdk.java.net/jdk/pull/2920/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2920=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8189198
  Stats: 74 lines in 22 files changed: 14 ins; 0 del; 60 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2920.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2920/head:pull/2920

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

Re: RFR: 8263509: LdapSchemaParser.readNextTag checks array length incorrectly

[Alan Bateman]

On Fri, 12 Mar 2021 13:25:31 GMT, Aleksey Shipilev  wrote:

> SonarCloud rightfully says:
>   The length of "values" is always ">=0", so update this test to either "==0" 
> or ">0".
> 
> // make sure at least one value was returned
> if(values.length < 0) { // <--- here
> throw new InvalidAttributeValueException("no values for " +
>  "attribute "" +
>  tagName + """);
> }
> 
> There is a subsequent access to values[0], which means the failure would 
> throw `AIOOB`, not `InvalidAttributeValueException`.
> 
> Additional testing:
>  - [x] Linux x86_64 fastdebug, `com/sun/jndi`

This looks right but I'm surprised it isn't caught by tests (@AlekseiEfimov - 
do you have suggests for tests that would be useful to add here?)

-

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

RFR: 8263509: LdapSchemaParser.readNextTag checks array length incorrectly

[Aleksey Shipilev]

SonarCloud rightfully says:
  The length of "values" is always ">=0", so update this test to either "==0" 
or ">0".

// make sure at least one value was returned
if(values.length < 0) { // <--- here
throw new InvalidAttributeValueException("no values for " +
 "attribute "" +
 tagName + """);
}

There is a subsequent access to values[0], which means the failure would throw 
`AIOOB`, not `InvalidAttributeValueException`.

Additional testing:
 - [x] Linux x86_64 fastdebug, `com/sun/jndi`

-

Commit messages:
 - 8263509: LdapSchemaParser.readNextTag checks array length incorrectly

Changes: https://git.openjdk.java.net/jdk/pull/2968/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2968=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263509
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2968.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2968/head:pull/2968

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

Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]

[Chris Hegarty]

On Fri, 12 Mar 2021 13:23:39 GMT, Peter Levart  wrote:

>> Jaikiran Pai has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Follow Peter Levart's review suggestion - remove volatile
>
> This looks good to me. Maybe if you wanted the previous performance back 
> (when the `cannonicalMap` field was static final and therefore JIT could 
> constant-fold the Map instance), you could now annotate the field with 
> `@Stable` annotation to allow JIT to produce similar code. I would also move 
> the `Map.ofEntries(...)` expression into a separate private static method 
> since I believe it has substantial bytecode size. `tryCanonicalize()` method 
> bytecode size would then more likely fall below the inlining threshold.

What you have is probably fine, but has any consideration been given to using 
the initialization-on-demand holder idiom? e.g.

 static private class CanonicalMapHolder {
static final Map, 
ConstantDesc>> CANONICAL_MAP
  = Map.ofEntries(..);
 }

-

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

RFR: 8263508: Remove dead code in MethodHandleImpl

[Claes Redestad]

Remove unused methods.

-

Commit messages:
 - Revert assertSame removal (used from InvokerByteCodeGenerator)
 - Remove dead code in MethodHandleImpl

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

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

Re: RFR: 8263108: Class initialization deadlock in java.lang.constant [v2]

[Peter Levart]

On Wed, 10 Mar 2021 02:15:28 GMT, Jaikiran Pai  wrote:

>> Can I please get a review for this proposed patch for the issue reported in 
>> https://bugs.openjdk.java.net/browse/JDK-8263108?
>> 
>> As noted in that issue, the `java.lang.constant.DynamicConstantDesc` and 
>> `java.lang.constant.ConstantDescs` can end up in a classloading deadlock due 
>> to the nature of the code involved in their static blocks. A thread dump of 
>> one such deadlock (reproduced using the code attached to that issue) is as 
>> follows:
>> 
>> "Thread A" #14 prio=5 os_prio=31 cpu=101.45ms elapsed=8.30s 
>> tid=0x7ff88e01ca00 nid=0x6003 in Object.wait()  [0x7a4c6000]
>>java.lang.Thread.State: RUNNABLE
>>  at 
>> java.lang.constant.DynamicConstantDesc.(java.base@16-ea/DynamicConstantDesc.java:67)
>>  - waiting on the Class initialization monitor for 
>> java.lang.constant.ConstantDescs
>>  at Deadlock.threadA(Deadlock.java:14)
>>  at Deadlock$$Lambda$1/0x000800c00a08.run(Unknown Source)
>>  at java.lang.Thread.run(java.base@16-ea/Thread.java:831)
>> 
>> "Thread B" #15 prio=5 os_prio=31 cpu=103.15ms elapsed=8.30s 
>> tid=0x7ff88b936a00 nid=0x9b03 in Object.wait()  [0x7a5c9000]
>>java.lang.Thread.State: RUNNABLE
>>  at 
>> java.lang.constant.ClassDesc.ofDescriptor(java.base@16-ea/ClassDesc.java:145)
>>  - waiting on the Class initialization monitor for 
>> java.lang.constant.DynamicConstantDesc
>>  at 
>> java.lang.constant.ConstantDescs.(java.base@16-ea/ConstantDescs.java:239)
>>  at Deadlock.threadB(Deadlock.java:24)
>>  at Deadlock$$Lambda$2/0x000800c00c28.run(Unknown Source)
>>  at java.lang.Thread.run(java.base@16-ea/Thread.java:831)
>> 
>> The commit in this PR resolves that deadlock by moving the `canonicalMap` 
>> initialization in `DynamicConstantDesc`, from the static block, to a lazily 
>> initialized map, into the `tryCanonicalize` (private) method of the same 
>> class. That's the only method which uses this map.
>> 
>> The implementation in `tryCanonicalize` carefully ensures that the deadlock 
>> isn't shifted from the static block to this method, by making sure that the 
>> `synchronization` on `DynamicConstantDesc` in this method happens only when 
>> it's time to write the state to the shared `canonicalMap`. This has an 
>> implication that the method local variable `canonDescs` can get initialized 
>> by multiple threads unnecessarily but from what I can see, that's the only 
>> way we can avoid this deadlock. This penalty of multiple threads 
>> initializing and then throwing away that map isn't too huge because that 
>> will happen only once when the `canonicalMap` is being initialized for the 
>> first time.
>> 
>> An alternate approach that I thought of was to completely get rid of this 
>> shared cache `canonicalMap` and instead just use method level map (that gets 
>> initialized each time) in the `tryCanonicalize` method (thus requiring no 
>> locks/synchronization). I ran a JMH benchmark with the current change 
>> proposed in this PR and with the alternate approach of using the method 
>> level map. The performance number from that run showed that the approach of 
>> using the method level map has relatively big impact on performance (even 
>> when there's a cache hit in that method). So I decided to not pursue that 
>> approach. I'll include the benchmark code and the numbers in a comment in 
>> this PR, for reference.
>> 
>> The commit in this PR also includes a jtreg testcase which (consistently) 
>> reproduces the deadlock without the fix and passes when this fix is applied. 
>> Extra manual testing has been done to verify that the classes of interest 
>> (noted in that testcase) are indeed getting loaded in those concurrent 
>> threads and not in the main thread. For future maintainers, there's a 
>> implementation note added on that testcase explaining why it cannot be moved 
>> into testng test.
>
> Jaikiran Pai has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Follow Peter Levart's review suggestion - remove volatile

This looks good to me. Maybe if you wanted the previous performance back (when 
the `cannonicalMap` field was static final and therefore JIT could 
constant-fold the Map instance), you could now annotate the field with 
`@Stable` annotation to allow JIT to produce similar code. I would also move 
the `Map.ofEntries(...)` expression into a separate private static method since 
I believe it has substantial bytecode size. `tryCanonicalize()` method bytecode 
size would then more likely fall below the inlining threshold.

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v25]

[Anton Kozlov]

On Thu, 11 Mar 2021 20:28:46 GMT, Stefan Karlsson  wrote:

>> Anton Kozlov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8262903: [macos_aarch64] Thread::current() called on detached thread
>
> Marked as reviewed by stefank (Reviewer).

> you still need to use Thread::current_or_null_safe() [for SafeFetch].

OK :) I fixed this in 
https://github.com/openjdk/jdk/pull/2200/commits/fd4812e585e0528010a8863df50956a3b64a6744

@dcubed-ojdk, I also updated copyrights, this concludes fixes for the review 
https://github.com/openjdk/jdk/pull/2200#pullrequestreview-581784107.

@theRealAph, could you elaborate on what is need to be done for 
https://github.com/openjdk/jdk/pull/2200#pullrequestreview-600597066.

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Vladimir Kempik]

On Tue, 2 Mar 2021 08:12:10 GMT, Anton Kozlov  wrote:

>> I wasn't able to replicate JDK-8020753 and JDK-8186286. So will remove these 
>> workaround
>> @gerard-ziemski, 8020753 was originally your fix, do you know if it still 
>> needed on intel-mac ?
>
> The x86_bsd still carries the workaround 
> https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp#L745.
>  It's worth having macos ports to be aligned by features. I would propose to 
> have this workaround for now, and decide on it later for macos/x86 and 
> macos/aarch64 at once. Sorry for chiming in so late.

Hello Anton.
Please keep JDK-8020753 away from this port, as JDK-8020753 was very 
intel-specific workaround and macos11.0 on aarch64 doesn't show any signs of 
original bug.

-

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

Re: RFR: 8173970: jar tool should have a way to extract to a directory

[Lance Andersen]

On Mar 12, 2021, at 6:32 AM, Alan Bateman 
mailto:alan.bate...@oracle.com>> wrote:

On 11/03/2021 15:25, Lance Andersen wrote:

The only reason I included it is for completeness with tar which the jar 
options were originally derived from.

I think it would be surprising to have two GNU-style long options so I think we 
should pick one.  "--dir" seems okay to me and is consistent with the other the 
other JDK tools that extract to a directory (although those tools might not be 
well known). If you prefer "--directory" then I think we'll should add it to 
the other tools and hide "--dir" from the usage output.

I don’t have a strong preference but lean slightly towards ‘-directory’ as it 
is more descriptive, similar to the other GNU-style commands jar currently 
supports .  Tar supports ‘—cd’, ‘—directory’ in addition to ‘-C’ which is why I 
suggested supporting  both GNU-style long options.

Perhaps jpackage should also support —dir/directory in addition to ‘—dest' if 
we are looking at consistency between java tools.

I do agree that it would be nice to be consistent across the java tools for 
options so if we go the ‘-directory’, we should follow your suggestion and make 
it the primary and remove ‘—dir’ from the usage output.


-Alan

[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v26]

[Anton Kozlov]

> Please review the implementation of JEP 391: macOS/AArch64 Port.
> 
> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and 
> windows/aarch64. 
> 
> Major changes are in:
> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks 
> JDK-8253817, JDK-8253818)
> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with necessary 
> adjustments (JDK-8253819)
> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), 
> required on macOS/AArch64 platform. It's implemented with 
> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a 
> thread, so W^X mode change relates to the java thread state change (for java 
> threads). In most cases, JVM executes in write-only mode, except when calling 
> a generated stub like SafeFetch, which requires a temporary switch to 
> execute-only mode. The same execute-only mode is enabled when a java thread 
> executes in java or native states. This approach of managing W^X mode turned 
> out to be simple and efficient enough.
> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941)

Anton Kozlov has updated the pull request incrementally with three additional 
commits since the last revision:

 - Add Azul copyright
 - Update Oracle copyright years
 - Use Thread::current_or_null_safe in SafeFetch

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2200/files
  - new: https://git.openjdk.java.net/jdk/pull/2200/files/f6fb01b2..29991c92

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=2200=25
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2200=24-25

  Stats: 83 lines in 53 files changed: 41 ins; 0 del; 42 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2200.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2200/head:pull/2200

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

Withdrawn: 8261031: Move some ClassLoader name checking to native/VM

[Claes Redestad]

On Wed, 3 Feb 2021 12:21:30 GMT, Claes Redestad  wrote:

> This patch moves some sanity checking done in ClassLoader.java to the 
> corresponding endpoints in native or VM code.

This pull request has been closed without being integrated.

-

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

Re: RFR: 8261031: Move some ClassLoader name checking to native/VM [v3]

[Claes Redestad]

On Fri, 12 Feb 2021 22:48:51 GMT, Mandy Chung  wrote:

>> This more limited cleanup looks good.
>
> This patch changes `JVM_FindLoadedClass` interface to only accept a binary 
> name.   It used to accept both a binary name and internal form.  Most, if not 
> all, JVM entry points take the name of internal name.   So this change makes 
> this JVM entry point inconsistent with others. 
> 
> Looking closer each API that involves `fixClassName` or `verifyXXXClassName`, 
> the JVM entry points called expects the internal form except 
> `JVM_FindLoadedClass` (see details below).   I think a better change is to 
> change the native `JVM_FindLoadedClass` to accept the internal form only and 
> have `findLoadedClass0` method to detect if the name contains '/' or '['.
> 
> ClassLoader API does not allow loading of an array type whereas 
> `Class::forName` allows to find an array type.  Perhaps `verifyFixClassName` 
> should be renamed like `binaryNameToInternalForm`.   I think we don't need 
> `fixClassname`?
> 
> ClassLoader::defineClass
> - `preDefineClass` checks the name and throws if it contains '/' or '['
> - no name check in `JVM_DefineClassWithSource` and `JVM_LookupDefineClass`
>  which expects the name is of internal form
> 
> native Class::forName0
> - converts the binary name to internal form (i.e. replace '.' with '/') 
> - throw if the name contains '/'
>  - no explicit name check in `JVM_FindClassFromCaller`
> 
> ClassLoader::loadClass
> - calls native `findLoadedClass0` that calls `JVM_FindLoadedClass` which  
>   
>accepts binary form and converts '.' to '/' but the current implementation
>accepts both binary name and internal form
> - calls `native findBootstrapClass` which converts '.' to '/' and pass the 
> internal
>form to `JVM_FindBootstrapClass`.  
> 
> It'd be helpful to document the internal APIs and JVM entry points clearly 
> what it expects for example binary name vs internal form and where it does 
> the validation e.g. Class::forName0 allows array type and native library 
> methods do the name validation.

Abandoning this. Sorry for wasting everyone's time.

-

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

Re: RFR: 8173970: jar tool should have a way to extract to a directory

[Alan Bateman]

On 11/03/2021 15:25, Lance Andersen wrote:


The only reason I included it is for completeness with tar which the 
jar options were originally derived from.


I think it would be surprising to have two GNU-style long options so I 
think we should pick one.  "--dir" seems okay to me and is consistent 
with the other the other JDK tools that extract to a directory (although 
those tools might not be well known). If you prefer "--directory" then I 
think we'll should add it to the other tools and hide "--dir" from the 
usage output.


-Alan