On Wed, 12 Apr 2023 10:43:31 GMT, Markus Grönlund <mgron...@openjdk.org> wrote:
>> src/hotspot/share/prims/jvmtiAgent.cpp line 323:
>>
>>> 321: assert(agent != nullptr, "invariant");
>>> 322: if (!agent->is_loaded()) {
>>> 323: if (!load_agent_from_executable(agent, on_load_symbols,
>>> num_symbol_entries)) {
>>
>> It feels like I'm missing something.
>> We already checked and found at line 322 that `agent->is_loaded() == false`.
>> Also, we have the comment at line 265:
>>
>> 265 // If this function returns true, then agent->is_static_lib().&&
>> agent->is_loaded().
>> 266 static bool load_agent_from_executable(Agent* agent, const char*
>> on_load_symbols[], size_t num_symbol_entries) {
>>
>> As the `agent->is_loaded() == false` then t he condition
>> `agent->is_static_lib() && agent->is_loaded()` has to be `false` and can not
>> be `true`. Then one of the if-checks at lines 322 and 323 is not needed and
>> can be removed. Is it right? Otherwise, the comment at line 265 can be
>> incorrect.
>
> Good observation, Serguei.
>
> It is because some paths call into lookup_On_load_Entry_point() twice.
>
> It is primarily the attempted conversion of xrun agents, the first invocation
> comes from JvmtiAgent::convert_xrun_agent(). This will have the agent
> "loaded". If there is an Agent_OnLoad function, the agent is converted (i.e.
> xrun removed).
>
> Then when the agent is to invoke the Agent_OnLoad function, there is a second
> invocation. Here a converted xrun library is already loaded, so I bypass
> attempting to load it again by checking the is_loaded() property.
Thanks.
>> src/hotspot/share/prims/jvmtiAgent.cpp line 357:
>>
>>> 355: vm_exit_during_initialization("Could not find JVM_OnLoad or
>>> Agent_OnLoad function in the library", name());
>>> 356: }
>>> 357: _xrun = false; // converted
>>
>> Just questions to understand it better.
>> Neither `JVM_Onload` nor `Agent_Onload` entry points are stored after these
>> lookups. It means that in order to be called later (as the comment at line
>> 350 says) they have to be looked up again.
>> Is it right? Was it the same originally?
>
> The entry points are not saved and so have to be looked up again. It was the
> same originally.
>
> That is why there is a check and branch on agent->is_loaded().
Thank you. I'm okay with it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1165306181
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1165307170
