Fix bug in profiled inlined method relocation #5704

dsouzai · 2019-05-09T15:29:39Z

For the TR_RelocationRecordProfiledInlinedMethod and subclass relocation
records, what's stored in the relo header is:

The class chain of class specified in the TR_VirtualGuard
The class chain of the first class loaded by the classloader that
loaded the class in 1.
the VFT offset

The idea was to validate that the receiver was the same, and then to add
the appropriate inlined J9Method into the J9JitExceptionTable's inlined
method table during relocation time. The VFT offset was determined by
using the symref stored in the guard (which is the symref of the
caller).

A problem occurs when there is an invokeVirtual called on a method via a
class that implements an interface AND the method being invoked is a
default method of that interface. In other words, the defining class of
the method is an interface class, and the implementing class inherits
this method.

When the compiler creates the profiled guard, the TR_VirtualGuard
contains the J9Class of the defining class; in this case it would be the
interface class. However, the symref contains the VFT offset of a
concrete class. Thus, at load time, the relo infrastructure will
materialize the interface class based on the class chain info in the
relo header, and then try to pull out a J9Method from that class' VFT
using the VFT offset. However, interface classes do not have a VFT. As a
result, this can cause undefined behaviour.

This PR addresses this issue by modifying the relo header to store
the method index from the defining class rather than the VFT offset from
the caller. This involves also now changing the the class chains stored
to:

The class chain of the defining class of the inlined method
The class chain of the first class loaded by the classloader that
loaded the class in 1.

Because this is a profiled inlined method, whether the guard
is a Method Test, VFT Test, or Removed Guard, this will yield the
correct method to populate in the inlined table of the
J9JITExceptionTable.

One important subtlety though, is that in the case of the VFT Test, the
inlined code might have certain assumptions based on the type. However,the
TR_RelocationRecordProfiledInlinedMethod (and subtypes) primarily
deal with populating the inlined table in the J9JITExceptionTable. The
VFT test will result in a TR_ClassPointer relocation record that will
relocate the class being tested by the VFT test guard.

dsouzai · 2019-05-09T15:32:02Z

Didn't realize I had to make the PR a draft at creation time, so WIP it'll have to be.

@jdmpapin @mstoodle could you guys please review? I want to do more tests on this, but I figure it's worth getting your eyes on the code.

jdmpapin

Logic LGTM. Just a few subjective/stylistic notes.

The duplication of logic newly added to the implementations of initializeAOTRelocationHeader() sticks out to me, but that's the case for everything in those methods and deduplicating it is a separate issue

jdmpapin · 2019-05-09T18:59:51Z

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

-            //traceMsg(comp(),"ProfiledGuard offset for non-interface is %d\n", offset);
-            offset = (uintptrj_t) fej9->virtualCallOffsetToVTableSlot((int32_t) callSymRef->getOffset());
-            //traceMsg(comp(),"ProfiledGuard offset for non-interface is now %d\n", offset);
+            comp->failCompilation<TR::CompilationException>("Failed to get method index for profiled inlined method");


This seems like it should be pretty impossible. Should we use TR_ASSERT_FATAL?

jdmpapin · 2019-05-09T19:00:40Z

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

+
+         uint32_t numMethods = fej9->getNumMethods(inlinedCodeClass);
+         uint32_t methodIndex = 0;
+         for (; methodIndex < numMethods ; methodIndex++)


It bothers me a little bit that this is looping when a pointer subtraction would do. Unfortunately, pointer subtraction is UB in case some bug causes inlinedJ9Method to be missing from resolvedMethods. I think it's possible to do the subtraction using integers and check the range though, e.g.

uintptr_t methodOffset = (uintptr_t)inlinedJ9Method - (uintptr_t)resolvedMethods; TR_ASSERT_FATAL(methodOffset % sizeof (J9Method) == 0, "..."); uintptr_t methodIndex = methodOffset / sizeof (J9Method); TR_ASSERT_FATAL(methodIndex < numMethods, "..."); // Now it's safe to truncate methodIndex to 32-bit

But I understand if you're more comfortable with the loop 🤷‍♂️

I don't have a concern with not using a loop as long as i add a comment describing what the code's doing.

dsouzai · 2019-05-09T20:23:28Z

The duplication of logic newly added to the implementations of initializeAOTRelocationHeader() sticks out to me, but that's the case for everything in those methods and deduplicating it is a separate issue

Yeah I'm taking care of that as part of my consolidation effort (which, amusingly enough, will now have a merge conflict because of this change 🙂)

mstoodle

I hope I'm not the only one saddened that you managed to make a significant change to the structure of one relocation record without having to change a single comment :( . The code is fairly straight-forward, but you've created the new term "method index" that I don't think we use anywhere else (replacing "vtable slot" which is well understood). Please add some comments to explain what a method index is.

The fact that we can argue over how to compute it also makes it seem like a new "thing". I would suggest hiding the duplication behind a new front end query that will essentially define what "method index" means (getInlinedMethodIndex() maybe?).

dsouzai · 2019-05-14T15:24:46Z

@jdmpapin @mstoodle made the requested changes. Could you guys please re-review?

mstoodle

LGTM

mstoodle · 2019-05-14T16:02:47Z

runtime/compiler/env/VMJ9.cpp

+ * index of the J9Method in the J9Class' array of J9Methods.
+ */
+uintptr_t
+TR_J9VMBase::getMethodIndexInClass(TR_OpaqueClassBlock *classPointer, TR_OpaqueMethodBlock *methodPointer)


better name than the one I quickly proposed. I like this a lot better now.

jdmpapin · 2019-05-14T18:49:36Z

runtime/compiler/env/VMJ9.cpp

+   TR_ASSERT_FATAL((methodOffset % sizeof (J9Method)) == 0, "methodOffset %p isn't a multiple of sizeof(J9Method)\n", methodOffset);
+
+   uintptr_t methodIndex = methodOffset / sizeof (J9Method);
+   TR_ASSERT_FATAL(methodIndex < numMethods, "methodIndex %p greater than numMethods %d for method %p in class %p\n",


Should this format string (and the one just above) be using %p for integers?

I'd like to recommend PRIxPTR, but the format string is used for both vfprintf() and omrstr_vprintf(), which don't agree on the interpretation of all format specifiers. In particular, omrstr_vprintf() reads a 32-bit integer for %lx even on LP64 systems, where PRIxPTR could be "lx".

I think the safest way to format a pointer-sized integer here is to cast it to long long (which should be 64-bit on all of our platforms) and use %llx.

I"ll cast it to uint64_t (just in case) and use %llx as you suggest.

jdmpapin · 2019-05-14T18:50:49Z

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

-         //traceMsg(comp(),"ProfiledGuard offset using %d\n", offset);
-         *(uintptrj_t *) cursor = offset;
+         uintptrj_t methodIndex = fej9->getMethodIndexInClass(inlinedCodeClass, inlinedMethod->getNonPersistentIdentifier());
+         *(uintptrj_t *)cursor =  static_cast<uintptrj_t>(methodIndex);


This line (along with the other copies) is now casting from uintptrj_t to uintptrj_t.

This commit adds a front end query that takes a ramclass and rammethod and returns the index of the rammethod in the ramclass' array of rammethods. It expects the rammethod to be defined by the ramclass, and therefore has a couple of TR_ASSERT_FATALs to (indirectly) ensure this. Signed-off-by: Irwin D'Souza <[email protected]>

For the TR_RelocationRecordProfiledInlinedMethod and subclass relocation records, what's stored in the relo header is: 1. The class chain of class specified in the TR_VirtualGuard 2. The class chain of the first class loaded by the classloader that loaded the class in 1. 3. the VFT offset The idea was to validate that the receiver was the same, and then to add the appropriate inlined J9Method into the J9JitExceptionTable's inlined method table during relocation time. The VFT offset was determined by using the symref stored in the guard (which is the symref of the *caller*). A problem occurs when there is an invokeVirtual called on a method via a class that implements an interface AND the method being invoked is a default method of that interface. In other words, the defining class of the method is an interface class, and the implementing class inherits this method. When the compiler creates the profiled guard, the TR_VirtualGuard contains the J9Class of the defining class; in this case it would be the interface class. However, the symref contains the VFT offset of a concrete class. Thus, at load time, the relo infrastructure will materialize the interface class based on the class chain info in the relo header, and then try to pull out a J9Method from that class' VFT using the VFT offset. However, interface classes do not have a VFT. As a result, this can cause undefined behaviour. This commit addresses this issue by modifying the relo header to store the method index from the defining class rather than the VFT offset from the caller. This involves also now changing the the class chains stored to: 1. The class chain of the defining class of the inlined method 2. The class chain of the first class loaded by the classloader that loaded the class in 1. Because this is a profiled inlined method, whether the guard is a Method Test, VFT Test, or Removed Guard, this will yield the correct method to populate in the inlined table of the J9JITExceptionTable. One important subtlety though, is that in the case of the VFT Test, the inlined code might have certain assumptions based on the type (because the class chain stored now. However,the TR_RelocationRecordProfiledInlinedMethod (and subtypes) primarily deal with populating the inlined table in the J9JITExceptionTable. The VFT test will result in a TR_ClassPointer relocation record that will relocate the class being tested by the VFT test guard. Signed-off-by: Irwin D'Souza <[email protected]>

dsouzai · 2019-05-14T19:45:05Z

@jdmpapin made the requested changes (see https://github.com/eclipse/openj9/compare/1aa0eefd5f423f23dbe2b2bcb0aa1f572f6fa437..5d9d9a4925adbe2bddeec3f11e5006306c29ecb4)

dsouzai · 2019-05-14T20:24:16Z

Jenkins test sanity.functional+aot all jdk8

dsouzai · 2019-05-14T20:25:11Z

I've opened #4875 to remove existing tests that are not valid for AOT, but I figure it's worth seeing if the AOT tests work :)

dsouzai · 2019-05-14T20:36:34Z

Jenkins test sanity.functional+aot xlinux,plinux,zlinux jdk8,jdk11

dsouzai · 2019-05-15T15:03:54Z

I took a look at the failing jobs (there were only a handful, which was nice to see). None were AOT issues, but rather, test and configuration issues (for which I've opened #5767 and #5768). For now I'll just kick off regular sanity builds.

dsouzai · 2019-05-15T15:04:44Z

Jenkins test sanity.functional xlinux,plinux,zlinux,osx,win32 jdk8,jdk11

dsouzai · 2019-05-15T17:50:52Z

@mstoodle all tests passed; could you merge if everything looks good to you.

dsouzai added comp:jit comp:jit:aot labels May 9, 2019

dsouzai changed the title ~~Fix bug in profiled inlined method relocation~~ WIP: Fix bug in profiled inlined method relocation May 9, 2019

jdmpapin reviewed May 9, 2019

View reviewed changes

mstoodle requested changes May 10, 2019

View reviewed changes

dsouzai force-pushed the profInlMethodBug branch from 29ab961 to 1aa0eef Compare May 14, 2019 15:24

dsouzai changed the title ~~WIP: Fix bug in profiled inlined method relocation~~ Fix bug in profiled inlined method relocation May 14, 2019

mstoodle approved these changes May 14, 2019

View reviewed changes

jdmpapin reviewed May 14, 2019

View reviewed changes

Irwin D'Souza added 2 commits May 14, 2019 15:44

dsouzai force-pushed the profInlMethodBug branch from 1aa0eef to 5d9d9a4 Compare May 14, 2019 19:44

jdmpapin approved these changes May 14, 2019

View reviewed changes

mstoodle merged commit 7df6278 into eclipse-openj9:master May 15, 2019

dsouzai deleted the profInlMethodBug branch June 25, 2019 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in profiled inlined method relocation #5704

Fix bug in profiled inlined method relocation #5704

dsouzai commented May 9, 2019 •

edited

Loading

dsouzai commented May 9, 2019

jdmpapin left a comment

jdmpapin May 9, 2019

jdmpapin May 9, 2019

dsouzai May 9, 2019 •

edited

Loading

dsouzai commented May 9, 2019

mstoodle left a comment

dsouzai commented May 14, 2019

mstoodle left a comment

mstoodle May 14, 2019

jdmpapin May 14, 2019

dsouzai May 14, 2019

jdmpapin May 14, 2019

dsouzai commented May 14, 2019

dsouzai commented May 14, 2019

dsouzai commented May 14, 2019

dsouzai commented May 14, 2019

dsouzai commented May 15, 2019 •

edited

Loading

dsouzai commented May 15, 2019

dsouzai commented May 15, 2019

Fix bug in profiled inlined method relocation #5704

Fix bug in profiled inlined method relocation #5704

Conversation

dsouzai commented May 9, 2019 • edited Loading

dsouzai commented May 9, 2019

jdmpapin left a comment

Choose a reason for hiding this comment

jdmpapin May 9, 2019

Choose a reason for hiding this comment

jdmpapin May 9, 2019

Choose a reason for hiding this comment

dsouzai May 9, 2019 • edited Loading

Choose a reason for hiding this comment

dsouzai commented May 9, 2019

mstoodle left a comment

Choose a reason for hiding this comment

dsouzai commented May 14, 2019

mstoodle left a comment

Choose a reason for hiding this comment

mstoodle May 14, 2019

Choose a reason for hiding this comment

jdmpapin May 14, 2019

Choose a reason for hiding this comment

dsouzai May 14, 2019

Choose a reason for hiding this comment

jdmpapin May 14, 2019

Choose a reason for hiding this comment

dsouzai commented May 14, 2019

dsouzai commented May 14, 2019

dsouzai commented May 14, 2019

dsouzai commented May 14, 2019

dsouzai commented May 15, 2019 • edited Loading

dsouzai commented May 15, 2019

dsouzai commented May 15, 2019

dsouzai commented May 9, 2019 •

edited

Loading

dsouzai May 9, 2019 •

edited

Loading

dsouzai commented May 15, 2019 •

edited

Loading