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

Unused fields in SAWScript.JavaMethodSpec.VerifyParams #1003

Closed
RyanGlScott opened this issue Jan 14, 2021 · 4 comments
Closed

Unused fields in SAWScript.JavaMethodSpec.VerifyParams #1003

RyanGlScott opened this issue Jan 14, 2021 · 4 comments
Milestone

Comments

@RyanGlScott
Copy link
Contributor

The VerifyParams data types is currently defined as:

data VerifyParams = VerifyParams
{ vpCode :: Codebase
, vpContext :: SharedContext
, vpOpts :: Options
, vpSpec :: JavaMethodSpecIR
, vpOver :: [JavaMethodSpecIR]
}

Of these fields, only vpOpts and vpSpec are ever used anywhere (in runValidation). As a proof of concept, it's possible to remove the other fields entirely:

diff --git a/src/SAWScript/JavaBuiltins.hs b/src/SAWScript/JavaBuiltins.hs
index d763b5b2..e9884b65 100644
--- a/src/SAWScript/JavaBuiltins.hs
+++ b/src/SAWScript/JavaBuiltins.hs
@@ -219,13 +219,7 @@ verifyJava cls mname overrides setup = do
   let pos = fixPos -- TODO
   setupRes <- runJavaSetup pos cb cls mname setup
   let ms = jsSpec setupRes
-      vp = VerifyParams {
-             vpCode = cb
-           , vpContext = sc
-           , vpOpts = opts
-           , vpSpec = ms
-           , vpOver = overrides
-           }
+      vp = VerifyParams { vpOpts = opts, vpSpec = ms }
   when (jsSimulate setupRes) $ do
     let overrideText =
           case overrides of
@@ -260,10 +254,9 @@ verifyJava cls mname overrides setup = do
                 SS.Unsat _ -> liftIO $ printOutLn opts Debug "Valid."
                 SS.SatMulti _ vals ->
                        io $ showCexResults opts sc (rwPPOpts rw) ms vs exts vals
-        let ovds = vpOver vp
         initPS <- initializeVerification' sc ms bs cl
-        liftIO $ printOutLn opts Debug $ "Overriding: " ++ show (map specName ovds)
-        mapM_ (overrideFromSpec sc (specPos ms)) ovds
+        liftIO $ printOutLn opts Debug $ "Overriding: " ++ show (map specName overrides)
+        mapM_ (overrideFromSpec sc (specPos ms)) overrides
         liftIO $ printOutLn opts Debug $ "Running method: " ++ specName ms
         -- Execute code.
         run
diff --git a/src/SAWScript/JavaMethodSpec.hs b/src/SAWScript/JavaMethodSpec.hs
index 706d5e5e..0375652c 100644
--- a/src/SAWScript/JavaMethodSpec.hs
+++ b/src/SAWScript/JavaMethodSpec.hs
@@ -425,11 +425,8 @@ overrideFromSpec de pos ir
        key = methodKey method
 
 data VerifyParams = VerifyParams
-  { vpCode    :: Codebase
-  , vpContext :: SharedContext
-  , vpOpts    :: Options
-  , vpSpec    :: JavaMethodSpecIR
-  , vpOver    :: [JavaMethodSpecIR]
+  { vpOpts :: Options
+  , vpSpec :: JavaMethodSpecIR
   }
 
 type SymbolicRunHandler =

In fact, runValidation is the only function that uses VerifyParams at all (as far as I can tell), so it might even be worthwhile to just remove the data type in favor of passing the arguments directly:

-runValidation :: Prover -> VerifyParams -> SymbolicRunHandler
+runValidation :: Prover -> Options -> JavaMethodSpecIR -> SymbolicRunHandler
@RyanGlScott
Copy link
Contributor Author

Some further digging reveals that the biJavaCodebase field of BuiltinContext also goes unused.

@brianhuffman
Copy link
Contributor

brianhuffman commented Jan 14, 2021

You may be interested in #1005, which will make all of this code go away :)

@RyanGlScott
Copy link
Contributor Author

Nice! I suppose that means that the only task remaining after #1005 would be to remove biJavaCodebase. Perhaps I can find a way to put that in a PR that covers similar territory.

@atomb atomb added this to the 0.8 milestone Jan 15, 2021
RyanGlScott added a commit that referenced this issue Feb 2, 2021
This allows SAW 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 `SAWScript.JavaCodebase`.
This fixes #861.

Remaining tasks:

* Ideally, the code in `SAWScript.JavaCodebase` would be upstreamed to
  `crucible-jvm`, where the all-important `Codebase` data type lives.
  Until that happens, I needed to introduce some ugly hacks in order to make
  everything typecheck. In particular, the (hopefully temporary)
  `SAWScript.JavaCodebase` module defines a version of `Codebase` that keeps
  track of JIMAGE-related paths.

Other things:

* 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. This fixes #1003.
@brianhuffman
Copy link
Contributor

#1005 made this problem go away.

RyanGlScott added a commit that referenced this issue Feb 5, 2021
This allows SAW to deal with JDK 9 or later, which packages its standard
library not in a JAR file, but in a JIMAGE file. This leverages `crucible-jvm`
changes from GaloisInc/crucible#634.

This fixes #861.

Other things:

* 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. This fixes #1003.
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

No branches or pull requests

3 participants