Consolidate AOT Relocation Records #10916

dsouzai · 2020-10-16T20:19:59Z

This one seems relatively big compared to the previous such PRs. However,

it involves records where the header info is gathered differently on some platforms
it is the last of the consolidation efforts; this should be the penultimate PR as the next PR is just miscellaneous cleanup (such as moving the binary template definitions back to the .cpp file, and so on).

dsouzai · 2020-10-19T14:57:08Z

@mstoodle could you please revew?

dsouzai · 2020-10-19T18:50:58Z

runtime/compiler/codegen/J9AheadOfTimeCompile.cpp

-   reloRecord->setSize(reloTarget, relocation->getSizeOfRelocationData());
-   reloRecord->setType(reloTarget, kind);
-
-   uint8_t wideOffsets = relocation->needsWideOffsets() ? RELOCATION_TYPE_WIDE_OFFSET : 0;
-   reloRecord->setFlag(reloTarget, wideOffsets);
-


The reason for this is because of an ordering issue, namely:

Do platform specific way of gathering data for the relocation header, if it exists

Otherwse, do common way of gathering data for the relocation header

If a relo type has both a platform specific implementation and a common implementation, the platform specific implementation must be done. This means that even though this deleted code is common regardless of the relotype, we cannot move it into the code that does the common way of gathering data, because we might not call into it. Therefore, this code is moved into each of the platform specific versions of initializeAOTRelocationHeader

not a huge fan of spraying code like that. Did you consider defining a "common" initialization routine that must be called for all platforms followed/preceded by a call to either the platform or "default" initialization code? You'd still have the flexibility to move stuff from the "common" initialization routine to all the platform routines if needed, but if you could share it, it can go into that common routine.

FWIW, this code was technically redundant because the the TR_RelocationRecordBinaryTemplate fields were being initialized in each of the various implementations via the raw pointer manipulation, before executing this code, so I definitely didn't increase the code duplication, I just didn't decrease it heh :).

However, adding a common pre-initialization routine is definitely a good idea; I will add it in the misc clean up PR that I'll open after this. Given how big this PR is I don't want to complicate it further or introduce some stupid bug.

dsouzai · 2020-10-19T18:53:07Z

runtime/compiler/aarch64/codegen/J9AheadOfTimeCompile.cpp

-   TR::LabelSymbol *table;
-   uint8_t *codeLocation;
+   // Zero-initialize header
+   memset(cursor, 0, sizeOfReloData);


This is extremely necessary. Prior to this change, the flags were written in via a pointer. Now, it is written in using the setFlag API, which masks it in. Therefore, we need to make sure that buffer does not contain random data or it could result in that random data being interpreted as valid bits in the flag field.

dsouzai · 2020-10-19T18:55:16Z

runtime/compiler/aarch64/codegen/J9AheadOfTimeCompile.cpp

-   // This has to be created after the kind has been written into the header
-   TR_RelocationRecord storage;
-   TR_RelocationRecord *reloRecord = TR_RelocationRecord::create(&storage, reloRuntime, reloTarget, reinterpret_cast<TR_RelocationRecordBinaryTemplate *>(relocation->getRelocationData()));
+   cursor += sizeof(TR_RelocationRecordBinaryTemplate);


Temporary in this commit, this is removed in the last commit in this PR. It was needed because the yet to be consolidated relocation records assume the position of the cursor is past the TR_RelocationRecordBinaryTemplate portion of the header.

dsouzai · 2020-10-19T18:56:34Z

runtime/compiler/x/codegen/J9AheadOfTimeCompile.cpp

+   TR_RelocationRecord *reloRecord = TR_RelocationRecord::create(&storage, reloRuntime, targetKind, reinterpret_cast<TR_RelocationRecordBinaryTemplate *>(cursor));
+
+   uint8_t wideOffsets = relocation->needsWideOffsets() ? RELOCATION_TYPE_WIDE_OFFSET : 0;
+   uint32_t *wordAfterHeader = &reinterpret_cast<TR_RelocationRecordPicTrampolineBinaryTemplate *>(cursor)->_numTrampolines;


Temporary, this is removed by the end of the PR. Consequence of the hacky

#if defined(TR_HOST_64BIT) uint32_t _extra; #endif

field in TR_RelocationRecordBinaryTemplate

dsouzai · 2020-10-27T14:44:56Z

@mstoodle review reminder :)

dsouzai · 2020-11-02T16:36:01Z

@mstoodle friendly review reminder.

dsouzai · 2020-11-09T14:47:44Z

@mstoodle friendly review reminder

mstoodle

would you please change title to "Add new TR_RelocationRecord::create API" and add a few words to explain why you're adding this new API?

mstoodle · 2020-11-13T16:24:37Z

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

+         moRecord->setConstantPool(reloTarget, reinterpret_cast<uintptr_t>(symRef->getOwningMethod(comp)->constantPool()));
+         moRecord->setReloFlags(reloTarget, flags);
+
+         cursor = relocation->getRelocationData() + TR_RelocationRecord::getSizeOfAOTRelocationHeader(static_cast<TR_RelocationRecordType>(targetKind));


the fact that this relocation is implemented differently for this platform should probably be fixed at some point. Would you please create an issue for someone to bring these two implementations in line?

Opened #11201

mstoodle · 2020-11-13T17:11:12Z

runtime/compiler/arm/codegen/J9AheadOfTimeCompile.cpp

-         uintptr_t inlinedSiteIndex = self()->findCorrectInlinedSiteIndex(tempSR->getOwningMethod(comp)->constantPool(), recordInfo->data2);
+         TR::SymbolReference *symRef = reinterpret_cast<TR::SymbolReference *>(recordInfo->data1);
+         uintptr_t inlinedSiteIndex = reinterpret_cast<uintptr_t>(recordInfo->data2);
+         uint8_t flags = static_cast<uint8_t>(recordInfo->data3);


similar comment here for the ARM and P implementations, which only seem to differ in the requirement for flags? Seems like it could be folded into the common implementation. Another issue or maybe combine into one issue?

mstoodle

Looks really good @dsouzai ! Was a lot to get through in one gulp, but virtually all of it is good to go. I got tired of pointing out the places (that I know you already know about) where we should do some post PR cleanup to try to avoid having e.g. ARM and POWER require very slightly different steps (due to flags setting and use of different information passing mechanisms to configure the relocation records) to construct the relocations themselves.

mstoodle · 2020-11-13T17:13:31Z

runtime/compiler/codegen/J9AheadOfTimeCompile.cpp

+         self()->traceRelocationOffsets(cursor, offsetSize, endOfCurrentRecord, orderedPair);
+         if (isVerbose)
+            {
+            traceMsg(self()->comp(), "\nTR_DataAddress: InlinedCallSite index = %d, Constant pool = %x, cpIndex = %d, offset = %x",


Shouldn't the flags also be printed by this code? Same comment for the MethodObject and ClassAddress relocations, I think.

mstoodle · 2020-11-13T18:38:37Z

jenkins test sanity all jdk8,jdk11

Add a new API that does not require the relocation record binary template header to already exist in order to determine the kind of the relocation record when creating it. This is necessary because the TR_RelocationRecord and subclasses are an API class that are used to both read and write data to and from, respectively, the binary template header. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_DiscontiguousSymbolFromManager relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_MethodObject relocation header information, and consolidate it in one canonical location. Note: TR_MethodObject is not used anywhere on P; in fact that code that wrote out the header data was incorrect as it wrote in the fields in the wrong order. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_ClassAddress relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_DataAddress relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_AbsoluteMethodAddressOrderedPair relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_FixedSequenceAddress relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_FixedSequenceAddress2 relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_ArrayCopyToc & TR_ArrayCopyHelper relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_BodyInfoAddressLoad relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_RamMethodSequence relocation header information, and consolidate it in one canonical location. Also, delete the unused TR_RamMethodSequenceReg relocation. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_ArbitraryClassAddress relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_GlobalValue relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_HCR relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_EmitClass relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_PicTrampolines relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

This relocation type is added in needsInvokeExactJ2IThunk. However, invokeExact isn't supported in AOT. Additionally, the code to actually do the relocation is missing, meaning this relo type is essentially dead code. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Remove the duplicated code that writes the TR_DebugCounter relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

mstoodle · 2020-11-16T21:16:43Z

jenkins test sanity win32 jdk8

dsouzai · 2020-11-16T21:18:24Z

@mstoodle I'm gonna push the requested review changes (once I've verified they build), so it might not be worth running any more tests until then since it'll kind of get swept under the rug.

mstoodle · 2020-11-16T21:23:17Z

ok thanks @dsouzai ....it was only one build that was resubmitted anyway because win32 cratered last time.

dsouzai · 2020-11-17T01:41:24Z

All tests passed; will push new changes up.

dsouzai · 2020-11-17T01:42:02Z

@mstoodle review changes made.

fjeremic · 2020-11-17T17:17:35Z

Jenkins test sanity all jdk8

dsouzai · 2020-11-17T18:45:38Z

Builds failed because of a whole bunch of jenkins exceptions

dsouzai · 2020-11-17T18:47:29Z

Jenkins test sanity plinux,plinuxxl,xlinux,xlinuxxl,osx jdk8

dsouzai · 2020-11-17T22:05:51Z

Looks like more jenkins failures..

dsouzai · 2020-11-18T21:06:14Z

Jenkins test sanity plinux,plinuxxl,xlinux,xlinuxxl jdk8

fjeremic · 2020-11-19T16:44:20Z

Finally success!

dsouzai added comp:jit comp:jit:aot labels Oct 16, 2020

dsouzai mentioned this pull request Oct 16, 2020

Consolidate writing of relocation record header #4803

Open

13 tasks

dsouzai marked this pull request as ready for review October 17, 2020 06:39

dsouzai commented Oct 19, 2020

View reviewed changes

mstoodle requested changes Nov 13, 2020

View reviewed changes

mstoodle reviewed Nov 13, 2020

View reviewed changes

mstoodle approved these changes Nov 13, 2020

View reviewed changes

dsouzai added 13 commits November 16, 2020 16:09

Clean up writing of common header

0f5a13f

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_DiscontiguousSymbolFromManager

90edafa

Remove the duplicated code that writes the TR_DiscontiguousSymbolFromManager relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_ClassAddress

a9df604

Remove the duplicated code that writes the TR_ClassAddress relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_DataAddress

a1772dd

Remove the duplicated code that writes the TR_DataAddress relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_AbsoluteMethodAddressOrderedPair

94103d0

Remove the duplicated code that writes the TR_AbsoluteMethodAddressOrderedPair relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_FixedSequenceAddress

f2a862c

Remove the duplicated code that writes the TR_FixedSequenceAddress relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_FixedSequenceAddress2

dd24720

Remove the duplicated code that writes the TR_FixedSequenceAddress2 relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_ArrayCopyToc & TR_ArrayCopyHelper

df8ffd3

Remove the duplicated code that writes the TR_ArrayCopyToc & TR_ArrayCopyHelper relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_BodyInfoAddressLoad

50cfd61

Remove the duplicated code that writes the TR_BodyInfoAddressLoad relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Delete TR_ConstantPoolOrderedPair

b191914

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

dsouzai added 8 commits November 16, 2020 16:09

Consolidate TR_ArbitraryClassAddress

cc9e8f6

Remove the duplicated code that writes the TR_ArbitraryClassAddress relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_GlobalValue

52fdd2d

Remove the duplicated code that writes the TR_GlobalValue relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_HCR

152a6c8

Remove the duplicated code that writes the TR_HCR relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_EmitClass

01a87a4

Remove the duplicated code that writes the TR_EmitClass relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_PicTrampolines

e64a0d4

Remove the duplicated code that writes the TR_PicTrampolines relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Consolidate TR_DebugCounter

e34f52a

Remove the duplicated code that writes the TR_DebugCounter relocation header information, and consolidate it in one canonical location. Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

Print flags for relo records that may set it

dd619b5

Signed-off-by: Irwin D'Souza <dsouzai.gh@gmail.com>

dsouzai force-pushed the aotConsolidateRecords branch from 9d64e26 to dd619b5 Compare November 17, 2020 01:41

fjeremic self-assigned this Nov 19, 2020

fjeremic merged commit 2981dcb into eclipse-openj9:master Nov 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate AOT Relocation Records #10916

Consolidate AOT Relocation Records #10916

dsouzai commented Oct 16, 2020 •

edited

Loading

dsouzai commented Oct 19, 2020

dsouzai Oct 19, 2020 •

edited

Loading

mstoodle Nov 13, 2020

dsouzai Nov 16, 2020

dsouzai Oct 19, 2020 •

edited

Loading

dsouzai Oct 19, 2020

dsouzai Oct 19, 2020

dsouzai commented Oct 27, 2020

dsouzai commented Nov 2, 2020

dsouzai commented Nov 9, 2020

mstoodle left a comment

mstoodle Nov 13, 2020

dsouzai Nov 16, 2020

mstoodle Nov 13, 2020

mstoodle left a comment

mstoodle Nov 13, 2020

mstoodle commented Nov 13, 2020

mstoodle commented Nov 16, 2020

dsouzai commented Nov 16, 2020

mstoodle commented Nov 16, 2020

dsouzai commented Nov 17, 2020

dsouzai commented Nov 17, 2020

fjeremic commented Nov 17, 2020

dsouzai commented Nov 17, 2020

dsouzai commented Nov 17, 2020

dsouzai commented Nov 17, 2020

dsouzai commented Nov 18, 2020

fjeremic commented Nov 19, 2020

Consolidate AOT Relocation Records #10916

Consolidate AOT Relocation Records #10916

Conversation

dsouzai commented Oct 16, 2020 • edited Loading

dsouzai commented Oct 19, 2020

dsouzai Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsouzai Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsouzai commented Oct 27, 2020

dsouzai commented Nov 2, 2020

dsouzai commented Nov 9, 2020

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

mstoodle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstoodle commented Nov 13, 2020

mstoodle commented Nov 16, 2020

dsouzai commented Nov 16, 2020

mstoodle commented Nov 16, 2020

dsouzai commented Nov 17, 2020

dsouzai commented Nov 17, 2020

fjeremic commented Nov 17, 2020

dsouzai commented Nov 17, 2020

dsouzai commented Nov 17, 2020

dsouzai commented Nov 17, 2020

dsouzai commented Nov 18, 2020

fjeremic commented Nov 19, 2020

dsouzai commented Oct 16, 2020 •

edited

Loading

dsouzai Oct 19, 2020 •

edited

Loading

dsouzai Oct 19, 2020 •

edited

Loading