-
Notifications
You must be signed in to change notification settings - Fork 343
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
JVMClassInfo$Initializer.setBootstrapMethod ArrayIndexOutOfBoundsException #53
Comments
It seems that support for this attribute in jpf-core is specific to lambda expressions. The current code does not even check the number of arguments and whether a given bootstrap method is a lamba expression. |
This affects method public void setBootstrapMethod (ClassFile cf, Object tag, int idx, int refKind, String cls, String mth, String descriptor, int[] cpArgs) in JVMClassInfo.java. cpArgs is bmArgs in ClassFile.java, which calls this function. The current code handles just a fixed number of arguments, and uses them in a specific way, to know how to use the lambda expression. To make the code robust against crashes, the code should use the existing functionality if the length of cpArgs == 3, and otherwise just store that array but not evaluate it further. |
It seems that to resolve this properly, we also need to look at ClassInfo.java: The first case is the one that's handled correctly by the current specific code in JPF: #883 REF_invokeStatic java/lang/invoke/LambdaMetafactory.metafactory This deals with lamdba expressions, which have a very specific structure. Perhaps arg 0 is always java/lang/Object, which is why it was ignored. I suggest to check the u2 bootstrap_method_ref entry in the bytecode. This is passed as "refKind" to setBootstrapMethod, and currently unchecked. If it matches the type above, we can execute the current code. Otherwise, we should store the cpArgs information and NOT evaluate the specific arguments. We may still have to evaluate these arguments in a different way, e.g.: #284 REF_invokeStatic java/lang/invoke/StringConcatFactory.makeConcatWithConstants:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite; This creates a method that appends a particular string. If we do not cover this case (and only the lambda meta factory case), I hope we get rid of ArrayBounds problems, but we may get an incorrect output in some cases; we can fix this in a separate step. |
I understand this better now: Lambda expressions get compiled into specific cases of bootstrap methods. Bootstrap methods are templates of methods, which are generated at run time, when the expression is encountered for the first time. The code in setLambdaDirectCallCode (JVMClassInfo) parses the necessary type descriptors and creates a new method that loads the arguments and calls the actual method that is referred to in the constant pool. As I wrote in the new comment on the issue tracker, the first step would be to check whether we deal with the specific case that was (so far) the only case in Java 8. After that, we can try to generalize the code. I hope we can see from the examples why arg0 was not used before, and how to update the code to find the right type descriptors for a different number of arguments. |
It seems that input parameters start deliberately at 1; see the first loop (from 0) and the second one (from 1) in JVMClassInfo.setLambdaDirectCallCode:
|
The value of the As stated in below, jpf-core/src/main/gov/nasa/jpf/vm/BootstrapMethodInfo.java Lines 23 to 28 in 18a0c42
As stated in the SO post
We'll also have different descriptors for the bootstrap methods LambdaMetafactory.metafactory and LambdaMetafactory.altMetafactory. clsName = "java/lang/invoke/LambdaMetafactory" clsName = "java/lang/invoke/LambdaMetafactory" So why not just check the value of Working examples:tag = {JVMClassInfo@1311} "ClassInfo[name=java.lang.CharSequence]"
tag = {JVMClassInfo@1409} "ClassInfo[name=java.util.Map$Entry]"
Failing cases:tag = {JVMClassInfo@1714} "ClassInfo[name=java.lang.Class]"
|
Let's try adding a check if "clsName" and see if this makes some of the tests pass. After that, let's look at the remaining cases.
Documentation: https://docs.oracle.com/javase/10/docs/api/java/lang/invoke/package-summary.html |
|
I added a clasName check in the setBootstrapMethod method like the following:
After implementing the check, ArrayIndexOutOfBoundsException has disappeared . There's also a significant improvement in the test results:
Before implementing clsName check:
After implementing clsName check:
In the After case, I added the suggested StringAppendTest in to StringTest.java. Surprisingly that test also do pass. I'm currently not sure why. I also test the same changes in the master branch with Java 8 Master branch with Java 8In Java 8 all the test cases pass, which somewhat verifies that the existing functionalities are not affected by the extra clsName check. Travis log: https://travis-ci.org/gayanW/jpf-core/builds/395538672 |
It seems to me that verifyNoPropertyViolation() in the tests is what initiates JPF which cause methods to run through JPF. If not the tests are invoked directly. Test 1This test pass with no errors printed in the console.
Test 2
The test above also passes, but print out the following stacktrace.
It looks like that the test is still executed.without resolving the ClassInfos for the model classes as it fails when trying to resolve the StartupSystemClassInfos. May be we will not be able to assert:
.. before we could fix the system class loading issue. Let me know if you want me to proceed with just the step 1? |
Good work on the initial fix, which solves a lot of problems.
Test 1 executes on the host JVM; it therefore does not use JPF. It is
therefore expected to pass.
Test 2 actually uses JPF. It is good that it fails, because it is a very
small test that makes it easier to debug what exactly needs to be supported.
It would be great if we could support the remaining cases without having to
implement a lot of diverse bootstrap methods by hand, with the generic
method that I mentioned earlier. If this does not work, we'll have to
implement the 10 or so cases that are currently part of the invoke package.
Please continue to look into what exactly fails in test 2, and how it could
be fixed.
Gayan Weerakutti wrote:
… It seems to me that verifyNoPropertyViolation() in the tests is what
initiates JPF which run methods through JPF. Unless the tests are invoked
directly.
Test 1
This test pass with no errors printed in the console.
***@***.*** public void testConcat() { String str = new String("Hello, "); String
out = str + "World!"; assert out.equals("Hello, World!"); } |
Test 2
***@***.*** public void testConcat() { if (verifyNoPropertyViolation()) { String
str = new String("Hello, "); String out = str + "World!"; assert
out.equals("Hello, World!"); } } |
The test above also passes, but print out the following stacktrace.
|[junit] Running gov.nasa.jpf.test.java.lang.StringTest [junit] Tests run:
1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.044 sec [junit]
Running gov.nasa.jpf.test.java.lang.StringTest [junit] running jpf with
args: [junit] gov.nasa.jpf.vm.ClassInfoException: class not found:
java.io.Serializable [junit] at
gov.nasa.jpf.vm.ClassLoaderInfo.getResolvedClassInfo(ClassLoaderInfo.java:363)
[junit] at
gov.nasa.jpf.vm.SystemClassLoaderInfo.getResolvedClassInfo(SystemClassLoaderInfo.java:147)
[junit] at
gov.nasa.jpf.vm.SystemClassLoaderInfo.loadClass(SystemClassLoaderInfo.java:182)
[junit] at
gov.nasa.jpf.vm.ClassInfo.resolveReferencedClass(ClassInfo.java:2414)
[junit] at gov.nasa.jpf.vm.ClassInfo.loadInterfaces(ClassInfo.java:2366)
[junit] at gov.nasa.jpf.vm.ClassInfo.resolveClass(ClassInfo.java:2383)
[junit] at gov.nasa.jpf.vm.ClassInfo.resolveAndLink(ClassInfo.java:548)
[junit] at gov.nasa.jpf.jvm.JVMClassInfo.<init>(JVMClassInfo.java:624)
[junit] at
gov.nasa.jpf.jvm.JVMClassFileContainer$JVMClassFileMatch.createClassInfo(JVMClassFileContainer.java:58)
[junit] at
gov.nasa.jpf.jvm.JVMClassFileContainer$JVMClassFileMatch.createClassInfo(JVMClassFileContainer.java:33)
[junit] at
gov.nasa.jpf.vm.ClassLoaderInfo.getResolvedClassInfo(ClassLoaderInfo.java:353)
[junit] at
gov.nasa.jpf.vm.SystemClassLoaderInfo.getResolvedClassInfo(SystemClassLoaderInfo.java:147)
[junit] at gov.nasa.jpf.vm.VM.getStartupSystemClassInfos(VM.java:445)
[junit] at gov.nasa.jpf.vm.VM.initializeMainThread(VM.java:564) [junit] at
gov.nasa.jpf.vm.SingleProcessVM.initialize(SingleProcessVM.java:130)
[junit] at gov.nasa.jpf.JPF.run(JPF.java:611) [junit] at
gov.nasa.jpf.util.test.TestJPF.createAndRunJPF(TestJPF.java:675) [junit] at
gov.nasa.jpf.util.test.TestJPF.noPropertyViolation(TestJPF.java:806)
[junit] at
gov.nasa.jpf.util.test.TestJPF.verifyNoPropertyViolation(TestJPF.java:830)
[junit] at
gov.nasa.jpf.test.java.lang.StringTest.testConcat(StringTest.java:307) |
It looks like that the test is still executed.without resolving the
ClassInfos for the model classes as it fails when trying to resolve the
StartupSystemClassInfos.
May be we will not be able to assert:
Add the simple example (StringAppendTest) as a new test.
Please add this case first and try to ensure it fails. You may have to
"print" statement to an assertion that compares the result against the
expected value.
.. before we could fix the system class loading issue.
Let me know if you want me to proceed with just the step 1?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#53 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG9eRzztEIBemqTtXTNjt1Gtnj6qm6l9ks5t_bougaJpZM4UIKYz>.
--
Regards,
Cyrille Artho - http://artho.com/
We are all like soldiers,
crouching behind the fortifications we have raised.
-- Steven Erikson, "Midnight Tides"
|
Test 2 also passes. But with the above stacktrace. What I meant was that we won't be able to reproduce the case 2, without fixing the class loading issue first. As shown in the stacktrace, Test 2 fails to resolve any ClassInfos, yet it continues with the assertion |
If the test fails with a stack trace from JPF itself, then the assertion is
never executed. Hence there is no assertion failure; the execution stops
before the assertion is reached.
The challenge is now in finding where/when exactly JPF throws
ClassInfoException (which class(es) are loaded, which class is being
loaded) and trying to figure out how to fix this.
Gayan Weerakutti wrote:
… Test 2 actually uses JPF. It is good that it fails, because it is a
very small test that makes it easier to debug what exactly needs to be
supported.
Test 2 also passes. But with the above stacktrace. What I meant was that we
won't be able to reproduce the case 2, without fixing the class loading
issue first. As shown in the stacktrace Test 2 fails to resolve any
ClassInfos, yet it continues with the assertion |assert out.equals("Hello,
World!")|, hence passing the test.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#53 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG9eRzBnU3p35_WSx0JCO80wi80ug2btks5t_gMcgaJpZM4UIKYz>.
--
Regards,
Cyrille Artho - http://artho.com/
We are all like soldiers,
crouching behind the fortifications we have raised.
-- Steven Erikson, "Midnight Tides"
|
ClassInfoException is related to the sun.boot.class.path issue and is not directly related to this one. It will be good to create a new issue for it. Once that is being fixed, StringAppend test inhere should also fail. I'll create a new issue for it with detailed information, so we could come up with a solution. I expects it have more implementation changes in addition to be able to just retrive the boot class path. (Like changes to generated class URIs) |
Yes, please work on that and create a new issue.
It's good to get an overview of all pending problems (at least as far as we
can tell now) before we try to fix them one by one.
Gayan Weerakutti wrote:
… ClassInfoException is related to the sun.boot.class.path
<https://stackoverflow.com/q/50451536/3647002> issue and is not directly
related to this one. It will be good to create a new issue for it. Once
that is being fixed, StringAppend test inhere should also fail.
I'll create a new issue for it with detailed information, so we could come
up with a solution. I expects it have more implementation changes in
addition to be able to just retrive the boot class path. (Like changes to
generated class URIs)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#53 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AG9eRy4C8H_SiYs6eW4ACIEbsH3_sl2Fks5t_zdqgaJpZM4UIKYz>.
--
Regards,
Cyrille Artho - http://artho.com/
None are more hopelessly enslaved than those
who falsely believe they are free.
-- Goethe
|
It is necessary to construct and handle BootstrapMethodInfo for the special case (StringConcatFactory#makeConcatWithConstants) for the tests with string concatenation to pass. We can construct BootstrapMethodInfo using a constructor similar to BootstrapMethodInfo(JVMClassInfo.this, cpArgs) without evaluating the cpArgs further as discussed before, but it'll still fail in JVMClassInfo#setLambdaDirectCallCode as setLambdaDirectCallCode method expects a BootstrapMethodInfo with a lambdaBody and a samDescriptor.
I'm still not sure how to evaluate the lambdaBody, and samDescriptor for the special case (with single bmArg). Failing case
Working case
FYI: evaluated values for an instance of the working case:
|
Resolved, at least for parsing the bytecode in several cases we tested, as part of PR#210. |
The ClassFile parser seems to find just single bootstrap method argument, whereas in JVMClassInfo$Initializer.setBootstrapMethod it expects an element with index 1 in the bmArgs array.
jpf-core/src/main/gov/nasa/jpf/jvm/ClassFile.java
Lines 1386 to 1422 in 18a0c42
jpf-core/src/main/gov/nasa/jpf/jvm/JVMClassInfo.java
Lines 91 to 93 in 18a0c42
This seems to happen when trying to create ClassInfo for startupSystemClasses. So for instance
sysCl.getResolvedClassInfo("java.lang.Class")
would throw java.lang.ArrayIndexOutOfBoundsException' exception.sysCl.getResolvedClassInfo("java.lang.Object")
however resolves fine.References
Saved diff of Class.class for Java 8 and 10:
https://www.diffchecker.com/0A0yIEXK
Travis log:
https://travis-ci.org/javapathfinder/jpf-core/builds/381994510#L3057-L3083
The text was updated successfully, but these errors were encountered: