Re: RFR: 8326587: Separate out Microsoft toolchain linking
On Fri, 23 Feb 2024 22:05:44 GMT, Erik Joelsson wrote: > And then I think this will actually help Julian, since we'll push the > microsoft strangeness away in a separate file Thanks! Though I do first need to rebase on top of it and fix all the merge conflicts first... (Thinking about it, this likely means I have to excise some of the Microsoft linking logic out into the "regular" linking file since some of the Microsoft linking process is used by Windows linking in general, even with the gcc toolchain) Just a comment: Why not reuse the AR variable for lib rather than introduce an entirely new LIB variable? The logic treating lib.exe as ar is gone with this change, but that doesn't mean they have to be split into entirely different variables. LinkMicrosoft could just run $($1_AR) without treating it as ar. This saves one autoconf and Makefile variable - PR Comment: https://git.openjdk.org/jdk/pull/17987#issuecomment-1962272988
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution
On Fri, 23 Feb 2024 16:23:43 GMT, Magnus Ihse Bursie wrote: > The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. !!! Not to be a party pooper, but this seems like it removes pretty some useful functionality from the build system. What if this is needed down the line? On the motivation of this change, TOOLCHAIN_LINK_CXX is pretty clear that the linker to be used is g++ instead of gcc for instance, while the new LANG parameter makes it look like SetupNativeCompilation only accepts and compiles C++ files if C++ is passed, and only C files if the new default of C is passed (For anyone looking in on this Pull Request who isn't familiar with the build system, it can compile a mix of both without issue). I'm not particularly sure this is a good idea, since a lot of flexibility seems to be lost with this change. I don't seem to see any simplification to SetupNativeCompilation either, maybe I'm missing something? - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1962269640
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v43]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: Refactor most of TestMarkdown.java into separate tests, grouped by functionality - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/d460ee33..398f93fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=42 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=41-42 Stats: 2001 lines in 9 files changed: 1143 ins; 857 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: 8326587: Separate out Microsoft toolchain linking [v3]
On Fri, 23 Feb 2024 21:40:22 GMT, Magnus Ihse Bursie wrote: >> There is not much overlap on how linking is done on Windows on one hand, and >> on all Unix platforms on the other. This makes Link.gmk basically consists >> of two parts, each in it own half of if statements, and the few common parts >> are artificially shoehorned in to fit both sides. >> >> The code will be much clearer if we just split this into two different files. >> >> Note that this PR slightly collides with JDK-8326583 >> (https://github.com/openjdk/jdk/pull/17986). Whichever of this goes in first >> will mean that the other one needs to make some adaptations. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename Windows to Microsoft I think this looks good. There looks to be a conflict with https://github.com/openjdk/jdk/pull/17986, but I assume you are aware. make/autoconf/flags-other.m4 line 45: > 43: AC_DEFUN([FLAGS_SETUP_LIBFLAGS], > 44: [ > 45: # LIB is used to create static libraries on Windows indentation. - PR Review: https://git.openjdk.org/jdk/pull/17987#pullrequestreview-1899019687 PR Review Comment: https://git.openjdk.org/jdk/pull/17987#discussion_r1501222826
Re: RFR: 8326587: Separate out Microsoft toolchain linking
On Fri, 23 Feb 2024 21:31:09 GMT, Magnus Ihse Bursie wrote: > Ok, fair enough, I should have phrase this split as "Microsoft toolchain" and > "everything else". The point is that linking is vastly different on Windows > than on other platforms. Our efforts to force "lib" to behave as "ar" etc was > far-fetched to begin with. And to fully support static libraries, we need to > separate the static linking into several steps, running ld, objcopy and then > finally ar There is no correspondence to of this at all with the Microsoft > toolchain, and trying to shoehorn this in is just going to make matters worse. > > I'll rename the split according to toolchain and not platform. (And then I > think this will actually help Julian, since we'll push the microsoft > strangeness away in a separate file.) Sounds OK to you then? That sounds good to me, thanks! - PR Comment: https://git.openjdk.org/jdk/pull/17987#issuecomment-1962063986
Re: RFR: 8326587: Separate out Microsoft toolchain linking [v3]
> There is not much overlap on how linking is done on Windows on one hand, and > on all Unix platforms on the other. This makes Link.gmk basically consists of > two parts, each in it own half of if statements, and the few common parts are > artificially shoehorned in to fit both sides. > > The code will be much clearer if we just split this into two different files. > > Note that this PR slightly collides with JDK-8326583 > (https://github.com/openjdk/jdk/pull/17986). Whichever of this goes in first > will mean that the other one needs to make some adaptations. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Rename Windows to Microsoft - Changes: - all: https://git.openjdk.org/jdk/pull/17987/files - new: https://git.openjdk.org/jdk/pull/17987/files/27500808..afd1edd7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17987&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17987&range=01-02 Stats: 20 lines in 3 files changed: 0 ins; 0 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/17987.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17987/head:pull/17987 PR: https://git.openjdk.org/jdk/pull/17987
Re: RFR: 8326587: Separate out Microsoft toolchain linking
On Fri, 23 Feb 2024 16:24:48 GMT, Magnus Ihse Bursie wrote: > There is not much overlap on how linking is done on Windows on one hand, and > on all Unix platforms on the other. This makes Link.gmk basically consists of > two parts, each in it own half of if statements, and the few common parts are > artificially shoehorned in to fit both sides. > > The code will be much clearer if we just split this into two different files. > > Note that this PR slightly collides with JDK-8326583 > (https://github.com/openjdk/jdk/pull/17986). Whichever of this goes in first > will mean that the other one needs to make some adaptations. Ok, fair enough, I should have phrase this split as "Microsoft toolchain" and "everything else". The point is that linking is vastly different on Windows than on other platforms. Our efforts to force "lib" to behave as "ar" etc was far-fetched to begin with. And to fully support static libraries, we need to separate the static linking into several steps, running ld, objcopy and then finally ar There is no correspondence to of this at all with the Microsoft toolchain, and trying to shoehorn this in is just going to make matters worse. I'll rename the split according to toolchain and not platform. (And then I think this will actually help Julian, since we'll push the microsoft strangeness away in a separate file.) Sounds OK to you then? - PR Comment: https://git.openjdk.org/jdk/pull/17987#issuecomment-1962018364
Re: RFR: 8326587: Split Link.gmk into a Windows and Unix part
On Fri, 23 Feb 2024 16:24:48 GMT, Magnus Ihse Bursie wrote: > There is not much overlap on how linking is done on Windows on one hand, and > on all Unix platforms on the other. This makes Link.gmk basically consists of > two parts, each in it own half of if statements, and the few common parts are > artificially shoehorned in to fit both sides. > > The code will be much clearer if we just split this into two different files. > > Note that this PR slightly collides with JDK-8326583 > (https://github.com/openjdk/jdk/pull/17986). Whichever of this goes in first > will mean that the other one needs to make some adaptations. I have verified that there is no differences in the resulting output using COMPARE_BUILD, for the platforms in Oracle's CI: windows-x64,linux-x64,linux-aarch64,macosx-x64,macosx-aarch64, confirming that this is a pure build system refactoring. - PR Comment: https://git.openjdk.org/jdk/pull/17987#issuecomment-1962011909
Re: RFR: 8326587: Split Link.gmk into a Windows and Unix part
On Fri, 23 Feb 2024 16:24:48 GMT, Magnus Ihse Bursie wrote: > There is not much overlap on how linking is done on Windows on one hand, and > on all Unix platforms on the other. This makes Link.gmk basically consists of > two parts, each in it own half of if statements, and the few common parts are > artificially shoehorned in to fit both sides. > > The code will be much clearer if we just split this into two different files. > > Note that this PR slightly collides with JDK-8326583 > (https://github.com/openjdk/jdk/pull/17986). Whichever of this goes in first > will mean that the other one needs to make some adaptations. Is it really a good idea to split makefiles based on target platform? It creates a less flexible separation that makes experimentation with different combinations of toolchains and platforms much harder. We currently have a pretty well established mapping between target platform and toolchain type in practice, while trying to keep the concepts somewhat separated, but this kind of change really cements that mapping. Given Julian's ambition for making it possible to compile for Windows using a different toolchain, I'm not sure this is the right way to go. - PR Comment: https://git.openjdk.org/jdk/pull/17987#issuecomment-1961739183
Re: RFR: 8326587: Split Link.gmk into a Windows and Unix part [v2]
> There is not much overlap on how linking is done on Windows on one hand, and > on all Unix platforms on the other. This makes Link.gmk basically consists of > two parts, each in it own half of if statements, and the few common parts are > artificially shoehorned in to fit both sides. > > The code will be much clearer if we just split this into two different files. > > Note that this PR slightly collides with JDK-8326583 > (https://github.com/openjdk/jdk/pull/17986). Whichever of this goes in first > will mean that the other one needs to make some adaptations. Magnus Ihse Bursie has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains one commit: 8326587: Split Link.gmk into a Windows and Unix part - Changes: https://git.openjdk.org/jdk/pull/17987/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17987&range=01 Stats: 668 lines in 13 files changed: 371 ins; 283 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/17987.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17987/head:pull/17987 PR: https://git.openjdk.org/jdk/pull/17987
RFR: 8174269: Remove COMPAT locale data provider from JDK
This PR intends to remove the legacy `COMPAT` locale data from the JDK. The `COMPAT` locale data was introduced for applications' migratory purposes transitioning to `CLDR`. It is becoming a technical debt and now is the time to remove it (we've been emitting a warning at JVM startup since JDK21, if the app is using `COMPAT`). A corresponding CSR has also been drafted. - Commit messages: - cleanup - BreakIteratorProvider available locales fix - clean-up - test fixes - makeZoneData.pl fix - Vanguard fix - test fixes - tz fixes - Specification changes - Restoring a test - ... and 29 more: https://git.openjdk.org/jdk/compare/b419e951...f3db6099 Changes: https://git.openjdk.org/jdk/pull/17991/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17991&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8174269 Stats: 67652 lines in 566 files changed: 478 ins; 66408 del; 766 mod Patch: https://git.openjdk.org/jdk/pull/17991.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17991/head:pull/17991 PR: https://git.openjdk.org/jdk/pull/17991
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution
On Fri, 23 Feb 2024 16:23:43 GMT, Magnus Ihse Bursie wrote: > The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17986#pullrequestreview-1898688421
Integrated: 8326585: COMPARE_BUILD=PATCH fails if patch -R fails
On Fri, 23 Feb 2024 15:18:11 GMT, Magnus Ihse Bursie wrote: > At the end of a COMPARE_BUILD run with a patch file, the build tries to > revert the patch to restore the workspace as it was. On some versions of > patch, this fails if one or more files were deleted, causing the makefile to > abort just before the actual comparison. :-( > > Reverting the patch is just a courtesy act for running on a developer > workspace, so even if this fails, we should continue with the comparison. This pull request has now been integrated. Changeset: 27574b38 Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/27574b384cb5c46358a8bba1bffa8d57d85f6670 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod 8326585: COMPARE_BUILD=PATCH fails if patch -R fails Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/17983
Re: RFR: 8326585: COMPARE_BUILD=PATCH fails if patch -R fails
On Fri, 23 Feb 2024 15:18:11 GMT, Magnus Ihse Bursie wrote: > At the end of a COMPARE_BUILD run with a patch file, the build tries to > revert the patch to restore the workspace as it was. On some versions of > patch, this fails if one or more files were deleted, causing the makefile to > abort just before the actual comparison. :-( > > Reverting the patch is just a courtesy act for running on a developer > workspace, so even if this fails, we should continue with the comparison. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17983#pullrequestreview-1898603155
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution
On Fri, 23 Feb 2024 16:23:43 GMT, Magnus Ihse Bursie wrote: > The idea of setting up general "toolchains" in the native build was good, but > it turned out that we really only need a single toolchain, with a single > twist: if it should use CC or CPP to link. This is better described by a > specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the > default). > > There is a single exception to this rule, and that is if we want to compile > for the build platform rather than the target platform. (This is only done > for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD > (or TARGET_TYPE := TARGET, the default). > > The final odd-case was the hack for building hsdis/bin on mingw. This can be > resolved using direct variables to SetupNativeCompilation, instead of first > creating a toolchain. > > Doing this refactoring will simplify the SetupNativeCompilation code, and > make it much clearer if we use the C or C++ linker. I have verified that there is no differences in the resulting output using COMPARE_BUILD, for the platforms in Oracle's CI: windows-x64,linux-x64,linux-aarch64,macosx-x64,macosx-aarch64, confirming that this is a pure build system refactoring. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1961679913
RFR: 8326587: Split Link.gmk into a Windows and Unix part
There is not much overlap on how linking is done on Windows on one hand, and on all Unix platforms on the other. This makes Link.gmk basically consists of two parts, each in it own half of if statements, and the few common parts are artificially shoehorned in to fit both sides. The code will be much clearer if we just split this into two different files. Note that this PR slightly collides with JDK-8326583 (https://github.com/openjdk/jdk/pull/17986). Whichever of this goes in first will mean that the other one needs to make some adaptations. - Commit messages: - 8326587: Split Link.gmk into a Windows and Unix part Changes: https://git.openjdk.org/jdk/pull/17987/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17987&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326587 Stats: 659 lines in 13 files changed: 362 ins; 283 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/17987.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17987/head:pull/17987 PR: https://git.openjdk.org/jdk/pull/17987
RFR: 8326583: Remove over-generalized DefineNativeToolchain solution
The idea of setting up general "toolchains" in the native build was good, but it turned out that we really only need a single toolchain, with a single twist: if it should use CC or CPP to link. This is better described by a specific argument to SetupNativeCompilation, LANG := C++ or LANG := C (the default). There is a single exception to this rule, and that is if we want to compile for the build platform rather than the target platform. (This is only done for adlc) To keep expressing this difference, introduce TARGET_TYPE := BUILD (or TARGET_TYPE := TARGET, the default). The final odd-case was the hack for building hsdis/bin on mingw. This can be resolved using direct variables to SetupNativeCompilation, instead of first creating a toolchain. Doing this refactoring will simplify the SetupNativeCompilation code, and make it much clearer if we use the C or C++ linker. - Commit messages: - 8326583: Remove over-generalized DefineNativeToolchain solution Changes: https://git.openjdk.org/jdk/pull/17986/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17986&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326583 Stats: 225 lines in 14 files changed: 55 ins; 137 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/17986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17986/head:pull/17986 PR: https://git.openjdk.org/jdk/pull/17986
RFR: 8326585: COMPARE_BUILD=PATCH fails if patch -R fails
At the end of a COMPARE_BUILD run with a patch file, the build tries to revert the patch to restore the workspace as it was. On some versions of patch, this fails if one or more files were deleted, causing the makefile to abort just before the actual comparison. :-( Reverting the patch is just a courtesy act for running on a developer workspace, so even if this fails, we should continue with the comparison. - Commit messages: - 8326585: COMPARE_BUILD=PATCH fails if patch -R fails Changes: https://git.openjdk.org/jdk/pull/17983/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17983&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326585 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17983.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17983/head:pull/17983 PR: https://git.openjdk.org/jdk/pull/17983
Re: RFR: 8326406: Remove mapfile support from makefiles
On Fri, 23 Feb 2024 12:42:08 GMT, Magnus Ihse Bursie wrote: > Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) is done, we > have no need for mapfiles anymore in the JDK project. The mapfile handling > was inherited from the old build system and is just messy. We should remove > it to make the linking process more clear. > > Note that this PR is [dependent > on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) > https://github.com/openjdk/jdk/pull/17955. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17982#pullrequestreview-1898299670
Re: RFR: 8326406: Remove mapfile support from makefiles
On Fri, 23 Feb 2024 12:42:08 GMT, Magnus Ihse Bursie wrote: > Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) is done, we > have no need for mapfiles anymore in the JDK project. The mapfile handling > was inherited from the old build system and is just messy. We should remove > it to make the linking process more clear. > > Note that this PR is [dependent > on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) > https://github.com/openjdk/jdk/pull/17955. Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17982#pullrequestreview-1898222180
Re: RFR: 8017234: Hotspot should stop using mapfiles [v6]
On Fri, 23 Feb 2024 12:38:23 GMT, Magnus Ihse Bursie wrote: >> **Summary:** Finally get rid of the mapfiles in Hotspot, and replace it with >> compiler options and `JNIEXPORT` on all platforms. >> >> The bug that this PR solves, >> [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234), was created in >> 2013. Even back then the use of mapfiles in Hotspot was dated, so this is >> really good riddance with old rubbish. >> >> This code touches on central but not well understood parts of the Hotspot >> dynamic library, which has contributed to why this bug has stayed unresolved >> for so long. I will need to explain this fix in more detail than usually >> necessary. (Please bare with me if this gets long.) I also anticipate that >> not all solutions that I've picked will be accepted, and we'll have to >> discuss how to proceed. I think it is better to have actual concrete code to >> discuss around, rather than starting by an abstract discussion. To keep this >> description short, I will post the discussion as a comment to the PR. >> >> I have run this PR through tier 1-3 in our CI system. I have also carefully >> checked how the resulting dynamic library differs with this patch (not much; >> see discussion below). For build system changes, this is often the most >> relevant metric. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Revert "Use #pragma instead of HIDDEN define" > > This reverts commit 00e40a7f6e4cdef6592d72b3d08063cdcc41532a. Windows-x64 and Linux-x64 look fine to me. - Marked as reviewed by djelinski (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17955#pullrequestreview-1898203882
Re: RFR: 8017234: Hotspot should stop using mapfiles [v2]
On Thu, 22 Feb 2024 13:36:23 GMT, Magnus Ihse Bursie wrote:
> I just realized I could keep an extremely simplified linker script
> ("mapfile") for gcc, and thereby keeping the `@SUNWprivate_1.1` on the
> exported symbols, and keeping `__bss_start` and friends local. This further
> minimizes the difference to the existing libjvm.so for gcc builds.
Why do this just for GCC? Shouldn't this be for Linux as we are doing it for
backwards compatibility with user JNI libraries.
-
PR Comment: https://git.openjdk.org/jdk/pull/17955#issuecomment-1961367943
Re: RFR: 8017234: Hotspot should stop using mapfiles [v6]
On Thu, 22 Feb 2024 15:26:34 GMT, Magnus Ihse Bursie wrote: >> I see, seems like it's an unfortunate situation where a fix is hard or even >> impossible. If we file a gcc bug for it now, it'll get fixed in some >> insanely new gcc version (such as gcc 15 or 16) and we'd have to jump to >> that version, which I'm sure no one is going to like :( >> >> Nevertheless, since there are so many instances of it, I suggest #pragma GCC >> visibility push(hidden) instead of adding a new macro to this file, in my >> opinion it's somewhat cleaner and also makes the change smaller. >> >> #pragma GCC visibility push(hidden) >> // Junk that needs hidden visibility to work properly >> #pragma GCC visibility pop >> >> Also, why TARGET_COMPILER_gcc instead of __GNUC__? Does this have to work >> with Apple Clang on macOS as well? > > This is not an issue with clang, only gcc. > > My understanding was that the proper way to test for compiler in Hotspot code > was by using `TARGET_COMPILER_gcc`. > > I can try using the `#pragma` route instead; if it works I agree it is > slightly better than to sprinkle `HIDDEN` all over the place. Drat, I thought the pragma would work. Sorry. I would file a bug for gcc, but I have no idea what to even describe it as - PR Review Comment: https://git.openjdk.org/jdk/pull/17955#discussion_r1500624847
RFR: 8326406: Remove mapfile support from makefiles
Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) is done, we have no need for mapfiles anymore in the JDK project. The mapfile handling was inherited from the old build system and is just messy. We should remove it to make the linking process more clear. Note that this PR is [dependent on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) https://github.com/openjdk/jdk/pull/17955. - Depends on: https://git.openjdk.org/jdk/pull/17955 Commit messages: - 8326406: Remove mapfile support from makefiles Changes: https://git.openjdk.org/jdk/pull/17982/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17982&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326406 Stats: 54 lines in 4 files changed: 0 ins; 47 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/17982.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17982/head:pull/17982 PR: https://git.openjdk.org/jdk/pull/17982
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v3]
> Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) has been > integrated, it is possible to do some cleanup. The goal of > [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) was to not change > any behavior, even if that behavior seemed odd. > > Now let's try to fix that. We can: > > a) remove JNIEXPORT from c2v functions. > b) make debug.cpp functions exported similarly on all platforms. > c) remove JNIEXPORT from aarch64 asm debug function. > > Note that this PR is [dependent > on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) > https://github.com/openjdk/jdk/pull/17955. 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: - Remove jvmci globals - 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 - Changes: - all: https://git.openjdk.org/jdk/pull/17967/files - new: https://git.openjdk.org/jdk/pull/17967/files/216d0db5..b6bd5f22 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17967&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17967&range=01-02 Stats: 31 lines in 1 file changed: 4 ins; 7 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/17967.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17967/head:pull/17967 PR: https://git.openjdk.org/jdk/pull/17967
Re: RFR: 8017234: Hotspot should stop using mapfiles [v5]
On Fri, 23 Feb 2024 08:18:17 GMT, Daniel Jeliński wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Use #pragma instead of HIDDEN define > > src/hotspot/share/oops/accessBackend.cpp line 40: > >> 38: #if defined(TARGET_COMPILER_gcc) >> 39: // Needed to work around bug in gcc causing these symbols to be visible >> despite -fvisibility=hidden >> 40: #pragma GCC visibility push(hidden) > > does it work for you? because it doesn't hide the arraycopy symbols for me. > The explicit visibility(hidden) attribute did work. You are absolutely correct. I thought I checked that it did work, but I must have messed up somehow, because now that I retest, I see that it did not have any effect. I'm reverting that change. Thanks for being attentive! (Presumably, gcc already knew that it where compiling with visibility hidden, so the pragma did nothing, in contrast with the explicit attribute on individual functions.) - PR Review Comment: https://git.openjdk.org/jdk/pull/17955#discussion_r1500596597
Re: RFR: 8017234: Hotspot should stop using mapfiles [v6]
On Fri, 23 Feb 2024 12:38:23 GMT, Magnus Ihse Bursie wrote: >> **Summary:** Finally get rid of the mapfiles in Hotspot, and replace it with >> compiler options and `JNIEXPORT` on all platforms. >> >> The bug that this PR solves, >> [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234), was created in >> 2013. Even back then the use of mapfiles in Hotspot was dated, so this is >> really good riddance with old rubbish. >> >> This code touches on central but not well understood parts of the Hotspot >> dynamic library, which has contributed to why this bug has stayed unresolved >> for so long. I will need to explain this fix in more detail than usually >> necessary. (Please bare with me if this gets long.) I also anticipate that >> not all solutions that I've picked will be accepted, and we'll have to >> discuss how to proceed. I think it is better to have actual concrete code to >> discuss around, rather than starting by an abstract discussion. To keep this >> description short, I will post the discussion as a comment to the PR. >> >> I have run this PR through tier 1-3 in our CI system. I have also carefully >> checked how the resulting dynamic library differs with this patch (not much; >> see discussion below). For build system changes, this is often the most >> relevant metric. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Revert "Use #pragma instead of HIDDEN define" > > This reverts commit 00e40a7f6e4cdef6592d72b3d08063cdcc41532a. Great. The only remaining difference I see is that the PR adds the following export: _ZGRN14AsyncLogWriter4NoneE_@@SUNWprivate_1.1 Any idea what it might be? If not, I guess we can live with that. - PR Comment: https://git.openjdk.org/jdk/pull/17955#issuecomment-1961255985
Re: RFR: 8017234: Hotspot should stop using mapfiles [v5]
On Fri, 23 Feb 2024 12:33:48 GMT, Magnus Ihse Bursie wrote: > > We had the PR in our SAP internal build/test queue, so issues seen with it. > > What issues did you see? Or was that a typo for "no issues"? Sorry Magnus, tests were fine no issues were observed. - PR Comment: https://git.openjdk.org/jdk/pull/17955#issuecomment-1961250951
Re: RFR: 8017234: Hotspot should stop using mapfiles [v5]
On Thu, 22 Feb 2024 16:15:23 GMT, Magnus Ihse Bursie wrote: >> **Summary:** Finally get rid of the mapfiles in Hotspot, and replace it with >> compiler options and `JNIEXPORT` on all platforms. >> >> The bug that this PR solves, >> [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234), was created in >> 2013. Even back then the use of mapfiles in Hotspot was dated, so this is >> really good riddance with old rubbish. >> >> This code touches on central but not well understood parts of the Hotspot >> dynamic library, which has contributed to why this bug has stayed unresolved >> for so long. I will need to explain this fix in more detail than usually >> necessary. (Please bare with me if this gets long.) I also anticipate that >> not all solutions that I've picked will be accepted, and we'll have to >> discuss how to proceed. I think it is better to have actual concrete code to >> discuss around, rather than starting by an abstract discussion. To keep this >> description short, I will post the discussion as a comment to the PR. >> >> I have run this PR through tier 1-3 in our CI system. I have also carefully >> checked how the resulting dynamic library differs with this patch (not much; >> see discussion below). For build system changes, this is often the most >> relevant metric. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Use #pragma instead of HIDDEN define > We had the PR in our SAP internal build/test queue, so issues seen with it. What issues did you see? Or was that a typo for "no issues"? - PR Comment: https://git.openjdk.org/jdk/pull/17955#issuecomment-1961248385
Re: RFR: 8017234: Hotspot should stop using mapfiles [v6]
> **Summary:** Finally get rid of the mapfiles in Hotspot, and replace it with > compiler options and `JNIEXPORT` on all platforms. > > The bug that this PR solves, > [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234), was created in > 2013. Even back then the use of mapfiles in Hotspot was dated, so this is > really good riddance with old rubbish. > > This code touches on central but not well understood parts of the Hotspot > dynamic library, which has contributed to why this bug has stayed unresolved > for so long. I will need to explain this fix in more detail than usually > necessary. (Please bare with me if this gets long.) I also anticipate that > not all solutions that I've picked will be accepted, and we'll have to > discuss how to proceed. I think it is better to have actual concrete code to > discuss around, rather than starting by an abstract discussion. To keep this > description short, I will post the discussion as a comment to the PR. > > I have run this PR through tier 1-3 in our CI system. I have also carefully > checked how the resulting dynamic library differs with this patch (not much; > see discussion below). For build system changes, this is often the most > relevant metric. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Revert "Use #pragma instead of HIDDEN define" This reverts commit 00e40a7f6e4cdef6592d72b3d08063cdcc41532a. - Changes: - all: https://git.openjdk.org/jdk/pull/17955/files - new: https://git.openjdk.org/jdk/pull/17955/files/00e40a7f..7be8372a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17955&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17955&range=04-05 Stats: 31 lines in 1 file changed: 4 ins; 7 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/17955.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17955/head:pull/17955 PR: https://git.openjdk.org/jdk/pull/17955
Re: RFR: 8017234: Hotspot should stop using mapfiles [v5]
On Fri, 23 Feb 2024 11:01:47 GMT, Magnus Ihse Bursie wrote: > Just to confirm: this PR passes tier 1-5. We had the PR in our SAP internal build/test queue, so issues seen with it. - PR Comment: https://git.openjdk.org/jdk/pull/17955#issuecomment-1961232277
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v2]
On Fri, 23 Feb 2024 11:41:04 GMT, Magnus Ihse Bursie wrote: >> cc @karianna @gdams, is there a Windows/AArch64 owner? >> >>> Or maybe it's time to retire the Windows/aarch64 port >> >> I guess Windows/AArch64 as a platform hasn't really taken off _yet_, give it >> a few more years 😉 I certainly would not advocate for removing it. > > Instead of discussing removal of windows/aarch64 (although the general rule > in OpenJDK is that ports that are not maintained by anyone should be > removed); let's stick to the question here: do we need to export `das1()` for > debugging? Personally I have never used `das`/`das1` on any AArch64 port when debugging. I guess it was somewhat handy when the backend was originally developed. I would argue that the built-in disassembler of any debugger and `disnm` from `debug.cpp` are good enough, and thus `das`/`das1` can be removed. What do you think @theRealAph ? - PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1500568587
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v2]
On Fri, 23 Feb 2024 11:06:04 GMT, Magnus Ihse Bursie wrote: >> Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) has been >> integrated, it is possible to do some cleanup. The goal of >> [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) was to not change >> any behavior, even if that behavior seemed odd. >> >> Now let's try to fix that. We can: >> >> a) remove JNIEXPORT from c2v functions. >> b) make debug.cpp functions exported similarly on all platforms. >> c) remove JNIEXPORT from aarch64 asm debug function. >> >> Note that this PR is [dependent >> on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) >> https://github.com/openjdk/jdk/pull/17955. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Remove jvmci globals Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17967#pullrequestreview-1897936208
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v2]
On Fri, 23 Feb 2024 11:35:19 GMT, Bernhard Urban-Forster wrote: >> Ah, right, there is a Windows/aarch64 port. Forgot about it. :( >> >> I don't know who maintains/cares about that port anymore. Pinging @mo-beck >> @lewurm @luhenry. Do anyone of you care about this anymore? If not, do you >> know if anyone has taken up the baton? (Or maybe it's time to retire the >> Windows/aarch64 port) > > cc @karianna @gdams, is there a Windows/AArch64 owner? > >> Or maybe it's time to retire the Windows/aarch64 port > > I guess Windows/AArch64 as a platform hasn't really taken off _yet_, give it > a few more years 😉 I certainly would not advocate for removing it. Instead of discussing removal of windows/aarch64 (although the general rule in OpenJDK is that ports that are not maintained by anyone should be removed); let's stick to the question here: do we need to export `das1()` for debugging? - PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1500547680
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v2]
On Fri, 23 Feb 2024 11:11:19 GMT, Magnus Ihse Bursie wrote:
>> src/hotspot/cpu/aarch64/assembler_aarch64.cpp line 122:
>>
>>> 120: }
>>> 121:
>>> 122: void das1(uintptr_t insn) {
>>
>> Doesn't this need to be exported for debugging on Windows-Aarch64?
>
> Ah, right, there is a Windows/aarch64 port. Forgot about it. :(
>
> I don't know who maintains/cares about that port anymore. Pinging @mo-beck
> @lewurm @luhenry. Do anyone of you care about this anymore? If not, do you
> know if anyone has taken up the baton? (Or maybe it's time to retire the
> Windows/aarch64 port)
cc @karianna @gdams, is there a Windows/AArch64 owner?
> Or maybe it's time to retire the Windows/aarch64 port
I guess Windows/AArch64 as a platform hasn't really taken off _yet_, give it a
few more years 😉 I certainly would not advocate for removing it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1500542216
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v2]
On Thu, 22 Feb 2024 22:18:59 GMT, David Holmes wrote:
>> Magnus Ihse Bursie has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove jvmci globals
>
> src/hotspot/cpu/aarch64/assembler_aarch64.cpp line 122:
>
>> 120: }
>> 121:
>> 122: void das1(uintptr_t insn) {
>
> Doesn't this need to be exported for debugging on Windows-Aarch64?
Ah, right, there is a Windows/aarch64 port. Forgot about it. :(
I don't know who maintains/cares about that port anymore. Pinging @mo-beck
@lewurm @luhenry. Do anyone of you care about this anymore? If not, do you know
if anyone has taken up the baton? (Or maybe it's time to retire the
Windows/aarch64 port)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1500517200
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v2]
> Once [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) has been > integrated, it is possible to do some cleanup. The goal of > [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234) was to not change > any behavior, even if that behavior seemed odd. > > Now let's try to fix that. We can: > > a) remove JNIEXPORT from c2v functions. > b) make debug.cpp functions exported similarly on all platforms. > c) remove JNIEXPORT from aarch64 asm debug function. > > Note that this PR is [dependent > on](https://mail.openjdk.org/pipermail/jdk-dev/2021-March/005232.html) > https://github.com/openjdk/jdk/pull/17955. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Remove jvmci globals - Changes: - all: https://git.openjdk.org/jdk/pull/17967/files - new: https://git.openjdk.org/jdk/pull/17967/files/18b3f1d6..216d0db5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17967&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17967&range=00-01 Stats: 8 lines in 1 file changed: 0 ins; 8 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17967.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17967/head:pull/17967 PR: https://git.openjdk.org/jdk/pull/17967
Re: RFR: 8017234: Hotspot should stop using mapfiles [v5]
On Thu, 22 Feb 2024 16:15:23 GMT, Magnus Ihse Bursie wrote: >> **Summary:** Finally get rid of the mapfiles in Hotspot, and replace it with >> compiler options and `JNIEXPORT` on all platforms. >> >> The bug that this PR solves, >> [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234), was created in >> 2013. Even back then the use of mapfiles in Hotspot was dated, so this is >> really good riddance with old rubbish. >> >> This code touches on central but not well understood parts of the Hotspot >> dynamic library, which has contributed to why this bug has stayed unresolved >> for so long. I will need to explain this fix in more detail than usually >> necessary. (Please bare with me if this gets long.) I also anticipate that >> not all solutions that I've picked will be accepted, and we'll have to >> discuss how to proceed. I think it is better to have actual concrete code to >> discuss around, rather than starting by an abstract discussion. To keep this >> description short, I will post the discussion as a comment to the PR. >> >> I have run this PR through tier 1-3 in our CI system. I have also carefully >> checked how the resulting dynamic library differs with this patch (not much; >> see discussion below). For build system changes, this is often the most >> relevant metric. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Use #pragma instead of HIDDEN define Just to confirm: this PR passes tier 1-5. - PR Comment: https://git.openjdk.org/jdk/pull/17955#issuecomment-1961125415
Re: RFR: 8017234: Hotspot should stop using mapfiles [v5]
On Thu, 22 Feb 2024 16:15:23 GMT, Magnus Ihse Bursie wrote: >> **Summary:** Finally get rid of the mapfiles in Hotspot, and replace it with >> compiler options and `JNIEXPORT` on all platforms. >> >> The bug that this PR solves, >> [JDK-8017234](https://bugs.openjdk.org/browse/JDK-8017234), was created in >> 2013. Even back then the use of mapfiles in Hotspot was dated, so this is >> really good riddance with old rubbish. >> >> This code touches on central but not well understood parts of the Hotspot >> dynamic library, which has contributed to why this bug has stayed unresolved >> for so long. I will need to explain this fix in more detail than usually >> necessary. (Please bare with me if this gets long.) I also anticipate that >> not all solutions that I've picked will be accepted, and we'll have to >> discuss how to proceed. I think it is better to have actual concrete code to >> discuss around, rather than starting by an abstract discussion. To keep this >> description short, I will post the discussion as a comment to the PR. >> >> I have run this PR through tier 1-3 in our CI system. I have also carefully >> checked how the resulting dynamic library differs with this patch (not much; >> see discussion below). For build system changes, this is often the most >> relevant metric. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Use #pragma instead of HIDDEN define src/hotspot/share/oops/accessBackend.cpp line 40: > 38: #if defined(TARGET_COMPILER_gcc) > 39: // Needed to work around bug in gcc causing these symbols to be visible > despite -fvisibility=hidden > 40: #pragma GCC visibility push(hidden) does it work for you? because it doesn't hide the arraycopy symbols for me. The explicit visibility(hidden) attribute did work. - PR Review Comment: https://git.openjdk.org/jdk/pull/17955#discussion_r1500329388
