On Mon, 17 Mar 2025 18:26:57 GMT, Larry Cable <duke at openjdk.org> wrote:
> on both Linux and MacOS libattach utilizes UNIX signal (QUIT) to cause a target JVM (attachee) to create the socket file used as transport for subsequent jcmds (and other attach based interactions) and to listen upon that for such.
>
> it should be noted that the default behavior for QUIT (if not blocked or caught) is to terminate the signalled process.
>
> during the early lifetime of a JVM, its signal handlers are not yet installed, and thus any signal such as QUIT will cause the
> default behavior to occur, in this case the JVM will be terminated.
>
> this is why some tests are failing with "not alive"
>
> the "fix" is similar in nature to that already implemented for linux (however using a different OS dependent mechanism to obtain the attachee JVM's signal masks: sysctl(2)).
>
> the method "checkCatchesAndSendQuitTo" will now obtain the "attachee" JVM signal masks and only kill(QUIT) if the
> current masks indicate that the JVM's signals are now being handled.
>
> the behavior in the success case is now identical to the previous implementation, however should the target JVM not
> become "ready" (signal handlers installed) prior to the attach "timeout" occurring the attach operation will throw an
> "AttachNotSupportedException" with a suitable error message.
>
> see also: https://bugs.openjdk.org/browse/JDK-8350766
Thanks very much for fixing this @larry-cable ! Changes look good. Just a few minor nits.
Also you need to remove the ProblemList entries in test/jdk/ProblemList.txt.
src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 126:
> 124:
> 125: /*
> 126: * early in the lifetime of a JVM it has not yet initialized its signal handlers, in particular the QUIT
Suggestion:
* Early in the lifetime of a JVM it has not yet initialized its signal handlers, in particular the QUIT
src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 129:
> 127: * handler, note that the default behavior of QUIT is to terminate the receiving process, if unhandled.
> 128: *
> 129: * since we use QUIT to initiate an attach operation, if we signal a JVM during this period early in its
Suggestion:
* Since we use QUIT to initiate an attach operation, if we signal a JVM during this period early in its
src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 133:
> 131: * are attempting to attach to!
> 132: *
> 133: * the following code guards the QUIT delivery by testing the current signal masks
Suggestion:
* The following code guards the QUIT delivery by testing the current signal masks. It is okay to send QUIT
* if the signal is caught but not ignored, as that implies a handler has been installed.
src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 138:
> 136: if (sysctl(mib, sizeof(mib) / sizeof(int), &kiproc, &kipsz, NULL, 0) == 0) {
> 137: const int ignored = (kiproc.kp_proc.p_sigignore & sigmask(SIGQUIT)) != 0;
> 138: const int caught = (kiproc.kp_proc.p_sigcatch & sigmask(SIGQUIT)) != 0;
Suggestion:
const bool ignored = (kiproc.kp_proc.p_sigignore & sigmask(SIGQUIT)) != 0;
const bool caught = (kiproc.kp_proc.p_sigcatch & sigmask(SIGQUIT)) != 0;
src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 140:
> 138: const int caught = (kiproc.kp_proc.p_sigcatch & sigmask(SIGQUIT)) != 0;
> 139:
> 140: // *only* send QUIT if the target is ready to catch and handle the signal to avoid default "death" if not
Delete this comment.
src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 143:
> 141:
> 142: // note: obviously the masks could change between testing and signalling however this is not the
> 143: // observed behavior of the current JVM implementation.
The mask should only change in one direction - from default behaviour to being handled - so this "race" is not a concern.
src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 146:
> 144:
> 145: if (caught && !ignored) {
> 146: if (kill((pid_t)pid, SIGQUIT)) {
```suggestion (no implicit booleans)
if (kill((pid_t)pid, SIGQUIT) != 0) {
src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 154:
> 152: char msg[100];
> 153:
> 154: snprintf(msg, sizeof(msg), "%d: state is not ready to participate in attach handshake!", (int)pid);
Suggestion:
snprintf(msg, sizeof(msg), "%d: process state is not ready to participate in attach handshake!", (int)pid);
Without the `cmd` the original message didn't quite read right.
-------------
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/24085#pullrequestreview-2692365572
PR Comment: https://git.openjdk.org/jdk/pull/24085#issuecomment-2731246857
PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999848252
PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999849256
PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999849992
PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999874844
PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999865604
PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999864741
PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999868536
PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999869609