Skip to content

mark extern block function signatures as FIXED #966

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 21, 2023
Merged

mark extern block function signatures as FIXED #966

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5214eda
mark extern block function signatures as FIXED
oinoom Jun 16, 2023
7f8bddb
put foreign signatures in gacx.fn_sig
oinoom Jun 21, 2023
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
56 changes: 41 additions & 15 deletions c2rust-analyze/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,46 @@ where
}
}

fn mark_foreign_fixed<'tcx>(
gacx: &mut GlobalAnalysisCtxt<'tcx>,
gasn: &mut GlobalAssignment,
tcx: TyCtxt<'tcx>,
) {
// FIX the inputs and outputs of function declarations in extern blocks
for did in gacx.foreign_mentioned_tys.clone() {
if let DefKind::Fn = tcx.def_kind(did) {
let sig = tcx.erase_late_bound_regions(tcx.fn_sig(did));
let inputs = sig
.inputs()
.iter()
.map(|&ty| gacx.assign_pointer_ids_with_info(ty, PointerInfo::ANNOTATED))
.collect::<Vec<_>>();
for input in inputs {
make_ty_fixed(gasn, input);
}

let output = gacx.assign_pointer_ids_with_info(sig.output(), PointerInfo::ANNOTATED);
make_ty_fixed(gasn, output)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This loop creates a bunch of LTys and marks them FIXED but doesn't store them anywhere, so they have no effect on anything. inputs and output should probably be bundled into an LFnSig and then stored in either gacx.fn_sigs or some new map.

Copy link
Contributor

Choose a reason for hiding this comment

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

gacx.fn_sigs just stores LFnSigs for body owners, so no body-less extern fns (how would some like a non-default trait fn be handled, by the way?) are included. Doesn't that mean we would never rewrite such an extern fn in the first place?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn't rewrite extern-block fns, but we need to retrieve their signatures anywhere they're called. That's why I was thinking it might be reasonable to put them in fn_sigs, alongside the signatures of all the normal fns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we just currently crash on calls to extern fns? That's why I'm working on fixing with hardcoding permissions for libc fns, but that's probably going to work a bit differently from fn_sigs. Right now, and for any extern fn we don't hardcode, we'll crash on the call when we try to lookup the LFnSig from fn_sigs. I just want to make sure we don't silently do the wrong thing instead of crashing if we include these extern fns in fn_sigs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added them to gacx.fn_sigs in 7f8bddb

Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we just currently crash on calls to extern fns? That's why I'm working on fixing with hardcoding permissions for libc fns

Yes - there are two parts to libc function handling:

  1. For analysis, we need to know the READ/WRITE/OFFSET permissions on the function signature. This is the part involving hard-coded permissions.
  2. For rewriting, we need to generate casts around calls to libc functions. Setting FIXED on the signature triggers insertion of these casts.

Right now, and for any extern fn we don't hardcode, we'll crash on the call when we try to lookup the LFnSig from fn_sigs. I just want to make sure we don't silently do the wrong thing instead of crashing if we include these extern fns in fn_sigs.

Good point. Currently util::ty_callee checks for extern-block fns directly (by looking at the parent DefKind), so that part will still work after this change. But are there other places in the code that assume extern-block fns will be absent from fn_sigs?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aneksteind @spernsteiner. I could move the foreign fn sigs to a new map, right? As long as I handle it during calls (which should be quite simple as I'm already changing that stuff), it should be fine, right? I think it can simplify things with #980 and #999.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how that helps, but yeah, you could move them as long as you update all the places where calls are handled (which includes at least dataflow::type_check and rewrite::expr::mir_op - I don't think borrowck::type_check handles calls yet).

}
}

// FIX the fields of structs mentioned in extern blocks
for adt_did in &gacx.adt_metadata.struct_dids {
Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, it would be easier to review (though in this case, it's a simple change so it's not bad) if moves were separated from actual changes in separate commits.

if gacx.foreign_mentioned_tys.contains(adt_did) {
let adt_def = tcx.adt_def(adt_did);
let fields = adt_def.all_fields();
for field in fields {
let field_lty = gacx.field_ltys[&field.did];
eprintln!(
"adding FIXED permission for {adt_did:?} field {:?}",
field.did
);
make_ty_fixed(gasn, field_lty);
}
}
}
}

fn run(tcx: TyCtxt) {
let mut gacx = GlobalAnalysisCtxt::new(tcx);
let mut func_info = HashMap::new();
Expand Down Expand Up @@ -529,21 +569,7 @@ fn run(tcx: TyCtxt) {
}
}

// FIX the fields of structs mentioned in extern blocks
for adt_did in &gacx.adt_metadata.struct_dids {
if gacx.foreign_mentioned_tys.contains(adt_did) {
let adt_def = tcx.adt_def(adt_did);
let fields = adt_def.all_fields();
for field in fields {
let field_lty = gacx.field_ltys[&field.did];
eprintln!(
"adding FIXED permission for {adt_did:?} field {:?}",
field.did
);
make_ty_fixed(&mut gasn, field_lty);
}
}
}
mark_foreign_fixed(&mut gacx, &mut gasn, tcx);

for info in func_info.values_mut() {
let num_pointers = info.acx_data.num_pointers();
Expand Down