RFR: 8299414: JVMTI FollowReferences should support references from VirtualThread stack [v9]

[Alex Menkov]

On Tue, 2 May 2023 10:10:32 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Alex Menkov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Added "no continuations" test case
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 2245:
> 
>> 2243:   bool is_top_frame;
>> 2244:   int depth;
>> 2245:   frame* last_entry_frame;
> 
> The field names of a helper class are usually started with '_' symbol.

renamed all fields

> src/hotspot/share/prims/jvmtiTagMap.cpp line 2319:
> 
>> 2317:           }
>> 2318:         }
>> 2319:       }
> 
> The fragments 2289-2303 and 2305-2319 are based on the `StackValueCollection` and look very similar.
> It can be worth to refactor these fragments into two function calls:
> 
>    bool report_stack_value_collection(jmethodID method, int idx_base,
>              StackValueCollection* elems, jlocation bci) {  
>      for (int index = 0; index < exprs->size(); index++) {
>         if (exprs->at(index)->type() == T_OBJECT) {
>           oop obj = elems->obj_at(index)();
>           if (obj == nullptr) {
>             continue;
>           }
>           // stack reference
>           if (!CallbackInvoker::report_stack_ref_root(thread_tag, tid, depth, method,
>                                                       bci, idx_base + index, obj)) {
>             return false;
>           }
>         }
>       }
>       return true; // ???
> 
>       . . . . .
>       jlocation bci = (jlocation)jvf->bci();
>       StackValueCollection* locals = jvf->locals();
>       if (!report_stack_value_collection(method, locals, 0 /* idx_base*/, bci)) {
>         return false;
>       }      
>       StackValueCollection* exprs = jvf->expressions();
>       if (!report_stack_value_collection(method, exprs, locals->size(), bci)) {
>         return false;
>       }
> 
> Other complete fragments can be also implemented as separate functions:
>   2321-2328 (?), 2330-2351

refactored.

> src/hotspot/share/prims/jvmtiTagMap.cpp line 2796:
> 
>> 2794:   if (!java_thread->has_last_Java_frame()) {
>> 2795:     // this may be only platform thread
>> 2796:     assert(mounted_vt == nullptr, "must be");
> 
> I'm not sure this assert is right.
> I think, a virtual thread may have an empty stack observable from a VM_op,
> for instance when it is in a process of being terminated.
> Though, it is not that easy to make this assert fired with a test case and prove this can happen.
> Another danger is that a virtual thread can be observed from a VM_op as in a VTMS (mount/unmount) transition. I need to think a little bit about possible consequences. Is it better to treat current thread identity as of a carrier thread in such a case?

removed the assert for safety.
I have no idea how vthread stack (frames on carrier thread and stack chunks) can look like during VTMS transitions (and it's very hard to reproduce the case by test)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1184336378
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1184337458
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1184335758