Skip to content
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.

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

8315771: [JVMCI] Resolution of bootstrap methods with int[] static arguments #15588

Closed
wants to merge 13 commits into from
Closed
Changes from 3 commits
Commits
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
14 changes: 11 additions & 3 deletions src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
Original file line number Diff line number Diff line change
@@ -770,9 +770,11 @@ C2V_VMENTRY_NULL(jobjectArray, resolveBootstrapMethod, (JNIEnv* env, jobject, AR
}
// Get the indy entry based on CP index
int indy_index = -1;
for (int i = 0; i < cp->resolved_indy_entries_length(); i++) {
if (cp->resolved_indy_entry_at(i)->constant_pool_index() == index) {
indy_index = i;
if (is_indy) {
for (int i = 0; i < cp->resolved_indy_entries_length(); i++) {
if (cp->resolved_indy_entry_at(i)->constant_pool_index() == index) {
indy_index = i;
}
}
}
// Resolve the bootstrap specifier, its name, type, and static arguments
@@ -838,6 +840,11 @@ C2V_VMENTRY_NULL(jobjectArray, resolveBootstrapMethod, (JNIEnv* env, jobject, AR
return JVMCIENV->get_jobjectArray(bsmi);
C2V_END

C2V_VMENTRY_0(jint, bootstrapArgumentIndexAt, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint cpi, jint index))
constantPoolHandle cp(THREAD, UNPACK_PAIR(ConstantPool, cp));
return cp->bootstrap_argument_index_at(cpi, index);
C2V_END

C2V_VMENTRY_0(jint, lookupNameAndTypeRefIndexInPool, (JNIEnv* env, jobject, ARGUMENT_PAIR(cp), jint index, jint opcode))
constantPoolHandle cp(THREAD, UNPACK_PAIR(ConstantPool, cp));
return cp->name_and_type_ref_index_at(index, (Bytecodes::Code)opcode);
@@ -3134,6 +3141,7 @@ JNINativeMethod CompilerToVM::methods[] = {
{CC "lookupConstantInPool", CC "(" HS_CONSTANT_POOL2 "IZ)" JAVACONSTANT, FN_PTR(lookupConstantInPool)},
{CC "constantPoolRemapInstructionOperandFromCache", CC "(" HS_CONSTANT_POOL2 "I)I", FN_PTR(constantPoolRemapInstructionOperandFromCache)},
{CC "resolveBootstrapMethod", CC "(" HS_CONSTANT_POOL2 "I)[" OBJECT, FN_PTR(resolveBootstrapMethod)},
{CC "bootstrapArgumentIndexAt", CC "(" HS_CONSTANT_POOL2 "II)I", FN_PTR(bootstrapArgumentIndexAt)},
{CC "getUncachedStringInPool", CC "(" HS_CONSTANT_POOL2 "I)" JAVACONSTANT, FN_PTR(getUncachedStringInPool)},
{CC "resolveTypeInPool", CC "(" HS_CONSTANT_POOL2 "I)" HS_KLASS, FN_PTR(resolveTypeInPool)},
{CC "resolveFieldInPool", CC "(" HS_CONSTANT_POOL2 "I" HS_METHOD2 "B[I)" HS_KLASS, FN_PTR(resolveFieldInPool)},
Original file line number Diff line number Diff line change
@@ -476,6 +476,24 @@ Object[] resolveBootstrapMethod(HotSpotConstantPool constantPool, int cpi) {

private native Object[] resolveBootstrapMethod(HotSpotConstantPool constantPool, long constantPoolPointer, int cpi);

/**
* Gets the constant pool index of a static argument of a bootstrap specifier (a constant pool entry
* containing information about a bootstrap method). Used when the list of static arguments in the
* {@link BootstrapMethodInvocation} is a {@code List<PrimitiveConstant>} of the form
* {{@code arg_count}, {@code pool_index}}, meaning the arguments are not already resolved and that
* the JDK has to lookup the arguments when they are needed. The {@code cpi} corresponds to
* {@code pool_index} and the {@code index} has to be smaller than {@code arg_count}.
*
* @param cpi the index of a bootstrap specifier in the constant pool
Copy link
Member

Choose a reason for hiding this comment

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

     * @param cpi the index of a {@code CONSTANT_Dynamic_info} or @{code CONSTANT_InvokeDynamic_info} entry

* @param index the index of the static argument in the list of static arguments
Copy link
Member

Choose a reason for hiding this comment

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

What happens when index is out of bounds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, it returns another constant pool index or a random number. I can add a check in the CachedBSMArgs.get method to prevent this.

Copy link
Member

Choose a reason for hiding this comment

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

Yes please. We should opt for exceptions over undefined behavior whenever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After double checking, there is in fact already an assert for checking if the index is out of bounds. Should I change it to an exception?

Copy link
Member

Choose a reason for hiding this comment

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

Is the assertion in code that can throw exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The assertion is here. I don't think we can throw exception there. I could check the size in jvmciCompilerToVM.bootstrapArgumentIndexAt, but it would require a bit of code duplication to obtain it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. You should just add a note in the javadoc that the result is undefined if index is out of bounds. This is all internal API so I assume we can guarantee the value is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is only used in CachedBSMArgs.get, which will throw an ArrayIndexOutOfBoundsException before we arrive there.

* @return the constant pool index associated with the static argument
*/
int bootstrapArgumentIndexAt(HotSpotConstantPool constantPool, int cpi, int index) {
return bootstrapArgumentIndexAt(constantPool, constantPool.getConstantPoolPointer(), cpi, index);
}

private native int bootstrapArgumentIndexAt(HotSpotConstantPool constantPool, long constantPoolPointer, int cpi, int index);

/**
* If {@code cpi} denotes an entry representing a signature polymorphic method ({@jvms 2.9}),
* this method ensures that the type referenced by the entry is loaded and initialized. It
Original file line number Diff line number Diff line change
@@ -583,6 +583,11 @@ private static String argumentAsString(JavaConstant arg) {
}
}

@Override
public int bootstrapArgumentIndexAt(int cpi, int index) {
return compilerToVM().bootstrapArgumentIndexAt(this, cpi, index);
}

@Override
public BootstrapMethodInvocation lookupBootstrapMethodInvocation(int index, int opcode) {
int cpi = opcode == -1 ? index : indyIndexConstantPoolIndex(index, opcode);
@@ -604,8 +609,7 @@ public BootstrapMethodInvocation lookupBootstrapMethodInvocation(int index, int
staticArgumentsList = List.of((JavaConstant[]) staticArguments);
} else {
int[] bsciArgs = (int[]) staticArguments;
String message = String.format("Resolving bootstrap static arguments for %s using BootstrapCallInfo %s not supported", method.format("%H.%n(%p)"), Arrays.toString(bsciArgs));
throw new IllegalArgumentException(message);
staticArgumentsList = List.of(Arrays.stream(bsciArgs).mapToObj(i -> JavaConstant.forInt(i)).toArray(JavaConstant[]::new));
}
return new BootstrapMethodInvocationImpl(tag.name.equals("InvokeDynamic"), method, name, type, staticArgumentsList);
default:
Original file line number Diff line number Diff line change
@@ -131,7 +131,24 @@ default JavaMethod lookupMethod(int cpi, int opcode) {

/**
* The details for invoking a bootstrap method associated with a {@code CONSTANT_Dynamic_info}
* or {@code CONSTANT_InvokeDynamic_info} pool entry .
* or {@code CONSTANT_InvokeDynamic_info} pool entry.
*
* The procedure to obtain and use a {@link BootstrapMethodInvocation} is the following:
*
* <pre>
* bsmInvocation = lookupBootstrapMethodInvocation(index, opcode);
* staticArguments = bsmInvocation.getStaticArguments();
* if staticArguments are PrimitiveConstant {
* argCount = staticArguments.get(0).asInt();
* cpi = staticArguments.get(1).asInt();
* for (int i = 0; i < argCount; ++i) {
* arguments[i] = lookupConstant(cpi, i);
Copy link
Member

Choose a reason for hiding this comment

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

What lookupConstant method is this?
Where is the usage of the new bootstrapArgumentIndexAt method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a mistake sorry, it is now correct.
The idea is to use bootstrapArgumentIndexAt to obtain the constant pool index of the static argument and use it to call ConstantPool.lookupConstant(int cpi, boolean resolve) to get the constant.

* }
* call bootstrap method with newly resolved arguments
* } else {
* call bootstrap method with provided arguments
* }
* </pre>
*
* @jvms 4.4.10 The {@code CONSTANT_Dynamic_info} and {@code CONSTANT_InvokeDynamic_info}
* Structures
@@ -170,6 +187,22 @@ interface BootstrapMethodInvocation {
List<JavaConstant> getStaticArguments();
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

This is very confusing documentation. It's not clear what index or entry you are referring to each time you say "index". Please provide a concrete example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have rewritten the documentation, I hope it is clearer. I also added a second argument to be able to access different different static arguments on the same bootstrap specifier.

Copy link
Member

Choose a reason for hiding this comment

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

It still needs an example showing how this API should be used. Please add a complete (pseudo-code) example to the javadoc of BootstrapMethodInvocation.

* Gets the constant pool index of a static argument of a bootstrap specifier (a constant pool entry
* containing information about a bootstrap method). Used when the list of static arguments in the
* {@link BootstrapMethodInvocation} is a {@code List<PrimitiveConstant>} of the form
* {{@code arg_count}, {@code pool_index}}, meaning the arguments are not already resolved and that
* the JDK has to lookup the arguments when they are needed. The {@code cpi} corresponds to
* {@code pool_index} and the {@code index} has to be smaller than {@code arg_count}.
*
* @param cpi the index of a bootstrap specifier in the constant pool
Copy link
Member

Choose a reason for hiding this comment

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

the index of a bootstrap specifier in the constant pool

What's a bootstrap specifier? That is not mentioned anywhere else so you either have to explain it or use existing terminology.

* @param index the index of the static argument in the list of static arguments
* @return the constant pool index associated with the static argument
*/
default int bootstrapArgumentIndexAt(int cpi, int index) {
throw new UnsupportedOperationException();
}

/**
* Gets the details for invoking a bootstrap method associated with the
* {@code CONSTANT_Dynamic_info} or {@code CONSTANT_InvokeDynamic_info} pool entry
@@ -182,8 +215,6 @@ interface BootstrapMethodInvocation {
* {@code index} was not decoded from a bytecode stream
* @return the bootstrap method invocation details or {@code null} if the entry specified by {@code index}
* is not a {@code CONSTANT_Dynamic_info} or @{code CONSTANT_InvokeDynamic_info}
* @throws IllegalArgumentException if the bootstrap method invocation makes use of
* {@code java.lang.invoke.BootstrapCallInfo}
* @jvms 4.7.23 The {@code BootstrapMethods} Attribute
*/
default BootstrapMethodInvocation lookupBootstrapMethodInvocation(int index, int opcode) {