On Fri, 1 May 2026 15:37:12 GMT, Kevin Walls <[email protected]> wrote:
> This implements "jcmd on core files" for Linux, and for MiniDumps on Windows > (MacOS is "future work"). > jcmd "revives" the VM memory and .so/.dll from the core/minidump, and runs > the existing native diagnostic command parser and command implementations. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Great PR! I have some comments. src/hotspot/os/posix/os_posix.cpp line 1655: > 1653: > 1654: jlong os::elapsed_counter() { > 1655: return os::javaTimeNanos() - _initial_time_count; I guess `os::elapsed_counter()` should not be called in revival mode because the result is wrong due to `_initial_time_count` is zero. So should the assert be inserted as following? Suggestion: assert(!Thread::is_revived(), "must be"); return os::javaTimeNanos() - _initial_time_count; src/hotspot/share/runtime/thread.cpp line 611: > 609: > 610: void* Thread::process_revival() { > 611: _revived_vm = true; Should we access it with atomic operation? src/hotspot/share/runtime/thread.cpp line 617: > 615: #ifdef _WINDOWS > 616: os::win32::revive_init(); > 617: #endif I think it is better to throw fatal error explicitly on unsupported platforms as following. Suggestion: #ifdef LINUX os::Linux::revive_init(); #elif defined _WINDOWS os::win32::revive_init(); #else fatal("revival mode is not supported on this platform"); #endif src/hotspot/share/runtime/thread.cpp line 644: > 642: Mutex::revive_all(); > 643: > 644: struct revival_data* rdata = &vm_revival_data; I think `rdata` is not needed - we can use `vm_revival_data` directly. src/hotspot/share/runtime/thread.cpp line 649: > 647: rdata->version = REVIVAL_VERSION; > 648: rdata->size_this = sizeof(vm_revival_data); > 649: rdata->status = 1; What does "1" mean in this case? I think it is better to be `enum` if possible. src/hotspot/share/services/diagnosticCommand.cpp line 546: > 544: } > 545: // Paprallel threads request ignored if revived. > 546: uint parallel_thread_num = (num == 0) Typo: Paprallel -> Parallel ? src/jdk.attach/linux/native/revivalhelper/elffile.cpp line 1: > 1: /* HotSpot and SA already have ELF parser. Can we unify them? src/jdk.attach/linux/native/revivalhelper/revival_linux.cpp line 1: > 1: /* Most of the code in this would be able to be leveraged with SA code... src/jdk.attach/linux/native/revivalhelper/revival_linux.cpp line 61: > 59: uint64_t bin_addr; > 60: uint64_t bin_end; > 61: #define LIBC_NAME "libc.so.6" Can it work on musl (Alpine)? src/jdk.attach/share/classes/com/sun/tools/attach/VirtualMachine.java line 276: > 274: } > 275: AttachNotSupportedException lastExc = null; > 276: for (AttachProvider provider: providers) { I'm not sure, but it might be better to insert blank. Suggestion: for (AttachProvider provider : providers) { src/jdk.attach/share/classes/sun/tools/attach/VirtualMachineCoreDumpImpl.java line 97: > 95: throw new IOException("Truncated file '" + filename + > "'"); > 96: } > 97: if (System.getProperty("os.name").startsWith("Windows")) { Can we use `jdk.internal.util.OperatingSystem` to identify OS? src/jdk.attach/share/classes/sun/tools/attach/VirtualMachineCoreDumpImpl.java line 142: > 140: > 141: // Invoke "JDK/lib/revivalhelper corefilename jcmd command..." > 142: String jdkLibDir = System.getProperty("java.home") + > File.separator + "lib"; It might be simple as following: Suggestion: String jdkLibDir = Path.of(System.getProperty("java.home"), "lib").toString(); src/jdk.attach/share/classes/sun/tools/attach/VirtualMachineCoreDumpImpl.java line 241: > 239: private static String drain(InputStream is) throws IOException { > 240: StringBuilder sb = new StringBuilder(); > 241: try (BufferedReader br = new BufferedReader(new > InputStreamReader(is, UTF_8))) { Is it ok to close `is` after `drain()` call? src/jdk.attach/share/classes/sun/tools/attach/VirtualMachineCoreDumpImpl.java line 247: > 245: } > 246: } > 247: return sb.toString(); We can simplify this method like: Suggestion: try (var reader = new InputStreamReader(is, UTF_8)))) { return reader.readAllAsString(); } src/jdk.attach/share/native/librevival_support/revival_support.c line 34: > 32: > 33: #define TRUE 1 > 34: #define FALSE 0 `TRUE` and `FALSE` are defined on Windows by default. So it might be better as following: Suggestion: #ifndef TRUE #define TRUE 1 #endif #ifndef FALSE #define FALSE 0 #endif src/jdk.attach/share/native/revivalhelper/revival.cpp line 721: > 719: */ > 720: char* find_filename_in_one_dir(const char* dir, const char* filename) { > 721: char path[BUFLEN]; Should we use `PATH_MAX` rather than `BUFLEN`? src/jdk.attach/share/native/revivalhelper/revival.hpp line 84: > 82: #define PATH_SEPARATOR ":" > 83: > 84: #define SYM_VM_RELEASE "_ZN19Abstract_VM_Version13_s_vm_releaseE" This symbol does not seem to be exposed, thus it is better to find it in debuginfo if possible like SA. ------------- PR Review: https://git.openjdk.org/jdk/pull/31011#pullrequestreview-4214318465 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176032075 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176463998 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176457636 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176459514 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176460351 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176467432 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176469553 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176473421 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176470885 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176476681 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176479519 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176482363 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176486246 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176490043 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176517125 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176530373 PR Review Comment: https://git.openjdk.org/jdk/pull/31011#discussion_r3176527158
