Add TR_J2IVirtualThunkPointer relocation kind #2874

jdmpapin · 2018-09-14T15:37:29Z

This relocates a J2I (JIT to interpreter) virtual thunk pointer along
with a nearby constant pool address and index. The relocation is
necessary because with nestmates, the data consumed by PicBuilder now
contains a J2I thunk pointer, which is used for direct dispatch in case
the callee has not yet been compiled.

No such relocations are created yet. That will be done in each code
generator's call snippet code.

jdmpapin · 2018-09-14T15:38:09Z

Marked WIP because this depends on eclipse-omr/omr#2965

jdmpapin · 2018-09-14T17:58:43Z

@dsouzai @mstoodle Would you mind reviewing? This goes with #2875 and eclipse-omr/omr#2965

jdmpapin · 2018-09-14T17:59:52Z

@irinarada FYI

dsouzai

LGTM

mstoodle · 2018-09-14T21:14:25Z

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

@@ -740,7 +756,9 @@ uint32_t J9::ARM::AheadOfTimeCompile::_relocationTargetTypeToHeaderSizeMap[TR_Nu
   0,                                        // TR_NativeMethodAbsolute                = 56,
   0,                                        // TR_NativeMethodRelative                = 57,
   16,                                       // TR_ArbitraryClassAddress               = 58,
-   28                                        // TR_DebugCounter                        = 59
+   28,                                       // TR_DebugCounter                        = 59
+   0,                                        // TR_ClassUnloadAssumption               = 60


is TR_ClassUnloadAssumption supposed to be in this pull request?

I see, you're also co-dependent on #2672 (one of you will conflict with whichever is merged first :) ).

TR_ClassUnloadAssumption is an existing TR_ExternalRelocationTargetKind with value 60. It wasn't in the size tables previously because it's not for AOT:

TR_ClassUnloadAssumption = 60, // this should not be used in AOT relocations

I need to specify a value there in order to get to index 61 (TR_J2IVirtualThunkPointer).

It's not for AOT, but the relocations are still external ones used for JIT as a Service which means the size entries will need to be set appropriately (just not as part of this PR :) ). I'm fine with them being set to zero here unless, of course, #2672 is merged first.

mstoodle · 2018-09-14T21:30:59Z

I have to express some regret that this solution is convoluting an already convoluted thing more than it really should be. I would have preferred to evolve this code in a direction of simplicity.

Let me elaborate a little bit. TR_Thunks is currently "over optimized" in my opinion in that performs a constant pool relocation as well as the work required to relocate the thunk. On top of that, it reads the cpIndex out of the code cache rather than having cpIndex as an element in the relocation record. We should really evolve TR_Thunks to only relocate the thunk, and create a TR_ConstantPool relocation to relocate the constant pool. Then we would have introduced a new relocation to accomplish the relocation needed here. I've spoken offline with @jdmpapin and I believe we're in agreement on this general approach being a better way forward.

Sadly, that's too much work to get this done in time for our impending 0.10 release. So, I will (reluctantly but pragmatically) review this code as-is even though it's not the generally agreed upon right way forward.

@jdmpapin can I ask you to open an issue to detail the improved design so that we do not lose track of this work, and I'll review. It would be awesome to see that correction made in time for 0.11, but its timeline isn't all that far away either, so I could be convinced we shouldn't muck with it until after 0.11 goes out the door.

mstoodle

Mostly looks very good, but I'd like to see that assert and I think you need to implement TR_RelocationRecordJ2IVirtualThunkPointer::preparePrivateData()

mstoodle · 2018-09-14T21:15:50Z

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

@@ -740,7 +756,9 @@ uint32_t J9::ARM::AheadOfTimeCompile::_relocationTargetTypeToHeaderSizeMap[TR_Nu
   0,                                        // TR_NativeMethodAbsolute                = 56,
   0,                                        // TR_NativeMethodRelative                = 57,
   16,                                       // TR_ArbitraryClassAddress               = 58,
-   28                                        // TR_DebugCounter                        = 59
+   28,                                       // TR_DebugCounter                        = 59
+   0,                                        // TR_ClassUnloadAssumption               = 60


I see, you're also co-dependent on #2672 (one of you will conflict with whichever is merged first :) ).

mstoodle · 2018-09-14T21:45:59Z

runtime/compiler/runtime/RelocationRecord.cpp

+   TR_RelocationTarget *reloTarget,
+   uint8_t *reloLocation,
+   void *thunk)
+   {


I'd stick an assert here that thunk != NULL just for completeness

Assertion added

mstoodle · 2018-09-14T21:49:29Z

runtime/compiler/runtime/RelocationRecord.cpp

+   // J2I thunk pointer, but rather the location of the constant pool address.
+   // Find the J2I thunk pointer relative to that.
+   auto recordData = (TR_RelocationRecordJ2IVirtualThunkPointerBinaryTemplate *)_record;
+   reloLocation += recordData->_offsetToJ2IVirtualThunkPointer;


I think you're missing the preparePrivateData() code here....nothing sets _offsetToJ2IVirtualThunkPointer from the binary relocation data, does it? Or did I miss it?

Doesn't _record point into the binary relocation data? I do get the correct offset (when I run with the further change in #2875)

Ok, I was mis-remembering (and didn't read your code well enough :( ) this part of the code, so my apologies for the wrong suggestion.

You do not need to use the private relo data for this particular case. But you do need to use the reloTarget to access the value out of the binary reloRecord. You should define something like TR_RelocationRecordJ2IVirtualThunkPointer::offsetToJ2IVirtualThunkPointer(TR_RelocationTarget *reloTarget) (akin to TR_RelocationRecordConstantPoolWithIndex::cpIndex(TR_RelocationTarget *reloTarget) to fetch the value out of the reloRecord using an appropriate function from reloTarget.

The reason to consistently use the reloTarget is so that we have a hope to deal with endian issues if we ever cross compile (which, for the record, may be something we are drawn towards once the JIT as a Service work matures).

Updated to define and call a helper method that uses reloTarget to read _offsetToJ2IVirtualThunkPointer

fjeremic · 2018-09-15T02:30:41Z

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

+         *(uintptrj_t *)cursor = (uintptrj_t)info->data1; // constantPool
+         cursor += SIZEPOINTER;
+
+         *(uintptrj_t *)cursor = (uintptrj_t)info->data3; // offset to J2I virtual thunk pointer


Ah, the good ol' data1, data2, data3 field names. I definitely agree with @mstoodle we need an overhaul in this area of the code. This seems like too much work just to add a pointer relocation.

This relocates a J2I (JIT to interpreter) virtual thunk pointer along with a nearby constant pool address and index. The relocation is necessary because with nestmates, the data consumed by PicBuilder now contains a J2I thunk pointer, which is used for direct dispatch in case the callee has not yet been compiled. No such relocations are created yet. That will be done in each code generator's call snippet code. Signed-off-by: Devin Papineau <devinmp@ca.ibm.com>

jdmpapin · 2018-09-17T18:51:29Z

Updated to address @mstoodle's feedback, and to resolve conflicts since #2672 has been merged

jdmpapin · 2018-09-17T18:54:50Z

Removed WIP now that eclipse-omr/omr#2965 has been merged

mstoodle

LGTM. Thanks, @jdmpapin ! Just need to run the tests before we merge...

mstoodle · 2018-09-17T19:07:18Z

@andrewcraik has graciously agreed to usher this PR the rest of the way, since I will not be able to keep a very close eye on it for the next little while. Thanks, Andrew!

andrewcraik · 2018-09-17T19:09:14Z

Jenkins test sanity xlinux,win jdk11

andrewcraik · 2018-09-17T21:45:47Z

travis failed due to the il change that depended on #2649 which has been merged now to resolve the failure

andrewcraik · 2018-09-17T21:46:24Z

JDK11 windows fails look to be existing known issues/unrelated to this change

andrewcraik · 2018-09-17T21:58:48Z

JDK11 Linux fails look to be existing known issues/unrelated to this change

andrewcraik · 2018-09-17T22:00:46Z

Given the failures are unrelated/known issues I'll merge this now to unblock other changes (eg #2875)

jdmpapin mentioned this pull request Sep 14, 2018

Relocate the J2I virtual thunk in PIC data on x86 #2875

Merged

andrewcraik added comp:jit depends:omr Pull request is dependent on a corresponding change in OMR labels Sep 14, 2018

dsouzai approved these changes Sep 14, 2018

View reviewed changes

mstoodle reviewed Sep 14, 2018

View reviewed changes

mstoodle requested changes Sep 14, 2018

View reviewed changes

IBMJimmyk mentioned this pull request Sep 14, 2018

Relocate the J2I thunk in PIC data on Power #2882

Merged

fjeremic reviewed Sep 15, 2018

View reviewed changes

NigelYiboYu mentioned this pull request Sep 17, 2018

Enable nestmate J2I thunk AOT support #2892

Merged

jdmpapin force-pushed the aot-nestmates branch from 3b66171 to 0da7fa8 Compare September 17, 2018 18:49

jdmpapin changed the title ~~WIP: Add TR_J2IVirtualThunkPointer relocation kind~~ Add TR_J2IVirtualThunkPointer relocation kind Sep 17, 2018

mstoodle approved these changes Sep 17, 2018

View reviewed changes

mstoodle assigned andrewcraik Sep 17, 2018

andrewcraik merged commit 1bd7735 into eclipse-openj9:master Sep 17, 2018

jdmpapin mentioned this pull request Sep 17, 2018

Clean up TR_Thunks and TR_J2IVirtualThunkPointer relocations #2903

Open

jdmpapin mentioned this pull request Sep 19, 2018

[Nestmates] Handle private invoke for jitted unresolved invokevirtual/interface #2673

Closed

jdmpapin deleted the aot-nestmates branch September 25, 2018 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TR_J2IVirtualThunkPointer relocation kind #2874

Add TR_J2IVirtualThunkPointer relocation kind #2874

jdmpapin commented Sep 14, 2018

jdmpapin commented Sep 14, 2018

jdmpapin commented Sep 14, 2018

jdmpapin commented Sep 14, 2018

dsouzai left a comment

mstoodle Sep 14, 2018

mstoodle Sep 14, 2018

jdmpapin Sep 14, 2018

mstoodle Sep 15, 2018 •

edited

Loading

mstoodle commented Sep 14, 2018 •

edited

Loading

mstoodle left a comment

mstoodle Sep 14, 2018

mstoodle Sep 14, 2018

jdmpapin Sep 17, 2018

mstoodle Sep 14, 2018

jdmpapin Sep 14, 2018 •

edited

Loading

mstoodle Sep 15, 2018 •

edited

Loading

jdmpapin Sep 17, 2018

fjeremic Sep 15, 2018

jdmpapin commented Sep 17, 2018

jdmpapin commented Sep 17, 2018

mstoodle left a comment

mstoodle commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

Add TR_J2IVirtualThunkPointer relocation kind #2874

Add TR_J2IVirtualThunkPointer relocation kind #2874

Conversation

jdmpapin commented Sep 14, 2018

jdmpapin commented Sep 14, 2018

jdmpapin commented Sep 14, 2018

jdmpapin commented Sep 14, 2018

dsouzai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoodle Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

mstoodle commented Sep 14, 2018 • edited Loading

mstoodle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdmpapin Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

mstoodle Sep 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdmpapin commented Sep 17, 2018

jdmpapin commented Sep 17, 2018

mstoodle left a comment

Choose a reason for hiding this comment

mstoodle commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

andrewcraik commented Sep 17, 2018

mstoodle Sep 15, 2018 •

edited

Loading

mstoodle commented Sep 14, 2018 •

edited

Loading

jdmpapin Sep 14, 2018 •

edited

Loading

mstoodle Sep 15, 2018 •

edited

Loading