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 #2403 VerifyError when implementing interface default methods #91

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jamesmudd
Contributor

jamesmudd commented Oct 2, 2017

There are 2 commits the first adds a failing test for this then the second resolves it.
I don't think this should be merged as is. In order for the test to compile the source level was increased to 1.8 when default methods were added, as we still support 1.7 this is not acceptable.

However the second commit fixes the issue by increasing the Opcode to v1.6 this appears to resolve the issue with the test and passes the regrtests. I don't understand why. Increasing it higher 1.7 or 1.8 cause failures.

The fix commit could be merged without the test which would improve usability with Java 8

@shoaniki

This comment has been minimized.

Show comment
Hide comment
@shoaniki

shoaniki Oct 10, 2017

The reason this works is that the change switches the generated class file version from 49 to 50, which happens to be the point at which the JVM starts using the up-to-date type-checking bytecode verifier instead of the old type-inferring one. Apparently the old verifier does not accept the code Jython emits when implementing a default method (specifically, the instruction that attempts to call the default implementation if the Python class did not override it), while the new verifier recognises that it is legal.

Having this fix merged would be very useful, if doing so without the test is an option. If the risk associated with a change in bytecode verifier is unpalatable, perhaps the opcode version could be made configurable?

shoaniki commented Oct 10, 2017

The reason this works is that the change switches the generated class file version from 49 to 50, which happens to be the point at which the JVM starts using the up-to-date type-checking bytecode verifier instead of the old type-inferring one. Apparently the old verifier does not accept the code Jython emits when implementing a default method (specifically, the instruction that attempts to call the default implementation if the Python class did not override it), while the new verifier recognises that it is legal.

Having this fix merged would be very useful, if doing so without the test is an option. If the risk associated with a change in bytecode verifier is unpalatable, perhaps the opcode version could be made configurable?

@jamesmudd

This comment has been minimized.

Show comment
Hide comment
@jamesmudd

jamesmudd Nov 5, 2017

Contributor

I have just pushed an update to this pull request that doesn't include the test and therefore the Java 8 requirement. I have been playing with this a bit more and I would suggest it should be merged, it fully fixes this problem in every case I can think of. As discussed above, this works as it changes the bytecode verifier used to the current one. I think the risk associated with this is small I can't think of any case where this would cause problem. However would be good if someone else could have a look at this to check.

Contributor

jamesmudd commented Nov 5, 2017

I have just pushed an update to this pull request that doesn't include the test and therefore the Java 8 requirement. I have been playing with this a bit more and I would suggest it should be merged, it fully fixes this problem in every case I can think of. As discussed above, this works as it changes the bytecode verifier used to the current one. I think the risk associated with this is small I can't think of any case where this would cause problem. However would be good if someone else could have a look at this to check.

@jeff5 jeff5 self-assigned this Jan 2, 2018

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5

jeff5 Jan 3, 2018

Member

Thanks @jamesmudd and @shoaniki. I'll try this out.

Member

jeff5 commented Jan 3, 2018

Thanks @jamesmudd and @shoaniki. I'll try this out.

@jeff5

This comment has been minimized.

Show comment
Hide comment
@jeff5
Member

jeff5 commented Jan 3, 2018

@jeff5 jeff5 closed this Jan 3, 2018

@jamesmudd jamesmudd deleted the jamesmudd:2403 branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment