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

Try setting superblock locals from an initialised exception handler #1084

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

carlosame
Copy link
Contributor

The patch to ClassFileWriter contains two hunks: the first one is the real fix, the second unmaps dead exception handlers (that generally happen with nested finally clauses) which otherwise may trigger JRE verifier errors. That second hunk could be removed if the performance hit is considered undesirable.

Closes #915.

Copy link
Contributor

@tuchida tuchida left a comment

Choose a reason for hiding this comment

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

Thank you very much for fixing it. Once this has been merged, #916 will be ready for review.

if (handlerSB.isInitialized()) {
break;
} else {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to continue?
Since it is if (sbStart > eteStart && and if (eteStart > sbStart &&, the result is the same, but it feels more readable to write the same code as here.

if (handlerSB.isInitialized()) {
locals = handlerSB.getLocals();
break;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I try to be efficient in a hot code path (the comment immediately above says so, and I agree), and while perhaps a smart JIT compiler would put a jump right there, I'd rather put it myself and make no assumptions about the JIT. :-)

@gbrail
Copy link
Collaborator

gbrail commented Nov 18, 2021

This is great -- thanks for tracking it down and fixing it!

@gbrail gbrail merged commit 734dcfa into mozilla:master Nov 18, 2021
@carlosame carlosame deleted the issue-915 branch November 18, 2021 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

java.lang.VerifyError in language/statements/try of test262
3 participants