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

(c2rust-analyze) 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 #999

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Jul 19, 2023

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.

… the `SymbolName` so that we don't ICE on generic foreign `fn`s like `extern "rust-intrinsic"`s.
@kkysen kkysen requested a review from spernsteiner July 19, 2023 20:33
@kkysen
Copy link
Contributor Author

kkysen commented Jul 19, 2023

@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
Copy link
Collaborator

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
Copy link
Contributor Author

kkysen commented Jul 20, 2023

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:

  • "Rust"
  • "rust-intrinsic"
  • "rust-call"
  • "rust-cold"

@spernsteiner
Copy link
Collaborator

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
Copy link
Contributor Author

kkysen commented Jul 20, 2023

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

analyze: ICE on pointer transmute
2 participants