immunant / c2rust

Migrate C code to Rust
https://c2rust.com/
Other
3.79k stars 219 forks source link

(`c2rust-analyze`) Use `SubstsRef` when creating an `Instance` to get the `SymbolName` so that we don't ICE on generic foreign `fn`s like `extern "rust-intrinsic"`s #999

Open kkysen opened 11 months ago

kkysen commented 11 months ago

Use SubstsRef when creating an Instance to get the SymbolName so that we don't ICE on generic foreign fns like extern "rust-intrinsic"s.

kkysen commented 11 months ago

@spernsteiner, I'm not sure if this is the right approach here yet, but I'd like to see if it fixes the issue at least. I'm looking a bit more into how Instance and fn compute_symbol_name work now.

spernsteiner commented 11 months ago

Seems like a reasonable approach. Though I will say I don't think it's worth trying to extend the known_fn machinery to handle generic functions like transmute. We should just bail out on those, one way or another. With the current changes, we bail out because symbol_name (I assume) produces some mangled name that won't match anything in the permission lookup tables, but we could also bail out earlier based on the substs being non-empty or by looking at the ABI (the result of tcx.fn_sig(def_id) include an abi field).

kkysen commented 11 months ago

With the current changes, we bail out because symbol_name (I assume) produces some mangled name that won't match anything in the permission lookup tables

Yeah, that was the plan. But as you said, there might be better ways to do this. Maybe just check if substs.is_empty(). For checking the ABI, there are a lot of different Abis. We'd presumably want to skip any of the Rust-like ones*, i.e. ones that could be generic. Should we also skip these ones (like rust-intrinsic) in fn gather_foreign_sigs in the first place?

The possibly generic Rusty ones, from what I can guess:

spernsteiner commented 11 months ago

Maybe just check if substs.is_empty().

One thing I'm unsure about is whether this would work with known_fn_ptr_perms, which calls known_fn but doesn't have a substs available to check.

We'd presumably want to skip any of the Rust-like ones*, i.e. ones that could be generic.

extern "Rust" { ... } doesn't allow generic functions in the extern block. I think the only ABI that allows that is extern "rust-intrinsic" { ... } (which is very much a special case).

Should we also skip these ones (like rust-intrinsic) in fn gather_foreign_sigs in the first place?

gather_foreign_sigs feeds into the "mark FFI-exposed types FIXED" pass, right? We could potentially skip rust-intrinsics there, since they shouldn't have any local (= potentially rewritable) types in their signature anyway. But extern "Rust" { ... } should generally be treated about the same as extern "C" { ... }.

kkysen commented 11 months ago

extern "Rust" { ... } doesn't allow generic functions in the extern block. I think the only ABI that allows that is extern "rust-intrinsic" { ... } (which is very much a special case).

It doesn't? I didn't realize that. Do you know where it documents that? I've had trouble finding much documentation about the different ABIs in general.

spernsteiner commented 11 months ago

Do you know where it documents that?

No idea, but if you try something like extern "Rust" { fn foo<T>(x: T); }, you get "error[E0044]: foreign items may not have type parameters". Also, it's not clear what it would mean to have such a thing, or how you would implement linking (especially dynamic linking) for one. extern "rust-intrinsic" only allows it because each rust-intrinsic function is special-cased within the compiler.