On Tue, 23 Jan 2024 03:31:59 GMT, Dean Long <dl...@openjdk.org> wrote:
>> Volker Simonis has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Guard the feature with a diagnostic option and update the comments in the
>> code
>
> src/hotspot/share/runtime/init.cpp line 121:
>
>> 119: if (AlwaysRecordEvolDependencies) {
>> 120: JvmtiExport::set_can_hotswap_or_post_breakpoint(true);
>> 121: JvmtiExport::set_all_dependencies_are_recorded(true);
>
> I think the check for AlwaysRecordEvolDependencies needs to be moved into
> set_can_hotswap_or_post_breakpoint and set_all_dependencies_are_recorded,
> otherwise don't we risk the value being accidentally reset to false when
> set_can_hotswap_or_post_breakpoint() is called again by
> JvmtiManageCapabilities::update()?
A good question, but after deep digging (it took me quite some time to figure
this out myself :) I don't think that
`can_hotswap_or_post_breakpoint`/`all_dependencies_are_recorded` can ever be
reset. Here's how it works:
- there's a global set of `always_capabilities` which is initialized when the
first JVMTI environment is created:
// capabilities which are always potentially available
jvmtiCapabilities JvmtiManageCapabilities::always_capabilities;
void JvmtiManageCapabilities::initialize() {
_capabilities_lock = new Mutex(Mutex::nosafepoint, "Capabilities_lock");
always_capabilities = init_always_capabilities();
- as the name implies, this set of capabilities contains all generally
available capabilities (except for the `onload` and `solo` capabilites, which
are maintained in separate sets).
- `JvmtiManageCapabilities::update()` only operates on this global sets of
capabilites (and *not* on the concrete capabilities of a specific JVMTI
environment):
void JvmtiManageCapabilities::update() {
jvmtiCapabilities avail;
// all capabilities
either(&always_capabilities, &always_solo_capabilities, &avail);
...
// If can_redefine_classes is enabled in the onload phase then we know that
the
// dependency information recorded by the compiler is complete.
if ((avail.can_redefine_classes || avail.can_retransform_classes) &&
JvmtiEnv::get_phase() == JVMTI_PHASE_ONLOAD) {
JvmtiExport::set_all_dependencies_are_recorded(true);
}
...
JvmtiExport::set_can_hotswap_or_post_breakpoint(
avail.can_generate_breakpoint_events ||
avail.can_redefine_classes ||
avail.can_retransform_classes);
- This means that `JvmtiManageCapabilities::update()` is always conservative
regarding its exports to `JvmtiExport` as it always exports *all* potentially
available capabilites once a JVMTI environment is created and not only the
specific capabilities requested by concrete JVMTI environment. This means that
even if we create a JVMTI agent with an empty set of capabilities,
`can_hotswap_or_post_breakpoint`/`all_dependencies_are_recorded` will still be
set in `JvmtiExport`.
- There's no code path (at least I couldn't find one) which takes capabilities
away from the `always_capabilities` set. Only if an environment requests
`on_load` capabilities, they will be transferred from the global
`onload_capabilities` to the `always_capabilities` set:
jvmtiError JvmtiManageCapabilities::add_capabilities(const jvmtiCapabilities
*current,
const jvmtiCapabilities
*prohibited,
const jvmtiCapabilities
*desired,
jvmtiCapabilities *result)
{
...
// onload capabilities that got added are now permanent - so, also remove
from onload
both(&onload_capabilities, desired, &temp);
either(&always_capabilities, &temp, &always_capabilities);
exclude(&onload_capabilities, &temp, &onload_capabilities);
If you like I could put an assertion into
`JvmtiExport::set_can_hotswap_or_post_breakpoint()` which verifies that
`can_hotswap_or_post_breakpoint` never gets reset once it was set to `true`.
What do you think?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17509#discussion_r1463584725
