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. Thanks for reviews/responses. I'll go ahead with integration. We won't be reliant on the new version immediately, so we can still reconsider if it causes someone problems and they bring it up soon-ish. - PR Comment: https://git.openjdk.org/jdk/pull/17862#issuecomment-1978107784
Re: RFR: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL [v2]
On Tue, 5 Mar 2024 07:12:09 GMT, Kim Barrett wrote: >> Please review this change to update the HotSpot Style Guide's discussion of >> nullptr and its use. >> >> I suggest this is an editorial rather than substantive change to the style >> guide. As such, the normal HotSpot PR process can be used for this change. > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > respond to shipilev comments Current revised text looks good to me. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18101#pullrequestreview-1916033052
Integrated: 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. This pull request has now been integrated. Changeset: d6f2a174 Author:Kim Barrett URL: https://git.openjdk.org/jdk/commit/d6f2a174fcf41f0b091ef7eabea5d06fae90e0b2 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod 8325881: Require minimum gcc version 10 Reviewed-by: ihse, shade - PR: https://git.openjdk.org/jdk/pull/17899
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. Thanks for reviews/responses. I'll go ahead with integration. We won't be reliant on the newer version immediately, so we can still reconsider if it causes someone problems and they bring it up soon-ish. - PR Comment: https://git.openjdk.org/jdk/pull/17899#issuecomment-1978101584
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v4]
On Mon, 4 Mar 2024 23:44:14 GMT, Erik Joelsson wrote: >> Executables and dynamic libraries on Linux can encode a search path that the >> dynamic linker will use when looking up library dependencies, generally >> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature >> to set search paths relative to the location of the binary itself. Typically >> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find >> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to >> find each other. >> >> There are two different types of such rpaths, RPATH and RUNPATH. The former >> is the earlier incantation but RUNPATH has been around since about 2003 and >> has been default in prominent Linux distros for a long time, and now also >> seems to be default in the linker directly from binutils. The toolchain used >> by Oracle defaulted to RPATH until at least JDK 11, but since then with some >> toolchain upgrade, the default was flipped to RUNPATH. >> >> The main (relevant in this case) difference between the two is that RPATH is >> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is >> only considered after LD_LIBRARY_PATH. For libraries that are part of a >> Linux distribution, letting users, or the system, control and override >> builtin rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. >> However, for the JDK, there really is no usecase for having an externally >> configured LD_LIBRARY_PATH potentially getting in the way of the JDK >> libraries finding each other correctly. If a user environment sets >> LD_LIBRARY_PATH, and there is a library in that path with the same name as a >> JDK library (e.g. libnet.so or some other generically named library) that >> external library will be loaded instead of the JDK internal library and that >> is basically guaranteed to break the JDK. There is no supported usecase that >> I can think of for injecting other versions of such libraries in a JDK >> distribution. >> >> I propose that we explicitly configure the JDK build to set RPATH instead of >> RUNPATH for Linux binaries. This is done with the linker flag >> "--disable-new-dtags". > > Erik Joelsson has updated the pull request incrementally with one additional > commit since the last revision: > > bug ref Thanks for the further explanation and adding the comment. LGTM. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18050#pullrequestreview-1916027301
Re: RFR: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL [v2]
On Mon, 4 Mar 2024 09:52:15 GMT, Aleksey Shipilev wrote: >> Kim Barrett has updated the pull request incrementally with one additional >> commit since the last revision: >> >> respond to shipilev comments > > doc/hotspot-style.md line 738: > >> 736: expressions with value zero. C++14 replaced that with integer literals >> with >> 737: value zero. Some compilers continue to treat a zero-valued integral >> constant >> 738: expressions as a _null pointer constant_ even in C++14 mode. > > It is not very clear why should this concern any Hotspot developers? If one is familiar with C++14 (or later) or were to look up _null pointer constant_ in the C++14 standard, one might wonder why this style guide admonishes against using non-literal constant expressions for such. But I've rewritten the part about 0 as a pointer to (I think) make things clearer. - PR Review Comment: https://git.openjdk.org/jdk/pull/18101#discussion_r1512225238
Re: RFR: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL [v2]
On Mon, 4 Mar 2024 18:01:35 GMT, Vladimir Kozlov wrote: >> I think it would be enough to write 1..2 sentences about this, and then >> defer to N2431 already linked here for more details. > > I agree with Aleksey. Good point. I decided just referring to the paper for rationale is sufficient. - PR Review Comment: https://git.openjdk.org/jdk/pull/18101#discussion_r1512223990
Re: RFR: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL [v2]
> Please review this change to update the HotSpot Style Guide's discussion of > nullptr and its use. > > I suggest this is an editorial rather than substantive change to the style > guide. As such, the normal HotSpot PR process can be used for this change. Kim Barrett has updated the pull request incrementally with one additional commit since the last revision: respond to shipilev comments - Changes: - all: https://git.openjdk.org/jdk/pull/18101/files - new: https://git.openjdk.org/jdk/pull/18101/files/fdfcdc58..13dc01ac Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18101&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18101&range=00-01 Stats: 17 lines in 2 files changed: 0 ins; 5 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/18101.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18101/head:pull/18101 PR: https://git.openjdk.org/jdk/pull/18101
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v6]
On Mon, 4 Mar 2024 19:07:44 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 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 44 additional commits since > the last revision: > > - Merge branch 'master' into JDK-8174269-COMPAT-Removal > - Addressing review comments > - Update test/jdk/java/text/Format/DateFormat/Bug6683975.java > >Co-authored-by: Justin Lu > - Remove `GensrcLocaleData.gmk` > - Update make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java > >Co-authored-by: Andrey Turbanov > - cleanup > - BreakIteratorProvider available locales fix > - clean-up > - test fixes > - makeZoneData.pl fix > - ... and 34 more: https://git.openjdk.org/jdk/compare/9da59104...b771e303 LGTM. This is a lot of work. Looking through the files alone takes hours. Kudos to the great work! I kind of like some of the date formats in COMPACT to be honest :-) make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java line 1320: > 1318: * "USdst" -> "D" > 1319: * > 1320: * `tzdbLinks` retains `Link`s of time zones. if the value nit, "if the value" seems to be an unfinished sentence. src/java.base/share/classes/sun/util/locale/provider/BaseLocaleDataMetaInfo.java line 31: > 29: * It is used to avoid loading non-existent localized resources so that > 30: * jar files won't be opened unnecessary to look up them. > 31: */ nit: move the class description to right above the class? "unnecessary" -> "unnecessarily" - Marked as reviewed by joehw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17991#pullrequestreview-1915980264 PR Comment: https://git.openjdk.org/jdk/pull/17991#issuecomment-1978071713 PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1512206671 PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1512207766
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v4]
> Executables and dynamic libraries on Linux can encode a search path that the > dynamic linker will use when looking up library dependencies, generally > referred to as an "rpath". In the JDK we use this with the $ORIGIN feature to > set search paths relative to the location of the binary itself. Typically > executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find > libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to > find each other. > > There are two different types of such rpaths, RPATH and RUNPATH. The former > is the earlier incantation but RUNPATH has been around since about 2003 and > has been default in prominent Linux distros for a long time, and now also > seems to be default in the linker directly from binutils. The toolchain used > by Oracle defaulted to RPATH until at least JDK 11, but since then with some > toolchain upgrade, the default was flipped to RUNPATH. > > The main (relevant in this case) difference between the two is that RPATH is > considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is > only considered after LD_LIBRARY_PATH. For libraries that are part of a Linux > distribution, letting users, or the system, control and override builtin > rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. However, > for the JDK, there really is no usecase for having an externally configured > LD_LIBRARY_PATH potentially getting in the way of the JDK libraries finding > each other correctly. If a user environment sets LD_LIBRARY_PATH, and there > is a library in that path with the same name as a JDK library (e.g. libnet.so > or some other generically named library) that external library will be loaded > instead of the JDK internal library and that is basically guaranteed to break > the JDK. There is no supported usecase that I can think of for injecting > other versions of such libraries in a JDK distribution. > > I propose that we explicitly configure the JDK build to set RPATH instead of > RUNPATH for Linux binaries. This is done with the linker flag > "--disable-new-dtags". Erik Joelsson has updated the pull request incrementally with one additional commit since the last revision: bug ref - Changes: - all: https://git.openjdk.org/jdk/pull/18050/files - new: https://git.openjdk.org/jdk/pull/18050/files/faafef8a..8639eee5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18050&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18050&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18050.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18050/head:pull/18050 PR: https://git.openjdk.org/jdk/pull/18050
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v45]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 63 commits: - fix test after merge - Merge remote-tracking branch 'upstream/master' into 8298405.doclet-markdown-v3 - Revise `Markdown.update` to better handle container blocks. - Refactor most of TestMarkdown.java into separate tests, grouped by functionality - add support for (top-level) trailing code blocks add simple test case with tabs add TestMarkdownCodeBlocks.testTypical - fix whitespace - fix Markdown.update for new POST_LIST_INDENT test case given in review feedback - refactor tests for Markdown headings into a separate test class. - update DocCommentParser and tests to improve handling of code blocks and code spans in Markdown documentation comments - fix indentation, for consistency - ... and 53 more: https://git.openjdk.org/jdk/compare/6f8d351e...292ff0f1 - Changes: https://git.openjdk.org/jdk/pull/16388/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=44 Stats: 23548 lines in 205 files changed: 22863 ins; 252 del; 433 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: 8174269: Remove COMPAT locale data provider from JDK [v6]
> 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 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 44 additional commits since the last revision: - Merge branch 'master' into JDK-8174269-COMPAT-Removal - Addressing review comments - Update test/jdk/java/text/Format/DateFormat/Bug6683975.java Co-authored-by: Justin Lu - Remove `GensrcLocaleData.gmk` - Update make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java Co-authored-by: Andrey Turbanov - cleanup - BreakIteratorProvider available locales fix - clean-up - test fixes - makeZoneData.pl fix - ... and 34 more: https://git.openjdk.org/jdk/compare/fe877cfd...b771e303 - Changes: - all: https://git.openjdk.org/jdk/pull/17991/files - new: https://git.openjdk.org/jdk/pull/17991/files/d5953788..b771e303 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17991&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17991&range=04-05 Stats: 19239 lines in 1290 files changed: 10762 ins; 3853 del; 4624 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: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
On Mon, 4 Mar 2024 09:51:16 GMT, Aleksey Shipilev wrote: >> doc/hotspot-style.md line 730: >> >>> 728: Use `nullptr` >>> 729: >>> ([n2431](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2431.pdf)) >>> 730: rather than `NULL`. Don't use (constant expression or literal) 0 for >>> pointers. >> >> Should there be any discussion here of why we want to avoid `NULL`? >> Specifically the varying >> definitions and the pitfalls of `NULL` in varargs context. Also template >> and overload issues. > > I think it would be enough to write 1..2 sentences about this, and then defer > to N2431 already linked here for more details. I agree with Aleksey. - PR Review Comment: https://git.openjdk.org/jdk/pull/18101#discussion_r1511561200
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled
On Mon, 4 Mar 2024 13:52:13 GMT, Jan Lahoda wrote: > Currently, JDK modules load by the bootstrap and platform ClassLoaders are > automatically granted the native access. I am working on an upgrade of JLine > inside the `jdk.internal.le` module, and I would like to replace the current > native bindings with FFM-based bindings (which are now somewhat provided by > JLine). But, for that, native access is needed for the `jdk.internal.le` > module. We could possibly move the module to the platform ClassLoader, but it > seems it might be better to have more control over which modules have the > native access. > > This patch introduces an explicit list of modules that will automatically be > granted the native access. Note this patch is not yet intended to change the > end behavior - the list of modules granted native access is supposed to be > the same as modules in the boot and platform ClassLoaders. src/java.base/share/classes/java/lang/ModuleLayer.java line 885: > 883: > 884: /** > 885: * Returns the module with the given name in this later only. Suggestion: * Returns the module with the given name in this layer only. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1511542904
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 One little side note, I tried now also on a Linux x86 system and saw the same issue, so it is not limited to macOS or aarch64. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976999445
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v44]
On Mon, 4 Mar 2024 14:40:06 GMT, Pavel Rappo wrote:
> I have a test case to report. The following results in no `@param`
> information being rendered, which I think is a bug:
>
> ```
> /// Hello, _Markdown_ world!
> ///
> ///
> /// @param hello
> ///
> ///
> public class C { }
> ```
Scratch that. After experimenting a bit more with **traditional** javadoc
comments, I realised that it might be expected that `@param` would be lost in
that `...` block; okay.
Still, I find it surprising that the description of the `@param` tag in the
above comment, hosted in `///` or `/**...*/`, is a single node of type
`RawText` with the following content:
hello
(Note, the content includes ``.)
-
PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1976818803
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Mon, 4 Mar 2024 11:45:59 GMT, Julian Waters wrote: > I do hope that dropping ccache support isn't something that's planned though > :( Not really. The benefit of dropping it is quite small, and there might be use cases where it helps. But I think we should perhaps be more explicit in the documentation that it is not obvious that it helps performance, and encourage testing before enabling it. And perhaps even print a warning in configure if you enable it that you need to check that it helps. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976777022
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v3]
On Mon, 4 Mar 2024 14:57:44 GMT, Erik Joelsson wrote: >> Executables and dynamic libraries on Linux can encode a search path that the >> dynamic linker will use when looking up library dependencies, generally >> referred to as an "rpath". In the JDK we use this with the $ORIGIN feature >> to set search paths relative to the location of the binary itself. Typically >> executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find >> libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to >> find each other. >> >> There are two different types of such rpaths, RPATH and RUNPATH. The former >> is the earlier incantation but RUNPATH has been around since about 2003 and >> has been default in prominent Linux distros for a long time, and now also >> seems to be default in the linker directly from binutils. The toolchain used >> by Oracle defaulted to RPATH until at least JDK 11, but since then with some >> toolchain upgrade, the default was flipped to RUNPATH. >> >> The main (relevant in this case) difference between the two is that RPATH is >> considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is >> only considered after LD_LIBRARY_PATH. For libraries that are part of a >> Linux distribution, letting users, or the system, control and override >> builtin rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. >> However, for the JDK, there really is no usecase for having an externally >> configured LD_LIBRARY_PATH potentially getting in the way of the JDK >> libraries finding each other correctly. If a user environment sets >> LD_LIBRARY_PATH, and there is a library in that path with the same name as a >> JDK library (e.g. libnet.so or some other generically named library) that >> external library will be loaded instead of the JDK internal library and that >> is basically guaranteed to break the JDK. There is no supported usecase that >> I can think of for injecting other versions of such libraries in a JDK >> distribution. >> >> I propose that we explicitly configure the JDK build to set RPATH instead of >> RUNPATH for Linux binaries. This is done with the linker flag >> "--disable-new-dtags". > > Erik Joelsson has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment Marked as reviewed by ihse (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18050#pullrequestreview-1914533756
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v2]
On Mon, 4 Mar 2024 14:53:21 GMT, Magnus Ihse Bursie wrote: >> make/autoconf/flags-cflags.m4 line 40: >> >>> 38: # Default works for linux, might work on other platforms as well. >>> 39: SHARED_LIBRARY_FLAGS='-shared' >>> 40: SET_EXECUTABLE_ORIGIN='-Wl,-rpath,\$$ORIGIN[$]1 >>> -Wl,--disable-new-dtags' >> >> The reason we use `--disable-new-dtags` needs to be documented somewhere. > > I agree. A short description and a reference to this bug number would be nice. Added comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/18050#discussion_r1511291650
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v2]
On Sun, 3 Mar 2024 22:12:19 GMT, David Holmes wrote: >> Erik Joelsson has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Use @requires in test > > make/autoconf/flags-cflags.m4 line 40: > >> 38: # Default works for linux, might work on other platforms as well. >> 39: SHARED_LIBRARY_FLAGS='-shared' >> 40: SET_EXECUTABLE_ORIGIN='-Wl,-rpath,\$$ORIGIN[$]1 >> -Wl,--disable-new-dtags' > > The reason we use `--disable-new-dtags` needs to be documented somewhere. I agree. A short description and a reference to this bug number would be nice. - PR Review Comment: https://git.openjdk.org/jdk/pull/18050#discussion_r1511290973
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v3]
> Executables and dynamic libraries on Linux can encode a search path that the > dynamic linker will use when looking up library dependencies, generally > referred to as an "rpath". In the JDK we use this with the $ORIGIN feature to > set search paths relative to the location of the binary itself. Typically > executables in the bin/ directory have the rpath "$ORIGIN/../lib" to find > libjli.so. Most of the libraries in lib/ have rpath set to just "$ORIGIN" to > find each other. > > There are two different types of such rpaths, RPATH and RUNPATH. The former > is the earlier incantation but RUNPATH has been around since about 2003 and > has been default in prominent Linux distros for a long time, and now also > seems to be default in the linker directly from binutils. The toolchain used > by Oracle defaulted to RPATH until at least JDK 11, but since then with some > toolchain upgrade, the default was flipped to RUNPATH. > > The main (relevant in this case) difference between the two is that RPATH is > considered before the LD_LIBRARY_PATH environment variable, while RUNPATH is > only considered after LD_LIBRARY_PATH. For libraries that are part of a Linux > distribution, letting users, or the system, control and override builtin > rpaths with LD_LIBRARY_PATH seems like a reasonable thing to prefer. However, > for the JDK, there really is no usecase for having an externally configured > LD_LIBRARY_PATH potentially getting in the way of the JDK libraries finding > each other correctly. If a user environment sets LD_LIBRARY_PATH, and there > is a library in that path with the same name as a JDK library (e.g. libnet.so > or some other generically named library) that external library will be loaded > instead of the JDK internal library and that is basically guaranteed to break > the JDK. There is no supported usecase that I can think of for injecting > other versions of such libraries in a JDK distribution. > > I propose that we explicitly configure the JDK build to set RPATH instead of > RUNPATH for Linux binaries. This is done with the linker flag > "--disable-new-dtags". Erik Joelsson has updated the pull request incrementally with one additional commit since the last revision: Add comment - Changes: - all: https://git.openjdk.org/jdk/pull/18050/files - new: https://git.openjdk.org/jdk/pull/18050/files/592281f2..faafef8a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18050&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18050&range=01-02 Stats: 4 lines in 1 file changed: 3 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18050.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18050/head:pull/18050 PR: https://git.openjdk.org/jdk/pull/18050
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v2]
On Sun, 3 Mar 2024 22:09:51 GMT, David Holmes wrote: > I find it somewhat odd that we seem to be add odds with the general > programming community when it comes to this behaviour. Giving precedence to > `LD_LIBRARY_PATH` is the new behaviour, enabled via `--enable-new-dtags`; and > at some point this was made the default behaviour so it must be considered > generally desirable. But we have now decided we do not want this behaviour so > we have to explicitly disable it via `--disable-new-dtags`. Your point here is the reason it took me this long to come to the realization that my proposal here is correct for a product like the JDK. I have been dealing with these rpaths several time over the years and have been confused over the significance of the two kinds, but figured that since RUNPATH is the new variant and the general move is towards that, we should just stick to that too. However, as I allude to in the bug description, we aren't really fitting the usecases the default is aiming to solve. If you are building a single library for distribution, or to install in your local Linux distribution, then letting the user have the option to customize the dynamic loader with LD_LIBRARY_PATH by default seems reasonable. I can see why Linux distributions prefer having LD_LIBRARY_PATH available to override rpaths as well. They want to promote applications that are well integrated with the system and using the system versions of all dependency libraries instead of bundling their own competing versions. However, what we are building is more of a self contained application. When users jlink an application, it becomes even more obvious. RPATH allows us to completely self contain the dependencies between our _internal_ native libraries. Any external dependency that the JDK has (glibc etc) we still link to from the system the same way as before. Looking for discussions about RPATH and RUNPATH online, this is basically the conclusion I find. A better solution would perhaps have been to name our internal libraries better, using a namespace that would make name clashes with other external libraries less likely (e.g. libjavanet.so instead of libnet.so). If we had done that, then this issue would probably not have been worth raising. - PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-1976750074
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v44]
On Fri, 1 Mar 2024 21:49:29 GMT, Jonathan Gibbons wrote:
>> 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:
>
> Revise `Markdown.update` to better handle container blocks.
I have a test case to report. The following results in no `@param` information
being rendered, which I think is a bug:
/// Hello, _Markdown_ world!
///
///
/// @param hello
///
///
public class C { }
-
PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1976734010
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled
On Mon, 4 Mar 2024 13:52:13 GMT, Jan Lahoda wrote: > Currently, JDK modules load by the bootstrap and platform ClassLoaders are > automatically granted the native access. I am working on an upgrade of JLine > inside the `jdk.internal.le` module, and I would like to replace the current > native bindings with FFM-based bindings (which are now somewhat provided by > JLine). But, for that, native access is needed for the `jdk.internal.le` > module. We could possibly move the module to the platform ClassLoader, but it > seems it might be better to have more control over which modules have the > native access. > > This patch introduces an explicit list of modules that will automatically be > granted the native access. Note this patch is not yet intended to change the > end behavior - the list of modules granted native access is supposed to be > the same as modules in the boot and platform ClassLoaders. Build changes look good. - Marked as reviewed by erikj (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1914434715
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled
On Mon, 4 Mar 2024 13:52:13 GMT, Jan Lahoda wrote:
> Currently, JDK modules load by the bootstrap and platform ClassLoaders are
> automatically granted the native access. I am working on an upgrade of JLine
> inside the `jdk.internal.le` module, and I would like to replace the current
> native bindings with FFM-based bindings (which are now somewhat provided by
> JLine). But, for that, native access is needed for the `jdk.internal.le`
> module. We could possibly move the module to the platform ClassLoader, but it
> seems it might be better to have more control over which modules have the
> native access.
>
> This patch introduces an explicit list of modules that will automatically be
> granted the native access. Note this patch is not yet intended to change the
> end behavior - the list of modules granted native access is supposed to be
> the same as modules in the boot and platform ClassLoaders.
Looks good.
src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 273:
> 271:
> 272: /**
> 273: * Updates module named name in layer layer to allow access to
> restricted methods.
Suggestion:
* Updates module named {@code name} in layer {@code layer} to allow access
to restricted methods.
src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 806:
> 804: /**
> 805: * Process the --enable-native-access option to grant access to
> restricted methods to selected modules.
> 806: * Also add Enable native access from JDK modules.
I don't think this extra comment is needed.
-
Marked as reviewed by mcimadamore (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1914419463
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1511226929
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1511223692
RFR: 8327218: Add an ability to specify modules which should have native access enabled
Currently, JDK modules load by the bootstrap and platform ClassLoaders are automatically granted the native access. I am working on an upgrade of JLine inside the `jdk.internal.le` module, and I would like to replace the current native bindings with FFM-based bindings (which are now somewhat provided by JLine). But, for that, native access is needed for the `jdk.internal.le` module. We could possibly move the module to the platform ClassLoader, but it seems it might be better to have more control over which modules have the native access. This patch introduces an explicit list of modules that will automatically be granted the native access. Note this patch is not yet intended to change the end behavior - the list of modules granted native access is supposed to be the same as modules in the boot and platform ClassLoaders. - Commit messages: - Explicitly listing the modules that should get native access. - Native access modules-1 Changes: https://git.openjdk.org/jdk/pull/18106/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18106&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327218 Stats: 120 lines in 9 files changed: 103 ins; 9 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18106.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18106/head:pull/18106 PR: https://git.openjdk.org/jdk/pull/18106
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Mon, 4 Mar 2024 12:58:28 GMT, Claudio Nieder wrote: > > Could I trouble you to mention what exactly was different? > > No trouble at all. > > `CCACHE_BASEDIR=/tmp/bceaed6d/jdk/`is completely missing. (The directory is > where I checked out OpenJDK) > > `CCACHE_SLOPPINESS` has the value `pch_defines,time_macros` with the working > parent commit but just `pch_defines` with this commit. I.e. > `CCACHE_SLOPPINESS=pch_defines,time_macros` vs > `CCACHE_SLOPPINESS=pch_defines`. Thanks! Seems curious to me, off the top of my head I can't seem to discern why these would change, perhaps it's time for a little investigating - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976534197
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Mon, 4 Mar 2024 11:45:59 GMT, Julian Waters wrote: > Could I trouble you to mention what exactly was different? No trouble at all. `CCACHE_BASEDIR=/tmp/bceaed6d/jdk/`is completely missing. (The directory is where I checked out OpenJDK) `CCACHE_SLOPPINESS` has the value `pch_defines,time_macros` with the working parent commit but just `pch_defines` with this commit. I.e. `CCACHE_SLOPPINESS=pch_defines,time_macros` vs `CCACHE_SLOPPINESS=pch_defines`. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976528283
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Mon, 4 Mar 2024 11:28:02 GMT, Magnus Ihse Bursie wrote: > > I wonder if it's the SetupToolchain changes that has caused this. ccache is > > collapsed into CC and CXX to my knowledge > > Yeah, it must have been. Would you like to take a look at it? Otherwise I'll > file a bug and fix it. Breaking ccache was an unintended regression, so it > needs to be fixed. If we want to drop ccache support it needs to be done > explicitly and separately. No problem! I do hope that dropping ccache support isn't something that's planned though :( > Additionally some environment variables are missing/different. Could I trouble you to mention what exactly was different? - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976398726
Re: RFR: 8326583: Remove over-generalized DefineNativeToolchain solution [v4]
On Mon, 4 Mar 2024 06:42:30 GMT, Julian Waters wrote: > I wonder if it's the SetupToolchain changes that has caused this. ccache is > collapsed into CC and CXX to my knowledge Yeah, it must have been. Would you like to take a look at it? Otherwise I'll file a bug and fix it. Breaking ccache was an unintended regression, so it needs to be fixed. If we want to drop ccache support it needs to be done explicitly and separately. - PR Comment: https://git.openjdk.org/jdk/pull/17986#issuecomment-1976368808
Re: RFR: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
On Mon, 4 Mar 2024 08:38:09 GMT, Kim Barrett wrote: > Please review this change to update the HotSpot Style Guide's discussion of > nullptr and its use. > > I suggest this is an editorial rather than substantive change to the style > guide. As such, the normal HotSpot PR process can be used for this change. Looks okay, but questions/suggestions: doc/hotspot-style.md line 738: > 736: expressions with value zero. C++14 replaced that with integer literals > with > 737: value zero. Some compilers continue to treat a zero-valued integral > constant > 738: expressions as a _null pointer constant_ even in C++14 mode. It is not very clear why should this concern any Hotspot developers? - PR Review: https://git.openjdk.org/jdk/pull/18101#pullrequestreview-1913874746 PR Review Comment: https://git.openjdk.org/jdk/pull/18101#discussion_r1510891459
Re: RFR: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
On Mon, 4 Mar 2024 08:41:46 GMT, Kim Barrett wrote: >> Please review this change to update the HotSpot Style Guide's discussion of >> nullptr and its use. >> >> I suggest this is an editorial rather than substantive change to the style >> guide. As such, the normal HotSpot PR process can be used for this change. > > doc/hotspot-style.md line 730: > >> 728: Use `nullptr` >> 729: >> ([n2431](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2431.pdf)) >> 730: rather than `NULL`. Don't use (constant expression or literal) 0 for >> pointers. > > Should there be any discussion here of why we want to avoid `NULL`? > Specifically the varying > definitions and the pitfalls of `NULL` in varargs context. Also template and > overload issues. I think it would be enough to write 1..2 sentences about this, and then defer to N2431 already linked here for more details. - PR Review Comment: https://git.openjdk.org/jdk/pull/18101#discussion_r1510890143
RFR: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
Please review this change to update the HotSpot Style Guide's discussion of nullptr and its use. I suggest this is an editorial rather than substantive change to the style guide. As such, the normal HotSpot PR process can be used for this change. - Commit messages: - update nullptr usage Changes: https://git.openjdk.org/jdk/pull/18101/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18101&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8327173 Stats: 19 lines in 2 files changed: 10 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18101.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18101/head:pull/18101 PR: https://git.openjdk.org/jdk/pull/18101
Re: RFR: 8327173: HotSpot Style Guide needs update regarding nullptr vs NULL
On Mon, 4 Mar 2024 08:38:09 GMT, Kim Barrett wrote: > Please review this change to update the HotSpot Style Guide's discussion of > nullptr and its use. > > I suggest this is an editorial rather than substantive change to the style > guide. As such, the normal HotSpot PR process can be used for this change. doc/hotspot-style.md line 730: > 728: Use `nullptr` > 729: > ([n2431](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2431.pdf)) > 730: rather than `NULL`. Don't use (constant expression or literal) 0 for > pointers. Should there be any discussion here of why we want to avoid `NULL`? Specifically the varying definitions and the pitfalls of `NULL` in varargs context. Also template and overload issues. - PR Review Comment: https://git.openjdk.org/jdk/pull/18101#discussion_r1510773801
