Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods
On Tue, 18 Jun 2024 16:30:37 GMT, Jorn Vernee wrote: > This PR adds a new JDK tool, called `jnativescan`, that can be used to find > code that accesses native functionality. Currently this includes `native` > method declarations, and methods marked with `@Restricted`. > > The tool accepts a list of class path and module path entries through > `--class-path` and `--module-path`, and a set of root modules through > `--add-modules`, as well as an optional target release with `--release`. > > The default mode is for the tool to report all uses of `@Restricted` methods, > and `native` method declaration in a tree-like structure: > > > app.jar (ALL-UNNAMED): > main.Main: > main.Main::main(String[])void references restricted methods: > java.lang.foreign.MemorySegment::reinterpret(long)MemorySegment > main.Main::m()void is a native method declaration > > > The `--print-native-access` option can be used print out all the module names > of modules doing native access in a comma separated list. For class path > code, this will print out `ALL-UNNAMED`. > > Testing: > - `langtools_jnativescan` tests. > - Running the tool over jextract's libclang bindings, which use the FFM API, > and thus has a lot of references to `@Restricted` methods. > - tier 1-3 One more tool. or... Could this be coalesced into a tool that does deprscan and restricted methods, and other "lint" type checks? I might go so far as to suggest it be extensible and accept patterns or regular expressions for matching classes, methods, fields, etc. - PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2178860810
Re: RFR: 8325621: Improve jspawnhelper version checks [v4]
On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy 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 JDK-8325621-2 > - Update style and improve error messages > - Print to stdout instead of stderr > - Compare version using VERSION_STRING > - Code cleanup > - Remove string.h include > - Update test > - Pass version as integer instead of string > - Read in version using int instead of string > - 8325621: Improve jspawnhelper version checks Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1935071268
Re: RFR: 8325621: Improve jspawnhelper version checks [v2]
On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy wrote: >> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621) >> >> Updates jspawnhelper to check that JDK version and jspawnhelper version are >> the same. Updates test to include check for version. Also tested manually by >> replacing jspawnhelper with incorrect version to confirm that check works. > > Chad Rakoczy has updated the pull request incrementally with one additional > commit since the last revision: > > Code cleanup make/modules/java.base/Launcher.gmk line 85: > 83: -DVERSION_INTERIM=$(VERSION_INTERIM) \ > 84: -DVERSION_UPDATE=$(VERSION_UPDATE) \ > 85: -DVERSION_PATCH=$(VERSION_PATCH), \ Using all 4 is way overkill for the problem at hand. Just the FEATURE_VERSION is sufficient. We all know better than to make incompatible changes in minor versions let alone update or patch version. - PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520364358
Re: RFR: 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed
On Wed, 3 Jan 2024 12:09:42 GMT, Pavel Rappo wrote: > Please review this PR to update `blessed-modifier.order.sh` for the > `default`, `sealed`, `non-sealed`, and `strictfp` modifiers. > > In a [discussion][] preceding this PR, it was agreed that the script should > better refer to relevant JLS sections rather than to > [`java.lang.reflect.Modifier#toString`][], which does not model Java source. > > - the `sealed` and `non-sealed` class or interface modifiers were introduced > in JEP 409, but have never been accounted for > > - the `default` method modifier was introduced in Java 8, but has not been > explicitly accounted for. It's unclear whether it was an omission or a > conscious decision. The current edition of JLS [suggests] that in compilable, > modern and customary formatted code, `default` appears alone and, thus, is > always canonically ordered. > > However, a somewhat similar argument applies to the `public` modifier on an > interface method. Its use is discouraged, yet the script would enforce its > order if `public` appears not alone. > > So for completeness, this PR proposes to explicitly account for `default`. > > * While not proposing to do anyting about it, this PR recognises (for the > record) that `strictfp` class, interface, or method modifier became > [obsolete][] in JDK 17. > > I've run the updated script on JDK. The script found 8 missorted `sealed` and > 20 missorted `default`. The script also found 3 false positive `default`: > > - > https://github.com/openjdk/jdk/blob/249d553659ab75a2271e98c77e7d62f662ffa684/src/java.desktop/share/classes/java/awt/dnd/DragSource.java#L526-L527 > - > https://github.com/openjdk/jdk/blob/98da03af50e2372817a7b5e381eea5ee6f2cb919/src/java.management/share/classes/javax/management/MBeanServerFactory.java#L91 > - > https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.desktop/share/classes/javax/print/PrintServiceLookup.java#L193 > > None of those findings are addressed in this PR. > > [discussion]: > https://mail.openjdk.org/pipermail/core-libs-dev/2024-January/117347.html > [`java.lang.reflect.Modifier#toString`]: > https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int) > [suggests]: > https://docs.oracle.com/javase/specs/jls/se21/html/jls-9.html#jls-9.4 > [obsolete]: https://openjdk.org/jeps/306 Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17242#pullrequestreview-1802350655
Re: RFR: 8317979: Use TZ database style abbreviations in the CLDR locale provider [v3]
On Tue, 17 Oct 2023 16:52:12 GMT, Naoto Sato wrote: >> CLDR provides very few short names for time zones, such as PST/PDT. This >> will typically end up substituting names from the COMPAT provider. Once the >> COMPAT is removed, they will be displayed in the GMT format, i.e., >> GMT+XX:YY. Although some of the short names in the COMPAT provider are >> somewhat questionable (less common ones are simply made up from the long >> names by taking the initials), it would not be desirable for them to fall >> back to the GMT format. >> To mitigate the situation, CLDR can use the abbreviated names from the TZ >> database, which contains legacy (major) short names as FORMAT. The CLDR >> provider can use them instead of the GMT offset style. This enhancement is a >> precursor to the future removal of the COMPAT provider. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Delay populating GMT format at runtime. Reflecting review comments. Marked as reviewed by rriggs (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16206#pullrequestreview-1683418583
Re: RFR: 8317979: Use TZ database style abbreviations in the CLDR locale provider [v2]
On Mon, 16 Oct 2023 23:37:51 GMT, Naoto Sato wrote:
>> CLDR provides very few short names for time zones, such as PST/PDT. This
>> will typically end up substituting names from the COMPAT provider. Once the
>> COMPAT is removed, they will be displayed in the GMT format, i.e.,
>> GMT+XX:YY. Although some of the short names in the COMPAT provider are
>> somewhat questionable (less common ones are simply made up from the long
>> names by taking the initials), it would not be desirable for them to fall
>> back to the GMT format.
>> To mitigate the situation, CLDR can use the abbreviated names from the TZ
>> database, which contains legacy (major) short names as FORMAT. The CLDR
>> provider can use them instead of the GMT offset style. This enhancement is a
>> precursor to the future removal of the COMPAT provider.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use ZoneOffset to parse offset
Looks fine.
(I can't help thinking that some of this pre-processing would be easier to
maintain with a bit more formal parsing and creating of records for the data in
these files; but that's for another time.)
make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java line 1294:
> 1292: .map(Path::toFile)
> 1293: .filter(File::isFile)
> 1294: .forEach(f -> {
I'd keep the Path going through the stream to avoid converting and then back
again.
Suggestion:
.filter(p -> p.toFile().isFile())
.forEach(p -> {
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16206#pullrequestreview-1681188045
PR Review Comment: https://git.openjdk.org/jdk/pull/16206#discussion_r1361419853
Re: RFR: 8296246: Update Unicode Data Files to Version 15.1.0 [v3]
On Mon, 18 Sep 2023 18:33:20 GMT, Naoto Sato wrote:
>> This PR is to incorporate the latest Unicode 15.1, which was released
>> yesterday. Besides the usual character data update, an upgraded
>> implementation of RegEx which reflects the Indic Conjunct Break specified in
>> the latest [Unicode Annex #29 ("Unicode Text
>> Segmentation")](https://unicode.org/reports/tr29/) is included. 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 10 additional commits since
> the last revision:
>
> - Fix GensrcRegex.gmk
> - Merge branch 'master' into JDK-8296246-Unicode15.1
> - Update
> make/jdk/src/classes/build/tools/generateextraproperties/GenerateExtraProperties.java
>
>Co-authored-by: Andrey Turbanov
> - Update
> make/jdk/src/classes/build/tools/generateextraproperties/GenerateExtraProperties.java
>
>Co-authored-by: Andrey Turbanov
> - TR29 final version
> - .md file update
> - Final 8/28
> - Draft 8/11
> - GenerateExtraProperties tool
> - initial commit
Looks good; Its not easy to map all the code changes to spec changes.
src/java.base/share/classes/jdk/internal/util/regex/Grapheme.java line 41:
> 39: * (http://www.unicode.org/reports/tr29/tr29-43.html)
> 40: *
> 41: * @spec http://www.unicode.org/reports/tr29/tr29-43.html
Is the first link now redundant with the @spec link?
-
Marked as reviewed by rriggs (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15728#pullrequestreview-1633892125
PR Review Comment: https://git.openjdk.org/jdk/pull/15728#discussion_r1330509464
Re: RFR: 8309130: x86_64 AVX512 intrinsics for Arrays.sort methods (int, long, float and double arrays) [v30]
On Mon, 28 Aug 2023 21:27:25 GMT, Srinivas Vamsi Parasa wrote: >> The goal is to develop faster sort routines for x86_64 CPUs by taking >> advantage of AVX512 instructions. This enhancement provides an order of >> magnitude speedup for Arrays.sort() using int, long, float and double arrays. >> >> This PR shows upto ~7x improvement for 32-bit datatypes (int, float) and >> upto ~4.5x improvement for 64-bit datatypes (long, double) as shown in the >> performance data below. >> >> >> **Arrays.sort performance data using JMH benchmarks for arrays with random >> data** >> >> |Arrays.sort benchmark | Array Size | Baseline >> (us/op)| AVX512 Sort (us/op) | Speedup | >> |--- | --- | --- | --- | --- >> | >> |ArraysSort.doubleSort | 10 | 0.034 | 0.035 >> | 1.0 | >> |ArraysSort.doubleSort | 25 | 0.116 | 0.089 >> | 1.3 | >> |ArraysSort.doubleSort | 50 | 0.282 | 0.291 >> | 1.0 | >> |ArraysSort.doubleSort | 75 | 0.474 | 0.358 >> | 1.3 | >> |ArraysSort.doubleSort | 100 | 0.654 | 0.623 >> | 1.0 | >> |ArraysSort.doubleSort | 1000| 9.274 | 6.331 >> | 1.5 | >> |ArraysSort.doubleSort | 1 | 323.339 | 71.228 >> | **4.5** | >> |ArraysSort.doubleSort | 10 | 4471.871| >> 1002.748| **4.5** | >> |ArraysSort.doubleSort | 100 | 51660.742 | >> 12921.295 | **4.0** | >> |ArraysSort.floatSort| 10 | 0.045 | 0.046 >> | 1.0 | >> |ArraysSort.floatSort| 25 | 0.103 | 0.084 >> | 1.2 | >> |ArraysSort.floatSort| 50 | 0.285 | 0.33 >> | 0.9 | >> |ArraysSort.floatSort| 75 | 0.492 | 0.346 >> | 1.4 | >> |ArraysSort.floatSort| 100 | 0.597 | 0.326 >> | 1.8 | >> |ArraysSort.floatSort| 1000| 9.811 | 5.294 >> | 1.9 | >> |ArraysSort.floatSort| 1 | 323.955 | 50.547 >> | **6.4** | >> |ArraysSort.floatSort| 10 | 4326.38 | 731.152 >> | **5.9** | >> |ArraysSort.floatSort| 100 | 52413.88| >> 8409.193| **6.2** | >> |ArraysSort.intSort | 10 | 0.033 | 0.033 >> | 1.0 | >> |ArraysSort.intSort | 25 | 0.086 | 0.051 >> | 1.7 | >> |ArraysSort.intSort | 50 | 0.236 | 0.151 >> | 1.6 | >> |ArraysSort.intSort | 75 | 0.416 | 0.332 >> | 1.3 | >> |ArraysSort.intSort | 100 | 0.63| 0.521 >> | 1.2 | >> |ArraysSort.intSort | 1000| 10.518 | 4.698 >> | 2.2 | >> |ArraysSort.intSort | 1 | 309.659 | 42.518 >> | **7.3** | >> |ArraysSort.intSort | 10 | 4130.917| >> 573.956 | **7.2** | >> |ArraysSort.intSort | 100 | 49876.307 | >> 6712.812| **7.4** | >> |ArraysSort.longSort | 10 | 0.036 | 0.037 >> | 1.0 | >> |ArraysSort.longSort | 25 | 0.094 | 0.08 >> | 1.2 | >> |ArraysSort.longSort | 50 | 0.218 | 0.227 >> | 1.0 | >> |ArraysSort.longSort | 75 | 0.466 | 0.402 >> | 1.2 | >> |ArraysSort.longSort | 100 | 0.76| 0.58 >> | 1.3 | >> |ArraysSort.longSort | 1000| 10.449 | 6 > > Srinivas Vamsi Parasa has updated the pull request incrementally with one > additional commit since the last revision: > > Clean up parameters passed to arrayPartition; update the check to load > library @mcimadamore Does Panama have anything to offer over hard coded stubs? - PR Comment: https://git.openjdk.org/jdk/pull/14227#issuecomment-1697957043
Re: RFR: 8314753: Remove support for @beaninfo, @ToDo, @since.unbundled, and @Note [v2]
On Tue, 22 Aug 2023 14:55:18 GMT, Pavel Rappo wrote: >> Please review this trivial PR. > > Pavel Rappo has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains two commits: > > - Merge branch 'master' into 8314753 > - Initial commit Looks trivial only after reviewing the issue and knowing the background. The PR description could be a bit more complete and save a bunch of clicking around. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15385#pullrequestreview-1589653044
Re: RFR: 8310890: Normalize identifier names
On Mon, 26 Jun 2023 14:07:03 GMT, Pavel Rappo wrote: > Please review this cleanup PR to normalize names of identifiers which are > Java variables/fields or tokens in text files. Those names either contain a > pronoun that is very rarely used in code, or seem like they contain such a > pronoun, which, in fact, they don't. Either way, the goal is to improve > readability and clarity. > > Also, this PR fixes a few related typos. Looks good, with or without the suggestion. src/java.base/share/classes/java/util/EnumMap.java line 690: > 688: Object otherValue = em.vals[i]; > 689: if (otherValue != ourValue && > 690: (otherValue == null || !otherValue.equals(ourValue))) Is this the same as java.util.Objects: `!Objects.equals(vals[i], em.vals[i]);` - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14653#pullrequestreview-1499137712 PR Review Comment: https://git.openjdk.org/jdk/pull/14653#discussion_r1242585695
Re: RFR: 8305669: RuntimeException when running benchmarks through make on Windows/WSL [v2]
On Mon, 8 May 2023 20:05:50 GMT, Chen Liang wrote: >> This patch replaces `FIXPATH` with `FixPath` on individual path argumenets. >> The root cause might be that JMH requires passing VM args to benchmarks in >> quotes, which might have triggered incorrect detections in `FIXPATH` (as in >> comparison, `$1_MICRO_JAVA_OPTIONS += >> --add-opens=java.base/java.io=ALL-UNNAMED` right above this patch seems to >> work fine) >> Also removed a useless and wrong Java flag to the java running javac, >> originally intended to enable running `make test TEST="loom.obsolete"` >> benchmarks. >> >> Since I only have a windows cygwin environment, I am not quite sure if this >> works elsewhere. > > Chen Liang 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 six additional commits since > the last revision: > > - Comment about the fix logic >We have a mix and match of spaces and tabs, might need to fix it later > - Is this the right way instead? > - space fails linux > - Expose fixpath import and let jmh run use it > - Merge branch 'master' into jmh-windows > - 8305669: RuntimeException when running benchmarks through make on > Windows/WSL The JMH tests should be runnable standalone and be self contained. - PR Comment: https://git.openjdk.org/jdk/pull/13550#issuecomment-1540683767
Integrated: 8304915: Create jdk.internal.util.Architecture enum and apply
On Wed, 5 Apr 2023 15:58:08 GMT, Roger Riggs wrote: > Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. This pull request has now been integrated. Changeset: f00a748b Author:Roger Riggs URL: https://git.openjdk.org/jdk/commit/f00a748bc5b708d4f8f277d075859b058f9d575c Stats: 411 lines in 7 files changed: 343 ins; 57 del; 11 mod 8304915: Create jdk.internal.util.Architecture enum and apply Reviewed-by: erikj, mdoerr, amitkumar - PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v17]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Correct comment on isPPC64() and refer to isLittleEndian() instead of mentioning PPC64LE - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/b95a312d..accf67f9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=16 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=15-16 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v16]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 19 commits: - Use and test of "s390" verified by reviewer. - Merge branch 'master' into 8304915-arch-enum - Merge branch 'master' into 8304915-arch-enum - ArchTest on Debian RISC-V 64 confirmed by reviewer - Fixed isPPC64(). Consolidated switch cases in ArchTest. Moved mapping of build TARGET_OS and TARGET_CPU to the build to avoid multiple mappings in more than one place. - Correct mapping and test of ppc64 - Add ppc64 as mapping to PPC64 Architecture - Modified test to check Architecture is64bits() and isLittleEndian() against Unsafe respective values. Relocated code mapping OS name and arch name from PlatformProps to OperatingSystem and Architecture. Kept the mapping of names in the template close to where the values are filled in by the build. - Remove unused static and import of Stabile - Rename OperatingSystemProps to PlatformProps. Refactor OperatingSystem initialization to use strings instead of integers. - ... and 9 more: https://git.openjdk.org/jdk/compare/0f3828dd...b95a312d - Changes: https://git.openjdk.org/jdk/pull/13357/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13357=15 Stats: 410 lines in 7 files changed: 342 ins; 57 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v15]
On Mon, 17 Apr 2023 20:59:06 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 17 commits: > > - Merge branch 'master' into 8304915-arch-enum > - ArchTest on Debian RISC-V 64 confirmed by reviewer > - Fixed isPPC64(). >Consolidated switch cases in ArchTest. >Moved mapping of build TARGET_OS and TARGET_CPU to the build >to avoid multiple mappings in more than one place. > - Correct mapping and test of ppc64 > - Add ppc64 as mapping to PPC64 Architecture > - Modified test to check Architecture is64bits() and isLittleEndian() >against Unsafe respective values. >Relocated code mapping OS name and arch name from PlatformProps to >OperatingSystem and Architecture. Kept the mapping of names >in the template close to where the values are filled in by the build. > - Remove unused static and import of Stabile > - Rename OperatingSystemProps to PlatformProps. >Refactor OperatingSystem initialization to use strings instead of integers. > - Revised mapping mechanism of build target architecture names to enum > values. >Unrecognized values from the build are mapped to enum value "OTHER". >Renamed PPC64LE to PPC64 to reflect only the architecture, not the > endianness. >Added an `isLittleEndian` method to return the endianness (not currently > used anywhere) > - Revert changes to jdk.accessibility AccessBridge > - ... and 7 more: https://git.openjdk.org/jdk/compare/8858d543...99a93b7e Last call for Reviewers. Tnx - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1513760009
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v15]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 17 commits: - Merge branch 'master' into 8304915-arch-enum - ArchTest on Debian RISC-V 64 confirmed by reviewer - Fixed isPPC64(). Consolidated switch cases in ArchTest. Moved mapping of build TARGET_OS and TARGET_CPU to the build to avoid multiple mappings in more than one place. - Correct mapping and test of ppc64 - Add ppc64 as mapping to PPC64 Architecture - Modified test to check Architecture is64bits() and isLittleEndian() against Unsafe respective values. Relocated code mapping OS name and arch name from PlatformProps to OperatingSystem and Architecture. Kept the mapping of names in the template close to where the values are filled in by the build. - Remove unused static and import of Stabile - Rename OperatingSystemProps to PlatformProps. Refactor OperatingSystem initialization to use strings instead of integers. - Revised mapping mechanism of build target architecture names to enum values. Unrecognized values from the build are mapped to enum value "OTHER". Renamed PPC64LE to PPC64 to reflect only the architecture, not the endianness. Added an `isLittleEndian` method to return the endianness (not currently used anywhere) - Revert changes to jdk.accessibility AccessBridge - ... and 7 more: https://git.openjdk.org/jdk/compare/8858d543...99a93b7e - Changes: https://git.openjdk.org/jdk/pull/13357/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13357=14 Stats: 410 lines in 7 files changed: 342 ins; 57 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v13]
On Sat, 15 Apr 2023 17:17:13 GMT, Glavo wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed isPPC64(). >> Consolidated switch cases in ArchTest. >> Moved mapping of build TARGET_OS and TARGET_CPU to the build >> to avoid multiple mappings in more than one place. > > test/jdk/jdk/internal/util/ArchTest.java line 70: > >> 68: case "x86", "i386" -> X86; >> 69: case "aarch64" -> AARCH64; >> 70: case "riscv64" -> RISCV64; // unverified > > Suggestion: > > case "riscv64" -> RISCV64; @Glavo Thanks for verifying. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1169059529
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v14]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: ArchTest on Debian RISC-V 64 confirmed by reviewer - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/0cd6ce70..7094db61 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=13 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=12-13 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v13]
On Fri, 14 Apr 2023 17:08:10 GMT, Erik Joelsson wrote: >> make/modules/java.base/gensrc/GensrcMisc.gmk line 72: >> >>> 70: endif >>> 71: >>> 72: $(eval $(call SetupTextFileProcessing, BUILD_PLATFORMPROPERTIES_JAVA, \ >> >> @erikj79 Is there a better/good way to do these mappings, or select "local" >> variable names? > > Not sure if it's better, but you could consider doing this directly in the > switch statements in `make/autoconf/platform.m4` and add the corresponding > variables in `spec.gmk.in`. That would make them available globally in the > build, which may also be detrimental as parts of the build could start > relying on them, and we end up with even more variants sprinkled around. I > think what you have here is better for now. ok, I'd rather keep it local. Thanks - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1167290252
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v13]
On Wed, 12 Apr 2023 17:31:49 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The value of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed isPPC64(). > Consolidated switch cases in ArchTest. > Moved mapping of build TARGET_OS and TARGET_CPU to the build > to avoid multiple mappings in more than one place. make/modules/java.base/gensrc/GensrcMisc.gmk line 72: > 70: endif > 71: > 72: $(eval $(call SetupTextFileProcessing, BUILD_PLATFORMPROPERTIES_JAVA, \ @erikj79 Is there a better/good way to do these mappings, or select "local" variable names? - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1166913016
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v12]
On Wed, 12 Apr 2023 09:07:34 GMT, Martin Doerr wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Correct mapping and test of ppc64 > > Works on PPC64 Big Endian, now. However, little Endian fails: > STARTEDArchTest::isArch '[6] PPC64, false' > org.opentest4j.AssertionFailedError: Mismatch PPC64 == PPC64 vs isPPC64 ==> > expected: but was: > at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55) > at > org.junit.jupiter.api.AssertionUtils.failNotEqual(AssertionUtils.java:62) > at > org.junit.jupiter.api.AssertEquals.assertEquals(AssertEquals.java:182) > at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1152) > at ArchTest.isArch(ArchTest.java:99) > ... > System property os.arch: "ppc64le", Architecture.current(): "PPC64" @TheRealMDoerr Thanks for the ppc64 feedback and testing. - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1505685254
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v13]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Fixed isPPC64(). Consolidated switch cases in ArchTest. Moved mapping of build TARGET_OS and TARGET_CPU to the build to avoid multiple mappings in more than one place. - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/71135e3b..0cd6ce70 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=12 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=11-12 Stats: 60 lines in 5 files changed: 19 ins; 22 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v12]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Correct mapping and test of ppc64 - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/1c0c0220..71135e3b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=11 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=10-11 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v9]
On Tue, 11 Apr 2023 19:33:28 GMT, Roger Riggs wrote: >>> > Would be great if you could support "os.arch = ppc64" for AIX and legacy >>> > linux, too. >>> >>> Changing os.arch is out of scope for this PR. The best way for that would >>> someone supporting ppc to develop and propose a PR. >> >> It seems that you replied in the wrong place. > > Likely, I misunderstood the request. Mapping ppc64 to PPC64 is easy enough. > But currently, there is no direct support in the build/makefiles for ppc. See > [OpenJDK supported > platforms](https://wiki.openjdk.org/display/Build/Supported+Build+Platforms). > For anything other than the most trivial and obvious, someone who can build > ppc and test should propose the changes. The naming of x86_64 was raised earlier and resolved to use X64. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1163242850
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v9]
On Tue, 11 Apr 2023 18:37:11 GMT, Glavo wrote: >>> Would be great if you could support "os.arch = ppc64" for AIX and legacy >>> linux, too. >> >> Changing os.arch is out of scope for this PR. >> The best way for that would someone supporting ppc to develop and propose a >> PR. > >> > Would be great if you could support "os.arch = ppc64" for AIX and legacy >> > linux, too. >> >> Changing os.arch is out of scope for this PR. The best way for that would >> someone supporting ppc to develop and propose a PR. > > It seems that you replied in the wrong place. Likely, I misunderstood the request. Mapping ppc64 to PPC64 is easy enough. But currently, there is no direct support in the build/makefiles for ppc. See [OpenJDK supported platforms](https://wiki.openjdk.org/display/Build/Supported+Build+Platforms). For anything other than the most trivial and obvious, someone who can build ppc and test should propose the changes. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1163241987
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v11]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Add ppc64 as mapping to PPC64 Architecture - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/fc40270a..1c0c0220 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=10 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v10]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The value of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Modified test to check Architecture is64bits() and isLittleEndian() against Unsafe respective values. Relocated code mapping OS name and arch name from PlatformProps to OperatingSystem and Architecture. Kept the mapping of names in the template close to where the values are filled in by the build. - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/f3646dc9..fc40270a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=09 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=08-09 Stats: 105 lines in 4 files changed: 46 ins; 41 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v9]
On Tue, 11 Apr 2023 10:39:39 GMT, Martin Doerr wrote: >> This should (probably) be correct: >> Suggestion: >> >> case PPC64 -> !OperatingSystem.isAix() && >> Architecture.isLittleEndian(); > > Looks correct, but makes the test pointless for any linux on PPC64. Will change to compare `isLittleEndian` with `!Unsafe.isBigEndian()`. The runtime must match the build time. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1163112421
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v9]
On Tue, 11 Apr 2023 11:41:24 GMT, Glavo wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused static and import of Stabile > > src/java.base/share/classes/jdk/internal/util/PlatformProps.java.template > line 68: > >> 66: // The variables are named to match the Architecture value names, and >> 67: // the values are named to match the build variables. >> 68: static final boolean TARGET_ARCH_IS_X64 = >> "@@OPENJDK_TARGET_CPU@@" == "x86_64"; > > Would it be better to rename the enum entry to `X86_64`? I personally prefer > the name x86-64 because I feel it is more regular than x64.. > Would be great if you could support "os.arch = ppc64" for AIX and legacy > linux, too. Changing os.arch is out of scope for this PR. The best way for that would someone supporting ppc to develop and propose a PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1163098619
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v9]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Remove unused static and import of Stabile - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/53c20c77..f3646dc9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=08 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=07-08 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Fri, 7 Apr 2023 12:05:28 GMT, Lutz Schmidt wrote: >> Okay, Lutz is the expert here. Sorry for the noise. > > Just to let my voice be heard directly after being cited several times: s390 > is used to designate the CPU architecture. The arch-specific files are stored > in src/hotspot/cpu/s390 for that reason. Back in the days when SAP > contributed the s390 port, there was a discussion which architecture name to > use. SAP had used zArch_64 which more closely reflects what the port really > is (a 64-bit only port to IBM's z/Architecture). It was a majority decision > to use s390 for some historic reasons (I don't remember exactly anymore). > > s390x is (almost) always used in conjunction with a Linux OS port. In that > respect, linuxs390x is a tautology. OpenJDK does not use this tautology, see > src/hotspot/os_cpu/linux_s390 for example. Thanks for the authoritative answer. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160968894
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Fri, 7 Apr 2023 06:03:11 GMT, Thomas Stuefe wrote: >>> What did you use as the example that would not compile on the other >>> architecture? >> >> https://github.com/openjdk/jdk/blob/52ca4a70fc3de9e285964f9545ea8cd54e2d9924/src/java.base/share/classes/jdk/internal/util/OperatingSystemProps.java.template#L40 >> >> I don't understand how the above code can be compiled on architectures such >> as LongArch64, MIPS64el, or ARM32 without modifying the source file. >> >>> Did you make the other changes to the build that would be needed for a new >>> architecture? >> >> I'm not talking about the new architecture, I'm talking about the >> architecture that OpenJDK already supports. >> >> According to my understanding, the above code breaks the support of all >> architectures not listed in `Architecture`. But has `Architecture` really >> listed all the supported architectures in the OpenJDK mainline? >> >> For example, I want to compile OpenJDK for 32-bit ARM. I think in this PR, >> I cannot compile without modifying the source code because `TARGET_ARCH_arm` >> does not exist. But before this PR, I was able to compile. > > @Glavo Arm32 builds, though, see GHAs (crossbuilding test). As for the > others, as long as the ports have not been integrated into mainline, I expect > port maintainers will adapt the enum downstream. They do this anyway. > > Edit: I now understand your point better and agree in parts, see my reply > below. Refactored the way that build time architecture values are mapped to the Architecture enum. It now uses strings and does not require predefined names in the template file. Explicitly maps some build time arch values to the corresponding (but not identical Arch enum); for example x86_64 to X64. Added a method to expose the endian-ness of the build; this allows it to be separated from the architecture. So for example "ppc64le" maps to PPC64 and the endian-ness is little. Added an "OTHER" to the enum values so that unrecognized architecture values from the build can be mapped to that enum. Renamed the template file so it can more naturally cover the os, architecture, endianness, and 64bit'ness of the implementation. Reworked OperatingSystem to use the same string based interface between the build and the enum. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160968389
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v8]
On Fri, 7 Apr 2023 21:13:03 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, >> PPC64LE` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The values of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with three additional > commits since the last revision: > > - Rename OperatingSystemProps to PlatformProps. >Refactor OperatingSystem initialization to use strings instead of integers. > - Revised mapping mechanism of build target architecture names to enum > values. >Unrecognized values from the build are mapped to enum value "OTHER". >Renamed PPC64LE to PPC64 to reflect only the architecture, not the > endianness. >Added an `isLittleEndian` method to return the endianness (not currently > used anywhere) > - Revert changes to jdk.accessibility AccessBridge Take a fresh look and comment. Thanks - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1500655738
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v8]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with three additional commits since the last revision: - Rename OperatingSystemProps to PlatformProps. Refactor OperatingSystem initialization to use strings instead of integers. - Revised mapping mechanism of build target architecture names to enum values. Unrecognized values from the build are mapped to enum value "OTHER". Renamed PPC64LE to PPC64 to reflect only the architecture, not the endianness. Added an `isLittleEndian` method to return the endianness (not currently used anywhere) - Revert changes to jdk.accessibility AccessBridge - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/52ca4a70..53c20c77 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=07 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=06-07 Stats: 237 lines in 10 files changed: 123 ins; 81 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 22:44:52 GMT, Phil Race wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unneeded qualified export from java.base to jdk.attach
>
> src/jdk.accessibility/windows/classes/com/sun/java/accessibility/internal/AccessBridge.java
> line 169:
>
>> 167: // Load the appropriate DLLs
>> 168: boolean is32on64 = false;
>> 169: if (Architecture.isX86()) {
>
> This would be another coupling of java.desktop into java.base internals and I
> don't think it worth it.
> Particularly because this check is obsolete !
> It is a remnant of when accessbridge was an unbundled download so provided
> both 32 and 64 bit.
> We don't even have the file it is looking for in the build - even if someone
> did a 32 bit build.
> Instead of making this change a bug should be filed to remove this code.
Reverting; filed [8305745](https://bugs.openjdk.org/browse/JDK-8305745)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160748988
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 20:42:26 GMT, Glavo wrote:
>> There is no benefit to preemptively defining a full set of architectures; we
>> only need those that are used in the OpenJDK runtime selection of options or
>> parameters.
>> The other and unknown cases can be handled in code using switch as `default`
>> or for `if (xx) {} else {...}` in the else clause.
>> Ports not supported the OpenJDK can add enum values and the corresponding
>> static `isXXX()` methods if needed.
>>
>> The template file used in the implementation could be renamed to be more
>> agnostic to OS or arch.
>
> I understand that there is no need to define enum entries for all
> architectures now, but the `current` method seems to cause crashes on other
> platforms.
>
> Even worse, `OperatingSystemProps` seems to be unable to compile on other
> architectures at all:
>
> https://github.com/openjdk/jdk/blob/52ca4a70fc3de9e285964f9545ea8cd54e2d9924/src/java.base/share/classes/jdk/internal/util/OperatingSystemProps.java.template#L40
>
> Does `Architecture` really need to be implemented as an enum? The value of
> this enum has never been used in this PR except for testing. I think perhaps
> just providing the isXXX methods is enough.
The point of Architecture is reflect the architecture as supported by the build
and the runtime mutually and to reflect dependencies on the target hardware.
What did you use as the example that would not compile on the other
architecture?
Did you make the other changes to the build that would be needed for a new
architecture?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160252604
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 20:38:02 GMT, Thomas Stuefe wrote: > Another question, how does this work with Zero? Zero is orthogonal to architecture; the interpreter is compiled for a specific target architecture. - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1499606550
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 20:28:29 GMT, Thomas Stuefe wrote: > What about PPC (big endian)? Used on AIX? I'm not aware of any code (in OpenJDK) related to big/little endian that is derived from `os.arch`. > > On Arm, it may be useful to know whether we built for thumb mode (We recently > had this problem in tests). For tests, there is the test library `jdk.test.lib.Platform` class with its own set of predicates. For the moment, the idea is to do the minimum to replace OpenJDK internal uses of `os.arch`. - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1499597036
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 19:55:07 GMT, Glavo wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove unneeded qualified export from java.base to jdk.attach
>
> src/java.base/share/classes/jdk/internal/util/Architecture.java line 100:
>
>> 98: */
>> 99: public static Architecture current() {
>> 100: return archValues[OperatingSystemProps.CURRENT_ARCH_ORDINAL];
>
> I think the `Architecture ` is different from the `OperatingSystem`. There
> are ports of [other
> architectures](https://github.com/openjdk/jdk/blob/1d517afbd4547171ad6fb6a3356351c2554c8279/make/autoconf/platform.m4#L33-L188)
> (Including `MIPS64el`, `LoongArch64`, `SPARC v9`, etc) in the upstream.
>
> Will hard coding architectures destroy these ports? Should a "OTHER" or
> "UNKNOWN" entry be added to the enum to represent other architectures that
> are not of concern?
There is no benefit to preemptively defining a full set of architectures; we
only need those that are used in the OpenJDK runtime selection of options or
parameters.
The other and unknown cases can be handled in code using switch as `default` or
for `if (xx) {} else {...}` in the else clause.
Ports not supported the OpenJDK can add enum values and the corresponding
static `isXXX()` methods if needed.
The template file used in the implementation could be renamed to be more
agnostic to OS or arch.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160226259
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Remove unneeded qualified export from java.base to jdk.attach - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/ab084c27..52ca4a70 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=06 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=05-06 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v6]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Remove obsolete conditions in Windows AttachProviderImpl. All platforms and architectures that can be built support attach. - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/105ce605..ab084c27 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=05 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=04-05 Stats: 11 lines in 1 file changed: 0 ins; 11 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v5]
On Thu, 6 Apr 2023 18:44:02 GMT, Alan Bateman wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Revert changes to Foreign API to avoid class with another PR
>> Rename S390X to S390, representing just the s390 architecture
>> Removed ARM and IA64 enum values; they are unused; they can be added later
>> if needed
>
> src/jdk.attach/windows/classes/sun/tools/attach/AttachProviderImpl.java line
> 51:
>
>> 49: !Architecture.isAARCH64()) {
>> 50: throw new RuntimeException(
>> 51: "This provider is not supported on this processor
>> architecture: " + Architecture.current());
>
> Could this constructor be changed to be a no-op? The check for Windows 9 and
> Windows Me dates back to JDK 6 and is no longer needed. The os.arch check
> also dates from JDK 6 when the attach provider didn't work on IA64 The only
> Windows builds now are windows-x86, windows-x64, windows-aarch64 so I assume
> this check can go away.
Removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160154277
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v5]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Revert changes to Foreign API to avoid class with another PR Rename S390X to S390, representing just the s390 architecture Removed ARM and IA64 enum values; they are unused; they can be added later if needed - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/8916dcae..105ce605 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=04 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=03-04 Stats: 58 lines in 5 files changed: 5 ins; 32 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Thu, 6 Apr 2023 15:27:49 GMT, David M. Lloyd wrote:
>> src/java.base/share/classes/jdk/internal/util/Architecture.java line 47:
>>
>>> 45:
>>> 46: // Cache a copy of the array for lightweight indexing
>>> 47: private static final Architecture[] archValues =
>>> Architecture.values();
>>
>> This needs to be annotated with `@jdk.internal.vm.annotation.Stable` for
>> `Architecture.current()` to be constant foldable:
>> Suggestion:
>>
>> private static final @Stable Architecture[] archValues =
>> Architecture.values();
>
> Even if it's `static` *and* `final`? I thought `@Stable` exists to
> "...process non-null stable fields (final or otherwise) in a similar manner
> to static final fields with respect to promoting the field's value to a
> constant", implying that `static final` fields already have this property.
The use here is not particularly performance sensitive. So it doesn't provide
much value.
In this case, it allows each array element to be considered stable.
>From the @Stable javadoc:
* Fields which are declared {@code final} may also be annotated as stable.
* Since final fields already behave as stable values, such an annotation
* conveys no additional information regarding change of the field's value, but
* still conveys information regarding change of additional components values if
* the type of the field is an array type (as described above).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160018375
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Thu, 6 Apr 2023 03:17:57 GMT, ExE Boss wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Correct spelling of isAARCH64 in WIndows AttachProviderImpl > > src/java.base/share/classes/jdk/internal/util/Architecture.java line 39: > >> 37: X86(), >> 38: IA64(), >> 39: ARM(), > > Suggestion: > > ARM32(), I'm inclined to drop the enum value and isArm method because they are legacy and are not being used in OpenJDK sources. This could also apply to IA64. They can be added if needed and otherwise are just unused bulk. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159852394
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Thu, 6 Apr 2023 08:05:14 GMT, ExE Boss wrote:
>> src/java.base/share/classes/jdk/internal/util/Architecture.java line 85:
>>
>>> 83: */
>>> 84: @ForceInline
>>> 85: public static boolean isRISCV64() {
>>
>> Are all the "isers" necessary to provide constant code folding or is the
>> samt thing achievable via expressions like `(Architecture.current() ==
>> Architecture.X)` ?
>
> `Architecture.current()` requires the `Architecture.archValues` array to be
> annotated with `@jdk.internal.vm.annotation.Stable` for it to be constant
> foldable by the JIT.
The `isXXX` is lighterweight and easier to read.
With current compiler technology, it can result in dead code elimination.
The HotSpot compiler reduces `(Architecture.current() == Architecture.X)` at
runtime.
Project Leyden may enable dead code based on evaluating more complex
expressions as above.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159811629
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Thu, 6 Apr 2023 07:41:48 GMT, Per Minborg wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Correct spelling of isAARCH64 in WIndows AttachProviderImpl > > src/java.base/share/classes/jdk/internal/util/Architecture.java line 2: > >> 1: /* >> 2: * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights >> reserved. > > Why 2018? Copy/paste error. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159813009
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Wed, 5 Apr 2023 23:39:00 GMT, Jorn Vernee wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Correct spelling of isAARCH64 in WIndows AttachProviderImpl > > test/jdk/java/foreign/TestUnsupportedLinker.java line 26: > >> 24: /* >> 25: * @test >> 26: * @ignore architecture can not be overridden by setting os.arch - >> delete test > > FWIW, we introduce a property in CABI to override it directly in the JEP > patch: > https://github.com/openjdk/jdk/pull/13079/files#diff-1e9526318ae485495af012cbf0450693d77fd8241be62afe422a75304de5aed1R28 I'll drop the changes to foreign to avoid conflicts with PR #13079. - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159801845
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Thu, 6 Apr 2023 03:17:37 GMT, ExE Boss wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Correct spelling of isAARCH64 in WIndows AttachProviderImpl
>
> src/java.base/share/classes/jdk/internal/util/Architecture.java line 37:
>
>> 35: public enum Architecture {
>> 36: X64(),// Represents AMD64 and X86_64
>> 37: X86(),
>
> Suggestion:
>
> X86_64(),// Represents AMD64 and X86_64
> X86_32(),
The underscores are too ugly and X86 and X64 are well accepted names for those
architectures.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159804212
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Wed, 5 Apr 2023 20:31:43 GMT, Bernd wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Correct spelling of isAARCH64 in WIndows AttachProviderImpl
>
> test/jdk/jdk/internal/util/ArchTest.java line 58:
>
>> 56: @Test
>> 57: public void nameVsCurrent() {
>> 58: String osArch =
>> System.getProperty("os.arch").toLowerCase(Locale.ROOT);
>
> Is it planned to determine os.arch with the same enum in the future (keeping
> the legacy names)?
That's a different set of twisty build and native source files, I'll create an
issue to re-visit that.
It may not change too much, the native code needs to initialize `os.arch` very
early.
[JDK-8305676](https://bugs.openjdk.org/browse/JDK-8305676)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159009755
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Wed, 5 Apr 2023 20:25:43 GMT, Bernd wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Correct spelling of isAARCH64 in WIndows AttachProviderImpl
>
> src/java.base/share/classes/jdk/internal/foreign/CABI.java line 48:
>
>> 46: // might be running in a 32-bit VM on a 64-bit platform.
>> 47: // addressSize will be correctly 32
>> 48: if (Architecture.isX64() && ADDRESS_SIZE == 64) {
>
> Is there a difference to Architecture.is64bit (I.e. is the later or the
> former runtime vs compiletime
There should be no difference; I was hesitant to drop the ADDRESS_SIZE check
without knowing more about the foreign api dependencies. ADDRESS_SIZE is
computed (I think) from `UNSAFE.ADDRESS_SIZE * 8`.
But I can't think of how it can be different than the CPU_BITS that are defined
when the JDK is built.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159001239
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Correct spelling of isAARCH64 in WIndows AttachProviderImpl - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/3349b46d..8916dcae Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=03 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v3]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Correct name of isRISCV64 method. - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/bef30f96..3349b46d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=01-02 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v2]
> Define an internal jdk.internal.util.Architecture enumeration and static > methods to replace uses of the system property `os.arch`. > The enumeration values are defined to match those used in the build. > The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, > PPC64LE` > Note that `amd64` and `x86_64` in the build are represented by `X64`. > The values of the system property `os.arch` is unchanged. > > The API is similar to the jdk.internal.util.OperatingSystem enum created by > #[12931](https://git.openjdk.org/jdk/pull/12931). > Uses in `java.base` and a few others are included but other modules will be > done in separate PRs. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Rename isXXX method to be uppercase architecture names matching the enum. Refactor the is64Bit method to be a static method for the current runtime. - Changes: - all: https://git.openjdk.org/jdk/pull/13357/files - new: https://git.openjdk.org/jdk/pull/13357/files/0c61382d..bef30f96 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13357=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13357=00-01 Stats: 72 lines in 5 files changed: 35 ins; 13 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply
On Wed, 5 Apr 2023 16:23:08 GMT, Glavo wrote:
>> Define an internal jdk.internal.util.Architecture enumeration and static
>> methods to replace uses of the system property `os.arch`.
>> The enumeration values are defined to match those used in the build.
>> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X,
>> PPC64LE`
>> Note that `amd64` and `x86_64` in the build are represented by `X64`.
>> The values of the system property `os.arch` is unchanged.
>>
>> The API is similar to the jdk.internal.util.OperatingSystem enum created by
>> #[12931](https://git.openjdk.org/jdk/pull/12931).
>> Uses in `java.base` and a few others are included but other modules will be
>> done in separate PRs.
>
> src/java.base/share/classes/jdk/internal/util/Architecture.java line 77:
>
>> 75: * {@return {@code true} if the current architecture is IA64}
>> 76: */
>> 77: public static boolean isIA64() {
>
> Why is the name case correct only for IA-64, while `AArch64`, `RISC-V 64` and
> other architectures only have the first letter capitalized?
>
> I think names like `isRiscv64`, `isPpc64le`, and `isAarch64` are quite ugly,
> and there is no precedent yet.
Yes, the capitalization names should be disciplined. In the enum, there are all
uppercase, following the style of manifest constants and enums. In the build
they are always lower case. Camel case is usually used for method names.
Using all upper case would make them consistent with the Enum names.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1158801096
RFR: 8304915: Create jdk.internal.util.Architecture enum and apply
Define an internal jdk.internal.util.Architecture enumeration and static methods to replace uses of the system property `os.arch`. The enumeration values are defined to match those used in the build. The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, PPC64LE` Note that `amd64` and `x86_64` in the build are represented by `X64`. The values of the system property `os.arch` is unchanged. The API is similar to the jdk.internal.util.OperatingSystem enum created by #[12931](https://git.openjdk.org/jdk/pull/12931). Uses in `java.base` and a few others are included but other modules will be done in separate PRs. - Commit messages: - 8304915: Create jdk.internal.util.Architecture enum and apply Changes: https://git.openjdk.org/jdk/pull/13357/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13357=00 Issue: https://bugs.openjdk.org/browse/JDK-8304915 Stats: 279 lines in 10 files changed: 265 ins; 3 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13357.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13357/head:pull/13357 PR: https://git.openjdk.org/jdk/pull/13357
Integrated: 8303485: Replacing os.name for operating system customization
On Wed, 8 Mar 2023 19:15:16 GMT, Roger Riggs wrote: > Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. This pull request has now been integrated. Changeset: 6c3b10fb Author:Roger Riggs URL: https://git.openjdk.org/jdk/commit/6c3b10fb1d95fb03e2f7d988d4c772960af11c91 Stats: 411 lines in 13 files changed: 297 ins; 40 del; 74 mod 8303485: Replacing os.name for operating system customization Reviewed-by: naoto, erikj, alanb - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v9]
> Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Remove unused SuppressWarnings - Changes: - all: https://git.openjdk.org/jdk/pull/12931/files - new: https://git.openjdk.org/jdk/pull/12931/files/3b88f233..574e37be Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12931=08 - incr: https://webrevs.openjdk.org/?repo=jdk=12931=07-08 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v8]
> Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Correct javadoc throws keyword - Changes: - all: https://git.openjdk.org/jdk/pull/12931/files - new: https://git.openjdk.org/jdk/pull/12931/files/bfc3de93..3b88f233 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12931=07 - incr: https://webrevs.openjdk.org/?repo=jdk=12931=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v6]
On Fri, 24 Mar 2023 16:53:05 GMT, Michael Osipov wrote: >> If you are referring to "Red Hat Enterprise Linux", it'd be correct (AFAIK) >> to say that Red Hat identifies "Red Hat Enterprise Linux" as an operating >> system, but I wouldn't go so far as to say that Red Hat calls "Linux" an >> operating system (well... subject to the reality of human inaccuracy). And >> the validity and correctness of the term "GNU/Linux" is *definitely* >> disputed (and likely always will be). >> >> Maybe "Operating systems based on the Linux kernel" would be satisfactory? > > That is acceptable, totally. Not the rabbit-hole I expected to go down today or for an informal comment on an internal API. I have no skin in that game. https://www.redhat.com/en/topics/linux/what-is-linux - PR Review Comment: https://git.openjdk.org/jdk/pull/12931#discussion_r1147850108
Re: RFR: 8303485: Replacing os.name for operating system customization [v7]
> Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: clarify Linux - Changes: - all: https://git.openjdk.org/jdk/pull/12931/files - new: https://git.openjdk.org/jdk/pull/12931/files/8372978f..bfc3de93 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12931=06 - incr: https://webrevs.openjdk.org/?repo=jdk=12931=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v6]
On Fri, 24 Mar 2023 16:04:38 GMT, Roger Riggs wrote: >> src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 81: >> >>> 79: */ >>> 80: AIX, >>> 81: ; >> >> So no Unknown value? > > Correct, as explained above. If additional OS values are needed, they can be added to the enum and add the corresponding `isXXX()` method building on the same pattern used in OperatingSystemProps.java.template. - PR Review Comment: https://git.openjdk.org/jdk/pull/12931#discussion_r1147804555
Re: RFR: 8303485: Replacing os.name for operating system customization [v6]
On Fri, 24 Mar 2023 16:03:26 GMT, Michael Osipov wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Rename OperatingSystem enum values to uppercase > > src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 66: > >> 64: >> 65: /** >> 66: * The Linux Operating system. > > For the sake if completeness, Linux isn't an operating system, so this > statement isn't correct. A bit of a quibble, several internet sources identify Linux as an operating system (including Redhat). - PR Review Comment: https://git.openjdk.org/jdk/pull/12931#discussion_r1147790687
Re: RFR: 8303485: Replacing os.name for operating system customization [v6]
On Fri, 24 Mar 2023 16:02:59 GMT, Michael Osipov wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Rename OperatingSystem enum values to uppercase > > src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 81: > >> 79: */ >> 80: AIX, >> 81: ; > > So no Unknown value? Correct, as explained above. - PR Review Comment: https://git.openjdk.org/jdk/pull/12931#discussion_r1147786572
Re: RFR: 8303485: Replacing os.name for operating system customization [v6]
> Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Rename OperatingSystem enum values to uppercase - Changes: - all: https://git.openjdk.org/jdk/pull/12931/files - new: https://git.openjdk.org/jdk/pull/12931/files/32ee9d4a..8372978f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12931=05 - incr: https://webrevs.openjdk.org/?repo=jdk=12931=04-05 Stats: 25 lines in 4 files changed: 0 ins; 0 del; 25 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v5]
On Thu, 23 Mar 2023 15:33:59 GMT, Daniel Fuchs wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Group imports for jdk.internal.util > > src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 81: > >> 79: */ >> 80: AIX, >> 81: ; > > While browsing another PR I noticed that jlink also has an [OperatingSystem > enum](https://github.com/openjdk/jdk/blob/master/src/jdk.jlink/share/classes/jdk/tools/jlink/internal/Platform.java#L34), > which has an additional `UNKNOWN` enum constant. Would it make sense to have > an `UNKNOWN` constant here too, which could also make it possible to use this > enum directly with jlink too? Its not needed here. In the jlink version, that value is only used if the mapping from os.name fails. With the new OperatingSystem enum, it is exactly one of the os's supported by the build. If there is no build support then it never gets to a runtime error. - PR Review Comment: https://git.openjdk.org/jdk/pull/12931#discussion_r1146408856
Re: RFR: 8303485: Replacing os.name for operating system customization [v5]
> Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Group imports for jdk.internal.util - Changes: - all: https://git.openjdk.org/jdk/pull/12931/files - new: https://git.openjdk.org/jdk/pull/12931/files/ead84379..32ee9d4a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12931=04 - incr: https://webrevs.openjdk.org/?repo=jdk=12931=03-04 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v4]
On Tue, 14 Mar 2023 18:21:33 GMT, Roger Riggs wrote: >> Improvements to support OS specific customization for JDK internal use: >> - To select values and code; allowing elimination of unused code and values >> - Optionally evaluated by build processes, compilation, or archiving (i.e. >> CDS) >> - Simple API to replace adhoc comparisons with `os.name` >> - Clear and consistent use across build, runtime, and JDK modules >> >> The PR includes updates within java.base to use the new API. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Cover default case in switch on OS. Please review the revisions. - PR Comment: https://git.openjdk.org/jdk/pull/12931#issuecomment-1480241882
Re: RFR: 8303018: Unicode Emoji Properties [v3]
On Wed, 15 Mar 2023 18:21:11 GMT, Naoto Sato wrote: >> Proposing accessor methods to Emoji properties defined in [Unicode Technical >> Standard #51](https://unicode.org/reports/tr51/) in `java.lang.Character` >> class. This is per a request from the client group, as well as refining the >> currently existing ad-hoc emoji implementation in regex. A CSR has also been >> drafted, and I would appreciate reviews for it too. > > Naoto Sato has updated the pull request incrementally with two additional > commits since the last revision: > > - InternalError message/mask constants cleanup > - Indentation/print comment fix Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/13006
Re: RFR: 8303018: Unicode Emoji Properties [v2]
On Tue, 14 Mar 2023 15:49:56 GMT, Naoto Sato wrote:
>> Proposing accessor methods to Emoji properties defined in [Unicode Technical
>> Standard #51](https://unicode.org/reports/tr51/) in `java.lang.Character`
>> class. This is per a request from the client group, as well as refining the
>> currently existing ad-hoc emoji implementation in regex. A CSR has also been
>> drafted, and I would appreciate reviews for it too.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed method descriptions
Looks good.
There are opportunities to modernize the code, but maybe separately.
make/jdk/src/classes/build/tools/generatecharacter/EmojiData.java line 99:
> 97: case "Emoji_Component" -> EMOJI_COMPONENT;
> 98: case "Extended_Pictographic" -> EXTENDED_PICTOGRAPHIC;
> 99: default -> throw new InternalError();
It would be useful to include the "type" as the exception argument. It give
some idea as to the corruption or missing case.
make/jdk/src/classes/build/tools/generatecharacter/GenerateCharacter.java line
214:
> 212: maskEmojiModifierBase = 0x0200L,
> 213: maskEmojiComponent = 0x0400L,
> 214: maskExtendedPictographic = 0x0800L;
It would be good to leverage a common definition (perhaps a bit number) here
and in EmojiData.java
and build the constants with <<< shifts.
make/jdk/src/classes/build/tools/generatecharacter/GenerateCharacter.java line
810:
> 808: if (x.equals("maskEmojiModifierBase")) return "0x" +
> hex4(maskEmojiModifierBase >> 32);
> 809: if (x.equals("maskEmojiComponent")) return "0x" +
> hex4(maskEmojiComponent >> 32);
> 810: if (x.equals("maskExtendedPictographic")) return "0x" +
> hex4(maskExtendedPictographic >> 32);
An upgrade would be to modify hex4(), hexNN() to use
`HexFormat.of().toUpperCase().toHexDigits((short)xxx)`
The HexFormat is reusable and would avoid creating extra strings.
Perhaps also create a method that combines the repetitive shift and prefixing.
This if...then... sequence could be an expression switch (x) {...}.
-
Marked as reviewed by rriggs (Reviewer).
PR: https://git.openjdk.org/jdk/pull/13006
Re: RFR: 8303485: Replacing os.name for operating system customization [v4]
> Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Cover default case in switch on OS. - Changes: - all: https://git.openjdk.org/jdk/pull/12931/files - new: https://git.openjdk.org/jdk/pull/12931/files/be27adb0..ead84379 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12931=03 - incr: https://webrevs.openjdk.org/?repo=jdk=12931=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v3]
On Tue, 14 Mar 2023 15:04:47 GMT, Alan Bateman wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix indentation and improve exception message > > src/java.base/share/classes/jdk/internal/util/OperatingSystem.java line 29: > >> 27: >> 28: /** >> 29: * Enumeration of operating system types and testing for the current OS. > > Would it be more correct to say an enumerating of operating system names > rather than types? It is more OS type or kind than name; there are multiple distros that are Linux, i.e. Ubuntu, Fedora, Oracle Linux, etc.. It is not equivalent to `os.name`. - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v3]
> Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Fix indentation and improve exception message - Changes: - all: https://git.openjdk.org/jdk/pull/12931/files - new: https://git.openjdk.org/jdk/pull/12931/files/342f30f2..be27adb0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12931=02 - incr: https://webrevs.openjdk.org/?repo=jdk=12931=01-02 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v2]
On Fri, 10 Mar 2023 05:42:18 GMT, David Holmes wrote: >> Good point. `OperatingSystemProps.java` can be a OS-specific class like: >> >> src/java.base/macosx/classes/jdk/internal/misc/OperatingSystemProps.java >> linux/classes/jdk/internal/misc/OperatingSystemProps.java >> windows/classes/jdk/internal/misc/OperatingSystemProps.java >> >> >> OTOH, if we include `os.arch`, that would need to be a template. A single >> template file to include both OS and ARCH would do it. > > I was literally thinking of the `#if[xx]` conditional compilation facility > rather than an OS specific file, but okay ... The stream preprocessor (SPP) is more involved than the simple substitutions used to insert the target os into the template. - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v2]
On Fri, 10 Mar 2023 21:21:56 GMT, Roger Riggs wrote: >> Improvements to support OS specific customization for JDK internal use: >> - To select values and code; allowing elimination of unused code and values >> - Optionally evaluated by build processes, compilation, or archiving (i.e. >> CDS) >> - Simple API to replace adhoc comparisons with `os.name` >> - Clear and consistent use across build, runtime, and JDK modules >> >> The PR includes updates within java.base to use the new API. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Move OperatingSystem from jdk.internal.misc to jdk.internal.util > Rename Mac -> MacOS; isMac() -> isMacOS() A quick look at the freebsd port (jdk19) shows about 19k lines of new code or changes across 294 files, config, makefiles, hotspot and other native code. Pretty much what you'd expect in terms of api differences, configuration differences, and alternate implementations. The changes are done with ifdefs in native code, adding enumeration cases where necessary, and testing of os.name. Changing to an equivalent test for operating system is a lateral move. - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization [v2]
> Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Move OperatingSystem from jdk.internal.misc to jdk.internal.util Rename Mac -> MacOS; isMac() -> isMacOS() - Changes: - all: https://git.openjdk.org/jdk/pull/12931/files - new: https://git.openjdk.org/jdk/pull/12931/files/736ab3cc..342f30f2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12931=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12931=00-01 Stats: 467 lines in 15 files changed: 210 ins; 235 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Wed, 8 Mar 2023 23:23:38 GMT, Mandy Chung wrote: > I wonder if `jdk.internal.platform` would be a better home for > `OperatingSystem` class? jdk.internal.platform seems to be focused on information about container environments such as Docker. jdk.internal.misc already contains other classes that are shared out of java.base to other modules. - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Wed, 8 Mar 2023 19:15:16 GMT, Roger Riggs wrote: > Improvements to support OS specific customization for JDK internal use: > - To select values and code; allowing elimination of unused code and values > - Optionally evaluated by build processes, compilation, or archiving (i.e. > CDS) > - Simple API to replace adhoc comparisons with `os.name` > - Clear and consistent use across build, runtime, and JDK modules > > The PR includes updates within java.base to use the new API. To see the knock-on effects of using the API in other modules, see the Draft PR #12961 - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Thu, 9 Mar 2023 16:05:56 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/jdk/internal/misc/OperatingSystem.java line 98:
>>
>>> 96: @ForceInline
>>> 97: public static boolean isLinux() {
>>> 98: return OperatingSystemProps.TARGET_OS_IS_LINUX;
>>
>> Suggestion:
>>
>> return OperatingSystemProps.CURRENT_OS_ORDINAL == Linux.ordinal();
>>
>>
>> This will also simplify the template file as `TARGET_OS_IS_XXX` constants
>> are not needed.
>>
>> Also suggest to rename `TARGET_OS_ORDINAL` to `CURRENT_OS_ORDINAL` since it
>> represents the current OS (vs the build target).
>
> That would not yield a compile time constant.
> The TARGET_IS_XXX values must evaluate to compile time constants as evaluated
> by javac.
Using the naming from the build makes it clearer that there is a dependency
between the build names and those in the template.
-
PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Thu, 9 Mar 2023 17:07:28 GMT, Mandy Chung wrote: >> The one in the template file is independent to the build variables. > >> I would argue that we should keep the replacement string matching the make >> variable its getting the value from. It makes it easier to trace the origin >> of the value. > > Then we can define a new make variable that will also allow it be a different > value than `OPENJDK_TARGET_OS` if desirable. There is no need to define a new variable, there are already too many in the build and the build and the compiled files should be using the same symbols. - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
Hi Justin,
How would I go about building one of those? Or knowing what the
dependencies are?
Thanks, Roger
On 3/8/23 11:02 PM, Justin King wrote:
Let's please not kill generic BSD support if at all possible. There is
NetBSD, OpenBSD, FreeBSD, and DragonflyBSD. I know FreeBSD and NetBSD
have OpenJDK 19 and 17 respectively.
On Wed, Mar 8, 2023, 6:54 PM David Holmes wrote:
On Wed, 8 Mar 2023 19:15:16 GMT, Roger Riggs
wrote:
> Improvements to support OS specific customization for JDK
internal use:
> - To select values and code; allowing elimination of unused
code and values
> - Optionally evaluated by build processes, compilation, or
archiving (i.e. CDS)
> - Simple API to replace adhoc comparisons with `os.name
<http://os.name>`
> - Clear and consistent use across build, runtime, and JDK modules
>
> The PR includes updates within java.base to use the new API.
I guess I'm surprised this hasn't been done long before now. :)
Just a couple of drive by comments (I agree with comments made by
others).
Has this totally killed of BSD support on the JDK side? I thought
building non-macOS BSD was still viable, but perhaps not -
certainly not after this change.
Thanks
src/java.base/share/classes/jdk/internal/misc/OperatingSystem.java
line 48:
> 46: * For example,
> 47: * {@snippet lang = "java":
> 48: * if (OperatingSystem.current() == Windows) {
Doesn't `Windows` need to be prefixed with `OperatingSystem` here?
Ditto for dispatch example following.
src/java.base/share/classes/jdk/internal/misc/OperatingSystem.java
line 105:
> 103: */
> 104: @ForceInline
> 105: public static boolean isMac() {
suggestion: isMacOS
-
PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Wed, 8 Mar 2023 23:09:06 GMT, Mandy Chung wrote: >> Improvements to support OS specific customization for JDK internal use: >> - To select values and code; allowing elimination of unused code and values >> - Optionally evaluated by build processes, compilation, or archiving (i.e. >> CDS) >> - Simple API to replace adhoc comparisons with `os.name` >> - Clear and consistent use across build, runtime, and JDK modules >> >> The PR includes updates within java.base to use the new API. > > src/java.base/share/classes/jdk/internal/misc/OperatingSystem.java line 85: > >> 83: ; >> 84: >> 85: // Cache a copy of the array for lightweight indexing > > This is a reference to the Enum values array (not a copy). The implementation of OperatingSystem.values() does a clone() of the array. It would be risky to allow the enum to change its own members. - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Thu, 9 Mar 2023 00:45:02 GMT, Naoto Sato wrote:
>> Improvements to support OS specific customization for JDK internal use:
>> - To select values and code; allowing elimination of unused code and values
>> - Optionally evaluated by build processes, compilation, or archiving (i.e.
>> CDS)
>> - Simple API to replace adhoc comparisons with `os.name`
>> - Clear and consistent use across build, runtime, and JDK modules
>>
>> The PR includes updates within java.base to use the new API.
>
> src/java.base/unix/classes/java/lang/ProcessImpl.java line 106:
>
>> 104: try {
>> 105: // Should be value of a LaunchMechanism enum
>> 106: LaunchMechanism lm =
>> LaunchMechanism.valueOf(s.toUpperCase(Locale.ENGLISH));
>
> I think `Locale.ROOT` is preferred here.
ok, but not strictly in scope for this PR; that's pre-existing code.
-
PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Thu, 9 Mar 2023 15:24:04 GMT, Erik Joelsson wrote: >> src/java.base/share/classes/jdk/internal/misc/OperatingSystemProps.java.template >> line 39: >> >>> 37: >>> 38: // Index/ordinal of the current OperatingSystem enum as substituted >>> by the build >>> 39: static final int TARGET_OS_ORDINAL = >>> TARGET_OS_@@OPENJDK_TARGET_OS@@; >> >> This represents the current OS. What about renaming it to: >> Suggestion: >> >> static final int CURRENT_OS_ORDINAL = @@OS_NAME@@; > > I would argue that we should keep the replacement string matching the make > variable its getting the value from. It makes it easier to trace the origin > of the value. > > Using the terms `CURRENT` or `TARGET` inside the java class is less clear. > `TARGET` is a references to how the JDK was built while `CURRENT` is more of > a reference to the current runtime. Both are correct, it's more about what > mental model we want at this level. The symbol has to match the build usage of OPENJDK_TARGET_OS, not the name. - PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Wed, 8 Mar 2023 23:11:34 GMT, Mandy Chung wrote:
>> Improvements to support OS specific customization for JDK internal use:
>> - To select values and code; allowing elimination of unused code and values
>> - Optionally evaluated by build processes, compilation, or archiving (i.e.
>> CDS)
>> - Simple API to replace adhoc comparisons with `os.name`
>> - Clear and consistent use across build, runtime, and JDK modules
>>
>> The PR includes updates within java.base to use the new API.
>
> src/java.base/share/classes/jdk/internal/misc/OperatingSystem.java line 98:
>
>> 96: @ForceInline
>> 97: public static boolean isLinux() {
>> 98: return OperatingSystemProps.TARGET_OS_IS_LINUX;
>
> Suggestion:
>
> return OperatingSystemProps.CURRENT_OS_ORDINAL == Linux.ordinal();
>
>
> This will also simplify the template file as `TARGET_OS_IS_XXX` constants are
> not needed.
>
> Also suggest to rename `TARGET_OS_ORDINAL` to `CURRENT_OS_ORDINAL` since it
> represents the current OS (vs the build target).
That would not yield a compile time constant.
The TARGET_IS_XXX values must evaluate to compile time constants as evaluated
by javac.
> src/java.base/share/classes/jdk/internal/misc/OperatingSystemProps.java.template
> line 29:
>
>> 27: * @see OperatingSystem
>> 28: */
>> 29: class OperatingSystemProps {
>
> Have you considered to include OS architecture here for future use? So this
> file be `PlatformProps.java.template` instead.
>
> `jdk.tools.jlink.internal.Platform` needs to get the runtime OS and
> architecture.
I plan to come back to os.arch, it has a similar but not identical requirements.
-
PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Wed, 8 Mar 2023 22:49:05 GMT, Raffaello Giulietti
wrote:
>> Improvements to support OS specific customization for JDK internal use:
>> - To select values and code; allowing elimination of unused code and values
>> - Optionally evaluated by build processes, compilation, or archiving (i.e.
>> CDS)
>> - Simple API to replace adhoc comparisons with `os.name`
>> - Clear and consistent use across build, runtime, and JDK modules
>>
>> The PR includes updates within java.base to use the new API.
>
> src/java.base/share/classes/jdk/internal/misc/OperatingSystem.java line 74:
>
>> 72: * The Mac OS X Operating system.
>> 73: */
>> 74: Mac("Mac OS X"),
>
> The current spelling used by Apple is "macOS", probably to align to other
> Apple systems (iOS, iPadOS, etc.).
Names and branding have changed over the years.
It may be prudent to remove the name entirely and stick to a 'generic'
identification of the OS as the Enum name.
> src/java.base/share/classes/jdk/internal/misc/OperatingSystemProps.java.template
> line 34:
>
>> 32: // The values must match the ordinals of the respective enum
>> 33: private static final int TARGET_OS_linux = 0;
>> 34: private static final int TARGET_OS_macosx = 1;
>
> `TARGET_OS_macos` maybe?
These names need to match the names used in the build makefiles; currently is
macosx.
-
PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303485: Replacing os.name for operating system customization
On Thu, 9 Mar 2023 02:51:24 GMT, David Holmes wrote:
> Has this totally killed of BSD support on the JDK side? I thought building
> non-macOS BSD was still viable, but perhaps not - certainly not after this
> change.
I haven't found any use of BSD and I don't think the build supports a BSD build.
> src/java.base/share/classes/jdk/internal/misc/OperatingSystem.java line 48:
>
>> 46: * For example,
>> 47: * {@snippet lang = "java":
>> 48: * if (OperatingSystem.current() == Windows) {
>
> Doesn't `Windows` need to be prefixed with `OperatingSystem` here? Ditto for
> dispatch example following.
Yes, either a static import or a qualified name is needed.
(This is internal javadoc).
> src/java.base/share/classes/jdk/internal/misc/OperatingSystem.java line 105:
>
>> 103: */
>> 104: @ForceInline
>> 105: public static boolean isMac() {
>
> suggestion: isMacOS
As above, sticking to a generic term can make this a bit less sensitive to
branding changes.
-
PR: https://git.openjdk.org/jdk/pull/12931
RFR: 8303485: Replacing os.name for operating system customization
Improvements to support OS specific customization for JDK internal use: - To select values and code; allowing elimination of unused code and values - Optionally evaluated by build processes, compilation, or archiving (i.e. CDS) - Simple API to replace adhoc comparisons with `os.name` - Clear and consistent use across build, runtime, and JDK modules The PR includes updates within java.base to use the new API. - Commit messages: - 8303485: Replacing os.name for operating system customization Changes: https://git.openjdk.org/jdk/pull/12931/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12931=00 Issue: https://bugs.openjdk.org/browse/JDK-8303485 Stats: 435 lines in 13 files changed: 322 ins; 39 del; 74 mod Patch: https://git.openjdk.org/jdk/pull/12931.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12931/head:pull/12931 PR: https://git.openjdk.org/jdk/pull/12931
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments [v2]
On Mon, 6 Mar 2023 20:22:48 GMT, Pavel Rappo wrote: >> Please review this superficial documentation cleanup that was triggered by >> unrelated analysis of doc comments in JDK API. >> >> The only effect that this multi-area PR has on the JDK API Documentation >> (i.e. the observable effect on the generated HTML pages) can be summarized >> as follows: >> >> >> diff -ur >> build/macosx-aarch64/images/docs-before/api/serialized-form.html >> build/macosx-aarch64/images/docs-after/api/serialized-form.html >> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html >> 2023-03-02 11:47:44 >> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html >> 2023-03-02 11:48:45 >> @@ -17084,7 +17084,7 @@ >> throws > href="java.base/java/io/IOException.html" title="class in >> java.io">IOException, >> ClassNotFoundException >> readObject is called to restore the >> state of the >> - (@code BasicPermission} from a stream. >> + BasicPermission from a stream. >> >> Parameters: >> s - the ObjectInputStream from which data >> is read >> >> Notes >> - >> >> * I'm not an expert in any of the affected areas, except for jdk.javadoc, >> and I was merely after misused tags. Because of that, I would appreciate >> reviews from experts in other areas. >> * I discovered many more issues than I included in this PR. The excluded >> issues seem to occur in infrequently updated third-party code (e.g. >> javax.xml), which I assume we shouldn't touch unless necessary. >> * I will update copyright years after (and if) the fix had been approved, as >> required. > > Pavel Rappo has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains two additional > commits since the last revision: > > - Merge branch 'master' into 8303480 > - Initial commit Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8298047: Remove all non-significant trailing whitespace from properties files [v2]
On Mon, 16 Jan 2023 16:50:06 GMT, Magnus Ihse Bursie wrote: >> [JDK-8295729](https://bugs.openjdk.org/browse/JDK-8295729) was created in an >> attempt to remove all trailing whitespace from properties files, and enable >> a jcheck verification that they did not come back, similar to other source >> code. It turned out that this was not so simple, however, since trailing >> whitespace in values is actually significant in properties files. >> >> To address this, I have opened four bugs on different component teams to >> either remove the trailing significant whitespace (if it is there >> erroneously), or convert it to the unicode sequence `\u0020`: >> [JDK-8298042](https://bugs.openjdk.org/browse/JDK-8298042), >> [JDK-8298044](https://bugs.openjdk.org/browse/JDK-8298044), >> [JDK-8298045](https://bugs.openjdk.org/browse/JDK-8298045) and >> [JDK-8298046](https://bugs.openjdk.org/browse/JDK-8298046). >> >> That leaves all the other trailing spaces, in blank lines and in the end of >> comments. These serve no purpose and should just be removed, and is what >> this issue concerns. >> >> When this and the four "significant whitespace" bugs are all finally >> integrated, we can finally turn on the verification in jcheck for properties >> files as well. >> >> The changes in this PR is based on >> [JDK-8295729](https://bugs.openjdk.org/browse/JDK-8295729), in #10792. I >> have restored all the "delete trailing whitespace" changes, except for those >> with significance. These changes were in turned created by running `find . >> -type f -iname "*.properties" | xargs gsed -i -e 's/[ \t]*$//'`. > > 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 two commits: > > - Merge branch 'master' into properties-eol-clean-safe > - 8298047: Remove all non-significant trailing whitespace from properties > files The non-client parts look fine. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/11491
Re: RFR: 8286394: Address possibly lossy conversions in jdk.naming.dns
On Fri, 7 Oct 2022 14:14:13 GMT, Darragh Clarke wrote: > Added an int cast for the `timeoutLeft` assignment, looking through the code > and other uses of the variable it seems to show no issues with lossy > conversion. As part of this I removed the file to disable the conversion > warning > > I tested this build and it doesn't give any warnings on this file LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10607
Re: RFR: 8294456: Fix misleading-indentation warnings in JDK
On Thu, 29 Sep 2022 13:11:03 GMT, Raffaello Giulietti wrote: > This fixes misleading indentations, which allows enabling the (currently > disabled) `misleading-indentation` warning flag on two `.gmk` files. LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/10487
Re: RFR: 8291360: Create entry points to expose low-level class file information [v3]
On Tue, 2 Aug 2022 14:12:44 GMT, Harold Seigel wrote: >> Please review this change to fix JDK-8291360. This fix adds entry points >> getClassFileVersion() and getClassAccessFlagsRaw() to class java.lang.Class. >> The new entry points return the current class's class file version and its >> raw access flags. >> >> The fix was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, >> and Mach5 tiers 1-3 on Linux x64. Additionally, the JCK lang, vm, and api >> tests and new regression tests were run locally on Linux x64. >> >> Thanks, Harold > > Harold Seigel has updated the pull request incrementally with one additional > commit since the last revision: > > add test case, minor renames LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/9688
Re: RFR: 8291360: Create entry points to expose low-level class file information
On Mon, 1 Aug 2022 16:38:48 GMT, Harold Seigel wrote: >> test/hotspot/jtreg/runtime/ClassFile/classAccessFlagsRaw.jcod line 25: >> >>> 23: */ >>> 24: >>> 25: // Class with ACC_SUPER set >> >> Can these classes be defined more succinctly either in Java or .asm? > > The jcod test files are relatively small so I don' think they need to be > rewritten using asm. ok, It mostly knowing which few of the lines are relevant to the test vs. just boilerplate to make a complete class. - PR: https://git.openjdk.org/jdk/pull/9688
Re: RFR: 8291360: Create entry points to expose low-level class file information
On Mon, 1 Aug 2022 16:44:22 GMT, Harold Seigel wrote: >> My companion question was whether the native code can for efficiently map >> from array class to component type than Java. > > I don't know whether Java or the JVM is faster but I would rather keep this > code in Java so that I don't have to change existing method > JVM_GetClassAccessFlags(). (It's used by Class.getClassAccessFlagsRaw().) Makes sense, re-using the existing method. - PR: https://git.openjdk.org/jdk/pull/9688
Re: RFR: 8291360: Create entry points to expose low-level class file information
On Sat, 30 Jul 2022 07:45:31 GMT, David Holmes wrote: >> It is usual for the Java code to do such checks and throw exceptions, so >> that the VM assumes it is correct and only asserts incase something has been >> missed on the Java side. > > Though in this case the Java code has defined behaviour for array types so it > is correct for the VM to assume this is not an array type and to assert if it > is. My companion question was whether the native code can for efficiently map from array class to component type than Java. - PR: https://git.openjdk.org/jdk/pull/9688
Re: RFR: 8291360: Create entry points to expose low-level class file information
On Fri, 29 Jul 2022 21:05:19 GMT, Roger Riggs wrote: >> Please review this change to fix JDK-8291360. This fix adds entry points >> getClassFileVersion() and getClassAccessFlagsRaw() to class java.lang.Class. >> The new entry points return the current class's class file version and its >> raw access flags. >> >> The fix was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, >> and Mach5 tiers 1-3 on Linux x64. Additionally, the JCK lang, vm, and api >> tests and new regression tests were run locally on Linux x64. >> >> Thanks, Harold > > src/hotspot/share/prims/jvm.cpp line 4059: > >> 4057: return JVM_CLASSFILE_MAJOR_VERSION; >> 4058: } >> 4059: assert(!java_lang_Class::as_Klass(mirror)->is_array_klass(), >> "unexpected array class"); > > Can this throw IllegalArgumentException instead. > Asserts only report problems when built with debug (Right?) Or, Can the VM do this traversal as/more efficiently than doing at the java level? - PR: https://git.openjdk.org/jdk/pull/9688
Re: RFR: 8291360: Create entry points to expose low-level class file information
On Fri, 29 Jul 2022 18:02:46 GMT, Harold Seigel wrote:
> Please review this change to fix JDK-8291360. This fix adds entry points
> getClassFileVersion() and getClassAccessFlagsRaw() to class java.lang.Class.
> The new entry points return the current class's class file version and its
> raw access flags.
>
> The fix was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows,
> and Mach5 tiers 1-3 on Linux x64. Additionally, the JCK lang, vm, and api
> tests and new regression tests were run locally on Linux x64.
>
> Thanks, Harold
src/hotspot/share/prims/jvm.cpp line 4059:
> 4057: return JVM_CLASSFILE_MAJOR_VERSION;
> 4058: }
> 4059: assert(!java_lang_Class::as_Klass(mirror)->is_array_klass(),
> "unexpected array class");
Can this throw IllegalArgumentException instead.
Asserts only report problems when built with debug (Right?)
test/hotspot/jtreg/runtime/ClassFile/ClassAccessFlagsRawTest.java line 59:
> 57: // test primitive array. should return ACC_ABSTRACT | ACC_FINAL
> | ACC_PUBLIC.
> 58: int flags = (int)m.invoke((new int[3]).getClass());
> 59: if (flags != 1041) {
Can this be a hex constant, It's easier to understand the bits.
Or assemble the flags from java.lang.reflect.Modifier.XXX static fields.
test/hotspot/jtreg/runtime/ClassFile/classAccessFlagsRaw.jcod line 25:
> 23: */
> 24:
> 25: // Class with ACC_SUPER set
Can these classes be defined more succinctly either in Java or .asm?
-
PR: https://git.openjdk.org/jdk/pull/9688
Re: RFR: 8288979: Improve CLDRConverter run time [v3]
On Thu, 23 Jun 2022 08:53:37 GMT, Daniel Jeliński wrote: >> This PR improves the performance of deduplication done by >> ResourceBundleGenerator. >> >> The original implementation compared every pair of values, requiring O(n^2) >> time. The new implementation uses a HashMap to find duplicates, trading off >> some extra memory consumption for O(n) computational complexity. In practice >> the time to generate jdk.localedata on my Linux VM files dropped from 14 to >> 8 seconds. >> >> The resulting files (under build/support/gensrc/java.base and >> jdk.localedata) have different contents; map iteration order depends on the >> insertion order, and the insertion order of the new implementation is >> different from the original. >> The files generated before and after this change have the same size. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Sort output LGTM - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.org/jdk/pull/9243
Re: RFR: 8288979: Improve CLDRConverter run time [v2]
On Wed, 22 Jun 2022 17:07:11 GMT, Daniel Jeliński wrote: >> make/jdk/src/classes/build/tools/cldrconverter/ResourceBundleGenerator.java >> line 146: >> >>> 144: // generic reduction of duplicated values >>> 145: Map newMap = new HashMap<>(map); >>> 146: Map dedup = new >>> HashMap<>(map.size()); >> >> LinkedHashMap could be used to retain the iteration order. >> Or TreeMap if some deterministic order was desirable. > > True. Which raises the question: do we need any arbitrary order? The original > code used a hashmap too. It preserved the original order only when no > duplicates were detected. A stable order is useful when comparing between builds (by a human). It also supports the goal of reproducible builds. @naotoj What do you think? - PR: https://git.openjdk.org/jdk/pull/9243
