On Mon, 18 May 2026 12:59:57 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). > > Kevin Walls has updated the pull request incrementally with one additional > commit since the last revision: > > Attach API implNote I checked the API docs in the latest commit and I have more comments on the API changes: 1. Both VirtualThread.attach methods need to make it clear how they delegate to the provider. It's not clear right now unless you click on "attachVirtualMachine" to see if it calls the 1-arg or 2-arg attachVirtualMachine method. The simplest way to fix this is to change ".. attachVirtualMachine} method in turn" to ".. attachVirtualMachine(String)} method in turn", and same for the 2-arg method. The attach(VirtualMachineDescriptor) has a similar link and will need to be updated too. 2. Can you explain again why the 1-arg attach can't be specified to be equivalent to the 2-arg attach with an empty map? I'm sure this will come up in the CSR discussion. The API docs allow the env to be empty and the HotSpot provider should be able to distinguish a process ID from a file path. 3. "env" is specified to as "... provider-specific settings to configure the attach". I think this will need to be worded differently as the properties may be more than what is needed to attach. The examples you have aid lookup and configured a directory for caching, so they are more about supporting an attaching target. 4. It doesn't seem possible for an attach provider to throw if env parameter does not contain required properties, contains an invalid combination of properties, or a bad value. I realize you don't want to eagerly validates the values of libDirs and revivalCachePath but I think the provider interface has to allow for this, maybe a different provider implementation, maybe a future enhancement to this feature. The summary is that I think the 2-arg attach and SPI attachVirtualMachine method needs to specify IllegalArgumentException. 5. The IOException that the 2-arg attachVirtualMachine can throw is not specified. 6. The default implementation of 2-arg attachVirtualMachine throws UOE but that isn't currently specified (no `@throws` UOE. I realize we discussed in other comments here about invoking the 1-arg attachVirtualMachine when the id parses as an integer and the map is empty. Looking it again, I think the least surprising default is to just invoke the 1-arg attachVirtualMachine if the map is empty (no checking of id). Throw AttachNotSupportedException if not empty. I realize UOE is tempting, and I may have suggested it at one point, but this leaks into the API and it will hard to explain UOE vs. AttachNotSupportedException. ------------- PR Comment: https://git.openjdk.org/jdk/pull/31011#issuecomment-4480952596
