Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Thu, 12 May 2022 00:26:41 GMT, Yasumasa Suenaga wrote: >> I don't understand what the actual warning is getting at .. can anyone >> explain it ? >> >> FWIW the code is still the same in upstream harfbuzz >> https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-font.cc > > I've pasted a part of warning messages to description of this PR, all of > messages are here: > > > In function 'void trampoline_reference(hb_trampoline_closure_t*)', > inlined from 'void hb_font_funcs_set_glyph_func(hb_font_funcs_t*, > hb_font_get_glyph_func_t, void*, hb_destroy_func_t)' at > /home/ysuenaga/github-forked/jdk/src/java.desktop/share/native/libharfbuzz/hb-font.cc:2368:24: So upstream say it is not a problem since the analysis overlooks that it would not reach that free if it were immutable because of a previous check. I guess we disable the false positive warning for now. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v5]
On Fri, 13 May 2022 10:02:30 GMT, Yasumasa Suenaga wrote: >> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 >> on Fedora 36. >> As you can see, the warnings spreads several areas. Let me know if I should >> separate them by area. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Revert change for java.c , parse_manifest.c , LinuxPackage.c Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v4]
On Thu, 12 May 2022 01:27:30 GMT, Yasumasa Suenaga wrote: >> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 >> on Fedora 36. >> As you can see, the warnings spreads several areas. Let me know if I should >> separate them by area. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Calculate char offset before realloc() I will see what upstream thinks about the harfbuzz warning but in the mean time we can just disable it. - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v2]
On Wed, 11 May 2022 13:35:00 GMT, Kim Barrett wrote: >> Yasumasa Suenaga has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Avoid pragma error in before GCC 12 > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 462: > >> 460:HARFBUZZ_DISABLED_WARNINGS_gcc := type-limits >> missing-field-initializers strict-aliasing >> 461:HARFBUZZ_DISABLED_WARNINGS_CXX_gcc := reorder >> delete-non-virtual-dtor strict-overflow \ >> 462: maybe-uninitialized class-memaccess unused-result extra >> use-after-free > > Globally disabling use-after-free warnings for this package seems really > questionable. If these are problems in the code, just suppressing the warning > and leaving the problems to bite us seems like a bad idea. And the problems > ought to be reported upstream to the HarfBuzz folks. I don't understand what the actual warning is getting at .. can anyone explain it ? FWIW the code is still the same in upstream harfbuzz https://github.com/harfbuzz/harfbuzz/blob/main/src/hb-font.cc - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings [v4]
On Wed, 4 May 2022 08:00:08 GMT, Matthias Baesken wrote: >> Currently we set _WIN32_WINNT at various places in the codebase; this is >> used to target a minimum Windows version we want to support. See also for >> more detailled information : >> https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt >> Macros for Conditional Declarations >> "Certain functions that depend on a particular version of Windows are >> declared using conditional code. This enables you to use the compiler to >> detect whether your application uses functions that are not supported on its >> target version(s) of Windows." >> >> However currently we have quite a lot of differing settings of _WIN32_WINNT >> in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible >> would make sense because we have this setting already at java_props_md.c >> (so targeting older Windows versions at other places is most likely not >> useful). > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust API level to Windows 8 for security.cpp and do some cleanup Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments
On Mon, 9 May 2022 15:56:35 GMT, Adam Sotona wrote:
> Please review this patch adding new lint option, **lossy-conversions**, to
> javac to warn about type casts in compound assignments with possible lossy
> conversions.
>
> The new lint warning is shown if the type of the right-hand operand of a
> compound assignment is not assignment compatible with the type of the
> variable.
>
> The implementation of the warning is based on similar check performed to emit
> "possible lossy conversion" compilation error for simple assignments.
>
> Proposed patch also include complex matrix-style test with positive and
> negative test cases of lossy conversions in compound assignments.
>
> Proposed patch also disables this new lint option in all affected JDK modules
> and libraries to allow smooth JDK build. Individual cases to address possibly
> lossy conversions warnings in JDK are already addressed in a separate
> umbrella issue and its sub-tasks.
>
> Thanks for your review,
> Adam
Marked as reviewed by prr (Reviewer).
test/langtools/tools/javac/lint/LossyConversions.java line 131:
> 129: @SuppressWarnings("lossy-conversions")
> 130: public void supressedLossyConversions() {
> 131: byte a = 0;
you might want to spell suppressed correctly.
-
PR: https://git.openjdk.java.net/jdk/pull/8599
Re: RFR: JDK-8285730: unify _WIN32_WINNT settings
On Wed, 27 Apr 2022 14:57:41 GMT, Matthias Baesken wrote: > Currently we set _WIN32_WINNT at various places in the codebase; this is used > to target a minimum Windows version we want to support. See also for more > detailled information : > https://docs.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers?redirectedfrom=MSDN#setting-winver-or-_win32_winnt > Macros for Conditional Declarations > "Certain functions that depend on a particular version of Windows are > declared using conditional code. This enables you to use the compiler to > detect whether your application uses functions that are not supported on its > target version(s) of Windows." > > However currently we have quite a lot of differing settings of _WIN32_WINNT > in the codebase ; setting _WIN32_WINNT to 0x0601 (Windows 7) where possible > would make sense because we have this setting already at java_props_md.c > (so targeting older Windows versions at other places is most likely not > useful). This is OK by me, but I wonder if the build folks might want to think about this and whether it should be centralised somehow since it seems odd to mandate different versions of windows in different places. - PR: https://git.openjdk.java.net/jdk/pull/8428
Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times [v3]
On Wed, 23 Mar 2022 23:38:52 GMT, Sergey Bylokhov wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update comment to show we don't care about these files. > > Do we really want to change the product performance characteristics based on > the compilation time? Can we try parallelize/cache or something first? > @mrserb Phil said that we don't even use the subset part of harfbuzz. > > And unless you're backing up your claim that this patch is changing runtime > characteristic with data, that's just a guess, just like the initial > optimization level of this library (and as most of the native libraries in > the JDK... :-() was just a guess. Otoh, that we get a build time regression > for these two files is a proven fact. > > What is it you want to cache? It is a reasonable question. Build times might seem important to a small number of developers but runtime performance is King. That's why I asked to reduce it to just the files we don't care about. Would we give up even 0.1% of hotspot GC performance on Linux to avoid 30 seconds of compile time ? I doubt it. And haven't we improved compile time a lot with the parallelism ? So are we now over-optimising in the wrong place ? I mean good to keep an eye on it but I mean if a previous build arch took 15 mins and now with parallelism we take 3 mins and 45 secs .. is it so bad to be back up to 4 mins for more runtime benefit ? (Numbers entirely made up) There's always going to be a long pole (something that is last) and the questions are whether its understood and for a good reason and so forth. I mean we can doubtless try to improve but at some point it is diminishing returns. Perhaps someone can ask gcc devs why they are so slow on this code ? Maybe there's a good reason that helps runtime or maybe they just have design issues. They may want to know. - PR: https://git.openjdk.java.net/jdk/pull/7919
Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times [v2]
On Wed, 23 Mar 2022 23:13:04 GMT, Magnus Ihse Bursie wrote: >> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 487: >> >>> 485: LIBFONTMANAGER_OPTIMIZATION := HIGHEST >>> 486: >>> 487: ifneq ($(filter $(TOOLCHAIN_TYPE), gcc clang), ) >> >> Can we have a note here that the de-opt is possible because these two files >> aren't important to OpenJDK ? >> If these files were to be renamed in an updated version of harfbuzz, would >> there be a compilation error ? > > Comment updated. > > No, these directives will be just silently ignored. So if that happens we run > the risk of reintroducing the build performance regression. Not a big risk. > In any case, at next harfbuzz update it can be worth re-checking if these > exceptions are still needed. (If you're the one doing it and need help with > that, just let me know!) As of today they are still there in upstream harfbuzz on github. - PR: https://git.openjdk.java.net/jdk/pull/7919
Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times [v3]
On Wed, 23 Mar 2022 23:17:38 GMT, Magnus Ihse Bursie wrote: >> [JDK-8247872](https://bugs.openjdk.java.net/browse/JDK-8247872) (upgrade >> HarfBuzz to 2.7.2) caused build time to go up with 24 seconds on my >> reference linux machine. This was one of the four culprits that caused a >> 25-30% build time regression over the last two years. >> >> The problem here was that the new HarfBuzz code caught really bad behaviour >> from gcc when compiling with optimizations. The official HarfBuzz build does >> not use any -O flags at all for gcc, so presumably the HarfBuzz team is: >> >> a) not thinking compiler optimization is important for the performance of >> this library, and >> b) unaware that their code causes such a headache for gcc. >> >> (Other compilers fare much better: visual studio makes no difference at all, >> and for clang just a small regression was observed.) >> >> The current optimization level was introduced by >> [JDK-8255790](https://bugs.openjdk.java.net/browse/JDK-8255790), which were >> really about moving libharfbuzz compilation back into libfontmanager. I >> could find no comments/discussion relating to the change of optimization >> level, so I assume it was incidental, and just seemed good at the time. >> >> This patch changes the optimization level to `SIZE` (which is the closest >> thing we have to no optimization level) on gcc. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Update comment to show we don't care about these files. Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7919
Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times [v2]
On Wed, 23 Mar 2022 21:13:30 GMT, Magnus Ihse Bursie wrote: >> [JDK-8247872](https://bugs.openjdk.java.net/browse/JDK-8247872) (upgrade >> HarfBuzz to 2.7.2) caused build time to go up with 24 seconds on my >> reference linux machine. This was one of the four culprits that caused a >> 25-30% build time regression over the last two years. >> >> The problem here was that the new HarfBuzz code caught really bad behaviour >> from gcc when compiling with optimizations. The official HarfBuzz build does >> not use any -O flags at all for gcc, so presumably the HarfBuzz team is: >> >> a) not thinking compiler optimization is important for the performance of >> this library, and >> b) unaware that their code causes such a headache for gcc. >> >> (Other compilers fare much better: visual studio makes no difference at all, >> and for clang just a small regression was observed.) >> >> The current optimization level was introduced by >> [JDK-8255790](https://bugs.openjdk.java.net/browse/JDK-8255790), which were >> really about moving libharfbuzz compilation back into libfontmanager. I >> could find no comments/discussion relating to the change of optimization >> level, so I assume it was incidental, and just seemed good at the time. >> >> This patch changes the optimization level to `SIZE` (which is the closest >> thing we have to no optimization level) on gcc. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Restore HIGHEST for entire lib, just set SIZE to two files make/modules/java.desktop/lib/Awt2dLibraries.gmk line 487: > 485: LIBFONTMANAGER_OPTIMIZATION := HIGHEST > 486: > 487: ifneq ($(filter $(TOOLCHAIN_TYPE), gcc clang), ) Can we have a note here that the de-opt is possible because these two files aren't important to OpenJDK ? If these files were to be renamed in an updated version of harfbuzz, would there be a compilation error ? - PR: https://git.openjdk.java.net/jdk/pull/7919
Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times
On Wed, 23 Mar 2022 12:25:08 GMT, Magnus Ihse Bursie wrote: > [JDK-8247872](https://bugs.openjdk.java.net/browse/JDK-8247872) (upgrade > HarfBuzz to 2.7.2) caused build time to go up with 24 seconds on my reference > linux machine. This was one of the four culprits that caused a 25-30% build > time regression over the last two years. > > The problem here was that the new HarfBuzz code caught really bad behaviour > from gcc when compiling with optimizations. The official HarfBuzz build does > not use any -O flags at all for gcc, so presumably the HarfBuzz team is: > > a) not thinking compiler optimization is important for the performance of > this library, and > b) unaware that their code causes such a headache for gcc. > > (Other compilers fare much better: visual studio makes no difference at all, > and for clang just a small regression was observed.) > > The current optimization level was introduced by > [JDK-8255790](https://bugs.openjdk.java.net/browse/JDK-8255790), which were > really about moving libharfbuzz compilation back into libfontmanager. I could > find no comments/discussion relating to the change of optimization level, so > I assume it was incidental, and just seemed good at the time. > > This patch changes the optimization level to `SIZE` (which is the closest > thing we have to no optimization level) on gcc. harfbuzz does not make it easy to subset the build. So no. - PR: https://git.openjdk.java.net/jdk/pull/7919
Re: RFR: 8283323: libharfbuzz optimization level results in extreme build times
On Wed, 23 Mar 2022 12:25:08 GMT, Magnus Ihse Bursie wrote: > [JDK-8247872](https://bugs.openjdk.java.net/browse/JDK-8247872) (upgrade > HarfBuzz to 2.7.2) caused build time to go up with 24 seconds on my reference > linux machine. This was one of the four culprits that caused a 25-30% build > time regression over the last two years. > > The problem here was that the new HarfBuzz code caught really bad behaviour > from gcc when compiling with optimizations. The official HarfBuzz build does > not use any -O flags at all for gcc, so presumably the HarfBuzz team is: > > a) not thinking compiler optimization is important for the performance of > this library, and > b) unaware that their code causes such a headache for gcc. > > (Other compilers fare much better: visual studio makes no difference at all, > and for clang just a small regression was observed.) > > The current optimization level was introduced by > [JDK-8255790](https://bugs.openjdk.java.net/browse/JDK-8255790), which were > really about moving libharfbuzz compilation back into libfontmanager. I could > find no comments/discussion relating to the change of optimization level, so > I assume it was incidental, and just seemed good at the time. > > This patch changes the optimization level to `SIZE` (which is the closest > thing we have to no optimization level) on gcc. We definitely do not want to downgrade the optimisation level of the entire library. Performance of this library actually matters quite a lot. When we added some additional checking to the previous library we used (ICU) we had complaints of as much as 20% loss in PDF conversion (a backend process) and we've had escalations on user responsiveness in editors due to the computational complexity of layout vs just counting glyph advances. Getting that performance back was a lot of work .. except increasing compiler optimisation was the easy one that made a big difference. To see the performance degradation may require a specific combination of font (Linux fonts vary widely) and the text being laid out. The example we used in the previous case required a windows font, so I don't have a handy test case I know will show this .. and it was years ago .. Looking at the list of files from Erik, I think we don't use the subset functionality. Certainly not directly and likely not at all. So downgrading that probably won't have any impact at all at runtime. But hb-ot-layout.cc is one of the most important files in the library, maybe the most important. I strongly suggest that one not be downgraded just for 7 seconds of build time. - PR: https://git.openjdk.java.net/jdk/pull/7919
Integrated: 8283457: [macos] libpng build failures with Xcode13.3
On Tue, 22 Mar 2022 18:53:20 GMT, Phil Race wrote: > Disable a warning from the Xcode 13.3 clang compiler when compiling libpng, > which is used by libsplashscreen. > > Policy is not to change the upstream code locally unless it is also changed > in the same way upstream. > We could consider reporting it upstream but libpng releases are very > infrequent. This pull request has now been integrated. Changeset: 33eb89df Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/33eb89dfeb4a43e1ad2c3dd657ec3b6ee7abbb3a Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8283457: [macos] libpng build failures with Xcode13.3 Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/7911
RFR: 8283457: [macos] libpng build failures with Xcode13.3
Disable a warning from the Xcode 13.3 clang compiler when compiling libpng, which is used by libsplashscreen. Policy is not to change the upstream code locally unless it is also changed in the same way upstream. We could consider reporting it upstream but libpng releases are very infrequent. - Commit messages: - 8283457: [macos] libpng build failures with Xcode13.3 Changes: https://git.openjdk.java.net/jdk/pull/7911/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7911=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283457 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7911.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7911/head:pull/7911 PR: https://git.openjdk.java.net/jdk/pull/7911
Re: RFR: 8257733: Move module-specific data from make to respective module [v9]
On Fri, 18 Mar 2022 21:24:43 GMT, Phil Race wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix typos > > This doesn't seem right to me to put x11wrappergen into share. > > This is X11 specific. It should not be in share. > > Same for all of the fontconfig files. In make/data it did not seem too weird > but it is very weird to put them all in share. If you were to go back and > look how it used to be before someone moved them to make I am sure you'd find > them in platform specific source directories. > @prrace I have now moved the fontconfig files to `$OS/data`. (I also took the > opportunity to clean up the GenData file, which had a quite hairy logic for a > trivial task.) I have moved the x11wrappergen files to `unix/data`. > > (Strictly speaking, "unix" and "x11" do not have the same "sense" (in the > Fregean meaning), even if it has the same "referent". But we've used that > convention before so I'm sticking to it.) Indeed they do not but it is a better approximation than "shared". - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v9]
On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typos This doesn't seem right to me to put x11wrappergen into share. This is X11 specific. It should not be in share. Same for all of the fontconfig files. In make/data it did not seem too weird but it is very weird to put them all in share. If you were to go back and look how it used to be before someone moved them to make I am sure you'd find them in platform specific source directories. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v9]
On Thu, 17 Mar 2022 00:12:38 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix typos Changes requested by prr (Reviewer). make/modules/java.desktop/gensrc/GensrcX11Wrappers.gmk line 27: > 25: > 26: # Generate java sources using the X11 offsets that are precalculated in > files > 27: # src/java.desktop/share/data/x11wrappergen/sizes-.txt. This doesn't seem right to me. This is X11 specific. It should not be in share. Same for related files. - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8257733: Move module-specific data from make to respective module [v6]
On Tue, 15 Mar 2022 23:50:20 GMT, Magnus Ihse Bursie wrote: >> A lot (but not all) of the data in make/data is tied to a specific module. >> For instance, the publicsuffixlist is used by java.base, and fontconfig by >> java.desktop. (A few directories, like mainmanifest, is *actually* used by >> make for the whole build.) >> >> These data files should move to the module they belong to. The are, after >> all, "source code" for that module that is "compiler" into resulting >> deliverables, for that module. (But the "source code" language is not Java >> or C, but typically a highly domain specific language or data format, and >> the "compilation" is, often, a specialized transformation.) >> >> This misplacement of the data directory is most visible at code review time. >> When such data is changed, most of the time build-dev (or the new build >> label) is involved, even though this has nothing to do with the build. While >> this is annoying, a worse problem is if the actual team that needs to review >> the patch (i.e., the team owning the module) is missed in the review. >> >> ### Modules reviewed >> >> - [x] java.base >> - [x] java.desktop >> - [x] jdk.compiler >> - [x] java.se > > Magnus Ihse Bursie has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains 12 commits: > > - Merge branch 'master' into shuffle-data-reborn > - Fix merge > - Merge tag 'jdk-19+13' into shuffle-data-reborn > >Added tag jdk-19+13 for changeset 5df2a057 > - Move characterdata templates to share/classes/java/lang. > - Update comment refering to "make" dir > - Move new symbols to jdk.compiler > - Merge branch 'master' into shuffle-data > - Move macosxicons from share to macosx > - Move to share/data, and move jdwp.spec to java.se > - Update references in test > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/83d77186...598f740f How are folks reviewing this huge PR ? My browser paints a few files and that's it, and the drop down list to "jump to" is too big to display - it says ! - PR: https://git.openjdk.java.net/jdk/pull/1611
Re: RFR: 8282657: Code cleanup: removing double semicolons at the end of lines
On Fri, 28 Jan 2022 14:39:31 GMT, Matteo Baccan wrote: > Hi > > I have reviewed the code for removing double semicolons at the end of lines > > all the best > matteo Marked as reviewed by prr (Reviewer). Looks like there's only one client source code file touched Most of the client changes are only in jtreg tests - and this is very trivial, so I'm OK with them being included here. - PR: https://git.openjdk.java.net/jdk/pull/7268
Re: RFR: 8278251: Enable "missing-explicit-ctor" check in the jdk.unsupported.desktop module
On Fri, 3 Dec 2021 20:55:23 GMT, Sergey Bylokhov wrote: > The "missing-explicit-ctor" check was disabled by the > [JDK-8071961](https://bugs.openjdk.java.net/browse/JDK-8071961) and later was > fixed by the [JDK-8250853](https://bugs.openjdk.java.net/browse/JDK-8250853). > So we can re-enable this check again. > > The fix will remove the "Java.gmk" file and as a result the > "jdk.unsupported.desktop" folder. > > All "Pre-submit tests" builds are green. Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6708
Re: RFR: JDK-8278175: Enable all doclint warnings for build of java.desktop
On Fri, 3 Dec 2021 01:18:20 GMT, Joe Darcy wrote: > In JDK 18, JDK-8189591 added the ability to suppress doclint warnings. > Therefore, it is now possible to enable the full doclint checks for the > java.desktop module if the instances of warnings are suppressed. This patch > does this; it would be preferable to address the doc warnings directly, but > that is beyond what I'm attempting to do here. Looks OK. Could you file a P4 bug showing the warnings/errors that would be generated so that we know what we need to work on to later remove these the right way. - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6687
Re: RFR: 8276905: Use appropriate macosx_version_minimum value while compiling metal shaders [v4]
On Mon, 15 Nov 2021 14:00:59 GMT, Jayathirth D V wrote: >> When JDK is built on BigSur with Xcode12.5 and used to run jtreg tests in >> macOS10.15 with Xcode <=12.4 we are getting compilation error. We dont set >> any deployment target when when we use xcrun to create .air file and this >> issue looks similar to https://developer.apple.com/forums/thread/105719. >> Also https://developer.apple.com/forums/thread/105719 states that even if >> they set app deployment target they need to explicitly set deployment in >> "xcrun metal". >> >> Made same change to use -mmacosx-version-min=10.12.00 in our make file. >> Whether we should keep 10.12.00 or 10.13/14 is open to discussion for metal >> pipeline. >> >> SwingSet2, J2DDemo & JCK/jtreg runs in CI are green. Also Vitaly(Submitter) >> has confirmed that patch resolves the issue. > > Jayathirth D V has updated the pull request incrementally with one additional > commit since the last revision: > > Use MACOSX_VERSION_MIN for aarch64 This should be the most compatible solution. - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6346
Re: RFR: 8276905: Function frag_col has a deployment target which is incompatible with this OS [v2]
On Thu, 11 Nov 2021 15:32:01 GMT, Jayathirth D V wrote: >> When JDK is built on BigSur with Xcode12.5 and used to run jtreg tests in >> macOS10.15 with Xcode <=12.4 we are getting compilation error. We dont set >> any deployment target when when we use xcrun to create .air file and this >> issue looks similar to https://developer.apple.com/forums/thread/105719. >> Also https://developer.apple.com/forums/thread/105719 states that even if >> they set app deployment target they need to explicitly set deployment in >> "xcrun metal". >> >> Made same change to use -mmacosx-version-min=10.12.00 in our make file. >> Whether we should keep 10.12.00 or 10.13/14 is open to discussion for metal >> pipeline. >> >> SwingSet2, J2DDemo & JCK/jtreg runs in CI are green. Also Vitaly(Submitter) >> has confirmed that patch resolves the issue. > > Jayathirth D V has updated the pull request incrementally with one additional > commit since the last revision: > > Use Macro for version " Also note that on aarch64 the global value is 11.00.00, and in that case we should use the global value." I have no idea what happens if you specify 10.14 to the metal compiler on aarch64 Something else to check although probably the safest option is to make it 11.0 on that architecture - PR: https://git.openjdk.java.net/jdk/pull/6346
Re: RFR: 8276905: Function frag_col has a deployment target which is incompatible with this OS [v2]
On Thu, 11 Nov 2021 15:32:01 GMT, Jayathirth D V wrote: >> When JDK is built on BigSur with Xcode12.5 and used to run jtreg tests in >> macOS10.15 with Xcode <=12.4 we are getting compilation error. We dont set >> any deployment target when when we use xcrun to create .air file and this >> issue looks similar to https://developer.apple.com/forums/thread/105719. >> Also https://developer.apple.com/forums/thread/105719 states that even if >> they set app deployment target they need to explicitly set deployment in >> "xcrun metal". >> >> Made same change to use -mmacosx-version-min=10.12.00 in our make file. >> Whether we should keep 10.12.00 or 10.13/14 is open to discussion for metal >> pipeline. >> >> SwingSet2, J2DDemo & JCK/jtreg runs in CI are green. Also Vitaly(Submitter) >> has confirmed that patch resolves the issue. > > Jayathirth D V has updated the pull request incrementally with one additional > commit since the last revision: > > Use Macro for version My understanding is that the flag here affects ONLY the metal compiler - for compiling metal shaders. And if you don't specify -Dsun.java2d.metal=true (since metal is off by default) its a 100% no-op for the rest of the JDK. And already, if you specify Dsun.java2d.metal=true and you are on 10.13 or lower, we do not honour the request so we haven't changed what platforms will work at all if we do it this way. So our effective deployment target for metal is already 10.14 And I also would not be surprised if someone wants to backport this to 17u, in which case a config change would have the effect of making 17u no longer run on macos 12 .. which I guess will happen sometime during the life of the LTS but right now ?? So making it a metal-specific change is what I think we should do FOR NOW and we can have a follow-on fix that aligns both of these .. maybe that is a subsequent JDK 18 fix, or perhaps it should be an early JDK 19 fix ? - PR: https://git.openjdk.java.net/jdk/pull/6346
Re: RFR: 8276905: Function frag_col has a deployment target which is incompatible with this OS [v2]
On Fri, 12 Nov 2021 03:56:57 GMT, Phil Race wrote: >> Jayathirth D V has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Use Macro for version > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 850: > >> 848: COMMAND := $(METAL) -c -std=osx-metal2.0 \ >> 849: -mmacosx-version-min=$(MACOSX_VERSION_MIN) \ >> 850: -o $(SHADERS_AIR) $(SHADERS_SRC), \ > > No .. we decided metal requires macos 10.14 as a minimum. In fact it won't > run (by policy) on earlier releases so settting it to what the broader JDK > defines as a minimum is not appropriate right now. > And I've no idea what restrictions we'd be placing on metal saying it can > only use 10.12 features. > Nor do I know what the xcode 12.4 used to build JDK 17 defaults to and how > much a change to either 10.12 or 10.14 might require in re-testing. > > So - > we should use 10.14 > what's the actual impact of that on a current build using xcode 12.4 - does > it change anything we use ? bear in mind "impact" might mean metal can't use some new faster code, not just runtime errors - PR: https://git.openjdk.java.net/jdk/pull/6346
Re: RFR: 8276905: Function frag_col has a deployment target which is incompatible with this OS [v2]
On Thu, 11 Nov 2021 15:32:01 GMT, Jayathirth D V wrote: >> When JDK is built on BigSur with Xcode12.5 and used to run jtreg tests in >> macOS10.15 with Xcode <=12.4 we are getting compilation error. We dont set >> any deployment target when when we use xcrun to create .air file and this >> issue looks similar to https://developer.apple.com/forums/thread/105719. >> Also https://developer.apple.com/forums/thread/105719 states that even if >> they set app deployment target they need to explicitly set deployment in >> "xcrun metal". >> >> Made same change to use -mmacosx-version-min=10.12.00 in our make file. >> Whether we should keep 10.12.00 or 10.13/14 is open to discussion for metal >> pipeline. >> >> SwingSet2, J2DDemo & JCK/jtreg runs in CI are green. Also Vitaly(Submitter) >> has confirmed that patch resolves the issue. > > Jayathirth D V has updated the pull request incrementally with one additional > commit since the last revision: > > Use Macro for version make/modules/java.desktop/lib/Awt2dLibraries.gmk line 850: > 848: COMMAND := $(METAL) -c -std=osx-metal2.0 \ > 849: -mmacosx-version-min=$(MACOSX_VERSION_MIN) \ > 850: -o $(SHADERS_AIR) $(SHADERS_SRC), \ No .. we decided metal requires macos 10.14 as a minimum. In fact it won't run (by policy) on earlier releases so settting it to what the broader JDK defines as a minimum is not appropriate right now. And I've no idea what restrictions we'd be placing on metal saying it can only use 10.12 features. Nor do I know what the xcode 12.4 used to build JDK 17 defaults to and how much a change to either 10.12 or 10.14 might require in re-testing. So - we should use 10.14 what's the actual impact of that on a current build using xcode 12.4 - does it change anything we use ? - PR: https://git.openjdk.java.net/jdk/pull/6346
Re: RFR: 8272332: --with-harfbuzz=system doesn't add -lharfbuzz after JDK-8255790
On Wed, 11 Aug 2021 17:58:04 GMT, Severin Gehwolf wrote: > Please review this simple build fix to correct the type done in JDK-8255790. > After the patch correct external library is added to the `libfontmanager.so` > link command when building with `--with-harfbuzz=system`. > > Thoughts? Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5091
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]
On Mon, 24 May 2021 13:53:34 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > keep only one systemProperty tag Marked as reviewed by prr (Reviewer). I'm OK with this now given that the refactoring is already underway at https://github.com/openjdk/jdk/pull/4138 - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
I haven't seen any response to this comment I made a couple of days ago and I
suspect it got missed
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {
Who or what prompted this change ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 07:06:00 GMT, Alan Bateman wrote: >> The JEP isn't PTT for 17 so there's plenty of time isn't there ? > > There are 827 files in this patch. Phil is right that adding SW at the class > level is introducing technical debt but if addressing that requires > refactoring and re-testing of AWT or font code then it would be better to > have someone from that area working on. Is there any reason why this can't be > going on now on awt-dev/2d-dev and integrated immediately after JEP 411? I > don't think we should put Max through the wringer here as there are too many > areas to cover. Are you suggesting that the patch doesn't need testing as it is ? It should be the same either way. It is very straight forward to run the automated tests across all platforms these days. I get the impression that no one is guaranteeing to do this straight after integration. It sounds like it is up for deferral if time runs out. The amount of follow-on work that I am hearing about here, and may be for tests, does not make it sound like this JEP is nearly as done as first presented. If there was some expectation that groups responsible for an area would get involved with this issue which I am assured was already known about, then why was it not raised before and made part of the plan ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Integrated: 8256372: [macos] Unexpected symbol was displayed on JTextField with Monospaced font
On Tue, 18 May 2021 15:55:09 GMT, Phil Race wrote: > See the bug report for lots of explanation. > The short version (not that short) is that Swing adds a new line char to > editable TextComponents > It used to work because harfbuzz asked CoreText which mapped it to an > invisible glyph > Now harfbuzz does its own processing of AAT fonts it is different. > Harfbuzz looks up the mapping for this but there is none so it gets a missing > glyph. > > Solution: Since HB now does AAT processing we can dispense with all the > special handling of that case > and core text and just treat AAT fonts like OT fonts. > This means its gets the same mapping we get when not using Layout. > > And the test passes on all platforms .. not just macOS .. This pull request has now been integrated. Changeset: 005d8a7f Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/005d8a7fca8b4d9519d2bde0a7cdbbece1cd3981 Stats: 1425 lines in 9 files changed: 65 ins; 1352 del; 8 mod 8256372: [macos] Unexpected symbol was displayed on JTextField with Monospaced font Reviewed-by: erikj, serb - PR: https://git.openjdk.java.net/jdk/pull/4097
Re: RFR: 8256372: [macos] Unexpected symbol was displayed on JTextField with Monospaced font
On Tue, 18 May 2021 19:53:36 GMT, Sergey Bylokhov wrote: >> See the bug report for lots of explanation. >> The short version (not that short) is that Swing adds a new line char to >> editable TextComponents >> It used to work because harfbuzz asked CoreText which mapped it to an >> invisible glyph >> Now harfbuzz does its own processing of AAT fonts it is different. >> Harfbuzz looks up the mapping for this but there is none so it gets a >> missing glyph. >> >> Solution: Since HB now does AAT processing we can dispense with all the >> special handling of that case >> and core text and just treat AAT fonts like OT fonts. >> This means its gets the same mapping we get when not using Layout. >> >> And the test passes on all platforms .. not just macOS .. > > src/java.desktop/share/native/libharfbuzz/hb-coretext.cc line 1: > >> 1: /* > > How we will track these files for the next HB update? Do we need to document > it somehow? otherwise, I think it will be added again as a new files. No, they won't. We already pull in only the files we need, not every file, so this is not a precedent. - PR: https://git.openjdk.java.net/jdk/pull/4097
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 04:05:23 GMT, Phil Race wrote: >> By converting JDK-8267432 to a bug, there is an extra benefit that we can >> fix it after RDP. So I'll convert it now. > > That is unfortunate, but nonetheless I think required to be done. > We have acknowledeged this can't reasonably be called an RFE, so the JEP is > introducing bugs/technical debt/call it what you will. This should generally > be part of a sandbox for the JEP and fixed before integration of the JEP. > From my point of view it is a blocker. The JEP isn't PTT for 17 so there's plenty of time isn't there ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Thu, 20 May 2021 02:09:57 GMT, Weijun Wang wrote: >> I can make it a bug. >> >> I don't want to do it here is because it involves indefinite amount of >> manual work and we will see everyone having their preferences. The more time >> we spend on this PR the more likely there will be merge conflict. There are >> just too many files here. >> >> And no matter if we include that change in this PR or after it, it will be >> after the automatic conversion. In fact, the data about which cases are more >> worth fixing come from the automatic conversion itself. Also, as the manual >> work will be done part by part, if the automatic conversion is after it, >> there will be rounds and rounds of history rewriting and force push. This is >> quite unfriendly to the reviewers. >> >> Altogether, there are 117 class-level annotations. Unfortunately, >> `java.awt.Component` is the one with the biggest class -- estimated number >> of bytes that the annotation covers is over 380K. In the client area, the >> 2nd place is `java.awt.Container`, and then we have >> `sun.font.SunFontManager`, `java.awt.Window`, and `java.awt.Toolkit` which >> are over 100KB, and other 25 classes over 10KB, and other 11 classes smaller >> than 10KB. > > By converting JDK-8267432 to a bug, there is an extra benefit that we can fix > it after RDP. So I'll convert it now. That is unfortunate, but nonetheless I think required to be done. We have acknowledeged this can't reasonably be called an RFE, so the JEP is introducing bugs/technical debt/call it what you will. This should generally be part of a sandbox for the JEP and fixed before integration of the JEP. >From my point of view it is a blocker. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 22:14:20 GMT, Weijun Wang wrote: >> I don't think it is a separate P4 enhancement (?) that someone will (maybe) >> do next release. >> I think it should all be taken care of as part of this proposed change. > > I just made it P3 (P4 was the default value), and I will target it to 17 once > JEP 411 is targeted 17. But I think it's probably not a good idea to include > it inside *this* PR. There are some middle ground where it's debatable if a > change is worth doing (Ex: which is uglier between an > a-liitle-faraway-annotation and a temporary variable?) so it's not obvious > what the scope of the refactoring can be. Thus it will be divided into > smaller sub-tasks. It's not totally impossible that part of the work will be > delayed to next release. Well .. as an enhancement (P3 or otherwise) I can see it being dropped and definitely if it misses the fork, and I don't get why you can't do it here. And if it isn't done the JEP isn't really ready. I already pasted the patch for Component.java and it took about 60 seconds to do that. Then yes, you would have to deal with the fact that now you need to reapply your automated tool to the file but this is just work you'd have to have done anyway if it was already refactored. I only *noticed* Component and Container. And stopped there to raise the question. How many more cases are there ? - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 21:53:35 GMT, Weijun Wang wrote:
>> That's a sad limitation of the annotation stuff then, but I don't think that
>> it is insurmountable.
>> You can define a static private method to contain this and call it from the
>> static initializer block.
>> Much better than applying the annotation to an entire class.
>>
>> --- a/src/java.desktop/share/classes/java/awt/Component.java
>> +++ b/src/java.desktop/share/classes/java/awt/Component.java
>> @@ -618,6 +618,17 @@ public abstract class Component implements
>> ImageObserver, MenuContainer,
>> */
>> static boolean isInc;
>> static int incRate;
>> +
>> +private static void initIncRate() {
>> +String s = java.security.AccessController.doPrivileged(
>> + new
>> GetPropertyAction("awt.image.incrementaldraw"));
>> +isInc = (s == null || s.equals("true"));
>> +
>> +s = java.security.AccessController.doPrivileged(
>> + new GetPropertyAction("awt.image.redrawrate"));
>> +incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +}
>> +
>> static {
>> /* ensure that the necessary native libraries are loaded */
>> Toolkit.loadLibraries();
>> @@ -625,14 +636,7 @@ public abstract class Component implements
>> ImageObserver, MenuContainer,
>> if (!GraphicsEnvironment.isHeadless()) {
>> initIDs();
>> }
>> -
>> -String s = java.security.AccessController.doPrivileged(
>> - new
>> GetPropertyAction("awt.image.incrementaldraw"));
>> -isInc = (s == null || s.equals("true"));
>> -
>> -s = java.security.AccessController.doPrivileged(
>> -new
>> GetPropertyAction("awt.image.redrawrate"));
>> -incRate = (s != null) ? Integer.parseInt(s) : 100;
>> +initIncRate();
>> }
>
> Correct, there are ways to modify the code to make it more
> annotation-friendly. We thought about whether it's good to do it before
> adding the annotations or after it. Our decision now is to do it after
> because it will be more easy to see why it's necessary and we can take time
> to do them little by little. A new enhancement at
> https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.
I don't think it is a separate P4 enhancement (?) that someone will (maybe) do
next release.
I think it should all be taken care of as part of this proposed change.
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 18:38:39 GMT, Weijun Wang wrote:
>> src/java.desktop/share/classes/java/awt/Component.java line 217:
>>
>>> 215: * @author Sami Shaio
>>> 216: */
>>> 217: @SuppressWarnings("removal")
>>
>> Why is this placed on the *entire class* ??
>> This class is 10535 lines long and mentions AccessControl something like 8
>> places it uses AccessController or AcessControlContext.
>
> This happens when a deprecated method is called inside a static block. The
> annotation can only be added to a declaration and here it must be the whole
> class. The call in this file is
>
> s = java.security.AccessController.doPrivileged(
> new
> GetPropertyAction("awt.image.redrawrate"));
That's a sad limitation of the annotation stuff then, but I don't think that it
is insurmountable.
You can define a static private method to contain this and call it from the
static initializer block.
Much better than applying the annotation to an entire class.
--- a/src/java.desktop/share/classes/java/awt/Component.java
+++ b/src/java.desktop/share/classes/java/awt/Component.java
@@ -618,6 +618,17 @@ public abstract class Component implements ImageObserver,
MenuContainer,
*/
static boolean isInc;
static int incRate;
+
+private static void initIncRate() {
+String s = java.security.AccessController.doPrivileged(
+ new
GetPropertyAction("awt.image.incrementaldraw"));
+isInc = (s == null || s.equals("true"));
+
+s = java.security.AccessController.doPrivileged(
+ new GetPropertyAction("awt.image.redrawrate"));
+incRate = (s != null) ? Integer.parseInt(s) : 100;
+}
+
static {
/* ensure that the necessary native libraries are loaded */
Toolkit.loadLibraries();
@@ -625,14 +636,7 @@ public abstract class Component implements ImageObserver,
MenuContainer,
if (!GraphicsEnvironment.isHeadless()) {
initIDs();
}
-
-String s = java.security.AccessController.doPrivileged(
- new
GetPropertyAction("awt.image.incrementaldraw"));
-isInc = (s == null || s.equals("true"));
-
-s = java.security.AccessController.doPrivileged(
-new
GetPropertyAction("awt.image.redrawrate"));
-incRate = (s != null) ? Integer.parseInt(s) : 100;
+initIncRate();
}
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
src/java.desktop/share/classes/java/awt/Component.java line 217:
> 215: * @author Sami Shaio
> 216: */
> 217: @SuppressWarnings("removal")
Why is this placed on the *entire class* ??
This class is 10535 lines long and mentions AccessControl something like 8
places it uses AccessController or AcessControlContext.
src/java.desktop/share/classes/java/awt/Container.java line 97:
> 95: * @since 1.0
> 96: */
> 97: @SuppressWarnings("removal")
Same issue as with Component. a > 5,000 line file that uses AccessController in
just 4 places.
Where else are you adding this to entire classes instead of the specific site ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote: >> Please review this implementation of [JEP >> 411](https://openjdk.java.net/jeps/411). >> >> The code change is divided into 3 commits. Please review them one by one. >> >> 1. >> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1 >> The essential change for this JEP, including the `@Deprecate` annotations >> and spec change. It also update the default value of the >> `java.security.manager` system property to "disallow", and necessary test >> change following this update. >> 2. >> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66 >> Manual changes to several files so that the next commit can be generated >> programatically. >> 3. >> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950 >> Automatic changes to other source files to avoid javac warnings on >> deprecation for removal >> >> The 1st and 2nd commits should be reviewed carefully. The 3rd one is >> generated programmatically, see the comment below for more details. If you >> are only interested in a portion of the 3rd commit and would like to review >> it as a separate file, please comment here and I'll generate an individual >> webrev. >> >> Due to the size of this PR, no attempt is made to update copyright years for >> any file to minimize unnecessary merge conflict. >> >> Furthermore, since the default value of `java.security.manager` system >> property is now "disallow", most of the tests calling >> `System.setSecurityManager()` need to launched with >> `-Djava.security.manager=allow`. This is covered in a different PR at >> https://github.com/openjdk/jdk/pull/4071. >> >> Update: the deprecation annotations and javadoc tags, build, compiler, >> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are >> reviewed. Rest are 2d, awt, beans, sound, and swing. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java This change is so large that github won't even display the list of files so I can't jump to where I want to go ! And when I try to scroll I get just a blank page. - PR: https://git.openjdk.java.net/jdk/pull/4073
Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]
On Wed, 19 May 2021 13:47:53 GMT, Weijun Wang wrote:
>> Please review this implementation of [JEP
>> 411](https://openjdk.java.net/jeps/411).
>>
>> The code change is divided into 3 commits. Please review them one by one.
>>
>> 1.
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>> The essential change for this JEP, including the `@Deprecate` annotations
>> and spec change. It also update the default value of the
>> `java.security.manager` system property to "disallow", and necessary test
>> change following this update.
>> 2.
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>> Manual changes to several files so that the next commit can be generated
>> programatically.
>> 3.
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>> Automatic changes to other source files to avoid javac warnings on
>> deprecation for removal
>>
>> The 1st and 2nd commits should be reviewed carefully. The 3rd one is
>> generated programmatically, see the comment below for more details. If you
>> are only interested in a portion of the 3rd commit and would like to review
>> it as a separate file, please comment here and I'll generate an individual
>> webrev.
>>
>> Due to the size of this PR, no attempt is made to update copyright years for
>> any file to minimize unnecessary merge conflict.
>>
>> Furthermore, since the default value of `java.security.manager` system
>> property is now "disallow", most of the tests calling
>> `System.setSecurityManager()` need to launched with
>> `-Djava.security.manager=allow`. This is covered in a different PR at
>> https://github.com/openjdk/jdk/pull/4071.
>>
>> Update: the deprecation annotations and javadoc tags, build, compiler,
>> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are
>> reviewed. Rest are 2d, awt, beans, sound, and swing.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 57: ProcessCommunicator
> 58: .executeChildProcess(Consumer.class, new
> String[0]);
> 59: if (!"Hello".equals(processResults.getStdOut())) {
Who or what prompted this change ?
-
PR: https://git.openjdk.java.net/jdk/pull/4073
RFR: 8256372: [macos] Unexpected symbol was displayed on JTextField with Monospaced font
See the bug report for lots of explanation. The short version (not that short) is that Swing adds a new line char to editable TextComponents It used to work because harfbuzz asked CoreText which mapped it to an invisible glyph Now harfbuzz does its own processing of AAT fonts it is different. Harfbuzz looks up the mapping for this but there is none so it gets a missing glyph. Solution: Since HB now does AAT processing we can dispense with all the special handling of that case and core text and just treat AAT fonts like OT fonts. This means its gets the same mapping we get when not using Layout. And the test passes on all platforms .. not just macOS .. - Commit messages: - 8256372: [macos] Unexpected symbol was displayed on JTextField with Monospaced font Changes: https://git.openjdk.java.net/jdk/pull/4097/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4097=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8256372 Stats: 1425 lines in 9 files changed: 65 ins; 1352 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/4097.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4097/head:pull/4097 PR: https://git.openjdk.java.net/jdk/pull/4097
Re: RFR: 8266545: 8261169 broke Harfbuzz build with gcc 7 and 8 [v2]
On Thu, 6 May 2021 06:58:16 GMT, Thomas Stuefe wrote: >> Harfbuzz upgrade broke Linux x64 build on older gccs. For details see JBS >> issue. >> >> I fixed the issue in the harfbuzz sources, but I am not sure of the policy >> here. Do we modify the harfbuzz sources or leave them untouched? Advice is >> welcome. >> >> The patch fixes linux x64 fastdebug and opt build for me. > > Thomas Stuefe has updated the pull request incrementally with one additional > commit since the last revision: > > switch off warning in build instead of fixing it The policy is to do what you ended up doing. We (almost) never modify 3rd party sources in such cases. About the only case is if a fix has already been pushed upstream then we might copy that fix. You did the right thing notifying upstream but still best to just disable this warning for now. - PR: https://git.openjdk.java.net/jdk/pull/3873
Integrated: 8261169: Upgrade HarfBuzz to the latest 2.8.0
On Fri, 30 Apr 2021 20:07:53 GMT, Phil Race wrote: > Upgrade to harfbuzz 2.8 This pull request has now been integrated. Changeset: 80323b7f Author: Phil Race URL: https://git.openjdk.java.net/jdk/commit/80323b7f66541e24177d02cc668a2eb9267962b9 Stats: 13120 lines in 123 files changed: 7021 ins; 2757 del; 3342 mod 8261169: Upgrade HarfBuzz to the latest 2.8.0 Reviewed-by: serb - PR: https://git.openjdk.java.net/jdk/pull/3826
Re: RFR: 8261169: Upgrade HarfBuzz to the latest 2.8.0
On Sat, 1 May 2021 01:42:04 GMT, Sergey Bylokhov wrote: > I assume the build using bundled lib is fine on all our platforms. Yes. - PR: https://git.openjdk.java.net/jdk/pull/3826
RFR: 8261169: Upgrade HarfBuzz to the latest 2.8.0
Upgrade to harfbuzz 2.8 - Commit messages: - 8261169: Upgrade HarfBuzz to the latest 2.8.0 Changes: https://git.openjdk.java.net/jdk/pull/3826/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3826=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261169 Stats: 13120 lines in 123 files changed: 7021 ins; 2757 del; 3342 mod Patch: https://git.openjdk.java.net/jdk/pull/3826.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3826/head:pull/3826 PR: https://git.openjdk.java.net/jdk/pull/3826
Re: RFR: 8266318: Switch to macos prefix for macOS bundles
On Thu, 29 Apr 2021 19:15:52 GMT, Mikael Vidstedt wrote: > Apple rebranded the operating system as "macOS" back in 2016. The JDK bundles > files are still use the legacy "osx" string. They should be modernized to use > "macos" instead. Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3801
Integrated: 8263833: Stop disabling warnings for sunFont.c with gcc
On Thu, 18 Mar 2021 20:54:27 GMT, Phil Race wrote: > As per the bug report the code that was the reason for disabling warnings on > this file was removed in November 2019 > as part of https://bugs.openjdk.java.net/browse/JDK-8220231 so we can get rid > of the need to disable warnings. > > Longer history in the bug report. This pull request has now been integrated. Changeset: ed1e25d5 Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/ed1e25d5 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod 8263833: Stop disabling warnings for sunFont.c with gcc Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/3081
RFR: 8263833: Stop disabling warnings for sunFont.c with gcc
As per the bug report the code that was the reason for disabling warnings on this file was removed in November 2019 as part of https://bugs.openjdk.java.net/browse/JDK-8220231 so we can get rid of the need to disable warnings. Longer history in the bug report. - Commit messages: - 8263833: Stop disabling warnings for sunFont.c with gcc Changes: https://git.openjdk.java.net/jdk/pull/3081/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3081=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8263833 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3081.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3081/head:pull/3081 PR: https://git.openjdk.java.net/jdk/pull/3081
Integrated: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux
On Sat, 13 Mar 2021 00:15:16 GMT, Phil Race wrote: > From a build perspective this partially reverts > https://bugs.openjdk.java.net/browse/JDK-8249821 except that it keeps > the harfbuzz sources separate and still supports building and running against > a system harfbuzz which is only of interest or relevance on Linux. > > I ended up having to go this way because its is the least unsatisfactory > solution. > I did not want us to build a devkit to link against a system linux only to > find we couldn't use it at runtime > because too many systems have to old a version of harfbuzz. > > This solves the Manjaro Linux problem and I've manually verified building > against a system hardbuxz on Ubuntu 20.10 > > There are couple of incidental fixes in here too > - "libharfbuzz" should not have been in the EXTRA_HEADERS var when building > against a system version > - harfbuzz/hb-ucdn is gone and should not be listed as a header directory > needed to build the bundled copy > - I expect it also resolves https://bugs.openjdk.java.net/browse/JDK-8262502 This pull request has now been integrated. Changeset: 05fe06a6 Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/05fe06a6 Stats: 96 lines in 2 files changed: 7 ins; 51 del; 38 mod 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux Reviewed-by: serb, ihse, azvegint - PR: https://git.openjdk.java.net/jdk/pull/2982
Re: RFR: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux [v2]
On Tue, 16 Mar 2021 10:38:19 GMT, Alexander Zvegintsev wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 456: > >> 454:# when building libharfbuzz >> 455:ifeq ($(call isTargetOs, aix), true) >> 456: LIBHARFBUZZ_CFLAGS += -qdebug=necan > > Should it be `HARFBUZZ_CFLAGS` instead of `LIBHARFBUZZ_CFLAGS`? I don't see > any usage of `LIBHARFBUZZ_CFLAGS` in this makefile. D'oh. It was LIB* in the old code and I copy / pasted and of course can't test that. I've uploaded a fix. - PR: https://git.openjdk.java.net/jdk/pull/2982
Re: RFR: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux [v3]
> From a build perspective this partially reverts > https://bugs.openjdk.java.net/browse/JDK-8249821 except that it keeps > the harfbuzz sources separate and still supports building and running against > a system harfbuzz which is only of interest or relevance on Linux. > > I ended up having to go this way because its is the least unsatisfactory > solution. > I did not want us to build a devkit to link against a system linux only to > find we couldn't use it at runtime > because too many systems have to old a version of harfbuzz. > > This solves the Manjaro Linux problem and I've manually verified building > against a system hardbuxz on Ubuntu 20.10 > > There are couple of incidental fixes in here too > - "libharfbuzz" should not have been in the EXTRA_HEADERS var when building > against a system version > - harfbuzz/hb-ucdn is gone and should not be listed as a header directory > needed to build the bundled copy > - I expect it also resolves https://bugs.openjdk.java.net/browse/JDK-8262502 Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux - Changes: - all: https://git.openjdk.java.net/jdk/pull/2982/files - new: https://git.openjdk.java.net/jdk/pull/2982/files/f668b327..a92a146c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2982=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2982=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2982.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2982/head:pull/2982 PR: https://git.openjdk.java.net/jdk/pull/2982
Re: RFR: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux [v2]
On Mon, 15 Mar 2021 18:52:38 GMT, Magnus Ihse Bursie wrote: > I was actually thinking that such a change would be simpler; more or less > amounting to changing from LIBRARY to STATIC_LIBRARY. But if you feel that it > is too much work, fine... If you say so .. I have no idea what build changes would be needed. I'm just more familiar and comfortable with this and leaving aside the graal stuff I don't know if it is used like that anywhere by the main JDK build. - PR: https://git.openjdk.java.net/jdk/pull/2982
Re: RFR: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux [v2]
> From a build perspective this partially reverts > https://bugs.openjdk.java.net/browse/JDK-8249821 except that it keeps > the harfbuzz sources separate and still supports building and running against > a system harfbuzz which is only of interest or relevance on Linux. > > I ended up having to go this way because its is the least unsatisfactory > solution. > I did not want us to build a devkit to link against a system linux only to > find we couldn't use it at runtime > because too many systems have to old a version of harfbuzz. > > This solves the Manjaro Linux problem and I've manually verified building > against a system hardbuxz on Ubuntu 20.10 > > There are couple of incidental fixes in here too > - "libharfbuzz" should not have been in the EXTRA_HEADERS var when building > against a system version > - harfbuzz/hb-ucdn is gone and should not be listed as a header directory > needed to build the bundled copy > - I expect it also resolves https://bugs.openjdk.java.net/browse/JDK-8262502 Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux - Changes: - all: https://git.openjdk.java.net/jdk/pull/2982/files - new: https://git.openjdk.java.net/jdk/pull/2982/files/29d3e4b2..f668b327 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2982=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2982=00-01 Stats: 10 lines in 1 file changed: 6 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2982.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2982/head:pull/2982 PR: https://git.openjdk.java.net/jdk/pull/2982
Re: RFR: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux
On Sat, 13 Mar 2021 00:15:16 GMT, Phil Race wrote: > From a build perspective this partially reverts > https://bugs.openjdk.java.net/browse/JDK-8249821 except that it keeps > the harfbuzz sources separate and still supports building and running against > a system harfbuzz which is only of interest or relevance on Linux. > > I ended up having to go this way because its is the least unsatisfactory > solution. > I did not want us to build a devkit to link against a system linux only to > find we couldn't use it at runtime > because too many systems have to old a version of harfbuzz. > > This solves the Manjaro Linux problem and I've manually verified building > against a system hardbuxz on Ubuntu 20.10 > > There are couple of incidental fixes in here too > - "libharfbuzz" should not have been in the EXTRA_HEADERS var when building > against a system version > - harfbuzz/hb-ucdn is gone and should not be listed as a header directory > needed to build the bundled copy > - I expect it also resolves https://bugs.openjdk.java.net/browse/JDK-8262502 > Just thinking out loud, you don't think it would be better to build harfbuzz > separately, but as a static library, that is then included in libfontmanager? > The main win of doing that, I think, is the ability to have all the disabled > warnings confined to the lower-quality upstream source. The resulting code > would be the same. And from a build performance perspective I don't think any > way of doing it matters. I don't know that this would be worth such an effort. I've clearly separated the warnings we are disabling for HARFBUZZ and there's really not a lot of likelihood in my mind that these warnings will suddenly also start to apply to the small amount of JDK code that is in libfontmanager which is also much simpler code. BTW I noticed we are still disabling all warnings for sunFont.c and after this is done I'll likely see if I can make a source code change to resolve that. - PR: https://git.openjdk.java.net/jdk/pull/2982
Re: RFR: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux
On Mon, 15 Mar 2021 10:46:45 GMT, Alexander Zvegintsev wrote: >> From a build perspective this partially reverts >> https://bugs.openjdk.java.net/browse/JDK-8249821 except that it keeps >> the harfbuzz sources separate and still supports building and running >> against a system harfbuzz which is only of interest or relevance on Linux. >> >> I ended up having to go this way because its is the least unsatisfactory >> solution. >> I did not want us to build a devkit to link against a system linux only to >> find we couldn't use it at runtime >> because too many systems have to old a version of harfbuzz. >> >> This solves the Manjaro Linux problem and I've manually verified building >> against a system hardbuxz on Ubuntu 20.10 >> >> There are couple of incidental fixes in here too >> - "libharfbuzz" should not have been in the EXTRA_HEADERS var when building >> against a system version >> - harfbuzz/hb-ucdn is gone and should not be listed as a header directory >> needed to build the bundled copy >> - I expect it also resolves https://bugs.openjdk.java.net/browse/JDK-8262502 > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 479: > >> 477: # when building libharfbuzz >> 478: ifeq ($(call isTargetOs, aix), true) >> 479: LIBHARFBUZZ_CFLAGS += -qdebug=necan > > I couldn't find the `-qdebug=necan` in the updated makefile. Is the > [JDK-8258484](https://bugs.openjdk.java.net/browse/JDK-8258484) handled in > some other way? I'll restore this. > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 465: > >> 463: tautological-constant-out-of-range-compare int-to-pointer-cast \ >> 464: undef missing-field-initializers range-loop-analysis >> 465: HARFBUZZ_DISABLED_WARNINGS_microsoft := 4267 4244 4090 4146 4334 >> 4819 4101 4068 4805 4138 > > Looks like that this block has extra indent of 1 space. Yes, I'll update it. - PR: https://git.openjdk.java.net/jdk/pull/2982
RFR: 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux
>From a build perspective this partially reverts >https://bugs.openjdk.java.net/browse/JDK-8249821 except that it keeps the harfbuzz sources separate and still supports building and running against a system harfbuzz which is only of interest or relevance on Linux. I ended up having to go this way because its is the least unsatisfactory solution. I did not want us to build a devkit to link against a system linux only to find we couldn't use it at runtime because too many systems have to old a version of harfbuzz. This solves the Manjaro Linux problem and I've manually verified building against a system hardbuxz on Ubuntu 20.10 There are couple of incidental fixes in here too - "libharfbuzz" should not have been in the EXTRA_HEADERS var when building against a system version - harfbuzz/hb-ucdn is gone and should not be listed as a header directory needed to build the bundled copy - I expect it also resolves https://bugs.openjdk.java.net/browse/JDK-8262502 - Commit messages: - 8255790: GTKL: Java 16 crashes on initialising GTKL on Manjaro Linux Changes: https://git.openjdk.java.net/jdk/pull/2982/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2982=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255790 Stats: 96 lines in 2 files changed: 7 ins; 57 del; 32 mod Patch: https://git.openjdk.java.net/jdk/pull/2982.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2982/head:pull/2982 PR: https://git.openjdk.java.net/jdk/pull/2982
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v13]
On Thu, 11 Mar 2021 18:00:15 GMT, Ajit Ghaisas wrote: >> **Description :** >> This is the implementation of [JEP 382 : New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361) >> It implements a Java 2D internal rendering pipeline for macOS using the >> Apple Metal API. >> The entire work on this was done under [OpenJDK Project - >> Lanai](http://openjdk.java.net/projects/lanai/) >> >> We iterated through several Early Access (EA) builds and have reached a >> stage where it is ready to be integrated to openjdk/jdk. The latest EA build >> is available at - https://jdk.java.net/lanai/ >> >> A new option -Dsun.java2d.metal=true | True needs to be used to use this >> pipeline. >> >> **Testing :** >> This implementation has been tested with the tests present at - [Test Plan >> for JEP 382: New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396) >> >> **Note to reviewers :** >> 1) Default rendering pipeline on macOS has not been changed by this PR. >> OpenGL still stays as the default rendering pipeline and Metal rendering >> pipeline is optional to choose. >> >> 2) To apply and test this PR - >> To enable the metal pipeline you must specify on command line >> -Dsun.java2d.metal=true (No message will be printed in this case) or >> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is >> enabled gets printed) >> >> 3) Review comments (including some preliminary informal review comments) are >> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598 > > Ajit Ghaisas 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 47 additional > commits since the last revision: > > - Lanai PR#214 - 8263324 - avu > - Merge branch 'master' into 8260931_lanai_JEP_branch > - Lanai PR#213 - 8263325 - avu > - Lanai PR#212 - 8259825 - aghaisas > - Lanai PR#211 - 8262882 - aghaisas > - Merge branch 'master' into 8260931_lanai_JEP_branch > - Lanai PR#210 - 8263159 - jdv > - Lanai PR#209 - 8262936 - jdv > - Lanai PR#208 - 8262928 - jdv > - Lanai PR#207 - 8262750 - jdv > - ... and 37 more: > https://git.openjdk.java.net/jdk/compare/ada1a022...369c3d21 Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v8]
On Mon, 15 Feb 2021 20:55:13 GMT, Phil Race wrote: >> Marked as reviewed by gziemski (Committer). > >> > > Thanks @gerard-ziemski for taking a detailed look at this. >> > > We do have an open bug to address this. Please refer >> > > [JDK-8259825](https://bugs.openjdk.java.net/browse/JDK-8259825). >> > >> > >> > Hi Gerard, expecting a process and parsing its output is definitely not >> > ideal and that's why there's this open bug. >> > One thing that is under consideration is to equate >= 10.14 with Metal is >> > available since as of 10.14 macOS won't install if a system does not >> > support metal. We have no compelling reason to support metal on earlier >> > releases anyway .. those are either already unsupported or barely >> > supported and OGL will always be available there. >> >> I did not know that there already was a bug covering this issue. >> >> Your idea seems reasonable. >> >> More than just focusing on this immediate issue, however, I was hoping to >> raise the point of us starting adopting profiling tools to analyze our code >> (memory utilization, leaks, cpu/gpu profiling etc). A new feature that >> brings 10% benefit, but uses 50% more resources for example would probably >> not be a good investment. And to figure that we need to relay on some good >> tools and Xcode provides some. > > There were actually tasks to do profiling of the memory footprint and look > for leaks. We did not think it possible to be able to assert "Metal must use > less memory than OpenGL" or dig into tiny differences but it was intended to > find the big issues. See https://bugs.openjdk.java.net/browse/JDK-8237856 > @prsadhuk maybe able to say how much of it was doing using Xcode. Regarding this comment from @mrserb > Probably place it near J2Dbench? I can't find the context but if it is suggesting moving RenderPerfTest to be co-located with J2Bench let's NOT do that. It is much more likely that J2DBench will be moved out of demo into test ... - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v11]
On Mon, 8 Mar 2021 08:06:03 GMT, Ajit Ghaisas wrote: >> **Description :** >> This is the implementation of [JEP 382 : New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361) >> It implements a Java 2D internal rendering pipeline for macOS using the >> Apple Metal API. >> The entire work on this was done under [OpenJDK Project - >> Lanai](http://openjdk.java.net/projects/lanai/) >> >> We iterated through several Early Access (EA) builds and have reached a >> stage where it is ready to be integrated to openjdk/jdk. The latest EA build >> is available at - https://jdk.java.net/lanai/ >> >> A new option -Dsun.java2d.metal=true | True needs to be used to use this >> pipeline. >> >> **Testing :** >> This implementation has been tested with the tests present at - [Test Plan >> for JEP 382: New macOS Rendering >> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396) >> >> **Note to reviewers :** >> 1) Default rendering pipeline on macOS has not been changed by this PR. >> OpenGL still stays as the default rendering pipeline and Metal rendering >> pipeline is optional to choose. >> >> 2) To apply and test this PR - >> To enable the metal pipeline you must specify on command line >> -Dsun.java2d.metal=true (No message will be printed in this case) or >> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is >> enabled gets printed) >> >> 3) Review comments (including some preliminary informal review comments) are >> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598 > > Ajit Ghaisas 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 41 additional > commits since the last revision: > > - Lanai PR#210 - 8263159 - jdv > - Lanai PR#209 - 8262936 - jdv > - Lanai PR#208 - 8262928 - jdv > - Lanai PR#207 - 8262750 - jdv > - Merge branch 'master' into 8260931_lanai_JEP_branch > - Lanai PR#206 - 8262729 - aghaisas > - Lanai PR#205 - 8262496 - avu > - Lanai PR#203 - 8262313 - jdv > - Lanai PR#202 - 8262293 - avu > - Lanai PR#201 - 8261891 - avu > - ... and 31 more: > https://git.openjdk.java.net/jdk/compare/32b236e4...de456939 test/jdk/performance/client/RenderPerfTest/build.xml line 72: > 70: > 71: > 72: This was presumably borrowed from J2DBench so this comment should be fixed ! - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8263136: C4530 was reported from VS 2019 at access bridge
On Sun, 7 Mar 2021 03:18:53 GMT, Yasumasa Suenaga wrote: > I saw C4530 with VS 2019 (16.9.0) as following (on Japanese locale): > > AccessBridgeDebug.cpp > メモ: インクルード ファイル: > d:\github-forked\jdk\src\jdk.accessibility\windows\native\common\AccessBridgeDebug.h > > : > > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): > error C2220: 次の警 > 告はエラーとして処理されます > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\ostream(611): > warning C4530: C++ > 例外処理を使っていますが、アンワインド セマンティクスは有効にはなりません。/EHsc を指定してください。 > メモ: インクルード ファイル: > c:\progra~2\micros~2\2019\commun~1\vc\tools\msvc\1428~1.299\include\string > > `/EHsc` has been already passed in other makefiles, and also > AccessBridgeDebug.cpp uses some STL classes (e.g. `chrono` namespace). So > `/EHsc` is a solution for this problem. translate.google.com says the error in (almost) English is : c: \ program ~ 2 \ micros ~ 2 \ 2019 \ commun ~ 1 \ vc \ tools \ msvc \ 1428 ~ 1.299 \ include \ ostream (611): warning C4530: C ++ I'm using exception handling, but unwind semantics aren't enabled. Please specify / EHsc. - PR: https://git.openjdk.java.net/jdk/pull/2859
Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v18]
On Wed, 17 Feb 2021 12:36:10 GMT, Anton Kozlov wrote: >> Please review the implementation of JEP 391: macOS/AArch64 Port. >> >> It's heavily based on existing ports to linux/aarch64, macos/x86_64, and >> windows/aarch64. >> >> Major changes are in: >> * src/hotspot/cpu/aarch64: support of the new calling convention (subtasks >> JDK-8253817, JDK-8253818) >> * src/hotspot/os_cpu/bsd_aarch64: copy of os_cpu/linux_aarch64 with >> necessary adjustments (JDK-8253819) >> * src/hotspot/share, test/hotspot/gtest: support of write-xor-execute (W^X), >> required on macOS/AArch64 platform. It's implemented with >> pthread_jit_write_protect_np provided by Apple. The W^X mode is local to a >> thread, so W^X mode change relates to the java thread state change (for java >> threads). In most cases, JVM executes in write-only mode, except when >> calling a generated stub like SafeFetch, which requires a temporary switch >> to execute-only mode. The same execute-only mode is enabled when a java >> thread executes in java or native states. This approach of managing W^X mode >> turned out to be simple and efficient enough. >> * src/jdk.hotspot.agent: serviceability agent implementation (JDK-8254941) > > Anton Kozlov has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 88 commits: > > - Merge remote-tracking branch 'upstream/jdk/master' into jdk-macos > - Re-do safefetch.hpp > - Merge remote-tracking branch 'origin/jdk/8261075-stubroutines-inline' into > jdk-macos > - stubRoutines.inline.hpp -> safefetch.hpp > - Update copyrights > - Merge remote-tracking branch 'upstream/jdk/master' into > 8261075-stubroutines-inline > - Merge remote-tracking branch 'upstream/jdk/master' into > 8261075-stubroutines-inline > - Extract SafeFetch32/N to stubRoutines.inline.hpp > - Revert "Extract SafeFetch32/N to stubRoutines.inline.hpp" > >This reverts commit b873c25f31dd21349d140b790713cc9ccb5f2dc0. > - Merge pull request #9 from VladimirKempik/pull/2200 > >Removed unused variables > - ... and 78 more: > https://git.openjdk.java.net/jdk/compare/b955f85e...ab72613c Looks like the compiler warning changess are now the only desktop changes. That is fine by me. - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2200
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v8]
On Mon, 15 Feb 2021 20:52:09 GMT, Gerard Ziemski wrote: >> Ajit Ghaisas 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 20 additional >> commits since the last revision: >> >> - Lanai PR#191 - 8261705 - jdv >> - Lanai PR#190 - 8261706 - jdv >> - Lanai PR#189 - 8261712 - avu >> - Lanai PR#187 - 8261704 - jdv >> - Lanai PR#186 - 8261638 - avu >> - Lanai PR#185 - 8261632 - jdv >> - Lanai PR#184 - 8261620 - aghaisas >> - Lanai PR#182 - 8261547 - psadhukhan >> - Merge branch 'master' into 8260931_lanai_JEP_branch >> - Lanai PR#181 - 8261143 - aghaisas >> - ... and 10 more: >> https://git.openjdk.java.net/jdk/compare/c8554bef...7b0b0dc4 > > Marked as reviewed by gziemski (Committer). > > > Thanks @gerard-ziemski for taking a detailed look at this. > > > We do have an open bug to address this. Please refer > > > [JDK-8259825](https://bugs.openjdk.java.net/browse/JDK-8259825). > > > > > > Hi Gerard, expecting a process and parsing its output is definitely not > > ideal and that's why there's this open bug. > > One thing that is under consideration is to equate >= 10.14 with Metal is > > available since as of 10.14 macOS won't install if a system does not > > support metal. We have no compelling reason to support metal on earlier > > releases anyway .. those are either already unsupported or barely supported > > and OGL will always be available there. > > I did not know that there already was a bug covering this issue. > > Your idea seems reasonable. > > More than just focusing on this immediate issue, however, I was hoping to > raise the point of us starting adopting profiling tools to analyze our code > (memory utilization, leaks, cpu/gpu profiling etc). A new feature that brings > 10% benefit, but uses 50% more resources for example would probably not be a > good investment. And to figure that we need to relay on some good tools and > Xcode provides some. There were actually tasks to do profiling of the memory footprint and look for leaks. We did not think it possible to be able to assert "Metal must use less memory than OpenGL" or dig into tiny differences but it was intended to find the big issues. See https://bugs.openjdk.java.net/browse/JDK-8237856 @prsadhuk maybe able to say how much of it was doing using Xcode. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Mon, 15 Feb 2021 18:30:51 GMT, Gerard Ziemski wrote: >> Changes requested by gziemski (Committer). > > I took a look at https://bugs.openjdk.java.net/browse/JDK-8261408 and noticed > a startup time regression with the Metal rendering pipeline, so I dug a bit > and here is what I found using Xcode startup profiler: > > Here is the OpenGL pipeline: > src="https://user-images.githubusercontent.com/63425797/107981859-8efb4780-6f88-11eb-849a-385c29f3ff6f.png;> > > > Here is the Metal pipeline: > src="https://user-images.githubusercontent.com/63425797/107981882-9ae70980-6f88-11eb-9b73-d1bd19dfa07f.png;> > > If I read the results correctly, there is a weird 330ms time gap in the case > of the Metal pipeline where nothing is done and it looks like the culprit is > `Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable` > > The OpenGL pipeline gets blocked only for 83ms in > `Java_sun_java2d_opengl_CGLGraphicsConfig_getCGLConfigInfo` > > I would advice that we take a closer look at why > 'Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable' takes so > long, and optimize it, or cache the results, so the next VM instance can skip > it if needed (assuming the call doesn't end up initializing the native Metal > or something like that), still worth taking a look. > > I would also recommend that you adopt Xcode and its Instruments profiler > tools for future work. Please let me know if you need help in this area. > > I only scratched the surface here and I think that there is plenty of > profiling that can be done to investigate the startup time regression further. > Thanks @gerard-ziemski for taking a detailed look at this. > We do have an open bug to address this. Please refer > [JDK-8259825](https://bugs.openjdk.java.net/browse/JDK-8259825). Hi Gerard, expecting a process and parsing its output is definitely not ideal and that's why there's this open bug. One thing that is under consideration is to equate >= 10.14 with Metal is available since as of 10.14 macOS won't install if a system does not support metal. We have no compelling reason to support metal on earlier releases anyway .. those are either already unsupported or barely supported and OGL will always be available there. - PR: https://git.openjdk.java.net/jdk/pull/2403
Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v7]
On Thu, 11 Feb 2021 21:04:16 GMT, Gerard Ziemski wrote: >> I guess you will only see this if `Metal API Validation Enabled` > > Which actually begs a question of whether we tested with `Metal API > Validation Enabled` ? I submitted https://bugs.openjdk.java.net/browse/JDK-8261620 for this bug. Could be that there are more such issues but since this crash occurs on start up I can't say what else there might be. - PR: https://git.openjdk.java.net/jdk/pull/2403
Integrated: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4
On Thu, 4 Feb 2021 02:32:31 GMT, Phil Race wrote: > remove un-needed disabling now JNF has gone .. This pull request has now been integrated. Changeset: 3bb6a3d2 Author: Phil Race URL: https://git.openjdk.java.net/jdk/commit/3bb6a3d2 Stats: 8 lines in 2 files changed: 0 ins; 8 del; 0 mod 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4 Reviewed-by: serb, ihse, erikj - PR: https://git.openjdk.java.net/jdk/pull/2396
Re: RFR: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4 [v2]
> remove un-needed disabling now JNF has gone .. Phil Race has updated the pull request incrementally with one additional commit since the last revision: Remove condition that should have been fixed as part of 8257858 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2396/files - new: https://git.openjdk.java.net/jdk/pull/2396/files/34dcbfb1..93fd193f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2396=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2396=00-01 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2396/head:pull/2396 PR: https://git.openjdk.java.net/jdk/pull/2396
Re: RFR: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4 [v2]
On Thu, 4 Feb 2021 11:42:48 GMT, Magnus Ihse Bursie wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove condition that should have been fixed as part of 8257858 > > Marked as reviewed by ihse (Reviewer). Magnus pointed out a condition that I think should have been removed in the fix for 8257858 : Remove JNF dependency from libosxsecurity/KeystoreImpl.m since its a build related change too, and I've verified that after removing it we still build, I am rolling it in here, if that's OK with folks - PR: https://git.openjdk.java.net/jdk/pull/2396
RFR: 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4
remove un-needed disabling now JNF has gone .. - Commit messages: - 8261109: [macOS] Remove disabled warning for JNF in make/autoconf/flags-cflags.m4 Changes: https://git.openjdk.java.net/jdk/pull/2396/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2396=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261109 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2396/head:pull/2396 PR: https://git.openjdk.java.net/jdk/pull/2396
Integrated: 8260616: Removing remaining JNF dependencies in the java.desktop module
On Fri, 29 Jan 2021 00:30:21 GMT, Phil Race wrote: > This completes the desktop module JNF removal > > * remove -framework JavaNativeFoundation from make files > > * remove #import from all > source files. If needed add import of JNIUtilities.h to get jni.h definitions > - better anyway since then it gets the current JDK ones not the ones from the > O/S > > * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString > with JavaStringToNSString > > * replace JNFNormalizedNSStringForPath with > NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with > NormalizedPathJavaStringFromNSString > > * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI > > * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast > majority already did this) > > * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject > perform* methods. > > * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and > use where needed. > > * Remove the single usage of JNFPerformEnvBlock > > * replace JNFJavaToNSNumber in single A11Y file with local replacement > > * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with > local replacement > > * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m > > * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) This pull request has now been integrated. Changeset: 8760688d Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/8760688d Stats: 580 lines in 74 files changed: 243 ins; 99 del; 238 mod 8260616: Removing remaining JNF dependencies in the java.desktop module Reviewed-by: gziemski, ihse, serb - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v5]
> This completes the desktop module JNF removal > > * remove -framework JavaNativeFoundation from make files > > * remove #import from all > source files. If needed add import of JNIUtilities.h to get jni.h definitions > - better anyway since then it gets the current JDK ones not the ones from the > O/S > > * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString > with JavaStringToNSString > > * replace JNFNormalizedNSStringForPath with > NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with > NormalizedPathJavaStringFromNSString > > * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI > > * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast > majority already did this) > > * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject > perform* methods. > > * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and > use where needed. > > * Remove the single usage of JNFPerformEnvBlock > > * replace JNFJavaToNSNumber in single A11Y file with local replacement > > * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with > local replacement > > * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m > > * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) Phil Race has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge branch 'master' into jnf_string - 8260616: Removing remaining JNF dependencies in the java.desktop module - 8260616: Removing remaining JNF dependencies in the java.desktop module - Merge branch 'master' into jnf_string - 8260616: Removing remaining JNF dependencies in the java.desktop module - 8260616: Removing remaining JNF dependencies in the java.desktop modul - 8260616: Removing remaining JNF dependencies in the java.desktop module - Changes: - all: https://git.openjdk.java.net/jdk/pull/2305/files - new: https://git.openjdk.java.net/jdk/pull/2305/files/8a014fa6..43880e5f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2305=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2305=03-04 Stats: 3151 lines in 249 files changed: 1263 ins; 952 del; 936 mod Patch: https://git.openjdk.java.net/jdk/pull/2305.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2305/head:pull/2305 PR: https://git.openjdk.java.net/jdk/pull/2305
Integrated: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Thu, 28 Jan 2021 22:40:57 GMT, Phil Race wrote: > This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? This pull request has now been integrated. Changeset: 2be60e37 Author:Phil Race URL: https://git.openjdk.java.net/jdk/commit/2be60e37 Stats: 45 lines in 2 files changed: 35 ins; 2 del; 8 mod 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m Reviewed-by: ihse, cjplummer - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
On Tue, 2 Feb 2021 20:27:17 GMT, Chris Plummer wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
>
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 294:
>
>> 292:
>> 293: NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
>> 294: @try {
>
> Although there are only 3 places where the `JNF_COCOA_ENTER/EXIT` macros are
> used, it seems it would still be worth taking the same approach you did for
> `java.desktop` and add the replacement macros instead of inlining them. So
> just copy what you added to
> `src/java.desktop/macosx/native/libosxapp/JNIUtilities.h`, and replace
> `JNF_COCOA_ENTER` with `JNI_COCOA_ENTER/EXIT`. Otherwise the
> `JNF_COCOA_ENTER/EXIT` changes look fine to me, but I'm just basing this on a
> comparison with what you've done with `java.desktop`. I'm no expert in this
> area.
OK .. I don't really mind either way and if this helps gets it pushed .. so
I've updated.
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 296:
>
>> 294: @try {
>> 295:
>> 296: NSString *symbolNameString = JavaStringToNSString(env, symbolName);
>
> Is there a reason why `java.desktop` continues to use `JNFJavaToNSString`? I
> was looking to see how this was handled in other places, but I couldn't find
> an example where `JNFJavaToNSString` was converted to call a newly
> implemented `JavaStringToNSString`.
As Magnus said that is in progress. Hoping it will be pushed very soon.
-
PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
> This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m - Changes: - all: https://git.openjdk.java.net/jdk/pull/2304/files - new: https://git.openjdk.java.net/jdk/pull/2304/files/9919a02c..d7ed0158 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2304=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2304=00-01 Stats: 39 lines in 1 file changed: 18 ins; 15 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2304/head:pull/2304 PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Tue, 2 Feb 2021 21:26:28 GMT, Kevin Rushforth wrote: >> The changes look good to me, but please see my comment from my review about >> documenting `NormalizedPathNSStringFromJavaString()` API. > > The following commit was integrated to jdk master since you created this > branch: > > acbcde8 : [JDK-8256111](https://bugs.openjdk.java.net/browse/JDK-8256111) : > Create implementation for NSAccessibilityStaticText protocol > > with a new reference to JNF, so it fails to compile with this patch. You will > need to merge openjdk/jdk master into your PR branch and then fix the new JNF > reference. Updated as follows - merged to current master and fixed the new A11Y usage of JNF - added comment provided by Gerard - removed comments as suggested by Sergey - made failure of GetStringChars in CTextPipe throw OOME - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v4]
> This completes the desktop module JNF removal > > * remove -framework JavaNativeFoundation from make files > > * remove #import from all > source files. If needed add import of JNIUtilities.h to get jni.h definitions > - better anyway since then it gets the current JDK ones not the ones from the > O/S > > * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString > with JavaStringToNSString > > * replace JNFNormalizedNSStringForPath with > NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with > NormalizedPathJavaStringFromNSString > > * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI > > * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast > majority already did this) > > * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject > perform* methods. > > * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and > use where needed. > > * Remove the single usage of JNFPerformEnvBlock > > * replace JNFJavaToNSNumber in single A11Y file with local replacement > > * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with > local replacement > > * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m > > * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) Phil Race has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - 8260616: Removing remaining JNF dependencies in the java.desktop module - 8260616: Removing remaining JNF dependencies in the java.desktop module - Merge branch 'master' into jnf_string - 8260616: Removing remaining JNF dependencies in the java.desktop module - 8260616: Removing remaining JNF dependencies in the java.desktop modul - 8260616: Removing remaining JNF dependencies in the java.desktop module - Changes: - all: https://git.openjdk.java.net/jdk/pull/2305/files - new: https://git.openjdk.java.net/jdk/pull/2305/files/7ea57c85..8a014fa6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2305=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2305=02-03 Stats: 34374 lines in 817 files changed: 13984 ins; 9392 del; 10998 mod Patch: https://git.openjdk.java.net/jdk/pull/2305.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2305/head:pull/2305 PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Tue, 2 Feb 2021 22:33:09 GMT, Phil Race wrote: >> I read it and not sure that it is fine to ignore this error, why not throw >> an exception and signal the CTextPipe_doDrawString that an error occurred >> like InvalidPipeException or something(Sometimes we wrap other exception >> like OOM into the InvalidPipeException and this seems similar case)? > >> The changes look good to me, but please see my comment from my review about >> documenting `NormalizedPathNSStringFromJavaString()` API. > > I see it. I will copy what you wrote in there. > I read it and not sure that it is fine to ignore this error, why not throw an > exception and signal the CTextPipe_doDrawString that an error occurred like > InvalidPipeException or something(Sometimes we wrap other exception like OOM > into the InvalidPipeException and this seems similar case)? OK. I will do something like throw OOME or InternalError. Since we already checked for NULL as an arg any failure here implies a deep VM problem rather than anytjhing like a 2D invalid pipe - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Tue, 2 Feb 2021 21:48:36 GMT, Sergey Bylokhov wrote: >> Look a few lines further up at my reply 3 days ago Gerard about this. > > I read it and not sure that it is fine to ignore this error, why not throw an > exception and signal the CTextPipe_doDrawString that an error occurred like > InvalidPipeException or something(Sometimes we wrap other exception like OOM > into the InvalidPipeException and this seems similar case)? > The changes look good to me, but please see my comment from my review about > documenting `NormalizedPathNSStringFromJavaString()` API. I see it. I will copy what you wrote in there. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Tue, 2 Feb 2021 22:02:14 GMT, Sergey Bylokhov wrote: >> I ran some tests embedding JavaFX into Swing and vice versa both with and >> without `-Djavafx.embed.singleThread=true` and I don't see any regression in >> behavior. > > I am mostly worried about the usage of JNF by someone else's native code, as > far as I understand it could be "broken" now. But it is good that FX does not > use it. > > BTW looks like all comments like "// AWT_THREADING Safe (AWTRunLoop)" could > be removed now. SWT does not use JNF at all. I've looked at their source code. I can clean up those comments. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
On Mon, 1 Feb 2021 22:17:38 GMT, Sergey Bylokhov wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8260616: Removing remaining JNF dependencies in the java.desktop module
>
> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 611:
>
>> 609: const jchar *unichars = (*env)->GetStringChars(env, str, NULL);
>> 610: if (unichars == NULL) {
>> 611: return;
>
> Do not we need to throw an exception here? Otherwise, GetStringChars error
> will be ignored?
Look a few lines further up at my reply 3 days ago Gerard about this.
> src/java.desktop/macosx/native/libawt_lwawt/awt/JavaComponentAccessibility.m
> line 967:
>
>> 965: static NSNumber* JavaNumberToNSNumber(JNIEnv *env, jobject jnumber) {
>> 966: if (jnumber == NULL) {
>> 967: return nil;
>
> Based on its usage it is probably should be zero on NULL number?
Not an unreasonable idea and I considered it but :
- It is never called with NULL. There is always a null check
- The JNF equivalent returns nil on NULL
BTW two of the functions in which the code appears :
accessibilityMinValueAttribute and accessibilityMaxValueAttribute (SFAIC)
aren't used anywhere.
> src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 30:
>
>> 28: NSString* JavaStringToNSString(JNIEnv *env, jstring jstr) {
>> 29: if (jstr == NULL) {
>> 30: return NULL;
>
> In other methods, the nil is used
Not understanding what you mean ? This is the same behavior as the function
being replaced.
> src/java.desktop/macosx/native/libosxapp/ThreadUtilities.m line 53:
>
>> 51: @implementation ThreadUtilities
>> 52:
>> 53: + (void)initialize {
>
> I think we need to check how this new modes will work when the AWT is
> embedded inside SWT and FX.
We are just specifying an additional run mode for JDK internal use.
It means that when we are saying to process only events for that mode, then
only those will be processed.
And it is used only for nested event loops.
Nothing eternal should be assuming AWT uses JNF at all, never mind in a
particular way.
There is a special case for FX but I don't see a problem.
If you look at Java_sun_lwawt_macosx_LWCToolkit_doAWTRunLoopImpl
there's a variable "inAWT".
isRunning = [[NSRunLoop currentRunLoop] runMode:(inAWT ? [JNFRunLoop
javaRunLoopMode] : NSDefaultRunLoopMode) ... ]];
This is specified as
inAWT = AccessController.doPrivileged(new PrivilegedAction() {
@Override
public Boolean run() {
return
!Boolean.parseBoolean(System.getProperty("javafx.embed.singleThread", "false"));
}
});
}
So generally FX doesn't care, and is ignorant of this new mode.
Unless you set that property to true, in which case AWT use the
NSDefaultRunLoopMode and so again FX is unaffected.
Nothing in the FX sources goes anywhere near JNF .. or has a reference to the
special mode.
Bottom line I don't see it being affected.
I'll check with Kevin but also Gerard had a lot to do with the creation of the
current FX toolkit.
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v3]
> This completes the desktop module JNF removal > > * remove -framework JavaNativeFoundation from make files > > * remove #import from all > source files. If needed add import of JNIUtilities.h to get jni.h definitions > - better anyway since then it gets the current JDK ones not the ones from the > O/S > > * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString > with JavaStringToNSString > > * replace JNFNormalizedNSStringForPath with > NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with > NormalizedPathJavaStringFromNSString > > * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI > > * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast > majority already did this) > > * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject > perform* methods. > > * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and > use where needed. > > * Remove the single usage of JNFPerformEnvBlock > > * replace JNFJavaToNSNumber in single A11Y file with local replacement > > * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with > local replacement > > * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m > > * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8260616: Removing remaining JNF dependencies in the java.desktop module - Changes: - all: https://git.openjdk.java.net/jdk/pull/2305/files - new: https://git.openjdk.java.net/jdk/pull/2305/files/efab1de5..7ea57c85 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2305=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2305=01-02 Stats: 46 lines in 3 files changed: 34 ins; 1 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/2305.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2305/head:pull/2305 PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Mon, 1 Feb 2021 11:35:22 GMT, Magnus Ihse Bursie wrote: >> Phil Race has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8260616: Removing remaining JNF dependencies in the java.desktop modul > > Build changes look good for this PR. > * There is also a test dependency that I have seen being addressed, in > `make/test/JtregNativeJdk.gmk` line 82, for `libTestMainKeyWindow`. update on this. It Is used by precisely one jtreg AWT test. So I guess it is fair game to add resolving this to the desktop module update. I am currently testing it before committing - will take a couple of hours as the headful tests take a while to run. - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Fri, 29 Jan 2021 17:22:49 GMT, Phil Race wrote: >> Build changes look good. > >> Is it because they are not in a place that can be accessed from this file? > Right 99% of JNF usage was the desktop module many, many files, 1300 > references .. the entire rest of the JDK had just 3 files each in a different > module ! It did not seem worth creating a JDK-wide platform-specific library > to support this. > For testing you want test/jdk/sun/tools/jhsdb/ and > test/hotspot/jtreg/serviceability/sa These tests passed with my changes - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v3]
On Fri, 29 Jan 2021 22:47:52 GMT, Weijun Wang wrote: >> This fix covers both >> >> - [[macOS]: Remove JNF dependency from >> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858) >> - [[macOS]: Remove JNF dependency from >> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860) > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > phil comment Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v2]
On Fri, 29 Jan 2021 21:47:32 GMT, Weijun Wang wrote:
>> src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 619:
>>
>>> 617: (*env)->ReleaseCharArrayElements(env, passwordObj,
>>> passwordChars,
>>> 618: JNI_ABORT);
>>> 619: }
>>
>> Although you have it in the later code, here you are missing
>> @catch (NSException *e) {
>> NSLog(@"%@", [e callStackSymbols]);
>> }
>
> Will add.
>
> BTW, will these be shown on stdout or stderr? If so, is this a good idea?
should be stderr. Whether to log is up to you. You could wrap it in something
that checks for a debug build.
The idea is that if this ever happens there is a serious problem. NSException
is only thrown (by the OS frameworks) in supposedly fatal scenarios.
>> src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line
>> 57:
>>
>>> 55: }
>>> 56: }
>>> 57: (*localVM)->DetachCurrentThread(localVM);
>>
>> I think you only want to detach if you actually attached ! you don't want to
>> be detaching VM threads.
>
> So I should remember how env was retrieved and only detach when it's from
> `AttachCurrentThreadAsDaemon`? In fact, in my test the plain `GetEnv` has
> never succeeded and it was always attached.
Yes that is the idea. BTW which thread was it attached to in what you saw ?
-
PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8254702: jpackage app launcher crashes on CentOS
On Fri, 29 Jan 2021 21:33:40 GMT, Alexey Semenyuk wrote: > Fix for https://bugs.openjdk.java.net/browse/JDK-8254702 > > The fix splits Linux app launcher in app launcher and launcher shared lib. > App launcher is pure C and doesn't have C++ code. App launcher lib > incorporates bulk of C++ code from app launcher. > At startup app launcher loads launcher shared lib and calls functions it > provides to get data for launching JVM (path to jli lib and arguments for > JLI_Launch function call). > App launcher unloads launcher shared lib before launching JVM to remove C++ > runtime from the process memory. > > Getting rid of C++ code from app launcher required to rewrite app > installation location lookup code from C++ to C. LinuxPackage.c source is C > alternative for > https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Package.cpp > and > https://github.com/openjdk/jdk/blob/master/src/jdk.jpackage/linux/native/applauncher/Executor.cpp. > > Layout of jpackage's native code changed: > - `common`: code shared between all jpackage binaries. > - `applauncher`: launcher only code. > - `applauncherlib`: launcher lib code on Linux and launcher code on other > platforms. > - `applaunchercommon`: code shared between launcher and launcher lib on Linux > and launcher code on other platforms. So after this change if you bundle and run an app on Linux and then do "ps" .. what is shown to be running ? Java or the app-name you expected ? - PR: https://git.openjdk.java.net/jdk/pull/2320
Re: RFR: 8257858: [macOS]: Remove JNF dependency from libosxsecurity/KeystoreImpl.m [v2]
On Fri, 29 Jan 2021 14:57:56 GMT, Weijun Wang wrote:
>> This fix covers both
>>
>> - [[macOS]: Remove JNF dependency from
>> libosxsecurity/KeystoreImpl.m](https://bugs.openjdk.java.net/browse/JDK-8257858)
>> - [[macOS]: Remove JNF dependency from
>> libosxkrb5/SCDynamicStoreConfig.m](https://bugs.openjdk.java.net/browse/JDK-8257860)
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> same behavior as before -- empty realm map
make/modules/java.security.jgss/Lib.gmk line 84:
> 82: $(call SET_SHARED_LIBRARY_ORIGIN), \
> 83: LIBS := -framework Cocoa -framework SystemConfiguration \
> 84: -framework Kerberos -ljava, \
The need to add -ljava is interesting. It implies we were getting something
from the platform that usually we'd expect to come from the JDK itself ??
src/java.base/macosx/classes/apple/security/KeychainStore.java line 820:
> 818: private void createKeyEntry(String alias, long creationDate, long
> secKeyRef,
> 819: long[] secCertificateRefs, byte[][]
> rawCertData) {
> 820: KeyEntry ke = new KeyEntry();
removing these exceptions is presumably just clean up - not directly related ??
src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 28:
> 26: #import "apple_security_KeychainStore.h"
> 27: #include "jni.h"
> 28: #include "jni_util.h"
jni_util.h includes jni.h so I don't understand the need for this change.
Also why did you change import to include ? import is the Obj-C norm ...
src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 619:
> 617: (*env)->ReleaseCharArrayElements(env, passwordObj,
> passwordChars,
> 618: JNI_ABORT);
> 619: }
Although you have it in the later code, here you are missing
@catch (NSException *e) {
NSLog(@"%@", [e callStackSymbols]);
}
src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 41:
> 39: if ([keys count] == 0) return;
> 40: if (![keys containsObject:KERBEROS_DEFAULT_REALMS] && ![keys
> containsObject:KERBEROS_DEFAULT_REALM_MAPPINGS]) return;
> 41: //JNFPerformEnvBlock(JNFThreadDetachOnThreadDeath |
> JNFThreadSetSystemClassLoaderOnAttach | JNFThreadAttachAsDaemon, ^(JNIEnv
> *env) {
remove commented out code
src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m line 57:
> 55: }
> 56: }
> 57: (*localVM)->DetachCurrentThread(localVM);
I think you only want to detach if you actually attached ! you don't want to be
detaching VM threads.
-
PR: https://git.openjdk.java.net/jdk/pull/1845
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Fri, 29 Jan 2021 16:23:20 GMT, Gerard Ziemski wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8260616: Removing remaining JNF dependencies in the java.desktop modul
>
> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 601:
>
>> 599: jchar unichars[len];
>> 600: (*env)->GetStringRegion(env, str, 0, len, unichars);
>> 601: CHECK_EXCEPTION();
>
> Are `JNF_CHECK_AND_RETHROW_EXCEPTION` and `CHECK_EXCEPTION` equivalent?
>
> `JNF_CHECK_AND_RETHROW_EXCEPTION` seems to throw exception, but
> `CHECK_EXCEPTION` does not?
JNF_CHECK_AND_RETHROW_EXCEPTION is this
// JNF_CHECK_AND_RETHROW_EXCEPTION - rethrows exceptions from Java
//
// Takes an exception thrown from Java, and transforms it into an
// NSException. The NSException should bubble up to the upper-most
// JNF_COCOA_ENTER/JNF_COCOA_EXIT pair, and then be re-thrown as
// a Java exception when returning from JNI. This check should be
// done after raw JNI operations which could cause a Java exception
// to be be thrown. The JNF{Get/Set/Call} macros below do this
// check automatically.
#define JNF_CHECK_AND_RETHROW_EXCEPTION(env)
\
{
\
jthrowable _exception = (*env)->ExceptionOccurred(env);
\
if (_exception) [JNFException raise:env throwable:_exception]; \
}
So what it actually does is clear the exception, raise an NSException and when
the code reaches JNF_COCOA_EXIT - which then has to be there - it clears the
NSException
and re-throws the Java exception.
So the name of the macro is misleading since this does NOT itself rethrow the
exception,
it relies on other code to do that.
CHECK_EXCEPTION will amount to the same - it throws an NSException if there is
a Java exception pending.
And it will clear the NSException before exiting back to Java.
The difference is that it doesn't clear the Java Exception and later rethrow it
since there is no need
There is one exception to this in CHECK_EXCEPTION - if this is the main (ie
appkit) thread the java exception is cleared since there's no one to return it
to ...
> src/java.desktop/macosx/native/libawt_lwawt/awt/CTextPipe.m line 608:
>
>> 606: {
>> 607: // Get string to draw and the length
>> 608: const jchar *unichars = JNFGetStringUTF16UniChars(env, str);
>
> According to `JNFString.h`, the `JNFGetStringUTF16UniChars()` API:
>
> /*
> * Gets UTF16 unichars from a Java string, and checks for errors and raises a
> JNFException if
> * the unichars cannot be obtained from Java.
> */
>
> raises a (JNFException) if it can't get the chars, but now we simply return
> if `GetStringChars()` can not return the chars.
>
> Seems like we handle this case differently in the new code now - is this
> change desired and correct?
You are correct, but I decided that here it is fine.
If GetStringChars fails it will return NULL (it does not raise any excptions
itself).
I added a check for NULL if NULL we then return from the JNI method to Java.
In the old case it also would return but would additionally propagate that
raised exception to Java which JNF
decided to throw into the mix when there's already a standard way of testing
failure.
And since we already know str != NULL we are in the realms of something went
badly wrong inside the VM for
this to fail. So I decided this was all fine.
> src/java.desktop/macosx/native/libosxapp/JNIUtilities.m line 83:
>
>> 81: stringWithFileSystemRepresentation:chs length:len];
>> 82: return result;
>> 83: }
>
> Why are we doing:
>
> `java_string -> chars -> NSString -> chars -> [NSFileManager
> stringWithFileSystemRepresentation]`
>
> ?
>
> Why not just get the raw characters form JNI and feed them directly into
> `[NSFileManager stringWithFileSystemRepresentation]`, ie:
>
> `java_string -> chars -> [NSFileManager stringWithFileSystemRepresentation]`
>
> and skip the `JavaStringToNSString` step altogether?
I tried to explain the odd-ness of this in the comments preceding the function
definition.
Near as I could figure out that intermediate call is needed to do the
decomposition since the NSFileManager won't do that.
But this is not exactly my area of expertise and there may be a better way. Who
is an expert on the intricacies of the macOS file system naming ?
-
PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Fri, 29 Jan 2021 10:01:42 GMT, Magnus Ihse Bursie wrote: >> This removes the JNF dependency from the jdk.hotspot.agent module. >> The macro expansions are the same as already used in the Java desktop module >> - which actually uses a macro >> still but there there are hundreds of uses. >> The function of this macro code is to prevent NSExceptions escaping to Java >> and also to drain the auto-release pool. >> What test group would be good for verifying this change ? > > Build changes look good. > Is it because they are not in a place that can be accessed from this file? Right 99% of JNF usage was the desktop module many, many files, 1300 references .. the entire rest of the JDK had just 3 files each in a different module ! It did not seem worth creating a JDK-wide platform-specific library to support this. - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
On Fri, 29 Jan 2021 17:02:30 GMT, Gerard Ziemski wrote: >> Changes requested by gziemski (Committer). > > Lots of small changes you had to handle here. Thank you for doing it! I pushed a commit with the remaining -lframework ... removals in the desktop makefiles that I had somehow missed ... - PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module [v2]
> This completes the desktop module JNF removal > > * remove -framework JavaNativeFoundation from make files > > * remove #import from all > source files. If needed add import of JNIUtilities.h to get jni.h definitions > - better anyway since then it gets the current JDK ones not the ones from the > O/S > > * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString > with JavaStringToNSString > > * replace JNFNormalizedNSStringForPath with > NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with > NormalizedPathJavaStringFromNSString > > * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI > > * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast > majority already did this) > > * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject > perform* methods. > > * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and > use where needed. > > * Remove the single usage of JNFPerformEnvBlock > > * replace JNFJavaToNSNumber in single A11Y file with local replacement > > * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with > local replacement > > * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m > > * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8260616: Removing remaining JNF dependencies in the java.desktop modul - Changes: - all: https://git.openjdk.java.net/jdk/pull/2305/files - new: https://git.openjdk.java.net/jdk/pull/2305/files/1951d6d8..efab1de5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2305=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2305=00-01 Stats: 3 lines in 2 files changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2305.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2305/head:pull/2305 PR: https://git.openjdk.java.net/jdk/pull/2305
Re: RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module
On Fri, 29 Jan 2021 10:53:42 GMT, Magnus Ihse Bursie wrote: >> This completes the desktop module JNF removal >> >> * remove -framework JavaNativeFoundation from make files >> >> * remove #import from all >> source files. If needed add import of JNIUtilities.h to get jni.h >> definitions - better anyway since then it gets the current JDK ones not the >> ones from the O/S >> >> * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString >> with JavaStringToNSString >> >> * replace JNFNormalizedNSStringForPath with >> NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with >> NormalizedPathJavaStringFromNSString >> >> * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI >> >> * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the >> vast majority already did this) >> >> * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject >> perform* methods. >> >> * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and >> use where needed. >> >> * Remove the single usage of JNFPerformEnvBlock >> >> * replace JNFJavaToNSNumber in single A11Y file with local replacement >> >> * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with >> local replacement >> >> * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m >> >> * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) > > I mostly have questions about what is missing from this PR. :-) (If this is > supposed to remove the final remnants of JNF) > > - There is a disabled warning in `make/autoconf/flags-cflags.m4`, line 173, > referring to JavaNativeFoundation. It can presumably be removed. If it > triggers individually instead, the warning should be disabled on a > per-library basis. > > - In `make/modules/java.base/Lib.gmk`, line 99 & 113, are references to > JavaNativeFoundation. It seems that `libosxsecurity` needs to be cleaned from > JNF as well. Also, the comments indicate that the exception for STATIC_BUILD > can be removed. (You can verify this with `configure --enable-static-build`) > > - In `make/modules/java.desktop/Lib.gmk`, line 129, and > `make/modules/java.desktop/lib/Awt2dLibraries.gmk`, line 866 & 094, it seems > like `libosx`, `libawt_lwawt`, and `liboxui` also has JNF that needs to be > removed. If these are fixed in any of the other issues for the umbrella > JDK-8257852, I apologize. I could not figure that out. > > - There is also a test dependency that I have seen being addressed, in > `make/test/JtregNativeJdk.gmk` line 82, for `libTestMainKeyWindow`. Right, this is the desktop module as per the first line of the comment. java.base needs to be removed by the other PR as Erik said. I had not spotted it, but I don't see why the make/test /JtregNativeJdk.gmk case needs to link this framework. I don't see it being used by the test in question. But we can just remove it and prove it - but probably a separate PR since it is nothing to do with the desktop module and the autoconf code needs to be updated once everything else is in. - PR: https://git.openjdk.java.net/jdk/pull/2305
RFR: 8260616: Removing remaining JNF dependencies in the java.desktop module
This completes the desktop module JNF removal * remove -framework JavaNativeFoundation from make files * remove #import from all source files. If needed add import of JNIUtilities.h to get jni.h definitions - better anyway since then it gets the current JDK ones not the ones from the O/S * replace JNFNSToJavaString with NSStringToJavaString and JNFJavaToNSString with JavaStringToNSString * replace JNFNormalizedNSStringForPath with NormalizedPathNSStringFromJavaString and JNFNormalizedJavaStringForPath with NormalizedPathJavaStringFromNSString * replace JNFGet/ReleaseStringUTF16UniChars with direct calls to JNI * Map all JNFRunLoop perform* calls to the ThreadUtilities versions (the vast majority already did this) * Redo the ThreadUtilities calls to JNFRunLoop to directly invoke NSObject perform* methods. * define new javaRunLoopMode in ThreadUtilities to replace the JNF one and use where needed. * Remove the single usage of JNFPerformEnvBlock * replace JNFJavaToNSNumber in single A11Y file with local replacement * replace single usage of JNFNSTimeIntervalToJavaMillis in ScreenMenu.m with local replacement * remove un-needed JNFRunLoopDidStartNotification from NSApplicationAWT.m * misc. remaining cleanup (eg missed JNF_CHECK_AND_RETHROW_EXCEPTION) - Commit messages: - 8260616: Removing remaining JNF dependencies in the java.desktop module Changes: https://git.openjdk.java.net/jdk/pull/2305/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2305=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8260616 Stats: 431 lines in 71 files changed: 196 ins; 96 del; 139 mod Patch: https://git.openjdk.java.net/jdk/pull/2305.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2305/head:pull/2305 PR: https://git.openjdk.java.net/jdk/pull/2305
RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
This removes the JNF dependency from the jdk.hotspot.agent module. The macro expansions are the same as already used in the Java desktop module - which actually uses a macro still but there there are hundreds of uses. The function of this macro code is to prevent NSExceptions escaping to Java and also to drain the auto-release pool. What test group would be good for verifying this change ? - Commit messages: - 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m - 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m Changes: https://git.openjdk.java.net/jdk/pull/2304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2304=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257988 Stats: 42 lines in 2 files changed: 32 ins; 2 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2304/head:pull/2304 PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: JDK-8260518: Change default -mmacosx-version-min to 10.12
On Wed, 27 Jan 2021 19:23:48 GMT, Erik Joelsson wrote: > To guarantee backwards compatible binaries on Macos, we use the option > -mmacosx-version-min. This is currently set to 10.9, which is a really > ancient version. I propose we bump this to 10.12, which is still a rather > conservative old version (support ended in 2019). > > The driving issue for bumping this now is the aarch64 port, where building > for aarch64 requires the version min to be set to 11.0. Having a large gap > between the target versions becomes problematic as we hit a lot of > deprecation warnings in shared code. To be able to fix these deprecation > warnings, we need a smaller version gap. > > Just bumping us to 10.12 triggers warnings in libsplashscreen, so I will > temporarily add "deprecated-declarations" to the list of disabled warnings > there until they can be fixed in JDK-8260402. Marked as reviewed by prr (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2268
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 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 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 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
