Re: RFR: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4
On Thu, 4 Feb 2021 02:32:31 GMT, Phil Race wrote: > remove un-needed disabling now JNF has gone .. Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2396
RFR: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4
remove un-needed disabling now JNF has gone .. - Commit messages: - 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4 Changes: https://git.openjdk.java.net/jdk/pull/2396/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2396&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261109 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2396/head:pull/2396 PR: https://git.openjdk.java.net/jdk/pull/2396
Integrated: Merge jdk16
On Thu, 4 Feb 2021 01:17:48 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 16 -> JDK 17 This pull request has now been integrated. Changeset: 9b7a8f19 Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/9b7a8f19 Stats: 2645 lines in 56 files changed: 2497 ins; 69 del; 79 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/2392
Integrated: 8260616: Removing remaining JNF dependencies in the java.desktop module
On Fri, 29 Jan 2021 00:30:21 GMT, Phil Race wrote: > This completes the desktop module JNF removal > > * remove -framework JavaNativeFoundation from make files > > * remove #import from all > source files. If needed add import of JNIUtilities.h to get jni.h definitions > - better anyway since then it gets the current JDK ones not the ones from the > O/S > > * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString > with JavaStringToNSString > > * replace JNFNormalizedNSStringForPath with > NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with > NormalizedPathJavaStringFromNSString > > * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI > > * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast > majority already did this) > > * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject > perform* methods. > > * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and > use where needed. > > * Remove the single usage of JNFPerformEnvBlock > > * replace JNFJavaToNSNumber in single A11Y file with local replacement > > * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with > local replacement > > * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m > > * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) This pull request has now been integrated. Changeset: 8760688d Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/8760688d Stats: 580 lines in 74 files changed: 243 ins; 99 del; 238 mod 8260616: Removing remaining JNF dependencies in the java.desktop module Reviewed-by: gziemski, ihse, serb - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v5]
> This completes the desktop module JNF removal > > * remove -framework JavaNativeFoundation from make files > > * remove #import from all > source files. If needed add import of JNIUtilities.h to get jni.h definitions > - better anyway since then it gets the current JDK ones not the ones from the > O/S > > * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString > with JavaStringToNSString > > * replace JNFNormalizedNSStringForPath with > NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with > NormalizedPathJavaStringFromNSString > > * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI > > * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast > majority already did this) > > * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject > perform* methods. > > * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and > use where needed. > > * Remove the single usage of JNFPerformEnvBlock > > * replace JNFJavaToNSNumber in single A11Y file with local replacement > > * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with > local replacement > > * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m > > * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) Phil Race 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 seven additional commits since the last revision: - Merge branch 'master' into jnf_string - 8260616: Removing remaining JNF dependencies in the java.desktop module - 8260616: Removing remaining JNF dependencies in the java.desktop module - Merge branch 'master' into jnf_string - 8260616: Removing remaining JNF dependencies in the java.desktop module - 8260616: Removing remaining JNF dependencies in the java.desktop modul - 8260616: Removing remaining JNF dependencies in the java.desktop module - Changes: - all: https://git.openjdk.java.net/jdk/pull/2305/files - new: https://git.openjdk.java.net/jdk/pull/2305/files/8a014fa6..43880e5f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2305&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2305&range=03-04 Stats: 3151 lines in 249 files changed: 1263 ins; 952 del; 936 mod Patch: https://git.openjdk.java.net/jdk/pull/2305.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2305/head:pull/2305 PR: https://git.openjdk.java.net/jdk/pull/2305
RFR: Merge jdk16
Forwardport JDK 16 -> JDK 17 - Commit messages: - Merge - 8259794: Remove EA from JDK 16 version string starting with Initial RC promotion on Feb 04, 2021(B35) - 8260704: ParallelGC: oldgen expansion needs release-store for _end - 8260927: StringBuilder::insert is incorrect without Compact Strings - 8258378: Final nroff manpage update for JDK 16 - 8257215: JFR: Events dropped when streaming over a chunk rotation - 8260473: [vector] ZGC: VectorReshape test produces incorrect results with ZGC enabled - 8260632: Build failures after JDK-8253353 - 8260339: JVM crashes when executing PhaseIdealLoop::match_fill_loop - 8260608: add a regression test for 8260370 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/f025bc1d...dad835ee The webrevs contain the adjustments done while merging with regards to each parent branch: - master: https://webrevs.openjdk.java.net/?repo=jdk&pr=2392&range=00.0 - jdk16: https://webrevs.openjdk.java.net/?repo=jdk&pr=2392&range=00.1 Changes: https://git.openjdk.java.net/jdk/pull/2392/files Stats: 2645 lines in 56 files changed: 2497 ins; 69 del; 79 mod Patch: https://git.openjdk.java.net/jdk/pull/2392.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2392/head:pull/2392 PR: https://git.openjdk.java.net/jdk/pull/2392
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v4]
On Tue, 2 Feb 2021 23:22:12 GMT, Phil Race wrote: >> This completes the desktop module JNF removal >> >> * remove -framework JavaNativeFoundation from make files >> >> * remove #import from all >> source files. If needed add import of JNIUtilities.h to get jni.h >> definitions - better anyway since then it gets the current JDK ones not the >> ones from the O/S >> >> * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString >> with JavaStringToNSString >> >> * replace JNFNormalizedNSStringForPath with >> NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with >> NormalizedPathJavaStringFromNSString >> >> * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI >> >> * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the >> vast majority already did this) >> >> * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject >> perform* methods. >> >> * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and >> use where needed. >> >> * Remove the single usage of JNFPerformEnvBlock >> >> * replace JNFJavaToNSNumber in single A11Y file with local replacement >> >> * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with >> local replacement >> >> * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m >> >> * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) > > Phil Race 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 six additional commits since > the last revision: > > - 8260616: Removing remaining JNF dependencies in the java.desktop module > - 8260616: Removing remaining JNF dependencies in the java.desktop module > - Merge branch 'master' into jnf_string > - 8260616: Removing remaining JNF dependencies in the java.desktop module > - 8260616: Removing remaining JNF dependencies in the java.desktop modul > - 8260616: Removing remaining JNF dependencies in the java.desktop module Marked as reviewed by serb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
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]
On Wed, 3 Feb 2021 22:48:33 GMT, Gerard Ziemski wrote: >> 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 :-( 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? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
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]
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]
On Wed, 3 Feb 2021 20:29:48 GMT, Gerard Ziemski wrote: >> 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`? 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 🙂 - PR: https://git.openjdk.java.net/jdk/pull/2200
Integrated: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Thu, 28 Jan 2021 22:40:57 GMT, Phil Race wrote: > This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? This pull request has now been integrated. Changeset: 2be60e37 Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/2be60e37 Stats: 45 lines in 2 files changed: 35 ins; 2 del; 8 mod 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m Reviewed-by: ihse, cjplummer - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
On Wed, 3 Feb 2021 20:17:03 GMT, Phil Race wrote: >> This removes the JNF dependency from the jdk.hotspot.agent module. >> The macro expansions are the same as already used in the Java desktop module >> - which actually uses a macro >> still but there there are hundreds of uses. >> The function of this macro code is to prevent NSExceptions escaping to Java >> and also to drain the auto-release pool. >> What test group would be good for verifying this change ? > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
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]
On Wed, 3 Feb 2021 09:14:24 GMT, Andrew Haley wrote:
>> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5271:
>>
>>> 5269: //
>>> 5270: void MacroAssembler::get_thread(Register dst) {
>>> 5271: RegSet saved_regs = RegSet::range(r0, r1) +
>>> BSD_ONLY(RegSet::range(r2, r17)) + lr - dst;
>>
>> The comment needs to be updated, since on BSD we also seem to clobber r2,r17
>> ?
>
> Surely this should be
>
> saved_regs = RegSet::range(r0, r1) BSD_ONLY(+ RegSet::range(r2, r17)) + lr -
> dst;```
>
> Shouldn't it?
Interesting, I wonder why it has built successfully on Linux. I'm going to fix
this under as JDK-8261072
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
On Tue, 2 Feb 2021 20:27:17 GMT, Chris Plummer wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
>
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 294:
>
>> 292:
>> 293: NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
>> 294: @try {
>
> Although there are only 3 places where the `JNF_COCOA_ENTER/EXIT` macros are
> used, it seems it would still be worth taking the same approach you did for
> `java.desktop` and add the replacement macros instead of inlining them. So
> just copy what you added to
> `src/java.desktop/macosx/native/libosxapp/JNIUtilities.h`, and replace
> `JNF_COCOA_ENTER` with `JNI_COCOA_ENTER/EXIT`. Otherwise the
> `JNF_COCOA_ENTER/EXIT` changes look fine to me, but I'm just basing this on a
> comparison with what you've done with `java.desktop`. I'm no expert in this
> area.
OK .. I don't really mind either way and if this helps gets it pushed .. so
I've updated.
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 296:
>
>> 294: @try {
>> 295:
>> 296: NSString *symbolNameString = JavaStringToNSString(env, symbolName);
>
> Is there a reason why `java.desktop` continues to use `JNFJavaToNSString`? I
> was looking to see how this was handled in other places, but I couldn't find
> an example where `JNFJavaToNSString` was converted to call a newly
> implemented `JavaStringToNSString`.
As Magnus said that is in progress. Hoping it will be pushed very soon.
-
PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
> This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m - Changes: - all: https://git.openjdk.java.net/jdk/pull/2304/files - new: https://git.openjdk.java.net/jdk/pull/2304/files/9919a02c..d7ed0158 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2304&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2304&range=00-01 Stats: 39 lines in 1 file changed: 18 ins; 15 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2304/head:pull/2304 PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 18:00:06 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 > > src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 390: > >> 388: store_and_inc(_to, from_obj, NativeStack::intSpace); >> 389: >> 390: _num_int_args++; > > `pass_byte()` and `pass_short()` use only one `_num_int_args++;` after the > `if else` but other methods use 2 of them inside `if else` branches. > > We should be consistent. Agree. I'm going to do as much refactoring as needed before this patch under JDK-8261071 - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Feb 2021 20:05:29 GMT, Anton Kozlov wrote:
>> Thank you all for your comments regarding W^X implementation. I've made a
>> change that reduces the footprint of the implementation, also addressing
>> most of the comments. I'll revisit them individually to make sure nothing is
>> forgotten.
>>
>> The basic principle has not changed: when we execute JVM code (owned by
>> libjvm.so, starting from JVM entry function), we switch to Write state. When
>> we leave JVM to execute generated or JNI code, we switch to Executable
>> state. I would like to highlight that JVM code does not mean the VM state of
>> the java thread. After @stefank's suggestion, I could also drop a few W^X
>> state switches, so now it should be more clear that switches are tied to JVM
>> entry functions.
>
>> I wonder if this is the right choice
>> ...
>> ```
>> OopStorageParIterPerf::~OopStorageParIterPerf() {
>> ...
>> ```
>>
>
> The transition in OopStorageParIterPerf was made for gtest setup to execute
> in WXWrite context. For tests themselves, defining macro set WXWrite.
>
> I've simplified the scheme and now we switch to WXWrite once at the gtest
> launcher. So this transition was dropped.
>
> I've also refreshed my memory and tried to switch to WXWrite as close as
> possible to each place where we'll be writing executable memory. There are a
> lot of such places! As you correctly noted, code cache contains objects, not
> plain data. For example, CodeCache memory management structures,
> CompiledMethod, ... are there, so we need more WXWrite switches than we have
> in the current approach. I had a comparable amount of them just to run
> -version, but certainly not enough to run tier1 tests.
>
> Following your advice, I don't require a known "from" state anymore. So a few
> W^X transitions were dropped, e.g. when the JVM code calls a JNI entry
> function, which expects to be called from the native code. I had to switch to
> WXExec just only to satisfy the expectations. After the update, we don't need
> this anymore.
>
> W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM
> functions are not marked as entries for some reason, although they are called
> directly from e.g. interpreter. I added W^X management to such functions.
Out of curiosity, have you looked at the performance of the W^X state
transition? In particular I'm wondering if the cost is effectively negligible
so doing it unconditionally on JVM entry is a no-brainer and just
easier/cleaner than the alternatives, or if there are reasons to look at only
doing the transition if/when needed (perhaps do it lazily and revert back to X
when leaving the JVM?).
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 23:03:45 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/share/runtime/thread.cpp line 3991: > >> 3989: JavaThread* thread = JavaThread::current(); >> 3990: ThreadToNativeFromVM ttn(thread); >> 3991: Thread::WXExecFromWriteSetter wx_exec; > > I saw somewhere in this review a comment about why this new > WXExecFromWriteSetter helper isn't folded into ThreadToNativeFromVM > and I understand that not every current ThreadToNativeFromVM needs > the new helper. If the vast majority of the ThreadToNativeFromVM > locations need WXExecFromWriteSetter, then perhaps those locations > that currently have a ThreadToNativeFromVM followed by the new > WXExecFromWriteSetter should use a new helper: > > ThreadToNativeWithWXExecFromVM > > so we'll see changes from: > > ThreadToNativeFromVM -> ThreadToNativeWithWXExecFromVM > > where we need them and hopefully a short comment can be added > at the same time to explain the need for WXExec. This will allow us > to easily distinguish ThreadToNativeFromVM locations that DO NOT > need WXExec from those that DO need it. With a small overhead for tracking the current W^X state, I avoided WXExecFromWriteSetter near ThreadToNativeFromVM at all. New ThreadWXEnable(WXExec) is used only to call a generated function. More common ThreadWXEnable(WXWrite) is tied to JVM entry functions. I added comments for functions that are not clear to be entries, although they are. Thank you for the suggestion! - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
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 [v2]
On Tue, 26 Jan 2021 12:50:22 GMT, Anton Kozlov wrote:
>> Yes, that's why I thought it should be added to the classes
>> ThreadInVMfromNative, etc like:
>> class ThreadInVMfromNative : public ThreadStateTransition {
>> ResetNoHandleMark __rnhm;
>>
>> We can look at it with cleaning up the thread transitions RFE or as a
>> follow-on. If every line of ThreadInVMfromNative has to have one of these
>> Thread::WXWriteVerifier __wx_write; people are going to miss them when
>> adding the former.
>
> Not every ThreadInVMfromNative needs this, for example JIT goes to Native
> state without changing of W^X state. But from some experience of maintaining
> this patch, I actually had a duty to add missing W^X transitions after assert
> failures. A possible solution is actually to make W^X transition a part of
> ThreadInVMfromNative (and similar), but controlled by an optional constructor
> parameter with possible values "do exec->write", "verify write"...;. So in a
> common case ThreadInVMfromNative would implicitly do the transition and still
> would allow something uncommon like verification of the state for the JIT. I
> have to think about this.
I've dropped this transition here and in similar places after state tracking
always available. As a benefit, there are few places really using the setter
and all of them are tied to VM_ENTRY macro or similar one. I expect we don't
need to do W^X management near every java thread state change.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Tue, 26 Jan 2021 12:01:30 GMT, Coleen Phillimore wrote: >> I assume a WXVerifier class that tracks W^X mode in debug mode and does >> nothing in release mode. I've considered to do this, it's relates to small >> inefficiencies, while this patch brings zero overhead (in release) for a >> platform that does not need W^X. >> * We don't need thread instance in release to call >> `os::current_thread_enable_wx`. Having WXVerifier a part of the Thread will >> require calling `Thread::current()` first and we could only hope for >> compiler to optimize this out, not sure if it will happen at all. In some >> contexts the Thread instance is available, in some it's not. >> * An instance of the empty class (as WXVerifier will be in the release) will >> occupy non-zero space in the Thread instance. >> >> If such costs are negligible, I can do as suggested. > > I really just want the minimal number of lines of code and hooks in > thread.hpp. You can still access it through the thread, just like these > lines currently. Look at HandshakeState as an example. Please take a look at the recent changes. Changes in thread.hpp were reduced: https://github.com/openjdk/jdk/pull/2200/files#diff-abdc409967d04172ecc20e3747aa55a79e755584d570b57c4d58902a9813d188 thread.inline.hpp provides definitions of accessors (non-trivial): https://github.com/openjdk/jdk/pull/2200/files#diff-3a29f7f952bf2bd936f49e97cb3b86a7324851133e879c142dec724455310b50 And new threadWXSetters.hpp defines RAII context setter: https://github.com/openjdk/jdk/pull/2200/files#diff-6424782ec43941031282f079e89adaa76d341ce340a3b78b0e9657358ec16278 - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Wed, 3 Feb 2021 19:57:24 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.
>
> Thank you all for your comments regarding W^X implementation. I've made a
> change that reduces the footprint of the implementation, also addressing most
> of the comments. I'll revisit them individually to make sure nothing is
> forgotten.
>
> The basic principle has not changed: when we execute JVM code (owned by
> libjvm.so, starting from JVM entry function), we switch to Write state. When
> we leave JVM to execute generated or JNI code, we switch to Executable state.
> I would like to highlight that JVM code does not mean the VM state of the
> java thread. After @stefank's suggestion, I could also drop a few W^X state
> switches, so now it should be more clear that switches are tied to JVM entry
> functions.
> I wonder if this is the right choice
> ...
> ```
> OopStorageParIterPerf::~OopStorageParIterPerf() {
> ...
> ```
>
The transition in OopStorageParIterPerf was made for gtest setup to execute in
WXWrite context. For tests themselves, defining macro set WXWrite.
I've simplified the scheme and now we switch to WXWrite once at the gtest
launcher. So this transition was dropped.
I've also refreshed my memory and tried to switch to WXWrite as close as
possible to each place where we'll be writing executable memory. There are a
lot of such places! As you correctly noted, code cache contains objects, not
plain data. For example, CodeCache memory management structures,
CompiledMethod, ... are there, so we need more WXWrite switches than we have
in the current approach. I had a comparable amount of them just to run
-version, but certainly not enough to run tier1 tests.
Following your advice, I don't require a known "from" state anymore. So a few
W^X transitions were dropped, e.g. when the JVM code calls a JNI entry
function, which expects to be called from the native code. I had to switch to
WXExec just only to satisfy the expectations. After the update, we don't need
this anymore.
W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM
functions are not marked as entries for some reason, although they are called
directly from e.g. interpreter. I added W^X management to such functions.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 23:10:17 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 > > 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. Thank you all for your comments regarding W^X implementation. I've made a change that reduces the footprint of the implementation, also addressing most of the comments. I'll revisit them individually to make sure nothing is forgotten. The basic principle has not changed: when we execute JVM code (owned by libjvm.so, starting from JVM entry function), we switch to Write state. When we leave JVM to execute generated or JNI code, we switch to Executable state. I would like to highlight that JVM code does not mean the VM state of the java thread. After @stefank's suggestion, I could also drop a few W^X state switches, so now it should be more clear that switches are tied to JVM entry functions. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v10]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2200/files - new: https://git.openjdk.java.net/jdk/pull/2200/files/3c705ae5..80827176 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2200&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2200&range=08-09 Stats: 444 lines in 64 files changed: 112 ins; 278 del; 54 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
Integrated: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m
On Fri, 18 Dec 2020 19:20:47 GMT, Weijun Wang wrote: > This fix covers both > > - [[macOS]: Remove JNF dependency from > libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858) > - [[macOS]: Remove JNF dependency from > libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860) This pull request has now been integrated. Changeset: 4a8b5c16 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/4a8b5c16 Stats: 805 lines in 12 files changed: 369 ins; 317 del; 119 mod 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m 8257860: [macOS]: Remove JNF dependency from libosxkrb5/SCDynamicStoreConfig.m Reviewed-by: erikj, prr, ihse, valeriep - PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v8]
> This fix covers both > > - [[macOS]: Remove JNF dependency from > libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858) > - [[macOS]: Remove JNF dependency from > libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860) Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: valerie comment, remove useless test - Changes: - all: https://git.openjdk.java.net/jdk/pull/1845/files - new: https://git.openjdk.java.net/jdk/pull/1845/files/b5f1e15d..41fed31f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1845&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1845&range=06-07 Stats: 107 lines in 2 files changed: 2 ins; 105 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1845.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1845/head:pull/1845 PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v7]
On Wed, 3 Feb 2021 11:27:49 GMT, Valerie Peng wrote:
>> Weijun Wang has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - file attr error
>> - objc use .m
>
> src/java.security.jgss/share/classes/sun/security/krb5/SCDynamicStoreConfig.java
> line 103:
>
>> 101: ri.put("kdc", kdcs);
>> 102: }
>> 103: realms.put(nextRealm, ri);
>
> Do you need to check if ri is empty?
It should make no difference to applications whether there is an empty section
or not, but I can remove it.
-
PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v7]
On Mon, 1 Feb 2021 16:51:12 GMT, Weijun Wang wrote: >> This fix covers both >> >> - [[macOS]: Remove JNF dependency from >> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858) >> - [[macOS]: Remove JNF dependency from >> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860) > > Weijun Wang has updated the pull request incrementally with two additional > commits since the last revision: > > - file attr error > - objc use .m Rest of the Kerberos config handling changes look fine. Thanks! - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v7]
On Mon, 1 Feb 2021 16:51:12 GMT, Weijun Wang wrote:
>> This fix covers both
>>
>> - [[macOS]: Remove JNF dependency from
>> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858)
>> - [[macOS]: Remove JNF dependency from
>> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860)
>
> Weijun Wang has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - file attr error
> - objc use .m
src/java.security.jgss/share/classes/sun/security/krb5/SCDynamicStoreConfig.java
line 103:
> 101: ri.put("kdc", kdcs);
> 102: }
> 103: realms.put(nextRealm, ri);
Do you need to check if ri is empty?
-
PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On 2021-02-02 21:47, Chris Plummer wrote: Is there a reason why `java.desktop` continues to use `JNFJavaToNSString`? I was looking to see how this was handled in other places, but I couldn't find an example where `JNFJavaToNSString` was converted to call a newly implemented `JavaStringToNSString`. That conversion is handled in https://github.com/openjdk/jdk/pull/2305. /Magnus
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 21:49:36 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/cpu/aarch64/macroAssembler_aarch64.cpp line 323:
>
>> 321: str(zr, Address(rthread, JavaThread::last_Java_pc_offset()));
>> 322:
>> 323: str(zr, Address(rthread, JavaFrameAnchor::saved_fp_address_offset()));
>
> I don't think this switch from `JavaThread::saved_fp_address_offset()`
> to `JavaFrameAnchor::saved_fp_address_offset()` is correct since
> `rthread` is still used and is a JavaThread*. The new code will give you:
>
> `rthread` + offset of the `saved_fp_address` field in a JavaFrameAnchor
>
> The old code gave you:
>
> `rthread` + offset of the `saved_fp_address` field in the JavaFrameAnchor
> field in the JavaThread
>
> Those are not the same things.
I agree, I don't understand why this change was made.
> src/hotspot/os_cpu/bsd_aarch64/os_bsd_aarch64.hpp line 45:
>
>> 43: // Atomically copy 64 bits of data
>> 44: static void atomic_copy64(const volatile void *src, volatile void
>> *dst) {
>> 45: *(jlong *) dst = *(const jlong *) src;
>
> Is this construct actually atomic on aarch64?
Yes.
> src/hotspot/os_cpu/windows_aarch64/os_windows_aarch64.hpp line 37:
>
>> 35:
>> 36: private:
>> 37:
>
> 'private' usually has one space in front of it.
> Also, why the blank line after it?
It reads better with the blank line, and it's not in violation of HotSpot
conventions.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]
On Tue, 2 Feb 2021 18:03:50 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
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5271:
>
>> 5269: //
>> 5270: void MacroAssembler::get_thread(Register dst) {
>> 5271: RegSet saved_regs = RegSet::range(r0, r1) +
>> BSD_ONLY(RegSet::range(r2, r17)) + lr - dst;
>
> The comment needs to be updated, since on BSD we also seem to clobber r2,r17 ?
Surely this should be
saved_regs = RegSet::range(r0, r1) BSD_ONLY(+ RegSet::range(r2, r17)) + lr -
dst;```
Shouldn't it?
-
PR: https://git.openjdk.java.net/jdk/pull/2200
