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.

Sign up for GitHub

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

Jump to bottom

Don't differenciate Q from L types in verifier #17236

Merged
merged 1 commit into from May 4, 2023
Merged

Don't differenciate Q from L types in verifier #17236

merged 1 commit into from May 4, 2023

Conversation

ehrenjulzert
Copy link

@ehrenjulzert ehrenjulzert commented Apr 20, 2023 •
edited

In #15053 code was added to ensure Q types could
not be null, and to print different error
messages for Q types. However, according to the
new spec the verifier no longer needs to make
these distinctions

@ehrenjulzert
Copy link
Author

@hangshao0

@hangshao0
Copy link
Contributor

hangshao0 commented Apr 20, 2023 •
edited

Can you add a comment in the commit message mentioning the original PR and this is due to new spec that the null checks are not needed anymore ?

@ehrenjulzert
Copy link
Author

Okay, added in fe07095

@hangshao0
Copy link
Contributor

Can you review this ? @ChengJin01

This is for the the new spec which says:
The verification type system does not enforce null restrictions, and makes no distinction between standard reference types (LFoo;) and null-restricted reference types (QFoo;).
This is the old spec.

@ChengJin01
Copy link
Contributor

ChengJin01 commented Apr 20, 2023 •
edited

This is for the the new spec which says:
The verification type system does not enforce null restrictions, and makes no distinction between standard reference types (LFoo;) and null-restricted reference types (QFoo;).
This is the old spec.

@ehrenjulzert , you might need to remove all changes with convertClassNameToStackMapType related to the Qtypes in the old changes at https://github.com/eclipse-openj9/openj9/pull/15053/files given there is no distinction between standard reference types (LFoo;) and null-restricted reference types (QFoo;) in the latest Spec.

@ehrenjulzert ehrenjulzert changed the title Remove null checks for Q types in verifier Don't differenciate Q from L types in verifier Apr 24, 2023
@ehrenjulzert
Copy link
Author

I've removed all the convertClassNameToStackMapType changes related to QTypes and changed the error messages to no longer distinguish between Q types and L types.

ChengJin01
ChengJin01 approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@ChengJin01 ChengJin01 left a comment •
edited

Choose a reason for hiding this comment

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

Approved the changes given its intention is to restore everything back to the original code without Valhalla.

@ChengJin01
Copy link
Contributor

FYI: @tajila, @DanHeidinga

@tajila
Copy link
Contributor

tajila commented Apr 24, 2023

@ehrenjulzert can you post a link to the PR that added the changes

@hangshao0 hangshao0 added comp:vm project:valhalla Used to track Project Valhalla related work labels Apr 24, 2023
hangshao0
hangshao0 approved these changes Apr 24, 2023
View reviewed changes
Copy link
Contributor

@hangshao0 hangshao0 left a comment

Choose a reason for hiding this comment

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

LGTM

@ehrenjulzert
Copy link
Author

can you post a link to the PR that added the changes

Changes were added in #15053

@hangshao0 hangshao0 requested a review from tajila May 2, 2023 18:30
@tajila
Copy link
Contributor

tajila commented May 2, 2023

@ehrenjulzert let me know when this is rebased and Ill launch the tests

Don't differenciate Q from L types in verifier
f473893 
In eclipse-openj9#15053 code was added to ensure Q types could
not be null, and to print different error
messages for Q types. However, according to the
new spec the verifier no longer needs to make
these distinctions

Signed-off-by: Ehren Julien-Neitzert <ehren.julien-neitzert@ibm.com>
@ehrenjulzert ehrenjulzert force-pushed the remove_null_check_in_verifier branch from 24748bb to f473893 Compare May 2, 2023 21:10
@ehrenjulzert
Copy link
Author

Okay I just rebased

@tajila
Copy link
Contributor

tajila commented May 2, 2023

jenkins test sanity win jdk8

@tajila
Copy link
Contributor

tajila commented May 2, 2023

Jenkins test sanity xlinuxvalst jdknext

@tajila
Copy link
Contributor

tajila commented May 2, 2023

Jenkins test sanity,extended plinuxval jdknext

@tajila
Copy link
Contributor

tajila commented May 3, 2023

@JasonFengJ9 is the VT build broken? there are quite a few failures

@JasonFengJ9
Copy link
Member

@tajila The latest nightly build was green.

Those test failures are known JDK21 issues:

@tajila tajila merged commit 74d31a5 into eclipse-openj9:master May 4, 2023
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangshao0 hangshao0 hangshao0 approved these changes

@ChengJin01 ChengJin01 ChengJin01 approved these changes

@tajila tajila Awaiting requested review from tajila

Assignees
No one assigned
Labels
comp:vm project:valhalla Used to track Project Valhalla related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ehrenjulzert @hangshao0 @ChengJin01 @tajila @JasonFengJ9