-
Type:
Bug
-
Resolution: Unresolved
-
Priority:
P4
-
Affects Version/s: None
-
Component/s: hotspot
Noticed this while looking at related code. JDK-8154580 started to store the klass mirror in interpreter frames, so that we do not lose the GC paths to the klasses:
```
// The method pointer in the frame might be the only path to the method's
// klass, and the klass needs to be kept alive while executing. The GCs
// don't trace through method pointers, so the mirror of the method's klass
// is installed as a GC root.
f->do_oop(interpreter_frame_mirror_addr());
```
So when we construct the interpreter frame, we effectively go and pull Klass::java_mirror()->resolve().
But if you look into *native* parts of Hotspot, you would notice this:
```
// Loading the java_mirror does not keep its holder alive. See Klass::keep_alive().
inline oop Klass::java_mirror() const {
return _java_mirror.resolve();
}
```
And Klass::keep_alive() resolves the CLD:
```
// This loads and keeps the klass's loader alive.
inline oop Klass::klass_holder() const {
return class_loader_data()->holder();
}
inline void Klass::keep_alive() const {
// Resolving the holder (a WeakHandle) will keep the klass alive until the next safepoint.
// Making the klass's CLD handle oops (e.g. the java_mirror), safe to store in the object
// graph and its roots (e.g. Handles).
static_cast<void>(klass_holder());
}
```
So how do we guarantee that we are keeping the klass alive when putting it into the interpreter frame? Shouldn't we instead resolve and save CLD, not Klass in the interpreter frames? E.g.:
```
diff --git a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
index 47ef0aef2bb..6618db301d8 100644
--- a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
+++ b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
@@ -578,7 +578,8 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
// Get mirror and store it in the frame as GC root for this Method*.
// rax is still ConstantPool*, resolve ConstantPool* -> InstanceKlass* -> Java mirror.
__ movptr(rdx, Address(rax, ConstantPool::pool_holder_offset()));
- __ movptr(rdx, Address(rdx, in_bytes(Klass::java_mirror_offset())));
+ __ movptr(rdx, Address(rdx, in_bytes(Klass::class_loader_data_offset())));
+ __ movptr(rdx, Address(rdx, in_bytes(ClassLoaderData::holder_offset())));
__ resolve_oop_handle(rdx, rscratch2);
__ push(rdx);
```
```
// The method pointer in the frame might be the only path to the method's
// klass, and the klass needs to be kept alive while executing. The GCs
// don't trace through method pointers, so the mirror of the method's klass
// is installed as a GC root.
f->do_oop(interpreter_frame_mirror_addr());
```
So when we construct the interpreter frame, we effectively go and pull Klass::java_mirror()->resolve().
But if you look into *native* parts of Hotspot, you would notice this:
```
// Loading the java_mirror does not keep its holder alive. See Klass::keep_alive().
inline oop Klass::java_mirror() const {
return _java_mirror.resolve();
}
```
And Klass::keep_alive() resolves the CLD:
```
// This loads and keeps the klass's loader alive.
inline oop Klass::klass_holder() const {
return class_loader_data()->holder();
}
inline void Klass::keep_alive() const {
// Resolving the holder (a WeakHandle) will keep the klass alive until the next safepoint.
// Making the klass's CLD handle oops (e.g. the java_mirror), safe to store in the object
// graph and its roots (e.g. Handles).
static_cast<void>(klass_holder());
}
```
So how do we guarantee that we are keeping the klass alive when putting it into the interpreter frame? Shouldn't we instead resolve and save CLD, not Klass in the interpreter frames? E.g.:
```
diff --git a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
index 47ef0aef2bb..6618db301d8 100644
--- a/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
+++ b/src/hotspot/cpu/x86/templateInterpreterGenerator_x86.cpp
@@ -578,7 +578,8 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
// Get mirror and store it in the frame as GC root for this Method*.
// rax is still ConstantPool*, resolve ConstantPool* -> InstanceKlass* -> Java mirror.
__ movptr(rdx, Address(rax, ConstantPool::pool_holder_offset()));
- __ movptr(rdx, Address(rdx, in_bytes(Klass::java_mirror_offset())));
+ __ movptr(rdx, Address(rdx, in_bytes(Klass::class_loader_data_offset())));
+ __ movptr(rdx, Address(rdx, in_bytes(ClassLoaderData::holder_offset())));
__ resolve_oop_handle(rdx, rscratch2);
__ push(rdx);
```
- relates to
-
JDK-8154580 Save mirror in interpreter frame to enable cleanups of CLDClosure
-
- Resolved
-