Clean up J9HookDisable calls in the compiler

[dsouzai]

The compiler uses J9HookDisable not to disable a hook but rather to check if the hook is disabled. However, this can have unintended consequences in CRIU mode (see #16959). Therefore, this PR cleans up the code to check the flag rather than check if J9HookDisable fails.

[dsouzai]

@pshipton could you please review?

[pshipton]

Does this depend on something not yet merged or promoted? I don't find any J9_EVENT_CAN_BE_HOOKED.

jenkins compile amac jdk11

[dsouzai]

Does the depend on something not yet merged or promoted? I don't find any J9_EVENT_CAN_BE_HOOKED.

I added J9_EVENT_CAN_BE_HOOKED here https://github.com/eclipse-openj9/openj9/pull/16960/files#diff-51af6fe7c0bf6f4488a519463f637a9723ddbaf483132564b42976597c19d664R28; I wanted to run especially the change to this file by you.

[pshipton]

Why is this added to OpenJ9 rather than OMR?

[dsouzai]

Partly to minimize the number of PRs needed, but also because usually if the name has J9 in it I try as much as possible to keep in the openj9 codebase. However, if you think I should move it to OMR I can do that and open that PR.

[pshipton]

I think it should be in OMR. The macros J9_EVENT_IS_HOOKED and J9_EVENT_IS_RESERVED are defined in omrhookable.h

[dsouzai]

OK, will do (pending GAC's blessing wrt the rest of the changes in this PR).

[DanHeidinga]

Has someone run this past @gacholio ? The interactions between various pieces of which hooks are disabled (or not) was IMHO subtle and finicky with GAC being the one most likely to remember why it was designed the way it was....

[dsouzai]

No I haven't run it past GAC; @gacholio do you know if these compiler changes have any unintended consequences? Because the VM has to still function normally if the compiler isn't loaded such as with -Xint, I wouldn't expect there to be an implicit dependency on the compiler making these J9HookDisable calls. I did see these calls in vmhook.c, so I imagine that's probably what can't be removed if there is some implicit dependency, though if such a dependency does exist then it should be documented.

[gacholio]

I don't believe this is correct. It's important that the hooks be disabled - the JIT will generate code assuming that it doesn't need to support a hookable event. If the hook is not disabled, JVMTI will allow an agent to acquire capabilities which can not be supported by the compiled code.

[dsouzai]

I don't believe this is correct. It's important that the hooks be disabled - the JIT will generate code assuming that it doesn't need to support a hookable event. If the hook is not disabled, JVMTI will allow an agent to acquire capabilities which can not be supported by the compiled code.

I do agree that the hooks should be disabled (and not left floating in an uninitialized state). However, aren't the calls to J9HookDisable in vmhook.c sufficient? Also is it even a good idea for the VM to depend on the JIT to set the correct state for these hooks? If for example -Xnojit -Xnoaot or -Xint is set, then the compiler is not going to make these calls. That's why I didn't try to update the VM code that calls J9HookDisable because I don't know the circumstances under which they're called, but I do know that the compiler only uses the call to determine whether the hook is enabled or not. I don't think the compiler should be the one actually initializing the value of the hook though.

[gacholio]

Example: The JIT says "I can't support the exception catch event/hook because the code generation is going to be optimized using throwToGoto", implemented by checking and disabling the related hook.

If the -Xint case, the hook will simply not be disabled, allowing any agent to acquire the matching JVMTI capability.

[dsouzai]

Example: The JIT says "I can't support the exception catch event/hook because the code generation is going to be optimized using throwToGoto", implemented by checking and disabling the related hook.

The JIT does inverse; it generates throwToGoto by default unless the hook is enabled, in which case it disables throwToGoto. It doesn't try to enforce an optimization by disabling a hook, at least not intentionally, and by that I mean:

It is my understanding is that if JVMTI is enabled, then the agent needs to inform the VM of the capabilities it will want (even if it's late attach). If one of these capabilities is for example being informed about a exception catch, then J9_EVENT_IS_RESERVED(interface, J9HOOK_VM_EXCEPTION_CATCH) should return TRUE. If that's the case, then ((*vmHooks)->J9HookDisable(vmHooks, J9HOOK_VM_EXCEPTION_CATCH) != 0) will be true, and so the JIT will disable throwToGoto. However, if my assumption about JVMTI is wrong in that the agent does not need to inform the VM of the capabilities it needs for late attach, then I suppose the JIT, as a side-effect of checking if the event is enabled, ends up preventing the agent from being able to ask for exception catch events.

[gacholio]

This is not correct. Late attach agents request capabilties when they are loaded, so if you have not disabled the exception catch hook, the agent will be able to acquire the capability, but no compiled code that used throwToGoto would ever generate the hook call to report the event.

[dsouzai]

Ah I understand. Well that complicates things for CRIU mode then...

Thanks for explaining it, this is indeed very subtle. Thanks @DanHeidinga for word of caution.

Closing this since this change is fundamentally wrong.