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

Lang.JVM.Codebase: experimental support for loading JIMAGE files #638

Merged
merged 2 commits into from
Feb 5, 2021

Conversation

RyanGlScott
Copy link
Contributor

This allows crucible-jvm to deal with JDK 9 or later, which packages its standard library not in a JAR file, but in a JIMAGE file. Extracting .class files from JIMAGE files proves to be surprisingly tricky, and I've carefully documented the intricacies of doing so in Note [Loading classes from JIMAGE files] in Lang.JVM.Codebase. This is part of a fix for GaloisInc/saw-script#861.

…base

`Lang.JVM.Codebase` has a `CodebaseState` data type that is used within an
`IORef` in `Codebase`. However, only two of the four fields of `CodebaseState`
are ever updated with `writeIORef`, so it doesn't make much sense to keep the
other two fields in an `IORef`. This patch moves them from `CodebaseState` to
`Codebase` to make this more obvious.

This is purely a refactoring and has no user-visible changes in behavior.
@RyanGlScott
Copy link
Contributor Author

I'm marking this as a draft for now since there appears to be a lurking bug that is uncovered when running crucible-jvm's test suite with JDK 11:

$ make clean && ./test.sh 
rm -f *.class
javac -g Str.java
Compiled from "Str.java"
class Str {
  Str();
    Code:
       0: aload_0
       1: invokespecial #1                  // Method java/lang/Object."<init>":()V
       4: return

  public static void main(java.lang.String[]);
    Code:
       0: ldc           #2                  // String 1234567890
       2: astore_1
       3: new           #3                  // class java/lang/StringBuffer
       6: dup
       7: aload_1
       8: invokespecial #4                  // Method java/lang/StringBuffer."<init>":(Ljava/lang/String;)V
      11: astore_2
      12: aload_2
      13: iconst_2
      14: ldc           #5                  // String abcd
      16: invokevirtual #6                  // Method java/lang/StringBuffer.insert:(ILjava/lang/String;)Ljava/lang/StringBuffer;
      19: pop
      20: getstatic     #7                  // Field java/lang/System.out:Ljava/io/PrintStream;
      23: aload_2
      24: invokevirtual #8                  // Method java/io/PrintStream.println:(Ljava/lang/Object;)V
      27: return
}
[Crux] Checking "Str.java"
starting executeCrucibleJVM
[Crux] Index 10 is not a method reference.
CallStack (from HasCallStack):
  error, called at src/Language/JVM/Parser.hs:387:12 in jvm-parser-0.3.0.0-inplace:Language.JVM.Parser

Amazingly, I haven't come across this before in the SAW test cases that I've ran, so I'm glad I thought to try this. I still need to figure out why this happens, however.

This adds basic functionality for `crucible-jvm` to deal with JDK 9 or later,
which packages its standard library not in a JAR file, but in a JIMAGE file.
Extracting `.class` files from JIMAGE files proves to be surprisingly tricky,
and I've carefully documented the intricacies of doing so in
`Note [Loading classes from JIMAGE files]` in `Lang.JVM.Codebase`.
This is part of a fix for GaloisInc/saw-script#861.

In general, support for JDK 9 or later is still experimental, as there are
still unresolved bugs to diagnose. See #641.
@RyanGlScott RyanGlScott force-pushed the support-java-9-or-later branch from bfa8c30 to f839892 Compare February 5, 2021 22:00
@RyanGlScott RyanGlScott changed the title Lang.JVM.Codebase: support loading JIMAGE files Lang.JVM.Codebase: experimental support for loading JIMAGE files Feb 5, 2021
@RyanGlScott RyanGlScott marked this pull request as ready for review February 5, 2021 22:00
@RyanGlScott
Copy link
Contributor Author

Per the discussion in GaloisInc/saw-script#861 (comment), I've unmarked this as a draft. The crucible-jvm tool now prints out a warning if you are using JDK 9 or later indicating that support for modern JDKs is still experimental, and the warning includes a pointer to #641. This warning should serve as a template for SAW.

Copy link
Contributor

@atomb atomb left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for including lots of design rationale documentation!

@RyanGlScott RyanGlScott merged commit e1531a0 into master Feb 5, 2021
@RyanGlScott RyanGlScott deleted the support-java-9-or-later branch February 5, 2021 23:33
RyanGlScott added a commit to GaloisInc/saw-script that referenced this pull request Feb 8, 2021
This bumps the `crucible` submodule to include GaloisInc/crucible#638, which
adds basic support for handling JDK 9 or later. JDK 9+ packages its standard
library not in a JAR file, but in a JIMAGE file. For more details on how
`crucible-jvm` handles JIMAGE files, refer to
`Note [Loading classes from JIMAGE files]` in `Lang.JVM.Codebase`.

This fixes #861, although there are still unsolved issues that arise when
using modern JDKs with certain classes, such as `String`. As a result, I have
decided to label support for JDK 9+ as experimental:

* I have updated the SAW documentation to mention these shortcomings.
* I have opened GaloisInc/crucible#641 to track the remaining issues.

Other things:

* GaloisInc/crucible#636 and GaloisInc/crucible#638 upstreamed the code from
  `SAWScript.JavaTools` and `SAWScript.ProcessUtils` into `crucible-jvm`, so
  we can remove these modules in favor of importing `Lang.JVM.JavaTools` and
  `Lang.JVM.ProcessUtils` from `crucible-jvm`.

* I removed the dependency on the `xdg-basedir`, as it was unused. This
  dependency was likely added quite some time ago, and it appears that
  `saw-script` switched over to using XDG-related functionality from the
  `directory` library since then. I opted to use `directory` to find the
  `.cache` directory as well, so I have made that clear in the `.cabal` file.

* The `biJavaCodebase :: Codebase` field of `BuiltinContext` is completely
  unused, which I noticed when making changes to the `Codebase` type. Let's
  just remove it.
RyanGlScott added a commit to GaloisInc/saw-script that referenced this pull request Feb 11, 2021
This bumps the `crucible` submodule to include GaloisInc/crucible#638, which
adds basic support for handling JDK 9 or later. JDK 9+ packages its standard
library not in a JAR file, but in a JIMAGE file. For more details on how
`crucible-jvm` handles JIMAGE files, refer to
`Note [Loading classes from JIMAGE files]` in `Lang.JVM.Codebase`.

This fixes #861, although there are still unsolved issues that arise when
using modern JDKs with certain classes, such as `String`. As a result, I have
decided to label support for JDK 9+ as experimental:

* I have updated the SAW documentation to mention these shortcomings.
* I have opened GaloisInc/crucible#641 to track the remaining issues.

Other things:

* GaloisInc/crucible#636 and GaloisInc/crucible#638 upstreamed the code from
  `SAWScript.JavaTools` and `SAWScript.ProcessUtils` into `crucible-jvm`, so
  we can remove these modules in favor of importing `Lang.JVM.JavaTools` and
  `Lang.JVM.ProcessUtils` from `crucible-jvm`.

* I removed the dependency on the `xdg-basedir`, as it was unused. This
  dependency was likely added quite some time ago, and it appears that
  `saw-script` switched over to using XDG-related functionality from the
  `directory` library since then. I opted to use `directory` to find the
  `.cache` directory as well, so I have made that clear in the `.cabal` file.

* The `biJavaCodebase :: Codebase` field of `BuiltinContext` is completely
  unused, which I noticed when making changes to the `Codebase` type. Let's
  just remove it.
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.

2 participants