RFR: 8328824: Clean up java.base native compilation
This is a follow-up on [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same kind of cleanup to java.base. Some code needed more special treatment here, so there is some additional effects outside of the modules/java.base directory. - Commit messages: - Fix other libraries - Fix core libraries - Fix launchers - Move tzmappings to copy phase Changes: https://git.openjdk.org/jdk/pull/18457/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18457=00 Issue: https://bugs.openjdk.org/browse/JDK-8328824 Stats: 248 lines in 7 files changed: 111 ins; 46 del; 91 mod Patch: https://git.openjdk.org/jdk/pull/18457.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18457/head:pull/18457 PR: https://git.openjdk.org/jdk/pull/18457
Re: RFR: 8328824: Clean up java.base native compilation
On Fri, 22 Mar 2024 17:09:16 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on > [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same > kind of cleanup to java.base. Some code needed more special treatment here, > so there is some additional effects outside of the modules/java.base > directory. I can confirm that COMPARE_BUILD only sees changed files in java.base. Once again, as far as I can tell, the changes are all related to the reordering of libraries to link with. make/common/JdkNativeCompilation.gmk line 269: > 267: endif > 268: > 269: ifeq ($$($1_SRC), ) General note: There amount of duplication between SetupJdkLibrary and SetupJdkExecutable is quickly becoming staggering... When I'm done with this cleanup, I'll unify them. - PR Comment: https://git.openjdk.org/jdk/pull/18457#issuecomment-2016208913 PR Review Comment: https://git.openjdk.org/jdk/pull/18457#discussion_r1535919753
OpenJDK 21 Build on MacOS Sonoma throwing WARNING: Secure coding is automatically enabled
Dear Team Can you please help me with this? *2023-10-24 12:16:57.027 java[97952:198365] WARNING: Secure coding is automatically enabled for restorable state! However, not on all supported macOS versions of this application. Opt-in to secure coding explicitly by implementing NSApplicationDelegate.applicationSupportsSecureRestorableState:.* I have created OpenJDK 21+35 Build on MacOS 14 Sonoma. I am not able to complete few AWT tests in JCK21 (JT Harness Runtime). Best regards Asif
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 22 Mar 2024 10:29:36 GMT, Robbin Ehn wrote: >>> What is the relevance of SVE support at build time? Should it matter what >>> the build machine is? >> >> My understanding is that we need a compiler that supports >> `-march=armv8-a+sve`; otherwise we can't build the redirect library >> properly. But maybe that is a misunderstanding? > >> > What is the relevance of SVE support at build time? Should it matter what >> > the build machine is? >> >> My understanding is that we need a compiler that supports >> `-march=armv8-a+sve`; otherwise we can't build the redirect library >> properly. But maybe that is a misunderstanding? > > Yes, it should be in gcc 8 and above, and [we seem to require gcc > 10](https://openjdk.org/groups/build/doc/building.html). > @robehn Great! I guess that option goes hand-in-hand with arm_sve.h? If so, > we can remove the complication of the SVE check completely. If we all agree to optimistically integrate the current pr (needs both build time + run time sleef support) first, I can check related things, and do necessary modification/improvement. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2015353690
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 22 Mar 2024 12:42:04 GMT, Magnus Ihse Bursie wrote: > > Ah, it'll only be the redirect library that's compiled with > > -march=armv8-a+sve Forget that. > > But that raises an interesting question. What happens if you try to load a > library compiled with `-march=armv8-a+sve` on a non-SVE system? Is the ELF > flagged to require SVE so it will fail to load? I'm hoping this is the case > -- if so, everything will work as it should in this PR, but I honestly don't > know. (After spending like 10 years working with building, I still discover > things I need to learn...). I think we can handle it, when a jdk built with sve support runs on a non-sve machine, the sve related code will not be executed with the protection of UseSVE vm flag which is detected at runtime startup. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2015348458
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 15 Mar 2024 13:58:05 GMT, Hamlin Li wrote: >> Hi, >> Can you help to review this patch? >> Thanks >> >> This is a continuation of work based on [1] by @XiaohongGong, most work was >> done in that pr. In this new pr, just rebased the code in [1], then added >> some minor changes (renaming of calling method, add libsleef as extra lib in >> CI cross-build on aarch64 in github workflow); I aslo tested the combination >> of following scenarios: >> * at build time >> * with/without sleef >> * with/without sve support >> * at runtime >> * with/without sleef >> * with/without sve support >> >> [1] https://github.com/openjdk/jdk/pull/16234 >> >> ## Regression Test >> * test/jdk/jdk/incubator/vector/ >> * test/hotspot/jtreg/compiler/vectorapi/ >> >> ## Performance Test >> Previously, @XiaohongGong has shared the data: >> https://github.com/openjdk/jdk/pull/16234#issuecomment-1767727028 > > Hamlin Li has updated the pull request incrementally with one additional > commit since the last revision: > > fix jni includes > Some updates: I had a look at the source code for libsleef, and frankly, to > integrate building of libsleef into the OpenJDK build is going to be a major > PITA. :-( They have a cmake based build system, which generates a ton of > native build tools, which preprocess source code and generates output > artifacts (like sleef.h). > > I guess we _could_ do it, but I am terrified about trying to get that to > work, especially for cross-compilation. > > So after seeing this, I think the better solution is to require a > pre-compiled libsleef present in the system. (What I just called the "worse > solution" an hour ago...). I guess we can make the requirement of libsleef > the default value, but allow the user to override this to skip libsleef. At > least then it is an active choice to disable libsleef for your build. > > Or, we could do something like how we used to do for freetype: have a > --with-libsleef-src that points to a checked out version of libsleef, and let > configure build it. In this case we could even build it as a static archive, > so there'd be no need for libvectormath.so to have any external dependencies. > > In any case, we are going to have to consider carefully how to proceed. No worry, seems works for me, I just had a very draft version tested locally. we just need to get the final inline versions of sleef generated by sleef cmake install. A question about the licensing: how long does it take to finish the legal process of the sleef licence? - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2015341870
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 22 Mar 2024 10:29:36 GMT, Robbin Ehn wrote: > > > What is the relevance of SVE support at build time? Should it matter what > > > the build machine is? > > > > > > My understanding is that we need a compiler that supports > > `-march=armv8-a+sve`; otherwise we can't build the redirect library > > properly. But maybe that is a misunderstanding? > > Yes, it should be in gcc 8 and above, and [we seem to require gcc > 10](https://openjdk.org/groups/build/doc/building.html). For ACLE, it's supported in [10.1](https://gcc.gnu.org/gcc-10/changes.html) - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2015334896
Re: RFR: 8328705: GHA: Cross-compilation jobs do not require build JDK [v2]
> Noticed this while fixing other GHA issues. > > We pass build JDK to cross-compilation jobs, which makes them dependent on > Linux-x64 builds. But we only build hotspot in all those jobs, so AFAICS > there is no need for build JDK, and therefore no need for the dependency. > Untying cross-compilation jobs from Linux-x64 builds make GHA more parallel. > > Additional testing: > - [ ] GHA Aleksey Shipilev 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 two additional commits since the last revision: - Merge branch 'master' into JDK-8328705-gha-cross-noneed-build - Fix - Changes: - all: https://git.openjdk.org/jdk/pull/18437/files - new: https://git.openjdk.org/jdk/pull/18437/files/91073362..ab09c4ba Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18437=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18437=00-01 Stats: 52703 lines in 2267 files changed: 7683 ins; 3769 del; 41251 mod Patch: https://git.openjdk.org/jdk/pull/18437.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18437/head:pull/18437 PR: https://git.openjdk.org/jdk/pull/18437
Integrated: 8326960: GHA: RISC-V sysroot cannot be debootstrapped due to ongoing Debian t64 transition
On Thu, 21 Mar 2024 15:33:21 GMT, Aleksey Shipilev wrote: > Debian "unstable" is currently undergoing t64 (hello, Y2038) transition, > which breaks "sid" repo that we use for debootstrapping RISC-V very often. > This is seen across multiple repositories, and requires everyone to look > through GHA failures all the time. This PR tries to fix this: if debootstrap > for "sid" repos fails, the build for that arch would not continue. IMO, it is > a better compromise for current situation. We will flip back to expecting the > debootstrap to complete well after either t64 transition finishes, or we > switch to more stable Debian repo once it graduates. > > Additional testing: > - [x] GHA This pull request has now been integrated. Changeset: f207aa94 Author:Aleksey Shipilev URL: https://git.openjdk.org/jdk/commit/f207aa94f9296932276c2952252b263efc793b3f Stats: 15 lines in 1 file changed: 14 ins; 0 del; 1 mod 8326960: GHA: RISC-V sysroot cannot be debootstrapped due to ongoing Debian t64 transition Reviewed-by: fyang, erikj - PR: https://git.openjdk.org/jdk/pull/18435
Re: RFR: 8326960: GHA: RISC-V sysroot cannot be debootstrapped due to ongoing Debian t64 transition
On Thu, 21 Mar 2024 15:33:21 GMT, Aleksey Shipilev wrote: > Debian "unstable" is currently undergoing t64 (hello, Y2038) transition, > which breaks "sid" repo that we use for debootstrapping RISC-V very often. > This is seen across multiple repositories, and requires everyone to look > through GHA failures all the time. This PR tries to fix this: if debootstrap > for "sid" repos fails, the build for that arch would not continue. IMO, it is > a better compromise for current situation. We will flip back to expecting the > debootstrap to complete well after either t64 transition finishes, or we > switch to more stable Debian repo once it graduates. > > Additional testing: > - [x] GHA And away we go! Thanks Erik. - PR Comment: https://git.openjdk.org/jdk/pull/18435#issuecomment-2015193651
Integrated: 8328680: Introduce JDK_LIB, and clean up module native compilation
On Thu, 21 Mar 2024 11:49:11 GMT, Magnus Ihse Bursie wrote: > This is the first step of several, in which I will clean up the native > compilation code as used by modules. In this first step `java.base`, > `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since > they require more work. The changes in the remaining modules are trivial by > comparison. > > The changes done here are: > > 1) A new argument `JDK_LIB` has been introduced, that is used for linking > with other libraries produced by the JDK build. As a follow-up, this will be > further cleaned up and generalized, but the goal for this change is just to > separate them out from external libraries. > > 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in > alphabetical order. Note that this change will affect the resulting binaries > (since the order libraries are given are stored in the binary), but this > change should only be superficial. (If we have symbol clashes between > libraries, then we have problems on a whole other level...). > > 3) The code has been checked for inconsistencies and style guide errors, and > a common programming style has been applied to all `Lib.gmk` and > `Launcher.gmk` files, making sure that all parts follow best practices. > > This PR will be followed up by invidual PRs for the modules requiring not > jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and > `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across > platforms, and automatically apply proper dependencies. This pull request has now been integrated. Changeset: e80619a0 Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/e80619a032c16d16de6e063e7650b60bc317ee7e Stats: 413 lines in 41 files changed: 211 ins; 73 del; 129 mod 8328680: Introduce JDK_LIB, and clean up module native compilation Reviewed-by: erikj, jwaters - PR: https://git.openjdk.org/jdk/pull/18430
Re: RFR: 8328614: hsdis: dlsym can't find decode symbol [v2]
On Thu, 21 Mar 2024 06:58:43 GMT, Robbin Ehn wrote: >> Hi, please consider. >> >> [8327045](https://bugs.openjdk.org/browse/JDK-8327045) hide these symbols. >> Tested with gcc and clang, and llvm and binutils backend. >> >> I didn't find any use of the "DLL_ENTRY", so I removed it. >> >> Thanks, Robbin > > Robbin Ehn has updated the pull request incrementally with one additional > commit since the last revision: > > remove swap file Unless you start calling JDK functions, you will not make a program less stand-alone by including jni.h. In this case, you will only use a compile-time definition. Ideally, we should have had more general EXPORT definitions separate from the rest of the JNI code, but someone started doing things that way, and well, here we are, 25 years later and now JNIEXPORT is everywhere in the JDK source base. :( I'd say that access to JNIEXPORT is about 50% of the reason jni.h is included... - PR Comment: https://git.openjdk.org/jdk/pull/18400#issuecomment-2015191729
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18430#pullrequestreview-1954897229
Re: RFR: 8326960: GHA: RISC-V sysroot cannot be debootstrapped due to ongoing Debian t64 transition
On Thu, 21 Mar 2024 15:33:21 GMT, Aleksey Shipilev wrote: > Debian "unstable" is currently undergoing t64 (hello, Y2038) transition, > which breaks "sid" repo that we use for debootstrapping RISC-V very often. > This is seen across multiple repositories, and requires everyone to look > through GHA failures all the time. This PR tries to fix this: if debootstrap > for "sid" repos fails, the build for that arch would not continue. IMO, it is > a better compromise for current situation. We will flip back to expecting the > debootstrap to complete well after either t64 transition finishes, or we > switch to more stable Debian repo once it graduates. > > Additional testing: > - [x] GHA Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18435#pullrequestreview-1954886889
Re: RFR: 8328614: hsdis: dlsym can't find decode symbol [v2]
On Fri, 22 Mar 2024 11:43:34 GMT, Magnus Ihse Bursie wrote: > > is this how you want us to export these symbols? > > Close but no cigar. :-) > > Use `JNIEXPORT` instead, that is properly defined for this purpose and works > on all compilers. You will need to also add: > > ``` > #include "jni.h" > ``` > > If this is not picked up correctly, let me know and I'll help you get the > include paths correctly in the build. It's stand alone library, should we really make it dependent on the JDK? - PR Comment: https://git.openjdk.org/jdk/pull/18400#issuecomment-2015129906
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 22 Mar 2024 12:31:53 GMT, Andrew Haley wrote: > Ah, it'll only be the redirect library that's compiled with > -march=armv8-a+sve Forget that. But that raises an interesting question. What happens if you try to load a library compiled with `-march=armv8-a+sve` on a non-SVE system? Is the ELF flagged to require SVE so it will fail to load? I'm hoping this is the case -- if so, everything will work as it should in this PR, but I honestly don't know. (After spending like 10 years working with building, I still discover things I need to learn...). - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2015020065
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 22 Mar 2024 10:29:36 GMT, Robbin Ehn wrote: >>> What is the relevance of SVE support at build time? Should it matter what >>> the build machine is? >> >> My understanding is that we need a compiler that supports >> `-march=armv8-a+sve`; otherwise we can't build the redirect library >> properly. But maybe that is a misunderstanding? > >> > What is the relevance of SVE support at build time? Should it matter what >> > the build machine is? >> >> My understanding is that we need a compiler that supports >> `-march=armv8-a+sve`; otherwise we can't build the redirect library >> properly. But maybe that is a misunderstanding? > > Yes, it should be in gcc 8 and above, and [we seem to require gcc > 10](https://openjdk.org/groups/build/doc/building.html). @robehn Great! I guess that option goes hand-in-hand with arm_sve.h? If so, we can remove the complication of the SVE check completely. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2015010158
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Fri, 22 Mar 2024 10:49:13 GMT, Julian Waters wrote: >> bot-keep-alive >> >> @TheShermanTanker Did you understand the remaining changes that Phil has >> requested? Do you think you'll be able to fix this? > > @magicus Phil doesn't seem to be responding to my queries, I'm not too sure > what to do at this point @TheShermanTanker Phil is a busy man. I've helped you out (hopefully!) by being more concrete about what you need to fix, and reminded you on fixes that you seemed to have missed. Also, please use the functionality to resolve conversations, it really helps to see what remains to be fixed. (In OpenJDK, the Github project is configured so only the owner of a PR can resolve conversations.) Also, you can double check the PR yourself to see if you have introduced any changes in formatting that is not required by the change. Phil has made clear that he does not want any such changes in this PR, and you need to respect that. Finally, Phil has left a few comments more that I have not touched upon, so this is not all that needs to be fixed. (I left them out because I too had a hard time understanding exactly what he meant.) But if you start with these, it will be easier to see what remains. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2015000697
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Sat, 20 Jan 2024 00:40:40 GMT, Phil Race wrote:
>> Julian Waters has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 79 commits:
>>
>> - Merge branch 'openjdk:master' into patch-10
>> - Merge branch 'openjdk:master' into patch-10
>> - Fix awt_Window.cpp
>> - Fix awt_PrintJob.cpp
>> - -Zc:stringStrings no longer needed with -permissive- flags-cflags.m4
>> - Fix awt_Window.cpp
>> - awt_Window.cpp
>> - awt_PrintJob.cpp
>> - awt_Frame.cpp
>> - Whitespace awt_Component.cpp
>> - ... and 69 more: https://git.openjdk.org/jdk/compare/35e96627...cbfbaee6
>
> src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3308:
>
>> 3306: return;
>> 3307: } else {
>> 3308: pData = JNI_GET_PDATA(self);
>
> set directly
@TheShermanTanker What this means is that you need to replace the line:
PDATA pData;
above with:
AwtWindow *window;
and remove
AwtWindow *window = (AwtWindow *)pData;
below, and then change all references to `pData` in this block to `window`.
The same changes needs to be done in all places where pData is references. This
was a temporary variable that is no longer needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535500631
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 22 Mar 2024 12:31:01 GMT, Andrew Haley wrote: > > > What is the relevance of SVE support at build time? Should it matter what > > > the build machine is? > > > > > > My understanding is that we need a compiler that supports > > `-march=armv8-a+sve`; otherwise we can't build the redirect library > > properly. But maybe that is a misunderstanding? > > I hope not, otherwise the resulting binary won't run on most systems. Ah, it'll only be the redirect library that's compiled with `-march=armv8-a+sve` Forget that. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2014982742
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 15 Mar 2024 13:58:05 GMT, Hamlin Li wrote: >> Hi, >> Can you help to review this patch? >> Thanks >> >> This is a continuation of work based on [1] by @XiaohongGong, most work was >> done in that pr. In this new pr, just rebased the code in [1], then added >> some minor changes (renaming of calling method, add libsleef as extra lib in >> CI cross-build on aarch64 in github workflow); I aslo tested the combination >> of following scenarios: >> * at build time >> * with/without sleef >> * with/without sve support >> * at runtime >> * with/without sleef >> * with/without sve support >> >> [1] https://github.com/openjdk/jdk/pull/16234 >> >> ## Regression Test >> * test/jdk/jdk/incubator/vector/ >> * test/hotspot/jtreg/compiler/vectorapi/ >> >> ## Performance Test >> Previously, @XiaohongGong has shared the data: >> https://github.com/openjdk/jdk/pull/16234#issuecomment-1767727028 > > Hamlin Li has updated the pull request incrementally with one additional > commit since the last revision: > > fix jni includes > > > What is the relevance of SVE support at build time? Should it matter what > > > the build machine is? > > > > > > My understanding is that we need a compiler that supports > > `-march=armv8-a+sve`; otherwise we can't build the redirect library > > properly. But maybe that is a misunderstanding? > > I hope not, otherwise the resulting binary won't run on most systems. I can write you a redirect library that doesn't need any magic compiler options. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2014979694
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v14]
On Fri, 3 Nov 2023 02:39:26 GMT, Julian Waters wrote: >> src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 61: >> >>> 59: >>> 60: jfieldID AwtPrintDialog::pageID; >>> 61: >> >> where and why did this come from ? > > This came from below, all I did was move it up and out of the extern "C" > block. This cannot be inside extern "C" because it is a static class member > and has permanent C++ name mangling The question seems answered. Please resolve this discussion. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535492715
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Sat, 20 Jan 2024 00:32:19 GMT, Phil Race wrote:
>> Julian Waters has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 79 commits:
>>
>> - Merge branch 'openjdk:master' into patch-10
>> - Merge branch 'openjdk:master' into patch-10
>> - Fix awt_Window.cpp
>> - Fix awt_PrintJob.cpp
>> - -Zc:stringStrings no longer needed with -permissive- flags-cflags.m4
>> - Fix awt_Window.cpp
>> - awt_Window.cpp
>> - awt_PrintJob.cpp
>> - awt_Frame.cpp
>> - Whitespace awt_Component.cpp
>> - ... and 69 more: https://git.openjdk.org/jdk/compare/35e96627...cbfbaee6
>
> src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1365:
>
>> 1363: f = (AwtFrame *) pData;
>> 1364: HWND hwnd = f->GetHWnd();
>> 1365: if (::IsWindow(hwnd)) {
>
> more formatting to revert
Suggestion:
if (::IsWindow(hwnd))
{
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535491578
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v2]
On Sun, 3 Dec 2023 15:37:47 GMT, Julian Waters wrote: >> src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1641: >> >>> 1639: } >>> 1640: } >>> 1641: >> >> A possible improvement later (and for a future RFE) would be to use RAII for >> deletion and then get rid of the labels. awt is one of the few places that >> uses C++ for native code, so why not. > > Phil unfortunately rejected that approach, so we're on to a more manual way > of deleting things here The comment seems answered. Please resolve this discussion. >> src/java.desktop/windows/native/libawt/windows/awt_TextComponent.cpp line 59: >> >>> 57: AwtTextComponent::OleCallback AwtTextComponent::sm_oleCallback; >>> 58: WNDPROC AwtTextComponent::sm_pDefWindowProc = NULL; >>> 59: >> >> Did the compiler complain here? I'm fine with the change, just wanted to >> know the reason. > > the latter two are inside an extern "C" block, meaning their initial C++ > linkage (by virtue of them being static class members) conflicts with the now > C linkage, also the comment there states the AwtComponent fields are supposed > to be set here, and I have no idea why this was not done, so I moved them all > to be under that comment The question seem answered. Please resolve the discussion. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535492864 PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535495754
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Wed, 20 Mar 2024 06:38:59 GMT, Julian Waters wrote:
>> We should set the -permissive- flag for the Microsoft Visual C compiler, as
>> was requested by the now backed out
>> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes
>> the Visual C compiler much less accepting of ill formed code, which will
>> improve code quality on Windows in the future.
>
> Julian Waters has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Revert Formatting in awt_Component.cpp
> - Revert Formatting in awt_Window.cpp
src/java.desktop/windows/native/libawt/windows/awt_DnDDS.cpp line 44:
> 42: #include
> 43:
> 44: // Don't want to pull in the redefined allocation functions
Maybe this needs to be clarified a bit:
Suggestion:
// These files must be included before awt.h, since the latter redefines malloc
// to Do_Not_Use_Malloc, etc, and that will break these files.
Also, please mark conversations as resolved in the GitHub UI once you think
they are addressed. That makes it much easier for reviewers to understand what
parts of the reviews that have not been addressed.
src/java.desktop/windows/native/libawt/windows/awt_DnDDT.cpp line 31:
> 29:
> 30: // Don't want to pull in the redefined allocation functions
> 31: #include "awt_ole.h"
Suggestion:
// awt_ole.h must be included before awt.h, since the latter redefines malloc
// to Do_Not_Use_Malloc, etc, and that will break awt_ole.h.
#include "awt_ole.h"
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 544:
> 542: and for all other dialogs AwtToolkit's HWND is used.
> 543: */
> 544: else if (awtParent != NULL) {
Suggestion:
else if (awtParent != NULL)
{
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 546:
> 544: else if (awtParent != NULL) {
> 545: setup.hwndOwner = AwtToolkit::GetInstance().GetHWnd();
> 546: } else {
Suggestion:
}
else
{
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 779:
> 777: }
> 778:
> 779:
Suggestion:
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 1230:
> 1228: jdouble imgY = (jdouble) ((yPixelOrg * 72)/(jdouble) yPixelRes);
> 1229: jdouble imgWid = (jdouble) ((imgPixelWid * 72)/(jdouble) xPixelRes);
> 1230: jdouble imgHgt = (jdouble) ((imgPixelHgt * 72)/(jdouble) yPixelRes);
Suggestion:
jdouble imgX = (jdouble)((xPixelOrg * 72)/(jdouble)xPixelRes);
jdouble imgY = (jdouble)((yPixelOrg * 72)/(jdouble)yPixelRes);
jdouble imgWid = (jdouble)((imgPixelWid * 72)/(jdouble)xPixelRes);
jdouble imgHgt = (jdouble)((imgPixelHgt * 72)/(jdouble)yPixelRes);```
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 1316:
> 1314: env->CallVoidMethod(newPaper, setImageableID, ix, iy, iw, ih);
> 1315:
> 1316:
Suggestion:
src/java.desktop/windows/native/libawt/windows/awt_Window.cpp line 3153:
> 3151: }
> 3152:
> 3153: window = (AwtWindow *) pData;
Suggestion:
window = (AwtWindow *)pData;
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535490288
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535491034
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535493157
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535493362
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535493818
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535494973
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535495291
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1535496714
Re: RFR: 8327476: Upgrade JLine to 3.25.1 [v4]
On Wed, 20 Mar 2024 18:55:38 GMT, Jan Lahoda wrote: >> This is a patch that: >> a) upgrades the JLine inside the JDK to 3.25.1 >> b) since the new version of JLine has a FFM backend, our custom native >> backends are removed, and replaced with the FFM backend >> >> Some changes had to be made to the original JLine in order to fit into the >> JDK. Most of them were already done for the previous version (repackaging, >> avoiding non-ASCII characters, commenting out logging, adding ability to >> modify to wrap the InputStream used by the terminal), and have only been >> transferred to the new one. The main two new changes are: >> - fixes to the FFM backend, so that it works on Linux and JDK 22. These have >> been proposed to JLine itself: https://github.com/jline/jline3/pull/945 >> - disabling the `NativeFileDescriptorCreator`, as I believe we don't need >> it, and cannot make it work easily >> >> There's a full patch between the >> `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content >> of the corresponding sources of these original JLine sub-projects: >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm >> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal >> the patch is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff >> >> I've also cleaned the patch a little removing most of the changes for the >> rename. The result is here: >> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 26 commits: > > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'master' into jline-upgrade-3.25.1 > - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 > - Merge remote-tracking branch 'origin/native-access-modules1' into > native-access-modules1 > - Apply suggestions from code review > >Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> > - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1 > - Reflecting review feedback. > - Updating copyright headers. > - Re-enabling the exec provider. > - Cleanup. > - ... and 16 more: https://git.openjdk.org/jdk/compare/e5e7cd20...c8097592 Aha, now I understand! The new files in `jdk.internal.org.jline.terminal.impl.ffm` comes from upstream. I thought they were additions made by you. I guess that means my comments are still valid, but should be asked upstream instead. And that you don't need to address them here. For security reasons, though, I still recommend you remove the generic library loading part. JLine might think they can have such functionality (even if it is crazy), but we can't ship it as part of the JDK. - PR Comment: https://git.openjdk.org/jdk/pull/18142#issuecomment-2014944780
Re: RFR: 8327476: Upgrade JLine to 3.25.1 [v4]
On Wed, 20 Mar 2024 18:55:38 GMT, Jan Lahoda wrote:
>> This is a patch that:
>> a) upgrades the JLine inside the JDK to 3.25.1
>> b) since the new version of JLine has a FFM backend, our custom native
>> backends are removed, and replaced with the FFM backend
>>
>> Some changes had to be made to the original JLine in order to fit into the
>> JDK. Most of them were already done for the previous version (repackaging,
>> avoiding non-ASCII characters, commenting out logging, adding ability to
>> modify to wrap the InputStream used by the terminal), and have only been
>> transferred to the new one. The main two new changes are:
>> - fixes to the FFM backend, so that it works on Linux and JDK 22. These have
>> been proposed to JLine itself: https://github.com/jline/jline3/pull/945
>> - disabling the `NativeFileDescriptorCreator`, as I believe we don't need
>> it, and cannot make it work easily
>>
>> There's a full patch between the
>> `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content
>> of the corresponding sources of these original JLine sub-projects:
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal
>> the patch is here:
>> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff
>>
>> I've also cleaned the patch a little removing most of the changes for the
>> rename. The result is here:
>> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 26 commits:
>
> - Merge branch 'master' into jline-upgrade-3.25.1
> - Merge branch 'master' into jline-upgrade-3.25.1
> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1
> - Merge remote-tracking branch 'origin/native-access-modules1' into
> native-access-modules1
> - Apply suggestions from code review
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1
> - Reflecting review feedback.
> - Updating copyright headers.
> - Re-enabling the exec provider.
> - Cleanup.
> - ... and 16 more: https://git.openjdk.org/jdk/compare/e5e7cd20...c8097592
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java
line 517:
> 515: String hwName;
> 516: try {
> 517: Process p = Runtime.getRuntime().exec(new String[]
> {"uname", "-m"});
This seems very much adhoc. What systems have you tested this on?
If this is supposed to be a general method of locating a library in well-known
locations on different Linux systems, maybe it needs to be elevated to a more
prominent place where it can be shared by other parts of the JDK that might
want to do the same.
It seems like this is a bit of missing "glue" code from FFM -- how to actually
locate a system library. I understand that it is not part of FFM proper, nor
jextract, but I guess this is something that will need to be reimplemented many
times, unless some common functionality is provided. We can perhaps not do that
to save the world, but we can at least do that for the benefit of the JDK code,
especially as I see FFM replacing JNI more and more, going forward.
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java
line 832:
> 830: TCSAFLUSH = 0x2;
> 831:
> 832: VINTR = 0;
I'm just curious: How did you arrive at these values? They seem like they are
generated somehow. (I guess you did not just make them up and write them down
here). Can that process be documented? Or saved as a script in case they ever
need to be updated?
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/Kernel32.java
line 20:
> 18: final class Kernel32 {
> 19:
> 20: public static final int FORMAT_MESSAGE_FROM_SYSTEM = 0x1000;
Here is another bunch of constants that seem generated. Is this file created by
jextract? (I'm sorry, I have not played around enough with jextract to
recognize the output it generates).
In any way, some documentation on how this file was created, if any kind of
automation was involved, would be nice.
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/Kernel32.java
line 321:
> 319: static {
> 320: System.loadLibrary("msvcrt");
> 321: System.loadLibrary("Kernel32");
For consistency:
Suggestion:
System.loadLibrary("kernel32");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535466266
PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535468034
PR Review Comment:
Re: RFR: 8327476: Upgrade JLine to 3.25.1 [v4]
On Wed, 20 Mar 2024 18:55:38 GMT, Jan Lahoda wrote:
>> This is a patch that:
>> a) upgrades the JLine inside the JDK to 3.25.1
>> b) since the new version of JLine has a FFM backend, our custom native
>> backends are removed, and replaced with the FFM backend
>>
>> Some changes had to be made to the original JLine in order to fit into the
>> JDK. Most of them were already done for the previous version (repackaging,
>> avoiding non-ASCII characters, commenting out logging, adding ability to
>> modify to wrap the InputStream used by the terminal), and have only been
>> transferred to the new one. The main two new changes are:
>> - fixes to the FFM backend, so that it works on Linux and JDK 22. These have
>> been proposed to JLine itself: https://github.com/jline/jline3/pull/945
>> - disabling the `NativeFileDescriptorCreator`, as I believe we don't need
>> it, and cannot make it work easily
>>
>> There's a full patch between the
>> `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content
>> of the corresponding sources of these original JLine sub-projects:
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal
>> the patch is here:
>> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff
>>
>> I've also cleaned the patch a little removing most of the changes for the
>> rename. The result is here:
>> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 26 commits:
>
> - Merge branch 'master' into jline-upgrade-3.25.1
> - Merge branch 'master' into jline-upgrade-3.25.1
> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1
> - Merge remote-tracking branch 'origin/native-access-modules1' into
> native-access-modules1
> - Apply suggestions from code review
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1
> - Reflecting review feedback.
> - Updating copyright headers.
> - Re-enabling the exec provider.
> - Cleanup.
> - ... and 16 more: https://git.openjdk.org/jdk/compare/e5e7cd20...c8097592
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java
line 497:
> 495: sb.append("Unable to find openpty native method in static
> libraries and unable to load the util library.");
> 496: List suppressed = new ArrayList<>();
> 497: try {
This entire section trying to load libutil is quite complex. For a start, I'd
recommend you break it out into a separate method `loadLibUtil` or something
like that, to make this clearer.
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java
line 504:
> 502: }
> 503: if (openPtyAddr.isEmpty()) {
> 504: String libUtilPath =
> System.getProperty("jdk.internal.org.jline.ffm.libutil");
Wait a minute. If I get this right, then you can pass
`-Djdk.internal.org.jline.ffm.libutil=mydodgylib.so` as command line, and have
the JDK load any arbitrary library? That sounds super bad from a security
perspective. I recommend you remove this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535461403
PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535462748
Re: RFR: 8327476: Upgrade JLine to 3.25.1 [v4]
On Wed, 20 Mar 2024 18:55:38 GMT, Jan Lahoda wrote:
>> This is a patch that:
>> a) upgrades the JLine inside the JDK to 3.25.1
>> b) since the new version of JLine has a FFM backend, our custom native
>> backends are removed, and replaced with the FFM backend
>>
>> Some changes had to be made to the original JLine in order to fit into the
>> JDK. Most of them were already done for the previous version (repackaging,
>> avoiding non-ASCII characters, commenting out logging, adding ability to
>> modify to wrap the InputStream used by the terminal), and have only been
>> transferred to the new one. The main two new changes are:
>> - fixes to the FFM backend, so that it works on Linux and JDK 22. These have
>> been proposed to JLine itself: https://github.com/jline/jline3/pull/945
>> - disabling the `NativeFileDescriptorCreator`, as I believe we don't need
>> it, and cannot make it work easily
>>
>> There's a full patch between the
>> `src/jdk.internal.le/share/classes/jdk/internal/org` and the merged content
>> of the corresponding sources of these original JLine sub-projects:
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/reader
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal-ffm
>> https://github.com/jline/jline3/tree/jline-parent-3.25.1/terminal
>> the patch is here:
>> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade.diff
>>
>> I've also cleaned the patch a little removing most of the changes for the
>> rename. The result is here:
>> https://cr.openjdk.org/~jlahoda/8327476/jline-3.25.1-upgrade-significant.diff
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 26 commits:
>
> - Merge branch 'master' into jline-upgrade-3.25.1
> - Merge branch 'master' into jline-upgrade-3.25.1
> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1
> - Merge remote-tracking branch 'origin/native-access-modules1' into
> native-access-modules1
> - Apply suggestions from code review
>
>Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
>Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
> - Merge branch 'native-access-modules1' into jline-upgrade-3.25.1
> - Reflecting review feedback.
> - Updating copyright headers.
> - Re-enabling the exec provider.
> - Cleanup.
> - ... and 16 more: https://git.openjdk.org/jdk/compare/e5e7cd20...c8097592
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java
line 27:
> 25: import java.util.List;
> 26: import java.util.Optional;
> 27: //import java.util.logging.Level;
Maybe remove?
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java
line 41:
> 39: class CLibrary {
> 40:
> 41: //private static final Logger logger = Logger.getLogger("org.jline");
More commented-out code...
src/jdk.internal.le/share/classes/jdk/internal/org/jline/terminal/impl/ffm/CLibrary.java
line 549:
> 547: error = new LinkageError(sb.toString());
> 548: suppressed.forEach(error::addSuppressed);
> 549: //if (logger.isLoggable(Level.FINE)) {
More commented out code
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535455104
PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535455382
PR Review Comment: https://git.openjdk.org/jdk/pull/18142#discussion_r1535458026
Re: RFR: 8328614: hsdis: dlsym can't find decode symbol [v2]
On Thu, 21 Mar 2024 06:58:43 GMT, Robbin Ehn wrote: >> Hi, please consider. >> >> [8327045](https://bugs.openjdk.org/browse/JDK-8327045) hide these symbols. >> Tested with gcc and clang, and llvm and binutils backend. >> >> I didn't find any use of the "DLL_ENTRY", so I removed it. >> >> Thanks, Robbin > > Robbin Ehn has updated the pull request incrementally with one additional > commit since the last revision: > > remove swap file Also, apologies for forgetting to check hsdis when I changed the visibility. :-( - PR Comment: https://git.openjdk.org/jdk/pull/18400#issuecomment-2014910765
Re: RFR: 8328614: hsdis: dlsym can't find decode symbol [v2]
On Fri, 22 Mar 2024 10:25:33 GMT, Robbin Ehn wrote: > is this how you want us to export these symbols? Close but no cigar. :-) Use `JNIEXPORT` instead, that is properly defined for this purpose and works on all compilers. You will need to also add: #include "jni.h" If this is not picked up correctly, let me know and I'll help you get the include paths correctly in the build. - PR Comment: https://git.openjdk.org/jdk/pull/18400#issuecomment-2014907958
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 11:38:25 GMT, Magnus Ihse Bursie wrote: > I can give a spoiler to what the upcoming JDK_LIBS rewrite will do. > > Currently, if you want to link with e.g. `jli`, this is what you need to do: > > ``` > $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ > NAME := mylib, \ > EXTRA_HEADER_DIRS := java.base:libjli, \ > LDFLAGS_linux := -L$(call FindLibDirForModule, java.base), \ > LDFLAGS_macosx := -L$(call FindLibDirForModule, java.base), \ > JDK_LIBS_linux := -ljli, \ > JDK_LIBS_macosx := -ljli, \ > JDK_LIBS_windows := $(SUPPORT_OUTPUTDIR)/native/java.base/libjli/jli.lib, > \ > )) > > $(BUILD_LIBMYLIB): $(call FindLib, java.base, jli) > ``` > > This is cumbersome, and easy to forget (we almost never get it 100% right in > any place...). > > I intend to replace it with the following, and to have the build system > generate the boiler plate automatically: > > ``` > $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ > NAME := mylib, \ > JDK_LIBS := jli, \ > )) > ``` I see, that's a definite +1 for me - PR Comment: https://git.openjdk.org/jdk/pull/18430#issuecomment-2014905990
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation I can give a spoiler to what the upcoming JDK_LIBS rewrite will do. Currently, if you want to link with e.g. `jli`, this is what you need to do: $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ NAME := mylib, \ EXTRA_HEADER_DIRS := java.base:libjli, \ LDFLAGS_linux := -L$(call FindLibDirForModule, java.base), \ LDFLAGS_macosx := -L$(call FindLibDirForModule, java.base), \ JDK_LIBS_linux := -ljli, \ JDK_LIBS_macosx := -ljli, \ JDK_LIBS_windows := $(SUPPORT_OUTPUTDIR)/native/java.base/libjli/jli.lib, \ )) $(BUILD_LIBMYLIB): $(call FindLib, java.base, jli) This is cumbersome, and easy to forget (we almost never get it 100% right in any place...). I intend to replace it with the following, and to have the build system generate the boiler plate automatically: $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ NAME := mylib, \ JDK_LIBS := jli, \ )) - PR Comment: https://git.openjdk.org/jdk/pull/18430#issuecomment-2014900723
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:47:30 GMT, Julian Waters wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix indentation > > make/common/JdkNativeCompilation.gmk line 190: > >> 188: $1_SRC_HEADER_FLAGS += $$(addprefix -I, $$(call GetJavaHeaderDir, >> $$(MODULE))) >> 189: >> 190: $1_JDK_LIBS += $$($1_JDK_LIBS_$$(OPENJDK_TARGET_OS)) > > Should these follow LIBS and pick up more specific target options as well? > Not a request, just thinking out loud I'm not sure what targets you are thinking of here. This is one of the cases where the distinction between OS and toolchain is not entirely clear. I'm treating them as synonyms. There are two reasons two separate target OS here: 1) syntax in linker. The microsoft linker uses `jvm.lib`, all others `-ljvm`. 2) different needs in the source code. When there are vastly different implementations for a lib on different OSes, we can have a situation where e.g. the macOS port needs libjava, but the other platforms don't. Issue 1) will be resolved by the upcoming JDK_LIBS rewrite. Issue 2 is inherited from the limitations in the source code, and since that source code is structured on a per-OS basis, we will only need to have JDK_LIBS options per-OS. - PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535437421
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:43:40 GMT, Julian Waters wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix indentation > > make/autoconf/libraries.m4 line 174: > >> 172: >> 173: JDKLIB_LIBS="$BASIC_JDKLIB_LIBS" >> 174: JDKEXE_LIBS="" > > This is passed to LauncherCommon while JDKLIB_LIBS is manually passed to > every individual library it's used for. Why not unify them and pass > JDKLIB_LIBS to LibCommon instead? Other than that seems good JDKLIB_LIBS will be removed when I finish up the JDK_LIBS patch. (It is currently "-ljava -ljvm" on non-Windows, so basically a shortcut for setting dependencies on some commonly used JDK libraries.) JDKEXE_LIBS was just dead code, so I removed it "eagerly" in this patch. - PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535433200
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 15 Mar 2024 13:58:05 GMT, Hamlin Li wrote: >> Hi, >> Can you help to review this patch? >> Thanks >> >> This is a continuation of work based on [1] by @XiaohongGong, most work was >> done in that pr. In this new pr, just rebased the code in [1], then added >> some minor changes (renaming of calling method, add libsleef as extra lib in >> CI cross-build on aarch64 in github workflow); I aslo tested the combination >> of following scenarios: >> * at build time >> * with/without sleef >> * with/without sve support >> * at runtime >> * with/without sleef >> * with/without sve support >> >> [1] https://github.com/openjdk/jdk/pull/16234 >> >> ## Regression Test >> * test/jdk/jdk/incubator/vector/ >> * test/hotspot/jtreg/compiler/vectorapi/ >> >> ## Performance Test >> Previously, @XiaohongGong has shared the data: >> https://github.com/openjdk/jdk/pull/16234#issuecomment-1767727028 > > Hamlin Li has updated the pull request incrementally with one additional > commit since the last revision: > > fix jni includes Some updates: I had a look at the source code for libsleef, and frankly, to integrate building of libsleef into the OpenJDK build is going to be a major PITA. :-( They have a cmake based build system, which generates a ton of native build tools, which preprocess source code and generates output artifacts (like sleef.h). I guess we *could* do it, but I am terrified about trying to get that to work, especially for cross-compilation. So after seeing this, I think the better solution is to require a pre-compiled libsleef present in the system. (What I just called the "worse solution" an hour ago...). I guess we can make the requirement of libsleef the default value, but allow the user to override this to skip libsleef. At least then it is an active choice to disable libsleef for your build. Or, we could do something like how we used to do for freetype: have a --with-libsleef-src that points to a checked out version of libsleef, and let configure build it. In this case we could even build it as a static archive, so there'd be no need for libvectormath.so to have any external dependencies. In any case, we are going to have to consider carefully how to proceed. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2014876520
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Mon, 18 Mar 2024 15:55:15 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 84 commits: >> >> - Merge branch 'openjdk:master' into patch-10 >> - awt_Window.cpp >> - awt_Frame.cpp >> - awt_Component.cpp >> - awt_Canvas.cpp >> - Merge branch 'openjdk:master' into patch-10 >> - Merge branch 'openjdk:master' into patch-10 >> - Fix awt_Window.cpp >> - Fix awt_PrintJob.cpp >> - -Zc:stringStrings no longer needed with -permissive- flags-cflags.m4 >> - ... and 74 more: https://git.openjdk.org/jdk/compare/a474b372...0f34608b > > bot-keep-alive > > @TheShermanTanker Did you understand the remaining changes that Phil has > requested? Do you think you'll be able to fix this? @magicus Phil doesn't seem to be responding to my queries, I'm not too sure what to do at this point - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2014826062
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation make/common/JdkNativeCompilation.gmk line 190: > 188: $1_SRC_HEADER_FLAGS += $$(addprefix -I, $$(call GetJavaHeaderDir, > $$(MODULE))) > 189: > 190: $1_JDK_LIBS += $$($1_JDK_LIBS_$$(OPENJDK_TARGET_OS)) Should these follow LIBS and pick up more specific target options as well? Not a request, just thinking out loud - PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535391514
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation I had doubts about introducing JDK_LIB until I saw there were plans to further improve upon it, so I guess my uncertainty is now answered make/autoconf/libraries.m4 line 174: > 172: > 173: JDKLIB_LIBS="$BASIC_JDKLIB_LIBS" > 174: JDKEXE_LIBS="" This is passed to LauncherCommon while JDKLIB_LIBS is manually passed to every individual library it's used for. Why not unify them and pass JDKLIB_LIBS to LibCommon instead? Other than that seems good - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18430#pullrequestreview-1954491578 PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535387243
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 22 Mar 2024 08:51:47 GMT, Andrew Haley wrote: >> @theRealAph Are you saying that bundling the source code of libsleef is a >> hard requirement from your side to accept this code into the JDK? >> >> I'm not against it, I just want to understand what we're talking about here. >> >> In general, adding new libraries to OpenJDK will require a legal process in >> Oracle, which may (or may not) take some amount of time T, where T is larger >> than you'd wish for. >> >> So I guess we can either: >> 1) wait for libsleef source code to become a part of OpenJDK, and then >> integrate this PR. >> 2) integrate this PR optimistically, and in the background start a process >> of trying to get libsleef into OpenJDK. (Which, of course, can not be 100% >> guaranteed to happen.) >> 3) integrate this PR as is, and give up any idea of bundling libsleef. >> >> I also believe there is a fourth option, but that too seems like it has >> legal implications that needs to be checked: >> >> 4) if the libsleef dynamic library is found on the system during build time, >> bundle a copy of the dll with the built JDK. (Similar to how was done with >> freetype on Windows before.). And, optionally, provide an option for >> configure to require libsleef to be present, so the build fails if no >> libsleef can be found and bundled. (Leaving open as to if this should be >> default or not.) > >> @magicus @theRealAph Thanks for discussion. >> > In the short term, I'd build a shim library during the default standard >> > JDK build that does not need SLEEF at build time. Unless weo do that, >> > because SLEEF isn't on anyone's builders, it won't be used. >> >> I agree it's give user more chance to leverage the sleef when running, but I >> wonder if it's necessary to do that. As we have a long term solution, and >> the chance that end user lacks sleef library in their environment is much >> higher than release engineer lacks sleef library in their environment. > > That situation leads to my nightmare, in which some OpenJDK builds work with > vector support, and some don't, and there's no way to find out which unless > you try it, in which case there is no warning, your programs just run slowly. > Even if you've installed SLEEF. That would be bad for our users, bad for > OpenJDK, and bad for Java. > >> But it's not harm to do so. @theRealAph An alternative to building libsleef is to *require* it as a build prerequisite on linux/aarch64. That way you will at least not end up in the situation you fear, where no build environment for the popular distributions have libsleef during build time, and in practice no end user will benefit either. Otoh, doing that will require us to send out the message for downstream builders to prepare, and waiting for them to prepare, and the question is if this is faster than bundling the source code... And if we're going the latter route anyway, then this approach will have been pointless. So, I guess, this is a worse solution. Just wanted to point out that it exists. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2014795185
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 22 Mar 2024 10:22:54 GMT, Magnus Ihse Bursie wrote: > > What is the relevance of SVE support at build time? Should it matter what > > the build machine is? > > My understanding is that we need a compiler that supports > `-march=armv8-a+sve`; otherwise we can't build the redirect library properly. > But maybe that is a misunderstanding? Yes, it should be in gcc 8 and above, and [we seem to require gcc 10](https://openjdk.org/groups/build/doc/building.html). - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2014793399
Re: RFR: 8328614: hsdis: dlsym can't find decode symbol [v2]
On Thu, 21 Mar 2024 06:58:43 GMT, Robbin Ehn wrote: >> Hi, please consider. >> >> [8327045](https://bugs.openjdk.org/browse/JDK-8327045) hide these symbols. >> Tested with gcc and clang, and llvm and binutils backend. >> >> I didn't find any use of the "DLL_ENTRY", so I removed it. >> >> Thanks, Robbin > > Robbin Ehn has updated the pull request incrementally with one additional > commit since the last revision: > > remove swap file @magicus is this how you want us to export these symbols? - PR Comment: https://git.openjdk.org/jdk/pull/18400#issuecomment-2014786824
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 15 Mar 2024 16:58:06 GMT, Andrew Haley wrote: > What is the relevance of SVE support at build time? Should it matter what the > build machine is? My understanding is that we need a compiler that supports `-march=armv8-a+sve`; otherwise we can't build the redirect library properly. But maybe that is a misunderstanding? - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2014782528
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
> This is the first step of several, in which I will clean up the native > compilation code as used by modules. In this first step `java.base`, > `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since > they require more work. The changes in the remaining modules are trivial by > comparison. > > The changes done here are: > > 1) A new argument `JDK_LIB` has been introduced, that is used for linking > with other libraries produced by the JDK build. As a follow-up, this will be > further cleaned up and generalized, but the goal for this change is just to > separate them out from external libraries. > > 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in > alphabetical order. Note that this change will affect the resulting binaries > (since the order libraries are given are stored in the binary), but this > change should only be superficial. (If we have symbol clashes between > libraries, then we have problems on a whole other level...). > > 3) The code has been checked for inconsistencies and style guide errors, and > a common programming style has been applied to all `Lib.gmk` and > `Launcher.gmk` files, making sure that all parts follow best practices. > > This PR will be followed up by invidual PRs for the modules requiring not > jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and > `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across > platforms, and automatically apply proper dependencies. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Fix indentation - Changes: - all: https://git.openjdk.org/jdk/pull/18430/files - new: https://git.openjdk.org/jdk/pull/18430/files/b26b59f1..f68c4a29 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18430=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18430=00-01 Stats: 7 lines in 3 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/18430.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18430/head:pull/18430 PR: https://git.openjdk.org/jdk/pull/18430
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation
On Thu, 21 Mar 2024 17:56:12 GMT, Erik Joelsson wrote: > Looks good, just found some indentation levels not conforming to the > convention and as you are fixing style, I pointed them out. Absolutely! Fixing style was one of the points here. It's funny, really, I've been staring at this code until my eyes started bleeding, and I could still not see these problems. - PR Comment: https://git.openjdk.org/jdk/pull/18430#issuecomment-2014756999
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Thu, 21 Mar 2024 15:43:28 GMT, Magnus Ihse Bursie wrote: >>> > The problem I see is that J. Random Java User has no way to know if SLEEF >>> > is making their program faster without running benchmarks. They'll put >>> > SLEEF somewhere and hope that Java uses it. >>> >>> Please kindly correct me if I misunderstood your points. Seems the safest >>> solution to address your above concerns is to integrate the sleef source >>> into jdk? Lack of sleef at either build time or runtime will make the >>> user's code fall back to java implementation. >> >> Exactly, yes. That's why we've integrated the source code of many other >> libraries we depend on into the JDK. It's the only way to get the >> reliability our users expect. > > @theRealAph Are you saying that bundling the source code of libsleef is a > hard requirement from your side to accept this code into the JDK? > > I'm not against it, I just want to understand what we're talking about here. > > In general, adding new libraries to OpenJDK will require a legal process in > Oracle, which may (or may not) take some amount of time T, where T is larger > than you'd wish for. > > So I guess we can either: > 1) wait for libsleef source code to become a part of OpenJDK, and then > integrate this PR. > 2) integrate this PR optimistically, and in the background start a process of > trying to get libsleef into OpenJDK. (Which, of course, can not be 100% > guaranteed to happen.) > 3) integrate this PR as is, and give up any idea of bundling libsleef. > > I also believe there is a fourth option, but that too seems like it has legal > implications that needs to be checked: > > 4) if the libsleef dynamic library is found on the system during build time, > bundle a copy of the dll with the built JDK. (Similar to how was done with > freetype on Windows before.). And, optionally, provide an option for > configure to require libsleef to be present, so the build fails if no > libsleef can be found and bundled. (Leaving open as to if this should be > default or not.) > @magicus @theRealAph Thanks for discussion. > > In the short term, I'd build a shim library during the default standard JDK > > build that does not need SLEEF at build time. Unless weo do that, because > > SLEEF isn't on anyone's builders, it won't be used. > > I agree it's give user more chance to leverage the sleef when running, but I > wonder if it's necessary to do that. As we have a long term solution, and the > chance that end user lacks sleef library in their environment is much higher > than release engineer lacks sleef library in their environment. That situation leads to my nightmare, in which some OpenJDK builds work with vector support, and some don't, and there's no way to find out which unless you try it, in which case there is no warning, your programs just run slowly. Even if you've installed SLEEF. That would be bad for our users, bad for OpenJDK, and bad for Java. > But it's not harm to do so. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2014634593
Re: RFR: 8326960: GHA: RISC-V sysroot cannot be debootstrapped due to ongoing Debian t64 transition
On Thu, 21 Mar 2024 15:33:21 GMT, Aleksey Shipilev wrote: > Debian "unstable" is currently undergoing t64 (hello, Y2038) transition, > which breaks "sid" repo that we use for debootstrapping RISC-V very often. > This is seen across multiple repositories, and requires everyone to look > through GHA failures all the time. This PR tries to fix this: if debootstrap > for "sid" repos fails, the build for that arch would not continue. IMO, it is > a better compromise for current situation. We will flip back to expecting the > debootstrap to complete well after either t64 transition finishes, or we > switch to more stable Debian repo once it graduates. > > Additional testing: > - [x] GHA OK, great. GHAs are passing with RISC-V build skipped over broken sysroot. I will integrate after I get a second review from build/GHA maintainers. - PR Comment: https://git.openjdk.org/jdk/pull/18435#issuecomment-2014558684
