Skip to content

8315954: getArgumentValues002.java fails on Graal #15705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

8315954: getArgumentValues002.java fails on Graal #15705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
23b94e3
added ResolvedJavaMethod.getLiveObjectLocalsAt
dougxc Sep 13, 2023
b35ba22
simplified getLiveObjectLocalsAt and moved it from ResolvedJavaMethod…
dougxc Sep 13, 2023
0f01fe3
need to zero oop_map_buf
dougxc Sep 14, 2023
c6c6c0d
generalized getLiveObjectLocalsAt to getOopMapAt
dougxc Sep 15, 2023
3c903ec
add assertion to InterpreterOopMap::resource_copy
dougxc Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/hotspot/share/interpreter/oopMapCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@ void OopMapCacheEntry::flush() {
void InterpreterOopMap::resource_copy(OopMapCacheEntry* from) {
assert(_resource_allocate_bit_mask,
"Should not resource allocate the _bit_mask");
assert(from->has_valid_mask(),
"Cannot copy entry with an invalid mask");

set_method(from->method());
set_bci(from->bci());
Expand Down Expand Up @@ -612,7 +614,9 @@ void OopMapCache::compute_one_oop_map(const methodHandle& method, int bci, Inter
OopMapCacheEntry* tmp = NEW_C_HEAP_OBJ(OopMapCacheEntry, mtClass);
tmp->initialize();
tmp->fill(method, bci);
entry->resource_copy(tmp);
if (tmp->has_valid_mask()) {
entry->resource_copy(tmp);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If tmp is invalid (e.g. oop map was requested for invalid BCI), then resource_copy crashes the VM in strange ways since it blindly trusts the mask size to be valid.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the only place where resource_copy() is called, could you add an assert in resource_copy() itself to check that it is never called with an invalid bci/mask_size.
Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've added that assertion.

}
tmp->flush();
FREE_C_HEAP_OBJ(tmp);
}
4 changes: 3 additions & 1 deletion src/hotspot/share/interpreter/oopMapCache.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class InterpreterOopMap: ResourceObj {
private:
Method* _method; // the method for which the mask is valid
unsigned short _bci; // the bci for which the mask is valid
int _mask_size; // the mask size in bits
int _mask_size; // the mask size in bits (USHRT_MAX if invalid)
int _expression_stack_size; // the size of the expression stack in slots

protected:
Expand Down Expand Up @@ -146,6 +146,8 @@ class InterpreterOopMap: ResourceObj {

int expression_stack_size() const { return _expression_stack_size; }

// Determines if a valid mask has been computed
bool has_valid_mask() const { return _mask_size != USHRT_MAX; }
};

class OopMapCache : public CHeapObj<mtClass> {
Expand Down
84 changes: 64 additions & 20 deletions src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "compiler/oopMap.hpp"
#include "interpreter/bytecodeStream.hpp"
#include "interpreter/linkResolver.hpp"
#include "interpreter/oopMapCache.hpp"
#include "jfr/jfrEvents.hpp"
#include "jvmci/jvmciCodeInstaller.hpp"
#include "jvmci/jvmciCompilerToVM.hpp"
Expand Down Expand Up @@ -2512,7 +2513,7 @@ C2V_VMENTRY_NULL(jlongArray, registerNativeMethods, (JNIEnv* env, jobject, jclas
jlongArray info = (jlongArray) JNIHandles::make_local(THREAD, info_oop);
runtime->init_JavaVM_info(info, JVMCI_CHECK_0);
return info;
}
C2V_END

C2V_VMENTRY_PREFIX(jboolean, isCurrentThreadAttached, (JNIEnv* env, jobject c2vm))
if (thread == nullptr || thread->libjvmci_runtime() == nullptr) {
Expand Down Expand Up @@ -2777,7 +2778,7 @@ C2V_VMENTRY_0(jlong, translate, (JNIEnv* env, jobject, jobject obj_handle, jbool
return 0L;
}
return (jlong) PEER_JVMCIENV->make_global(result).as_jobject();
}
C2V_END

C2V_VMENTRY_NULL(jobject, unhand, (JNIEnv* env, jobject, jlong obj_handle))
requireJVMCINativeLibrary(JVMCI_CHECK_NULL);
Expand All @@ -2790,13 +2791,13 @@ C2V_VMENTRY_NULL(jobject, unhand, (JNIEnv* env, jobject, jlong obj_handle))

JVMCIENV->destroy_global(global_handle_obj);
return result;
}
C2V_END

C2V_VMENTRY(void, updateHotSpotNmethod, (JNIEnv* env, jobject, jobject code_handle))
JVMCIObject code = JVMCIENV->wrap(code_handle);
// Execute this operation for the side effect of updating the InstalledCode state
JVMCIENV->get_nmethod(code);
}
C2V_END

C2V_VMENTRY_NULL(jbyteArray, getCode, (JNIEnv* env, jobject, jobject code_handle))
JVMCIObject code = JVMCIENV->wrap(code_handle);
Expand All @@ -2812,7 +2813,7 @@ C2V_VMENTRY_NULL(jbyteArray, getCode, (JNIEnv* env, jobject, jobject code_handle
JVMCIPrimitiveArray result = JVMCIENV->new_byteArray(code_size, JVMCI_CHECK_NULL);
JVMCIENV->copy_bytes_from(code_bytes, result, 0, code_size);
return JVMCIENV->get_jbyteArray(result);
}
C2V_END

C2V_VMENTRY_NULL(jobject, asReflectionExecutable, (JNIEnv* env, jobject, ARGUMENT_PAIR(method)))
requireInHotSpot("asReflectionExecutable", JVMCI_CHECK_NULL);
Expand All @@ -2828,7 +2829,7 @@ C2V_VMENTRY_NULL(jobject, asReflectionExecutable, (JNIEnv* env, jobject, ARGUMEN
executable = Reflection::new_method(m, false, CHECK_NULL);
}
return JNIHandles::make_local(THREAD, executable);
}
C2V_END

static InstanceKlass* check_field(Klass* klass, jint index, JVMCI_TRAPS) {
if (!klass->is_instance_klass()) {
Expand All @@ -2850,7 +2851,7 @@ C2V_VMENTRY_NULL(jobject, asReflectionField, (JNIEnv* env, jobject, ARGUMENT_PAI
fieldDescriptor fd(iklass, index);
oop reflected = Reflection::new_field(&fd, CHECK_NULL);
return JNIHandles::make_local(THREAD, reflected);
}
C2V_END

static jbyteArray get_encoded_annotation_data(InstanceKlass* holder, AnnotationArray* annotations_array, bool for_class,
jint filter_length, jlong filter_klass_pointers,
Expand Down Expand Up @@ -2964,7 +2965,7 @@ C2V_VMENTRY_NULL(jobjectArray, getFailedSpeculations, (JNIEnv* env, jobject, jlo
JVMCIENV->put_object_at(result, result_index++, entry);
}
return JVMCIENV->get_jobjectArray(result);
}
C2V_END

C2V_VMENTRY_0(jlong, getFailedSpeculationsAddress, (JNIEnv* env, jobject, ARGUMENT_PAIR(method)))
methodHandle method(THREAD, UNPACK_PAIR(Method, method));
Expand All @@ -2975,19 +2976,19 @@ C2V_VMENTRY_0(jlong, getFailedSpeculationsAddress, (JNIEnv* env, jobject, ARGUME
method->set_method_data(method_data);
}
return (jlong) method_data->get_failed_speculations_address();
}
C2V_END

C2V_VMENTRY(void, releaseFailedSpeculations, (JNIEnv* env, jobject, jlong failed_speculations_address))
FailedSpeculation::free_failed_speculations((FailedSpeculation**)(address) failed_speculations_address);
}
C2V_END

C2V_VMENTRY_0(jboolean, addFailedSpeculation, (JNIEnv* env, jobject, jlong failed_speculations_address, jbyteArray speculation_obj))
JVMCIPrimitiveArray speculation_handle = JVMCIENV->wrap(speculation_obj);
int speculation_len = JVMCIENV->get_length(speculation_handle);
char* speculation = NEW_RESOURCE_ARRAY(char, speculation_len);
JVMCIENV->copy_bytes_to(speculation_handle, (jbyte*) speculation, 0, speculation_len);
return FailedSpeculation::add_failed_speculation(nullptr, (FailedSpeculation**)(address) failed_speculations_address, (address) speculation, speculation_len);
}
C2V_END

C2V_VMENTRY(void, callSystemExit, (JNIEnv* env, jobject, jint status))
JavaValue result(T_VOID);
Expand All @@ -2999,11 +3000,11 @@ C2V_VMENTRY(void, callSystemExit, (JNIEnv* env, jobject, jint status))
vmSymbols::int_void_signature(),
&jargs,
CHECK);
}
C2V_END

C2V_VMENTRY_0(jlong, ticksNow, (JNIEnv* env, jobject))
return CompilerEvent::ticksNow();
}
C2V_END

C2V_VMENTRY_0(jint, registerCompilerPhase, (JNIEnv* env, jobject, jstring jphase_name))
#if INCLUDE_JFR
Expand All @@ -3013,14 +3014,14 @@ C2V_VMENTRY_0(jint, registerCompilerPhase, (JNIEnv* env, jobject, jstring jphase
#else
return -1;
#endif // !INCLUDE_JFR
}
C2V_END

C2V_VMENTRY(void, notifyCompilerPhaseEvent, (JNIEnv* env, jobject, jlong startTime, jint phase, jint compileId, jint level))
EventCompilerPhase event;
if (event.should_commit()) {
CompilerEvent::PhaseEvent::post(event, startTime, phase, compileId, level);
}
}
C2V_END

C2V_VMENTRY(void, notifyCompilerInliningEvent, (JNIEnv* env, jobject, jint compileId, ARGUMENT_PAIR(caller), ARGUMENT_PAIR(callee), jboolean succeeded, jstring jmessage, jint bci))
EventCompilerInlining event;
Expand All @@ -3030,7 +3031,7 @@ C2V_VMENTRY(void, notifyCompilerInliningEvent, (JNIEnv* env, jobject, jint compi
JVMCIObject message = JVMCIENV->wrap(jmessage);
CompilerEvent::InlineEvent::post(event, compileId, caller, callee, succeeded, JVMCIENV->as_utf8_string(message), bci);
}
}
C2V_END

C2V_VMENTRY(void, setThreadLocalObject, (JNIEnv* env, jobject, jint id, jobject value))
requireInHotSpot("setThreadLocalObject", JVMCI_CHECK);
Expand All @@ -3040,7 +3041,7 @@ C2V_VMENTRY(void, setThreadLocalObject, (JNIEnv* env, jobject, jint id, jobject
}
THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
err_msg("%d is not a valid thread local id", id));
}
C2V_END

C2V_VMENTRY_NULL(jobject, getThreadLocalObject, (JNIEnv* env, jobject, jint id))
requireInHotSpot("getThreadLocalObject", JVMCI_CHECK_NULL);
Expand All @@ -3049,7 +3050,7 @@ C2V_VMENTRY_NULL(jobject, getThreadLocalObject, (JNIEnv* env, jobject, jint id))
}
THROW_MSG_0(vmSymbols::java_lang_IllegalArgumentException(),
err_msg("%d is not a valid thread local id", id));
}
C2V_END

C2V_VMENTRY(void, setThreadLocalLong, (JNIEnv* env, jobject, jint id, jlong value))
requireInHotSpot("setThreadLocalLong", JVMCI_CHECK);
Expand All @@ -3061,7 +3062,7 @@ C2V_VMENTRY(void, setThreadLocalLong, (JNIEnv* env, jobject, jint id, jlong valu
THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
err_msg("%d is not a valid thread local id", id));
}
}
C2V_END

C2V_VMENTRY_0(jlong, getThreadLocalLong, (JNIEnv* env, jobject, jint id))
requireInHotSpot("getThreadLocalLong", JVMCI_CHECK_0);
Expand All @@ -3073,7 +3074,49 @@ C2V_VMENTRY_0(jlong, getThreadLocalLong, (JNIEnv* env, jobject, jint id))
THROW_MSG_0(vmSymbols::java_lang_IllegalArgumentException(),
err_msg("%d is not a valid thread local id", id));
}
}
C2V_END

C2V_VMENTRY(void, getOopMapAt, (JNIEnv* env, jobject, ARGUMENT_PAIR(method),
jint bci, jlongArray oop_map_handle))
methodHandle method(THREAD, UNPACK_PAIR(Method, method));
if (bci < 0 || bci >= method->code_size()) {
JVMCI_THROW_MSG(IllegalArgumentException,
err_msg("bci %d is out of bounds [0 .. %d)", bci, method->code_size()));
}
InterpreterOopMap mask;
OopMapCache::compute_one_oop_map(method, bci, &mask);
if (!mask.has_valid_mask()) {
JVMCI_THROW_MSG(IllegalArgumentException, err_msg("bci %d is not valid", bci));
}
if (mask.number_of_entries() == 0) {
return;
}

int nslots = method->max_locals() + method->max_stack();
int nwords = ((nslots - 1) / 64) + 1;
JVMCIPrimitiveArray oop_map = JVMCIENV->wrap(oop_map_handle);
int oop_map_len = JVMCIENV->get_length(oop_map);
if (nwords > oop_map_len) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we sanity check against mask.number_of_entries()? One wrinkle here is that compute_one_oop_map also computes information about the stack so the mask it computes can be larger than just max_locals. For the purposes of OSR this doesn't matter as none of the JITs support OSR with a non-empty stack, so we would never call it for a bci with a non-empty stack. So should we disallow calling it with a non-empty stack or just properly handle it by passing in an array long enough to contain max_locals + max_stack?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only look up the mask for locals and so ignore stack indexes in the mask altogether. I'm assuming therefore that mask.is_oop(i) can never hit any problems.
Note that this API should be safe when called for any valid BCI, not just those for an OSR entry point. Even if called for a BCI with a non-empty stack, the current implementation simply ignores that part of the mask.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's implied by the name of the method. It would make me happy if there was a comment pointing out that we're explicitly ignoring whether the stack is non-empty and contains oops.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, I generalized getLiveObjectLocalsAt to getOopMapAt since the VM computation is for both locals and operand stack anyway.
When called for OSR entry points, the result will be the same since (currently) HotSpot requires the stack to be empty.

JVMCI_THROW_MSG(IllegalArgumentException,
err_msg("oop map too short: %d > %d", nwords, oop_map_len));
}

jlong* oop_map_buf = NEW_RESOURCE_ARRAY_IN_THREAD_RETURN_NULL(THREAD, jlong, nwords);
if (oop_map_buf == nullptr) {
JVMCI_THROW_MSG(InternalError, err_msg("could not allocate %d longs", nwords));
}
for (int i = 0; i < nwords; i++) {
oop_map_buf[i] = 0L;
}

BitMapView oop_map_view = BitMapView((BitMap::bm_word_t*) oop_map_buf, nwords * BitsPerLong);
for (int i = 0; i < nslots; i++) {
if (mask.is_oop(i)) {
oop_map_view.set_bit(i);
}
}
JVMCIENV->copy_longs_from((jlong*)oop_map_buf, oop_map, 0, nwords);
C2V_END

#define CC (char*) /*cast a literal from (const char*)*/
#define FN_PTR(f) CAST_FROM_FN_PTR(void*, &(c2v_ ## f))
Expand Down Expand Up @@ -3232,6 +3275,7 @@ JNINativeMethod CompilerToVM::methods[] = {
{CC "registerCompilerPhase", CC "(" STRING ")I", FN_PTR(registerCompilerPhase)},
{CC "notifyCompilerPhaseEvent", CC "(JIII)V", FN_PTR(notifyCompilerPhaseEvent)},
{CC "notifyCompilerInliningEvent", CC "(I" HS_METHOD2 HS_METHOD2 "ZLjava/lang/String;I)V", FN_PTR(notifyCompilerInliningEvent)},
{CC "getOopMapAt", CC "(" HS_METHOD2 "I[J)V", FN_PTR(getOopMapAt)},
};

int CompilerToVM::methods_count() {
Expand Down
9 changes: 9 additions & 0 deletions src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/CompilerToVM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1473,4 +1473,13 @@ public void close() {
}
}
}

/**
* @see HotSpotResolvedJavaMethod#getOopMapAt
*/
void getOopMapAt(HotSpotResolvedJavaMethodImpl method, int bci, long[] oopMap) {
getOopMapAt(method, method.getMethodPointer(), bci, oopMap);
}

native void getOopMapAt(HotSpotResolvedJavaMethodImpl method, long methodPointer, int bci, long[] oopMap);
}
15 changes: 15 additions & 0 deletions src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
package jdk.vm.ci.hotspot;

import java.lang.reflect.Modifier;
import java.util.BitSet;

import jdk.vm.ci.meta.JavaMethod;
import jdk.vm.ci.meta.ResolvedJavaMethod;
Expand Down Expand Up @@ -127,4 +128,18 @@ default boolean isDefault() {
boolean hasCodeAtLevel(int entryBCI, int level);

int methodIdnum();


/**
* Computes which local variables and operand stack slots in {@code method} contain
* live object values at the instruction denoted by {@code bci}. This is the "oop map"
* used by the garbage collector for interpreter frames.
*
* @param bci the index of an instruction in this method's bytecodes
* @return the computed oop map. The first {@link #getMaxLocals} bits are for
* the local variables, the remaining bits are for the stack slots.
* @throws IllegalArgumentException if this method has no bytecode or
* {@code bci} is not the index of a bytecode instruction
*/
BitSet getOopMapAt(int bci);
}
12 changes: 12 additions & 0 deletions src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotResolvedJavaMethodImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import java.lang.reflect.Executable;
import java.lang.reflect.Modifier;
import java.lang.reflect.Type;
import java.util.BitSet;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
Expand Down Expand Up @@ -767,4 +768,15 @@ private List<AnnotationData> getAnnotationData0(ResolvedJavaType... filter) {
byte[] encoded = compilerToVM().getEncodedExecutableAnnotationData(this, filter);
return VMSupport.decodeAnnotations(encoded, AnnotationDataDecoder.INSTANCE);
}

@Override
public BitSet getOopMapAt(int bci) {
if (getCodeSize() == 0) {
throw new IllegalArgumentException("has no bytecode");
}
int nwords = ((getMaxLocals() + getMaxStackSize() - 1) / 64) + 1;
long[] oopMap = new long[nwords];
compilerToVM().getOopMapAt(this, bci, oopMap);
return BitSet.valueOf(oopMap);
}
}
1 change: 1 addition & 0 deletions src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaMethod.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.Type;
import java.util.BitSet;

/**
* Represents a resolved Java method. Methods, like fields and types, are resolved through
Expand Down
Loading