mark extern block function signatures as FIXED #966
Conversation
oinoom
force-pushed
the
feat.analyze.foreign
branch
3 times, most recently
from
b4ff66a to
a16883c
Compare
oinoom
force-pushed
the
feat.analyze.foreign
branch
from
a16883c to
5214eda
Compare
| } | ||
|
|
||
| // FIX the fields of structs mentioned in extern blocks | ||
| for adt_did in &gacx.adt_metadata.struct_dids { |
There was a problem hiding this comment.
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.
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
LGTM once the unneeded allocations are removed and the usage of PointerInfo::ANNOTATED is confirmed (I'm assuming my latter thought is right, in which case it's all good).
c2rust-analyze/src/main.rs
Outdated
| let output = gacx.assign_pointer_ids_with_info(sig.output(), PointerInfo::ANNOTATED); | ||
| make_ty_fixed(gasn, output) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've added them to gacx.fn_sigs in 7f8bddb
There was a problem hiding this comment.
Don't we just currently crash on calls to
extern fns? That's why I'm working on fixing with hardcoding permissions forlibcfns
Yes - there are two parts to libc function handling:
- For analysis, we need to know the
READ/WRITE/OFFSETpermissions on the function signature. This is the part involving hard-coded permissions. - For rewriting, we need to generate casts around calls to libc functions. Setting
FIXEDon the signature triggers insertion of these casts.
Right now, and for any
extern fnwe don't hardcode, we'll crash on the call when we try to lookup theLFnSigfromfn_sigs. I just want to make sure we don't silently do the wrong thing instead of crashing if we include theseextern fns infn_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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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).
spernsteiner
left a comment
spernsteiner
left a comment
There was a problem hiding this comment.
Would be nice to have a test for this, but I guess that might be too much trouble when you can't actually call extern-block fns yet.
@spernsteiner how do you want to handle that in the short term, is it worth supporting minimally or in some more principled way? @kkysen didn't you have a WIP for this? |
Fixes #965. The inputs and outputs of body-owners are already marked as fixed, but the traversal of
all_fn_didsdoes not include function declarations inexternblocks