Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
On Tue, 27 Feb 2024 21:46:17 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 three > additional commits since the last revision: > > - Why did these creep back in? Merge error? > - Remove #undef JNIEXPORT > - Remove das1() Marked as reviewed by djelinski (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17967#pullrequestreview-1905367506
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
On Tue, 27 Feb 2024 21:46:17 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 three > additional commits since the last revision: > > - Why did these creep back in? Merge error? > - Remove #undef JNIEXPORT > - Remove das1() Seems fine to me. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17967#pullrequestreview-1905265100
Re: RFR: 8325881: Require minimum gcc version 10
On Sat, 17 Feb 2024 08:28:56 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of gcc > to be used for building OpenJDK from 6.0 to 10.0. > > This permits enabling C++17 (JDK-8314488), though gcc 9.0 might suffice for > that. A minimum of gcc 10 also obtains the primitives needed to support a > work-alick for std::is_constant_evaluated (added in C++20). There are a bunch > of improvements that would be enabled by that. Having it would also allow the > elimination of a bit of a mess in the HotSpot assert macros that was needed to > work around the lack of that feature (JDK-8303805). Either current or proposed > minimum versions of other supported compilers also provide the needed > primitives. > > Testing: mach5 tier1 (uses gcc13.2 on gcc-based platforms) > Locally (linux-x64) built and ran tier1 with gcc10.3. There was some discussion of gcc versions over in the PR proposing to enable C++17: https://github.com/openjdk/jdk/pull/14988#issuecomment-1895511862 https://github.com/openjdk/jdk/pull/14988#issuecomment-1895729165 Has that been sufficiently answered? Good to go here? @shipilev @theRealAph @TheRealMDoerr - PR Comment: https://git.openjdk.org/jdk/pull/17899#issuecomment-1968089822
Re: RFR: 8325878: Require minimum Clang version 13
On Thu, 15 Feb 2024 05:19:45 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of Clang > to be used for building OpenJDK from 3.5 to 13. > > This permits enabling C++17 (JDK-8314488), though Clang 5 might suffice for > that. A minimum of Clang 13 also obtains a critical bug fix for the > [[noreturn]] > attribute (see JDK-8303805). > > Testing: mach5 tier1, which includes building with a recent version of > Xcode/clang. There was some discussion of clang versions over in the PR proposing to enable C++17 (https://github.com/openjdk/jdk/pull/14988). The minimum version proposed here (13) seemed acceptable from that discussion. Good to go here? - PR Comment: https://git.openjdk.org/jdk/pull/17862#issuecomment-1968065624
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Wed, 14 Feb 2024 22:43:37 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of IBM > Open XL C/C++. SAP dropped support for older versions in JDK 22, only > supporting the version specified in this change. > > I need someone from the aix-ppc porters to test and review the change. There was one bit of discussion about this tool chain upgrade over in the PR for enabling C++17: https://github.com/openjdk/jdk/pull/14988#issuecomment-1741985638 Has that been sufficiently answered? Good to go here? @aixtools - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1968058127
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders [v3]
On Tue, 27 Feb 2024 13:44:15 GMT, Andrew Leonard wrote: >> Adds Linux -fdebug-prefix-map'ing for SUPPORT_OUTPUTDIR and >> HOTSPOT_OUTPUTDIR when absolute paths are not allowed in the binaries, thus >> making the building of a JDK identically reproducible from different build >> directories. > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > Re-trigger actions Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18009#pullrequestreview-1904920207
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
On Tue, 27 Feb 2024 15:23:09 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Only show runtime image suffix for JDK modules >From a build point of view, I don't have much to complain about. See inline >comments for some nits. make/autoconf/jdk-options.m4 line 596: > 594: > > 595: # > 596: # jlink related. This part of the comment seems a bit redundant. make/autoconf/spec.gmk.template line 904: > 902: RL_INTERMEDIATE_IMAGE_SUBDIR := runtime-link-initial-jdk > 903: RL_INTERMEDIATE_IMAGE_DIR := > $(IMAGES_OUTPUTDIR)/$(RL_INTERMEDIATE_IMAGE_SUBDIR) > 904: This intermediate directory is only used inside the Images.gmk. I don't think we need to define it in spec. It can be a local variable in Images.gmk. Also, unless you really want this intermediate image in "images" as some kind of deliverable I would prefer to put it somewhere under `$(SUPPORT_OUTPUTDIR)/images`. - PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-1904905122 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1505071076 PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1505072553
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
On Fri, 23 Feb 2024 12:03:37 GMT, Bernhard Urban-Forster wrote: >> 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 ? Removal makes sense - thank you @lewurm, @luhenry, and @adinn for your input! - PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1505070224
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
On Tue, 27 Feb 2024 16:11:34 GMT, Andrew Dinn wrote: >> You can probably remove the entire function given it's an indirection to >> `das` anyway. I remember using it for debugging during the first stages of >> the Windows-AArch64 port but I've long forgotten about it. > > @luhenry @magicus das1 was added when we were implementing the AArch64 port > in order to help us integrate the debugger support we provided when running > JITted code on our AArch64 simulator into gdb. Now that we have real hardware > it is redundant. Thanks @adinn for the confirmation! I've now removed it. - PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1505045060
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v4]
On Tue, 27 Feb 2024 11:01:03 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 with a new target base due to > a merge or a rebase. The pull request now contains 10 commits: > > - Merge branch 'master' into hotspot-symbols-followup > - Remove jvmci globals > - 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 > - Revert "Use #pragma instead of HIDDEN define" > >This reverts commit 00e40a7f6e4cdef6592d72b3d08063cdcc41532a. > - Use #pragma instead of HIDDEN define > - Rename the version script to version-script.txt > - Restore linker script to linux/gcc builds > - Rename the Windows export file to .def > - Remove unused symbol _Copy_conjoint_bytes on linux/arm32 > - 8017234: Hotspot should stop using mapfiles There was a bit back and forth with this PR, but now I believe it is finally ready. Any reviewers? - PR Comment: https://git.openjdk.org/jdk/pull/17967#issuecomment-1967657651
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v5]
> 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 three additional commits since the last revision: - Why did these creep back in? Merge error? - Remove #undef JNIEXPORT - Remove das1() - Changes: - all: https://git.openjdk.org/jdk/pull/17967/files - new: https://git.openjdk.org/jdk/pull/17967/files/43c3513b..387d243b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17967=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17967=03-04 Stats: 24 lines in 4 files changed: 0 ins; 24 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: 8326831 Clarify test harness control variables in make help
On Tue, 27 Feb 2024 13:56:56 GMT, Ludvig Janiuk wrote: > I'll note that you still need to run configure to execute make test-only > JTREG=help. Should this really be necessary? I agree that it would be convenient, in some rare usecase, to be able to run this without configure, but the added complexity needed to implement that is hardly worth the effort. In practice, if you are about to run `make test`, and trying to construct your command line, you would have already run configure. > Also, while `make test-only JTREG=help` lists the valid options, it still > does nothing to document their meaning. The documentation for running tests can be found in `doc/testing.md`. The help output from make is just meant to be a short version reminder for common cases. The list is constructed programmatically from the makefile constructs used to define the possible parameters, so it's guaranteed to be up to date. That is also why you must call the same target as when using the parameter (`test` or `test-only` in this case) as otherwise you wouldn't end up in the makefile where the parameters are defined. You can't really expect makefiles to be fully self documenting as we are limited by what we can reasonably implement in makefiles after all, and these makefiles are really pushing the boundaries of that already. - PR Comment: https://git.openjdk.org/jdk/pull/18028#issuecomment-1967575782
Re: RFR: 8326831 Clarify test harness control variables in make help
On Tue, 27 Feb 2024 13:55:53 GMT, Ludvig Janiuk wrote: > Clarifying text in `make help` output on how to list variables for e.g. > JTREG='...'. Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18028#pullrequestreview-1904771315
Re: Build the jdk with new or modified sources
Lorris, You cannot use new API in java.base in the code for javac, meaning the java.compiler and jdk.compiler modules. This is an ingherent limitation of the bootstrap process. -- Jon On 2/27/24 1:57 AM, Lorris wrote: Yes indeed, I am trying to use those changes from the jdk.compiler module. How can I make this work without causing problem when building the interim module ? Lorris On 26 Feb 2024, at 14:59, erik.joels...@oracle.com wrote: Hello Lorris, This is not expected behavior. Building incrementally is expected to work correctly. If you are making changes to java.base and then trying to use those changes from any of java.compiler, jdk.compiler and jdk.javadoc, you will have problems as those three modules have to be compatible with the previous JDK version (21 in this case). We build those modules for the "interim langtools" which is then used to compile the rest of the JDK by running this interim javac compiler on the boot jdk. I think the problems you are seeing appear when building the interim versions of these modules as they will never see the updates you are making to java.base. If that isn't the issue, could you share more details on what kind of environment you are building on (OS, arch etc), how you configured the build and the make command line you are using? /Erik On 2/26/24 05:40, Lorris wrote: Hello, I’m trying to build the JDK (version 23, my fork is up to date as of 2024/02/26) while adding new source files and also adding some fields or methods in existing files (in java.base and java.compiler modules). My problem is that when I try to build the image, most of the time (it seems to depend on the date of the last edited file) the new files and members I have added aren’t visible to the compiler which results in an error. Until now I have been using a workaround by building two times, firstly, only with the new code declarations and, secondly, with the references to this code. However, I’m now seeing the limits of this method. Regards, Lorris Creantor
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v5]
On Tue, 27 Feb 2024 11:24:08 GMT, Magnus Ihse Bursie wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Addressing review comments > > This looks good from a build perspective. Actually, it looks great! :) I'm > happy to get rid of this old strange construct. Thanks, @magicus > to get rid of this old strange construct. Removing legacy clutter is exactly the purpose of this exercise! - PR Comment: https://git.openjdk.org/jdk/pull/17991#issuecomment-1967125233
Re: RFR: 8326587: Separate out Microsoft toolchain linking [v4]
On Tue, 27 Feb 2024 11:06:15 GMT, Magnus Ihse Bursie wrote: >> This was actually an important part of this PR, trying to combat the all too >> general solution we had when trying to combine microsoft and non-microsoft >> linking. On all Unix-style linkers, the option to mark the output file is >> `-o `. That is such a fundemental option that we do not want to make it >> general, especially not considering the additional difficulty that the >> corresponding option for the microsoft `link.exe` is to specify >> `/out:`, without a space. This forced us to define LD_OUT_OPTION >> as `-o$(SPACE)` on unix linkers, and it forced us to setup the linker >> command line with `$(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE)` with no space >> between. >> >> That is in clear contrast with how we can add multiple sets of command line >> options like e.g. `(LDFLAGS_CXX_PARTIAL_LINKING) $$($1_SYSROOT_LDFLAGS)` >> where we can just stack them after each other with a space between. > > Also, > >> the flexibility of specifying the non-linker specific option here > > I don't know what you mean by that. LD_OUT_OPTION was thought of as an > abstraction between the difference of `-o ` and `/out:`. It should not be > used to include anything else, so no flexibility is lost. Thanks for the explanation. With the separation of Microsoft linking and this handling the Unix style linking only, using the non-general `-o` seems reasonable. - PR Review Comment: https://git.openjdk.org/jdk/pull/17987#discussion_r1504622620
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v4]
On Tue, 27 Feb 2024 15:35:56 GMT, Ludovic Henry wrote: >> It does not sound like anyone object to the removal of `JNIEXPORT` for >> `das1`, then. >> >> Or should I just remove the entire function, if it serves no purpose that >> any current maintainers know or care about? > > You can probably remove the entire function given it's an indirection to > `das` anyway. I remember using it for debugging during the first stages of > the Windows-AArch64 port but I've long forgotten about it. @luhenry @magicus das1 was added when we were implementing the AArch64 port in order to help us integrate the debugger support we provided when running JITted code on our AArch64 simulator into gdb. Now that we have real hardware it is redundant. - PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1504535787
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders [v3]
On Tue, 27 Feb 2024 13:44:15 GMT, Andrew Leonard wrote: >> Adds Linux -fdebug-prefix-map'ing for SUPPORT_OUTPUTDIR and >> HOTSPOT_OUTPUTDIR when absolute paths are not allowed in the binaries, thus >> making the building of a JDK identically reproducible from different build >> directories. > > Andrew Leonard has updated the pull request incrementally with one additional > commit since the last revision: > > Re-trigger actions @erikj79 ready for re-review please - PR Comment: https://git.openjdk.org/jdk/pull/18009#issuecomment-1966869370
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v4]
On Mon, 26 Feb 2024 20:07:35 GMT, Magnus Ihse Bursie wrote: >> I use `das` every day. In contrast, I don't know what `das1` is for. > > It does not sound like anyone object to the removal of `JNIEXPORT` for > `das1`, then. > > Or should I just remove the entire function, if it serves no purpose that any > current maintainers know or care about? You can probably remove the entire function given it's an indirection to `das` anyway. I remember using it for debugging during the first stages of the Windows-AArch64 port but I've long forgotten about it. - PR Review Comment: https://git.openjdk.org/jdk/pull/17967#discussion_r1504463486
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v18]
> Please review this patch which adds a jlink mode to the JDK which doesn't > need the packaged modules being present. A.k.a run-time image based jlink. > Fundamentally this patch adds an option to use `jlink` even though your JDK > install might not come with the packaged modules (directory `jmods`). This is > particularly useful to further reduce the size of a jlinked runtime. After > the removal of the concept of a JRE, a common distribution mechanism is still > the full JDK with all modules and packaged modules. However, packaged modules > can incur an additional size tax. For example in a container scenario it > could be useful to have a base JDK container including all modules, but > without also delivering the packaged modules. This comes at a size advantage > of `~25%`. Such a base JDK container could then be used to `jlink` > application specific runtimes, further reducing the size of the application > runtime image (App + JDK runtime; as a single image *or* separate bundles, > depending on the app b eing modularized). > > The basic design of this approach is to add a jlink plugin for tracking > non-class and non-resource files of a JDK install. I.e. files which aren't > present in the jimage (`lib/modules`). This enables producing a `JRTArchive` > class which has all the info of what constitutes the final jlinked runtime. > > Basic usage example: > > $ diff -u <(./bin/java --list-modules --limit-modules java.se) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules java.se) > $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) > <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules > --limit-modules jdk.jlink) > $ ls ../linux-x86_64-server-release/images/jdk/jmods > java.base.jmodjava.net.http.jmod java.sql.rowset.jmod > jdk.crypto.ec.jmod jdk.internal.opt.jmod > jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod > java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod > jdk.dynalink.jmod jdk.internal.vm.ci.jmod > jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod > java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod > jdk.editpad.jmod jdk.internal.vm.compiler.jmod > jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod > java.desktop.jmod java.scripting.jmod java.xml.jmod > jdk.hotspot.agent.jmod jdk.internal.vm.compiler.manage... Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Only show runtime image suffix for JDK modules - Changes: - all: https://git.openjdk.org/jdk/pull/14787/files - new: https://git.openjdk.org/jdk/pull/14787/files/00caf77c..b034cce2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14787=17 - incr: https://webrevs.openjdk.org/?repo=jdk=14787=16-17 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14787.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14787/head:pull/14787 PR: https://git.openjdk.org/jdk/pull/14787
RFR: 8326831 Clarify test harness control variables in make help
Clarifying text in `make help` output on how to list variables for e.g. JTREG='...'. - Commit messages: - 8326831 Clarify test harness control variables in make help Changes: https://git.openjdk.org/jdk/pull/18028/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18028=00 Issue: https://bugs.openjdk.org/browse/JDK-8326831 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18028.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18028/head:pull/18028 PR: https://git.openjdk.org/jdk/pull/18028
Re: RFR: 8326831 Clarify test harness control variables in make help
On Tue, 27 Feb 2024 13:55:53 GMT, Ludvig Janiuk wrote: > Clarifying text in `make help` output on how to list variables for e.g. > JTREG='...'. I'll note that you still need to run configure to execute `make test-only JTREG=help`. Should this really be necessary? Also, while `make test-only JTREG=help` lists the valid options, it still does nothing to document their meaning. - PR Comment: https://git.openjdk.org/jdk/pull/18028#issuecomment-1966607480 PR Comment: https://git.openjdk.org/jdk/pull/18028#issuecomment-1966609174
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders [v3]
> Adds Linux -fdebug-prefix-map'ing for SUPPORT_OUTPUTDIR and HOTSPOT_OUTPUTDIR > when absolute paths are not allowed in the binaries, thus making the building > of a JDK identically reproducible from different build directories. Andrew Leonard has updated the pull request incrementally with one additional commit since the last revision: Re-trigger actions - Changes: - all: https://git.openjdk.org/jdk/pull/18009/files - new: https://git.openjdk.org/jdk/pull/18009/files/4bd1d3dd..1af645c7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18009=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18009=01-02 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18009.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18009/head:pull/18009 PR: https://git.openjdk.org/jdk/pull/18009
Re: Build the jdk with new or modified sources
I don’t have problems with clock skew (or at least I never encountered any problem with it until today). The problem I was referring to in this statement is when I modify a class A, to add for example a method to it, and then reference this new method in another file B, when I build for the first time, I won’t have any problem. Although, if after this build I modify the B file, the next build will result in an error stating that the A method does not exist. If the A class has been modified after the files referencing the new methods it contains everything works correctly, but not vice versa. Lorris > On 26 Feb 2024, at 22:22, Magnus Ihse Bursie > wrote: > > On 2024-02-26 14:40, Lorris wrote: > >> (it seems to depend on the date of the last edited file) > Just checking: do you have a problem with clock skew on your machine, or are > you using some tools that modify time stamps on modified files? The make > machinery depends on the fact that edited files should have a newer > timestamp. If this does not work properly, dependency tracking will fail. > > /Magnus >
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 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. > > 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 five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution I can't really say I agree, but I guess we'll see where this goes from here first - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966471047
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders [v2]
On Tue, 27 Feb 2024 11:53:03 GMT, Andrew Leonard wrote: >> Adds Linux -fdebug-prefix-map'ing for SUPPORT_OUTPUTDIR and >> HOTSPOT_OUTPUTDIR when absolute paths are not allowed in the binaries, thus >> making the building of a JDK identically reproducible from different build >> directories. > > Andrew Leonard has updated the pull request incrementally with two additional > commits since the last revision: > > - 8326685: Linux builds not reproducible if two builds configured in > different build folders > >Signed-off-by: Andrew Leonard > - 8326685: Linux builds not reproducible if two builds configured in > different build folders > >Signed-off-by: Andrew Leonard Marked as reviewed by ihse (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18009#pullrequestreview-1903307285
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders [v2]
On Tue, 27 Feb 2024 11:50:35 GMT, Andrew Leonard wrote: >> Also, this mapping business is getting really convoluted. :-( I did not like >> it as it was, and this patch makes it even worse. I think we need to rewrite >> this to create some kind of dict/map, and then iterate over the map and >> apply `-fdebug-prefix-map` to all items in the map. Unfortunately, the data >> structures available in shell scripts is limited and will make this a bit >> tricky to pull off. :( > > @magicus thank you for the tip, it works nicely, ready for re-review please Looks good to me now, but please let Erik have a look as well before pushing. - PR Review Comment: https://git.openjdk.org/jdk/pull/18009#discussion_r1504114547
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:44:31 GMT, Daniel Jeliński wrote: > FWIW, when I added -lstdc++ before both -static-libstdc++ and replaced LDCXX > with LD, the code compiled and linked just fine. Both GCC and G++ call the same linker, and the parameter differences are well documented. It's only a matter of deciding if we want to keep the complexity of selecting the executable to use or not. My thinking matches yours. It would be nice to get rid of LDCXX. I'll look into it as a follow-up project. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966390350
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders [v2]
> Adds Linux -fdebug-prefix-map'ing for SUPPORT_OUTPUTDIR and HOTSPOT_OUTPUTDIR > when absolute paths are not allowed in the binaries, thus making the building > of a JDK identically reproducible from different build directories. Andrew Leonard has updated the pull request incrementally with two additional commits since the last revision: - 8326685: Linux builds not reproducible if two builds configured in different build folders Signed-off-by: Andrew Leonard - 8326685: Linux builds not reproducible if two builds configured in different build folders Signed-off-by: Andrew Leonard - Changes: - all: https://git.openjdk.org/jdk/pull/18009/files - new: https://git.openjdk.org/jdk/pull/18009/files/d3200dd3..4bd1d3dd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18009=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18009=00-01 Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18009.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18009/head:pull/18009 PR: https://git.openjdk.org/jdk/pull/18009
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders [v2]
On Tue, 27 Feb 2024 10:35:20 GMT, Magnus Ihse Bursie wrote: >> It seems correct to include the output dir in the remapping, but I have two >> objections/questions to the way you are doing it. >> 1) Why not just use OUTPUTDIR instead of the two specialized subdirs? That >> is simpler, more general and future-proof. >> 2) Why not expand the value of the OUTPUTDIR variable? I.e. >> >> DEBUG_PREFIX_CFLAGS="$DEBUG_PREFIX_CFLAGS -fdebug-prefix-map=$OUTPUTDIR/=" >> ``` >> >> instead of trying to preserve it as a variable to be expanded in the make >> execution. > > Also, this mapping business is getting really convoluted. :-( I did not like > it as it was, and this patch makes it even worse. I think we need to rewrite > this to create some kind of dict/map, and then iterate over the map and apply > `-fdebug-prefix-map` to all items in the map. Unfortunately, the data > structures available in shell scripts is limited and will make this a bit > tricky to pull off. :( @magicus thank you for the tip, it works nicely, ready for re-review please - PR Review Comment: https://git.openjdk.org/jdk/pull/18009#discussion_r1504103093
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Tue, 27 Feb 2024 11:19:59 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. > > 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 five commits: > > - Merge branch 'master' into remove-toolchain-define > - Rename LANG to LINK_TYPE > - Reword "lib" comment to fit in better > - Merge branch 'master' into remove-toolchain-define > - 8326583: Remove over-generalized DefineNativeToolchain solution FWIW, when I added `-lstdc++` before both `-static-libstdc++` and replaced LDCXX with LD, the code compiled and linked just fine. Both GCC and G++ call the same linker, and the parameter differences are well documented. It's only a matter of deciding if we want to keep the complexity of selecting the executable to use or not. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966368056
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 11:09:26 GMT, Magnus Ihse Bursie wrote: > > can we get rid of LDCXX? > > Yeah, that is something I plan to look into. Linking C++ object files with > gcc will fail; and it might be that linking pure C with g++ might be > problematic. If this is the case, I hope we can at least determine this > automatically which linker frontend to use, i.e. selecting g++ as linker > frontend if there is at least one .cpp file in the set of sources. > > This PR is actually a kind of prerequisite for that kind of work. I'd certainly opt for selecting which linker driver to use automatically than using one for both; According to some discussions with several gcc maintainers (https://web.libera.chat/) they really aren't meant to be intermingled Also was fine with LANG; There wasn't a need to change it to LINK_TYPE, but oh well - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966341897
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 08:14:56 GMT, Julian Waters wrote: > can we get rid of LDCXX? Yeah, that is something I plan to look into. Linking C++ object files with gcc will fail; and it might be that linking pure C with g++ might be problematic. If this is the case, I hope we can at least determine this automatically which linker frontend to use, i.e. selecting g++ as linker frontend if there is at least one .cpp file in the set of sources. This PR is actually a kind of prerequisite for that kind of work. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966312751
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
> 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. 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 five commits: - Merge branch 'master' into remove-toolchain-define - Rename LANG to LINK_TYPE - Reword "lib" comment to fit in better - Merge branch 'master' into remove-toolchain-define - 8326583: Remove over-generalized DefineNativeToolchain solution - Changes: https://git.openjdk.org/jdk/pull/17986/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17986=03 Stats: 231 lines in 14 files changed: 58 ins; 142 del; 31 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
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v5]
On Tue, 27 Feb 2024 00:24:08 GMT, Naoto Sato wrote: >> 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. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addressing review comments This looks good from a build perspective. Actually, it looks great! :) I'm happy to get rid of this old strange construct. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17991#pullrequestreview-1903238470
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 08:29:44 GMT, Julian Waters wrote: > All those new parameters to SetupNativeCompilation, were they always there > and the comments about them were just missing from the documentation about > the function? Yep. The toolchain definition was a way to "package" multiple arguments to SetupNativeCompilation, so you did not have to specify them individually in each call. But in the end they just turned into "normal" arguments to SetupNativeCompilation (but undocumented...). - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966314926
Integrated: 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. This pull request has now been integrated. Changeset: ac3ce2aa Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/ac3ce2aa156332fc4e6f33018ff669657ab4b797 Stats: 231 lines in 14 files changed: 58 ins; 142 del; 31 mod 8326583: Remove over-generalized DefineNativeToolchain solution Reviewed-by: erikj - PR: https://git.openjdk.org/jdk/pull/17986
Re: RFR: 8326587: Separate out Microsoft toolchain linking [v4]
On Tue, 27 Feb 2024 11:04:49 GMT, Magnus Ihse Bursie wrote: >> I tend to agree, this should not have been changed to specifying -o >> directly. We generally keep options inside Makefile variables rather than >> directly passing them like this, much like how $(OBJ_SUFFIX) was recently >> used to replace directly specifying the object file suffix in the make system > > This was actually an important part of this PR, trying to combat the all too > general solution we had when trying to combine microsoft and non-microsoft > linking. On all Unix-style linkers, the option to mark the output file is `-o > `. That is such a fundemental option that we do not want to make it general, > especially not considering the additional difficulty that the corresponding > option for the microsoft `link.exe` is to specify `/out:`, without > a space. This forced us to define LD_OUT_OPTION as `-o$(SPACE)` on unix > linkers, and it forced us to setup the linker command line with > `$(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE)` with no space between. > > That is in clear contrast with how we can add multiple sets of command line > options like e.g. `(LDFLAGS_CXX_PARTIAL_LINKING) $$($1_SYSROOT_LDFLAGS)` > where we can just stack them after each other with a space between. Also, > the flexibility of specifying the non-linker specific option here I don't know what you mean by that. LD_OUT_OPTION was thought of as an abstraction between the difference of `-o ` and `/out:`. It should not be used to include anything else, so no flexibility is lost. - PR Review Comment: https://git.openjdk.org/jdk/pull/17987#discussion_r1504051487
Re: RFR: 8326587: Separate out Microsoft toolchain linking [v4]
On Tue, 27 Feb 2024 05:20:11 GMT, Julian Waters wrote: >> make/common/native/Link.gmk line 131: >> >>> 129:$(if $$($1_LINK_OBJS_RELATIVE), $$(CD) $$(OUTPUTDIR) ; ) \ >>> 130: $$($1_LD) $(LDFLAGS_CXX_PARTIAL_LINKING) >>> $$($1_SYSROOT_LDFLAGS) \ >>> 131:-o $$($1_TARGET_RELOCATABLE) \ >> >> I noticed this change that replaces `$(LD_OUT_OPTION)` with `-o` when >> reviewing our integration changes. $1_LINK_OBJS_RELATIVE is currently only >> supported on Linux/clang, it still seems good to not take away the >> flexibility of specifying the non-linker specific option string here. Any >> thoughts? > > I tend to agree, this should not have been changed to specifying -o directly. > We generally keep options inside Makefile variables rather than directly > passing them like this, much like how $(OBJ_SUFFIX) was recently used to > replace directly specifying the object file suffix in the make system This was actually an important part of this PR, trying to combat the all too general solution we had when trying to combine microsoft and non-microsoft linking. On all Unix-style linkers, the option to mark the output file is `-o `. That is such a fundemental option that we do not want to make it general, especially not considering the additional difficulty that the corresponding option for the microsoft `link.exe` is to specify `/out:`, without a space. This forced us to define LD_OUT_OPTION as `-o$(SPACE)` on unix linkers, and it forced us to setup the linker command line with `$(LD_OUT_OPTION)$$($1_TARGET_RELOCATABLE)` with no space between. That is in clear contrast with how we can add multiple sets of command line options like e.g. `(LDFLAGS_CXX_PARTIAL_LINKING) $$($1_SYSROOT_LDFLAGS)` where we can just stack them after each other with a space between. - PR Review Comment: https://git.openjdk.org/jdk/pull/17987#discussion_r1504049839
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders
On Tue, 27 Feb 2024 10:59:54 GMT, Andrew Leonard wrote: >> Also, this mapping business is getting really convoluted. :-( I did not like >> it as it was, and this patch makes it even worse. I think we need to rewrite >> this to create some kind of dict/map, and then iterate over the map and >> apply `-fdebug-prefix-map` to all items in the map. Unfortunately, the data >> structures available in shell scripts is limited and will make this a bit >> tricky to pull off. :( > >> It seems correct to include the output dir in the remapping, but I have two >> objections/questions to the way you are doing it. >> >> 1. Why not just use OUTPUTDIR instead of the two specialized subdirs? >> That is simpler, more general and future-proof. >> >> 2. Why not expand the value of the OUTPUTDIR variable? I.e. >> >> >> ``` >> DEBUG_PREFIX_CFLAGS="$DEBUG_PREFIX_CFLAGS -fdebug-prefix-map=$OUTPUTDIR/=" >> ``` >> >> instead of trying to preserve it as a variable to be expanded in the make >> execution. > > Ah thanks @magicus I hadn't seen that existed! that would simplify it, i'll > try that now > Also, this mapping business is getting really convoluted. :-( I did not like > it as it was, and this patch makes it even worse. I think we need to rewrite > this to create some kind of dict/map, and then iterate over the map and apply > `-fdebug-prefix-map` to all items in the map. Unfortunately, the data > structures available in shell scripts is limited and will make this a bit > tricky to pull off. :( yeah, although that's not easy for example for the gcc include paths that we have to work out based on the users compiler paths, so it wouldn't be a simple dict - PR Review Comment: https://git.openjdk.org/jdk/pull/18009#discussion_r1504046943
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders
On Tue, 27 Feb 2024 10:35:20 GMT, Magnus Ihse Bursie wrote: >> It seems correct to include the output dir in the remapping, but I have two >> objections/questions to the way you are doing it. >> 1) Why not just use OUTPUTDIR instead of the two specialized subdirs? That >> is simpler, more general and future-proof. >> 2) Why not expand the value of the OUTPUTDIR variable? I.e. >> >> DEBUG_PREFIX_CFLAGS="$DEBUG_PREFIX_CFLAGS -fdebug-prefix-map=$OUTPUTDIR/=" >> ``` >> >> instead of trying to preserve it as a variable to be expanded in the make >> execution. > > Also, this mapping business is getting really convoluted. :-( I did not like > it as it was, and this patch makes it even worse. I think we need to rewrite > this to create some kind of dict/map, and then iterate over the map and apply > `-fdebug-prefix-map` to all items in the map. Unfortunately, the data > structures available in shell scripts is limited and will make this a bit > tricky to pull off. :( > It seems correct to include the output dir in the remapping, but I have two > objections/questions to the way you are doing it. > > 1. Why not just use OUTPUTDIR instead of the two specialized subdirs? > That is simpler, more general and future-proof. > > 2. Why not expand the value of the OUTPUTDIR variable? I.e. > > > ``` > DEBUG_PREFIX_CFLAGS="$DEBUG_PREFIX_CFLAGS -fdebug-prefix-map=$OUTPUTDIR/=" > ``` > > instead of trying to preserve it as a variable to be expanded in the make > execution. Ah thanks @magicus I hadn't seen that existed! that would simplify it, i'll try that now - PR Review Comment: https://git.openjdk.org/jdk/pull/18009#discussion_r1504043380
Re: RFR: 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 [v4]
> 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 pull request now contains 10 commits: - Merge branch 'master' into hotspot-symbols-followup - Remove jvmci globals - 8326509: Clean up JNIEXPORT in Hotspot after JDK-8017234 - Revert "Use #pragma instead of HIDDEN define" This reverts commit 00e40a7f6e4cdef6592d72b3d08063cdcc41532a. - Use #pragma instead of HIDDEN define - Rename the version script to version-script.txt - Restore linker script to linux/gcc builds - Rename the Windows export file to .def - Remove unused symbol _Copy_conjoint_bytes on linux/arm32 - 8017234: Hotspot should stop using mapfiles - Changes: https://git.openjdk.org/jdk/pull/17967/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17967=03 Stats: 12 lines in 3 files changed: 0 ins; 8 del; 4 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
Integrated: 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. This pull request has now been integrated. Changeset: bceaed6d Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/bceaed6d4de391d1fdac0d35384c415a7c484d28 Stats: 70 lines in 5 files changed: 0 ins; 62 del; 8 mod 8326406: Remove mapfile support from makefiles Reviewed-by: jwaters, erikj - PR: https://git.openjdk.org/jdk/pull/17982
Re: RFR: 8326406: Remove mapfile support from makefiles [v3]
> 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. 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 10 commits: - SetupLinkingMicrosoft is not needed once mapfiles are removed - Merge branch 'master' into remove-mapfile-support - 8326406: Remove mapfile support from makefiles - Revert "Use #pragma instead of HIDDEN define" This reverts commit 00e40a7f6e4cdef6592d72b3d08063cdcc41532a. - Use #pragma instead of HIDDEN define - Rename the version script to version-script.txt - Restore linker script to linux/gcc builds - Rename the Windows export file to .def - Remove unused symbol _Copy_conjoint_bytes on linux/arm32 - 8017234: Hotspot should stop using mapfiles - Changes: https://git.openjdk.org/jdk/pull/17982/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17982=02 Stats: 70 lines in 5 files changed: 0 ins; 62 del; 8 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: 8326685: Linux builds not reproducible if two builds configured in different build folders
On Tue, 27 Feb 2024 10:32:24 GMT, Magnus Ihse Bursie wrote: >>> Oh you are right, we should always add the OUTPUTDIR to both maps >>> regardless of if it's internal to the WORKSPACE_ROOT or not. Paths relative >>> to the OUTPUTDIR should be stable across builds in different output >>> directories, but paths inside the OUTPUTDIR, expressed as relative to the >>> WORKSPACE_ROOT will vary. >> >> @erikj79 @magicus Yes, it needs to always be present as you've pointed out >> the OUTPUTDIR when within the WORKSPACE will be >> eg.WORKSPACE/build/linux-aarch64-server-release/support and the standard >> WORKSPACE mapping will make that build/linux-aarch64-server-release/support, >> but a build dir outside will be simply /support > > It seems correct to include the output dir in the remapping, but I have two > objections/questions to the way you are doing it. > 1) Why not just use OUTPUTDIR instead of the two specialized subdirs? That is > simpler, more general and future-proof. > 2) Why not expand the value of the OUTPUTDIR variable? I.e. > > DEBUG_PREFIX_CFLAGS="$DEBUG_PREFIX_CFLAGS -fdebug-prefix-map=$OUTPUTDIR/=" > ``` > > instead of trying to preserve it as a variable to be expanded in the make > execution. Also, this mapping business is getting really convoluted. :-( I did not like it as it was, and this patch makes it even worse. I think we need to rewrite this to create some kind of dict/map, and then iterate over the map and apply `-fdebug-prefix-map` to all items in the map. Unfortunately, the data structures available in shell scripts is limited and will make this a bit tricky to pull off. :( - PR Review Comment: https://git.openjdk.org/jdk/pull/18009#discussion_r1504011145
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders
On Tue, 27 Feb 2024 08:19:11 GMT, Andrew Leonard wrote: >>> I have not thought this fully through, but just throwing out a question: >>> would this work even if one build directory were inside the workspace root, >>> and the other not? >> >> Yes, I have tested that scenario as well, and does work. It was in fact the >> scenario in which I discovered the problem in the first place. > >> Oh you are right, we should always add the OUTPUTDIR to both maps regardless >> of if it's internal to the WORKSPACE_ROOT or not. Paths relative to the >> OUTPUTDIR should be stable across builds in different output directories, >> but paths inside the OUTPUTDIR, expressed as relative to the WORKSPACE_ROOT >> will vary. > > @erikj79 @magicus Yes, it needs to always be present as you've pointed out > the OUTPUTDIR when within the WORKSPACE will be > eg.WORKSPACE/build/linux-aarch64-server-release/support and the standard > WORKSPACE mapping will make that build/linux-aarch64-server-release/support, > but a build dir outside will be simply /support It seems correct to include the output dir in the remapping, but I have two objections/questions to the way you are doing it. 1) Why not just use OUTPUTDIR instead of the two specialized subdirs? That is simpler, more general and future-proof. 2) Why not expand the value of the OUTPUTDIR variable? I.e. DEBUG_PREFIX_CFLAGS="$DEBUG_PREFIX_CFLAGS -fdebug-prefix-map=$OUTPUTDIR/=" ``` instead of trying to preserve it as a variable to be expanded in the make execution. - PR Review Comment: https://git.openjdk.org/jdk/pull/18009#discussion_r1504006780
Re: RFR: 8326406: Remove mapfile support from makefiles [v2]
> 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. 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. - Changes: - all: https://git.openjdk.org/jdk/pull/17982/files - new: https://git.openjdk.org/jdk/pull/17982/files/85a076ef..85a076ef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17982=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17982=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 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
Integrated: 8017234: Hotspot should stop using mapfiles
On Wed, 21 Feb 2024 23:32:15 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. This pull request has now been integrated. Changeset: da14aa46 Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/da14aa463b8fcd56ba1f1a4cdb3e3f7b19f78964 Stats: 1001 lines in 33 files changed: 239 ins; 578 del; 184 mod 8017234: Hotspot should stop using mapfiles Reviewed-by: djelinski, erikj, dholmes - PR: https://git.openjdk.org/jdk/pull/17955
Re: Build the jdk with new or modified sources
Yes indeed, I am trying to use those changes from the jdk.compiler module. How can I make this work without causing problem when building the interim module ? Lorris > On 26 Feb 2024, at 14:59, erik.joels...@oracle.com wrote: > > Hello Lorris, > > This is not expected behavior. Building incrementally is expected to work > correctly. > > If you are making changes to java.base and then trying to use those changes > from any of java.compiler, jdk.compiler and jdk.javadoc, you will have > problems as those three modules have to be compatible with the previous JDK > version (21 in this case). We build those modules for the "interim langtools" > which is then used to compile the rest of the JDK by running this interim > javac compiler on the boot jdk. I think the problems you are seeing appear > when building the interim versions of these modules as they will never see > the updates you are making to java.base. > > If that isn't the issue, could you share more details on what kind of > environment you are building on (OS, arch etc), how you configured the build > and the make command line you are using? > > /Erik > > On 2/26/24 05:40, Lorris wrote: >> Hello, >> >> I’m trying to build the JDK (version 23, my fork is up to date as of >> 2024/02/26) while adding new source files and also adding some fields or >> methods in existing files (in java.base and java.compiler modules). My >> problem is that when I try to build the image, most of the time (it seems to >> depend on the date of the last edited file) the new files and members I have >> added aren’t visible to the compiler which results in an error. Until now I >> have been using a workaround by building two times, firstly, only with the >> new code declarations and, secondly, with the references to this code. >> However, I’m now seeing the limits of this method. >> >> Regards, >> Lorris Creantor >>
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Mon, 26 Feb 2024 20:21:55 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. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename LANG to LINK_TYPE Just a small question by the way: All those new parameters to SetupNativeCompilation, were they always there and the comments about them were just missing from the documentation about the function? - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966025499
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders
On Tue, 27 Feb 2024 08:13:07 GMT, Andrew Leonard wrote: >> Oh you are right, we should always add the OUTPUTDIR to both maps regardless >> of if it's internal to the WORKSPACE_ROOT or not. Paths relative to the >> OUTPUTDIR should be stable across builds in different output directories, >> but paths inside the OUTPUTDIR, expressed as relative to the WORKSPACE_ROOT >> will vary. > >> I have not thought this fully through, but just throwing out a question: >> would this work even if one build directory were inside the workspace root, >> and the other not? > > Yes, I have tested that scenario as well, and does work. It was in fact the > scenario in which I discovered the problem in the first place. > Oh you are right, we should always add the OUTPUTDIR to both maps regardless > of if it's internal to the WORKSPACE_ROOT or not. Paths relative to the > OUTPUTDIR should be stable across builds in different output directories, but > paths inside the OUTPUTDIR, expressed as relative to the WORKSPACE_ROOT will > vary. @erikj79 @magicus Yes, it needs to always be present as you've pointed out the OUTPUTDIR when within the WORKSPACE will be eg.WORKSPACE/build/linux-aarch64-server-release/support and the standard WORKSPACE mapping will make that build/linux-aarch64-server-release/support, but a build dir outside will be simply /support - PR Review Comment: https://git.openjdk.org/jdk/pull/18009#discussion_r1503813251
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Tue, 27 Feb 2024 08:07:38 GMT, Daniel Jeliński wrote: > can we get rid of LDCXX? On my system LDCXX is mapped to `g++` and LD is > `gcc`; I searched for the differences, and the only thing I could find is > that `g++` implicitly adds `-lstdc++ -shared-libgcc`; I suppose we could > explicitly add these options, and use gcc everywhere. > > See: https://stackoverflow.com/a/172592 > https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html I don't think that's a good idea, the differences between the gcc and g++ drivers are supposed to be (opaque) implementation details. Doing so would make us dependent on internal mechanisms of gcc subject to change between versions and would make bugfixing hard to do if such a change really happened and the linker command line got messed up as a result (This likely would impact clang too, unless I am mistaken?) - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1966002485
Re: RFR: 8326685: Linux builds not reproducible if two builds configured in different build folders
On Mon, 26 Feb 2024 22:05:34 GMT, Erik Joelsson wrote: >> I have not thought this fully through, but just throwing out a question: >> would this work even if one build directory were inside the workspace root, >> and the other not? > > Oh you are right, we should always add the OUTPUTDIR to both maps regardless > of if it's internal to the WORKSPACE_ROOT or not. Paths relative to the > OUTPUTDIR should be stable across builds in different output directories, but > paths inside the OUTPUTDIR, expressed as relative to the WORKSPACE_ROOT will > vary. > I have not thought this fully through, but just throwing out a question: > would this work even if one build directory were inside the workspace root, > and the other not? Yes, I have tested that scenario as well, and does work. It was in fact the scenario in which I discovered the problem in the first place. - PR Review Comment: https://git.openjdk.org/jdk/pull/18009#discussion_r1503805898
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v3]
On Mon, 26 Feb 2024 20:21:55 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. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Rename LANG to LINK_TYPE can we get rid of LDCXX? On my system LDCXX is mapped to `g++` and LD is `gcc`; I searched for the differences, and the only thing I could find is that `g++` implicitly adds `-lstdc++ -shared-libgcc`; I suppose we could explicitly add these options, and use gcc everywhere. See: https://stackoverflow.com/a/172592 https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1965991536
