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
JDK-8288824: [arm32] Display isetstate in register output #9223
Conversation
/label: hotspot |
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
Gentle ping |
case 1: st->print_cr("Thumb"); break; | ||
case 2: st->print_cr("Jazelle"); break; | ||
case 3: st->print_cr("ThumbEE"); break; | ||
default: ShouldNotReachHere(); |
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.
Should we print here something like "undefined" rather than abort VM?
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.
Hi Dmitry,
I think this cannot happen, since isetstate is the combination of two bits (see line 451 ff). So an assert would be correct here, since this can only fire if we break that assumption by changing isetstate with a future patch.
But I am unemotional. If you prefer not to assert, I can use "undefined".
Cheers, Thomas
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.
Thank you for the explanation. I'm OK with current changes.
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.
Thanks Dmitry!
@tstuefe This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 185 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@kristylee88 are you a real person or a bot? I only see a number of comment-less approvals and no indication that you actually read the sources. |
@snazarkin could you take a look? Its a small patch, and there are not many arm32 reviewers around it seems. Thanks! |
@tstuefe Good idea, thank you. LGTM |
Thanks @dsamersoff and @snazarkin . /integrate |
Going to push as commit 75c0a5b.
Your commit was automatically rebased without conflicts. |
When analyzing JDK-8288719, to know the current isetstate was useful. It would be nice if that were printed clearly in the register output to save some mental cycles parsing CPSR.
Looks like this:
I refrained from parsing more information from the CPSR because I did not want to blow up the patch. ISETSTATE proved to be useful. More information can be added if needed.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9223/head:pull/9223
$ git checkout pull/9223
Update a local copy of the PR:
$ git checkout pull/9223
$ git pull https://git.openjdk.org/jdk pull/9223/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9223
View PR using the GUI difftool:
$ git pr show -t 9223
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9223.diff