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

Fix crash when using public fns reachability #2065

Merged
merged 3 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions kani-compiler/src/codegen_cprover_gotoc/compiler_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ use std::process::Command;
use std::rc::Rc;
use std::time::Instant;
use tempfile::Builder as TempFileBuilder;
use tracing::{debug, error, info, warn};
use tracing::{debug, error, info};

#[derive(Clone)]
pub struct GotocCodegenBackend {
Expand Down Expand Up @@ -310,10 +310,10 @@ fn check_crate_items(gcx: &GotocCtx) {
);
tcx.sess.err(&error_msg);
} else {
warn!(
tcx.sess.warn(format!(
"Ignoring global ASM in crate {}. Verification results may be impacted.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use {gcx.short_crate_name()} instead, which will be necessary to make clippy happy with the upcoming rust toolchain update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I didn't know you couldn't use expressions. We'll do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not working for me. I get the following error:

error: invalid format string: expected `'}'`, found `'.'`
   --> kani-compiler/src/codegen_cprover_gotoc/compiler_interface.rs:314:55
    |
314 |                     "Ignoring global ASM in crate {gcx.short_crate_name()}. Verification results may be impacted.",
    |                                                   -   ^ expected `}` in format string
    |                                                   |
    |                                                   because of this opening brace
    |
    = note: if you intended to print `{`, you can escape it using `{{`

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was wrong. Expressions cannot be used.

gcx.short_crate_name()
);
));
}
}
}
Expand Down Expand Up @@ -371,7 +371,7 @@ fn codegen_results(
fn collect_codegen_items<'tcx>(gcx: &GotocCtx<'tcx>) -> Vec<MonoItem<'tcx>> {
let tcx = gcx.tcx;
let reach = gcx.queries.get_reachability_analysis();
debug!(?reach, "starting_points");
debug!(?reach, "collect_codegen_items");
match reach {
ReachabilityType::Legacy => {
// Use rustc monomorphizer to retrieve items to codegen.
Expand Down Expand Up @@ -399,7 +399,8 @@ fn collect_codegen_items<'tcx>(gcx: &GotocCtx<'tcx>) -> Vec<MonoItem<'tcx>> {
ReachabilityType::PubFns => {
let entry_fn = tcx.entry_fn(()).map(|(id, _)| id);
let local_reachable = filter_crate_items(tcx, |_, def_id| {
tcx.is_reachable_non_generic(def_id) || entry_fn == Some(def_id)
(tcx.is_reachable_non_generic(def_id) && tcx.def_kind(def_id).is_fn_like())
|| entry_fn == Some(def_id)
});
collect_reachable_items(tcx, &local_reachable).into_iter().collect()
}
Expand Down
55 changes: 46 additions & 9 deletions kani-compiler/src/kani_middle/reachability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ use tracing::{debug, debug_span, trace, warn};
use rustc_data_structures::fingerprint::Fingerprint;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_hir::def::DefKind;
use rustc_hir::def_id::DefId;
use rustc_hir::ItemId;
use rustc_middle::mir::interpret::{AllocId, ConstValue, ErrorHandled, GlobalAlloc, Scalar};
use rustc_middle::mir::mono::MonoItem;
use rustc_middle::mir::visit::Visitor as MirVisitor;
Expand Down Expand Up @@ -57,18 +59,35 @@ pub fn collect_reachable_items<'tcx>(
}

/// Collect all (top-level) items in the crate that matches the given predicate.
/// An item can only be a root if they are: non-generic Fn / Static / GlobalASM
pub fn filter_crate_items<F>(tcx: TyCtxt, predicate: F) -> Vec<MonoItem>
where
F: FnMut(TyCtxt, DefId) -> bool,
F: Fn(TyCtxt, DefId) -> bool,
{
let mut filter = predicate;
tcx.hir_crate_items(())
.items()
.filter_map(|hir_id| {
let def_id = hir_id.owner_id.def_id.to_def_id();
filter(tcx, def_id).then(|| MonoItem::Fn(Instance::mono(tcx, def_id)))
})
.collect()
let crate_items = tcx.hir_crate_items(());
// Filter regular items.
let root_items = crate_items.items().filter_map(|item| {
let def_id = item.owner_id.def_id.to_def_id();
if !is_generic(tcx, def_id) && predicate(tcx, def_id) {
to_mono_root(tcx, item, def_id)
} else {
None
}
});

// Filter items from implementation blocks.
let impl_items = crate_items.impl_items().filter_map(|impl_item| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are impl_items not included in items above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my initial though, but I was wrong. For harnesses it didn't matter much, since we don't expect associated functions with them, but for other things they might be important.

FYI, for the little test I added in this PR, here are the breakdown of items and impl_items:

### ITEMS
def_id: DefId(0:1 ~ associated_fn[18c1]::{use#0}) -- kind: Use
def_id: DefId(0:2 ~ associated_fn[18c1]::std) -- kind: ExternCrate
def_id: DefId(0:3 ~ associated_fn[18c1]::Dummy) -- kind: Struct
def_id: DefId(0:5 ~ associated_fn[18c1]::{impl#0}) -- kind: Impl

### IMPL Items
def_id: DefId(0:6 ~ associated_fn[18c1]::{impl#0}::new) -- kind: AssocFn

Ps.: associated_fn is the name of the crate where they are defined. :)

let def_id = impl_item.owner_id.def_id.to_def_id();
if matches!(tcx.def_kind(def_id), DefKind::AssocFn)
&& !is_generic(tcx, def_id)
&& predicate(tcx, def_id)
{
Some(MonoItem::Fn(Instance::mono(tcx, def_id)))
} else {
None
}
});
root_items.chain(impl_items).collect()
}

/// Use a predicate to find `const` declarations, then extract all closures from those declarations
Expand Down Expand Up @@ -96,6 +115,24 @@ where
roots
}

fn is_generic(tcx: TyCtxt, def_id: DefId) -> bool {
let generics = tcx.generics_of(def_id);
generics.requires_monomorphization(tcx)
}

fn to_mono_root(tcx: TyCtxt, item_id: ItemId, def_id: DefId) -> Option<MonoItem> {
let kind = tcx.def_kind(def_id);
match kind {
DefKind::Static(..) => Some(MonoItem::Static(def_id)),
DefKind::Fn => Some(MonoItem::Fn(Instance::mono(tcx, def_id))),
DefKind::GlobalAsm => Some(MonoItem::GlobalAsm(item_id)),
_ => {
debug!(?def_id, ?kind, "Ignored item. Not a root type.");
None
}
}
}

struct MonoItemsCollector<'tcx> {
/// The compiler context.
tcx: TyCtxt<'tcx>,
Expand Down
15 changes: 15 additions & 0 deletions tests/expected/associated-fn/associated_fn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright Kani Contributors
// SPDX-License-Identifier: Apache-2.0 OR MIT
// kani-flags: --enable-unstable --function new
//! This ensures our public functions reachability module works for associated functions.

struct Dummy {
c: char,
}

impl Dummy {
#[no_mangle]
pub fn new() -> Self {
Dummy { c: ' ' }
}
}
3 changes: 3 additions & 0 deletions tests/expected/associated-fn/expected
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Checking harness new...
VERIFICATION:- SUCCESSFUL

15 changes: 15 additions & 0 deletions tests/kani/Static/pub_static.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright Kani Contributors
// SPDX-License-Identifier: Apache-2.0 OR MIT
// kani-flags: --enable-unstable --function harness
//! This test covers an issue we had with our public-fns implementation.
//! We were not checking if a root was a function in the first place.
//! https://github.com/model-checking/kani/issues/2047

pub static DAYS_OF_WEEK: [char; 7] = ['s', 'm', 't', 'w', 't', 'f', 's'];

#[no_mangle]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not kani::proof (and --harness)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the bug this change fixes was on how we were collecting starting points for the public functions reachability mode. For the harnesses reachability mode, the predicate only passes for functions.

pub fn harness() {
let day: usize = kani::any();
kani::assume(day < DAYS_OF_WEEK.len());
assert!(['s', 'm', 't', 'w', 'f'].contains(&DAYS_OF_WEEK[day]));
}