Abort compilations for exceptions during invokedynamic resolution

[nbhuiyan]

Invokedynamic call resolution can either result in a valid entry
in the CallSite table if successful, or an exception object
otherwise. The call site table entry is a two element array
containing the MemberName and appendix objects necessary for
constructing a resolved invokedynamic call. The exception object
would not be an array type object, and therefore it is sufficient
to check if the entry obtained from the CallSite table is an array
instance to determine if the resolution was successful.

This changeset handles the cases when we have an invalid entry
in the CallSite table that results in either failed inlining or
failed compilation, depending on whether the invokedynamic is part
of the inlining candidate or the root method, respectively.

Issue: #16965

[nbhuiyan]

@0xdaryl Requesting review

[nbhuiyan]

I believe the new VM API being added will need a JITServer override. I will try to implement that as well.

[0xdaryl]

First "is" -> "if"

[0xdaryl]

You can just say ; false otherwise for brevity and delete the next line.

[0xdaryl]

You may have chosen this name because it matches the helper you end up calling, but since this is an inquiry API in the FrontEnd would it be better to call it isArrayObject() instead?

[0xdaryl]

Just confirming.... Is this issue only with the JIT's (mis)understanding of invokedynamic and the fix applies to both the OpenJ9 MH implementation as well as the OpenJDK LambdaForm MH implementations?

[nbhuiyan]

@0xdaryl

Is this issue only with the JIT's (mis)understanding of invokedynamic and the fix applies to both the OpenJ9 MH implementation as well as the OpenJDK LambdaForm MH implementations?

No, this fix only applies to the OpenJDK MH implementation. The older OpenJ9 MH implementation has a different mechanism for invokedynamic resolution, and does not place an exception object in the CallSite table entry, and the valid table entries themselves are not arrays.

[nbhuiyan]

@0xdaryl I have addressed your initial review comments. There are some JITServer test failures due to the missing JITServer implementation. I am currently working on that.

[0xdaryl]

No, this fix only applies to the OpenJDK MH implementation. The older OpenJ9 MH implementation has a different mechanism for invokedynamic resolution, and does not place an exception object in the CallSite table entry, and the valid table entries themselves are not arrays.

OK, I just expanded the code around the fix and saw the #ifdefs guarding it, which is what I was going to ask you to add. So it is all good.

Tag me when you think this PR is ready for another review.

[nbhuiyan]

The JITServer test failures I observed before in my personal build now appear to be passing. @0xdaryl Ready for review.

@mpirvu I'd appreciate your review of the JITServer changes

[0xdaryl]

Jenkins test sanity,sanity.openjdk all jdk17

[0xdaryl]

Jenkins test sanity,sanity.openjdk all jdk21

[mpirvu]

LGTM
Since a new message has been added, could you please increment the MINOR_NUMBER here
https://github.com/eclipse-openj9/openj9/blob/master/runtime/compiler/net/CommunicationStream.hpp#L121

Also, I was wondering why the parameter for isArrayObject(TR_OpaqueClassBlock *object); is declared to be as a pointer to a class, when in fact it's an object.

[nbhuiyan]

@mpirvu I have updated the MINOR_NUMBER

I was wondering why the parameter for isArrayObject(TR_OpaqueClassBlock *object); is declared to be as a pointer to a class, when in fact it's an object

Thanks for pointing that out, I should not be using TR_OpaqueClassBlock* type for an object. Fixed that as well.

[mpirvu]

I am having second thoughts about the implementation. The code seems to be peeking inside objects without making sure it has VM access. Without VM access in hand, the GC can move the objects while the compiler attempts to inspect them. I made some suggestions on how the code needs to be changed.
Moreover, the JITServer should never receive any object references. Thus, the new frontend method should just fatal assert if executed at the server rather than sending a message at the client.

[mpirvu]

I am having second thoughts about this one. if *invokeCacheArray is truly a pointer to an object, what prevents GC from moving that object while we try to access it? We may need to hold VMAccess while executing if (!fej9()->isArrayObject(*invokeCacheArray)) (extend the VMAccessCriticalSection which starts below).
There is also a comment below that "// this will not work in AOT or JITServer", so probably there is some guarantee that we cannot enter this path for JITServer.

[nbhuiyan]

if *invokeCacheArray is truly a pointer to an object, what prevents GC from moving that object while we try to access it?

I did not feel it was necessary to ensure VM access for this. My reasoning was, immediately after checking if the corresponding entry in the call site table had a valid object pointer using

openj9/runtime/compiler/env/j9method.cpp

Lines 5915 to 5919 in ab5dfcf

	bool
	TR_ResolvedJ9Method::isUnresolvedCallSiteTableEntry(int32_t callSiteIndex)
	{
	return *(j9object_t*)callSiteTableEntryAddress(callSiteIndex) == NULL;
	}

We want to check whether that non-null object pointer cached in the call site table slot was an array or not. If that object pointer at ramClass->callSites[callSiteIndex] (i.e, *invokeCacheArray) is not pointing to a valid array object, then we should abort anyway. The slot is populated by an object that the VM's invokedynamic resolution mechanism allocates and (I'm speculating) the GC probably can not move. It's not a regular Java object. Once a call site table slot is populated (only if it was NULL to begin with), it is not modified again. What we do need VM access for however is accessing the individual elements in the invokeCacheArray, which is why we have the VM access being acquired right after this.

Based on my reasoning, do you still think it is necessary to put the isArrayObject check within the block with VM access acquired?

There is also a comment below that "// this will not work in AOT or JITServer", so probably there is some guarantee that we cannot enter this path for JITServer.

This was true when this code was first written during the initial OpenJDK MethodHandle implementation, but since then JITServer became capable of supporting OpenJDK MHs and I can confirm that this code path is executed in JITServer now based on my testing. I will update the comment here to reflect the current state.

[nbhuiyan]

Responding to your overall review comment:

Moreover, the JITServer should never receive any object references. Thus, the new frontend method should just fatal assert if executed at the server rather than sending a message at the client.

Wouldn't this mean that JITServer loses support for any method containing invokedynamic bytecodes?

[mpirvu]

If that array stored in one of the callsites is really special and cannot be moved by GC (maybe because it does not reside in the heap), then you don't need VM access for checking whether it's an array. Interesting though that the two elements of the array are protected by VM access.

[mpirvu]

fej9()->targetMethodFromMemberName() has a FATAL_ASSERT in the JITServer implementation.

TR_OpaqueMethodBlock*
TR_J9ServerVM::targetMethodFromMemberName(uintptr_t memberName)
   {
   TR_ASSERT_FATAL(false, "targetMethodFromMemberName must not be called on JITServer");
   return NULL;
   }

so if we ever entered that path while compiling at the server, the assert would be triggered.

[mpirvu]

Another point: getResolvedDynamicMethod() has a different implementation for JITServer where the server asks the client to execute the corresponding method and then, with the answers received from the client, the server builds a resolvedMethod. Thus, the server does not need to access Java objects.

[nbhuiyan]

Thank you for pointing those out, I had the wrong understanding of how things were being handled in JITServer mode. I will make the changes as you suggested.

I will also enforce VM access requirement for checking if object is array, so as not to cause confusion in the future for others, as there is no real benefit to doing this without VM access but lots of potential danger if this API was later used to check if heap objects were array types.

[mpirvu]

I am surprised though that we cache object references in J9Class (J9Class->callSites[]). Either GC cannot move such objects or somehow it updates all these pointers when objects are moved.

[nbhuiyan]

@mpirvu I have updated the implementation based on the review comments. I would appreciate another review.

[mpirvu]

LGTM

[mpirvu]

jenkins test sanity all jdk20

[mpirvu]

jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit jdk20

[mpirvu]

All tests have passed.
@0xdaryl if you officially approve this PR I can merge it. Thanks

[nbhuiyan]

@0xdaryl I have addressed your comments. The latest force push addressed indentation issues, removed an empty line I accidentally added, and added curly braces for an if statement. As the tests have passed earlier although not visible due to the updated branch, relaunching them should not be necessary for these changes alone.

[0xdaryl]

Recent changes were mostly whitespace. Previous testing was successful. No need to re-run CI. Merging.