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

Refine linkToVirtual to a direct call when no vtable entry exists #13474

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

nbhuiyan
Copy link
Member

@nbhuiyan nbhuiyan commented Sep 8, 2021

MethodHandleTransformer refines linkToVirtual calls to the target
method when MemberName is known. The call kind of the refined
linkToVirtual call target was always treated as Virtual.
However, for private virtual methods and java/lang/Object;
type virtual methods, there is no corresponding entry in the vtable,
and for such methods the interpreter vtable index is set as 0.
The dispatch is not performed through the vtable entry, but instead
it is a direct call to the target method obtained from MemberName.

By checking if jitVtableOffset is positive, we can determine if
a call needs to be dispatched through the vTable. Otherwise, we
can handle it similar to linkToStatic.

Closes: #13430

Signed-off-by: Nazim Bhuiyan nubhuiyan@ibm.com

@nbhuiyan
Copy link
Member Author

nbhuiyan commented Sep 8, 2021

@0xdaryl @knn-k @Akira1Saitoh FYI

MethodHandleTransformer refines linkToVirtual calls to the target
method when MemberName is known. The call kind of the refined
linkToVirtual call target was always treated as Virtual.
However, for private virtual methods and java/lang/Object;
type virtual methods, there is no corresponding entry in the vtable,
and for such methods the interpreter vtable index is set as 0.
The dispatch is not performed through the vtable entry, but instead
it is a direct call to the target method obtained from MemberName.

By checking if jitVtableOffset is positive, we can determine if
a call needs to be dispatched through the vTable. Otherwise, we
can handle it similar to linkToStatic.

Signed-off-by: Nazim Bhuiyan <nubhuiyan@ibm.com>
@nbhuiyan nbhuiyan changed the title Remove fatal assert verifying virtual call offset is zero Refine linkToVirtual to a direct call when no vtable entry exists Sep 9, 2021
@nbhuiyan
Copy link
Member Author

nbhuiyan commented Sep 9, 2021

@0xdaryl requesting review

@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 9, 2021

Jenkins test sanity all jdk17

Getting a head start on the testing...

@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 10, 2021

Jenkins test sanity alinux64 jdk17

@0xdaryl 0xdaryl self-assigned this Sep 10, 2021
// in Membername.vmtarget. In such cases, we can refine a linkToVirtual call similar to linkToStatic.
// jitVTableOffset is obtained using sizeof(J9Class) - interpreter vtable offset, resulting in a positive
// value when the interpreter vtable index we get (from MemberName.vmindex.vtableoffset) is set as 0.
if (jitVTableOffset > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this test. If the problematic interpreter vtable offset is "sizeof(J9Class)" or +376 then shouldn't this test be jitVTableOffset >= 0 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problematic interpreter vtable offset is when it is set as 0, resulting in the jitVtableOffset being sizeof(J9Class). Normally, interpreter vtable offset is set a value > sizeof(J9Class) so the vtable offset we obtain will always be negative, which is why I thought a simple test for a positive integer value was sufficient. We can't have jitVtableOffset == 0 as that is the start of the J9Class. Do you think changing the operator to >= would provide more clarity?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just for reference, here is where the interpreter vtable offset is set:

/* Iterate over the vtable from the end to the beginning, skipping
* the "magic" header entries. This ensures that the most "recent" override of the
* method is found first. Critically important for super sends
* ie: invokespecial) being correct
*/
J9VTableHeader *vTable = J9VTABLE_HEADER_FROM_RAM_CLASS(methodClass);
J9Method **vTableMethods = J9VTABLE_FROM_HEADER(vTable);
UDATA vTableIndex = vTable->size;
while (vTableIndex > 0) {
vTableIndex -= 1;
if (method == vTableMethods[vTableIndex]) {
return J9VTABLE_OFFSET_FROM_INDEX(vTableIndex);
}
}

J9VTABLE_OFFSET_FROM_INDEX takes the index in the vtable and adds sizeof(J9Class) + sizeof(J9VTableHeader), and the vtable entries begin following the J9VTableHeader. So whenever there's a vtable entry, the jitVTableOffset will end up being a negative value.

@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 13, 2021

Jenkins test sanity alinux64 jdk17

@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 13, 2021

Internal AArch64 testing succeeded (finally--after some infra problems).

@0xdaryl 0xdaryl merged commit 87c301e into eclipse-openj9:master Sep 13, 2021
@0xdaryl
Copy link
Contributor

0xdaryl commented Sep 13, 2021

Please create a PR for the 0.28.0 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants