Re: RFR: 8195129: System.load() fails to load from unicode paths [v2]

[Gerard Ziemski]

On Tue, 25 May 2021 09:45:34 GMT, Maxim Kartashev 
 wrote:

>> Character strings within JVM are produced and consumed in several formats. 
>> Strings come from/to Java in the UTF8 format and POSIX APIs (like fprintf() 
>> or dlopen()) consume strings also in UTF8. On Windows, however, the 
>> situation is far less simple: some new(er) APIs expect UTF16 (wide-character 
>> strings), some older APIs can only work with strings in a "platform" format, 
>> where not all UTF8 characters can be represented; which ones can depends on 
>> the current "code page".
>> 
>> This commit switches the Windows version of native library loading code to 
>> using the new UTF16 API `LoadLibraryW()` and attempts to streamline the use 
>> of various string formats in the surrounding code. 
>> 
>> Namely, exception messages are made to consume strings explicitly in the 
>> UTF8 format, while logging functions (that end up using legacy Windows API) 
>> are made to consume "platform" strings in most cases. One exception is 
>> `JVM_LoadLibrary()` logging where the UTF8 name of the library is logged, 
>> which can, of course, be fixed, but was considered not worth the additional 
>> code (NB: this isn't a new bug).
>> 
>> The test runs in a separate JVM in order to make NIO happy about non-ASCII 
>> characters in the file name; tests are executed with LC_ALL=C and that 
>> doesn't let NIO work with non-ASCII file names even on Linux or MacOS.
>> 
>> Tested by running `test/hotspot/jtreg:tier1` on Linux and 
>> `jtreg:test/hotspot/jtreg/runtime` on Windows 10. The new test (`   
>> jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`) was explicitly ran 
>> on those platforms as well.
>> 
>> Results from Linux:
>> 
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
>>jtreg:test/hotspot/jtreg:tier1 1784  1784 0 0 
>>   
>> ==
>> TEST SUCCESS
>> 
>> 
>> Building target 'run-test-only' in configuration 
>> 'linux-x86_64-server-release'
>> Test selection 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', 
>> will run:
>> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode
>> 
>> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode'
>> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java
>> Test results: passed: 1
>> 
>> 
>> Results from Windows 10:
>> 
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR
>>jtreg:test/hotspot/jtreg/runtime746   746 0 0
>> ==
>> TEST SUCCESS
>> Finished building target 'run-test-only' in configuration 
>> 'windows-x86_64-server-fastdebug'
>> 
>> 
>> Building target 'run-test-only' in configuration 
>> 'windows-x86_64-server-fastdebug'
>> Test selection 'test/hotspot/jtreg/runtime/jni/loadLibraryUnicode', will run:
>> * jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode
>> 
>> Running test 'jtreg:test/hotspot/jtreg/runtime/jni/loadLibraryUnicode'
>> Passed: runtime/jni/loadLibraryUnicode/LoadLibraryUnicodeTest.java
>> Test results: passed: 1
>
> Maxim Kartashev 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:
> 
>   8195129: System.load() fails to load from unicode paths

I tried verifying the test on **macOS** by running:

`jtreg -nr -va -jdk:./build/macosx-x86_64-server-fastdebug/images/jdk 
test/hotspot/jtreg/runtime/jni/loadLibraryUnicode`

and I get:

`TEST RESULT: Error. Use -nativepath to specify the location of native code`

I looked at the test and thought it was handling loading the native lib on its 
own, without the test needing to set  **nativepath** explicitly.  What am I 
doing wrong?

-

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

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

[Gerard Ziemski]

On Wed, 3 Mar 2021 17:46:41 GMT, Andrew Haley  wrote:

> > A list of the bugs that our internal testing revealed so far:
> 
> Are any of these blockers for integration? Some of them are to do with things 
> like features that aren't yet supported, and we can't fix what we can't see.

I don't personally think any of these issues are blockers. It's a great effort 
as it is and very much appreciated. Anything else can be fixed as a followup.

There might be some legal requirements (i.e. JCK) that I'm not in position to 
comment on, however, so someone else might need to chime in here.

-

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

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

[Gerard Ziemski]

On Tue, 2 Mar 2021 11:05:20 GMT, Anton Kozlov  wrote:

>> For platform files that were copied from other ports to this port, if the 
>> file wasn't
>> changed I presume the copyright years are left alone. If the file required 
>> changes
>> for this port, I expect the year to be updated to 2021. How are you 
>> verifying that
>> these copyright years are being properly managed on the new files?
>> 
>> For the new W^X helpers, e.g., WXWriteFromExecSetter, a short comment
>> explaining why one was landed in a particular place would help reviewers.
>> Also see my comment about creating a new ThreadToNativeWithWXExecFromVM
>> helper.
>> 
>> I'm stopping my review with all the src/hotspot files done for now.
>
>> For platform files that were copied from other ports to this port, if the 
>> file wasn't
>> changed I presume the copyright years are left alone. If the file required 
>> changes
>> for this port, I expect the year to be updated to 2021. How are you 
>> verifying that
>> these copyright years are being properly managed on the new files?
> 
> There are no exact copies, based on
> git -c diff.renameLimit=1000 diff --find-copies-harder -C75% 
> --name-status upstream/master...
> 
> So every file changed in the branch potentially needs the copyright update. 
> All file diffs are not trivial, IMHO.
> 
> I'll run the copyright update after we fix a few remaining issues with the 
> PR, to avoid updating copyright and changing/reverting the actual content.

A list of the bugs that our internal testing revealed so far:

https://bugs.openjdk.java.net/browse/JDK-8262952
https://bugs.openjdk.java.net/browse/JDK-8262894
https://bugs.openjdk.java.net/browse/JDK-8262895
https://bugs.openjdk.java.net/browse/JDK-8262896
https://bugs.openjdk.java.net/browse/JDK-8262897
https://bugs.openjdk.java.net/browse/JDK-8262898
https://bugs.openjdk.java.net/browse/JDK-8262899
https://bugs.openjdk.java.net/browse/JDK-8262900
https://bugs.openjdk.java.net/browse/JDK-8262901
https://bugs.openjdk.java.net/browse/JDK-8262903
https://bugs.openjdk.java.net/browse/JDK-8262904

-

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

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

[Gerard Ziemski]

On Tue, 2 Mar 2021 23:21:28 GMT, David Holmes  wrote:

> Note that `thread` can be NULL here if the signal handler is running in a 
> non-attached thread. If we then perform:
> `ThreadWXEnable(WXMode new_mode, Thread* thread = NULL) : _thread(thread ? 
> thread : Thread::current()),`
> we call Thread::current() on a non-attached thread and that will assert/crash 
> if we get NULL. Either avoid using WX when the thread is NULL, or else change 
> to use Thread::current_or_null_safe() and ensure all uses have a NULL check.

https://bugs.openjdk.java.net/browse/JDK-8262903 tracks this issue.

-

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

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

[Gerard Ziemski]

On Wed, 17 Feb 2021 12:36:10 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 88 commits:
> 
>  - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos
>  - Re-do safefetch.hpp
>  - Merge remote-tracking branch 'origin/jdk/8261075-stubroutines-inline' into 
> jdk-macos
>  - stubRoutines.inline.hpp -> safefetch.hpp
>  - Update copyrights
>  - Merge remote-tracking branch 'upstream/jdk/master' into 
> 8261075-stubroutines-inline
>  - Merge remote-tracking branch 'upstream/jdk/master' into 
> 8261075-stubroutines-inline
>  - Extract SafeFetch32/N to stubRoutines.inline.hpp
>  - Revert "Extract SafeFetch32/N to stubRoutines.inline.hpp"
>
>This reverts commit b873c25f31dd21349d140b790713cc9ccb5f2dc0.
>  - Merge pull request #9 from VladimirKempik/pull/2200
>
>Removed unused variables
>  - ... and 78 more: 
> https://git.openjdk.java.net/jdk/compare/b955f85e...ab72613c

Marked as reviewed by gziemski (Committer).

-

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

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

[Gerard Ziemski]

On Fri, 5 Feb 2021 12:26:27 GMT, Anton Kozlov  wrote:

>> Marked as reviewed by ihse (Reviewer).
>
>> I haven't got a MacOS AArch64 system right now. Is it possible to
>> enable W^X in Linux in order to kick the tyres?
> 
> I've just got rid of asserts that fired on Linux sometime :) As for W^X like 
> on macOS, I vaguely remember working with a Linux system with one-way 
> transition W->X, probably provided by SELinux. But I don't think it allowed 
> per-thread W^X state.

> _Mailing list message from [daniel.daugherty at 
> oracle.com](mailto:daniel.daughe...@oracle.com) on 
> [security-dev](mailto:security-...@openjdk.java.net):_
> 
> On 2/5/21 4:51 AM, Magnus Ihse Bursie wrote:
> 
> @magicus - I'm good with both of these answers. I personally like 'macosx'.
> 
> Dan

It's no longer `macosx`, it's just `macos` now - see 
https://en.wikipedia.org/wiki/MacOS

-

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

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

[Gerard Ziemski]

On Tue, 2 Feb 2021 22:09:58 GMT, Daniel D. Daugherty  wrote:

>> Anton Kozlov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   support macos_aarch64 in hsdis
>
> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 363:
> 
>> 361: address pc = os::Posix::ucontext_get_pc(uc);
>> 362: 
>> 363: if (pc != addr && uc->context_esr == 0x924F) { //TODO: figure 
>> out what this value means
> 
> Is this TODO going to be resolved by this port?

Where did this come from - some snippet/example/tech note code? Maybe other 
people can help figure it out if we provide more info.

> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 374:
> 
>> 372: 
>> 373:   last_addr = (address) -1;
>> 374: } else if (pc == addr && uc->context_esr == 0x820f) { //TODO: 
>> figure out what this value means
> 
> Is this TODO going to be resolved by this port?

Where did this come from - some snippet/example/tech note code? Maybe other 
people can help figure it out if we provide more info.

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:01:15 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 six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 403:

> 401:   }
> 402: 
> 403:   return false; // Mute compiler

Is this comment needed?

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 420:

> 418: size_t os::Posix::_compiler_thread_min_stack_allowed = 72 * K;
> 419: size_t os::Posix::_java_thread_min_stack_allowed = 72 * K;
> 420: size_t os::Posix::_vm_internal_thread_min_stack_allowed = 72 * K;

Those are slightly larger than their x86_64 counter parts. Are they 
conservative/aggressive values? How did we arrive at those?

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 652:

> 650: 
> 651: void os::setup_fpu() {
> 652: }

Is there really nothing to do here, or does still need to be implemented? A 
clarification comment here would help/.

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:01:15 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 six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

I reviewed bsd_aarch64.cpp digging bit deeper and left some comments.

-

Changes requested by gziemski (Committer).

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:01:15 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 six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 297:

> 295:   stub = SharedRuntime::handle_unsafe_access(thread, next_pc);
> 296: }
> 297:   } else if (sig == SIGILL && nativeInstruction_at(pc)->is_stop()) {

Can we add a comment here describing what this case means?

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:01:15 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 six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 302:

> 300: const uint64_t *detail_msg_ptr
> 301:   = (uint64_t*)(pc + NativeInstruction::instruction_size);
> 302: const char *detail_msg = (const char *)*detail_msg_ptr;

Where is `detail_msg` used?

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:01:15 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 six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 237:

> 235:   os::Posix::ucontext_set_pc(uc, 
> StubRoutines::continuation_for_safefetch_fault(pc));
> 236:   return true;
> 237: }

Isn't this case already handled by `JVM_HANDLE_XXX_SIGNAL()` ? Why do we need 
it here again?

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:01:15 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 six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 198:

> 196: 
> 197: NOINLINE frame os::current_frame() {
> 198:   intptr_t *fp = *(intptr_t **)__builtin_frame_address(0);

In the bsd_x86 counter part we initialize `fp` to `_get_previous_fp()` - do we 
need to implement it on aarch64 as well (and using address 0 is just a temp 
workaround) or is it doing the right thing here?

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:01:15 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 six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 291:

> 289: bool is_unsafe_arraycopy = (thread->doing_unsafe_access() && 
> UnsafeCopyMemory::contains_pc(pc));
> 290: if ((nm != NULL && nm->has_unsafe_access()) || 
> is_unsafe_arraycopy) {
> 291:   address next_pc = pc + NativeCall::instruction_size;

Replace

address next_pc = pc + NativeCall::instruction_size;

with

address next_pc = Assembler::locate_next_instruction(pc);

there is at least one other place that needs it.

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 322:

> 320: #ifdef __APPLE__
> 321:   } else if (sig == SIGFPE && info->si_code == FPE_NOOP) {
> 322: Unimplemented();

Is there a follow up issue for this?

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:01:15 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 six additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/jdk/jdk-macos' into jdk-macos
>  - Add comments to WX transitions
>
>+ minor change of placements
>  - Use macro conditionals instead of empty functions
>  - Add W^X to tests
>  - Do not require known W^X state
>  - Revert w^x in gtests

src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp line 194:

> 192: // may get turned off by -fomit-frame-pointer.
> 193: frame os::get_sender_for_C_frame(frame* fr) {
> 194:   return frame(fr->link(), fr->link(), fr->sender_pc());

Why is it

return frame(fr->link(), fr->link(), fr->sender_pc());

and not 

return frame(fr->sender_sp(), fr->link(), fr->sender_pc());

like in the bsd-x86 counter part?

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 22:17:02 GMT, Bernhard Urban-Forster  
wrote:

>> To answer my own question, it seems that code is still needed on `x86_64` 
>> for `lldb` with `EXC_MASK_BAD_ACCESS` or we keep tripping over 
>> `EXC_BAD_ACCESS`
>> 
>> Remaining questions:
>> 
>> a) why we need `EXC_MASK_ARITHMETIC` ?
>> b) we hit `signal SIGSEGV` in debugger even with the code in place, any way 
>> to avoid that?
>> c) does `BSD aarch64` need only `EXC_MASK_BAD_INSTRUCTION` or does it need 
>> `EXC_MASK_BAD_ACCESS` as well?
>> d) can we `#ifdef` the `EXC_MASK_BAD_INSTRUCTION` part of the mask only to 
>> apply to `aarch64`?
>
> Thanks for your questions Gerard.
> 
>> Part of the comment said This work-around is not necessary for 10.5+, as 
>> CrashReporter no longer intercedes on caught fatal signals.
> 
> That comment can probably be deleted since minversion is anyway 10.9 (and 
> soon 10.12 https://github.com/openjdk/jdk/pull/2268 ).
> 
>> Do you know if this also apply to lldb or is it gdb only specific? How do 
>> you run gdb on macOS nowadays anyhow?
> 
> `lldb` is shipped with Xcode, `gdb` isn't. You would need to build and sign 
> it yourself, I haven't tried that in a while. So, we should update that 
> comment to talk about `lldb`  
> 
>> a) why we need `EXC_MASK_ARITHMETIC` ?
> 
> I _believe_ this dates back to i386. As far as I can tell this isn't needed 
> for x86_64 or aarch64. I guess we can remove it, the worst case is that it 
> makes the debugging experience of the runtime a little bit worse. OTOH it 
> doesn't hurt either to have it here.
> 
>> b) we hit signal SIGSEGV in debugger even with the code in place, any way to 
>> avoid that?
> 
> The equivalent for `handle SIGSEGV nostop noprint` (gdb) in lldb is `process 
> handle -n false -p true -s false SIGSEGV`.
> 
>> c) does `BSD aarch6` need only `EXC_MASK_BAD_INSTRUCTION` or does it need 
>> `EXC_MASK_BAD_ACCESS` as well?
> 
> aarch64 needs `EXC_MASK_BAD_ACCESS` at least for implicit null checking, 
> there might be other cases.
> 
>> d) can we `#ifdef` the `EXC_MASK_BAD_INSTRUCTION` part of the mask only to 
>> apply to `aarch64`?
> 
> Maybe. I don't see any value in it though, except making the code more 
> complicated to read 

I don't like the idea of using masks on architectures that do not require them. 
How about something like this?

`#if defined(__APPLE__)`
`  // lldb (gdb) installs both standard BSD signal handlers, and mach exception`
`  // handlers. By replacing the existing task exception handler, we disable 
lldb's mach`
`  // exception handling, while leaving the standard BSD signal handlers 
functional.`
`  //`
`  // EXC_MASK_BAD_ACCESS needed by all architectures for NULL ptr checking`
`  // EXC_MASK_ARITHMETIC needed by i386`
`  // EXC_MASK_BAD_INSTRUCTION needed by aarch64 to initiate deoptimization`
`  kern_return_t kr;`
`  kr = task_set_exception_ports(mach_task_self(),`
`EXC_MASK_BAD_ACCESS`
`NOT_LP64(| EXC_MASK_ARITHMETIC)`
`AARCH64_ONLY(| EXC_MASK_BAD_INSTRUCTION),`
`MACH_PORT_NULL,`
`EXCEPTION_STATE_IDENTITY,`
`MACHINE_THREAD_STATE);`
` `
`  assert(kr == KERN_SUCCESS, "could not set mach task signal handler");`
`#endif`

If I just knew why i386 needs `EXC_MASK_ARITHMETIC` and add that to the comment 
I would be personally happy with that chunk of code.

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 23:13:12 GMT, Bernhard Urban-Forster  
wrote:

>> No idea how to insert spaces and make text align :-(
>
> using ` ```c ` 
> https://docs.github.com/en/github/writing-on-github/creating-and-highlighting-code-blocks
> 
> I was wrong about `SIGFPE` / `EXC_MASK_ARITHMETIC`, it's used on i386, x86_64:
> https://github.com/openjdk/jdk/blob/2be60e37e0e433141b2e3d3e32f8e638a4888e3a/src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp#L467-L524
> and aarch64:
> https://github.com/AntonKozlov/jdk/blob/80827176cbc5f0dd26003cf234a8076f3f557928/src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.cpp#L309-L323
> (What happened with the formatting here, ugh?)
> 
> Your suggestion sounds good otherwise. @AntonKozlov, do you mind to integrate 
> that?

So it should be:

#if defined(__APPLE__)
  // lldb (gdb) installs both standard BSD signal handlers, and mach exception
  // handlers. By replacing the existing task exception handler, we disable 
lldb's mach
  // exception handling, while leaving the standard BSD signal handlers 
functional.
  //
  // EXC_MASK_BAD_ACCESS needed by all architectures for NULL ptr checking
  // EXC_MASK_ARITHMETIC needed by all architectures for div by 0 checking
  // EXC_MASK_BAD_INSTRUCTION needed by aarch64 to initiate deoptimization
  kern_return_t kr;
  kr = task_set_exception_ports(mach_task_self(),
EXC_MASK_BAD_ACCESS | EXC_MASK_ARITHMETIC
AARCH64_ONLY(| EXC_MASK_BAD_INSTRUCTION),
MACH_PORT_NULL,
EXCEPTION_STATE_IDENTITY,
MACHINE_THREAD_STATE);

  assert(kr == KERN_SUCCESS, "could not set mach task signal handler");
#endif

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 22:44:18 GMT, Gerard Ziemski  wrote:

>> Thanks for your questions Gerard.
>> 
>>> Part of the comment said This work-around is not necessary for 10.5+, as 
>>> CrashReporter no longer intercedes on caught fatal signals.
>> 
>> That comment can probably be deleted since minversion is anyway 10.9 (and 
>> soon 10.12 https://github.com/openjdk/jdk/pull/2268 ).
>> 
>>> Do you know if this also apply to lldb or is it gdb only specific? How do 
>>> you run gdb on macOS nowadays anyhow?
>> 
>> `lldb` is shipped with Xcode, `gdb` isn't. You would need to build and sign 
>> it yourself, I haven't tried that in a while. So, we should update that 
>> comment to talk about `lldb`  
>> 
>>> a) why we need `EXC_MASK_ARITHMETIC` ?
>> 
>> I _believe_ this dates back to i386. As far as I can tell this isn't needed 
>> for x86_64 or aarch64. I guess we can remove it, the worst case is that it 
>> makes the debugging experience of the runtime a little bit worse. OTOH it 
>> doesn't hurt either to have it here.
>> 
>>> b) we hit signal SIGSEGV in debugger even with the code in place, any way 
>>> to avoid that?
>> 
>> The equivalent for `handle SIGSEGV nostop noprint` (gdb) in lldb is `process 
>> handle -n false -p true -s false SIGSEGV`.
>> 
>>> c) does `BSD aarch6` need only `EXC_MASK_BAD_INSTRUCTION` or does it need 
>>> `EXC_MASK_BAD_ACCESS` as well?
>> 
>> aarch64 needs `EXC_MASK_BAD_ACCESS` at least for implicit null checking, 
>> there might be other cases.
>> 
>>> d) can we `#ifdef` the `EXC_MASK_BAD_INSTRUCTION` part of the mask only to 
>>> apply to `aarch64`?
>> 
>> Maybe. I don't see any value in it though, except making the code more 
>> complicated to read 
>
> I don't like the idea of using masks on architectures that do not require 
> them. How about something like this?
> 
> `#if defined(__APPLE__)`
> `  // lldb (gdb) installs both standard BSD signal handlers, and mach 
> exception`
> `  // handlers. By replacing the existing task exception handler, we disable 
> lldb's mach`
> `  // exception handling, while leaving the standard BSD signal handlers 
> functional.`
> `  //`
> `  // EXC_MASK_BAD_ACCESS needed by all architectures for NULL ptr checking`
> `  // EXC_MASK_ARITHMETIC needed by i386`
> `  // EXC_MASK_BAD_INSTRUCTION needed by aarch64 to initiate deoptimization`
> `  kern_return_t kr;`
> `  kr = task_set_exception_ports(mach_task_self(),`
> `EXC_MASK_BAD_ACCESS`
> `NOT_LP64(| EXC_MASK_ARITHMETIC)`
> `AARCH64_ONLY(| EXC_MASK_BAD_INSTRUCTION),`
> `MACH_PORT_NULL,`
> `EXCEPTION_STATE_IDENTITY,`
> `MACHINE_THREAD_STATE);`
> ` `
> `  assert(kr == KERN_SUCCESS, "could not set mach task signal handler");`
> `#endif`
> 
> If I just knew why i386 needs `EXC_MASK_ARITHMETIC` and add that to the 
> comment I would be personally happy with that chunk of code.

No idea how to insert spaces and make text align :-(

-

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

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

[Gerard Ziemski]

On Wed, 3 Feb 2021 20:04:18 GMT, Gerard Ziemski  wrote:

>> See comment above about `gdb`, the same applies to `lldb` today. The AArch64 
>> backend uses `SIGILL` (~= `EXC_MASK_BAD_INSTRUCTION`) to initiate a 
>> deoptimization. Without this change you cannot continue debugging once you 
>> the debuggee receives `SIGILL`. This wasn't needed before as x86 doesn't use 
>> `SIGILL`.
>
> Part of the comment said `This work-around is not necessary for 10.5+, as 
> CrashReporter no longer intercedes on caught fatal signals.` so I thought it 
> was no longer needed, but it sounds like the part about `gdb` still applies 
> then.
> 
> We should update the comment to just say the `gdb` relevant part perhaps (and 
> evaluate which of the EXC_MASK_BAD_ACCESS | EXC_MASK_BAD_INSTRUCTION | 
> EXC_MASK_ARITHMETIC) we actually need for gdb:
> 
>  `// gdb installs both standard BSD signal handlers, and mach exception`
>  `// handlers. By replacing the existing task exception handler, we disable 
> gdb's mach`
>  `// exception handling, while leaving the standard BSD signal handlers 
> functional.`
> 
> Do you know if this also apply to `lldb` or is it `gdb` only specific? How do 
> you run `gdb` on macOS nowadays anyhow?

To answer my own question, it seems that code is still needed on `x86_64` for 
`lldb` with `EXC_MASK_BAD_ACCESS` or we keep tripping over `EXC_BAD_ACCESS`

Remaining questions:

a) why we need `EXC_MASK_ARITHMETIC` ?
b) we hit `signal SIGSEGV` in debugger even with the code in place, any way to 
avoid that?
c) does `BSD aarch64` need only `EXC_MASK_BAD_INSTRUCTION` or does it need 
`EXC_MASK_BAD_ACCESS` as well?
d) can we `#ifdef` the `EXC_MASK_BAD_INSTRUCTION` part of the mask only to 
apply to `aarch64`?

-

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

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

[Gerard Ziemski]

On Tue, 2 Feb 2021 19:23:16 GMT, Bernhard Urban-Forster  
wrote:

>> src/hotspot/os/posix/signals_posix.cpp line 1297:
>> 
>>> 1295:   kern_return_t kr;
>>> 1296:   kr = task_set_exception_ports(mach_task_self(),
>>> 1297: EXC_MASK_BAD_ACCESS | 
>>> EXC_MASK_BAD_INSTRUCTION | EXC_MASK_ARITHMETIC,
>> 
>> Could someone elaborate on why we need to add `EXC_MASK_BAD_INSTRUCTION` to 
>> the mask here?
>
> See comment above about `gdb`, the same applies to `lldb` today. The AArch64 
> backend uses `SIGILL` (~= `EXC_MASK_BAD_INSTRUCTION`) to initiate a 
> deoptimization. Without this change you cannot continue debugging once you 
> the debuggee receives `SIGILL`. This wasn't needed before as x86 doesn't use 
> `SIGILL`.

Part of the comment said `This work-around is not necessary for 10.5+, as 
CrashReporter no longer intercedes on caught fatal signals.` so I thought it 
was no longer needed, but it sounds like the part about `gdb` still applies 
then.

We should update the comment to just say the `gdb` relevant part perhaps (and 
evaluate which of the EXC_MASK_BAD_ACCESS | EXC_MASK_BAD_INSTRUCTION | 
EXC_MASK_ARITHMETIC) we actually need for gdb:

 `// gdb installs both standard BSD signal handlers, and mach exception`
 `// handlers. By replacing the existing task exception handler, we disable 
gdb's mach`
 `// exception handling, while leaving the standard BSD signal handlers 
functional.`

Do you know if this also apply to `lldb` or is it `gdb` only specific? How do 
you run `gdb` on macOS nowadays anyhow?

-

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

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

[Gerard Ziemski]

On Tue, 2 Feb 2021 18:52:29 GMT, Gerard Ziemski  wrote:

>> Anton Kozlov has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   support macos_aarch64 in hsdis
>
> Changes requested by gziemski (Committer).

There were bunch of assembly code that I couldn't review. I also shallow 
reviewed the brand new files that were copies of the existing BSD x86_64 files 
- I hope I can get back to those when I figure out how to build/run these 
changes.

-

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

Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]

[Gerard Ziemski]

On Mon, 1 Feb 2021 20:10:58 GMT, Ioi Lam  wrote:

>> - JVM_GetInterfaceVersion() was used by "HotSpot Express" (HSX) which 
>> allowed the same JDK library to use different version of HotSpot. However, 
>> HSX is no longer supported so this API should be removed.
>> - Implementations of APIs such as JVM_DTraceActivate, were removed in 
>> [JDK-8068976](https://bugs.openjdk.java.net/browse/JDK-8068976), so their 
>> declarations should be removed from jvm.h
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   fixed macos build

Marked as reviewed by gziemski (Committer).

-

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

Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]

[Gerard Ziemski]

On Mon, 1 Feb 2021 20:54:42 GMT, Ioi Lam  wrote:

>> src/java.base/share/native/libjava/check_version.c line 33:
>> 
>>> 31: DEF_JNI_OnLoad(JavaVM *vm, void *reserved)
>>> 32: {
>>> 33: return JNI_VERSION_1_2;
>> 
>> This leaves an entire file with one trivial function implementation. Can we 
>> remove the file and implement  `DEF_JNI_OnLoad()` in `jni_util.h` (or some 
>> other existing suitable file) ?
>
> I am not sure if jni_utils.c is the right file (it defines the `JNU_XXX` 
> functions that are used by other shared libraries).
> 
> There are other .c files that have trivial `DEF_JNI_OnLoad` functions (e.g., 
> java.base/share/native/libnio/nio_util.c).
> 
> @AlanBateman do you have any suggestions?

I'm fine with the way it is, just thought we might want to consider cleaning up 
a bit more, since it's a cleanup task itself.

-

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

Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]

[Gerard Ziemski]

On Mon, 1 Feb 2021 20:10:58 GMT, Ioi Lam  wrote:

>> - JVM_GetInterfaceVersion() was used by "HotSpot Express" (HSX) which 
>> allowed the same JDK library to use different version of HotSpot. However, 
>> HSX is no longer supported so this API should be removed.
>> - Implementations of APIs such as JVM_DTraceActivate, were removed in 
>> [JDK-8068976](https://bugs.openjdk.java.net/browse/JDK-8068976), so their 
>> declarations should be removed from jvm.h
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   fixed macos build

Changes requested by gziemski (Committer).

src/java.base/share/native/libjava/check_version.c line 33:

> 31: DEF_JNI_OnLoad(JavaVM *vm, void *reserved)
> 32: {
> 33: return JNI_VERSION_1_2;

This leaves an entire file with one trivial function implementation. Can we 
remove the file and implement  `DEF_JNI_OnLoad()` in `jni_util.h` (or some 
other existing suitable file) ?

-

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

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[gerard ziemski]

Hi all,

Please review this revision 2 of a cleanup, where we remove 
JDK_GetVersionInfo0 and related code, since we can get build versions 
directly from within the VM itself.


The rev2 differs from rev1 in that it's simpler due to JDK-8234185, 
which was just checked in yesterday. Waiting for this fix to go in 
first, allowed us not to have to delete the jdk_util.h file, which in 
turn means that we don't have to touch all those files that used that 
include.



bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev2
tests: passes Mach5 tier1,2,3 (another tier1,2,3,4,5,6 in progress...)


cheers

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[gerard ziemski]

Thank you for the review.

I'm definitively going to wait for Christoph to check in his fix first.

I tried in fact to leave jdk_util.c/.h files in empty without removing 
them from the repo, and even though Mac/Linux were OK with that, 
Solaris/Windows were not.



cheers

On 11/13/19 9:21 PM, David Holmes wrote:

Hi Gerard,

On 14/11/2019 6:05 am, Langer, Christoph wrote:

Hi Gerard,

generally it looks like a nice cleanup.

I've got a patch prepared though, which I was planning on posting 
tomorrow. It is about cleanup for the canonicalize function in 
libjava. I wanted to use jdk_util.h for the function prototype. I had 
not yet filed a bug but here is what I have:

http://cr.openjdk.java.net/~clanger/webrevs/cleanup-canonicalize/

So maybe you could refrain from removing jdk_util.h or maybe you can 
hold off submitting your change until my cleanup is reviewed?


I'd also suggest not deleting jdk_util.h. It seems very odd to me to 
have jdk_util_md.h with no shared jdk_util.h. If you keep jdk_util.h 
then you don't need to touch a number of the files.


Otherwise this looks like a good cleanup.

Thanks,
David
-


I'll create a bug and post an official review thread tomorrow...

Thanks
Christoph


-Original Message-
From: hotspot-dev  On Behalf Of
Mandy Chung
Sent: Mittwoch, 13. November 2019 20:30
To: gerard ziemski 
Cc: awt-...@openjdk.java.net; hotspot-dev developers ; core-libs-dev@openjdk.java.net
Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove
JDK_GetVersionInfo0 and the supporting code"



On 11/13/19 10:50 AM, gerard ziemski wrote:

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and
related code, since we can get build versions directly from within the
VM itself:

I'm including core-libs and awt in this review because the proposed
fix touches their corresponding files.


bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6



This is a good clean up.  JDK_GetVersionInfo0 was needed long time ago
in particular in HS express support that is no longer applicable.

One leftover comment should also be removed.

src/hotspot/share/runtime/vm_version.hpp
    // Gets the jvm_version_info.jvm_version defined in jvm.h

otherwise looks good.

Mandy

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[gerard ziemski]

Thank you for the review.

I'm happy to wait for your fix to go in first - this will make my fix 
smaller.



cheers

On 11/13/19 2:05 PM, Langer, Christoph wrote:

Hi Gerard,

generally it looks like a nice cleanup.

I've got a patch prepared though, which I was planning on posting tomorrow. It 
is about cleanup for the canonicalize function in libjava. I wanted to use 
jdk_util.h for the function prototype. I had not yet filed a bug but here is 
what I have:
http://cr.openjdk.java.net/~clanger/webrevs/cleanup-canonicalize/

So maybe you could refrain from removing jdk_util.h or maybe you can hold off 
submitting your change until my cleanup is reviewed?

I'll create a bug and post an official review thread tomorrow...

Thanks
Christoph


-Original Message-
From: hotspot-dev  On Behalf Of
Mandy Chung
Sent: Mittwoch, 13. November 2019 20:30
To: gerard ziemski 
Cc: awt-...@openjdk.java.net; hotspot-dev developers ; core-libs-dev@openjdk.java.net
Subject: Re: RFR (M) 8223261 "JDK-8189208 followup - remove
JDK_GetVersionInfo0 and the supporting code"



On 11/13/19 10:50 AM, gerard ziemski wrote:

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and
related code, since we can get build versions directly from within the
VM itself:

I'm including core-libs and awt in this review because the proposed
fix touches their corresponding files.


bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6


This is a good clean up.  JDK_GetVersionInfo0 was needed long time ago
in particular in HS express support that is no longer applicable.

One leftover comment should also be removed.

src/hotspot/share/runtime/vm_version.hpp
    // Gets the jvm_version_info.jvm_version defined in jvm.h

otherwise looks good.

Mandy

Re: RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[gerard ziemski]

Thank you for the review, will remove the comment and updated the 
webrev, but only after Christop Langer gets his fix in - I'm going to 
wait for him to check in first.



cheers

On 11/13/19 1:29 PM, Mandy Chung wrote:



On 11/13/19 10:50 AM, gerard ziemski wrote:

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and 
related code, since we can get build versions directly from within 
the VM itself:


I'm including core-libs and awt in this review because the proposed 
fix touches their corresponding files.



bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6



This is a good clean up.  JDK_GetVersionInfo0 was needed long time ago 
in particular in HS express support that is no longer applicable.


One leftover comment should also be removed.

src/hotspot/share/runtime/vm_version.hpp
  // Gets the jvm_version_info.jvm_version defined in jvm.h

otherwise looks good.

Mandy

RFR (M) 8223261 "JDK-8189208 followup - remove JDK_GetVersionInfo0 and the supporting code"

[gerard ziemski]

Hi all,

Please review this cleanup, where we remove JDK_GetVersionInfo0 and 
related code, since we can get build versions directly from within the 
VM itself:


I'm including core-libs and awt in this review because the proposed fix 
touches their corresponding files.



bug: https://bugs.openjdk.java.net/browse/JDK-8223261
webrev: http://cr.openjdk.java.net/~gziemski/8223261_rev1
tests: passes Mach5 tier1,2,3,4,5,6


cheers

Re: RFR 8173715: Remove Flat Profiler

[Gerard Ziemski]

Ping.

> On Aug 24, 2017, at 2:59 AM, David Holmes <david.hol...@oracle.com> wrote:
> 
> On 24/08/2017 2:20 AM, Gerard Ziemski wrote:
>> Thank you David.
>> Do I need a second reviewer here?
> 
> Yes any launcher changes should be signed off by launcher folk - so I've 
> directly cc'd Kumar, who is hopefully around :)
> 
> David
> 
>> cheers
>>  
>>> On Aug 22, 2017, at 9:15 PM, David Holmes <david.hol...@oracle.com> wrote:
>>> 
>>> Hi Gerard,
>>> 
>>> On 23/08/2017 3:41 AM, Gerard Ziemski wrote:
>>>> hi all,
>>>> The FlatProfiler (i.e. -Xprof) has been deprecated in jdk9, and now it’s 
>>>> the time to remove the code and obsolete the -Xprof flag.
>>>> issue:  https://bugs.openjdk.java.net/browse/JDK-8173715
>>>> webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev2_jdk/
>>> 
>>> These changes seem okay to me - though I believe the man page files are 
>>> slated for removal anyway.
>>> 
>>> Thanks,
>>> David

Re: RFR 8173715: Remove Flat Profiler

[Gerard Ziemski]

Thank you David.

Do I need a second reviewer here?


cheers
 
> On Aug 22, 2017, at 9:15 PM, David Holmes <david.hol...@oracle.com> wrote:
> 
> Hi Gerard,
> 
> On 23/08/2017 3:41 AM, Gerard Ziemski wrote:
>> hi all,
>> The FlatProfiler (i.e. -Xprof) has been deprecated in jdk9, and now it’s the 
>> time to remove the code and obsolete the -Xprof flag.
>> issue:  https://bugs.openjdk.java.net/browse/JDK-8173715
>> webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev2_jdk/
> 
> These changes seem okay to me - though I believe the man page files are 
> slated for removal anyway.
> 
> Thanks,
> David

RFR 8173715: Remove Flat Profiler

[Gerard Ziemski]

hi all,

The FlatProfiler (i.e. -Xprof) has been deprecated in jdk9, and now it’s the 
time to remove the code and obsolete the -Xprof flag.

issue:  https://bugs.openjdk.java.net/browse/JDK-8173715
webrev: http://cr.openjdk.java.net/~gziemski/8173715_rev2_jdk/


cheers

Re: RFR 8165634: Support multiple --add-module options on the command line

[Gerard Ziemski]

hi Harold,

The changes look fine. I have a couple of questions though:

#1 Would the "test/runtime/modules/ModuleOptionsTest.java” be more robust if we 
included a case with a non-error return value that takes more than one module? 
Ex: “java --add-modules=java.base 
--add-modules=jdk.internal.reflect=ALL-UNNAMED -version”

#2 Looking at the changes for jdk it appears we now also allow duplicate 
"--add-exports” and "--add-reads"? Does it also mean we allow duplicate 
"--add-modules”, Ex: “java --add-modules=java.base --add-modules=java.base 
-version”?


cheers


> On Sep 8, 2016, at 8:23 AM, harold seigel  wrote:
> 
> Hi,
> 
> Please review this fix for JDK-8165634.  The fix changes the --add-modules 
> option from being a 'last one wins' option to a cumulative one.  With this 
> change, if multiple --add-modules options are specified, the VM accumulates 
> all the options' values, instead of ignoring all but the last option's value. 
>  The --add-modules values are reported back to the JDK as properties using 
> the Arguments::create_numbered_property() function.
> 
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8165634
> 
> Open webrevs:
> 
>   http://cr.openjdk.java.net/~hseigel/bug_8165634.hs/
> 
>   http://cr.openjdk.java.net/~hseigel/bug_8165634.jdk/
> 
> The fix was tested with the JCK Lang and VM tests, the hotpot, and java/lang, 
> java/util and other JTreg tests, the RBT tier2 tests, and the NSK 
> non-colocated quick tests.
> 
> The JDK changes were done by Mandy Chung (mchung).
> 
> Thanks, Harold
> 
> 

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

[Gerard Ziemski]

> On Jun 29, 2016, at 6:47 PM, Mandy Chung  wrote:
> 
>> 
>> On Jun 29, 2016, at 12:36 PM, Brent Christian  
>> wrote:
>> 
>> Hi,
>> 
>> Please review the following change for JDK 9:
>> 
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8160370
>> Webrev:
>> http://cr.openjdk.java.net/~bchristi/8160370/webrev.00/
>> 
>> The fix for 7131356 fills in the "os.version" system property on Mac using 
>> [NSProcessInfo operatingSystemVersion], available starting in Mac OS 10.9.
>> 
>> While JDK 9 will not support versions of Mac OS prior to 10.9 [1], not all 
>> testing infrastructure has been updated, and we've seen failures of
>> 
>> java/lang/System/OsVersionTest.java
>> 
>> The code to restore behavior on older Mac systems is only a few lines, so 
>> that seems like a good way to get testing going again.
> 
> I think we should move away from testing on unsupported platforms.  Is this 
> just temporary?  when will this be removed?

I think we should leave the workaround code in forever - it will never execute 
on newer/supported OS X and will do the right thing on earlier ones.


cheers

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

[Gerard Ziemski]

hi Brent,

Thank you for fixing the original issue and for putting in this follow-up fix!

Looks good! (for full disclosure I proposed the workaround)


cheers

> On Jun 29, 2016, at 2:36 PM, Brent Christian  
> wrote:
> 
> Hi,
> 
> Please review the following change for JDK 9:
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8160370
> Webrev:
> http://cr.openjdk.java.net/~bchristi/8160370/webrev.00/
> 
> The fix for 7131356 fills in the "os.version" system property on Mac using 
> [NSProcessInfo operatingSystemVersion], available starting in Mac OS 10.9.
> 
> While JDK 9 will not support versions of Mac OS prior to 10.9 [1], not all 
> testing infrastructure has been updated, and we've seen failures of
> 
> java/lang/System/OsVersionTest.java
> 
> The code to restore behavior on older Mac systems is only a few lines, so 
> that seems like a good way to get testing going again.
> 
> Thanks,
> -Brent
> 
> 1. https://jdk9.java.net/jdk9_supported_platforms.html
> 2. https://bugs.openjdk.java.net/browse/JDK-8156132

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

[Gerard Ziemski]

hi Brent,

Thank you very much for tackling this. The code overall looks good (aside from 
the 3 code language sub tag case that we need to handle, as Naoto pointed out)

Here are my 2 small comments:

#1

 63 CFRelease(languages); // was missing from original patch

I think we can drop the comment from line 63?


#2

131 void setOSNameAndVersion(java_props_t *sprops) {
132 /* Don't rely on JRSCopyOSName because there's no guarantee the value 
will
133  * remain the same, or even if the JRS functions will continue to be 
part of
134  * Mac OS X.  So hardcode os_name, and fill in os_version if we can.
135  */

The comment from lines 132-135 should be dropped - we no longer use JSR - and 
just say that we're hardcoding the OS name?


cheers

> On Jun 13, 2016, at 3:58 PM, Brent Christian <brent.christ...@oracle.com> 
> wrote:
> 
> Hi,
> 
> Please review this Mac-only fix:
> 
> http://cr.openjdk.java.net/~bchristi/7131356/webrev.01/
> https://bugs.openjdk.java.net/browse/JDK-7131356
> 
> Thanks go to Gerard Ziemski for the thorough investigation and detailed 
> write-up in the bug report.
> 
> The symptoms:
> 
> On those OS X machines where the default system Java is not installed, any 
> attempt to instantiate JVM from a local JDK (for example via JNI as shown in 
> the bug) presents "No Java installed. Do you want to install Java?" dialog 
> and refuses to run. Clearly, a valid local JDK should be able to run in this 
> case.
> 
> The problem:
> 
> In java_props_macosx.c, there is code that dynamically looks up 4 methods in 
> JavaRuntimeSupport.framework (JRS) and calls into them to find out localized 
> OS version, default locale and the preferred language, but JRS checks for a 
> system available Java and refuses to run if none found.
> 
> Background:
> 
> JavaRuntimeSupport.framework (JRS) is a framework implemented and provided by 
> Apple for use by Java.  It is a "black box" that wraps SPIs required by the 
> Apple-provided implementation of JDK, exposing them to the JDK as APIs.
> 
> Now that the JDK implementation ownership has changed from Apple to Oracle, 
> we have the issue that we don't control the JRS implementation, but rely on 
> Apple to support it for our JDK to continue to run. Depending on a private 
> framework provided by a third party for our code to continue to run doesn't 
> seem like a good long term bet (see also 8024281).
> 
> Proposed solution:
> 
> Apple suggested changing the JDK initialization order so any access to JRS 
> happens only after the JLI_MemAlloc symbol is available.  We believe a better 
> solution is to re-implement the JSR APIs in question, as a move toward 
> eventually dropping reliance on JSR altogether.  The three main changes are:
> 
> 1. In "getMacOSXLocale", re-implement "JRSCopyPrimaryLanguage" using 
> "CFLocaleCopyPreferredLanguages", which gives us the preferred language as 
> set in "System Preferences"->"Language & Text"->"Language" in the form of 
> "xx" (ie. just the language code - ex. "en", "fr", etc.).  The original Apple 
> code then calls into "JRSCopyCanonicalLanguageForPrimaryLanguage" to map 
> "language" into "language_REGION" (ex. "en"-->"en_US", "fr"-->"fr_FR", etc.), 
> though this appears to be unnecessary.  Following the call to 
> setupMacOSXLocale() in ParseLocale()[1], mapLookup() is called to map the 
> language to a default country, per this comment:
> 
> * If the locale name (without .encoding@variant, if any) matches
> * any of the names in the locale_aliases list, map it to the
> * corresponding full locale name.  Most of the entries in the
> * locale_aliases list are locales that include a language name but
> * no country name, and this facility is used to map each language
> * to a default country if that's possible.
> 
> I tried out a few locales, for English (en_US, en_GB), German (de_DE, de_CH) 
> and French (fr_FR, fr_CA).  Language/country system properties behave the 
> same before and after this change, as does the java.util.Locale test attached 
> to the bug.
> 
> 2. In "setupMacOSXLocale" we simply drop the call to 
> "JRSSetDefaultLocalization" as it appears to be a NOP. According to Apple, 
> that API sets up native bundle locale, so that any access to native Cocoa UI 
> (like FileOpenChooser) uses localized strings. Testing shows that this does 
> not seem to work even in Apple's own JDK (ie. JDK 6), so dropping the call to 
> this SPI here does not result in a regression.  An issue was filed to 
> investigate further (8024279, a dup of 8019464) which has since been closed 
> as, "Not an Issue".
> 
> 3. In "setOSNameAndVersion", re-implement JRSCopyOSVersion using 
> [NSProcessInfo operatingSystemVersion].  (Use of JRSCopyOSName was already 
> removed by 7178922).
> 
> 
> Thanks,
> -Brent
> 
> 1. 
> http://hg.openjdk.java.net/jdk9/dev/jdk/file/79043a1c3547/src/java.base/unix/native/libjava/java_props_md.c
> 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Gerard Ziemski]

> On Apr 19, 2016, at 12:04 AM, David Holmes <david.hol...@oracle.com> wrote:
> 
> On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:
>> Hi Gerard,
>> 
>> 2016/04/19 3:14 "Gerard Ziemski" <gerard.ziem...@oracle.com
>> <mailto:gerard.ziem...@oracle.com>>:
>> >
>> > hi Yasumasa,
>> >
>> > Nice work. I have 2 questions:
>> >
>> > 
>> > File: java.c
>> >
>> > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
>> after every single JNI call? In this example instead of NULL_CHECK,
>> should we be using CHECK_EXCEPTION_NULL_LEAVE macro?
>> 
>> It is not critical if we encounter error at JNI function call  because
>> we cannot set native thread name only.
>> So I think that we do not need to leave from launcher process.
> 
> I agree we do not need to abort if an exception occurs (and in fact I don't 
> think an exception is even possible from this code), but we should ensure any 
> pending exception is cleared before any futher JNI calls might be made. Note 
> that NULL_CHECK is already used extensively throughout the launcher code - so 
> if this usage is wrong then it is all wrong! More on this code below ...

Isn’t there a risk that:

(*env)->CallVoidMethod(env, currentThread, setNativeNameID,
+   nameString, JNI_TRUE);

will raise an exception?

In the least, shouldn’t we clear any possible pending exceptions at the 
beginning of “SetNativeThreadName“, as you say, but also at the very end?


cheers

Re: RFR: JDK-8152690: main thread does not have native thread name

[Gerard Ziemski]

hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)” after every 
single JNI call? In this example instead of NULL_CHECK, should we be using 
CHECK_EXCEPTION_NULL_LEAVE macro?

#2 Should the comment for “SetNativeThreadName” be “Set native thread name if 
possible.” not "Set native thread name as possible.”?


cheers

> On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga  wrote:
> 
> Hi David,
> 
> I uploaded new webrev:
> 
> - hotspot:
>http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
> 
> - jdk:
>http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
> 
> 
>> it won't work unless you change the semantics of setName so I'm not sure 
>> what you were thinking here. To take advantage of an arg taking 
>> JVM_SetNativThreadName you would need to call it directly as no Java code 
>> will call it . ???
> 
> I added a flag for setting native thread name to JNI-attached thread.
> This change can set native thread name if main thread changes to JNI-attached 
> thread.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2016/04/16 16:11, David Holmes wrote:
>> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
>>> Hi David,
>>> 
 That change in behaviour may be a problem, it could be considered a
 regression that setName stops setting the native thread main, even
 though we never really intended it to work in the first place. :( Such
 a change needs to go through CCC.
>>> 
>>> I understood.
>>> Can I send CCC request?
>>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
>> 
>> Sorry you can't file a CCC request, you would need a sponsor for that. But 
>> at this stage I don't think I agree with the proposed change because of the 
>> change in behaviour - there's no way to restore the "broken" behaviour.
>> 
>>> I want to continue to discuss about it on JDK-8154331 [1].
>> 
>> Okay we can do that.
>> 
>>> 
 Further, we expect the launcher to use the supported JNI interface (as
 other processes would), not the internal JVM interface that exists for
 the JDK sources to communicate with the JVM.
>>> 
>>> I think that we do not use JVM interface if we add new method in
>>> LauncherHelper as below:
>>> 
>>> diff -r f02139a1ac84
>>> src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> --- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Wed Apr 13 14:19:30 2016 +
>>> +++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Sat Apr 16 11:25:53 2016 +0900
>>> @@ -960,4 +960,8 @@
>>>  else
>>>  return md.toNameAndVersion() + " (" + loc + ")";
>>>  }
>>> +
>>> +static void setNativeThreadName(String name) {
>>> +Thread.currentThread().setName(name);
>>> +}
>> 
>> You could also make that call via JNI directly, so not sure the helper adds 
>> much here. But it won't work unless you change the semantics of setName so 
>> I'm not sure what you were thinking here. To take advantage of an arg taking 
>> JVM_SetNativThreadName you would need to call it directly as no Java code 
>> will call it . ???
>> 
>> David
>> -
>> 
>>>  }
>>> diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
>>> --- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
>>> 2016 +
>>> +++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
>>> 2016 +0900
>>> @@ -125,6 +125,7 @@
>>>  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
>>>  static void ShowSettings(JNIEnv* env, char *optString);
>>>  static void ListModules(JNIEnv* env, char *optString);
>>> +static void SetNativeThreadName(JNIEnv* env, char *name);
>>> 
>>>  static void SetPaths(int argc, char **argv);
>>> 
>>> @@ -325,6 +326,7 @@
>>>   * mainThread.isAlive() to work as expected.
>>>   */
>>>  #define LEAVE() \
>>> +SetNativeThreadName(env, "DestroyJavaVM"); \
>>>  do { \
>>>  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
>>>  JLI_ReportErrorMessage(JVM_ERROR2); \
>>> @@ -488,6 +490,9 @@
>>>  mainArgs = CreateApplicationArgs(env, argv, argc);
>>>  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
>>> 
>>> +/* Set native thread name. */
>>> +SetNativeThreadName(env, "main");
>>> +
>>>  /* Invoke main method. */
>>>  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
>>> 
>>> @@ -1686,6 +1691,22 @@
>>>   joptString);
>>>  }
>>> 
>>> +/**
>>> + * Set native thread name as possible.
>>> + */
>>> +static void
>>> +SetNativeThreadName(JNIEnv *env, char *name)
>>> +{
>>> +jmethodID setNativeThreadNameID;
>>> +jstring nameString;
>>> +jclass cls = GetLauncherHelperClass(env);
>>> +NULL_CHECK(cls);
>>> +NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
>>> +"setNativeThreadName", "(Ljava/lang/String;)V"));
>>> +NULL_CHECK(nameString = (*env)->NewStringUTF(env, 

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Gerard Ziemski]

hi Roger,

I love that we are adding this Signal object. I have several questions, but 
please do not take them as criticism, I’m just seeking clarification and 
providing feedback:


#1 Re: "Consumers for signals that are unknown or reserved by the Java 
implementation can not be registered.”

- Why don't we want to allow unknown signals? This will make us have to update 
our implementation if we want to support new or platform specific signals in 
the future. 


#2 Re: "java.util.Signal.raise()”

- That API raises the signal in the current process, but what about sending a 
signal to another process for interprocess communication?


#3 Re: "Signal.of("SIGINT”)”

- Is this a factory method that returns the same object if called more than 
once?


#4 Re: "public boolean unregister(Consumer consumer)”

- Why is this API returning a value? Wouldn’t having a Signal API like “public 
Consumer getConsumer()” be more flexible?


#5 Re: "public void registerDefault(Consumer consumer)”

- Do we really need this API? Can’t the same be achieved with the plain vanilla 
“public void register(Consumer consumer)” I guess I don’t really see 
what makes this API special.


cheers

> On Feb 1, 2016, at 10:02 AM, Roger Riggs  wrote:
> 
> Please review an API addition to handle signals such as SIGINT, SIGHUP, and 
> SIGTERM.
> This JEP 260 motivated alternative to sun.misc.Signal supports the use case 
> for
> interactive applications that need to handle Control-C and other signals.
> 
> The new java.util.Signal class provides a settable primary signal handler and 
> a default
> signal handler.  The primary signal handler can be unregistered and handling 
> is restored
> to the default signal handler.  System initialization registers default 
> signal handlers
> to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API requires
> a permission if a SecurityManager is set.
> 
> The sun.misc.Signal implementation is modified to be layered on a common
> thread and dispatch mechanism. The VM handling of native signals is not 
> affected.
> The command option to reduce signal use by the runtime with -Xrs is 
> unmodified.
> 
> The changes to hotspot are minimal to rename the hardcoded callback to the 
> Java
> Signal dispatcher.
> 
> Please review and comment on the API and implementation.
> 
> javadoc:
>  http://cr.openjdk.java.net/~rriggs/signal-doc/
> 
> Webrev:
> jdk:  http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
> hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/
> 
> Issue:
>   https://bugs.openjdk.java.net/browse/JDK-8087286
> 
> JEP 260:
>  https://bugs.openjdk.java.net/browse/JDK-8132928
> 
> Thanks, Roger
> 
> 

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Gerard Ziemski]

> On Feb 1, 2016, at 1:25 PM, Roger Riggs <roger.ri...@oracle.com> wrote:
> 
> Hi Gerard,
> 
> On 2/1/2016 1:58 PM, Gerard Ziemski wrote:
>> hi Roger,
>> 
>> I love that we are adding this Signal object. I have several questions, but 
>> please do not take them as criticism, I’m just seeking clarification and 
>> providing feedback:
>> 
>> 
>> #1 Re: "Consumers for signals that are unknown or reserved by the Java 
>> implementation can not be registered.”
>> 
>> - Why don't we want to allow unknown signals? This will make us have to 
>> update our implementation if we want to support new or platform specific 
>> signals in the future. 
>> 
>> 
> The statement was aimed primarily at the Java Signal API; there is quite a 
> bit of detail
> oriented code in the VM to initialize and handle signals. Most of it is 
> agnostic to the signal number
> and would just pass it through.  If a signal is not supported by the OS 
> (think SIGHUP on Windows)
> that should bubble up as being not available.  The 'cannot be registered' 
> might be re-worded to say
> it throws an exception, as the method javadoc does.
> 
> The set of signals is a pretty slow moving target so updating implementations 
> should not be a big issue.

Right, but you don’t actually answer why we don’t allow unknown (to us at the 
moment) signals. Why have a limit in place, unless there is a good reason?


>> #2 Re: "java.util.Signal.raise()”
>> 
>> - That API raises the signal in the current process, but what about sending 
>> a signal to another process for interprocess communication?
>> 
> I left that for a separate issue but would be a straight-forward addition to 
> java.lang.ProcessHandle/Process.

The proposed Signal “feels” incomplete to me without this, though I understand 
that it meets the original goal. I would love to see at the very least a 
followup enhancement filed to address this.


>> 
>> 
>> #3 Re: "Signal.of("SIGINT”)”
>> 
>> - Is this a factory method that returns the same object if called more than 
>> once?
>> 
> Maybe, maybe not, why would it matter.  
> The real state is encapsulate is in the SignalImpl instances which are 
> singletons per signal.
> I was trying to keep the Signal object stateless to allow it to be a 
> value-type and lighter weight
> some day.

If it doesn’t matter, the why not just use constructor “Signal signal = new 
Signal(“SIGINT”)” ?


>> 
>> 
>> #4 Re: "public boolean unregister(Consumer consumer)”
>> 
>> - Why is this API returning a value? Wouldn’t having a Signal API like 
>> “public Consumer getConsumer()” be more flexible?
>> 
> 
> The return value reports whether it unregistered the specific consumer.  If 
> it was not
> the concurrently registered the caller might want to know it was currently 
> registered.
> I expect the return would mostly be ignored. 
> 
> The getConsumer()/unregister consumer pair would be vulnerable to race 
> conditions
> and require some external locking to get predictable behavior.  

Isn’t it also true for register/unregister?

> 
>> 
>> 
>> #5 Re: "public void registerDefault(Consumer consumer)”
>> 
>> - Do we really need this API? Can’t the same be achieved with the plain 
>> vanilla “public void register(Consumer consumer)” I guess I don’t 
>> really see what makes this API special.
>> 
> The java runtime currently registers termination handler to cleanly shutdown 
> if someone types control-c.
> It is useful to be able to remove the application provided signal handler and 
> be able to restore the
> system defaults.
> This API could be hidden as a pure implementation detail.  So unregistering 
> the signal handler
> would always restore the appropriate system ones.

I was hoping that behavior is all that’s needed.


> 
> Thanks for the review and comments, Roger

Thanks for all the work!


> 
>> 
>> 
>> cheers
>> 
>> 
>>> On Feb 1, 2016, at 10:02 AM, Roger Riggs <roger.ri...@oracle.com>
>>>  wrote:
>>> 
>>> Please review an API addition to handle signals such as SIGINT, SIGHUP, and 
>>> SIGTERM.
>>> This JEP 260 motivated alternative to sun.misc.Signal supports the use case 
>>> for
>>> interactive applications that need to handle Control-C and other signals.
>>> 
>>> The new java.util.Signal class provides a settable primary signal handler 
>>> and a default
>>> signal handler.  The primary signal handler can be unregistered and 
>>> handling is restored
>>> to the