On Wed, 12 Apr 2023 10:31:37 GMT, Serguei Spitsyn <[email protected]> wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> renames
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1163954754
