Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 23:35:52 GMT, Phil Race wrote: >> That may actually be a valid concern. Both say macOS 10.12+ ... which might >> conflict with the 10.9 target. > > Maybe you should just file a bug after all for this to be dealt with > separately. > Certainly if it is NOT fixed now such a bug needs to be filed. I have created https://bugs.openjdk.java.net/browse/JDK-8260402 which is blocked by jep-391 currently - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 23:34:04 GMT, Phil Race wrote: >> that sounds good to me, I am just afraid to break intel mac on older macos >> versions with this change. > > That may actually be a valid concern. Both say macOS 10.12+ ... which might > conflict with the 10.9 target. Maybe you should just file a bug after all for this to be dealt with separately. Certainly if it is NOT fixed now such a bug needs to be filed. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 22:47:33 GMT, Vladimir Kempik wrote: >> 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask >> 2) So maybe rather than the deprecation suppression you could change both >> constants to the new ones. >> Ordinarily I'd say let someone else do that but this looks like a simple >> obvious change and is dwarfed by all the other changes going on for Mac ARM >> ... > > that sounds good to me, I am just afraid to break intel mac on older macos > versions with this change. That may actually be a valid concern. Both say macOS 10.12+ ... which might conflict with the 10.9 target. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 19:38:16 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 two additional > commits since the last revision: > > - Refactor CDS disabling > - Redo builsys support for aarch64-darwin JDI changes also look good. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 19:38:16 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 two additional > commits since the last revision: > > - Refactor CDS disabling > - Redo builsys support for aarch64-darwin I looked through the SA changes. Overall looks good except for a couple of minor issues noted. For the most part it seems to have been boilerplate copy-n-paste from existing ports. If there is anything you want me to take a closer look at, let me know. src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 702: > 700: primitiveArray = (*env)->GetLongArrayElements(env, registerArray, > NULL); > 701: > 702: #undef REG_INDEX I'm not so sure why the #undef and subsequent #define of REG_INDEX is needed since it seems to just get #define'd back to the same value. src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/debugger/bsd/aarch64/BsdAARCH64ThreadContext.java line 2: > 1: /* > 2: * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved. Update copyright. - Marked as reviewed by cjplummer (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 22:42:40 GMT, Phil Race wrote: >> Min_macos version is changed to 11.0 for macos_aarch64 >> >> https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136 > > 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask > 2) So maybe rather than the deprecation suppression you could change both > constants to the new ones. > Ordinarily I'd say let someone else do that but this looks like a simple > obvious change and is dwarfed by all the other changes going on for Mac ARM > ... that sounds good to me, I am just afraid to break intel mac on older macos versions with this change. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 22:25:48 GMT, Vladimir Kempik wrote: >> Are you doing something somewhere to change the target version of macOS or >> SDK ? I had no such problem. >> I think we currently target a macOS 10.9 and if you are changing that it >> would need discussion. >> If you are changing it only for Mac ARM that may make more sense .. >> >> And these appear to be just API churn by Apple >> NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst >> >> https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc >> >> NSBorderlessWindowMask is replaced by NSWindowStyleMask >> >> https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ > > Min_macos version is changed to 11.0 for macos_aarch64 > > https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136 1) I meant change to NSWindowStyleMaskBorderless from NSBorderlessWindowMask 2) So maybe rather than the deprecation suppression you could change both constants to the new ones. Ordinarily I'd say let someone else do that but this looks like a simple obvious change and is dwarfed by all the other changes going on for Mac ARM ... - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 22:22:06 GMT, Phil Race wrote: >> It seems these workarounds are still needed: >> >> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39: >> error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS >> 10.14 [-Werror,-Wdeprecated-declarations] >> bitmapFormat: NSAlphaFirstBitmapFormat | >> NSAlphaNonpremultipliedBitmapFormat >> ^~~~ >> NSBitmapFormatAlphaFirst >> >> jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34: >> error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS >> 10.12 [-Werror,-Wdeprecated-declarations] >> styleMask: NSBorderlessWindowMask >> ^~ >> NSWindowStyleMaskBorderless > > Are you doing something somewhere to change the target version of macOS or > SDK ? I had no such problem. > I think we currently target a macOS 10.9 and if you are changing that it > would need discussion. > If you are changing it only for Mac ARM that may make more sense .. > > And these appear to be just API churn by Apple > NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst > > https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc > > NSBorderlessWindowMask is replaced by NSWindowStyleMask > > https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ Min_macos version is changed to 11.0 for macos_aarch64 https://github.com/openjdk/jdk/pull/2200/files/0c2cb0a372bf1a8607810d773b53d6959616a816#diff-7cd97cdbeb3053597e5d6659016cdf0f60b2c412bd39934a43817ee0b717b7a7R136 - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 21:18:59 GMT, Vladimir Kempik wrote: >> Hello >> I believe it was a workaround for issues with xcode 12.2 in early beta days. >> Those issues were fixed later in upstream jdk, so most likely we need to >> remove these workarounds. > > It seems these workarounds are still needed: > > jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39: > error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS > 10.14 [-Werror,-Wdeprecated-declarations] > bitmapFormat: NSAlphaFirstBitmapFormat | > NSAlphaNonpremultipliedBitmapFormat > ^~~~ > NSBitmapFormatAlphaFirst > > jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34: > error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS > 10.12 [-Werror,-Wdeprecated-declarations] > styleMask: NSBorderlessWindowMask > ^~ > NSWindowStyleMaskBorderless Are you doing something somewhere to change the target version of macOS or SDK ? I had no such problem. I think we currently target a macOS 10.9 and if you are changing that it would need discussion. If you are changing it only for Mac ARM that may make more sense .. And these appear to be just API churn by Apple NSAlphaFirstBitmapFormat is replaced by NSBitmapFormatAlphaFirst https://developer.apple.com/documentation/appkit/nsbitmapformat/nsbitmapformatalphafirst?language=objc NSBorderlessWindowMask is replaced by NSWindowStyleMask https://developer.apple.com/documentation/appkit/nswindowstylemask?language=occ - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 20:54:38 GMT, Vladimir Kempik wrote: >> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573: >> >>> 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \ >>> 572: WARNINGS_AS_ERRORS_xlc := false, \ >>> 573: DISABLED_WARNINGS_clang := deprecated-declarations, \ >> >> I see this added here and in another location. What is deprecated ? You >> really need to explain these sorts of things. >> I've built JDK using Xcode 12.3 and not had any need for this. > > Hello > I believe it was a workaround for issues with xcode 12.2 in early beta days. > Those issues were fixed later in upstream jdk, so most likely we need to > remove these workarounds. It seems these workarounds are still needed: jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:300:39: error: 'NSAlphaFirstBitmapFormat' is deprecated: first deprecated in macOS 10.14 [-Werror,-Wdeprecated-declarations] bitmapFormat: NSAlphaFirstBitmapFormat | NSAlphaNonpremultipliedBitmapFormat ^~~~ NSBitmapFormatAlphaFirst jdk/src/java.desktop/macosx/native/libsplashscreen/splashscreen_sys.m:438:34: error: 'NSBorderlessWindowMask' is deprecated: first deprecated in macOS 10.12 [-Werror,-Wdeprecated-declarations] styleMask: NSBorderlessWindowMask ^~ NSWindowStyleMaskBorderless - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 19:42:41 GMT, Phil Race wrote: >> Anton Kozlov has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Refactor CDS disabling >> - Redo builsys support for aarch64-darwin > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573: > >> 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \ >> 572: WARNINGS_AS_ERRORS_xlc := false, \ >> 573: DISABLED_WARNINGS_clang := deprecated-declarations, \ > > I see this added here and in another location. What is deprecated ? You > really need to explain these sorts of things. > I've built JDK using Xcode 12.3 and not had any need for this. Hello I believe it was a workaround for issues with xcode 12.2 in early beta days. Those issues were fixed later in upstream jdk, so most likely we need to remove these workarounds. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
On Mon, 25 Jan 2021 19:38:16 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 two additional > commits since the last revision: > > - Refactor CDS disabling > - Redo builsys support for aarch64-darwin make/modules/java.desktop/lib/Awt2dLibraries.gmk line 573: > 571: EXTRA_HEADER_DIRS := $(LIBFONTMANAGER_EXTRA_HEADER_DIRS), \ > 572: WARNINGS_AS_ERRORS_xlc := false, \ > 573: DISABLED_WARNINGS_clang := deprecated-declarations, \ I see this added here and in another location. What is deprecated ? You really need to explain these sorts of things. I've built JDK using Xcode 12.3 and not had any need for this. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v3]
> 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 two additional commits since the last revision: - Refactor CDS disabling - Redo builsys support for aarch64-darwin - Changes: - all: https://git.openjdk.java.net/jdk/pull/2200/files - new: https://git.openjdk.java.net/jdk/pull/2200/files/50b55f66..0c2cb0a3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2200&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2200&range=01-02 Stats: 36 lines in 3 files changed: 12 ins; 15 del; 9 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: 8260289: Unable to customize module lists after change JDK-8258411
On Mon, 25 Jan 2021 12:57:29 GMT, Andrew Leonard wrote: > A problem was found downstream with Eclipse OpenJ9 builds whereby since > JDK-8258411 they were unable to extend the module lists. > This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to > enable the module list extension again. > > Signed-off-by: Andrew Leonard This pull request has now been integrated. Changeset: 12ccd211 Author:Andrew Leonard URL: https://git.openjdk.java.net/jdk/commit/12ccd211 Stats: 9 lines in 1 file changed: 5 ins; 3 del; 1 mod 8260289: Unable to customize module lists after change JDK-8258411 Reviewed-by: ihse, alanb - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Mon, 25 Jan 2021 15:01:25 GMT, Anton Kozlov wrote:
>> src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 87:
>>
>>> 85: JavaThread* jt = JavaThread::thread_from_jni_environment(jni_env);
>>> 86: DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));;
>>> 87: Thread::WXWriteFromExecSetter wx_write;
>>
>> Is this on every transition to VM from Native? Would it be better to add to
>> ThreadInVMfromNative like the ResetNoHandleMark is?
>
> I've tried to do something like this initially. The idea was to use Write in
> VM state and Exec in Java and Native states. However, for example, JIT runs
> in the Native state and needs Write access. So instead, W^X is managed on
> entry and exit from VM code, in macros like JRT_ENTRY. Unfortunately, not
> every JVM entry function is defined with an appropriate macro (at least for
> now), so I had to add explicit W^X state management along with the explicit
> java thread state, like here.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Mon, 25 Jan 2021 09:52:00 GMT, Andrew Haley wrote: >> Hello >> Why is it not nice ? >> linux_aarch64 uses some linux specific tls function >> _ZN10JavaThread25aarch64_get_thread_helperEv from >> hotspot/os_cpu/linux_aarch64/threadLS_linux_aarch64.s >> which clobbers only r0 and r1 >> macos_aarch64 has no such tls code and uses generic C-call to >> Thread::current(); >> Hence we are saving possibly clobbered regs here. > > Surely if you did as Linux does you wouldn't need to clobber all those > registers. I see how this can be done, from looking at C example with `__thread`, which involves poorly documented relocations and private libc interface invocation. But now I also wonder how much benefit would we have from this optimization. `get_thread` is called from few places and all of them are guarded by `#ifdef ASSERT`. The release build is still fine after I removed MacroAssembler::get_thread definition (as a verification). Callers of get_thread: * StubAssembler::call_RT * Runtime1::generate_patching * StubGenerator::generate_call_stub * StubGenerator::generate_catch_exception All of them are heavy-weight functions, nonoptimal get_thread is unlikely to slow them down even in fastdebug. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Sun, 24 Jan 2021 15:32:59 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 two additional > commits since the last revision: > > - Address feedback for signature generators > - Enable -Wformat-nonliteral back Changes requested by prr (Reviewer). src/java.desktop/share/native/libharfbuzz/hb-common.h line 113: > 111: > 112: #define HB_TAG(c1,c2,c3,c4) > ((hb_tag_t)uint32_t)(c1)&0xFF)<<24)|(((uint32_t)(c2)&0xFF)<<16)|(((uint32_t)(c3)&0xFF)<<8)|((uint32_t)(c4)&0xFF))) > 113: #define HB_UNTAG(tag) (char)(((tag)>>24)&0xFF), > (char)(((tag)>>16)&0xFF), (char)(((tag)>>8)&0xFF), (char)((tag)&0xFF) I need a robust explanation of these changes. They are not mentioned, nor are they in upstream harfbuzz. Likely these should be pushed to upstream first if there is a reason for them. src/java.desktop/share/native/libharfbuzz/hb-coretext.cc line 193: > 191:* crbug.com/549610 */ > 192: // 0x0007 stands for "kCTVersionNumber10_10", see CoreText.h > 193: #if TARGET_OS_OSX && MAC_OS_X_VERSION_MIN_REQUIRED < __MAC_10_10 I need a robust explanation of these changes. They are not mentioned, nor are they in upstream harfbuzz. Likely these should be pushed to upstream first if there is a reason for them. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Sun, 24 Jan 2021 15:50:01 GMT, Anton Kozlov wrote:
>> src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 86:
>>
>>> 84:
>>> 85: switch (_num_int_args) {
>>> 86: case 0:
>>
>> I don't think you need such a large switch statement. I think this can be
>> expressed as
>> if (num_int_args <= 6) {
>> ldr(as_Register(num_int_args + r1.encoding()), src);
>> ... etc.
>
> I like the suggestion. For the defense, new functions were made this way
> intentionally, to match existing `pass_int`, `pass_long`,.. I take your
> comment as a blessing to fix all of them. But I feel that refactoring of
> existing code should go in a separate commit. Especially after `pass_int`
> used `ldr/str` instead of `ldrw/strw`, which looks wrong.
> https://github.com/openjdk/jdk/pull/2200/files#diff-1ff58ce70aeea7e9842d34e8d8fd9c94dd91182999d455618b2a171efd8f742cL87-R122
This is new code, and it should not repeat the mistakes of the past. There's no
need to refactor the similar existing code as part of this patch.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Mon, 25 Jan 2021 13:30:55 GMT, Vladimir Kempik wrote: >> make/modules/jdk.hotspot.agent/Lib.gmk line 34: >> >>> 32: >>> 33: else ifeq ($(call isTargetOs, macosx), true) >>> 34: SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \ >> >> Is this really proper for macos-x64? I thought we used -Damd64 as a marker >> for all macos-x64 C/C++ files. (Most files get their flags from the common >> setup in configure, but SA is a different beast due to historical reasons). > > @luhenry , could you please check this comment, I think SA-agent was MS's job > here. The target is identified by the header file now: https://github.com/openjdk/jdk/pull/2200/files#diff-51442e74eeef2163f0f0643df0ae995083f666367e25fba2b527a9a5bc8743a6R35-R41 Do you think there is any problem with this approach? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
On Mon, 25 Jan 2021 14:20:01 GMT, Andrew Leonard wrote: >> A problem was found downstream with Eclipse OpenJ9 builds whereby since >> JDK-8258411 they were unable to extend the module lists. >> This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to >> enable the module list extension again. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard 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: > > 8260289: Unable to customize module lists after change JDK-8258411 > > Signed-off-by: Andrew Leonard Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
On Mon, 25 Jan 2021 14:20:01 GMT, Andrew Leonard wrote: >> A problem was found downstream with Eclipse OpenJ9 builds whereby since >> JDK-8258411 they were unable to extend the module lists. >> This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to >> enable the module list extension again. >> >> Signed-off-by: Andrew Leonard > > Andrew Leonard 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. Marked as reviewed by ihse (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
On Mon, 25 Jan 2021 13:08:58 GMT, Magnus Ihse Bursie wrote: >> Andrew Leonard 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: >> >> 8260289: Unable to customize module lists after change JDK-8258411 >> >> Signed-off-by: Andrew Leonard > > Changes requested by ihse (Reviewer). > @magicus do you know what the magic pull request command is to re-generate > the webrev having updated the commit? Ah looks like it automatically picked it up :-) - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Mon, 25 Jan 2021 14:36:35 GMT, Coleen Phillimore wrote: >> Anton Kozlov has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Address feedback for signature generators >> - Enable -Wformat-nonliteral back > > src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 87: > >> 85: JavaThread* jt = JavaThread::thread_from_jni_environment(jni_env); >> 86: DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));; >> 87: Thread::WXWriteFromExecSetter wx_write; > > Is this on every transition to VM from Native? Would it be better to add to > ThreadInVMfromNative like the ResetNoHandleMark is? I've tried to do something like this initially. The idea was to use Write in VM state and Exec in Java and Native states. However, for example, JIT runs in the Native state and needs Write access. So instead, W^X is managed on entry and exit from VM code, in macros like JRT_ENTRY. Unfortunately, not every JVM entry function is defined with an appropriate macro (at least for now), so I had to add explicit W^X state management along with the explicit java thread state, like here. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
On Mon, 25 Jan 2021 13:57:26 GMT, Magnus Ihse Bursie wrote: >> Note, it was the change from "appending" to BOOT_MODULES to just "assigning" >> that broke the existing behaviour with the JDK-8258411 change. > > @andrew-m-leonard (Seems I can't get github to tag you???) > > That sounds good. I think you could move the IncludeCustomExtension to after > all *.conf files, to future-proof it and to make it a bit more consistent -- > "first include conf files, then adjust them in custom extensions". @magicus @AlanBateman please see new commit (webrev https://webrevs.openjdk.java.net/?repo=jdk&pr=2219&range=01) - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
On Mon, 25 Jan 2021 14:00:42 GMT, Andrew Leonard wrote: >> @andrew-m-leonard (Seems I can't get github to tag you???) >> >> That sounds good. I think you could move the IncludeCustomExtension to after >> all *.conf files, to future-proof it and to make it a bit more consistent -- >> "first include conf files, then adjust them in custom extensions". > > @magicus yes, that's exactly the new commit i've just pushed. > > @(lookups) have been a bit wobbly recently with github.com I have noticed, I > can find some people but not others! @andrew-m-leonard try searching at https://github.com/orgs/openjdk/people - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Mon, 25 Jan 2021 14:40:09 GMT, Coleen Phillimore wrote: >> Anton Kozlov has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Address feedback for signature generators >> - Enable -Wformat-nonliteral back > > src/hotspot/share/runtime/thread.hpp line 915: > >> 913: verify_wx_state(WXExec); >> 914: } >> 915: }; > > Rather than add all this to thread.hpp, can you make a wxVerifier.hpp and > just add the class instance as a field in thread.hpp? This could be a follow-up RFE. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Sun, 24 Jan 2021 15:32:59 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 two additional > commits since the last revision: > > - Address feedback for signature generators > - Enable -Wformat-nonliteral back src/hotspot/share/runtime/thread.hpp line 915: > 913: verify_wx_state(WXExec); > 914: } > 915: }; Rather than add all this to thread.hpp, can you make a wxVerifier.hpp and just add the class instance as a field in thread.hpp? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Sun, 24 Jan 2021 15:32:59 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 two additional > commits since the last revision: > > - Address feedback for signature generators > - Enable -Wformat-nonliteral back src/hotspot/share/jfr/instrumentation/jfrJvmtiAgent.cpp line 87: > 85: JavaThread* jt = JavaThread::thread_from_jni_environment(jni_env); > 86: DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_native(jt));; > 87: Thread::WXWriteFromExecSetter wx_write; Is this on every transition to VM from Native? Would it be better to add to ThreadInVMfromNative like the ResetNoHandleMark is? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
On Mon, 25 Jan 2021 13:57:26 GMT, Magnus Ihse Bursie wrote: >> Note, it was the change from "appending" to BOOT_MODULES to just "assigning" >> that broke the existing behaviour with the JDK-8258411 change. > > @andrew-m-leonard (Seems I can't get github to tag you???) > > That sounds good. I think you could move the IncludeCustomExtension to after > all *.conf files, to future-proof it and to make it a bit more consistent -- > "first include conf files, then adjust them in custom extensions". @magicus yes, that's exactly the new commit i've just pushed. @(lookups) have been a bit wobbly recently with github.com I have noticed, I can find some people but not others! - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
On Mon, 25 Jan 2021 13:45:23 GMT, Andrew Leonard wrote: >> The previous behavior provided the ability to "extend" the upstream MODULE >> lists, ie.just add OpenJ9 modules to the base module lists, see: >> https://github.com/ibmruntimes/openj9-openjdk-jdk/blob/01e13c398721628540babac94ac8663be716c0a8/closed/custom/common/Modules.gmk#L21 >> >> The present >> https://github.com/openjdk/jdk/blob/master/make/conf/module-loader-map.conf >> will simply over-write whatever is set in the BOOT_MODULES list. >> >> What I am now going to do is move the IncludeCustomExtension after the >> module-loader-map.conf inclusion. >> The other alternative would be to change module-loader-map.conf so it >> appended to any existing BOOT_MODULES variable, but I think you suggested >> that file is a straight conf file? >> >> @magicus thoughts? > > Note, it was the change from "appending" to BOOT_MODULES to just "assigning" > that broke the existing behaviour with the JDK-8258411 change. @andrew-m-leonard (Seems I can't get github to tag you???) That sounds good. I think you could move the IncludeCustomExtension to after all *.conf files, to future-proof it and to make it a bit more consistent -- "first include conf files, then adjust them in custom extensions". - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
On Mon, 25 Jan 2021 13:08:58 GMT, Magnus Ihse Bursie wrote: >> Andrew Leonard 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: >> >> 8260289: Unable to customize module lists after change JDK-8258411 >> >> Signed-off-by: Andrew Leonard > > Changes requested by ihse (Reviewer). @magicus do you know what the magic pull request command is to re-generate the webrev having updated the commit? - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411 [v2]
> A problem was found downstream with Eclipse OpenJ9 builds whereby since > JDK-8258411 they were unable to extend the module lists. > This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to > enable the module list extension again. > > Signed-off-by: Andrew Leonard Andrew Leonard 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: 8260289: Unable to customize module lists after change JDK-8258411 Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.java.net/jdk/pull/2219/files - new: https://git.openjdk.java.net/jdk/pull/2219/files/0250fdcc..ffd53bff Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2219&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2219&range=00-01 Stats: 14 lines in 2 files changed: 5 ins; 7 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2219.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2219/head:pull/2219 PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Mon, 25 Jan 2021 14:03:40 GMT, Per Liden wrote: > In `make/autoconf/jvm-features.m4` I notice that you haven't enabled ZGC for > macos/aarch64. Is that just an oversight, or is there a reason for that? because it does not work processor_id has no "official docs"-friendly implementation, only a hacky one from ios world. Also ZGC needs additional W^X work. I believe zgc supports needs to be done in a separate commit. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Mon, 25 Jan 2021 13:23:27 GMT, Magnus Ihse Bursie wrote: >> Anton Kozlov has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Address feedback for signature generators >> - Enable -Wformat-nonliteral back > > Changes requested by ihse (Reviewer). In `make/autoconf/jvm-features.m4` I notice that you haven't enabled ZGC for macos/aarch64. Is that just an oversight, or is there a reason for that? - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411
On Mon, 25 Jan 2021 13:41:06 GMT, Andrew Leonard wrote: >>> @AlanBateman Yes, that's what JDK-8258411 inadvertently changed and I am >>> fixing. I am going to now move the common/Modules.gmk >>> IncludeCustomExtension after the module-loader-map.conf include. >> >> Andrew, why can't you just use the existing IncludeCustomExtension then to >> load your own module-loader-map.conf? Or maybe that's just what you are >> going to do? > > The previous behavior provided the ability to "extend" the upstream MODULE > lists, ie.just add OpenJ9 modules to the base module lists, see: > https://github.com/ibmruntimes/openj9-openjdk-jdk/blob/01e13c398721628540babac94ac8663be716c0a8/closed/custom/common/Modules.gmk#L21 > > The present > https://github.com/openjdk/jdk/blob/master/make/conf/module-loader-map.conf > will simply over-write whatever is set in the BOOT_MODULES list. > > What I am now going to do is move the IncludeCustomExtension after the > module-loader-map.conf inclusion. > The other alternative would be to change module-loader-map.conf so it > appended to any existing BOOT_MODULES variable, but I think you suggested > that file is a straight conf file? > > @magicus thoughts? Note, it was the change from "appending" to BOOT_MODULES to just "assigning" that broke the existing behaviour with the JDK-8258411 change. - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411
On Mon, 25 Jan 2021 13:27:29 GMT, Magnus Ihse Bursie wrote: >> @magicus Is there an ordering issue in common/Modules.gmk? It also invokes >> IncludeCustomExtension but this is done before it includes the conf file. > >> @AlanBateman Yes, that's what JDK-8258411 inadvertently changed and I am >> fixing. I am going to now move the common/Modules.gmk IncludeCustomExtension >> after the module-loader-map.conf include. > > Andrew, why can't you just use the existing IncludeCustomExtension then to > load your own module-loader-map.conf? Or maybe that's just what you are going > to do? The previous behavior provided the ability to "extend" the upstream MODULE lists, ie.just add OpenJ9 modules to the base module lists, see: https://github.com/ibmruntimes/openj9-openjdk-jdk/blob/01e13c398721628540babac94ac8663be716c0a8/closed/custom/common/Modules.gmk#L21 The present https://github.com/openjdk/jdk/blob/master/make/conf/module-loader-map.conf will simply over-write whatever is set in the BOOT_MODULES list. What I am now going to do is move the IncludeCustomExtension after the module-loader-map.conf inclusion. The other alternative would be to change module-loader-map.conf so it appended to any existing BOOT_MODULES variable, but I think you suggested that file is a straight conf file? @magicus thoughts? - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Mon, 25 Jan 2021 13:18:34 GMT, Magnus Ihse Bursie wrote: >> Anton Kozlov has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Address feedback for signature generators >> - Enable -Wformat-nonliteral back > > make/common/NativeCompilation.gmk line 1180: > >> 1178: # silently fail otherwise. >> 1179: ifneq ($(CODESIGN), ) >> 1180: $(CODESIGN) -f -s "$(MACOSX_CODESIGN_IDENTITY)" >> --timestamp --options runtime \ > > What does -f do, and is it needed for macos-x64 as well? -f - replace signature if it's present, if not - just sign as usual macos-aarch64 binaries always have adhoc signature, so need to replace it. > make/modules/jdk.hotspot.agent/Lib.gmk line 34: > >> 32: >> 33: else ifeq ($(call isTargetOs, macosx), true) >> 34: SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \ > > Is this really proper for macos-x64? I thought we used -Damd64 as a marker > for all macos-x64 C/C++ files. (Most files get their flags from the common > setup in configure, but SA is a different beast due to historical reasons). @luhenry , could you please check this comment, I think SA-agent was MS's job here. - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411
On Mon, 25 Jan 2021 13:19:31 GMT, Alan Bateman wrote: >> make/conf/module-loader-map.conf line 100: >> >>> 98: # Hook to include the corresponding custom file, if present. >>> 99: $(eval $(call IncludeCustomExtension, conf/module-loader-map.conf)) >>> 100: >> >> Using IncludeCustomExtension is a good idea, but you should to this where >> module-loader-map.conf is included in common/Modules.gmk, not here. The >> `*.conf` files are supposed to be formatted as simple configuration files, >> and not include special make commands. > > @magicus Is there an ordering issue in common/Modules.gmk? It also invokes > IncludeCustomExtension but this is done before it includes the conf file. @AlanBateman I'm not sure. Frankly, most of the IncludeCustomExtension hooks are no longer used by Oracle since we've moved most of our code out in the open, so they kind of bit rot... I should probably do a sweep and ask the community which one to still keep, and remove the rest. > @AlanBateman Yes, that's what JDK-8258411 inadvertently changed and I am > fixing. I am going to now move the common/Modules.gmk IncludeCustomExtension > after the module-loader-map.conf include. Andrew, why can't you just use the existing IncludeCustomExtension then to load your own module-loader-map.conf? Or maybe that's just what you are going to do? - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC [v5]
On Mon, 25 Jan 2021 07:17:13 GMT, Thomas Stuefe wrote: >> Hi David, >> The changes look good. Just a couple of nits. >> 1. Could you add a comment explaining why AIX and BSD have their own version >> of javaTimeNanos_info(). >> 2. A few copyrights need to be updated to 2021. >> Thanks, Harold > > I don't see any new issues on AIX in our tests with this patch. Thanks for > waiting! Hi David, I think that os_posix.cpp is a good place to put a comment explaining why AIX and BSD have their own javaTimeNanos_info(). Stating something like "AIX and BSD have their own versions because their implementations of monotonic time have worked on those platforms." seems fine. Thanks for doing this! Harold - PR: https://git.openjdk.java.net/jdk/pull/2090
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Sun, 24 Jan 2021 15:32:59 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 two additional > commits since the last revision: > > - Address feedback for signature generators > - Enable -Wformat-nonliteral back Changes requested by ihse (Reviewer). make/autoconf/jvm-features.m4 line 269: > 267: if test "x$OPENJDK_TARGET_OS" != xaix && \ > 268: !( test "x$OPENJDK_TARGET_OS" = "xmacosx" && \ > 269: test "x$OPENJDK_TARGET_CPU" = "xaarch64" ) ; then This test is making my head spin. Can't you just invert it to this structure: if OS=aix || OS+CPU = mac+aarch64; then no else yes fi make/autoconf/platform.m4 line 75: > 73: ;; > 74: esac > 75: ;; This is solved in the wrong place. This code should just use the result from `config.guess/config.sub`. These files are imported from the autoconf project. Unfortunately, they have changed the license to one incompatible with OpenJDK, so we are stuck with an old version. Instead, we have created a "bugfix wrapper" that calls the original `autoconf-config.guess/sub` and fixes up the result, with stuff like this. make/autoconf/platform.m4 line 273: > 271: # Convert the autoconf OS/CPU value to our own data, into the > VAR_OS/CPU/LIBC variables. > 272: PLATFORM_EXTRACT_VARS_FROM_OS($build_os) > 273: PLATFORM_EXTRACT_VARS_FROM_CPU($build_cpu, $build_os) Fixing the comment above means this change, and the one below, can be reverted. make/common/NativeCompilation.gmk line 1180: > 1178: # silently fail otherwise. > 1179: ifneq ($(CODESIGN), ) > 1180: $(CODESIGN) -f -s "$(MACOSX_CODESIGN_IDENTITY)" > --timestamp --options runtime \ What does -f do, and is it needed for macos-x64 as well? make/modules/jdk.hotspot.agent/Lib.gmk line 34: > 32: > 33: else ifeq ($(call isTargetOs, macosx), true) > 34: SA_CFLAGS := -D_GNU_SOURCE -mno-omit-leaf-frame-pointer \ Is this really proper for macos-x64? I thought we used -Damd64 as a marker for all macos-x64 C/C++ files. (Most files get their flags from the common setup in configure, but SA is a different beast due to historical reasons). - PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411
On Mon, 25 Jan 2021 13:19:31 GMT, Alan Bateman wrote: >> make/conf/module-loader-map.conf line 100: >> >>> 98: # Hook to include the corresponding custom file, if present. >>> 99: $(eval $(call IncludeCustomExtension, conf/module-loader-map.conf)) >>> 100: >> >> Using IncludeCustomExtension is a good idea, but you should to this where >> module-loader-map.conf is included in common/Modules.gmk, not here. The >> `*.conf` files are supposed to be formatted as simple configuration files, >> and not include special make commands. > > @magicus Is there an ordering issue in common/Modules.gmk? It also invokes > IncludeCustomExtension but this is done before it includes the conf file. @AlanBateman Yes, that's what JDK-8258411 inadvertently changed and I am fixing. I am going to now move the common/Modules.gmk IncludeCustomExtension after the module-loader-map.conf include. - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411
On Mon, 25 Jan 2021 13:08:53 GMT, Magnus Ihse Bursie wrote: >> A problem was found downstream with Eclipse OpenJ9 builds whereby since >> JDK-8258411 they were unable to extend the module lists. >> This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to >> enable the module list extension again. >> >> Signed-off-by: Andrew Leonard > > make/conf/module-loader-map.conf line 100: > >> 98: # Hook to include the corresponding custom file, if present. >> 99: $(eval $(call IncludeCustomExtension, conf/module-loader-map.conf)) >> 100: > > Using IncludeCustomExtension is a good idea, but you should to this where > module-loader-map.conf is included in common/Modules.gmk, not here. The > `*.conf` files are supposed to be formatted as simple configuration files, > and not include special make commands. @magicus Is there an ordering issue in common/Modules.gmk? It also invokes IncludeCustomExtension but this is done before it includes the conf file. - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411
On Mon, 25 Jan 2021 13:08:53 GMT, Magnus Ihse Bursie wrote: >> A problem was found downstream with Eclipse OpenJ9 builds whereby since >> JDK-8258411 they were unable to extend the module lists. >> This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to >> enable the module list extension again. >> >> Signed-off-by: Andrew Leonard > > make/conf/module-loader-map.conf line 100: > >> 98: # Hook to include the corresponding custom file, if present. >> 99: $(eval $(call IncludeCustomExtension, conf/module-loader-map.conf)) >> 100: > > Using IncludeCustomExtension is a good idea, but you should to this where > module-loader-map.conf is included in common/Modules.gmk, not here. The > `*.conf` files are supposed to be formatted as simple configuration files, > and not include special make commands. Ah ok thanks Magnus, didn't realize that, I will update - PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8260289: Unable to customize module lists after change JDK-8258411
On Mon, 25 Jan 2021 12:57:29 GMT, Andrew Leonard wrote: > A problem was found downstream with Eclipse OpenJ9 builds whereby since > JDK-8258411 they were unable to extend the module lists. > This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to > enable the module list extension again. > > Signed-off-by: Andrew Leonard Changes requested by ihse (Reviewer). make/conf/module-loader-map.conf line 100: > 98: # Hook to include the corresponding custom file, if present. > 99: $(eval $(call IncludeCustomExtension, conf/module-loader-map.conf)) > 100: Using IncludeCustomExtension is a good idea, but you should to this where module-loader-map.conf is included in common/Modules.gmk, not here. The `*.conf` files are supposed to be formatted as simple configuration files, and not include special make commands. - PR: https://git.openjdk.java.net/jdk/pull/2219
RFR: 8260289: Unable to customize module lists after change JDK-8258411
A problem was found downstream with Eclipse OpenJ9 builds whereby since JDK-8258411 they were unable to extend the module lists. This PR adds a IncludeCustomExtension to the conf/module-loader-map.conf, to enable the module list extension again. Signed-off-by: Andrew Leonard - Commit messages: - 8260289: Unable to customize module lists after change JDK-8258411 Changes: https://git.openjdk.java.net/jdk/pull/2219/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2219&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260289 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2219.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2219/head:pull/2219 PR: https://git.openjdk.java.net/jdk/pull/2219
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Sun, 24 Jan 2021 16:29:31 GMT, Vladimir Kempik wrote:
>> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5272:
>>
>>> 5270: void MacroAssembler::get_thread(Register dst) {
>>> 5271: RegSet saved_regs = RegSet::range(r0, r1) +
>>> BSD_ONLY(RegSet::range(r2, r17)) + lr - dst;
>>> 5272: push(saved_regs, sp);
>>
>> This isn't very nice.
>
> Hello
> Why is it not nice ?
> linux_aarch64 uses some linux specific tls function
> _ZN10JavaThread25aarch64_get_thread_helperEv from
> hotspot/os_cpu/linux_aarch64/threadLS_linux_aarch64.s
> which clobbers only r0 and r1
> macos_aarch64 has no such tls code and uses generic C-call to
> Thread::current();
> Hence we are saving possibly clobbered regs here.
Surely if you did as Linux does you wouldn't need to clobber all those
registers.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v2]
On Sun, 24 Jan 2021 16:10:44 GMT, Anton Kozlov wrote:
>> src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp line 394:
>>
>>> 392:
>>> 393: class SlowSignatureHandler
>>> 394: : public NativeSignatureIterator {
>>
>> SlowSignatureHandler is turning into a maintenance nightmare. This isn't the
>> fault of this commit so much as some nasty cut-and-paste programming in the
>> code you're editing. It might well be better to rewrite this whole thing to
>> use a table-driven approach, with a table that contains the increment and
>> the size of each native type: then we'd have only a single routine which
>> could pass data of any type, and each OS needs only supply a table of
>> constants.
>
> Would you like me to do something about it now? The problem is that the
> functions of SlowSignatureHandler are subtly different, so it will be
> multiple tables, not sure how many. Such change is another candidate for a
> separate code enhancement, which I would like not to mix into the JEP
> implementation (it's already not a small change :)). But if you think it
> would be a good thing, please let me know. In another case, I'll add this to
> a queue of follow-up enhancements.
I'm not sure it can wait. This change turns already-messy code into something
significantly messier, to the extent that it's not really good enough to go
into mainline.
-
PR: https://git.openjdk.java.net/jdk/pull/2200
