Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v63]
> We should set the -permissive- flag for the Microsoft Visual C compiler, as > was requested by the now backed out > [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes > the Visual C compiler much less accepting of ill formed code, which will > improve code quality on Windows in the future. Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Circumvent pData in awt_Component.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/8ddb241d..da0c09b1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=62 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=61-62 Stats: 16 lines in 1 file changed: 0 ins; 11 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/15096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15096/head:pull/15096 PR: https://git.openjdk.org/jdk/pull/15096
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Tue, 26 Mar 2024 20:03:36 GMT, Magnus Ihse Bursie wrote: >> I see the advantage of collapsing self and parent into the same check, but >> it doesn't seem like getting rid of pData is of much benefit, the checks for >> null seem to remain the same either way > >> Sorry, I don't see a BOOL error anywhere? > > I think Phil misplaced the code block marker -- the BOOL error line and below > is supposed to be inside the second code block. > > What Phil is doing is trying to suggest two different approaches to make this > code nicer, the first with a bit more duplication, and the second with an > `error` boolean flag. And he says that he prefers the former, so the second > were propably just mentioned to show an alternative for discussion. > > I think what you should do here is to combine the `self` and `parent` null > checks as Phil suggests. And then replace all pData references to the final > variable, here as well as everywhere pData is involved. Ah, I see. Alright, will do so - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1540550556
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v61]
On Tue, 26 Mar 2024 08:55:56 GMT, Julian Waters wrote: >> We should set the -permissive- flag for the Microsoft Visual C compiler, as >> was requested by the now backed out >> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes >> the Visual C compiler much less accepting of ill formed code, which will >> improve code quality on Windows in the future. > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - Whitespace in awt_DnDDS.cpp > - Whitespace in awt_DnDDT.cpp Thanks, will assign the permissions. Though, I'll try to finish this myself so you don't have to- It seems like bad etiquette to make you fix something that I started. But all of this will still be meaningless if Phil doesn't return to this Pull Request any time soon :( - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2022058697
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v62]
> We should set the -permissive- flag for the Microsoft Visual C compiler, as > was requested by the now backed out > [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes > the Visual C compiler much less accepting of ill formed code, which will > improve code quality on Windows in the future. Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/ef9f3a62..8ddb241d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=61 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=60-61 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15096/head:pull/15096 PR: https://git.openjdk.org/jdk/pull/15096
Re: RFR: 8329131: Fold libjli_static back into libjli on AIX
On Tue, 26 Mar 2024 19:48:24 GMT, Magnus Ihse Bursie wrote: > @MBaesken @RealCLanger I will need your assistance for testing this on AIX. > > I believe all refactorings I have done will keep the static jli build > identical on AIX (apart from the name, of course). I have searched for jli > references in the code base to verify that there is indeed no use of the > dynamic jli on AIX. And finally I have temporarily changed the if to allow > building of a static library on my mac, and it seems to work fine. > > But in the end of the day it needs to be confirmed to work on AIX as well. > > For your information: My plan for going forward is to improve handling of > static vs dynamic libraries. This will allow us to remove the special checks > for AIX when setting up dependencies for libjli from other places. So it is > going to get even better in the future. :-) Thanks @magicus for doing this. We'll run it through our testing and let you know the results tomorrow. - PR Comment: https://git.openjdk.org/jdk/pull/18497#issuecomment-2022045199
Re: RFR: JDK-8324774: Add DejaVu web fonts [v2]
On Wed, 20 Mar 2024 15:54:12 GMT, Hannes Wallnöfer wrote: >> This change adds the DejaVu web fonts that were previously maintained >> externally to the open repository so they are available both in JDK API >> documentation and any API documentation generated with the `javadoc` tool. >> All files added in this PR are the same as the ones previously maintained >> externally, with the exception of added license and name/version comments in >> `dejavu.css`. >> >> Copying of font files to the generated documentation is done by looking for >> font file names in `dejavu.css`, so font file names can be changed without >> changing the code. However, the font file list is hard-coded in >> `APITest.java`. `CheckLibraryVersions.java` is updated to make sure the name >> and version in the legal file matches the one in the stylesheet. Of course I >> also performed manual tests to make sure the font and legal files are copied >> to the output tree and used correctly in browsers. >> >> Once #17411 is integrated, `dejavu.css` should also be added to the list of >> files checked by the new "pass-through" test. > > Hannes Wallnöfer has updated the pull request incrementally with two > additional commits since the last revision: > > - JDK-8327385: Add JavaDoc option to exclude web fonts from generated > documentation > - Merge try-with-resource statements Marked as reviewed by jjg (Reviewer). I like the new stuff for `JavadocTester` wrapped up in this work. - PR Review: https://git.openjdk.org/jdk/pull/17633#pullrequestreview-1962024918 PR Comment: https://git.openjdk.org/jdk/pull/17633#issuecomment-2021719299
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 23:10:44 GMT, Magnus Ihse Bursie wrote: >> This took me a while to decode. The old code will unconditionally override >> `LCMS_CFLAGS` which means whatever value it gets in configure is >> overwritten. That certainly seems like a bug. >> >> Your current patch clears the variable when building with an external >> liblcms. I agree this is equivalent behavior for the external liblcms case. >> If we can assume that `LCMS_CFLAGS` from configure is always empty when >> building liblcms from source, then it's also equivalent when building from >> source. I assume that `LCMS_CFLAGS` is only ever populated with `-I` paths >> pointing to the header files of an external liblcms. >> >> Am I understanding correctly that the fix you are proposing is to stop >> clearing `LCMS_CFLAGS` and just trust that configure sets it with the >> correct values if needed? I suppose doing it in a followup is probably >> better. > > The code, prior to this PR, includes these lines: > > # The fast floor code loses precision. > LCMS_CFLAGS=-DCMS_DONT_USE_FAST_FLOOR -DCMS_NO_HALF_SUPPORT > > > This will overwrite the value of `LCMS_CFLAGS` as given in `spec.gmk` by > configure. It is normally empty, but if you run `configure > --with-lcms=system`, then the configure script will query `pkg-config` about > the value needed for `CFLAGS` for LCMS. On my test system it was still empty, > but there is no guarantee that it will be so. (And if we truly believed that > would be the case, we wouldn't export LCMS_CFLAGS in spec.gmk...) > > Hence, it is a bug to replace this value. I am pretty certain that the > intention here was to add `-DCMS_DONT_USE_FAST_FLOOR -DCMS_NO_HALF_SUPPORT` > to the compilation, in addition to any value LCMS_CFLAGS would have from > configure. > If we can assume that LCMS_CFLAGS from configure is always empty when > building liblcms from source It is. It is only set in configure if we set USE_EXTERNAL_LCMS to true. > I assume that LCMS_CFLAGS is only ever populated with -I paths pointing to > the header files of an external liblcms. Presumably; we don't even set it ourselves, but rely on pkg-config. In theory it could also include e.g. -D values. In practice, it was empty in my test machine. > Am I understanding correctly that the fix you are proposing is to stop > clearing LCMS_CFLAGS and just trust that configure sets it with the correct > values if needed? I suppose doing it in a followup is probably better. Yes, that is correctly understood. I'll do it in a separate followup then. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540237249
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 22:01:33 GMT, Erik Joelsson wrote: >> This is a follow-up on >> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same >> kind of cleanup to java.desktop. Some code needed more special treatment >> here, so there is some additional effects outside of the >> modules/java.desktop directory. The code was also in worse shape than other >> modules, so some additional changes to the build logic where needed. > > make/autoconf/lib-bundled.m4 line 107: > >> 105: >> 106: USE_EXTERNAL_LIBGIF=true >> 107: GIFLIB_LIBS=-lgif > > GIFLIB vs LIBGIF? Just above we have LIBJPEG. Should we stick to one naming > convention, or did you want to avoid changing any variable names? I don't want to change any names right now, but even if I wanted to, I think the name is correct. We have used the traditional name of the project. In this case it's definitely "The GIFLIB project" (see https://giflib.sourceforge.net/). This is also the name that pkg-config uses. Many libraries use the "lib + name" convention on unix, but not all. FreeType is just called "freetype", not "libfreetype", while FFI is "libffi". It's not really consistent, but I think we do best in aligning with the traditional name used by the project (which also is what pkg-config uses). > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 127: > >> 125: CFLAGS := $(X_CFLAGS) $(LIBAWT_CFLAGS), \ >> 126: CFLAGS_gcc := -fgcse-after-reload, \ >> 127: CXXFLAGS := $(LIBAWT_CFLAGS) $(X_CFLAGS), \ > > Is the reverse ordering of flags for CXXFLAGS an accident? Yep, will fix. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540234519 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540234719
Re: RFR: JDK-8329074: AIX build fails after JDK-8328824
On Tue, 26 Mar 2024 09:21:20 GMT, Matthias Baesken wrote:
> After [JDK-8328824](https://bugs.openjdk.org/browse/JDK-8328824), we run in
> the AIX build into this failure :
>
> /opt/freeware/bin/bash: -c: line 1: syntax error near unexpected token `('
> gmake[3]: *** [lib/CoreLibraries.gmk:194:
> /openjdk/nb/aix_ppc64/jdk-dev-opt/support/native/java.base/libjli_static/args.o]
> Error 2
> gmake[3]: *** Waiting for unfinished jobs
>
> Looks like an addprefix usage is wrong, a '$' is missing.
I apologize for missing this typo. :-( Thanks for fixing it for me, and so
quickly!
-
PR Comment: https://git.openjdk.org/jdk/pull/18484#issuecomment-2021631327
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 19:36:04 GMT, Phil Race wrote: >> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 280: >> >>> 278: # as includes, instead the system headers should be used. >>> 279: LIBLCMS_HEADERS_FROM_SRC := false >>> 280: # FIXME: Keep old behavior and reset LCMS_CFLAGS. This is likely a >>> bug. >> >> A comment here: This code is equivalent with the old code, but it seems >> pretty obvious that this is a bug. I'm somewhat reluctant to changing the >> actual behavior in a refactor PR like this, but otoh this is a very small >> fix that would only affect someone running with an external lcms with >> non-empty CFLAGS. So if anyone thinks I should fix this right now in this >> PR, I can do so. Otherwise I'll file a follow-up bug and fix this in that >> one instead. (If nothing else, I think backporters will thank me for going >> that route instead.) > > I don't understand this. What bug ? > The prior value LCMS_CFLAGS doesn't matter if you are not building from src. The code, prior to this PR, includes these lines: # The fast floor code loses precision. LCMS_CFLAGS=-DCMS_DONT_USE_FAST_FLOOR -DCMS_NO_HALF_SUPPORT This will overwrite the value of `LCMS_CFLAGS` as given in `spec.gmk` by configure. It is normally empty, but if you run `configure --with-lcms=system`, then the configure script will query `pkg-config` about the value needed for `CFLAGS` for LCMS. On my test system it was still empty, but there is no guarantee that it will be so. (And if we truly believed that would be the case, we wouldn't export LCMS_CFLAGS in spec.gmk...) Hence, it is a bug to replace this value. I am pretty certain that the intention here was to add `-DCMS_DONT_USE_FAST_FLOOR -DCMS_NO_HALF_SUPPORT` to the compilation, in addition to any value LCMS_CFLAGS would have from configure. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540221335
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 19:19:14 GMT, Phil Race wrote: >> This is a follow-up on >> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same >> kind of cleanup to java.desktop. Some code needed more special treatment >> here, so there is some additional effects outside of the >> modules/java.desktop directory. The code was also in worse shape than other >> modules, so some additional changes to the build logic where needed. > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 107: > >> 105: >> 106: LIBAWT_CFLAGS += -D__MEDIALIB_OLD_NAMES -D__USE_J2D_NAMES $(X_CFLAGS) >> 107: > > Why is X_CFLAGS no longer needed ? It's just moved down. > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 569: > >> 567: LIBJAWT_EXTRA_HEADER_DIRS := \ >> 568: include \ >> 569: # > > A syntax question - what does the trailing # do / mean here ? > Superficially I'd expect this to expand to "include #" > which means there'd be a # embedded when you append the other folders below .. The `#` marks a comment, so will basically be ignored. We use this construct to define easy to read lists where every element can have a trailing ``, including the last. This makes it easier to add or remove items in the list without causing diffs in unrelated lines. In this case the list has only one element, so I would suggest just using a single line assignment. OTOH, it seems Magnus chose to use this construct for uniformity with all other HEADER and SRC lists. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540172045 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540221259
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 12:51:38 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on > [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same > kind of cleanup to java.desktop. Some code needed more special treatment > here, so there is some additional effects outside of the modules/java.desktop > directory. The code was also in worse shape than other modules, so some > additional changes to the build logic where needed. make/autoconf/lib-bundled.m4 line 107: > 105: > 106: USE_EXTERNAL_LIBGIF=true > 107: GIFLIB_LIBS=-lgif GIFLIB vs LIBGIF? Just above we have LIBJPEG. Should we stick to one naming convention, or did you want to avoid changing any variable names? make/modules/java.desktop/lib/Awt2dLibraries.gmk line 127: > 125: CFLAGS := $(X_CFLAGS) $(LIBAWT_CFLAGS), \ > 126: CFLAGS_gcc := -fgcse-after-reload, \ > 127: CXXFLAGS := $(LIBAWT_CFLAGS) $(X_CFLAGS), \ Is the reverse ordering of flags for CXXFLAGS an accident? make/modules/java.desktop/lib/Awt2dLibraries.gmk line 588: > 586: > 587: ifeq ($(call isTargetOs, macosx), true) > 588: # libjawt on macosx do not use the unix code Suggestion: # libjawt on macosx does not use the unix code - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540161296 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540172539 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540221866
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 19:36:04 GMT, Phil Race wrote: >> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 280: >> >>> 278: # as includes, instead the system headers should be used. >>> 279: LIBLCMS_HEADERS_FROM_SRC := false >>> 280: # FIXME: Keep old behavior and reset LCMS_CFLAGS. This is likely a >>> bug. >> >> A comment here: This code is equivalent with the old code, but it seems >> pretty obvious that this is a bug. I'm somewhat reluctant to changing the >> actual behavior in a refactor PR like this, but otoh this is a very small >> fix that would only affect someone running with an external lcms with >> non-empty CFLAGS. So if anyone thinks I should fix this right now in this >> PR, I can do so. Otherwise I'll file a follow-up bug and fix this in that >> one instead. (If nothing else, I think backporters will thank me for going >> that route instead.) > > I don't understand this. What bug ? > The prior value LCMS_CFLAGS doesn't matter if you are not building from src. This took me a while to decode. The old code will unconditionally override `LCMS_CFLAGS` which means whatever value it gets in configure is overwritten. That certainly seems like a bug. Your current patch clears the variable when building with an external liblcms. I agree this is equivalent behavior for the external liblcms case. If we can assume that `LCMS_CFLAGS` from configure is always empty when building liblcms from source, then it's also equivalent when building from source. I assume that `LCMS_CFLAGS` is only ever populated with `-I` paths pointing to the header files of an external liblcms. Am I understanding correctly that the fix you are proposing is to stop clearing `LCMS_CFLAGS` and just trust that configure sets it with the correct values if needed? I suppose doing it in a followup is probably better. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540197524
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 19:46:00 GMT, Phil Race wrote: >> This is a follow-up on >> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same >> kind of cleanup to java.desktop. Some code needed more special treatment >> here, so there is some additional effects outside of the >> modules/java.desktop directory. The code was also in worse shape than other >> modules, so some additional changes to the build logic where needed. > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 569: > >> 567: LIBJAWT_EXTRA_HEADER_DIRS := \ >> 568: include \ >> 569: # > > A syntax question - what does the trailing # do / mean here ? > Superficially I'd expect this to expand to "include #" > which means there'd be a # embedded when you append the other folders below .. `#` is the line comment marker in makefiles. What you see here is a common pattern in our makefiles, typically used in multi-line lists. The trailing # is a trick to avoid having a special treatment on the last line (without a ``), which is all too easy to forget, and very hard to debug. This particular list is very short, so `LIBJAWT_EXTRA_HEADER_DIRS := include` would have yielded the same result, and I would normally have chosen that syntax, but in this case the list will be conditionally appended to just below, so in a conceptual sense the list is much longer, but the limited syntax in makefiles does not allow for us to express this in a better way. > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 848: > >> 846: -framework Metal \ >> 847: -framework OpenGL \ >> 848: -framework QuartzCore \ > > The re-ordering here happens to be small, and it looks like some one already > might have been doing somethings lexically, but this again points to the need > for running tests as well as building. Here, as in many places, there have been attempts at keeping the lists sorted, but unfortunately this has not been kept 100%. :-( The extraction of -ljava is essential for the upcoming JDK_LIB rewrite. Once again, if you have any suggestions to what tests you think I should run, let me know! - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540223963 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540225111
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 23:01:55 GMT, Magnus Ihse Bursie wrote: >> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 145: >> >>> 143: -delayload:gdi32.dll -delayload:imm32.dll -delayload:ole32.dll >>> \ >>> 144: -delayload:shell32.dll -delayload:shlwapi.dll >>> -delayload:user32.dll \ >>> 145: -delayload:winmm.dll -delayload:winspool.drv, \ >> >> I suppose (presume?) that delayload isn't sensitive to the order ? >> But I do have to ask if you ran all the client automated tests, as well as >> making sure builds work ? >> And surely logical ordering related to dependencies is more important than >> lexical sort order ? > > `/delayload` is not order sensitive, no. > > I'm not sure what you mean by "logical ordering related to dependencies". All > these are dependencies for awt.dll, and there is no intra-dependency relation > between them. > > What you see here is just a single file -- I have gone through the entire > build system to make sure all calls to `SetupJdkLibrary` has been consistent. > Among the things I have standardized is making sure that all libraries are > listed in alphabetical order. It will not affect the final product but will > help us keep the makefile source code clean and consistent. I have run tier 1 + tier 3 on all platforms supported on the Oracle CI system, in addition to the comparison build that shows that the resulting binaries should have only superficial changes. Please advice if you think I need to run any additional testing. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540213312
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 19:25:47 GMT, Phil Race wrote: >> This is a follow-up on >> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same >> kind of cleanup to java.desktop. Some code needed more special treatment >> here, so there is some additional effects outside of the >> modules/java.desktop directory. The code was also in worse shape than other >> modules, so some additional changes to the build logic where needed. > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 145: > >> 143: -delayload:gdi32.dll -delayload:imm32.dll -delayload:ole32.dll \ >> 144: -delayload:shell32.dll -delayload:shlwapi.dll >> -delayload:user32.dll \ >> 145: -delayload:winmm.dll -delayload:winspool.drv, \ > > I suppose (presume?) that delayload isn't sensitive to the order ? > But I do have to ask if you ran all the client automated tests, as well as > making sure builds work ? > And surely logical ordering related to dependencies is more important than > lexical sort order ? `/delayload` is not order sensitive, no. I'm not sure what you mean by "logical ordering related to dependencies". All these are dependencies for awt.dll, and there is no intra-dependency relation between them. What you see here is just a single file -- I have gone through the entire build system to make sure all calls to `SetupJdkLibrary` has been consistent. Among the things I have standardized is making sure that all libraries are listed in alphabetical order. It will not affect the final product but will help us keep the makefile source code clean and consistent. > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 161: > >> 159: LIBS_windows := advapi32.lib comctl32.lib comdlg32.lib delayimp.lib >> \ >> 160: gdi32.lib imm32.lib kernel32.lib ole32.lib shell32.lib >> shlwapi.lib \ >> 161: user32.lib uuid.lib winmm.lib winspool.lib, \ > > same as above, I don't think sort oder is what is important here. It is not important for awt.dll. It is important in the context of all other calls to SetupJdkLibrary. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540212163 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540213657
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 19:19:14 GMT, Phil Race wrote: >> This is a follow-up on >> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same >> kind of cleanup to java.desktop. Some code needed more special treatment >> here, so there is some additional effects outside of the >> modules/java.desktop directory. The code was also in worse shape than other >> modules, so some additional changes to the build logic where needed. > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 107: > >> 105: >> 106: LIBAWT_CFLAGS += -D__MEDIALIB_OLD_NAMES -D__USE_J2D_NAMES $(X_CFLAGS) >> 107: > > Why is X_CFLAGS no longer needed ? It is still needed, and it is still there. It has just moved to inside the `SetupJdkLibrary` call. The idea is that we should not really use additional variables if it is possible to give the proper values to the parameters directly in SetupJdkLibrary. However, I see now that I have only partially succeeded with LIBAWT_CFLAGS. And also that I have handled CFLAGS and CXXFLAGS inconsistently. *sigh* I think I've been over this code a thousand times, and still I missed this. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540208549
Re: RFR: 8329102: Clean up jdk.jpackage native compilation
On Tue, 26 Mar 2024 21:44:12 GMT, Erik Joelsson wrote: >> This is a follow-up on JDK-8328680, making the same kind of cleanup to >> jdk.jpackage. The code here needed more work than for the other modules, so >> I wanted have it in a separate PR to get a more thorough review. > > make/common/JdkNativeCompilation.gmk line 294: > >> 292: $$($1_EXTRA_RCFLAGS) >> 293: >> 294: ifneq ($$($1_HEADERS_FROM_SRC), false) > > I note that this parameter in SetupJdkLibrary is documented in the comment > above the definition, but you haven't added any of the new parameters to > SetupJdkExecutable to the comment for that macro. Should we, or is the > intention to rework this file anyway? The intention is to unify SetupJdkLibrary and SetupJdkExecutable, and clean up this file at the same time. I have tried to keep it up to date meanwhile, but I have missed it here. So for me it's either or; I can fix it now, or later. - PR Review Comment: https://git.openjdk.org/jdk/pull/18491#discussion_r1540205036
Re: RFR: 8329131: Fold libjli_static back into libjli on AIX
On Tue, 26 Mar 2024 19:30:01 GMT, Magnus Ihse Bursie wrote: > On AIX, we need a static libjli, since the linker cannot find other libraries > (like libjvm.so and libjava.so) using a relative path, as on other platforms. > > However, for reasons unclear, we still build a dynamic libjli.so on AIX, even > though this is never used. Instead, we also build a static libjli_static.a > library (which is then forced to have a different name as to not collide with > the dynamic library). > > This should be fixed. We should build exactly one libjli on all platforms, be > it static or dynamic. Looks good, but the AIX people will need to validate of course. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18497#pullrequestreview-1961809651
Re: RFR: 8329102: Clean up jdk.jpackage native compilation
On Tue, 26 Mar 2024 16:18:43 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on JDK-8328680, making the same kind of cleanup to > jdk.jpackage. The code here needed more work than for the other modules, so I > wanted have it in a separate PR to get a more thorough review. Marked as reviewed by erikj (Reviewer). make/common/JdkNativeCompilation.gmk line 294: > 292: $$($1_EXTRA_RCFLAGS) > 293: > 294: ifneq ($$($1_HEADERS_FROM_SRC), false) I note that this parameter in SetupJdkLibrary is documented in the comment above the definition, but you haven't added any of the new parameters to SetupJdkExecutable to the comment for that macro. Should we, or is the intention to rework this file anyway? - PR Review: https://git.openjdk.org/jdk/pull/18491#pullrequestreview-1961795710 PR Review Comment: https://git.openjdk.org/jdk/pull/18491#discussion_r1540146998
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Tue, 26 Mar 2024 08:44:31 GMT, Julian Waters wrote: > Maybe as a further improvement, I can inline > THROW_NULL_PDATA_IF_NOT_DESTROYED at its callsites and replace the bad > NullPointerException error message with the proper null pointer name. Since > Phil isn't here, what do you think? That might be a good future code cleanup, but I suggest you refrain from doing that in this change. > Regardless, I really hope I can get this in by Thursday. I see that you are struggling a bit to get this done. I can probably help you fix the remaining issues, but I can't push to your PR. So either I'd need to create a fork of your `patch-10` branch in my personal fork and you'll have to pull from there into your branch, or you need to give me write permissions to your personal fork. I believe you can assign permissions on a per-branch basis. I'd really like to see this PR to come to conclusion. - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2021391154
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Wed, 20 Mar 2024 06:44:02 GMT, Julian Waters wrote: >> Sorry, I don't see a BOOL error anywhere? > > I see the advantage of collapsing self and parent into the same check, but it > doesn't seem like getting rid of pData is of much benefit, the checks for > null seem to remain the same either way > Sorry, I don't see a BOOL error anywhere? I think Phil misplaced the code block marker -- the BOOL error line and below is supposed to be inside the second code block. What Phil is doing is trying to suggest two different approaches to make this code nicer, the first with a bit more duplication, and the second with an `error` boolean flag. And he says that he prefers the former, so the second were propably just mentioned to show an alternative for discussion. I think what you should do here is to combine the `self` and `parent` null checks as Phil suggests. And then replace all pData references to the final variable, here as well as everywhere pData is involved. - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1540025427
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 13:00:41 GMT, Magnus Ihse Bursie wrote: >> This is a follow-up on >> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same >> kind of cleanup to java.desktop. Some code needed more special treatment >> here, so there is some additional effects outside of the >> modules/java.desktop directory. The code was also in worse shape than other >> modules, so some additional changes to the build logic where needed. > > make/modules/java.desktop/lib/Awt2dLibraries.gmk line 280: > >> 278: # as includes, instead the system headers should be used. >> 279: LIBLCMS_HEADERS_FROM_SRC := false >> 280: # FIXME: Keep old behavior and reset LCMS_CFLAGS. This is likely a >> bug. > > A comment here: This code is equivalent with the old code, but it seems > pretty obvious that this is a bug. I'm somewhat reluctant to changing the > actual behavior in a refactor PR like this, but otoh this is a very small fix > that would only affect someone running with an external lcms with non-empty > CFLAGS. So if anyone thinks I should fix this right now in this PR, I can do > so. Otherwise I'll file a follow-up bug and fix this in that one instead. (If > nothing else, I think backporters will thank me for going that route instead.) I don't understand this. What bug ? The prior value LCMS_CFLAGS doesn't matter if you are not building from src. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1539986542
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 12:51:38 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on > [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same > kind of cleanup to java.desktop. Some code needed more special treatment > here, so there is some additional effects outside of the modules/java.desktop > directory. The code was also in worse shape than other modules, so some > additional changes to the build logic where needed. make/modules/java.desktop/lib/Awt2dLibraries.gmk line 107: > 105: > 106: LIBAWT_CFLAGS += -D__MEDIALIB_OLD_NAMES -D__USE_J2D_NAMES $(X_CFLAGS) > 107: Why is X_CFLAGS no longer needed ? make/modules/java.desktop/lib/Awt2dLibraries.gmk line 145: > 143: -delayload:gdi32.dll -delayload:imm32.dll -delayload:ole32.dll \ > 144: -delayload:shell32.dll -delayload:shlwapi.dll > -delayload:user32.dll \ > 145: -delayload:winmm.dll -delayload:winspool.drv, \ I suppose (presume?) that delayload isn't sensitive to the order ? But I do have to ask if you ran all the client automated tests, as well as making sure builds work ? And surely logical ordering related to dependencies is more important than lexical sort order ? make/modules/java.desktop/lib/Awt2dLibraries.gmk line 161: > 159: LIBS_windows := advapi32.lib comctl32.lib comdlg32.lib delayimp.lib \ > 160: gdi32.lib imm32.lib kernel32.lib ole32.lib shell32.lib > shlwapi.lib \ > 161: user32.lib uuid.lib winmm.lib winspool.lib, \ same as above, I don't think sort oder is what is important here. make/modules/java.desktop/lib/Awt2dLibraries.gmk line 569: > 567: LIBJAWT_EXTRA_HEADER_DIRS := \ > 568: include \ > 569: # A syntax question - what does the trailing # do / mean here ? Superficially I'd expect this to expand to "include #" which means there'd be a # embedded when you append the other folders below .. make/modules/java.desktop/lib/Awt2dLibraries.gmk line 848: > 846: -framework Metal \ > 847: -framework OpenGL \ > 848: -framework QuartzCore \ The re-ordering here happens to be small, and it looks like some one already might have been doing somethings lexically, but this again points to the need for running tests as well as building. - PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1539965731 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1539974347 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1539977625 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r154977 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540011695
Re: RFR: 8329131: Fold libjli_static back into libjli on AIX
On Tue, 26 Mar 2024 19:30:01 GMT, Magnus Ihse Bursie wrote: > On AIX, we need a static libjli, since the linker cannot find other libraries > (like libjvm.so and libjava.so) using a relative path, as on other platforms. > > However, for reasons unclear, we still build a dynamic libjli.so on AIX, even > though this is never used. Instead, we also build a static libjli_static.a > library (which is then forced to have a different name as to not collide with > the dynamic library). > > This should be fixed. We should build exactly one libjli on all platforms, be > it static or dynamic. @MBaesken @RealCLanger I will need your assistance for testing this on AIX. I believe all refactorings I have done will keep the static jli build identical on AIX (apart from the name, of course). I have searched for jli references in the code base to verify that there is indeed no use of the dynamic jli on AIX. And finally I have temporarily changed the if to allow building of a static library on my mac, and it seems to work fine. But in the end of the day it needs to be confirmed to work on AIX as well. For your information: My plan for going forward is to improve handling of static vs dynamic libraries. This will allow us to remove the special checks for AIX when setting up dependencies for libjli from other places. So it is going to get even better in the future. :-) - PR Comment: https://git.openjdk.org/jdk/pull/18497#issuecomment-2021334572
RFR: 8329131: Fold libjli_static back into libjli on AIX
On AIX, we need a static libjli, since the linker cannot find other libraries (like libjvm.so and libjava.so) using a relative path, as on other platforms. However, for reasons unclear, we still build a dynamic libjli.so on AIX, even though this is never used. Instead, we also build a static libjli_static.a library (which is then forced to have a different name as to not collide with the dynamic library). This should be fixed. We should build exactly one libjli on all platforms, be it static or dynamic. - Commit messages: - 8329131: Fold libjli_static back into libjli on AIX Changes: https://git.openjdk.org/jdk/pull/18497/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18497&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329131 Stats: 49 lines in 7 files changed: 10 ins; 30 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18497.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18497/head:pull/18497 PR: https://git.openjdk.org/jdk/pull/18497
Re: RFR: 8329102: Clean up jdk.jpackage native compilation
On Tue, 26 Mar 2024 16:18:43 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on JDK-8328680, making the same kind of cleanup to > jdk.jpackage. The code here needed more work than for the other modules, so I > wanted have it in a separate PR to get a more thorough review. As with the other cleanups, I have run COMPARE_BUILD on Oracle CI. All platforms except Windows pass with identical bits. (That's a new one! :-)) On Windows, there are the usual changes (in jpackage.dll) due to the reordering of libraries. - PR Comment: https://git.openjdk.org/jdk/pull/18491#issuecomment-2021165032
RFR: 8329102: Clean up jdk.jpackage native compilation
This is a follow-up on JDK-8328680, making the same kind of cleanup to jdk.jpackage. The code here needed more work than for the other modules, so I wanted have it in a separate PR to get a more thorough review. - Commit messages: - 8329102: Clean up jdk.jpackage native compilation Changes: https://git.openjdk.org/jdk/pull/18491/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18491&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329102 Stats: 119 lines in 2 files changed: 30 ins; 39 del; 50 mod Patch: https://git.openjdk.org/jdk/pull/18491.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18491/head:pull/18491 PR: https://git.openjdk.org/jdk/pull/18491
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 15 Mar 2024 13:58:05 GMT, Hamlin Li wrote: >> Hi, >> Can you help to review this patch? >> Thanks >> >> This is a continuation of work based on [1] by @XiaohongGong, most work was >> done in that pr. In this new pr, just rebased the code in [1], then added >> some minor changes (renaming of calling method, add libsleef as extra lib in >> CI cross-build on aarch64 in github workflow); I aslo tested the combination >> of following scenarios: >> * at build time >> * with/without sleef >> * with/without sve support >> * at runtime >> * with/without sleef >> * with/without sve support >> >> [1] https://github.com/openjdk/jdk/pull/16234 >> >> ## Regression Test >> * test/jdk/jdk/incubator/vector/ >> * test/hotspot/jtreg/compiler/vectorapi/ >> >> ## Performance Test >> Previously, @XiaohongGong has shared the data: >> https://github.com/openjdk/jdk/pull/16234#issuecomment-1767727028 > > Hamlin Li has updated the pull request incrementally with one additional > commit since the last revision: > > fix jni includes OK, no worry. Let's discuss more thoroughly before we start implement it. Let me ask first question: are we still want sleef into jdk? * If the answer is `no` or similar, what's the better alternative solution? * If the answer is `yes`, let's continue the discussion about how to integrate sleef into jdk via this pr (maybe not imeplement it in this pr). I try to categorize the possible solutions below. 1. depends on external sleef at build time & run time, like this pr. Cons: requires sleef installed in system at build & run time. 2. depends on external sleef at run time only, like @theRealAph suggested. Cons: requires sleef installed in system at runtime 3. depends on external sleef at build time only, like @magicus suggested. Cons: requires sleef installed in system at build time 4. not depends on external sleef at all. 4.1. integrate source into jdk, use the genrated source from sleef cmake, check https://github.com/openjdk/jdk/pull/18294#issuecomment-2018839778, Cons: license legal process 4.2. is there other ways? 6. any other possible solutions not covered above? Seems to me, the best way is to integrate sleef souce into jdk. Does it need license work? If positive, how and who will work on it? Would you mind to share your comments, and make necessary additions? @theRealAph @magicus Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2020986186
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 15 Mar 2024 13:58:05 GMT, Hamlin Li wrote: >> Hi, >> Can you help to review this patch? >> Thanks >> >> This is a continuation of work based on [1] by @XiaohongGong, most work was >> done in that pr. In this new pr, just rebased the code in [1], then added >> some minor changes (renaming of calling method, add libsleef as extra lib in >> CI cross-build on aarch64 in github workflow); I aslo tested the combination >> of following scenarios: >> * at build time >> * with/without sleef >> * with/without sve support >> * at runtime >> * with/without sleef >> * with/without sve support >> >> [1] https://github.com/openjdk/jdk/pull/16234 >> >> ## Regression Test >> * test/jdk/jdk/incubator/vector/ >> * test/hotspot/jtreg/compiler/vectorapi/ >> >> ## Performance Test >> Previously, @XiaohongGong has shared the data: >> https://github.com/openjdk/jdk/pull/16234#issuecomment-1767727028 > > Hamlin Li has updated the pull request incrementally with one additional > commit since the last revision: > > fix jni includes I'm not making a decision; I'm making a suggestion. Also, I believe a lot more information have come to light in the discussion we have been having. For me at least, the full scope, impact and intention of this PR was not at all clear initially. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2020932361
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Fri, 15 Mar 2024 13:58:05 GMT, Hamlin Li wrote: >> Hi, >> Can you help to review this patch? >> Thanks >> >> This is a continuation of work based on [1] by @XiaohongGong, most work was >> done in that pr. In this new pr, just rebased the code in [1], then added >> some minor changes (renaming of calling method, add libsleef as extra lib in >> CI cross-build on aarch64 in github workflow); I aslo tested the combination >> of following scenarios: >> * at build time >> * with/without sleef >> * with/without sve support >> * at runtime >> * with/without sleef >> * with/without sve support >> >> [1] https://github.com/openjdk/jdk/pull/16234 >> >> ## Regression Test >> * test/jdk/jdk/incubator/vector/ >> * test/hotspot/jtreg/compiler/vectorapi/ >> >> ## Performance Test >> Previously, @XiaohongGong has shared the data: >> https://github.com/openjdk/jdk/pull/16234#issuecomment-1767727028 > > Hamlin Li has updated the pull request incrementally with one additional > commit since the last revision: > > fix jni includes OK, I'm fine with it. I just hope we could have made this decision earlier, as I have asked this question before: https://github.com/openjdk/jdk/pull/16234#issuecomment-1973345855, and at that time seems there is no "No" answer. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2020859623
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 12:51:38 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on > [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same > kind of cleanup to java.desktop. Some code needed more special treatment > here, so there is some additional effects outside of the modules/java.desktop > directory. The code was also in worse shape than other modules, so some > additional changes to the build logic where needed. As usual, I have run COMPARE_BUILD on Oracle CI to be able to study the differences. As before, I believe all differences are due to the reordering of libraries to link with. For Windows, the changes were larger in some libraries than usual, so I conducted a more thorough investigation. I have verified that the command lines to compile and link are essentially unchanged (except for ordering of command line flags). I believe the use of `-delayload` in libawt and libsplashscreen was the cause for the larger-than-usual changes in the resulting dll files. (I also incidentally discovered [JDK-8329107](https://bugs.openjdk.org/browse/JDK-8329107) when I was scrutinizing the command lines...) - PR Comment: https://git.openjdk.org/jdk/pull/18486#issuecomment-2020828348
RFR: 8329086: Clean up java.desktop native compilation
This is a follow-up on [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same kind of cleanup to java.desktop. Some code needed more special treatment here, so there is some additional effects outside of the modules/java.desktop directory. The code was also in worse shape than other modules, so some additional changes to the build logic where needed. - Commit messages: - Unify windows and unix build of jawt - Move bundled/system libfreetype logic to libfontmanager - * Setup LIBJPEG_LIBS and GIFLIB_LIBS in configure. - Trivial cleanups in java.desktop Changes: https://git.openjdk.org/jdk/pull/18486/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18486&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8329086 Stats: 605 lines in 4 files changed: 205 ins; 218 del; 182 mod Patch: https://git.openjdk.org/jdk/pull/18486.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18486/head:pull/18486 PR: https://git.openjdk.org/jdk/pull/18486
Re: RFR: 8329086: Clean up java.desktop native compilation
On Tue, 26 Mar 2024 12:51:38 GMT, Magnus Ihse Bursie wrote: > This is a follow-up on > [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same > kind of cleanup to java.desktop. Some code needed more special treatment > here, so there is some additional effects outside of the modules/java.desktop > directory. The code was also in worse shape than other modules, so some > additional changes to the build logic where needed. If the total PR is hard to read, I recommend reviewing this by looking at each individual commit instead. I've tried to make them simple and explain what I'm doing. make/modules/java.desktop/lib/Awt2dLibraries.gmk line 280: > 278: # as includes, instead the system headers should be used. > 279: LIBLCMS_HEADERS_FROM_SRC := false > 280: # FIXME: Keep old behavior and reset LCMS_CFLAGS. This is likely a bug. A comment here: This code is equivalent with the old code, but it seems pretty obvious that this is a bug. I'm somewhat reluctant to changing the actual behavior in a refactor PR like this, but otoh this is a very small fix that would only affect someone running with an external lcms with non-empty CFLAGS. So if anyone thinks I should fix this right now in this PR, I can do so. Otherwise I'll file a follow-up bug and fix this in that one instead. (If nothing else, I think backporters will thank me for going that route instead.) - PR Comment: https://git.openjdk.org/jdk/pull/18486#issuecomment-2020358981 PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1539179250
Re: RFR: JDK-8329074: AIX build fails after JDK-8328824
On Tue, 26 Mar 2024 09:21:20 GMT, Matthias Baesken wrote:
> After [JDK-8328824](https://bugs.openjdk.org/browse/JDK-8328824), we run in
> the AIX build into this failure :
>
> /opt/freeware/bin/bash: -c: line 1: syntax error near unexpected token `('
> gmake[3]: *** [lib/CoreLibraries.gmk:194:
> /openjdk/nb/aix_ppc64/jdk-dev-opt/support/native/java.base/libjli_static/args.o]
> Error 2
> gmake[3]: *** Waiting for unfinished jobs
>
> Looks like an addprefix usage is wrong, a '$' is missing.
windows aarch64 failure is unrelated, fails with some download
[build.sh][INFO] Downloading
https://archive.apache.org/dist/ant/binaries/apache-ant-1.10.8-bin.zip to
/d/a/jdk/jdk/jtreg/src/make/../build/deps/apache-ant-1.10.8-bin.zip
Error: sh][ERROR] wget exited with exit code 4
Thanks for the reviews !
-
PR Comment: https://git.openjdk.org/jdk/pull/18484#issuecomment-2019994055
PR Comment: https://git.openjdk.org/jdk/pull/18484#issuecomment-2019997165
Re: RFR: JDK-8329074: AIX build fails after JDK-8328824
On Tue, 26 Mar 2024 09:21:20 GMT, Matthias Baesken wrote:
> After [JDK-8328824](https://bugs.openjdk.org/browse/JDK-8328824), we run in
> the AIX build into this failure :
>
> /opt/freeware/bin/bash: -c: line 1: syntax error near unexpected token `('
> gmake[3]: *** [lib/CoreLibraries.gmk:194:
> /openjdk/nb/aix_ppc64/jdk-dev-opt/support/native/java.base/libjli_static/args.o]
> Error 2
> gmake[3]: *** Waiting for unfinished jobs
>
> Looks like an addprefix usage is wrong, a '$' is missing.
LGTM
-
Marked as reviewed by goetz (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18484#pullrequestreview-1959873067
Integrated: JDK-8329074: AIX build fails after JDK-8328824
On Tue, 26 Mar 2024 09:21:20 GMT, Matthias Baesken wrote:
> After [JDK-8328824](https://bugs.openjdk.org/browse/JDK-8328824), we run in
> the AIX build into this failure :
>
> /opt/freeware/bin/bash: -c: line 1: syntax error near unexpected token `('
> gmake[3]: *** [lib/CoreLibraries.gmk:194:
> /openjdk/nb/aix_ppc64/jdk-dev-opt/support/native/java.base/libjli_static/args.o]
> Error 2
> gmake[3]: *** Waiting for unfinished jobs
>
> Looks like an addprefix usage is wrong, a '$' is missing.
This pull request has now been integrated.
Changeset: b9c76ded
Author:Matthias Baesken
URL:
https://git.openjdk.org/jdk/commit/b9c76dedf4aa2248a5e561a535c9e3e181f7836a
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
8329074: AIX build fails after JDK-8328824
Reviewed-by: clanger, goetz
-
PR: https://git.openjdk.org/jdk/pull/18484
Integrated: 8327493: Update minimum Xcode version in docs
On Tue, 26 Mar 2024 09:19:19 GMT, Magnus Ihse Bursie wrote:
> The file building.{md,html} indicates the required minimum version of Xcode
> to use. When the required minimum version of Clang was updated to 13
> ([JDK-8325878](https://bugs.openjdk.org/browse/JDK-8325878)), the minimum
> Xcode version was not updated. It should now be Xcode 13.0, rather than 8.
This pull request has now been integrated.
Changeset: 7b1f2c80
Author:Magnus Ihse Bursie
URL:
https://git.openjdk.org/jdk/commit/7b1f2c801fee6366ee37e873cda29c1ae7ad7e74
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
8327493: Update minimum Xcode version in docs
Reviewed-by: clanger
-
PR: https://git.openjdk.org/jdk/pull/18483
Re: RFR: 8327493: Update minimum Xcode version in docs
On Tue, 26 Mar 2024 09:19:19 GMT, Magnus Ihse Bursie wrote:
> The file building.{md,html} indicates the required minimum version of Xcode
> to use. When the required minimum version of Clang was updated to 13
> ([JDK-8325878](https://bugs.openjdk.org/browse/JDK-8325878)), the minimum
> Xcode version was not updated. It should now be Xcode 13.0, rather than 8.
Marked as reviewed by clanger (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/18483#pullrequestreview-1959819155
Re: RFR: JDK-8329074: AIX build fails after JDK-8328824
On Tue, 26 Mar 2024 09:21:20 GMT, Matthias Baesken wrote:
> After [JDK-8328824](https://bugs.openjdk.org/browse/JDK-8328824), we run in
> the AIX build into this failure :
>
> /opt/freeware/bin/bash: -c: line 1: syntax error near unexpected token `('
> gmake[3]: *** [lib/CoreLibraries.gmk:194:
> /openjdk/nb/aix_ppc64/jdk-dev-opt/support/native/java.base/libjli_static/args.o]
> Error 2
> gmake[3]: *** Waiting for unfinished jobs
>
> Looks like an addprefix usage is wrong, a '$' is missing.
Obvious typo that broke the AIX build. Thanks for fixing.
-
Marked as reviewed by clanger (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18484#pullrequestreview-1959820693
RFR: JDK-8329074: AIX build fails after JDK-8328824
After [JDK-8328824](https://bugs.openjdk.org/browse/JDK-8328824), we run in the
AIX build into this failure :
/opt/freeware/bin/bash: -c: line 1: syntax error near unexpected token `('
gmake[3]: *** [lib/CoreLibraries.gmk:194:
/openjdk/nb/aix_ppc64/jdk-dev-opt/support/native/java.base/libjli_static/args.o]
Error 2
gmake[3]: *** Waiting for unfinished jobs
Looks like an addprefix usage is wrong, a '$' is missing.
-
Commit messages:
- JDK-8329074
Changes: https://git.openjdk.org/jdk/pull/18484/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18484&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8329074
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18484.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18484/head:pull/18484
PR: https://git.openjdk.org/jdk/pull/18484
RFR: 8327493: Update minimum Xcode version in docs
The file building.{md,html} indicates the required minimum version of Xcode to
use. When the required minimum version of Clang was updated to 13
([JDK-8325878](https://bugs.openjdk.org/browse/JDK-8325878)), the minimum Xcode
version was not updated. It should now be Xcode 13.0, rather than 8.
-
Commit messages:
- 8327493: Update minimum Xcode version in docs
Changes: https://git.openjdk.org/jdk/pull/18483/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18483&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8327493
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/18483.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18483/head:pull/18483
PR: https://git.openjdk.org/jdk/pull/18483
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v61]
On Tue, 26 Mar 2024 08:55:56 GMT, Julian Waters wrote:
>> We should set the -permissive- flag for the Microsoft Visual C compiler, as
>> was requested by the now backed out
>> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes
>> the Visual C compiler much less accepting of ill formed code, which will
>> improve code quality on Windows in the future.
>
> Julian Waters has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Whitespace in awt_DnDDS.cpp
> - Whitespace in awt_DnDDT.cpp
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 598:
> 596: int sz = GetLocaleInfo(LOCALE_USER_DEFAULT, LOCALE_IMEASURE,
> NULL, 0);
> 597: if (sz > 0) {
> 598: LPTSTR str = (LPTSTR) SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
> sizeof(TCHAR), sz);
Suggestion:
LPTSTR str = (LPTSTR)SAFE_SIZE_ARRAY_ALLOC(safe_Malloc,
sizeof(TCHAR), sz);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1538833940
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v61]
> We should set the -permissive- flag for the Microsoft Visual C compiler, as > was requested by the now backed out > [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes > the Visual C compiler much less accepting of ill formed code, which will > improve code quality on Windows in the future. Julian Waters has updated the pull request incrementally with two additional commits since the last revision: - Whitespace in awt_DnDDS.cpp - Whitespace in awt_DnDDT.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/90d9096f..ef9f3a62 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=60 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=59-60 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15096/head:pull/15096 PR: https://git.openjdk.org/jdk/pull/15096
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v60]
> We should set the -permissive- flag for the Microsoft Visual C compiler, as > was requested by the now backed out > [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes > the Visual C compiler much less accepting of ill formed code, which will > improve code quality on Windows in the future. Julian Waters has updated the pull request incrementally with two additional commits since the last revision: - Comment in src/java.desktop/windows/native/libawt/windows/awt_DnDDS.cpp Co-authored-by: Magnus Ihse Bursie - Comment in src/java.desktop/windows/native/libawt/windows/awt_DnDDT.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/1341d2e1..90d9096f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=59 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=58-59 Stats: 5 lines in 2 files changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15096/head:pull/15096 PR: https://git.openjdk.org/jdk/pull/15096
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Tue, 26 Mar 2024 07:44:22 GMT, Magnus Ihse Bursie wrote:
> > I have a concern since the null check bailout involves
> > THROW_NULL_PDATA_IF_NOT_DESTROYED, which is no longer accurate if we remove
> > the pData local.
>
> The name of the macro is not great, but it does not involve pData (the bad
> NPE error message notwithstanding):
>
> ```
> #define THROW_NULL_PDATA_IF_NOT_DESTROYED(peer) { \
> jboolean destroyed = JNI_GET_DESTROYED(peer); \
> if (destroyed != JNI_TRUE) { \
> env->ExceptionClear();\
> JNU_ThrowNullPointerException(env, "null pData"); \
> } \
> }
> ```
>
> So you can go ahead and replace the pData references with the variable that
> will eventually be used.
Alright, will do. Maybe as a further improvement, I can inline
THROW_NULL_PDATA_IF_NOT_DESTROYED at its callsites and replace the bad
NullPointerException error message with the proper null pointer name. Since
Phil isn't here, what do you think?
Regardless, I really hope I can get this in by Thursday. University for me
officially ramps up into _very_ high gear about that time, and I doubt I can
juggle both JDK work and it all at once by then
-
PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2019839348
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v59]
> We should set the -permissive- flag for the Microsoft Visual C compiler, as > was requested by the now backed out > [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes > the Visual C compiler much less accepting of ill formed code, which will > improve code quality on Windows in the future. Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/53e8bd6d..1341d2e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=58 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=57-58 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15096/head:pull/15096 PR: https://git.openjdk.org/jdk/pull/15096
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v58]
> We should set the -permissive- flag for the Microsoft Visual C compiler, as > was requested by the now backed out > [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes > the Visual C compiler much less accepting of ill formed code, which will > improve code quality on Windows in the future. Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_Canvas.cpp Co-authored-by: Magnus Ihse Bursie - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/0c409d45..53e8bd6d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=57 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15096&range=56-57 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15096/head:pull/15096 PR: https://git.openjdk.org/jdk/pull/15096
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Mon, 25 Mar 2024 20:19:04 GMT, Hamlin Li wrote: > But anyway, it's a furture incremental solution after this pr, am I right? Or > are we going to change direction? I'm honestly not sure what is the right way forward. It seems we all agree that this PR is not the end solution we want. So the question is, is it worth integrating it while waiting for the correct final solution? Arguments for integrating it: * The functionality can be tested * The source changes gets upstreamed, minimizing risk for future merge conflicts Arguments against integrating it: * In practice, this functionality will be missing from the build and no-one is going to be testing it * It is hard to tell if the functionality is enabled or not * The "glue" code for enabling/disabling this functionality is still not 100% * It might not be worth putting effort into the "glue" code, if this is going to be radically changed later on anyway I hope that was a decent summary of what I and @theRealAph have been saying. If you compare these two lists, it seems like the case for integrating this PR now is rather weak. So maybe it is better to sit on this for a while and await the proper solution? If you disagree, I think you need to explain once more, and more clearly, what you think the gains would be of having this integrated as-is. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2019791355
Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v4]
On Mon, 25 Mar 2024 20:19:04 GMT, Hamlin Li wrote: > It's not necessary to integrate libsleef build sysstem into jdk, we just need > to integrate the final sources (generated by sleef cmake) into jdk. I have > tested it, it works. That is an interesting approach. It will raise the bar for updating the sleef sources, but if we document the process carefully I guess that will not be an issue -- all imported third party sources needs some kind of curation when re-importing. I just had a quick look at the sleef sources, and it seems a bit more complicated. Apart from the generated source code, they also compile the same source code file multiple times with different defines on the command line. This is not something we have support for in the build system, but then again it is not nearly as difficult to solve as the code generation part, so it can certainly be handled in some way or another. So yes, I agree that using this approach it might actually be feasible to include the libsleef sources in the JDK repo. - PR Comment: https://git.openjdk.org/jdk/pull/18294#issuecomment-2019779075
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Tue, 26 Mar 2024 00:13:09 GMT, Julian Waters wrote:
> I have a concern since the null check bailout involves
> THROW_NULL_PDATA_IF_NOT_DESTROYED, which is no longer accurate if we remove
> the pData local.
The name of the macro is not great, but it does not involve pData (the bad NPE
error message notwithstanding):
#define THROW_NULL_PDATA_IF_NOT_DESTROYED(peer) { \
jboolean destroyed = JNI_GET_DESTROYED(peer); \
if (destroyed != JNI_TRUE) { \
env->ExceptionClear();\
JNU_ThrowNullPointerException(env, "null pData"); \
} \
}
So you can go ahead and replace the pData references with the variable that
will eventually be used.
-
PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2019589225
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v57]
On Tue, 26 Mar 2024 00:17:56 GMT, Julian Waters wrote: >> We should set the -permissive- flag for the Microsoft Visual C compiler, as >> was requested by the now backed out >> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes >> the Visual C compiler much less accepting of ill formed code, which will >> improve code quality on Windows in the future. > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert formatting change in > src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp > > Co-authored-by: Magnus Ihse Bursie src/java.desktop/windows/native/libawt/windows/awt_Canvas.cpp line 230: > 228: } > 229: > 230: AwtCanvas *c = (AwtCanvas*) pData; Suggestion: AwtCanvas *c = (AwtCanvas*)pData; - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1538714268
