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

Toplevel rust translation #1905

Merged
merged 12 commits into from
Aug 16, 2023
Merged

Toplevel rust translation #1905

merged 12 commits into from
Aug 16, 2023

Conversation

scuellar
Copy link
Collaborator

@scuellar scuellar commented Aug 11, 2023

This PR adds a new command

heapster_trans_rust_type : HeapsterEnv -> String -> TopLevel ()

takes the Rust signature for a function and prints the equivalent Heapser type. It is ideal for learning and experimenting with the Rust -> Heapster translation.

The PR includes the file rust_just_translation.saw which demonstrates how to use the command to translate various functions. The file also demonstrates how to (a) create the necessary definitions to support rust and (b) create a proper, empty environment for the command to work.

The example can be ran from the examples folder with

cabal run saw rust_just_translation.saw

@scuellar scuellar added the Heapster Issues specifically related to the Heapster sub-system label Aug 11, 2023
Copy link
Contributor

@eddywestbrook eddywestbrook left a comment

Choose a reason for hiding this comment

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

Looks good overall! Please address the comments I made before we can approve this for merge.

@@ -1020,7 +1022,7 @@ heapster_assume_fun_rename _bic _opts henv nm nm_to perms_string term_string =
do Some lm <- failOnNothing ("Could not find symbol: " ++ nm)
(lookupModContainingSym henv nm)
sc <- getSharedContext
let w = llvmModuleArchReprWidth lm
let w = llvmModuleArchReprWidth lm -- Should hardcode 64
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't actually be hard coded to 64, since it could potentially be useful to have it match some other architecture with a different bit width. Currently, the way to achieve that is to specify it in the Heapster environment. I could imagine making this an argument instead of requiring a Heapster environment, but the current approach seems easiest for now, so maybe just remove this comment?

@@ -1573,6 +1573,34 @@ parseFunPermFromRust env w args ret str
parseFunPermFromRust _ _ _ _ str =
fail ("Malformed Rust type: " ++ str)


-- | Just like `parseFunPermFromRust`, but doesn't take `args` and `ret`.
-- usefull to translate rust when there is no LLVM module (e.g. from command line)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not that there is no LLVM module, it's that there is no specified LLVM function.

Also, it seems like the only place where args and ret are used in parseFunPermFromRust is in the call to un3SomeFunPerm, so, in order to remove all the code duplication between that function and this one, you should redefine parseFunPermFromRust to just call parseSome3FunPermFromRust and then pass the result to un3SomeFunPerm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

I removed the duplication and got it to typecheck. It looks right to me, but it's now doing runLiftRustConvM twice. That seems ok to me, but please make sure it's not introducing any efficiency problems.

Alternatively, I can make parseSome3FunPermFromRust take "lifting" function which can be instantiated to id and for un3SomeFunPerm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right, I didn't notice that un3SomeFunPerm was inside RustConvM. It looks like that function could instead be changed to return an m (SomeFunPerm args ret) in any monad with a MonadFail instance, by using emptyPPInfo in place of rsPPInfo, since un3SomeFunPerm is only ever run at the top level, where the context is empty.

@@ -211,6 +211,20 @@ parseFunPermStringMaybeRust nm w env args ret str =
Just '<' -> parseFunPermFromRust env w args ret str
_ -> parseFunPermString nm env args ret str

-- -- | Just like `parseFunPermStringMaybeRust` but usese `parseSome3FunFromRust`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't leave these comments in the code.

--parseFunPermStringMaybeRust "permissions" w64 env args ret perms_string
liftIO $ putStrLn $ permPrettyString emptyPPInfo fun_perm

-- Lookup a how to print to io & and, looking into permissions.hs
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these comments from the code

withKnownNat w64 $ withLeqProof leq_proof $ do
Some3FunPerm fun_perm <-
parseSome3FunPermFromRust env w64 perms_string
--parseFunPermStringMaybeRust "permissions" w64 env args ret perms_string
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this commented out line from the code

"HeapsterEnv -> String -> TopLevel ()"
(bicVal heapster_translate_rust_type)
Experimental
[ "Parse and print back a set of Heapster permissions for a function"
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this description is slightly imprecise in a way that might confuse the user, because it sounds like the function is parsing Heapster permissions and printing them. The point of this function is to convert a Rust type to a Heapster type and print out the result. So maybe say something like that.

, prim "heapster_parse_test"
"LLVMModule -> String -> String -> TopLevel ()"
(bicVal heapster_parse_test)
Experimental
[ "Parse and print back a set of Heapster permissions for a function"
[ "Parse the Rust signature for a function and print the equivalent Heapser type. Ideal for learning how Rust types are translated into Heapster. "
Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh, it looks like the doc string for heapster_parse_test got switched with that for heapster_trans_rust_type. Slight tweak, though: technically, this isn't parsing the signature for a function, because there need not be any function associated with the type. Instead, it is just parsing a Rust function type.

"HeapsterEnv -> String -> TopLevel ()"
(bicVal heapster_translate_rust_type)
Experimental
[ "Parse a Rust function type and print the equivalent Heapser type. Ideal for learning how Rust types are translated into Heapster. "
Copy link
Contributor

Choose a reason for hiding this comment

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

Please wrap this line to 80 columns like the rest of the file

un3SomeFunPerm args ret (Some3FunPerm fun_perm)
| Just Refl <- testEquality args (funPermArgs fun_perm)
, Just Refl <- testEquality ret (funPermRet fun_perm) =
return $ SomeFunPerm fun_perm
un3SomeFunPerm args ret (Some3FunPerm fun_perm) =
rsPPInfo >>= \ppInfo ->
(return emptyPPInfo) >>= \ppInfo ->
Copy link
Contributor

Choose a reason for hiding this comment

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

How about let ppInfo = emptyPPInfo in instead of binding a return, which looks kind of weird to a Hasekll-er

Copy link
Contributor

@eddywestbrook eddywestbrook left a comment

Choose a reason for hiding this comment

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

Please address the one remaining small change I suggested. Otherwise, this looks good!

@scuellar scuellar added the ready-to-merge If at least one person approves this PR, automatically merge it when CI finishes successfully. label Aug 15, 2023
@eddywestbrook eddywestbrook merged commit d4b61c3 into master Aug 16, 2023
34 of 36 checks passed
@mergify mergify bot deleted the toplevel-rust-translation branch August 16, 2023 21:57
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Heapster Issues specifically related to the Heapster sub-system ready-to-merge If at least one person approves this PR, automatically merge it when CI finishes successfully.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants