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

Move symtab2gb to codegen step #1686

Merged
merged 3 commits into from
Sep 30, 2022

Conversation

danielsn
Copy link
Contributor

@danielsn danielsn commented Sep 16, 2022

Description of changes:

Currently, we compile each crate to a symbol-table json, then at the very end run symtab2gb.
This step logically belongs as part of codegen.
As a benefit, we now parallelize symtab2gb calls and get a speedup.

Resolved issues:

Resolves #1453

Related RFC:

Call-outs:

Testing:

  • How is this change tested? Existing reg tests

  • Is this a refactor change? No

Checklist

  • Each commit message has a non-empty body, explaining why the change was made
  • Methods or procedures are documented
  • Regression or unit tests are included, or existing tests cover the modified code
  • My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

@celinval
Copy link
Contributor

@danielsn can you please fix this PR?

@danielsn danielsn force-pushed the move-symtab-earlier branch 3 times, most recently from 1750d4a to 22f03d9 Compare September 29, 2022 19:34
@danielsn danielsn marked this pull request as ready for review September 29, 2022 19:34
@danielsn danielsn requested a review from a team as a code owner September 29, 2022 19:34
@@ -44,7 +43,9 @@ fn cargokani_main(input_args: Vec<OsString>) -> Result<()> {

let mut goto_objs: Vec<PathBuf> = Vec::new();
for symtab in &outputs.symtabs {
goto_objs.push(ctx.symbol_table_to_gotoc(symtab)?);
let goto_obj_filename = symtab.with_extension("out");
ctx.record_temporary_files(&[&goto_obj_filename]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we remove these files still? If so, wouldn't this break subsequent runs of cargo kani?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were being removed before, and I thought it best to keep that behaviour. @tedinski thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

The file removal behavior for cargo kani was something I went "huh that's strange" and just preserved what the original python scripts did. We seem to keep some things and delete others and I'm not sure why really.

I'm open to any behavior that makes sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@celinval? Vote? Honestly, I think for cargo we should probably just keep everything since it goes into a target folder anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do globs for what to link together in the end though, and that might motivate the deletion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, why don't we keep the current behaviour, and have an issue to have a principled set of things to keep/delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing showed that removing the files makes repeated cargo-kani fail

Copy link
Contributor

@celinval celinval Sep 30, 2022

Choose a reason for hiding this comment

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

I don't think they are random today. We cannot delete the files that are generated by kani-compiler, otherwise rebuild breaks as @danielsn was able to verify.

That said, I do think we need better strategy. We should eventually implement a cache, and we should also consider not using glob anymore. We can use cargo_metadata to retrieve the files generated by the compiler instead.

@danielsn danielsn merged commit 38ba4b5 into model-checking:main Sep 30, 2022
@danielsn danielsn deleted the move-symtab-earlier branch September 30, 2022 22:24
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.

Parallelize the conversion of symbol tables to goto binaries
3 participants