On Thu, 1 Jun 2023 06:46:41 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>>> 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.
>
> I see the source of my confusion.
> The function `invoke_Agent_OnAttach` is called from the `JvmtiAgent::load`.
> But at the time of `invoke_Agent_OnAttach` the agent has not been added to
> the list yet.
> It is added after the `JvmtiAgent::load` with the function
> `JvmtiAgentList::add`:
>
> jint JvmtiAgentList::load_agent(const char* agent_name, const char* absParam,
> const char* options, outputStream* st) {
> // The abs parameter should be "true" or "false"
> const bool is_absolute_path = (absParam != nullptr) && (strcmp(absParam,
> "true") == 0);
> JvmtiAgent* const agent = new JvmtiAgent(agent_name, options,
> is_absolute_path, /* dynamic agent */ true);
> if (agent->load(st)) {
> add(agent);
> . . .
>
> Now I see the code is correct.
> I'd suggest to add a small comment before the line 512 saying
> that the agent has not been included into the agents list yet.
> 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.
Yes, I did notice that there were two different `is_loaded` checks going on.
What's not clear is the relation between agent->is_loaded() and
JvmtiAgentList::is_loaded(library). I assume that at this point in time the
library is not on the list but the agent is loaded, so that's why it works, but
it's not clear to me why this is the case. I think a comment here explaining
why would be helpful so the reader doesn't need to track down when each of
these "loaded" conditions becomes true.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13899#discussion_r1213314470
