On Fri, 18 Nov 2022 06:15:39 GMT, Serguei Spitsyn <[email protected]> wrote:
>> If there are no command line agents, then on startup
>> `vthread_notify_jvmti_events` is not set true. Because it is not true, when
>> `javaClasses_init()` calls `init_static_notify_jvmti_events()`, it does
>> nothing. The whole point of the code we are reviewing here is to make sure
>> `init_static_notify_jvmti_events()` is called while
>> `vthread_notify_jvmti_events == true` so it actually does something.
>> However, the code here does not bother calling
>> `init_static_notify_jvmti_events()` if the current thread is detached, but
>> it does still set `vthread_notify_jvmti_events = true`. This means that if
>> this code gets called a second time, this time with the current thread
>> attached, it will not call `init_static_notify_jvmti_events()` due to
>> `vthread_notify_jvmti_events == true`, but it seems it should be calling it.
>>
>> What I believe to be the flaw here is that you call
>> `set_notify_jvmti_events(true)` even if you don't call
>> `init_static_notify_jvmti_events()`.
>
>> What I believe to be the flaw here is that you call
>> `set_notify_jvmti_events(true)`
>> even if you don't call `init_static_notify_jvmti_events()`.
>
> This only happen in a detached thread case which can be only at startup.
> It was implemented this way before my fix.
> The javaClasses has to be partially initialized before any call to
> `init_static_notify_jvmti_events()`:
>
> void javaClasses_init() {
> JavaClasses::compute_offsets(); <== This is needed
> JavaClasses::check_offsets();
> java_lang_VirtualThread::init_static_notify_jvmti_events();
> FilteredFieldsMap::initialize(); // must be done after computing offsets.
> }
>
>
> For detached thread case (which is at starup) it is guaranteed that the
> `init_static_notify_jvmti_events()` is unconditionally called later at
> initialization stage. Please, see the chain of calls:
> `create_vm() => init_globals() => javaClasses_init() =>
> java_lang_VirtualThread::init_static_notify_jvmti_events()`.
>
> There have to be no cases with a call to `set_notify_jvmti_events(true)`
> without a following call to `init_static_notify_jvmti_events()`:
> - detached thread case: the `init_static_notify_jvmti_events()` is called
> later at startup sequence
> - JavaThread case: the `init_static_notify_jvmti_events()` is always called
> explicitly after the call to `set_notify_jvmti_events(true)`
>
> You can think about the agents which do not call `GetEnv()`
> in`AgentOnLoad/AgentOnAttach` or have no `AgentOnLoad/AgentOnAttach` entry
> points. Such agents can later call `GetEnv()` from a detached thread.
> Is it your concern?
I've pushed an update.
Please, let me know of you are okay with that.
-------------
PR: https://git.openjdk.org/jdk/pull/11204
