-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8359920: Use names for frame types in stackmaps #25870
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into |
@coleenp This change is no longer ready for integration - check the PR body for details. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
SAME_FRAME = 64, | ||
SAME_LOCALS_1_STACK_ITEM_FRAME = 128, | ||
SAME_LOCALS_1_STACK_ITEM_EXTENDED = 247, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find these definitions a little confusing. SAME_FRAME is actually 0-63, with SAME_LOCALS_1_STACK_ITEM_FRAME being 64-127. Given many of these frame types imply tag ranges it may be clearer to define enum's for the start and end of ranges as applicable eg.
enum {
SAME_FRAME_START = 0,
SAME_FRAME_END = 63,
SAME_LOCALS_1_STACK_ITEM_FRAME_START = 64,
SAME_LOCALS_1_STACK_ITEM_FRAME_END = 127,
RESERVED_START = 128,
RESERVED_END = 246,
SAME_LOCALS_1_STACK_ITEM_EXTENDED = 247,
CHOP_FRAME_START = 248,
CHOP_FRAME_END = 250,
SAME_FRAME_EXTENDED = 251,
APPEND_FRAME_START = 252,
APPEND_FRAME_END = 254,
FULL_FRAME = 255
}
and then adjust the code usage as appropriate e.g.
if (frame_type <= SAME_FRAME_END) {
...
if (frame_type <= SAME_LOCALS_1_STACK_ITEM_FRAME_END) {
if (_first) {
offset = frame_type - SAME_LOCALS_1_STACK_ITEM_FRAME_START;
...
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't really up for a big rewrite but having the complete set of names would be really good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
I was also thinking about frame_type
ranges but no ideas on any improvements.
Co-authored-by: David Holmes <[email protected]>
This uses names for frame types for stackmaps in the verifier and redefinition.
Tested with tier1-7.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25870/head:pull/25870
$ git checkout pull/25870
Update a local copy of the PR:
$ git checkout pull/25870
$ git pull https://git.openjdk.org/jdk.git pull/25870/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 25870
View PR using the GUI difftool:
$ git pr show -t 25870
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25870.diff
Using Webrev
Link to Webrev Comment