Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v4]
On Tue, 5 Mar 2024 14:19:14 GMT, Erik Joelsson wrote: > Does the release note also looks ok? Yes. > Is there any process to formally review release notes? No. Typically just add a comment to the RN sub-task indicating it is okay. My comment, that I had made some minor changes was meant as an indicator of that. - PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-1980229076
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 16:13:14 GMT, Jan Lahoda wrote: >> I see. I then misunderstood a part of the PR description. > > I believe we could technically reuse the list for boot/platform modules. But, > the intent here is to provide more control over the modules with native > access, not only being able to add to the list, but also remove from the > list. So, to me, it seemed better to have an explicit list, from/to which we > can remove/add modules easily. Hello Jan, my suggestion was based on a misunderstanding that NATIVE_ACCESS_MODULES is always a superset of boot and platform modules. What you currently have looks fine to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513674172
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 18:43:44 GMT, Alan Bateman wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Apply suggestions from code review >> >> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> >> Co-authored-by: Maurizio Cimadamore >> <54672762+mcimadam...@users.noreply.github.com> > > make/conf/module-loader-map.conf line 139: > >> 137: jdk.unsupported \ >> 138: jdk.xml.dom \ >> 139: jdk.zipfs \ > > We should create a JBS issue to prune this. Many of these modules do not need native access in the current implementation. Should this list be trimmed to list the ones that need native access in the current implementation? - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513588480
Integrated: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries
On Wed, 28 Feb 2024 19:29:13 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". This pull request has now been integrated. Changeset: 721bfee5 Author:Erik Joelsson URL: https://git.openjdk.org/jdk/commit/721bfee53af5bc2e2d67eebc7b82f09a642d5309 Stats: 22 lines in 2 files changed: 8 ins; 3 del; 11 mod 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries Reviewed-by: ihse, dholmes - PR: https://git.openjdk.org/jdk/pull/18050
Integrated: 8174269: Remove COMPAT locale data provider from JDK
On Fri, 23 Feb 2024 21:24:10 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. This pull request has now been integrated. Changeset: 809995b5 Author:Naoto Sato URL: https://git.openjdk.org/jdk/commit/809995b526ea79e4fd9fd4f911bcce811f77eb89 Stats: 67953 lines in 568 files changed: 483 ins; 66704 del; 766 mod 8174269: Remove COMPAT locale data provider from JDK Reviewed-by: ihse, joehw - PR: https://git.openjdk.org/jdk/pull/17991
Re: RFR: JDK-8298405: Implement JEP 467: Markdown Documentation Comments [v44]
On Mon, 4 Mar 2024 15:16:32 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 ``.)
The Markdown aspect of this is behaving about as well as can be expected. But
the generated form is certainly confusing and can be considered a
less-than-optimal presentation of a paragraph within a definition list. You
can see the same behavior in a traditional comment with `@param
hello`. Ideally, we could/should tweak the CSS for this situation, in a
separate changeset.
-
PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1979452615
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 12:44:10 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.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply suggestions from code review
>
> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
I assume you'll bump the copyright header on all files before integrating.
make/conf/module-loader-map.conf line 139:
> 137: jdk.unsupported \
> 138: jdk.xml.dom \
> 139: jdk.zipfs \
We should create a JBS issue to prune this.
src/java.base/share/classes/java/lang/ModuleLayer.java line 896:
> 894: return nameToModule.get(name);
> 895: }
> 896:
What would you think about replacing this with addEnableNativeAccess(String
name) so it can be called by JLA. addEnableNativeAccess. The reason is that the
JLA methods are usually just calls to some non-public method but the changes
mean mean there is "core" in the JLA method that is not easy to find.
src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 812:
> 810: }
> 811:
> 812: private static void addEnableNativeAccess(ModuleLayer layer,
> Set moduleNames, boolean shouldWarn) {
The private methods in this class have a short comment to summarise what they
do.
-
PR Review: https://git.openjdk.org/jdk/pull/18106#pullrequestreview-1917862955
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513331594
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513345563
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513347180
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v7]
On Tue, 5 Mar 2024 17:50:09 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: > > Reflects review comments Marked as reviewed by joehw (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17991#pullrequestreview-1917852026
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 Approved. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18101#pullrequestreview-1917796573
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v6]
On Tue, 5 Mar 2024 06:44:52 GMT, Joe Wang wrote: >> 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/7faf5495...b771e303 > > 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. Thanks. Not only the sentence, the description itself was unfinished. Corrected. - PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1513248212
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v7]
> 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: Reflects review comments - Changes: - all: https://git.openjdk.org/jdk/pull/17991/files - new: https://git.openjdk.org/jdk/pull/17991/files/b771e303..78a5130e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17991&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17991&range=05-06 Stats: 13 lines in 3 files changed: 5 ins; 4 del; 4 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
Integrated: 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='...'. This pull request has now been integrated. Changeset: c6641c7d Author:Ludvig Janiuk Committer: Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/c6641c7d2d5fd3e085e39646c9a350eeb83e3c5b Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8326831: Clarify test harness control variables in make help Reviewed-by: erikj, gli, ihse - 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='...'. Marked as reviewed by ihse (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18028#pullrequestreview-1917514719
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 14:14:15 GMT, Jaikiran Pai wrote: >>> to make the intention clear as well as reduce the chances of missing some >>> boot or platform module in this NATIVE_ACCESS_MODULES? >> >> The list to be be granted native access is a subset, it shouldn't be granted >> all modules mapped to the boot or platform class loaders. > > I see. I then misunderstood a part of the PR description. I believe we could technically reuse the list for boot/platform modules. But, the intent here is to provide more control over the modules with native access, not only being able to add to the list, but also remove from the list. So, to me, it seemed better to have an explicit list, from/to which we can remove/add modules easily. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1513107379
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 Yes, the release note looks good. Is there any process to formally review release notes? - PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-197625
Re: RFR: 8326891: Prefer RPATH over RUNPATH for $ORIGIN rpaths in internal JDK binaries [v4]
On Tue, 5 Mar 2024 07:16:10 GMT, David Holmes wrote: > Thanks for the further explanation and adding the comment. > > LGTM. Thanks! Does the release note also looks ok? I understand it needs to be reviewed together with the PR. - PR Comment: https://git.openjdk.org/jdk/pull/18050#issuecomment-1978879864
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 14:01:55 GMT, Alan Bateman wrote:
>> make/conf/module-loader-map.conf line 95:
>>
>>> 93: #
>>> 94:
>>> 95: NATIVE_ACCESS_MODULES= \
>>
>> Hello Jan, does this make configuration allow for something like:
>>
>>
>> NATIVE_ACCESS_MODULES= ${BOOT_MODULES} \
>> ${PLATFORM_MODULES} \
>> foo.bar.additional.module
>>
>> (please ignore any syntax issues in that snippet)
>>
>> to make the intention clear as well as reduce the chances of missing some
>> boot or platform module in this NATIVE_ACCESS_MODULES?
>
>> to make the intention clear as well as reduce the chances of missing some
>> boot or platform module in this NATIVE_ACCESS_MODULES?
>
> The list to be be granted native access is a subset, it shouldn't be granted
> all modules mapped to the boot or platform class loaders.
I see. I then misunderstood a part of the PR description.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512894628
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 13:42:32 GMT, Jaikiran Pai wrote: > to make the intention clear as well as reduce the chances of missing some > boot or platform module in this NATIVE_ACCESS_MODULES? The list to be be granted native access is a subset, it shouldn't be granted all modules mapped to the boot or platform class loaders. - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512873855
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 12:44:10 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.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply suggestions from code review
>
> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
Some of the files updated in this PR need a copyright year update.
src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 817:
> 815: JLA.addEnableNativeAccessToAllUnnamed();
> 816: } else if (!JLA.addEnableNativeAccess(layer, name) &&
> shouldWarn) {
> 817: warnUnknownModule(ENABLE_NATIVE_ACCESS, name);
Should we instead warn irrespective of whether or not it's user configured
native access module name or a module name configured in JDK's build
configuration? Or are the build misconfiguration in the `NATIVE_ACCESS_MODULES`
expected to be caught much earlier in some other place?
-
PR Comment: https://git.openjdk.org/jdk/pull/18106#issuecomment-1978836356
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512869279
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 12:44:10 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.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Apply suggestions from code review
>
> Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com>
> Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
make/conf/module-loader-map.conf line 95:
> 93: #
> 94:
> 95: NATIVE_ACCESS_MODULES= \
Hello Jan, does this make configuration allow for something like:
NATIVE_ACCESS_MODULES= ${BOOT_MODULES} \
${PLATFORM_MODULES} \
foo.bar.additional.module
(please ignore any syntax issues in that snippet)
to make the intention clear as well as reduce the chances of missing some boot
or platform module in this NATIVE_ACCESS_MODULES?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512845758
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
On Tue, 5 Mar 2024 12:41:18 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. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Apply suggestions from code review > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> > Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> 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. Suggestion: - PR Review Comment: https://git.openjdk.org/jdk/pull/18106#discussion_r1512761636
Re: RFR: 8327218: Add an ability to specify modules which should have native access enabled [v2]
> 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. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/18106/files - new: https://git.openjdk.org/jdk/pull/18106/files/924e1aa0..6127044d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18106&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18106&range=00-01 Stats: 3 lines in 3 files changed: 0 ins; 1 del; 2 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: 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. It sounds like a good first step, yes. Let's push this and then clean out the xlc toolchain. - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1978480399
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Tue, 5 Mar 2024 09:10:50 GMT, Joachim Kern wrote: >>> > What does this mean? That you are not using xlc at all? Or is it clang >>> > but still with an xlc frontend, so all xlc flags etc need to stay? >>> >>> The `xlc` toolchain is for the compiler versions up to 16 (xlclang++); the >>> `clang` toolchain is for the compiler versions 17 (ibm-clang++_r) and >>> higher. For the 17 Compiler the frontend is `clang`-ish and we are using >>> the `clang` flags instead of the `xlc` flags. `toolchain.m4` decides on the >>> basis of the found compiler which toolchain to use as default. >>> >>> ``` >>> # On AIX the default toolchain depends on the installed (found) compiler >>> # xlclang++ -> xlc toolchain >>> # ibm-clang++_r -> clang toolchain >>> # The compiler is searched on the PATH and TOOLCHAIN_PATH >>> # xlclang++ has precedence over ibm-clang++_r if both are installed >>> ``` >>> >>> So, if we set the minimum compiler level for AIX to 17, we can remove the >>> xlc toolchain at all. We cannot remove every reference to xlc, because at >>> least some headers we still use the xlc version (globalDefinitions_xlc.hpp) >> >> This suggests there might be more that needs to be done here than simply >> updating TOOLCHAIN_MINIMUM_VERSION_xlc. I spent some time looking at the >> relevant code, but keep getting lost in the distinction between xlc and >> clang. >> Does updating that variable as proposed even work at all? >> >> I'm going to need some help from you aix-ppc maintainer folks. > >> > > What does this mean? That you are not using xlc at all? Or is it clang >> > > but still with an xlc frontend, so all xlc flags etc need to stay? >> > >> > >> > The `xlc` toolchain is for the compiler versions up to 16 (xlclang++); the >> > `clang` toolchain is for the compiler versions 17 (ibm-clang++_r) and >> > higher. For the 17 Compiler the frontend is `clang`-ish and we are using >> > the `clang` flags instead of the `xlc` flags. `toolchain.m4` decides on >> > the basis of the found compiler which toolchain to use as default. >> > ``` >> > # On AIX the default toolchain depends on the installed (found) compiler >> > # xlclang++ -> xlc toolchain >> > # ibm-clang++_r -> clang toolchain >> > # The compiler is searched on the PATH and TOOLCHAIN_PATH >> > # xlclang++ has precedence over ibm-clang++_r if both are installed >> > ``` >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > >> > So, if we set the minimum compiler level for AIX to 17, we can remove the >> > xlc toolchain at all. We cannot remove every reference to xlc, because at >> > least some headers we still use the xlc version (globalDefinitions_xlc.hpp) >> >> This suggests there might be more that needs to be done here than simply >> updating TOOLCHAIN_MINIMUM_VERSION_xlc. I spent some time looking at the >> relevant code, but keep getting lost in the distinction between xlc and >> clang. Does updating that variable as proposed even work at all? >> >> I'm going to need some help from you aix-ppc maintainer folks. > > As I already mentioned, This PR is just the start shot to remove the support > for the old xlc compilers below version 17. This means removing the xlc > toolchain support in a follow up PR by our team. This is feasible, because > the open xl compilers starting with version 17 are using the clang toolchain. > So, if this PR is through we feel empowered to remove the xlc toolchain. > Your current version still serves the purpose of disallowing usage of xlc 16 > and below. Users of the old compiler will get to know which compiler to use > at minimum. Users of any Open XL compilers are not affected because > "TOOLCHAIN_MINIMUM_VERSION_xlc" is not used for that AFAICS. But that's ok > IMHO because the minimum clang check will be in place and the supported > compilers are documented > (https://wiki.openjdk.org/display/Build/Supported+Build+Platforms). So, I > think this PR could get integrated and the old build pipeline get removed in > a separate PR. Do you agree @JoKern65? The non-use of `TOOLCHAIN_MINIMUM_VERSION_xlc` when using the version 17 compilers is one of the things that worried me. (So I did correctly figure out it wouldn't be used.) I think this means one could inadvertently use 17.1.0 rather than the desired requirement of 17.1.1. I think raising the minimum clang version to 13 (as proposed in https://github.com/openjdk/jdk/pull/17862) doesn't catch that either, since 17.1.0 is based on clang 13. But if you folks are okay with this as an interim step, then I will go ahead. - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1978476778
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 Marked as reviewed by shade (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18101#pullrequestreview-1916467258
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 I just discovered gcc `-Wzero-as-null-pointer-constant`. We might be able to make some quick progress on those legacy uses of 0 pointers. - PR Comment: https://git.openjdk.org/jdk/pull/18101#issuecomment-1978311253
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Tue, 5 Mar 2024 08:41:00 GMT, Kim Barrett wrote: > > > What does this mean? That you are not using xlc at all? Or is it clang > > > but still with an xlc frontend, so all xlc flags etc need to stay? > > > > > > The `xlc` toolchain is for the compiler versions up to 16 (xlclang++); the > > `clang` toolchain is for the compiler versions 17 (ibm-clang++_r) and > > higher. For the 17 Compiler the frontend is `clang`-ish and we are using > > the `clang` flags instead of the `xlc` flags. `toolchain.m4` decides on the > > basis of the found compiler which toolchain to use as default. > > ``` > > # On AIX the default toolchain depends on the installed (found) compiler > > # xlclang++ -> xlc toolchain > > # ibm-clang++_r -> clang toolchain > > # The compiler is searched on the PATH and TOOLCHAIN_PATH > > # xlclang++ has precedence over ibm-clang++_r if both are installed > > ``` > > > > > > > > > > > > > > > > > > > > > > > > So, if we set the minimum compiler level for AIX to 17, we can remove the > > xlc toolchain at all. We cannot remove every reference to xlc, because at > > least some headers we still use the xlc version (globalDefinitions_xlc.hpp) > > This suggests there might be more that needs to be done here than simply > updating TOOLCHAIN_MINIMUM_VERSION_xlc. I spent some time looking at the > relevant code, but keep getting lost in the distinction between xlc and > clang. Does updating that variable as proposed even work at all? > > I'm going to need some help from you aix-ppc maintainer folks. As I already mentioned, This PR is just the start shot to remove the support for the old xlc compilers below version 17. This means removing the xlc toolchain support in a follow up PR by our team. This is feasible, because the open xl compilers starting with version 17 are using the clang toolchain. So, if this PR is through we feel empowered to remove the xlc toolchain. - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1978279887
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. Your current version still serves the purpose of disallowing usage of xlc 16 and below. Users of the old compiler will get to know which compiler to use at minimum. Users of any Open XL compilers are not affected because "TOOLCHAIN_MINIMUM_VERSION_xlc" is not used for that AFAICS. But that's ok IMHO because the minimum clang check will be in place and the supported compilers are documented (https://wiki.openjdk.org/display/Build/Supported+Build+Platforms). So, I think this PR could get integrated and the old build pipeline get removed in a separate PR. Do you agree @JoKern65? - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1978274678
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Thu, 15 Feb 2024 11:10:32 GMT, Joachim Kern wrote: > > What does this mean? That you are not using xlc at all? Or is it clang but > > still with an xlc frontend, so all xlc flags etc need to stay? > > The `xlc` toolchain is for the compiler versions up to 16 (xlclang++); the > `clang` toolchain is for the compiler versions 17 (ibm-clang++_r) and higher. > For the 17 Compiler the frontend is `clang`-ish and we are using the `clang` > flags instead of the `xlc` flags. `toolchain.m4` decides on the basis of the > found compiler which toolchain to use as default. > > ``` > # On AIX the default toolchain depends on the installed (found) compiler > # xlclang++ -> xlc toolchain > # ibm-clang++_r -> clang toolchain > # The compiler is searched on the PATH and TOOLCHAIN_PATH > # xlclang++ has precedence over ibm-clang++_r if both are installed > ``` > > So, if we set the minimum compiler level for AIX to 17, we can remove the xlc > toolchain at all. We cannot remove every reference to xlc, because at least > some headers we still use the xlc version (globalDefinitions_xlc.hpp) This suggests there might be more that needs to be done here than simply updating TOOLCHAIN_MINIMUM_VERSION_xlc. I spent some time looking at the relevant code, but keep getting lost in the distinction between xlc and clang. Does updating that variable as proposed even work at all? I'm going to need some help from you aix-ppc maintainer folks. - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1978227259
Re: RFR: 8325878: Require minimum Clang version 13
On Tue, 5 Mar 2024 07:20:33 GMT, Kim Barrett wrote: > 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. Well, no I'm not. I don't want to integrate this until the minimum aix-ppc toolchain has been updated. I don't know that there would be a problem, but I don't know there wouldn't be either. - PR Comment: https://git.openjdk.org/jdk/pull/17862#issuecomment-1978199756
