Skip to content

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

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 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
b0940de
Implement lookup of bootstrap methods with static arguments of type i…
Zeavee Sep 6, 2023
2cdc0b4
Rewrite doc for bootstrapArgumentIndexAt and add second argument to a…
Zeavee Sep 6, 2023
b33ffed
Add more details to the documentation and add a pseudo code example f…
Zeavee Sep 7, 2023
093d0ce
Add missing line of pseudo code and applied modification suggestion
Zeavee Sep 7, 2023
4ec0a7b
Add CachedBSMArgs to resolve arguments lazily instead of letting the …
Zeavee Sep 7, 2023
079969c
Fix BootstrapMethodInvocation.getStaticArguments documentation and ad…
Zeavee Sep 8, 2023
d6aa593
Use lookupConstant instead of lookupConstantInPool in CachedBSMArgs.get
Zeavee Sep 13, 2023
d75e092
Add undefined behavior documentation for bootstrapArgumentIndexAt and…
Zeavee Sep 13, 2023
f4377fc
Update getStaticArguments documentation example and update TestDynami…
Zeavee Sep 14, 2023
9e0627a
Add -XX:UseBootstrapCallInfo=3 to TestDynamicConstant
Zeavee Sep 19, 2023
ea4aa98
Add an extra run configuration for TestDynamicConstant instead of rep…
Zeavee Sep 19, 2023
7c27481
Update BootstrapMethodInvocation.getStaticArguments documentation
Zeavee Sep 20, 2023
7a8d9f2
Iterate over all static arguments in TestDynamicConstant
Zeavee 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: 3 additions & 3 deletions src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,9 +840,9 @@ 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 index))
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(index, 0);
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))
Expand Down Expand Up @@ -3141,7 +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 "I)I", FN_PTR(bootstrapArgumentIndexAt)},
{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)},
Expand Down
15 changes: 12 additions & 3 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 @@ -476,11 +476,20 @@ Object[] resolveBootstrapMethod(HotSpotConstantPool constantPool, int cpi) {

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

int bootstrapArgumentIndexAt(HotSpotConstantPool constantPool, int cpi) {
return bootstrapArgumentIndexAt(constantPool, constantPool.getConstantPoolPointer(), cpi);
/**
* Gets the constant pool index of a static argument of a bootstrap specifier. Used when the list
* of static arguments in the {@link BootstrapMethodInvocation} is an {@code int[]}. The list has
* two elements. The first one is the number of arguments and the second one is the {@code cpi}.
*
* @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);
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}),
Expand Down
4 changes: 2 additions & 2 deletions src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/hotspot/HotSpotConstantPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,8 +584,8 @@ private static String argumentAsString(JavaConstant arg) {
}

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

@Override
Expand Down
10 changes: 6 additions & 4 deletions src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ConstantPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,15 @@ interface BootstrapMethodInvocation {
}

/**
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 the pool entry associated with the
* index in the static arguments list of a bootstrap method.
* Gets the constant pool index of a static argument of a bootstrap specifier. Used when the list
* of static arguments in the {@link BootstrapMethodInvocation} is an {@code int[]}. The list has
* two elements. The first one is the number of arguments and the second one is the {@code cpi}.
Copy link
Member

Choose a reason for hiding this comment

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

Are you referring to BootstrapMethodInvocation.getStaticArguments() whose return type is List<JavaConstant>, not int[]?

The list has two elements.

That's not consistent with the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to BootstrapMethodInvocation.getStaticArguments() whose return type is List, not int[]?

It is indeed the list returned by BootstrapMethodInvocation.getStaticArguments(). In this case, we have in fact a List which corresponds to the int[], but it is true it is confusing, I will rephrase it.

That's not consistent with the code.

In this case, the arguments come from here, which are always only two elements.

Copy link
Member

Choose a reason for hiding this comment

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

In this case

In what case? When introducing new API like this, you need to use precise language that spells out possible values, assumptions, typical usages etc. What you have written is a good start but needs more work.

*
* @param index a constant pool index
* @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 index) {
default int bootstrapArgumentIndexAt(int cpi, int index) {
throw new UnsupportedOperationException();
}

Expand Down