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

Update JNI Target relocation record to have offset to the reloLocation. #6326

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

dsouzai
Copy link
Member

@dsouzai dsouzai commented Feb 2, 2022

On x86, the JNI Target relocation puts the incorrect address when
generating a Runtime Assumption. However, the address that the
relocation infra itself needs to patch is correct. Thus, the relo record
has to hold not just the location to patch but also the location to
register the assumption against. This is the same on most platforms, but
different on x86.

eclipse-openj9/openj9#14421 depends on this change; OpenJ9 will be broken without a coordinated merge.

Copy link
Contributor

@mstoodle mstoodle left a comment

Choose a reason for hiding this comment

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

the changes look ok to me, given what's in the OpenJ9 pull request.

{
uint8_t *startOfInstruction = self()->getBinaryEncoding();
uint8_t *startOfImmediate = cursor;
intptr_t diff = reinterpret_cast<intptr_t>(startOfImmediate) - reinterpret_cast<intptr_t>(startOfInstruction);
Copy link
Contributor

Choose a reason for hiding this comment

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

did you want to add the < 256 assertion here as well as in OpenJ9 when the actual relocation record is created? I could understand if not (you could view as a requirement imposed by a downstream component) but given it's JNI and likely not to be reused by anything else, maybe catching a problem here is desirable? I'll leave it as optional, because I can see it both ways.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about it, but I think I mainly didn't add it here it so that there if, for whatever reason (perhaps on a different platform) the offset had to be increased to fit in a bigger type, then there would have to be an OMR PR just to update the assert. As such I figured it's probably best to just have the assert close to where the data in the relo header is actually stored.

@mstoodle
Copy link
Contributor

mstoodle commented Feb 3, 2022

jenkins build all

@mstoodle
Copy link
Contributor

mstoodle commented Feb 3, 2022

hm, guess I could have only tested x86 and z...oh well :(

@dsouzai
Copy link
Member Author

dsouzai commented Feb 3, 2022

Windows build error is because of a warning that's treated as an error, namely:

[2022-02-03T14:28:09.333Z]          C:\jenkins\workspace\PullRequest-win_x86-64\Build\compiler\x\codegen\X86BinaryEncoding.cpp(1103): warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized [C:\jenkins\workspace\PullRequest-win_x86-64\Build\build\jitbuilder\jitbuilder.vcxproj]

This is because of the line

TR_RelocationRecordInformation *info = new (comp->trHeapMemory()) TR_RelocationRecordInformation();

MSVC does not like the fact that TR_RelocationRecordInformation is a POD object and we're calling the empty constructor on it which will be default initialized. I guess I have to use the other way of allocating this object, namely:

TR_RelocationRecordInformation *info = 
   reinterpret_cast<TR_RelocationRecordInformation *>(
      comp->trMemory()->allocateHeapMemory(sizeof(TR_RelocationRecordInformation));

On x86, the JNI Target relocation puts the incorrect address when
generating a Runtime Assumption. However, the address that the
relocation infra itself needs to patch is correct. Thus, the relo record
has to hold not just the location to patch but also the location to
register the assumption against. This is the same on most platforms, but
different on x86.

Signed-off-by: Irwin D'Souza <[email protected]>
@mstoodle
Copy link
Contributor

mstoodle commented Feb 3, 2022

I think you could also just new it without the trailing () if it's a POD, but using allocateHeapMemory() certainly works.

@mstoodle
Copy link
Contributor

mstoodle commented Feb 3, 2022

jenkins build all

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

Successfully merging this pull request may close these issues.

2 participants