Added check to accept version 61, throw error for higher

[Mahmoud-Khawaja]

No description provided.

[Mahmoud-Khawaja]

Hello @cyrille-artho,
i noticed that there is no explicit check for the major version. so if we tried to parse a class of any version jpf will accept it leading to potentail errors. if we checked the class file versions from here : Java class file Versions we can find that java 17 works with class file version 61.0. so i added a check to prevent any kind of error later.

[cyrille-artho]

Hi Mahmoud,
Thank you for this PR. Can you please add a test that shows that the new check is working? You might have to add a .class file that you compiled offline with Java 21, which then causes the version check to fail. The unit test should then expect a failure of the version check in order to pass.

[Mahmoud-Khawaja]

im thinking of creating a FileVersionTest.java that has two tests on to test the supported and the other to test the unsupported version. then compile one time with java 21 and java 17 that should create two .class files
( fileVersionTest_java21.class,fileVersionTest_java17.class) if we disassembled .class for both one should have major version of 65 beacause we are compiling with java 21 and the other with major version of 61. so this what you mean?

[cyrille-artho]

Yes, that is what I mean. It may be difficult to compile one file with two versions of javac, as the typical workflow of Gradle-supported builds is designed for one compilation per source file. For us, either way will work: dual compilation of one file or addition of a pre-compiled .class file to the repository as data.

[cyrille-artho]

Together with a suitable test, try to make this PR against master, which is currently on Java 11.
This functionality is useful for that version, too, and we have 100 % passing tests on master, which makes it easier to review and merge new test cases and functionality.

[Mahmoud-Khawaja]

Great! I'm currently working on this. I will commit the changes for you to review once I finish.

[Mahmoud-Khawaja]

Hello @cyrille-artho ,
i've just implemented the unit tests. as you can see if we uncommented testUnsupportedVersionJava21() and tried to run the test with java 17 it will throw ClassParseException but it passes for testSupportedVersionJava17()

[Mahmoud-Khawaja]

i just need to verify if my approach is correct. i created two classes TestClassJava17 and TestClassJava21 complied both to get .class files and implemented the tests. but im not sure if there is a better way to handle testUnsupportedVersionJava21 in a better way but i guess it can be working for now.

please check and let me know if i have to do the same on the master branch

[cyrille-artho]

Thank you for your work. Please look at/change two things:

1. Your PR may not be up to date with all changes; a lot of unit tests currently fail instead of the expect three unit tests that reflect features that indeed don't work yet on java-17.
2. Can we remove src/tests/gov/nasa/jpf/jvm/resources/TestClassJava17.class and instead use the version that is compiled as part of the build process? I understand that keeping the version built with Java 21 is the more pragmatic choice (rather than additional build rules for one file with a different version of Java), but this branch uses Java 17, so the source file should be built as expected.

[cyrille-artho]

Another item to address regarding the tests.

[cyrille-artho]

Please enable this test and ensure it throws an exception. The test looks good; what was the reason for commenting it out? Perhaps JPF loads the version of the class file that was compiled by the build process rather than the one in src/tests, which would explain that this test does not throw an exception and thus fails?
In that case, try to find a way to make your manually uploaded class file take precedence in the class loader.

[Mahmoud-Khawaja]

Hello @cyrille-artho,

thanks for response, you're correct about removing TestClassJava17.class. but for now I've been struggling with a problem for hours. i have updated the tests case and i think it should be working correctly but as you can see in testUnsupportedVersionJava21() this should throw ClassParseException since we are parsing a compiled file of class version 65 but im getting AssertionError after alot of debugging i tried to print the major version for classes using getMajorVersion() and for both tests it was 61 but the testSupportedVersionJava17 passes which is okay in that case.
but the other not throwing the expected error. even thoug i used javap -verbose to check the byte code and its correct for both .class files. one has class version 61 and the other is 65.

after debugging again i tried to removed TestClassJava17.java and TestClassJava17.java and kept the compiled files. it throwed an IOException which means we are not even loading the .class files. its a strange behaviour and im not sure what is it but when i got TestClassJava17.java and TestClassJava21.java back again and removed the compiled files. i got the same problem testSupportedVersionJava17 passed and testSupportedVersionJava21 throwed AssertionError

so i think the problem is with the Class loading itself and im not sure why it can't read the compiled files
I kept TestClassJava17.class to analyze the behavior for now, but I'm stuck with this problem.

can you please help me with this?

[cyrille-artho]

I don't have time to look into this right now, but this sounds like it's very likely a classpath issue. Note that JPF's classpath might differ from the host JVM's classpath. You can try jpf -show to see its settings. By default, JPF would not look in src for class files, so it is probably the case that it does not use these class files there. You can try an additional argument +classpath=... in the test.
Unfortunately, like with the JVM, there are no log or debug messages for classpath entries that do not match your file system, so debugging incorrect path names can be tedious.

[Mahmoud-Khawaja]

got it! I will investigate this, and maybe I can find the error

thanks!

[Mahmoud-Khawaja]

Hello @cyrille-artho,
after debugging and searching for gradle standard behavior, i found out that .class files should be in a resources directory so i updated the build.gradle file to include .class files in the class path. and now it works correctly.
let me know if anything needs to be modified. thanks a lot for your help!

[cyrille-artho]

Hi,
Please check whether your PR is missing some recent changes, as it results in 12 failing unit tests in the CI test. Perhaps it does not include your fixes for certain record-related features?
You might also consider issuing a PR to master, with the test cases targeting earlier versions of Java instead (Java 11 being the supported version, Java 17 being "too new" on that branch).
This might be easier, as it would avoid the problem of some tests still failing in the CI build, and we would expect 100 % passing tests to merge a PR there.

[Mahmoud-Khawaja]

yes, i forgot to add the .class now i think they will pass.

[Mahmoud-Khawaja]

regarding the other failing tests i will investigate and see why they fail