-
Notifications
You must be signed in to change notification settings - Fork 101
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
Remove glob to collect all artifacts introduced by cargo kani #2246
Merged
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
b820644
Remove glob from cargo call
celinval 3638801
Replace glob by hacky file stub
celinval 1983382
Address PR comments
celinval 789dc19
Add more logs + improve error handling
celinval c920caa
Fix flaky behavior when artifact is reported out of order
celinval 9cf38d6
Merge branch 'main' into issue-1478-glob
celinval File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,16 @@ | |
|
||
use crate::args::KaniArgs; | ||
use crate::call_single_file::to_rustc_arg; | ||
use crate::project::Artifact; | ||
use crate::session::KaniSession; | ||
use anyhow::{bail, Context, Result}; | ||
use cargo_metadata::diagnostic::{Diagnostic, DiagnosticLevel}; | ||
use cargo_metadata::{Message, Metadata, MetadataCommand, Package}; | ||
use kani_metadata::{ArtifactType, CompilerArtifactStub}; | ||
use std::ffi::{OsStr, OsString}; | ||
use std::fs; | ||
use std::fs::{self, File}; | ||
use std::io::BufReader; | ||
use std::path::{Path, PathBuf}; | ||
use std::path::PathBuf; | ||
use std::process::Command; | ||
use tracing::{debug, trace}; | ||
|
||
|
@@ -29,12 +31,8 @@ pub struct CargoOutputs { | |
/// The directory where compiler outputs should be directed. | ||
/// Usually 'target/BUILD_TRIPLE/debug/deps/' | ||
pub outdir: PathBuf, | ||
/// The collection of *.symtab.out goto binary files written. | ||
pub symtab_gotos: Vec<PathBuf>, | ||
/// The location of vtable restrictions files (a directory of *.restrictions.json) | ||
pub restrictions: Option<PathBuf>, | ||
/// The kani-metadata.json files written by kani-compiler. | ||
pub metadata: Vec<PathBuf>, | ||
pub metadata: Vec<Artifact>, | ||
/// Recording the cargo metadata from the build | ||
pub cargo_metadata: Metadata, | ||
} | ||
|
@@ -105,6 +103,7 @@ impl KaniSession { | |
|
||
let mut found_target = false; | ||
let packages = packages_to_verify(&self.args, &metadata); | ||
let mut artifacts = vec![]; | ||
for package in packages { | ||
for target in package_targets(&self.args, package) { | ||
let mut cmd = Command::new("cargo"); | ||
|
@@ -118,7 +117,7 @@ impl KaniSession { | |
.env("CARGO_ENCODED_RUSTFLAGS", rustc_args.join(OsStr::new("\x1f"))) | ||
.env("CARGO_TERM_PROGRESS_WHEN", "never"); | ||
|
||
self.run_cargo(cmd)?; | ||
artifacts.extend(self.run_cargo(cmd)?.into_iter()); | ||
found_target = true; | ||
} | ||
} | ||
|
@@ -127,13 +126,7 @@ impl KaniSession { | |
bail!("No supported targets were found."); | ||
} | ||
|
||
Ok(CargoOutputs { | ||
outdir: outdir.clone(), | ||
symtab_gotos: glob(&outdir.join("*.symtab.out"))?, | ||
metadata: glob(&outdir.join("*.kani-metadata.json"))?, | ||
restrictions: self.args.restrict_vtable().then_some(outdir), | ||
cargo_metadata: metadata, | ||
}) | ||
Ok(CargoOutputs { outdir, metadata: artifacts, cargo_metadata: metadata }) | ||
} | ||
|
||
fn cargo_metadata(&self, build_target: &str) -> Result<Metadata> { | ||
|
@@ -165,9 +158,10 @@ impl KaniSession { | |
} | ||
|
||
/// Run cargo and collect any error found. | ||
/// TODO: We should also use this to collect the artifacts generated by cargo. | ||
fn run_cargo(&self, cargo_cmd: Command) -> Result<()> { | ||
/// We also collect the metadata file generated during compilation if any. | ||
fn run_cargo(&self, cargo_cmd: Command) -> Result<Option<Artifact>> { | ||
let support_color = atty::is(atty::Stream::Stdout); | ||
let mut artifact = None; | ||
if let Some(mut cargo_process) = self.run_piped(cargo_cmd)? { | ||
let reader = BufReader::new(cargo_process.stdout.take().unwrap()); | ||
let mut error_count = 0; | ||
|
@@ -196,9 +190,10 @@ impl KaniSession { | |
} | ||
} | ||
}, | ||
Message::CompilerArtifact(_) | ||
| Message::BuildScriptExecuted(_) | ||
| Message::BuildFinished(_) => { | ||
Message::CompilerArtifact(rustc_artifact) => { | ||
artifact = Some(rustc_artifact); | ||
} | ||
Message::BuildScriptExecuted(_) | Message::BuildFinished(_) => { | ||
// do nothing | ||
} | ||
Message::TextLine(msg) => { | ||
|
@@ -222,7 +217,11 @@ impl KaniSession { | |
); | ||
} | ||
} | ||
Ok(()) | ||
// We generate kani specific artifacts only for the build target. The build target is | ||
// always the last artifact generated in a build, and all the other artifacts are related | ||
// to dependencies or build scripts. Hence, we need to invoke `map_kani_artifact` only | ||
// for the last compiler artifact. | ||
Ok(artifact.and_then(map_kani_artifact)) | ||
} | ||
} | ||
|
||
|
@@ -236,15 +235,6 @@ fn print_msg(diagnostic: &Diagnostic, use_rendered: bool) -> Result<()> { | |
Ok(()) | ||
} | ||
|
||
/// Given a `path` with glob characters in it (e.g. `*.json`), return a vector of matching files | ||
fn glob(path: &Path) -> Result<Vec<PathBuf>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Woohoo! |
||
let results = glob::glob(path.to_str().context("Non-UTF-8 path enountered")?)?; | ||
// the logic to turn "Iter<Result<T, E>>" into "Result<Vec<T>, E>" doesn't play well | ||
// with anyhow, so a type annotation is required | ||
let v: core::result::Result<Vec<PathBuf>, glob::GlobError> = results.collect(); | ||
Ok(v?) | ||
} | ||
|
||
/// Extract the packages that should be verified. | ||
/// If `--package <pkg>` is given, return the list of packages selected. | ||
/// If `--workspace` is given, return the list of workspace members. | ||
|
@@ -276,6 +266,42 @@ fn packages_to_verify<'b>(args: &KaniArgs, metadata: &'b Metadata) -> Vec<&'b Pa | |
packages | ||
} | ||
|
||
/// Extract Kani artifact that might've been generated from a given rustc artifact. | ||
/// Not every rustc artifact will map to a kani artifact, hence the `Option<>`. | ||
/// | ||
/// Unfortunately, we cannot always rely on the messages to get the path for the original artifact | ||
/// that `rustc` produces. So we hack the content of the output path to point to the original | ||
/// metadata file. See <https://github.com/model-checking/kani/issues/2234> for more details. | ||
fn map_kani_artifact(rustc_artifact: cargo_metadata::Artifact) -> Option<Artifact> { | ||
debug!(?rustc_artifact, "map_kani_artifact"); | ||
if rustc_artifact.target.is_custom_build() { | ||
// We don't verify custom builds. | ||
return None; | ||
} | ||
let result = rustc_artifact.filenames.iter().find_map(|path| { | ||
if path.extension() == Some("rmeta") { | ||
let file_stem = path.file_stem()?.strip_prefix("lib")?; | ||
let parent = | ||
path.parent().map(|p| p.as_std_path().to_path_buf()).unwrap_or(PathBuf::new()); | ||
let mut meta_path = parent.join(file_stem); | ||
meta_path.set_extension(ArtifactType::Metadata); | ||
|
||
// This will check if the file exists and we just skip if it doesn't. | ||
Artifact::try_new(&meta_path, ArtifactType::Metadata).ok() | ||
} else if path.extension() == Some("rlib") { | ||
// We skip `rlib` files since we should also generate a `rmeta`. | ||
None | ||
} else { | ||
// For all the other cases we write the path of the metadata into the output file. | ||
let input_file = File::open(path).ok()?; | ||
let stub: CompilerArtifactStub = serde_json::from_reader(input_file).ok()?; | ||
Artifact::try_new(&stub.metadata_path, ArtifactType::Metadata).ok() | ||
} | ||
}); | ||
debug!(?result, "map_kani_artifact"); | ||
result | ||
} | ||
|
||
/// Possible verification targets. | ||
enum VerificationTarget { | ||
Bin(String), | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that there's a single artifact per cargo invocation? If so, I suggest adding an assertion that checks that, e.g:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. Every package will generate at least one artifact. However, we only care about the last one, which is the one that we pass the codegen flag to. Thus, I am only running the map in the last artifact that was collected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Perhaps add a comment to mention this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done