On Wed, 31 May 2023 20:21:15 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Alan Bateman has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 17 additional >> commits since the last revision: >> >> - Add impl note to document the XX option >> - Cleanup >> - Merge >> - Allow for warning to be skipped when same agent loaded a >> second/subsequent time >> - Merge >> - Tweak javadoc, update test to use more test infra >> - Merge >> - Merge >> - Refresh package description >> - Merge >> - ... and 7 more: https://git.openjdk.org/jdk/compare/a3c18c96...2d9d5922 > > src/hotspot/share/prims/jvmtiAgent.cpp line 512: > >> 510: >> 511: // Print warning if EnableDynamicAgentLoading not enabled on the >> command line >> 512: if (!FLAG_IS_CMDLINE(EnableDynamicAgentLoading) && >> !agent->is_instrument_lib() && !JvmtiAgentList::is_loaded(library)) { > > The use of `!JvmtiAgentList::is_loaded(library)` here is a bit confusing. > Isn't the library always already loaded by the time we get here (the assert > below seems to imply that)? If so, wouldn't it already be in the list? If > it's not in the list yet, perhaps a comment explaining why would be helpful > here. It looks like you are right. There is also and assert at line 519: `519 assert(agent->is_loaded(), "invariant");` So, the agent has to be loaded if we got to the line 512. Also, there is a statement at line 507 (before line 512): `507 agent->set_os_lib(library);` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212584288
