C1 code installation and JNIHandle::deleted_handle() oop

[Roman Kennke]

Am 14.11.2017 um 19:38 schrieb Vladimir Ivanov:
>>> However, as Aleksey pointed out, we hit JNIHandles::resolve() in 
>>> product path, JVMCI or not, and this touches the naked oop by 
>>> comparing it with another oop. This doesn't sound like a reliable 
>>> thing to do.
>> Scratch that. Looking at the code paths again, this doesn't seem to 
>> be true. I.e. we hit JNIHandles::resolve() only in assert and 
>> ObjectLookup (I trust you that it's only JVMCI). Not sure if it can 
>> be robust to compare oops in assert paths. It sure is a race and 
>> doesn't feel very well.
>>
>> I wonder if we should introduce a 
>> CollectedHeap::is_in_or_null(jobject) method, and let the GC figure 
>> it out. It might actually have a way to check it (simple address 
>> range check) without sending the thread to VM state.
>
> Yes, the assert just sanity checks that the handlized oop points into 
> the heap.
>
> Depending on the complexity of the check, I'd consider the following 
> options:
>   * dedicated CollectedHeap::is_in_or_null(jobject)
This would require some sort of JNIHandles::resolve_raw() or similar, 
that bypasses the checks. I don't really like it.
>   * ThreadInVMfromNative under #ifdef ASSERT (or factored out)
Different safepointing behaviour in release vs. debug build? Don't know...
>   * completely remove the assert

Maybe.

The root of the evil is still that we're accessing the naked oop while 
potentially at a safepoint (and oops moving around). You already 
established that JVMCI doesn't have the problem because it's already 
transitioned into VM. Why not make the other compilers consistent? I.e. 
transition into VM (VM_ENTRY_MARK) before going into the same code path. 
>From the looks of it, code paths start to be shared between JVMCI and 
others starting around 
DebugInformationRecorder::create_scope_values(GrowableArray<ScopeValue*>*), 
i.e. the VM_ENTRY_MARK could be placed around that in, e.g.,

void CodeEmitInfo::record_debug_info(DebugInformationRecorder* recorder, 
int pc_offset) {
   // record the safepoint before recording the debug info for enclosing 
scopes
   VM_ENTRY_MARK;
   recorder->add_safepoint(pc_offset, _oop_map->deep_copy());
   _scope_debug_info->record_debug_info(recorder, pc_offset, 
true/*topmost*/, _is_method_handle_invoke);
   recorder->end_safepoint(pc_offset);
}

Although similarities between this code and related code in JVMCI would 
suggest to place the VM_ENTRY_MARK even higher in the call stack.

WDYT?

Roman