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

Audit function signature codegen #1365

Closed
fzaiser opened this issue Jul 12, 2022 · 5 comments · Fixed by #2994
Closed

Audit function signature codegen #1365

fzaiser opened this issue Jul 12, 2022 · 5 comments · Fixed by #2994
Assignees
Labels
[C] Bug This is a bug. Something isn't working.

Comments

@fzaiser
Copy link
Contributor

fzaiser commented Jul 12, 2022

The code of closure_sig is copy-pasted from fn_sig_for_fn_abi in compiler/rustc_middle/src/ty/layout.rs. We may have to do something similar for generators (#416), so it would be good if we could use the rustc function fn_abi_of_instance directly (as the comment on fn_sig_for_fn_abi suggests. This would prevent problems with keeping in sync with rustc.

@fzaiser fzaiser self-assigned this Jul 28, 2022
@fzaiser
Copy link
Contributor Author

fzaiser commented Jul 29, 2022

The relevant function fn_sig_for_fn_abi is here. It has a comment saying:

    // NOTE(eddyb) this is private to avoid using it from outside of
    // `fn_abi_of_instance` - any other uses are either too high-level
    // for `Instance` (e.g. typeck would use `Ty::fn_sig` instead),
    // or should go through `FnAbi` instead, to avoid losing any
    // adjustments `fn_abi_of_instance` might be performing.

So the question is whether we should use Ty::fn_sig or fn_abi_of_instance. To find out, I looked at the cranelift backend. They use this function to obtain the signature of a function/closure/generator. I presume we should do something similar.

Note that the fn_abi_of_instance function makes some adjustments to the signature returned by the private function fn_sig_for_fn_abi: they happen in the call to fn_abi_new_uncached here, in that function they call fn_abi_adjust_for_abi here, which is defined just below.

@fzaiser
Copy link
Contributor Author

fzaiser commented Jul 29, 2022

@celinval The adjustments to the signature could be related to #1350. These adjustments don't currently happen in the Kani code, but they probably should.

@fzaiser fzaiser changed the title Cleanup function signature codegen Audit function signature codegen Jul 29, 2022
@fzaiser fzaiser removed their assignment Aug 12, 2022
@celinval celinval self-assigned this Jan 16, 2023
@celinval
Copy link
Contributor

celinval commented Jan 16, 2023

Totally! We should investigate if using fn_abi_of_instance would simplify how we handle functions today. I believe this is also related to the issue we were having with closure to function pointer casting: #274

For the ZST parameters, the parameter "mode" is set to Ignore.

@celinval
Copy link
Contributor

celinval commented May 2, 2023

@celinval The adjustments to the signature could be related to #1350. These adjustments don't currently happen in the Kani code, but they probably should.

I don't think this would actually fix #1350 though. I believe we add those functions to the symbol table and their signature matches CBMC ones.

FYI, we declare these functions and their signatures here: https://github.com/model-checking/kani/blob/main/cprover_bindings/src/goto_program/builtin.rs

@celinval
Copy link
Contributor

celinval commented Jul 3, 2023

Note that the function ABI will include the tracker location when required, so #374 and this issue are somewhat related.

@rahulku rahulku added the [C] Bug This is a bug. Something isn't working. label Sep 22, 2023
@rahulku rahulku assigned feliperodri and unassigned celinval Sep 22, 2023
adpaco-aws added a commit that referenced this issue Dec 18, 2023
Migrate these modules to use StableMIR except for calls that depend on
the function signature and ABI. Note that we shouldn't really be using
function signature as captured here:
#1365. So I suggest that we
move to using the FnAbi as soon as we add that to StableMIR.

---------

Co-authored-by: Adrian Palacios <73246657+adpaco-aws@users.noreply.github.com>
@celinval celinval assigned celinval and unassigned feliperodri Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C] Bug This is a bug. Something isn't working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants