On Thu, 1 Jun 2023 05:20:31 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> 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);`
> 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)?
JvmtiAgentList::load_agent creates the JvmtiAgent, attempts to load it, and
adds it to the agent list if is succeeds. The test that you are looking is
checking the list of already loaded agents. Maybe the function name "is_loaded"
is the confusion here, maybe a better name is needed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1212610098
