Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v4]
> WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Capstone Support - Add check for compiler in configure - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis - Changes: https://git.openjdk.org/jdk/pull/18915/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18915=03 Stats: 395 lines in 19 files changed: 201 ins; 41 del; 153 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v3]
> WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Julian Waters 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 three additional commits since the last revision: - Capstone Support - Add check for compiler in configure - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis - Changes: - all: https://git.openjdk.org/jdk/pull/18915/files - new: https://git.openjdk.org/jdk/pull/18915/files/09fb3d65..b0b088e9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18915=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18915=01-02 Stats: 121745 lines in 2137 files changed: 85534 ins; 24227 del; 11984 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 13:01:15 GMT, SendaoYan wrote: >> As confusing as they are, unfortunately GitHub UI does not render extra >> trailing newlines. This is the only one I could find with grepWin. > > I find the extra trailing newlines through below shell command: > > for i in `find . -iname "Makefile*" | sed "/./build/d"` ; do tail -n 2 $i | > grep -c "^$" | grep -q "^1$" ; if [[ 0 -eq $? ]] ; then echo $i ; fi ; done > > > There are only two files has been found: > > ./test/jdk/java/rmi/reliability/benchmark/bench/rmi/Makefile > ./test/jdk/java/rmi/reliability/benchmark/bench/Makefile Ah, I had not realized that there was more than 1 newline. GitHub's UI confused me here, so we're good to go - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631213656
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 07:26:39 GMT, SendaoYan wrote: >> test/jdk/java/rmi/reliability/benchmark/bench/rmi/Makefile line 1: >> >>> 1: # >> >> This file change is dubious: >> 1. It does not have any trailing whitespace that can fail the skara checks. >> 2. If the duplicate blank lines in the end of this Makefile is indeed >> problematic (as fixed here), please fix the only other occasion in the JDK, >> which is the Makefile in the parent directory. (Checked with `\n$^\n$\Z` >> pattern in all Makefiles) >> >> Recommended actions: Either >> 1. Revert changes in this file; >> 2. Also update `test/jdk/java/rmi/reliability/benchmark/bench/Makefile` to >> remove the trailing blank line. > > Thanks for the suggestion, the trailing blank line of > `test/jdk/java/rmi/reliability/benchmark/bench/Makefile` has been removed. Hmm, I'm inclined to keep the newlines at the EOF for both, what do the rest of you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631140457
Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]
On Fri, 7 Jun 2024 07:29:39 GMT, SendaoYan wrote: >> Hi all, >> This PR several extra empty spaces and extra empty lines in several >> Makefiles. It's trivial fix, no risk. >> >> Thanks. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > delete extra empty trailing blank line in > test/jdk/java/rmi/reliability/benchmark/bench/Makefile Marked as reviewed by jwaters (Committer). test/jdk/java/rmi/reliability/benchmark/bench/Makefile line 50: > 48: clean: > 49: rm -f *.class .classes > 50: Hmm, shouldn't this newline at EOF be kept? Asking @ all the people who've reviewed this so far, no need to change it just yet - PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2104439647 PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631140418
Re: RFR: 8333128: Linux x86_32 configure fail with --with-hsdis=binutils --with-binutils-src [v2]
On Mon, 3 Jun 2024 09:13:57 GMT, SendaoYan wrote: >> Hi all, >> This PR try to fix linux x86_32 configure fail with `--with-hsdis=binutils >> --with-binutils-src`. The libiberty.a locates in >> `build/linux-x86-server-fastdebug/configure-support/binutils-install/lib32` >> on linux ubuntu20. The change has been verified, the risk is low. >> >> Additional testing: >> >> - [x] linux x86_32 centos7 configure >> - [x] linux x86_64 centos7 configure >> - [x] linux x86_32 ubuntu20 configure >> - [x] linux x86_64 ubuntu20 configure >> >> >> [configure-linux-centos7-x86_32.log](https://github.com/user-attachments/files/15523974/configure-linux-centos7-x86_32.log) >> [configure-linux-centos7-x86_64.log](https://github.com/user-attachments/files/15523976/configure-linux-centos7-x86_64.log) >> [configure-ubuntu20-x86_32.log](https://github.com/user-attachments/files/15523977/configure-ubuntu20-x86_32.log) >> [configure-ubuntu20-x86_64.log](https://github.com/user-attachments/files/15523978/configure-ubuntu20-x86_64.log) > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > "or $BINUTILS_INSTALL_DIR/lib64" should probably be on the new line in this > comment Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19511#pullrequestreview-2093272582
Re: RFR: 8333128: Linux x86_32 configure fail with --with-hsdis=binutils --with-binutils-src
On Mon, 3 Jun 2024 09:02:47 GMT, Aleksey Shipilev wrote: > > Don't forget about me! :) > > Ah yes, you are also in Build Group now! Apologies. No worries, I was joking lightheartedly :P - PR Comment: https://git.openjdk.org/jdk/pull/19511#issuecomment-2144674185
Re: RFR: 8333128: Linux x86_32 configure fail with --with-hsdis=binutils --with-binutils-src
On Sun, 2 Jun 2024 02:49:47 GMT, SendaoYan wrote: > Hi all, > This PR try to fix linux x86_32 configure fail with `--with-hsdis=binutils > --with-binutils-src`. The libiberty.a locates in > `build/linux-x86-server-fastdebug/configure-support/binutils-install/lib32` > on linux ubuntu20. The change has been verified, the risk is low. > > Additional testing: > > - [x] linux x86_32 centos7 configure > - [x] linux x86_64 centos7 configure > - [x] linux x86_32 ubuntu20 configure > - [x] linux x86_64 ubuntu20 configure > > > [configure-linux-centos7-x86_32.log](https://github.com/user-attachments/files/15523974/configure-linux-centos7-x86_32.log) > [configure-linux-centos7-x86_64.log](https://github.com/user-attachments/files/15523976/configure-linux-centos7-x86_64.log) > [configure-ubuntu20-x86_32.log](https://github.com/user-attachments/files/15523977/configure-ubuntu20-x86_32.log) > [configure-ubuntu20-x86_64.log](https://github.com/user-attachments/files/15523978/configure-ubuntu20-x86_64.log) Don't forget about me! :) make/autoconf/lib-hsdis.m4 line 274: > 272: HSDIS_CFLAGS="-DLIBARCH_$OPENJDK_TARGET_CPU_LEGACY_LIB > -I$BINUTILS_INSTALL_DIR/include" > 273: > 274: # libiberty ignores --libdir and may be installed in > $BINUTILS_INSTALL_DIR/lib, $BINUTILS_INSTALL_DIR/lib32 or > $BINUTILS_INSTALL_DIR/lib64 Nit: "or $BINUTILS_INSTALL_DIR/lib64" should probably be on the new line in this comment - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/19511#pullrequestreview-2093236451 PR Review Comment: https://git.openjdk.org/jdk/pull/19511#discussion_r1624067867
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
On Wed, 15 May 2024 13:32:38 GMT, Magnus Ihse Bursie wrote:
> Hi Julian, sorry for not getting back to you.
>
> The problem from my PoV is that this is a very large and intrusive patch in
> the build of the actual product, for a claimed problem in the tangential
> hsdis library. I have not understood a pressing business need to get a
> Windows/gcc port for hsdis, which means I can't really prioritize trying to
> understand this patch.
>
> As you know, the build system does not currently really separate between "the
> OS is Windows" and "the toolchain is Microsoft". I understand that you want
> to fix that, and in theory I support it. However, you must also realize that
> making a complete fix of this requires a lot of work, for something that
> there is no clearly defined need. (After all, cl.exe works fine to create the
> build, has no apparent issues, and is as far as an "official" compiler for
> Windows as you can get.) That makes it fall squarely in the WIBNIs bucked
> ("wouldn't it be nice if...").
>
> If you can fix just the hsdis build without changing anything outside the
> hsdis Makefiles, that would be a different story. Such a change would be
> limited in scope, easy to say it will not affect the product proper, and be
> easier to review.
Oh, no worries. Sorry for sounding a little impatient.
The problem is that there are areas in the build system that will need changing
to support hsdis compilation, not just the hsdis Makefile and autoconf file
itself. I can try to work on minimizing the amount of changes to non-hsdis
files (I was hoping the current changes were small enough, but it seems they
are not), but I believe it's impossible to achieve this by only touching the
hsdis Makefile and lib-hsdis.m4. That is, there will necessarily be changes, no
matter how minimal, to non-hsdis files.
As for why do this at all, I was mainly driven by seeing the hack in place for
the binutils backend on Windows. It only supports Cygwin, of which the gcc
installation is subpar compared to the newer gcc provided by some MSYS2
subsystems (MINGW64 gcc is primarily battle tested on MSYS2, on Cygwin it
doesn't get as much attention and misses out further fixes and enhancements
from the MSYS2 team, for instance it even links to the obsolete msvcrt
library), and the hack itself has several flaws that don't seem apparent at
first (For instance, -lpthread will link to libwinpthread.dll and result in an
invisible dependency on that dll depending on the Windows platform, which will
cause loading hsdis to silently fail depending on how it's loaded for seemingly
random reasons!). I wanted to replace that (or I guess provide a better
alternative) by adding semi proper support to the hsdis build for the more
modern and better battle tested gcc that some MSYS2 subsystems provide, to
streamline comp
iling the binutils hsdis on Windows. So this is mainly about hsdis-binutils on
Windows. hsdis-capstone I also decided to support because it's small and
relatively easy to pile on top of the existing work, and MSYS2 also provides
Capstone as well. The same unfortunately could not be said for hsdis-llvm,
which was significantly more complex than Capstone is
I'm not sure where to go from here. I could support gcc for hsdis binutils by
extending the hack that exists for Cygwin, but I _really_ don't want to do
that. I could enhance the build system to semi properly support gcc for hsdis
only, as I've done, but the changes will necessarily touch non hsdis files as
well, no matter how minimal they are. I'll return this to draft for the time
being I suppose, I'd be interested in hearing which way forward you prefer
though
But while I work on making this change even smaller and easier to review, could
I ask the above questions again? (Well, except for the FIXPATH one, you can
ignore that)
> @magicus I think I might need some help here. Currently all the Cygwin stuff
> is gated behind ifeq ($(TOOLCHAIN_TYPE), microsoft) checks... should they be
> behind isBuildOsEnv cygwin checks instead? I don't know how to add support
> for Cygwin gcc for most of hsdis, since it is quite different from the more
> modern gcc distributions that are found in places like MSYS2 and MINGW64. But
> most of the existing logic seems to accomodate more for the Microsoft
> compiler than it is concerned about the OS environment being used, and for
> this reason I can't tell which of the 2 checks to use for the existing hack
> that switches from microsoft to gcc. Also, gcc doesn't require FIXPATH, but
> Microsoft does, but I don't want to check for microsoft inside
> TOOLCHAIN_FIND_COMPILER, what should I do instead to ensure gcc doesn't get
> FIXPATH while Microsoft does?
> Also @magicus, what is the typical path passed to --with-binutils like on
> Windows? Something like
> --with-binutils=/c/Users/vertig0/Downloads/binutils-2.42 doesn't work
> correctly, since the include path to dis-asm.h
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
On Thu, 9 May 2024 07:50:00 GMT, Julian Waters wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Add check for compiler in configure > - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis Magnus? Erik? You guys there? :( - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2112465392
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
On Thu, 9 May 2024 07:50:00 GMT, Julian Waters wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Julian Waters has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Add check for compiler in configure > - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis Hmm, it seems Magnus isn't available at the moment. @erikj79 might you be able to answer my questions regarding hsdis? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2109049333
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Please mark the PR as draft it is not intended for review. @magicus? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2106619396
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Please mark the PR as draft it is not intended for review. @magicus? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2103667760
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Please mark the PR as draft it is not intended for review. Also @magicus, what is the typical path passed to --with-binutils like on Windows? Something like --with-binutils=/c/Users/vertig0/Downloads/binutils-2.42 doesn't work correctly, since the include path to dis-asm.h would then become `#include "/c/Users/vertig0/Downloads/binutils-2.42/include/dis-asm.h"` Which causes a configure check to fail on the compile stage since gcc cannot recognise the MINGW-esque /c/ as a drive, and then causes configure to erroneously report binutils as using the Old API when it's in fact using the New API. --with-binutils=C:/Users/vertig0/Downloads/binutils-2.42 on the other hand works as expected. Should there be a fixup for the path there so gcc can recognise it properly? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2102146604
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]
> WIP > > This changeset contains hsdis for Windows/gcc Port. It supports both the > binutils and capstone backends, though the LLVM backend is left out due to > compatibility issues encountered during the build. Currently, which gcc > distributions are supported is still to be clarified, as several, ranging > from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, > the build system hack in place at the moment to compile the binutils backend > on Windows is still left in place, for now. Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains two commits: - Add check for compiler in configure - 8330988: Implementation of 8288293: Windows/gcc Port for hsdis - Changes: https://git.openjdk.org/jdk/pull/18915/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18915=01 Stats: 334 lines in 19 files changed: 154 ins; 25 del; 155 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Wed, 24 Apr 2024 09:15:21 GMT, Magnus Ihse Bursie wrote: >> WIP >> >> This changeset contains hsdis for Windows/gcc Port. It supports both the >> binutils and capstone backends, though the LLVM backend is left out due to >> compatibility issues encountered during the build. Currently, which gcc >> distributions are supported is still to be clarified, as several, ranging >> from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, >> the build system hack in place at the moment to compile the binutils backend >> on Windows is still left in place, for now. > > Please mark the PR as draft it is not intended for review. @magicus I think I might need some help here. Currently all the Cygwin stuff is gated behind ifeq ($(TOOLCHAIN_TYPE), microsoft) checks... should they be behind isBuildOsEnv cygwin checks instead? I don't know how to add support for Cygwin gcc for most of hsdis, since it is quite different from the more modern gcc distributions that are found in places like MSYS2 and MINGW64. But most of the existing logic seems to accomodate more for the Microsoft compiler than it is concerned about the OS environment being used, and for this reason I can't tell which of the 2 checks to use for the existing hack that switches from microsoft to gcc. Also, gcc doesn't require FIXPATH, but Microsoft does, but I don't want to check for microsoft inside TOOLCHAIN_FIND_COMPILER, what should I do instead to ensure gcc doesn't get FIXPATH while Microsoft does? - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2097196121
How do I reliably prevent CDS archive generation during builds?
Hi Thomas, --disable-jvm-feature-link-time-opt is for disabling Link Time Optimization when compiling the JVM itself, as in, requesting LTO from the linker that is linking the JVM. It doesn't have anything to do with what arguments the newly compiled JVM is called with and isn't related to the link optimization pass of the Java classfiles (I am not very familiar with that process), and it is also off by default for most platforms best regards, Julian
Re: RFR: 8330539: Use #include instead of -Dalloca'(size)'=__builtin_alloca'(size)' for AIX
On Thu, 2 May 2024 09:54:14 GMT, Joachim Kern wrote: > We need to find a better way to handle alloca on AIX. > > See the discussion in the PR for https://bugs.openjdk.org/browse/JDK-8329257, > e.g. https://github.com/openjdk/jdk/pull/18536#discussion_r1568650313 in > which three alternatives are suggested. Quoting: > > Let me summarize the choices we have and ask for your vote. > Magnus dislikes the -Dalloca'(size)'=__builtin_alloca'(size)' in > flags-cflags.m4 I introduced to get rid of > > #if defined(_AIX) > #include > #endif > > in globalDefinitions_gcc.hpp. > > We have four possible solutions > > 1. Reintroduce > > #if defined(_AIX) > #include > #endif > > in globalDefinitions_gcc.hpp. > > 2. Unconditionally introduce only #include in > globalDefinitions_gcc.hpp. This should work for all platforms using this > header including the unofficial Windows/gcc Port, although only AIX needs it. > > 3. Add > > #if defined(_AIX) > #include > #endif > > to the sources using alloca(). These are > /hotspot/share/runtime/os.cpp > /hotspot/share/runtime/javaThread.cpp > /hotspot/share/utilities/vmError.cpp > Here we need the AIX condition, because otherwise the classic Windows Build > (NTAMD64) fails. > > 4. Replace -Dalloca'(size)'=__builtin_alloca'(size)' in flags-cflags.m4 by > -U__STRICT_ANSI__ at the same place. Explanation can also found in > https://github.com/openjdk/jdk/pull/18536#discussion_r1583360569 and > following. > > I will implement the solution with the most likes and having no dislike. I stand corrected, it seems that MinGW distributions don't have alloca.h in their headers. No worries, the Windows/gcc Port will handle this downstream - PR Comment: https://git.openjdk.org/jdk/pull/19053#issuecomment-2091958780
Re: RFR: 8330539: Use #include instead of -Dalloca'(size)'=__builtin_alloca'(size)' for AIX
On Thu, 2 May 2024 09:54:14 GMT, Joachim Kern wrote: > We need to find a better way to handle alloca on AIX. > > See the discussion in the PR for https://bugs.openjdk.org/browse/JDK-8329257, > e.g. https://github.com/openjdk/jdk/pull/18536#discussion_r1568650313 in > which three alternatives are suggested. Quoting: > > Let me summarize the choices we have and ask for your vote. > Magnus dislikes the -Dalloca'(size)'=__builtin_alloca'(size)' in > flags-cflags.m4 I introduced to get rid of > > #if defined(_AIX) > #include > #endif > > in globalDefinitions_gcc.hpp. > > We have four possible solutions > > 1. Reintroduce > > #if defined(_AIX) > #include > #endif > > in globalDefinitions_gcc.hpp. > > 2. Unconditionally introduce only #include in > globalDefinitions_gcc.hpp. This should work for all platforms using this > header including the unofficial Windows/gcc Port, although only AIX needs it. > > 3. Add > > #if defined(_AIX) > #include > #endif > > to the sources using alloca(). These are > /hotspot/share/runtime/os.cpp > /hotspot/share/runtime/javaThread.cpp > /hotspot/share/utilities/vmError.cpp > Here we need the AIX condition, because otherwise the classic Windows Build > (NTAMD64) fails. > > 4. Replace -Dalloca'(size)'=__builtin_alloca'(size)' in flags-cflags.m4 by > -U__STRICT_ANSI__ at the same place. Explanation can also found in > https://github.com/openjdk/jdk/pull/18536#discussion_r1583360569 and > following. > > I will implement the solution with the most likes and having no dislike. I'd put alloca.h down below the Standard Headers, next to stuff like pthread.h and dlfcn.h, and have the AIX ifdef check so it's clearer that only AIX needs it, but otherwise no objections Compilation Failures are all unrelated to the actual change itself - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/19053#pullrequestreview-2035766825 PR Comment: https://git.openjdk.org/jdk/pull/19053#issuecomment-2090522085
Re: RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
On Tue, 30 Apr 2024 12:27:42 GMT, Magnus Ihse Bursie wrote: > Please revert back to the LINK_TYPE name. As long as it is not used for > anything else, this is a better description. If we start to use it to have a > broader meaning, we can rename it then. I switched it back to LANG since in the original change you switched it to LINK_TYPE from LANG after one of my objections. I had since retracted that objection and have been feeling bad about it. Have you since changed your mind about LANG vs LINK_TYPE in that time? It might take me a bit of time to address these reviews, sorry about that - PR Comment: https://git.openjdk.org/jdk/pull/18927#issuecomment-2085401560
Re: RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
On Tue, 30 Apr 2024 12:32:09 GMT, Magnus Ihse Bursie wrote: >> Currently, on Windows LANG is not assigned to C++ for some code that does >> use C++. This just works because link.exe does not bother about what kind of >> code it is currently linking. gcc however, does. It doesn't hurt to assign >> LANG to C++ as a formality in such cases, which this changeset does. This >> also renames LINK_TYPE to LANG, which the original change to remove the >> TOOLCHAIN parameter used to do > > make/modules/java.desktop/lib/AwtLibraries.gmk line 102: > >> 100: $(eval $(call SetupJdkLibrary, BUILD_LIBAWT, \ >> 101: NAME := awt, \ >> 102: LANG := $(if $(filter $(OPENJDK_TARGET_OS), windows), C++, C), \ > > No, this is not okay. You need to do this as for the LIBJSOUND_LINK_TYPE > above. The same goes for the one below, too. Oh, I'm surprised! I thought that you'd prefer the more lambda-like approach. I guess the other way of LIBAWT_LINK_TYPE works too in that case - PR Review Comment: https://git.openjdk.org/jdk/pull/18927#discussion_r1584867458
RFR: 8331020: Assign LANG to C++ for Windows code that uses C++
Currently, on Windows LANG is not assigned to C++ for some code that does use C++. This just works because link.exe does not bother about what kind of code it is currently linking. gcc however, does. It doesn't hurt to assign LANG to C++ as a formality in such cases, which this changeset does. This also renames LINK_TYPE to LANG, which the original change to remove the TOOLCHAIN parameter used to do - Commit messages: - Assign LANG to C++ in Lib.gmk - Assign LANG to C++ in Lib.gmk - Assign LANG to C++ in Launcher.gmk - Assign LANG to C++ in Lib.gmk - Assign LANG to C++ in AwtLibraries.gmk - Rename import in Lib.gmk - Rename ClientLibraries.gmk to 2dLibraries.gmk - LINK_TYPE to LANG in ClientLibraries.gmk - LINK_TYPE to LANG in JdkNativeCompilation.gmk - Add C++ to Windows in Lib.gmk - ... and 11 more: https://git.openjdk.org/jdk/compare/7a895552...8d0701bd Changes: https://git.openjdk.org/jdk/pull/18927/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18927=00 Issue: https://bugs.openjdk.org/browse/JDK-8331020 Stats: 52 lines in 19 files changed: 15 ins; 3 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/18927.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18927/head:pull/18927 PR: https://git.openjdk.org/jdk/pull/18927
Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
On Tue, 23 Apr 2024 14:25:22 GMT, Magnus Ihse Bursie wrote: > There's a huge amount of changes for just hsdis... You might have to separate > out the infrastructure changes that seem to amount to most of the changes > here. > > This is going to take me a while to get through. Sorry, it's still a WIP, and I believe something broke and scattered changes from one of my other local branches into this current changeset - PR Comment: https://git.openjdk.org/jdk/pull/18915#issuecomment-2072483937
RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis
WIP This changeset contains hsdis for Windows/gcc Port. It supports both the binutils and capstone backends, though the LLVM backend is left out due to compatibility issues encountered during the build. Currently, which gcc distributions are supported is still to be clarified, as several, ranging from Cygwin, to MinGW64, to the gcc subsystems from MSYS2. For this reason, the build system hack in place at the moment to compile the binutils backend on Windows is still left in place, for now. - Commit messages: - Implementation of hsdis for 8288293: Windows/gcc Port Changes: https://git.openjdk.org/jdk/pull/18915/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18915=00 Issue: https://bugs.openjdk.org/browse/JDK-8330988 Stats: 347 lines in 28 files changed: 161 ins; 22 del; 164 mod Patch: https://git.openjdk.org/jdk/pull/18915.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18915/head:pull/18915 PR: https://git.openjdk.org/jdk/pull/18915
Re: Questions about the Hermetic Java project
Oops, I meant
__declspec(dllexport) void exportedMethod() {} with braces, not brackets
On Thu, Apr 18, 2024 at 8:27 PM Julian Waters wrote:
>
> I have good (and bad news) for you!
>
> The good news is that from memory, ld and gcc (but I assume you are
> only concerned about ld) can work entirely on cl compiled object files
> with no hitch whatsoever, and partial linking is fully functional with
> ld on Windows! Symbol hiding also works, but with a gotcha.
>
> Now, the bad news. --localize-hidden, to my knowledge, only works on
> ELF visibilities. This means that for the following file:
>
> exports.c
> __declspec(dllexport) void exportedMethod() ()
> void unexportedMethod() {}
>
> When objcopy is run over exports.o with --localize-hidden, it will
> work on the cl compiled object file just fine, but it will not
> recognise unexportedMethod() as a hidden method, because dllexport and
> hidden visibility are entirely separate concepts to gcc. The resulting
> exports-objcopy.o file will still have unexportedMethod as public.
> However, not all is lost just yet. --localize-symbol= still
> works perfectly fine, and can be used as a workaround if we can find a
> way to check for non-dllexport methods in cl object files and then
> feed that through the command line. I should note that clang also
> suffers from the same issue, as --localize-hidden on clang's objcopy
> also leaves unexportedMethod as public.
>
> Happy to help!
>
> best regards,
> Julian
>
> On Thu, Apr 18, 2024 at 6:28 PM Magnus Ihse Bursie
> wrote:
> >
> > On 2024-04-17 14:06, Julian Waters wrote:
> >
> > > Hi Magnus,
> > >
> > > Yes, I'm talking about the MSYS2 objcopy, but to a lesser extent also
> > > standalone Windows objcopy builds too. objcopy should be able to
> > > handle .o files from cl.exe (I assume that's what everyone here is
> > > after considering all the talk about .o files?), but to answer that
> > > question properly, I'd need a bit more detail. What kind of usage of
> > > objcopy do you have in mind? A general-ish command line example could
> > > be helpful
> >
> > To make a static library behave somewhat like a dynamic library, and not
> > expose all its internal symbols to the rest of the world, there are
> > basically two operations that needs to be done:
> >
> > 1) Combine all .o files into a single .o file, to make it look like it
> > was compiled by a single source code. That way, symbols that were
> > created in one source file and referenced in another will now behave as
> > if they are internal to the "compilation unit", i.e. like they were
> > declared static.
> >
> > 2) Modify the symbol status so that symbols that are not exported are
> > changed so they look like they were actually declared "static" in the
> > source code.
> >
> > These operations are based on concepts that exists in the gcc and clang
> > toolchain, about symbol visibility. I am not sure how well they
> > translate to a Microsoft setting. But, if the dll_export marker
> > corresponds to visible symbols, then I guess we should be able to
> > achieve something similar.
> >
> > What needs to be done then is:
> >
> > 1) Combine multiple .obj (COFF object files) into one.
> >
> > 2) Change the visibility of symbols that are not marked as dll_export:ed
> > to they appear like they were declared static.
> >
> > In the clang/gcc world, the first step is done by "partial linking" by
> > ld. That is our first blocker -- link.exe cannot do that. So the first
> > question is really, is there a Windows build of ld that can work on COFF
> > files to achieve this?
> >
> > The second step is done by objcopy using the "--localize-hidden"
> > argument. The second question is, could this work on a COFF object file?
> >
> > /Magnus
> >
> >
> > >
> > > best regards,
> > > Julian
> > >
> > > On Wed, Apr 17, 2024 at 5:55 PM Magnus Ihse Bursie
> > > wrote:
> > >> On 2024-04-16 07:23, Julian Waters wrote:
> > >>
> > >>>> And finally, on top of all of this, is the question of widening the
> > >>>> platform support. To support linux/gcc with objcopy is trivial, but
> > >>>> the question about Windows still remain.
> > >>> objcopy is also available on Windows, if the question about
> > >>> alternative tooling is still unanswered :)
> > >> At this point, I think support for static build on Windows is to either
> > >>
Re: Questions about the Hermetic Java project
I have good (and bad news) for you!
The good news is that from memory, ld and gcc (but I assume you are
only concerned about ld) can work entirely on cl compiled object files
with no hitch whatsoever, and partial linking is fully functional with
ld on Windows! Symbol hiding also works, but with a gotcha.
Now, the bad news. --localize-hidden, to my knowledge, only works on
ELF visibilities. This means that for the following file:
exports.c
__declspec(dllexport) void exportedMethod() ()
void unexportedMethod() {}
When objcopy is run over exports.o with --localize-hidden, it will
work on the cl compiled object file just fine, but it will not
recognise unexportedMethod() as a hidden method, because dllexport and
hidden visibility are entirely separate concepts to gcc. The resulting
exports-objcopy.o file will still have unexportedMethod as public.
However, not all is lost just yet. --localize-symbol= still
works perfectly fine, and can be used as a workaround if we can find a
way to check for non-dllexport methods in cl object files and then
feed that through the command line. I should note that clang also
suffers from the same issue, as --localize-hidden on clang's objcopy
also leaves unexportedMethod as public.
Happy to help!
best regards,
Julian
On Thu, Apr 18, 2024 at 6:28 PM Magnus Ihse Bursie
wrote:
>
> On 2024-04-17 14:06, Julian Waters wrote:
>
> > Hi Magnus,
> >
> > Yes, I'm talking about the MSYS2 objcopy, but to a lesser extent also
> > standalone Windows objcopy builds too. objcopy should be able to
> > handle .o files from cl.exe (I assume that's what everyone here is
> > after considering all the talk about .o files?), but to answer that
> > question properly, I'd need a bit more detail. What kind of usage of
> > objcopy do you have in mind? A general-ish command line example could
> > be helpful
>
> To make a static library behave somewhat like a dynamic library, and not
> expose all its internal symbols to the rest of the world, there are
> basically two operations that needs to be done:
>
> 1) Combine all .o files into a single .o file, to make it look like it
> was compiled by a single source code. That way, symbols that were
> created in one source file and referenced in another will now behave as
> if they are internal to the "compilation unit", i.e. like they were
> declared static.
>
> 2) Modify the symbol status so that symbols that are not exported are
> changed so they look like they were actually declared "static" in the
> source code.
>
> These operations are based on concepts that exists in the gcc and clang
> toolchain, about symbol visibility. I am not sure how well they
> translate to a Microsoft setting. But, if the dll_export marker
> corresponds to visible symbols, then I guess we should be able to
> achieve something similar.
>
> What needs to be done then is:
>
> 1) Combine multiple .obj (COFF object files) into one.
>
> 2) Change the visibility of symbols that are not marked as dll_export:ed
> to they appear like they were declared static.
>
> In the clang/gcc world, the first step is done by "partial linking" by
> ld. That is our first blocker -- link.exe cannot do that. So the first
> question is really, is there a Windows build of ld that can work on COFF
> files to achieve this?
>
> The second step is done by objcopy using the "--localize-hidden"
> argument. The second question is, could this work on a COFF object file?
>
> /Magnus
>
>
> >
> > best regards,
> > Julian
> >
> > On Wed, Apr 17, 2024 at 5:55 PM Magnus Ihse Bursie
> > wrote:
> >> On 2024-04-16 07:23, Julian Waters wrote:
> >>
> >>>> And finally, on top of all of this, is the question of widening the
> >>>> platform support. To support linux/gcc with objcopy is trivial, but the
> >>>> question about Windows still remain.
> >>> objcopy is also available on Windows, if the question about
> >>> alternative tooling is still unanswered :)
> >> At this point, I think support for static build on Windows is to either
> >> require additional tooling on top of the Microsoft Visual Studio
> >> toolchain, or to drop it completely, so I am definitely interested in
> >> researching alternatives.
> >>
> >> Can objcopy (I assume this is from msys?) deal with COFF files generated
> >> by cl?
> >>
> >> Switching the entire toolchain is not relevant at this point (but if a
> >> non-Microsoft toolchain build for Windows is ever integrated, it might
> >> get static builds with no extra work as a bonus), but I could certainly
> >> accept the idea of having one or a few additio
Re: RFR: 8330107: Separate out "awt" libraries from Awt2dLibraries.gmk [v3]
On Tue, 16 Apr 2024 18:34:49 GMT, Magnus Ihse Bursie wrote: > There is no good IDE support for Makefiles, especially not with the level of > customization that we have done to make. I am positively struggling with > navigating this file, time and time again. I find Eclipse CDT to be rather helpful with Makefile syntax highlighting, if that helps. Just have to set the .gmk extension to be recognized as a Makefile, and then you should be good to go - PR Comment: https://git.openjdk.org/jdk/pull/18743#issuecomment-2063032309
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Wed, 17 Apr 2024 10:54:25 GMT, Joachim Kern wrote: >> (If some of these files happen to be files which are not compiled on >> Windows, I assume it will not hurt to drop the ifdef guard, but then again, >> it can certainly be kept as well for consistency.) > > @magicus @TheShermanTanker @TheRealMDoerr @kimbarrett > Let me summarize the choices we have and ask for your vote. > Julian dislikes the `-Dalloca'(size)'=__builtin_alloca'(size)'` in > `flags-cflags.m4` I introduced to get rid of > > #if defined(_AIX) > #include > #endif > > in `globalDefinitions_gcc.hpp`. > > We have three possible solutions > > 1. Reintroduce > > #if defined(_AIX) > #include > #endif > > in `globalDefinitions_gcc.hpp`. > > 2. Unconditionally introduce only `#include ` in > `globalDefinitions_gcc.hpp`. This should work for all platforms using this > header including the unofficial Windows/gcc Port, although only AIX needs it. > > 3. Add > > #if defined(_AIX) > #include > #endif > > to the sources using alloca(). These are > /hotspot/share/runtime/os.cpp > /hotspot/share/runtime/javaThread.cpp > /hotspot/share/utilities/vmError.cpp > Here we need the AIX condition, because otherwise the classic Windows Build > (NTAMD64) fails. > > I will implement the solution with the most likes and having no dislike. I don't mind all 3, though I certainly prefer 1 and 3 over 2 (The way I see it, the AIX macro check is more of a message to the programmer than it is important to the compiler, so I prefer the options that have it. However, I also don't mind if we were to go the way of option 2, this is more of a preference thing). The fact that only 3 files need it is also surprising to me, and makes option 3 seem like a good fit (Again, personal preference) Magnus and Kim, what do you guys think? - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1568718288
Re: RFR: 8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 16 Apr 2024 09:15:19 GMT, Magnus Ihse Bursie wrote: >> That was kind of where the discussion started, and which Kim did not like. >> If I read him correctly, his suggestion was instead to place: >> >> #if defined(_AIX) >> #include >> #endif >> >> in the files where `alloca` is needed on AIX. > > (If some of these files happen to be files which are not compiled on Windows, > I assume it will not hurt to drop the ifdef guard, but then again, it can > certainly be kept as well for consistency.) Windows does use this file, in the unofficial Windows/gcc Port. That said, I am fairly sure the Windows distribution of gcc does recognise alloca.h. It would be a little strange to unconditionally include this if only AIX needs it, though - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1567076923
Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v8]
On Wed, 10 Apr 2024 21:10:27 GMT, Magnus Ihse Bursie wrote: >> This is the pinnacle of the recent stream of refactorings in the build >> system. This patch introduces a more abstract concept of "JDK_LIBS", where >> only the library name (e.g. "java" or "java.desktop:jawt") is specified, and >> the build system turns this into suitable linker flags: `-ljawt -L> path>` or `jawt.lib -libpath:`, depending on linker. It will >> also automatically create proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix missing lib prefix No worries, I can clean it up on my end, since it's not that big of a change. I just needed to know whether the LIBPATH pointed to the import library or library itself when linking (Also another question, AddJdkLibrary _is_ what adds the library path and library itself to the LDFLAGS and LIBS respectively, right?) - PR Comment: https://git.openjdk.org/jdk/pull/18649#issuecomment-2056094573
Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v8]
On Wed, 10 Apr 2024 21:10:27 GMT, Magnus Ihse Bursie wrote: >> This is the pinnacle of the recent stream of refactorings in the build >> system. This patch introduces a more abstract concept of "JDK_LIBS", where >> only the library name (e.g. "java" or "java.desktop:jawt") is specified, and >> the build system turns this into suitable linker flags: `-ljawt -L> path>` or `jawt.lib -libpath:`, depending on linker. It will >> also automatically create proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix missing lib prefix Quick Question: On Windows, does it set LIBPATH to the path of the .lib import library? If so, are the import libraries in the same directories as the compiled .dll files? Reason is that gcc does not depend on .lib import libraries and directly links to the dll file, and if the path given is to the non-existent .lib file when compiling with gcc, linking will fail - PR Comment: https://git.openjdk.org/jdk/pull/18649#issuecomment-2054974650
Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v8]
On Wed, 10 Apr 2024 21:10:27 GMT, Magnus Ihse Bursie wrote: >> This is the pinnacle of the recent stream of refactorings in the build >> system. This patch introduces a more abstract concept of "JDK_LIBS", where >> only the library name (e.g. "java" or "java.desktop:jawt") is specified, and >> the build system turns this into suitable linker flags: `-ljawt -L> path>` or `jawt.lib -libpath:`, depending on linker. It will >> also automatically create proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix missing lib prefix make/common/JdkNativeCompilation.gmk line 212: > 210: > 211: ifneq ($(STATIC_LIBS), true) > 212: ifeq ($$(call isTargetOs, windows), true) I should've looked through this more carefully. The selection on whether to use -L and -libpath: should've been based on the compiler, not the target OS - PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1565162835
Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v5]
On Wed, 10 Apr 2024 13:51:30 GMT, Magnus Ihse Bursie wrote: >> This is the pinnacle of the recent stream of refactorings in the build >> system. This patch introduces a more abstract concept of "JDK_LIBS", where >> only the library name (e.g. "java" or "java.desktop:jawt") is specified, and >> the build system turns this into suitable linker flags: `-ljawt -L> path>` or `jawt.lib -libpath:`, depending on linker. It will >> also automatically create proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with three > additional commits since the last revision: > > - Clarify libjvm virtual library > - Fix indentation > - Remove misplaced line Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/18649#pullrequestreview-1991798951
Re: RFR: JDK-8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Wed, 10 Apr 2024 10:13:37 GMT, Joachim Kern wrote: >> Can `-Dalloca=__builtin_alloca` replace `#include `? > > Yes I believe. I will remove the `#pragma alloca` everywhere, I will remove > the `#include ` everywhere and I will add > `-Dalloca=__builtin_alloca` to the compile commands. If it works I will > update the PR. In my humble opinion the inclusion of alloca.h was slightly cleaner, but I guess it doesn't matter. Out of curiosity, why do you guys prefer not including it? - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1559450230
Re: RFR: 8328614: hsdis: dlsym can't find decode symbol [v2]
On Fri, 5 Apr 2024 12:43:30 GMT, Magnus Ihse Bursie wrote: > > Since windows build seems to need HSDIS_TOOLCHAIN_DEFAULT_CFLAGS > > Rght. That is since we use a completely different compiler, gcc instead > of cl. (Which is probably the worst hack in all of the JDK build system...) I plan on adding support for using gcc to compile hsdis on Windows sometime soon (Like proper support for gcc as a toolchain on Windows, at least enough to support hsdis, so people can freely use any gcc instead of only being restricted to Cygwin), what might be an elegant solution to this issue, if you have any in mind? - PR Comment: https://git.openjdk.org/jdk/pull/18400#issuecomment-203998
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v2]
On Thu, 4 Apr 2024 22:47:18 GMT, Magnus Ihse Bursie wrote:
>> This is a retake on https://github.com/openjdk/jdk/pull/15096.
>>
>> I tried to fix the remaining issues in that PR, but could not push them. In
>> the end, it seemed easier to create a new branch in my own personal fork.
>>
>> The majority of the work in this PR has been done by @TheShermanTanker. I
>> have stepped in and fixed the remaining comments, reverted some additional
>> unneeded changed code, and also tried to minimize code duplication in
>> `awt_PrintJob.cpp`.
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to
> a merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains two additional
> commits since the last revision:
>
> - Merge branch 'master' into awt-permissive-minus
> - 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft
> Visual C compiler
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6370:
> 6368: AwtComponent *awtParent = NULL;
> 6369:
> 6370: if (self == NULL) {
I had missed this in my earlier sweep of the changes, but didn't Phil request
that both the check for self and parent be merged into self == NULL || parent
== NULL? (Or as I'd prefer, nullptr)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18584#discussion_r1552961774
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler
On Tue, 2 Apr 2024 15:45:59 GMT, Magnus Ihse Bursie wrote: > This is a retake on https://github.com/openjdk/jdk/pull/15096. > > I tried to fix the remaining issues in that PR, but could not push them. In > the end, it seemed easier to create a new branch in my own personal fork. > > The majority of the work in this PR has been done by @TheShermanTanker. I > have stepped in and fixed the remaining comments, reverted some additional > unneeded changed code, and also tried to minimize code duplication in > `awt_PrintJob.cpp`. Looks good. Thanks for taking this up for me! In the future I hope @prrace will change his mind and allow the use of C++ Libraries in awt (awt already depends on the C++ Standard Library on Windows, verifiable if you run dumpbin -DEPENDENTS on it), because I have a change in mind relying on RAII that would look much cleaner and neater - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18584#pullrequestreview-1975384014
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Mon, 1 Apr 2024 10:33:41 GMT, Julian Waters wrote: >> Julian Waters has updated the pull request incrementally with four >> additional commits since the last revision: >> >> - Labels to empty line in awt_Window.cpp >> - Labels to empty line in awt_Window.cpp >> - Label to empty line in awt_Window.cpp >> - Label to empty line in awt_Window.cpp > > Bumping > @TheShermanTanker I tried to help you get this done. I added fixes to a copy > of your branch on my personal fork, but then it turned out I could not push > them to your branch. :-( > > It ended up with me creating a new PR, #18584. As a bonus, I think it might > be easier to review with a fresh start. This PR has grown quite heavy with > lots of comments and commits. > > I hope you don't feel like I'm stealing this away from you. You have done a > great job, and shown a lot of patience of carrying this all the way here. But > I also got the impression that you would appreciate my assistance in getting > the last pieces in place so we can integrate this. Not at all, I don't feel like you're stealing this from me. In fact, I should be the one apologising for giving you extra work! Thanks for taking this up, I once again apologise for making you do this instead, I've been very busy since Thursday (working on OpenJDK while in lectures at times), and during my breaks I've been too drained to continue, so i really appreciate your help :) I will keep this open until the other Pull Request has been integrated, in case this might still be needed - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2033427284
Re: RFR: JDK-8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc [v3]
On Tue, 2 Apr 2024 20:10:12 GMT, Kim Barrett wrote: >> I'm waiting for a bunch of tests to complete, so decided to just take that >> issue. > > https://github.com/openjdk/jdk/pull/18586 @kimbarrett I've been doing things to permit gcc/Windows, not clang. clang has too many different distributions on Windows for me to settle on one, and generalising all of them to be able to compile with any of the Windows clang distributions seamlessly and without issues sounds like a nightmare :P gcc on the other hand has just 2: MSYS2 MINGW64 with ucrt (Which is the one I'm working on) and standalone gcc Windows builds that link to ucrt I haven't sent a cleanup in this area because I thought my changes were too specific to gcc/Windows, and wouldn't help much with HotSpot in general. I've learnt from my mistakes in the past where I caused reviewers pain in reviewing my admittedly selfish changes to HotSpot :( That said, if it is requested of me, I can commit some cleanups to this file. What do you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1548833671
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
On Thu, 28 Mar 2024 07:36:04 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 four additional > commits since the last revision: > > - Labels to empty line in awt_Window.cpp > - Labels to empty line in awt_Window.cpp > - Label to empty line in awt_Window.cpp > - Label to empty line in awt_Window.cpp Bumping - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2029552898
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v49]
On Sun, 21 Jan 2024 19:50:16 GMT, Phil Race wrote: >> Fixed the formatting (at least in the marked cases), but am unsure what you >> mean by set directly? > >> Fixed the formatting (at least in the marked cases), but am unsure what you >> mean by set directly? > > See my comment > "like in my recoded case above, we no longer need the "pData" var which was > there only because that name > is magically known to the macro." > > so skip (and get rid of) the pData var and just set the target var directly I've switched the marked cases to use direct setting and bypass pData, should I do the unmarked ones too? @prrace - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2024592358
Re: RFR: JDK-8329257: AIX: Switch HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc
On Thu, 28 Mar 2024 16:50:20 GMT, Joachim Kern wrote: > As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building > the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect > clang by another name, and it uses the clang toolchain in the JDK build. Thus > the old xlc toolchain was removed by > [JDK-8327701](https://bugs.openjdk.org/browse/JDK-8327701). > Now we also switch the HOTSPOT_TOOLCHAIN_TYPE from xlc to gcc, removing the > last xlc rudiment. > This means merging the AIX specific content of > utilities/globalDefinitions_xlc.hpp and utilities/compilerWarnings_xlc.hpp > into the corresponding gcc files on the on side and removing the > defined(TARGET_COMPILER_xlc) blocks in the code, because the > defined(TARGET_COMPILER_gcc) blocks work out of the box for the new AIX > compiler. > The rest of the changes are needed because of using > utilities/compilerWarnings_xlc.hpp the compiler is much more nagging about > ill formatted printf That one singular build change looks good :) > The rest of the changes are needed because of using > utilities/compilerWarnings_xlc.hpp the compiler is much more nagging about > ill formatted printf Did you mean compilerWarnings_gcc.hpp? - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18536#pullrequestreview-1967860264 PR Comment: https://git.openjdk.org/jdk/pull/18536#issuecomment-2026674128
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v70]
> 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 four additional commits since the last revision: - Labels to empty line in awt_Window.cpp - Labels to empty line in awt_Window.cpp - Label to empty line in awt_Window.cpp - Label to empty line in awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/ee0297a6..04e3cce1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=69 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=68-69 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 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 [v69]
> 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 awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/45de8955..ee0297a6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=68 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=67-68 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: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v68]
> 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: - Label to empty line in awt_Component.cpp - Label to empty line in awt_Canvas.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/6027b1df..45de8955 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=67 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=66-67 Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 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 [v67]
> 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_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/360894b5..6027b1df Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=66 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=65-66 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 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 [v66]
> 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_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/c4f4e54b..360894b5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=65 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=64-65 Stats: 8 lines in 1 file changed: 0 ins; 2 del; 6 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 [v65]
> 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_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/8f4d9e95..c4f4e54b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=64 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=63-64 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 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 [v64]
> 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_Frame.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/da0c09b1..8f4d9e95 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=63 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=62-63 Stats: 7 lines in 1 file changed: 0 ins; 3 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 [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=15096=62 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=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=15096=61 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=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: 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=15096=60 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=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=15096=59 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=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=15096=58 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=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=15096=57 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=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: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v57]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/bf12c71a..0c409d45 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=56 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=55-56 Stats: 3 lines in 1 file changed: 2 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: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Mon, 25 Mar 2024 09:02:22 GMT, Magnus Ihse Bursie wrote:
> > The only thing I'm uncertain about is the pData local, which I don't see
> > much benefit in removing since the null check associated with it still has
> > to remain for code semantics to be correct
>
> The point is that you can do the null check on the correct variable directly,
> instead of going a detour with pData. So instead of:
>
> ```
> RealType realVal;
> void* pData = getVal()
> if (pData == null) {
> // bail out
> }
> realVal = (RealType) pData;
> ```
>
> you can just do:
>
> ```
> RealType realVal = getVal();
> if (realVal == null) {
> // bail out
> }
> ```
>
> as the code normally would have been written, had there not been an old macro
> that used the "magic" temporary pData variable.
>
> This is a recurring pattern in this patch and needs to be fixed everywhere.
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. Maybe Phil can suggest an alternative to that, because I don't
know what that might be. Although, if given the choice I'd use RAII through
unique_ptr like Thomas suggested, it just seems so much cleaner to me. Speaking
of, hopefully this causes enough noise to get Phil's attention again. In the
meantime, I've reverted as much of the formatting changes I could possibly find
-
PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2019140763
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v56]
> 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: - Partially revert formatting in AccessBridgeJavaEntryPoints.cpp - Partially revert formatting in AccessBridgeJavaEntryPoints.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/dfc3c491..bf12c71a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=55 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=54-55 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 [v55]
> 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 awt_Frame.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/1dbd5f7e..dfc3c491 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=54 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=53-54 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: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v54]
> We should set the -permissive- flag for the Microsoft Visual C compiler, as > was requested by the now backed out > [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes > the Visual C compiler much less accepting of ill formed code, which will > improve code quality on Windows in the future. Julian Waters has updated the pull request incrementally with two additional commits since the last revision: - Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp Co-authored-by: Magnus Ihse Bursie - Revert formatting in src/java.desktop/windows/native/libawt/windows/awt_Frame.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/663a7f34..1dbd5f7e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=53 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=52-53 Stats: 4 lines in 2 files changed: 2 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 Mon, 25 Mar 2024 08:59:08 GMT, Magnus Ihse Bursie wrote: >> I don't think I can commit this as there are 3 backticks at the end there :P > > Apologies. The point was that this was formatting changes that were not > needed and should be reverted. I know, I did want to commit your change directly though, just can't because of the backticks :P - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1538377635
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Fri, 22 Mar 2024 12:27:31 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Revert Formatting in awt_Component.cpp >> - Revert Formatting in awt_Window.cpp > > src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp line 1230: > >> 1228: jdouble imgY = (jdouble) ((yPixelOrg * 72)/(jdouble) yPixelRes); >> 1229: jdouble imgWid = (jdouble) ((imgPixelWid * 72)/(jdouble) >> xPixelRes); >> 1230: jdouble imgHgt = (jdouble) ((imgPixelHgt * 72)/(jdouble) >> yPixelRes); > > Suggestion: > > jdouble imgX = (jdouble)((xPixelOrg * 72)/(jdouble)xPixelRes); > jdouble imgY = (jdouble)((yPixelOrg * 72)/(jdouble)yPixelRes); > jdouble imgWid = (jdouble)((imgPixelWid * 72)/(jdouble)xPixelRes); > jdouble imgHgt = (jdouble)((imgPixelHgt * 72)/(jdouble)yPixelRes);``` I don't think I can commit this as there are 3 backticks at the end there :P - PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1537089274
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
On Wed, 20 Mar 2024 06:38:59 GMT, Julian Waters wrote: >> We should set the -permissive- flag for the Microsoft Visual C compiler, as >> was requested by the now backed out >> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes >> the Visual C compiler much less accepting of ill formed code, which will >> improve code quality on Windows in the future. > > Julian Waters has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert Formatting in awt_Component.cpp > - Revert Formatting in awt_Window.cpp Thanks for the clarifications! Makes my job a lot easier. The only thing I'm uncertain about is the pData local, which I don't see much benefit in removing since the null check associated with it still has to remain for code semantics to be correct - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2017282826
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v53]
> 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_Window.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/0439b138..663a7f34 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=52 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=51-52 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: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 11:38:25 GMT, Magnus Ihse Bursie wrote: > I can give a spoiler to what the upcoming JDK_LIBS rewrite will do. > > Currently, if you want to link with e.g. `jli`, this is what you need to do: > > ``` > $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ > NAME := mylib, \ > EXTRA_HEADER_DIRS := java.base:libjli, \ > LDFLAGS_linux := -L$(call FindLibDirForModule, java.base), \ > LDFLAGS_macosx := -L$(call FindLibDirForModule, java.base), \ > JDK_LIBS_linux := -ljli, \ > JDK_LIBS_macosx := -ljli, \ > JDK_LIBS_windows := $(SUPPORT_OUTPUTDIR)/native/java.base/libjli/jli.lib, > \ > )) > > $(BUILD_LIBMYLIB): $(call FindLib, java.base, jli) > ``` > > This is cumbersome, and easy to forget (we almost never get it 100% right in > any place...). > > I intend to replace it with the following, and to have the build system > generate the boiler plate automatically: > > ``` > $(eval $(call SetupJdkLibrary, BUILD_LIBMYLIB, \ > NAME := mylib, \ > JDK_LIBS := jli, \ > )) > ``` I see, that's a definite +1 for me - PR Comment: https://git.openjdk.org/jdk/pull/18430#issuecomment-2014905990
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Mon, 18 Mar 2024 15:55:15 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 84 commits: >> >> - Merge branch 'openjdk:master' into patch-10 >> - awt_Window.cpp >> - awt_Frame.cpp >> - awt_Component.cpp >> - awt_Canvas.cpp >> - Merge branch 'openjdk:master' into patch-10 >> - Merge branch 'openjdk:master' into patch-10 >> - Fix awt_Window.cpp >> - Fix awt_PrintJob.cpp >> - -Zc:stringStrings no longer needed with -permissive- flags-cflags.m4 >> - ... and 74 more: https://git.openjdk.org/jdk/compare/a474b372...0f34608b > > bot-keep-alive > > @TheShermanTanker Did you understand the remaining changes that Phil has > requested? Do you think you'll be able to fix this? @magicus Phil doesn't seem to be responding to my queries, I'm not too sure what to do at this point - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2014826062
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation make/common/JdkNativeCompilation.gmk line 190: > 188: $1_SRC_HEADER_FLAGS += $$(addprefix -I, $$(call GetJavaHeaderDir, > $$(MODULE))) > 189: > 190: $1_JDK_LIBS += $$($1_JDK_LIBS_$$(OPENJDK_TARGET_OS)) Should these follow LIBS and pick up more specific target options as well? Not a request, just thinking out loud - PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535391514
Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]
On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie wrote: >> This is the first step of several, in which I will clean up the native >> compilation code as used by modules. In this first step `java.base`, >> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since >> they require more work. The changes in the remaining modules are trivial by >> comparison. >> >> The changes done here are: >> >> 1) A new argument `JDK_LIB` has been introduced, that is used for linking >> with other libraries produced by the JDK build. As a follow-up, this will be >> further cleaned up and generalized, but the goal for this change is just to >> separate them out from external libraries. >> >> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in >> alphabetical order. Note that this change will affect the resulting binaries >> (since the order libraries are given are stored in the binary), but this >> change should only be superficial. (If we have symbol clashes between >> libraries, then we have problems on a whole other level...). >> >> 3) The code has been checked for inconsistencies and style guide errors, and >> a common programming style has been applied to all `Lib.gmk` and >> `Launcher.gmk` files, making sure that all parts follow best practices. >> >> This PR will be followed up by invidual PRs for the modules requiring not >> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and >> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across >> platforms, and automatically apply proper dependencies. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix indentation I had doubts about introducing JDK_LIB until I saw there were plans to further improve upon it, so I guess my uncertainty is now answered make/autoconf/libraries.m4 line 174: > 172: > 173: JDKLIB_LIBS="$BASIC_JDKLIB_LIBS" > 174: JDKEXE_LIBS="" This is passed to LauncherCommon while JDKLIB_LIBS is manually passed to every individual library it's used for. Why not unify them and pass JDKLIB_LIBS to LibCommon instead? Other than that seems good - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18430#pullrequestreview-1954491578 PR Review Comment: https://git.openjdk.org/jdk/pull/18430#discussion_r1535387243
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v49]
On Sun, 21 Jan 2024 19:50:16 GMT, Phil Race wrote: >> Fixed the formatting (at least in the marked cases), but am unsure what you >> mean by set directly? > >> Fixed the formatting (at least in the marked cases), but am unsure what you >> mean by set directly? > > See my comment > "like in my recoded case above, we no longer need the "pData" var which was > there only because that name > is magically known to the macro." > > so skip (and get rid of) the pData var and just set the target var directly @prrace Sorry for the page, but as I mentioned above I don't see much of an advantage to removing pData, since it appears we still have to do the null check anyway even without it. I'm also not sure what the incorrect code you mentioned above is - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2014317099
Re: RFR: 8328628: JDK-8328157 incorrectly sets -MT on all compilers in jdk.jpackage
On Wed, 20 Mar 2024 16:20:36 GMT, Magnus Ihse Bursie wrote: > In [JDK-8328157](https://bugs.openjdk.org/browse/JDK-8328157), I rewrote the > logic to replace -MD with -MT for compiling on Windows. Unfortunately, I > mistakenly set -MT for all compilers, not just for visual studio. Looks good. Unrelated but man the newly refactored build system takes some getting used to - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18401#pullrequestreview-1949478371
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v50]
On Mon, 18 Mar 2024 15:55:15 GMT, Magnus Ihse Bursie wrote: > bot-keep-alive > > @TheShermanTanker Did you understand the remaining changes that Phil has > requested? Do you think you'll be able to fix this? Hi Magnus, yes I do plan on fixing this. I've just been a bit busy and tired is all, and also to be honest I don't quite understand some of Phil's reviews, which is why I'm asking for clarification on some of them first - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-2008743382
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:22:50 GMT, Julian Waters wrote:
>> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6366:
>>
>>> 6364: jobject parent = data->parentComp;
>>> 6365:
>>> 6366: AwtComponent *awtComponent = nullptr;
>>
>> Looking at it (not tested) here through 6403 could be simplified as
>>
>> if (self == NULL || parent == NULL) {
>> env->ExceptionClear();
>> JNU_ThrowNullPointerException(env, "peer");
>> env->DeleteGlobalRef(self);
>> env->DeleteGlobalRef(parent);
>> delete data;
>> return;
>> }
>>
>> AwtComponent *awtComponent = (AwtComponent *)JNI_GET_PDATA(self);
>> if (awtComponent == NULL) {
>> THROW_NULL_PDATA_IF_NOT_DESTROYED(self);
>> env->DeleteGlobalRef(self);
>> env->DeleteGlobalRef(parent);
>> delete data;
>> return;
>> }
>>
>> AwtComponent *awtParent = (AwtComponent *)JNI_GET_PDATA(parent);
>> if (awtParent == NULL) {
>> THROW_NULL_PDATA_IF_NOT_DESTROYED(parent);
>> env->DeleteGlobalRef(self);
>> env->DeleteGlobalRef(parent);
>> delete data;
>> return;
>> }
>>
>>
>> I think I prefer that over
>> BOOL error = FALSE;
>> AwtComponent *awtComponent = nullptr;
>> AwtComponent *awtParent = nullptr;
>>
>> if (self == NULL || parent == NULL) {
>> env->ExceptionClear();
>> JNU_ThrowNullPointerException(env, "peer");
>> error = TRUE;
>> }
>>
>> if (!error) {
>> awtComponent = (AwtComponent *)JNI_GET_PDATA(self);
>> if (awtComponent == NULL) {
>> THROW_NULL_PDATA_IF_NOT_DESTROYED(self);
>> error = TRUE;
>> }
>> }
>>
>> if (!error) {
>> awtParent = (AwtComponent *)JNI_GET_PDATA(parent);
>> if (awtParent == NULL) {
>> THROW_NULL_PDATA_IF_NOT_DESTROYED(parent);
>> error = TRUE;
>> }
>>
>> if (error) {
>> env->DeleteGlobalRef(self);
>> env->DeleteGlobalRef(parent);
>> delete data;
>> return;
>> }
>
> 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
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1531567293
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v52]
> We should set the -permissive- flag for the Microsoft Visual C compiler, as > was requested by the now backed out > [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes > the Visual C compiler much less accepting of ill formed code, which will > improve code quality on Windows in the future. Julian Waters has updated the pull request incrementally with two additional commits since the last revision: - Revert Formatting in awt_Component.cpp - Revert Formatting in awt_Window.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/18014c3a..0439b138 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=51 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=50-51 Stats: 6 lines in 2 files changed: 2 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 [v51]
> 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 awt_PrintJob.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/15096/files - new: https://git.openjdk.org/jdk/pull/15096/files/0f34608b..18014c3a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=15096=50 - incr: https://webrevs.openjdk.org/?repo=jdk=15096=49-50 Stats: 2 lines in 1 file changed: 1 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: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Sat, 20 Jan 2024 00:17:02 GMT, Phil Race wrote:
>> Julian Waters has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 79 commits:
>>
>> - Merge branch 'openjdk:master' into patch-10
>> - Merge branch 'openjdk:master' into patch-10
>> - Fix awt_Window.cpp
>> - Fix awt_PrintJob.cpp
>> - -Zc:stringStrings no longer needed with -permissive- flags-cflags.m4
>> - Fix awt_Window.cpp
>> - awt_Window.cpp
>> - awt_PrintJob.cpp
>> - awt_Frame.cpp
>> - Whitespace awt_Component.cpp
>> - ... and 69 more: https://git.openjdk.org/jdk/compare/35e96627...cbfbaee6
>
> src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6366:
>
>> 6364: jobject parent = data->parentComp;
>> 6365:
>> 6366: AwtComponent *awtComponent = nullptr;
>
> Looking at it (not tested) here through 6403 could be simplified as
>
> if (self == NULL || parent == NULL) {
> env->ExceptionClear();
> JNU_ThrowNullPointerException(env, "peer");
> env->DeleteGlobalRef(self);
> env->DeleteGlobalRef(parent);
> delete data;
> return;
> }
>
> AwtComponent *awtComponent = (AwtComponent *)JNI_GET_PDATA(self);
> if (awtComponent == NULL) {
> THROW_NULL_PDATA_IF_NOT_DESTROYED(self);
> env->DeleteGlobalRef(self);
> env->DeleteGlobalRef(parent);
> delete data;
> return;
> }
>
> AwtComponent *awtParent = (AwtComponent *)JNI_GET_PDATA(parent);
> if (awtParent == NULL) {
> THROW_NULL_PDATA_IF_NOT_DESTROYED(parent);
> env->DeleteGlobalRef(self);
> env->DeleteGlobalRef(parent);
> delete data;
> return;
> }
>
>
> I think I prefer that over
> BOOL error = FALSE;
> AwtComponent *awtComponent = nullptr;
> AwtComponent *awtParent = nullptr;
>
> if (self == NULL || parent == NULL) {
> env->ExceptionClear();
> JNU_ThrowNullPointerException(env, "peer");
> error = TRUE;
> }
>
> if (!error) {
> awtComponent = (AwtComponent *)JNI_GET_PDATA(self);
> if (awtComponent == NULL) {
> THROW_NULL_PDATA_IF_NOT_DESTROYED(self);
> error = TRUE;
> }
> }
>
> if (!error) {
> awtParent = (AwtComponent *)JNI_GET_PDATA(parent);
> if (awtParent == NULL) {
> THROW_NULL_PDATA_IF_NOT_DESTROYED(parent);
> error = TRUE;
> }
>
> if (error) {
> env->DeleteGlobalRef(self);
> env->DeleteGlobalRef(parent);
> delete data;
> return;
> }
Sorry, I don't see a BOOL error anywhere?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15096#discussion_r1531553778
Re: RFR: 8314488: Compile the JDK as C++17 [v7]
> Compile the JDK as C++17, enabling the use of all C++17 language features Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge branch 'master' into patch-7 - Require clang 13 in toolchain.m4 - Remove unnecessary -std=c++17 option in Lib.gmk - Merge branch 'openjdk:master' into patch-7 - Compiler versions in toolchain.m4 - Merge branch 'openjdk:master' into patch-7 - Merge branch 'openjdk:master' into patch-7 - Revert vm_version_linux_riscv.cpp - vm_version_linux_riscv.cpp - allocation.cpp - ... and 1 more: https://git.openjdk.org/jdk/compare/269163d5...9286a964 - Changes: https://git.openjdk.org/jdk/pull/14988/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14988=06 Stats: 4 lines in 2 files changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/14988.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14988/head:pull/14988 PR: https://git.openjdk.org/jdk/pull/14988
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v5]
On Sat, 16 Mar 2024 22:20:53 GMT, Julian Waters wrote: >> Eclipse Shared Workspaces share the same directory as the JDK itself, which >> does not make much sense since there can be multiple different >> configurations of the JDK which Eclipse does depend on (as minimalistic as >> it is), and there is no one true JDK configuration, unlike what this setting >> suggests. This feature was created when I was much less familiar with the >> structure of how the JDK handles different builds, and should be removed > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Remove SHARED definition in Main.gmk Thanks Erik and Magnus for the reviews - PR Comment: https://git.openjdk.org/jdk/pull/18046#issuecomment-2005907236
Integrated: 8326964: Remove Eclipse Shared Workspaces
On Wed, 28 Feb 2024 15:07:14 GMT, Julian Waters wrote: > Eclipse Shared Workspaces share the same directory as the JDK itself, which > does not make much sense since there can be multiple different configurations > of the JDK which Eclipse does depend on (as minimalistic as it is), and there > is no one true JDK configuration, unlike what this setting suggests. This > feature was created when I was much less familiar with the structure of how > the JDK handles different builds, and should be removed This pull request has now been integrated. Changeset: 4ef591f7 Author:Julian Waters URL: https://git.openjdk.org/jdk/commit/4ef591f71f62ee6ea8a603ed7a3e568b348b2c16 Stats: 120 lines in 5 files changed: 1 ins; 75 del; 44 mod 8326964: Remove Eclipse Shared Workspaces Reviewed-by: erikj, ihse - PR: https://git.openjdk.org/jdk/pull/18046
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v5]
On Mon, 18 Mar 2024 10:06:26 GMT, Magnus Ihse Bursie wrote: >> Side tangent: How are the Workspaces for the other IDEs set up? Are they >> contained entirely in the ide subdirectory in the build directory or do they >> end up leaving their metadata in the top level directory like the Shared >> Workspaces used to do? I plan to improve Eclipse support over time, and >> these details might potentially be important to that effort > > Ok, it's fine for integration then. > > As for other IDEs: there is very little coordination on this; most IDE > integrations are like barely kept alive by some enthusiast. I would really > love to see some better grip on this, unifying IDE support and making it more > of a first-class citizen of the build, but this has unfortunately kept being > pushed down the prio list. :-( > > If you feel so inclined, please try out the different IDEs supported. The > very first step, I believe, is to have proper documentation in docs/ide.md. > Even if we have a process that requires several manual steps, it is better to > have it formalized and documented in a single place. Then it can form the > basis for a more automated approach. Sorry I think I may not understand what you mean by unifying IDE support "and making it more of a first class citizen of the build", do you mean a central Setup IDE function in make of some sort, much like SetupNativeCompilation? I don't see how that might be possible given how each IDE has wildly varying requirements and save data formats - PR Review Comment: https://git.openjdk.org/jdk/pull/18046#discussion_r1528469422
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v5]
On Mon, 18 Mar 2024 09:18:40 GMT, Julian Waters wrote: >> .gitignore line 22: >> >>> 20: /.settings/ >>> 21: /.project >>> 22: /.classpath >> >> `.project` is repeated so that is fine to remove, but why are you removing >> the other two? `.cproject` sounds like it could come from CDT. > > With the Shared Workspaces removed, the Workspace created no longer exists in > the JDK's top level source directory. This means the .gitignore entries are > now obsolete, and can be removed, since they only ignore .cproject and > .classpath in the top level source directory. They were added way back when I > introduced support for Eclipse, and was less familiar with how multiple > different JDK builds are contained Side tangent: How are the Workspaces for the other IDEs set up? Are they contained entirely in the ide subdirectory in the build directory or do they end up leaving their metadata in the top level directory like the Shared Workspaces used to do? I plan to improve Eclipse support over time, and these details might potentially be important to that effort - PR Review Comment: https://git.openjdk.org/jdk/pull/18046#discussion_r1528161453
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v5]
On Mon, 18 Mar 2024 09:07:26 GMT, Magnus Ihse Bursie wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove SHARED definition in Main.gmk > > .gitignore line 22: > >> 20: /.settings/ >> 21: /.project >> 22: /.classpath > > `.project` is repeated so that is fine to remove, but why are you removing > the other two? `.cproject` sounds like it could come from CDT. With the Shared Workspaces removed, the Workspace created no longer exists in the JDK's top level source directory. This means the .gitignore entries are now obsolete, and can be removed, since they only ignore .cproject and .classpath in the top level source directory. They were added way back when I introduced support for Eclipse, and was less familiar with how multiple different JDK builds are contained - PR Review Comment: https://git.openjdk.org/jdk/pull/18046#discussion_r1528154027
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v4]
On Fri, 15 Mar 2024 15:44:48 GMT, Erik Joelsson wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Address review comments in CreateWorkspace.gmk > > Marked as reviewed by erikj (Reviewer). @erikj79 Sorry it seems I forgot to remove the SHARED definition from the Makefile, could I get a re-review? Thanks @magicus Also waiting for your approval - PR Comment: https://git.openjdk.org/jdk/pull/18046#issuecomment-2001849914
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v5]
> Eclipse Shared Workspaces share the same directory as the JDK itself, which > does not make much sense since there can be multiple different configurations > of the JDK which Eclipse does depend on (as minimalistic as it is), and there > is no one true JDK configuration, unlike what this setting suggests. This > feature was created when I was much less familiar with the structure of how > the JDK handles different builds, and should be removed Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Remove SHARED definition in Main.gmk - Changes: - all: https://git.openjdk.org/jdk/pull/18046/files - new: https://git.openjdk.org/jdk/pull/18046/files/212fc3c1..4287082a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18046=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18046=03-04 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18046.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18046/head:pull/18046 PR: https://git.openjdk.org/jdk/pull/18046
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v3]
On Fri, 15 Mar 2024 12:42:17 GMT, Erik Joelsson wrote: >> Julian Waters 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 five additional >> commits since the last revision: >> >> - Merge branch 'openjdk:master' into patch-15 >> - Refresh documentation >> - Remove Shared Workspaces entirely >> - Remove shared targets >> - 8326964 > > make/ide/eclipse/CreateWorkspace.gmk line 192: > >> 190: # make dist-clean omitted since it would delete the Workspace as >> well >> 191: # $1_PLAIN_MAKE_TARGETS += reconfigure >> 192: # $1_PLAIN_MAKE_TARGETS += dist-clean > > Why leave these additions commented out and not just remove them? If you > think the comment is important, you can just put it above the > PLAIN_MAKE_TARGETS definition. Fair point, I'll address this now - PR Review Comment: https://git.openjdk.org/jdk/pull/18046#discussion_r1526271085
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v4]
> Eclipse Shared Workspaces share the same directory as the JDK itself, which > does not make much sense since there can be multiple different configurations > of the JDK which Eclipse does depend on (as minimalistic as it is), and there > is no one true JDK configuration, unlike what this setting suggests. This > feature was created when I was much less familiar with the structure of how > the JDK handles different builds, and should be removed Julian Waters has updated the pull request incrementally with one additional commit since the last revision: Address review comments in CreateWorkspace.gmk - Changes: - all: https://git.openjdk.org/jdk/pull/18046/files - new: https://git.openjdk.org/jdk/pull/18046/files/46dee2b7..212fc3c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18046=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18046=02-03 Stats: 7 lines in 1 file changed: 2 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18046.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18046/head:pull/18046 PR: https://git.openjdk.org/jdk/pull/18046
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v2]
On Wed, 28 Feb 2024 15:19:58 GMT, Julian Waters wrote: >> Eclipse Shared Workspaces share the same directory as the JDK itself, which >> does not make much sense since there can be multiple different >> configurations of the JDK which Eclipse does depend on (as minimalistic as >> it is), and there is no one true JDK configuration, unlike what this setting >> suggests. This feature was created when I was much less familiar with the >> structure of how the JDK handles different builds, and should be removed > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Remove shared targets This should be ready for review now - PR Comment: https://git.openjdk.org/jdk/pull/18046#issuecomment-1999062238
Re: RFR: 8326964: Remove Eclipse Shared Workspaces [v3]
> Eclipse Shared Workspaces share the same directory as the JDK itself, which > does not make much sense since there can be multiple different configurations > of the JDK which Eclipse does depend on (as minimalistic as it is), and there > is no one true JDK configuration, unlike what this setting suggests. This > feature was created when I was much less familiar with the structure of how > the JDK handles different builds, and should be removed Julian Waters 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 five additional commits since the last revision: - Merge branch 'openjdk:master' into patch-15 - Refresh documentation - Remove Shared Workspaces entirely - Remove shared targets - 8326964 - Changes: - all: https://git.openjdk.org/jdk/pull/18046/files - new: https://git.openjdk.org/jdk/pull/18046/files/0d29a355..46dee2b7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18046=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18046=01-02 Stats: 101532 lines in 2067 files changed: 16945 ins; 79335 del; 5252 mod Patch: https://git.openjdk.org/jdk/pull/18046.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18046/head:pull/18046 PR: https://git.openjdk.org/jdk/pull/18046
Compiling hsdis on Windows under MSYS2
Hi all, I'm currently working on allowing hsdis to compile under different Unix environments for Windows with gcc as the compiler, since it's required for the binutils backend (For good measure I'll throw capstone support in as well since it's relatively easy to add and it's also a package under the different MSYS2 subsystems. LLVM is way more complex and I'll leave it out for the time being). This means that Cygwin will no longer be the only way to compile hsdis for Windows, especially for the binutils backend, which I view as a big plus in terms of ease of setup. However, I have a couple of questions: 1. Right now I see a hack in the Makefiles to replace the Visual C++ compiler entirely with gcc when compiling the binutils. It's slightly better now after a recent change, but it's still there. Should this be replaced with proper-but-partial gcc support for Windows in autoconf and make, and instead require configuring with gcc if hsdis compilation is requested? If the hard requirement of configuring with gcc is a no go, is at least adding partial gcc support in configure and make still ok to add so hsdis can be configured to compile with gcc directly? 2. Currently there's a SYSTEM_BINUTILS define which gates the definitions of PACKAGE and PACKAGE_VERSION. It seems that the bfd.h header requires these to be defined, but strangely these are not defined if a system binutils is used on Linux. Does the bfd.h header there not require these to be defined? It causes compilation failures here on Windows when --with-binutils=system is passed (I am aware that --with-binutils=system is currently only supported on Linux, but we should make it supported under MSYS2 and other Windows environments that support system binutils as well) best regards, Julian
Re: RFR: 8328079: JDK-8326583 broke ccache compilation
On Wed, 13 Mar 2024 12:50:17 GMT, Magnus Ihse Bursie wrote: > According to > https://github.com/openjdk/jdk/pull/17986#issuecomment-1975396416, > [JDK-8326583](https://bugs.openjdk.org/browse/JDK-8326583) broke ccache > compilation. > > The reason was that the ccache command line included > `CCACHE_SLOPPINESS=pch_defines,time_macros`, and the comma was expanded > wrongly, causing `time_macros ...` to look as an additional argument to > `SetIfEmpty`. Marked as reviewed by jwaters (Committer). Looks good, sorry I couldn't do this one before you did - PR Review: https://git.openjdk.org/jdk/pull/18273#pullrequestreview-1934257071 PR Comment: https://git.openjdk.org/jdk/pull/18273#issuecomment-1994472575
Re: RFR: 8328074: Add jcheck whitespace checking for assembly files
On Wed, 13 Mar 2024 11:18:20 GMT, Magnus Ihse Bursie wrote: > As part of the ongoing effort to enable jcheck whitespace checking to all > text files, it is now time to address assembly (.S) files. The hotspot > assembly files were fixed as part of the hotspot mapfile removal, so only a > single incorrect jdk library remains. > > The main issue with `libjsvml` was inconsistent line starts, sometimes using > tabs. I used the `expand` unix command line tool to replace these with spaces. src/jdk.incubator.vector/linux/native/libjsvml/jsvml_d_acos_linux_x86.S line 37: > 35: .text > 36: # mark_begin; > 37:.align16,0x90 .align seems to ironically not be aligned with the rest of the directives - PR Review Comment: https://git.openjdk.org/jdk/pull/18268#discussion_r1523048988
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Mon, 12 Feb 2024 14:09:49 GMT, Julian Waters wrote: >> I have verified that this works fine in the Oracle internal CI. >> >> Erik's point still stands: >>> I still think it would be prudent to verify this patch with all the >>> currently accepted versions of autoconf (2.69, 2.70, 2.71, 2.72). >> >> I think the easiest way to achieve this is to checkout the autoconf at these >> versions, and build it yourself. Iirc it was quite easy to build autoconf >> (anything else would be a shame and very bad PR for the project! :-o). >> >> I'm hoping you are willing to do this, since I believe this is a superior >> solution and I'd like to see it in mainline. (Otherwise, let me know and >> I'll try to squeeze in doing it.) > >> I have verified that this works fine in the Oracle internal CI. >> >> Erik's point still stands: >> >> > I still think it would be prudent to verify this patch with all the >> > currently accepted versions of autoconf (2.69, 2.70, 2.71, 2.72). >> >> I think the easiest way to achieve this is to checkout the autoconf at these >> versions, and build it yourself. Iirc it was quite easy to build autoconf >> (anything else would be a shame and very bad PR for the project! :-o). >> >> I'm hoping you are willing to do this, since I believe this is a superior >> solution and I'd like to see it in mainline. (Otherwise, let me know and >> I'll try to squeeze in doing it.) > > It's actually even easier to do on Windows than it might appear, since > MinGW's pacman system has caches that contain past package versions. I > wouldn't have to build it at all in fact, all I'd have to do is downgrade my > autoconf to past versions (I think I'm currently on 2.71). I know you're > probably very busy right now, so I'll spare you the chore of having to do > that by testing it on my end. That said, should I test autoconf on all > platforms too? That aside, I'm still a little unhappy that there is no formal > way to unregister an AC_DEFUN macro though :( > > By the way, I've left you something in the mail. Hope you have some time to > check it out! > @TheShermanTanker I think this is essentially done. All that needs are the > testing. Can you do it or do you want help with it? I'm able to do so myself, I've just been busy with university so far. I've only tested 2.71 and 2.72 though, 2.69 and 2.70 might still need testing. I'm still unhappy that I can't properly undefine the macros, as a side tangent - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1991874882
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Mon, 11 Mar 2024 08:38:53 GMT, Joachim Kern wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Revert SEARCH_PATH changes > > doc/building.html line 679: > >> 677: IBM Open XL C/C++ >> 678: The minimum accepted version of Open XL is 17.1.1.4. This is in >> 679: essence clang 13, and will be treated as such by the OpenJDK build > > It is clang 15 not 13. Clang 13 was in 17.1.0 Is Open XL C/C++ considered a compiler or more akin to a development environment like Xcode is for macOS? Depending on which, we could just say clang is the compiler for AIX without needing to say that Open XL is treated like clang, etc Also, why did this remove the link to the Supported Build Platforms page? - PR Review Comment: https://git.openjdk.org/jdk/pull/18172#discussion_r1519394251
Re: RFR: 8327045: Consolidate -fvisibility=hidden as a basic flag for all compilation
On Thu, 29 Feb 2024 12:23:37 GMT, Magnus Ihse Bursie wrote: > After we removed mapfiles, we can setup -fvisibility=hidden (and > -Wl,--exclude-libs,ALL) in the most basic flags, so this applies to all > compilation. > > This will remove duplicate code and make the underlying assumptions of the > build clearer. > > Doing this will result in the same output result -- with one exception: > native test libraries has not been compiled with this flag (and this caused > an error for a Oracle-internal test). Marked as reviewed by jwaters (Committer). make/autoconf/flags-ldflags.m4 line 85: > 83: > 84: if test "x$OPENJDK_TARGET_OS" = xlinux; then > 85: BASIC_LDFLAGS="-Wl,--exclude-libs,ALL" Duplicated code? It already adds -Wl,--exclude-libs,ALL above if the compiler is gcc - PR Review: https://git.openjdk.org/jdk/pull/18061#pullrequestreview-1908903139 PR Review Comment: https://git.openjdk.org/jdk/pull/18061#discussion_r1507599902
Re: RFR: 8327701: Remove the xlc toolchain [v2]
On Fri, 8 Mar 2024 15:48:08 GMT, Magnus Ihse Bursie wrote: >> As of [JDK-8325880](https://bugs.openjdk.org/browse/JDK-8325880), building >> the JDK requires version 17 of IBM Open XL C/C++ (xlc). This is in effect >> clang by another name, and it uses the clang toolchain in the JDK build. >> Thus the old xlc toolchain is no longer supported, and should be removed. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Revert SEARCH_PATH changes Build changes look ok, but I'm not an AIX developer - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/18172#pullrequestreview-1925243852
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Thu, 7 Mar 2024 19:53:35 GMT, Andrew John Hughes wrote: > Also, it might be worth repeating one of my long-standing wishes: that the > version string should not be hard-coded into the build, but e.g. stored as a > string in the `release` file, and read from there. If we did that, the cost > of changing the version string would be negligible, and we wouldn't need to > worry as much about it. It would also be simple to compare different builds > which end up with the same bits since they are built from the same sources, > but by different version flags (e.g. -ea vs GA). (In fact, we'd turn a -ea > build into a GA just by updating the version string, so we'd know for sure we > are publishing what we tested.) Why not store the version string inside HotSpot, and have it as the one source of truth for the version string so it doesn't need to be hardcoded in other places? A text file seems too easy to modify to set the version string to a rubbish value - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1985087876
Re: RFR: 8327389: Remove use of HOTSPOT_BUILD_USER
On Wed, 6 Mar 2024 11:57:27 GMT, Andrew John Hughes wrote: > The HotSpot code has inserted the value of `HOTSPOT_BUILD_USER` into the > internal version string since before the JDK was open-sourced, so the > reasoning for this is not in the public history. See > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/abstract_vm_version.cpp#L284 > > ~~~ > $ /usr/lib/jvm/java-21-openjdk/bin/java -Xinternalversion > OpenJDK 64-Bit Server VM (21.0.2+13) for linux-amd64 JRE (21.0.2+13), built > on 2024-01-19T16:39:52Z by "mockbuild" with gcc 8.5.0 20210514 (Red Hat > 8.5.0-20) > ~~~ > > It is not clear what value this provides. By default, the value is set to the > system username, but it can be set to a static value using > `--with-build-user`. > > In trying to compare builds, this is a source of difference if the builds are > produced under different usernames and `--with-build-user` is not set to the > same value. > > It may also be a source of information leakage from the host system. It is > not clear what value it provides to the end user of the build. > > This patch proposes removing it altogether, but at least knowing why it needs > to be there would be progress from where we are now. We could also consider a > middle ground of only setting it for "adhoc" builds, those without set > version information, as is the case with the overall version output. > > With this patch: > > ~~~ > $ ~/builder/23/images/jdk/bin/java -Xinternalversion > OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) for > linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on 2024-03-06T00:23:37Z > with gcc 13.2.1 20230826 > > $ ~/builder/23/images/jdk/bin/java -XX:ErrorHandlerTest=1 > > $ grep '^vm_info' > /localhome/andrew/projects/openjdk/upstream/jdk/hs_err_pid1119824.log > vm_info: OpenJDK 64-Bit Server VM (fastdebug 23-internal-adhoc.andrew.jdk) > for linux-amd64 JRE (23-internal-adhoc.andrew.jdk), built on > 2024-03-06T00:23:37Z with gcc 13.2.1 20230826 > ~~~ > > The above are from a fastdebug build but I also built a product build with no > issues. Leaving a comment here as a reminder for myself to come back to this, this intrigues me - PR Comment: https://git.openjdk.org/jdk/pull/18136#issuecomment-1980739523
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Mon, 4 Mar 2024 12:58:28 GMT, Claudio Nieder wrote: > > Could I trouble you to mention what exactly was different? > > No trouble at all. > > `CCACHE_BASEDIR=/tmp/bceaed6d/jdk/`is completely missing. (The directory is > where I checked out OpenJDK) > > `CCACHE_SLOPPINESS` has the value `pch_defines,time_macros` with the working > parent commit but just `pch_defines` with this commit. I.e. > `CCACHE_SLOPPINESS=pch_defines,time_macros` vs > `CCACHE_SLOPPINESS=pch_defines`. Thanks! Seems curious to me, off the top of my head I can't seem to discern why these would change, perhaps it's time for a little investigating - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976534197
