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

Fix stackmap always marking ConstantDynamic as objects #8752

Merged
merged 1 commit into from
Mar 9, 2020

Conversation

fengxue-IS
Copy link
Contributor

  • Check Condy return type before marking stack slot

Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

@fengxue-IS fengxue-IS changed the title WIP: Fix stackmap always marking ConstantDynamic as objects Fix stackmap always marking ConstantDynamic as objects Mar 4, 2020
- Check Condy return type before marking stack slot

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@gacholio
Copy link
Contributor

gacholio commented Mar 5, 2020

I'd like @DanHeidinga to have a look before merge.

Copy link
Member

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

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

I'm OK with this.

An alternative approach would have been to switch on the J9DescriptionReturnTypeShift types and assert on the long/double cases but the corresponding bytecodes already have that assert in place. I think we're ok without adding it here

@DanHeidinga
Copy link
Member

@fengxue-IS It would be good to add tests for this case or identify a JTREG that already covers it

@gacholio
Copy link
Contributor

gacholio commented Mar 5, 2020

This issue will be difficult to reproduce with random tests - the condy value needs to be on the pending stack when a GC occurs.

Something like:

func(condyValue, gc())

where the gc() method forces some GCs before returning.

@gacholio
Copy link
Contributor

gacholio commented Mar 5, 2020

Should be easy enough to craft the above test and see that it crashes every time without the fix from this PR.

@fengxue-IS
Copy link
Contributor Author

I will look into adding a test case for this

@gacholio
Copy link
Contributor

gacholio commented Mar 5, 2020

@DanHeidinga Does the verifier need a similar change, or is it already there?

@DanHeidinga
Copy link
Member

@DanHeidinga Does the verifier need a similar change, or is it already there?

Verifier already does it correctly - see rtverify.c:
https://github.com/eclipse/openj9/blob/10d5686ffabe19806fc71bd3ea42ddbe9e99fdae/runtime/bcverify/rtverify.c#L720-L726

which pushes the ldc type using:
https://github.com/eclipse/openj9/blob/10d5686ffabe19806fc71bd3ea42ddbe9e99fdae/runtime/bcverify/vrfyhelp.c#L420-L425

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk11

@DanHeidinga DanHeidinga self-assigned this Mar 6, 2020
@fengxue-IS
Copy link
Contributor Author

@DanHeidinga can you please restart the PR build, looks like it failed due to network issue

[2020-03-06T18:26:29.903Z] Cannot contact rh7-390-4: java.lang.InterruptedException
[2020-03-06T18:35:50.499Z] wrapper script does not seem to be touching the log file in /home/jenkins/workspace/Build_JDK11_s390x_linux_Personal@tmp/durable-c2ff9402
[2020-03-06T18:35:50.499Z] (JENKINS-48300: if on an extremely laggy filesystem, consider -Dorg.jenkinsci.plugins.durabletask.BourneShellScript.HEARTBEAT_CHECK_INTERVAL=86400)

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk11

@DanHeidinga
Copy link
Member

Jenkins test sanity zlinux jdk11

@DanHeidinga
Copy link
Member

ported to 0.19 release in #877

@DanHeidinga DanHeidinga merged commit b4d5d32 into eclipse-openj9:master Mar 9, 2020
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.

None yet

4 participants