Re: RFR: 8332400: isspace argument should be a valid unsigned char [v2]
On Tue, 11 Jun 2024 18:07:10 GMT, Robert Toyonaga wrote: >> ### Summary >> This change ensures we don't get undefined behavior when >> calling[`isspace`](https://pubs.opengroup.org/onlinepubs/007904975/functions/isspace.html). >> `isspace` accepts an `int` argument that "the application shall ensure is >> a character representable as an unsigned char or equal to the value of the >> macro EOF.". >> >> Previously, there was no checking of the values passed to `isspace`. I've >> replaced direct calls with a new wrapper `os::is_space` that performs a >> range check and prevents the possibility of undefined behavior from >> happening. For instances outside of Hotspot, I've added casts to `unsigned >> char`. >> >> **Testing** >> - Added a new test in `test/hotspot/gtest/runtime/test_os.cpp` to check >> `os::is_space` is working correctly. >> - tier1 > > Robert Toyonaga has updated the pull request incrementally with one > additional commit since the last revision: > > Replace wrapper with casts. It was a good learning; You might want to update the copyright header for few of the files. Thanks, - Marked as reviewed by amitkumar (Committer). PR Review: https://git.openjdk.org/jdk/pull/19567#pullrequestreview-2112359981
Re: RFR: 8333566: Remove unused methods
On Tue, 4 Jun 2024 20:51:52 GMT, Cesar Soares Lucas wrote: > Please, consider this patch to remove unused methods from the code base. To > the best of my knowledge, these methods are only defined but never used. > > Here is a list with names of delete methods: > https://gist.github.com/JohnTortugo/fccc29781a1b584c03162aa4e160e874 > > Tested with Linux x86_64 tier1-4, GHA, and only cross building to other > platforms. src/hotspot/cpu/s390/vm_version_s390.hpp line 516: > 514: static void set_has_CompareTrap() { _features[0] |= > GnrlInstrExtFacilityMask; } > 515: static void set_has_RelativeLoadStore() { _features[0] |= > GnrlInstrExtFacilityMask; } > 516: static void set_has_ProcessorAssist() { _features[0] |= > ProcessorAssistMask; } This looks incorrect; there exist a second definition below; - PR Review Comment: https://git.openjdk.org/jdk/pull/19550#discussion_r1630102121
Re: RFR: 8333566: Remove unused methods
On Tue, 4 Jun 2024 20:51:52 GMT, Cesar Soares Lucas wrote: > Please, consider this patch to remove unused methods from the code base. To > the best of my knowledge, these methods are only defined but never used. > > Here is a list with names of delete methods: > https://gist.github.com/JohnTortugo/fccc29781a1b584c03162aa4e160e874 > > Tested with Linux x86_64 tier1-4, GHA, and only cross building to other > platforms. src/hotspot/cpu/s390/vm_version_s390.hpp line 516: > 514: static void set_has_CompareTrap() { _features[0] |= > GnrlInstrExtFacilityMask; } > 515: static void set_has_RelativeLoadStore() { _features[0] |= > GnrlInstrExtFacilityMask; } > 516: static void set_has_GnrlInstrExtensions() { _features[0] |= > GnrlInstrExtFacilityMask; } I know this PR is still in draft state. Just a thought: I would like to keep the methods in `vm_version_s390.hpp` file for now. I'm planning to remove the checks applicable to older hardware. So it would be better, If I clean these methods as a part of that PR :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/19550#discussion_r1628627936
Re: RFR: 8332923: ObjectMonitorUsage.java failed with unexpected waiter_count [v3]
On Thu, 30 May 2024 01:13:20 GMT, SendaoYan wrote: >> Hi all, >> ObjectMonitorUsage.java failed with `unexpected waiter_count` after >> [JDK-8328083](https://bugs.openjdk.org/browse/JDK-8328083) on linux x86_32. >> There are two changes in this PR: >> 1. In `JvmtiEnvBase::get_object_monitor_usage` function, change from >> `java_lang_VirtualThread::is_instance(thread_oop)` to >> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the >> alternative implementation. >> 2. In `Threads::get_pending_threads(ThreadsList *, int, address)` function >> of threads.cpp file, change from >> `java_lang_VirtualThread::is_instance(thread_oop)` to >> `thread_oop->is_a(vmClasses::BaseVirtualThread_klass())`to support the >> alternative implementation. This modified function only used by >> `JvmtiEnvBase::get_object_monitor_usage(JavaThread*, jobject, >> jvmtiMonitorUsage*)`, so the risk of the modified on threads.cpp file is low. >> >> >> >> Additional testing: >> - [x] linux x86_32 run all testcases in serviceability/jvmti, all testcases >> run successed expect >> `serviceability/jvmti/vthread/GetThreadState/GetThreadStateTest.java#default` >> run failed. This test also run failed before this PR, which has been >> recorded in [JDK-8333140](https://bugs.openjdk.org/browse/JDK-8333140) >> - [x] linux x86_64 run all testcases in serviceability/jvmti, all testcases >> run successed. > > SendaoYan has updated the pull request incrementally with one additional > commit since the last revision: > > change from java_lang_VirtualThread::is_instance(thread_oop) to > hread_oop->is_a(vmClasses::BaseVirtualThread_klass()) in > Threads::get_pending_threads() I have tested it on s390x as well. I don't see any new test failure appearing. Thanks for fixing it. - Marked as reviewed by amitkumar (Committer). PR Review: https://git.openjdk.org/jdk/pull/19405#pullrequestreview-2092069640
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes
On Fri, 29 Mar 2024 19:35:45 GMT, Vladimir Kozlov wrote: > Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE > [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) > which was used for AOT [JEP 295](https://openjdk.org/jeps/295) > implementation in JDK 9. The code was left in HotSpot assuming it will help > in a future. But during work on Leyden we decided to not use it. In Leyden > cached compiled code will be restored in CodeCache as normal nmethods: no > need to change VM's runtime and GC code to process them. > > I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce > header size in separate changes. In these changes I did simple fields > reordering to keep small (1 byte) fields together. > > I do not see (and not expected) performance difference with these changes. > > Tested tier1-5, xcomp, stress. Running performance testing. > > I need help with testing on platforms which Oracle does not support. I performed the build + testing `{fastdebug, release, slowdebug} X {tier1}` on `s390x` and result looks fine. - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2029655163
Re: RFR: JDK-8320005 : Allow loading of shared objects with .a extension on AIX [v22]
On Mon, 12 Feb 2024 18:04:21 GMT, Suchismith Roy wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Remove not matched trailing whitespaces Maybe the copyright year could be updated. Nothing else on my side. - Marked as reviewed by amitkumar (Committer). PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1881747083
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v2]
On Thu, 23 Nov 2023 08:08:51 GMT, suchismith1993 wrote: > The JBS issue with respect to that has been closed. Need to check if that PR > is required. Currently putting it on hold. This response on the issue suggest otherwise: : The JDK does not support dynamically loaded archive files (.a files) and there are no plans to add this support. Closing as will not fix - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1824009424
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v2]
On Wed, 22 Nov 2023 16:24:24 GMT, suchismith1993 wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > suchismith1993 has updated the pull request incrementally with one additional > commit since the last revision: > > change macro position Also are you planning to close this one : https://github.com/openjdk/jdk/pull/16490 ? JBS issue is already closed. - PR Comment: https://git.openjdk.org/jdk/pull/16604#issuecomment-1823849323
Re: RFR: JDK-8320005 : Native library suffix impact on hotspot code in AIX [v2]
On Wed, 22 Nov 2023 16:24:24 GMT, suchismith1993 wrote: >> J2SE agent does not start and throws error when it tries to find the shared >> library ibm_16_am. >> After searching for ibm_16_am.so ,the jvm agent throws and error as dll_load >> fails.It fails to identify the shared library ibm_16_am.a shared archive >> file on AIX. >> Hence we are providing a function which will additionally search for .a file >> on AIX ,when the search for .so file fails. > > suchismith1993 has updated the pull request incrementally with one additional > commit since the last revision: > > change macro position some nits you might want to consider. src/hotspot/os/aix/os_aix.cpp line 3064: > 3062: > 3063: //Replaces provided path with alternate path for the given file,if it > doesnt exist. > 3064: //For AIX,this replaces .so with .a. Suggestion: // Replaces the specified path with an alternative path for the given file if the original path doesn't exist. // For AIX, this replaces extension from ".so" to ".a". src/hotspot/os/aix/os_aix.cpp line 3065: > 3063: //Replaces provided path with alternate path for the given file,if it > doesnt exist. > 3064: //For AIX,this replaces .so with .a. > 3065: void os::Aix::mapAlternateName(char* buffer, const char *extension) { Suggestion: void os::Aix::map_alternate_name(char* buffer, const char *extension) { src/hotspot/os/aix/os_aix.hpp line 181: > 179: static int stat64x_via_LIBPATH(const char* path, struct stat64x* stat); > 180: // Provide alternate path name,if file does not exist. > 181: static void mapAlternateName(char* buffer, const char *extension); Suggestion: // Provides alternate path name, if file does not exist. static void map_alternate_name(char* buffer, const char *extension); - Changes requested by amitkumar (Committer). PR Review: https://git.openjdk.org/jdk/pull/16604#pullrequestreview-1745734586 PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402935976 PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402936171 PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1402936497
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code [v2]
On Tue, 30 May 2023 19:15:38 GMT, Johan Sjölen wrote: >> A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes >> I'd appreciate if this was considered trivial. > > Johan Sjölen has updated the pull request incrementally with two additional > commits since the last revision: > > - Align > - Suggestions Thanks for addressing the comment. Looks Good :-) - Marked as reviewed by amitkumar (Author). PR Review: https://git.openjdk.org/jdk/pull/14198#pullrequestreview-1452183165
Re: RFR: 8309044: Replace NULL with nullptr, final sweep of hotspot code
On Mon, 29 May 2023 10:09:15 GMT, Johan Sjölen wrote: > A final sweep of Hotspot to remove all re-added NULLs. With only 110 changes > I'd appreciate if this was considered trivial. not a review, but would you like to check if these could replaced as well :-) ./cpu/ppc/macroAssembler_ppc.hpp:735: void load_klass_check_null(Register dst, Register src, Label* is_null = NULL); ./cpu/ppc/stubGenerator_ppc.cpp:4700:if (UnsafeCopyMemory::_table == NULL) { ./cpu/riscv/codeBuffer_riscv.cpp:74: if (cb->stubs()->maybe_expand_to_ensure_remaining(total_requested_size) && cb->blob() == NULL) { ./share/include/jvm.h:423: * Find a class from a boot class loader. Returns NULL if class not found. ./share/include/cds.h:72: char* _mapped_base; // Actually mapped address (NULL if this region is not mapped). ./share/opto/runtime.cpp:491: fields[TypeFunc::Parms+0] = NULL; // void - PR Comment: https://git.openjdk.org/jdk/pull/14198#issuecomment-1567074248
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 for s390x, build is fine and tier1 (specifically `ArchTest.java`) passes. Thanks for the change. - Marked as reviewed by amitkumar (Author). PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1392455297
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v15]
On Wed, 19 Apr 2023 13:22:54 GMT, Martin Doerr wrote: >> 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 > > test/jdk/jdk/internal/util/ArchTest.java line 71: > >> 69: case "aarch64" -> AARCH64; >> 70: case "riscv64" -> RISCV64; >> 71: case "s390x", "s390" -> S390; // unverified > > This was also verified according to comments. Right, @offamitkumar? Yes, you're correct @TheRealMDoerr - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1171580811
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v10]
On Tue, 11 Apr 2023 18:07:41 GMT, Martin Doerr wrote: > Another remark: Old JDK on s390 used "os.arch = zArch_64", current one > "os.arch = s390x". @offamitkumar: You probably want to take a look. Martin, only concern was that I didn't have a good experience with `s390x` string in [past](https://github.com/openjdk/jdk/pull/13228#issuecomment-1488599607). But, I assume that `default` in `mapArchString` will unintentionally handle that scenario here. I tested it, and 'ArchTest.java' also passes. I guess @RealLucy might have some pointer over this. - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1504542573
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v14]
On Mon, 27 Mar 2023 14:43:04 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > RISCV patch and aarch64 improvement `{tier1, tier2} X {fast debug, slow debug, release}` testing done for s390x. PR seems clean. @matias9927 please include port for s390x from this commit: https://github.com/offamitkumar/jdk/commit/a582f32f97aefba33cebaf4ace540681dfc0eff5 Thanks src/hotspot/cpu/ppc/templateInterpreterGenerator_ppc.cpp line 652: > 650: // Scale the index to be the entry index * > sizeof(ResolvedInvokeDynamicInfo) > 651: __ sldi(size, size, log2i_exact(sizeof(ResolvedIndyEntry))); > 652: __ add(cache, cache, size); @reinrich Is there any specific reason, why you're not calling load_resolved_indy_entry() method here. On s390x build/changes are stable even with calling that helper method. - Marked as reviewed by amitkumar (Author). PR Review: https://git.openjdk.org/jdk/pull/12778#pullrequestreview-1361010070 PR Review Comment: https://git.openjdk.org/jdk/pull/12778#discussion_r1150566670
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v14]
On Mon, 27 Mar 2023 14:43:04 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > RISCV patch and aarch64 improvement Hi Matias, s390x port is almost complete. All builds are successful & tier1 test for fast debug are complete. For other builds, tests are in progress. Please don't integrate, Wait for us Thanks - PR Comment: https://git.openjdk.org/jdk/pull/12778#issuecomment-1486414339
Re: RFR: 8299795: Relativize locals in interpreter frames
On Tue, 17 Jan 2023 13:33:34 GMT, Martin Doerr wrote: >>> Works on PPC64. Thanks! Tests have passed on other platforms as well. >> >> Does "other platforms" include S390? > >> > Works on PPC64. Thanks! Tests have passed on other platforms as well. >> >> Does "other platforms" include S390? > > No, @backwaterred you may want to check. Hi @TheRealMDoerr and @fbredber , I've executed tier1 & tier2 test on slow, fast and release build on s390 and test-result seems fine. But please let me know if any specific tier I need to test. I would happily do it. - PR: https://git.openjdk.org/jdk/pull/11902