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

Fix bindgen.sh and solve TODO #4520

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zulinx86
Copy link
Contributor

@zulinx86 zulinx86 commented Mar 22, 2024

Changes

  • Fix bindgen.sh
  • Generates Rust code from prctl.h and solve TODO

Reason

bindgen.sh is broken for the following reasons:

  • --size_t-is-usize was removed since it became default behavior.
  • *_gen crates were moved under vmm crate and patching iff.rs no longer works.

Additional Info

ABI is compatbile.

# python3 tools/test_bindings.py firecracker_old firecracker_new
INFO:__main__:struct 'b'<serde::de::impls::ArrayVisitor<[kvm_bindings::x86_64::bindings::kvm_xcr; 16]> as serde::de::Expected>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::kvm_cpuid2 as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::kvm_pit_channel_state as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::kvm_msrs as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::kvm_msr_list as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::kvm_xcr as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::kvm_debug_exit_arch as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::__IncompleteArrayField<kvm_bindings::x86_64::bindings::kvm_msr_entry> as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<[kvm_bindings::x86_64::bindings::kvm_xcr; 16] as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_segment as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_dtable as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::__IncompleteArrayField<u32> as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::__IncompleteArrayField<kvm_bindings::x86_64::bindings::kvm_cpuid_entry2> as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&alloc::vec::Vec<kvm_bindings::x86_64::bindings::kvm_msr_list, alloc::alloc::Global> as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&alloc::vec::Vec<kvm_bindings::x86_64::bindings::kvm_cpuid2, alloc::alloc::Global> as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<[kvm_bindings::x86_64::bindings::kvm_pit_channel_state; 3] as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_vcpu_events__bindgen_ty_1 as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_vcpu_events__bindgen_ty_2 as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_vcpu_events__bindgen_ty_3 as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_vcpu_events__bindgen_ty_4 as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_vcpu_events__bindgen_ty_5 as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<serde::de::impls::ArrayVisitor<[kvm_bindings::x86_64::bindings::kvm_pit_channel_state; 3]> as serde::de::Expected>::'' matches
INFO:__main__:struct 'b'<vmm_sys_util::fam::FamStructWrapper<kvm_bindings::x86_64::bindings::kvm_cpuid2> as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<alloc::vec::Vec<kvm_bindings::x86_64::bindings::kvm_msrs, alloc::alloc::Global> as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_debugregs as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_lapic_state as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_mp_state as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_regs as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_sregs as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_vcpu_events as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_xcrs as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_xsave as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_pit_state2 as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<kvm_bindings::x86_64::bindings::kvm_clock_data as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'<&vmm_sys_util::fam::FamStructWrapper<kvm_bindings::x86_64::bindings::kvm_msr_list> as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'*mut [kvm_bindings::x86_64::bindings::kvm_msrs] '' matches
INFO:__main__:struct 'b'&mut [kvm_bindings::x86_64::bindings::kvm_cpuid_entry2] '' matches
INFO:__main__:struct 'b'&mut [kvm_bindings::x86_64::bindings::kvm_msr_entry] '' matches
INFO:__main__:struct 'b'&[kvm_bindings::x86_64::bindings::kvm_cpuid_entry2] '' matches
INFO:__main__:struct 'b'&[kvm_bindings::x86_64::bindings::kvm_msr_entry] '' matches
INFO:__main__:struct 'b'&[vmm::io_uring::bindings::io_uring_probe_op] '' matches
INFO:__main__:struct 'b'&[kvm_bindings::x86_64::bindings::kvm_pit_channel_state] '' matches
INFO:__main__:struct 'b'&[kvm_bindings::x86_64::bindings::kvm_xcr] '' matches
INFO:__main__:struct 'b'&[kvm_bindings::x86_64::bindings::kvm_msr_list] '' matches
INFO:__main__:struct 'b'&[kvm_bindings::x86_64::bindings::kvm_msrs] '' matches
INFO:__main__:struct 'b'&[kvm_bindings::x86_64::bindings::kvm_cpuid2] '' matches
INFO:__main__:struct 'b'&[vmm_sys_util::fam::FamStructWrapper<kvm_bindings::x86_64::bindings::kvm_msrs>] '' matches
INFO:__main__:struct 'b'&mut [kvm_bindings::x86_64::bindings::kvm_msrs] '' matches
INFO:__main__:struct 'b'*const [vmm::io_uring::bindings::io_uring_probe_op] '' matches
INFO:__main__:struct 'b'*mut [vmm::io_uring::bindings::io_uring_probe_op] '' matches
INFO:__main__:struct 'b'*const [kvm_bindings::x86_64::bindings::kvm_msrs] '' matches
INFO:__main__:struct 'b'&mut [vmm::io_uring::bindings::io_uring_restriction] '' matches
INFO:__main__:struct 'b'*mut [vmm::io_uring::bindings::io_uring_restriction] '' matches
INFO:__main__:struct 'b'*const [kvm_bindings::x86_64::bindings::kvm_cpuid_entry2] '' matches
INFO:__main__:struct 'b'*const [kvm_bindings::x86_64::bindings::kvm_msr_entry] '' matches
INFO:__main__:struct 'b'*mut [kvm_bindings::x86_64::bindings::kvm_msr_entry] '' matches
INFO:__main__:struct 'b'(*mut [kvm_bindings::x86_64::bindings::kvm_msr_entry], alloc::alloc::Global) '' matches
INFO:__main__:struct 'b'alloc::boxed::Box<[kvm_bindings::x86_64::bindings::kvm_msr_entry], alloc::alloc::Global> '' matches
INFO:__main__:struct 'b'(core::ptr::unique::Unique<[kvm_bindings::x86_64::bindings::kvm_msr_entry]>, alloc::alloc::Global) '' matches
INFO:__main__:struct 'b'*const [kvm_bindings::x86_64::bindings::kvm_cpuid2] '' matches
INFO:__main__:struct 'b'*const [kvm_bindings::x86_64::bindings::kvm_msr_list] '' matches
INFO:__main__:struct 'b'*mut [kvm_bindings::x86_64::bindings::kvm_cpuid_entry2] '' matches
INFO:__main__:struct 'b'*mut [vmm_sys_util::fam::FamStructWrapper<kvm_bindings::x86_64::bindings::kvm_msrs>] '' matches
INFO:__main__:struct 'b'*mut [vmm::io_uring::bindings::io_uring_probe] '' matches
INFO:__main__:struct 'b'*const [vmm_sys_util::fam::FamStructWrapper<kvm_bindings::x86_64::bindings::kvm_msrs>] '' matches
INFO:__main__:struct 'b'&[vmm::io_uring::bindings::io_uring_probe] '' matches
INFO:__main__:struct 'b'*const [vmm::io_uring::bindings::io_uring_probe] '' matches
INFO:__main__:struct 'b'&mut [vmm::io_uring::bindings::io_uring_probe] '' matches
INFO:__main__:struct 'b'&mut [vmm::io_uring::bindings::io_uring_probe_op] '' matches
INFO:__main__:struct 'b'*mut [kvm_bindings::x86_64::bindings::kvm_msr_list] '' matches
INFO:__main__:struct 'b'*mut [kvm_bindings::x86_64::bindings::kvm_cpuid2] '' matches
INFO:__main__:struct 'b'&mut [kvm_bindings::x86_64::bindings::kvm_msr_list] '' matches
INFO:__main__:struct 'b'&mut [kvm_bindings::x86_64::bindings::kvm_cpuid2] '' matches
INFO:__main__:struct 'b'<&kvm_bindings::x86_64::bindings::kvm_ioapic_state__bindgen_ty_1 as core::fmt::Debug>::'' matches
INFO:__main__:struct 'b'&[kvm_bindings::x86_64::bindings::kvm_ioapic_state__bindgen_ty_1] '' matches
INFO:__main__:Structure layout matches

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • [ ] If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • [ ] Any required documentation changes (code and docs) are included in this
    PR.
  • [ ] API changes follow the Runbook for Firecracker API changes.
  • [ ] User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • [ ] New TODOs link to an issue.
  • Commits meet
    contribution quality standards.

  • This functionality cannot be added in rust-vmm.

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

Attention: Patch coverage is 30.09302% with 1503 lines in your changes are missing coverage. Please review.

Project coverage is 80.06%. Comparing base (fd40204) to head (12b5b3d).

Files Patch % Lines
src/vmm/src/io_uring/bindings.rs 30.73% 613 Missing ⚠️
src/vmm/src/devices/virtio/net/gen/iff.rs 30.12% 399 Missing ⚠️
src/vmm/src/arch_gen/x86/mpspec.rs 27.63% 330 Missing ⚠️
src/vmm/src/devices/virtio/gen/virtio_net.rs 33.33% 122 Missing ⚠️
src/vmm/src/devices/virtio/net/gen/if_tun.rs 30.76% 36 Missing ⚠️
src/firecracker/src/main.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4520      +/-   ##
==========================================
- Coverage   82.14%   80.06%   -2.08%     
==========================================
  Files         255      255              
  Lines       31278    30029    -1249     
==========================================
- Hits        25692    24042    -1650     
- Misses       5586     5987     +401     
Flag Coverage Δ
4.14-c5n.metal 77.23% <29.99%> (-2.40%) ⬇️
4.14-c7g.metal ?
4.14-m5n.metal 77.22% <29.99%> (-2.40%) ⬇️
4.14-m6a.metal 76.33% <29.99%> (-2.52%) ⬇️
4.14-m6g.metal 74.58% <30.57%> (-2.11%) ⬇️
4.14-m6i.metal 77.21% <29.99%> (-2.40%) ⬇️
4.14-m7g.metal 74.58% <30.57%> (?)
5.10-c5n.metal 79.86% <30.13%> (?)
5.10-m5n.metal 79.85% <30.13%> (?)
5.10-m6a.metal 79.05% <30.13%> (?)
5.10-m6g.metal 77.44% <30.75%> (?)
5.10-m6i.metal 79.84% <30.13%> (?)
5.10-m7g.metal 77.44% <30.75%> (?)
6.1-c5n.metal 79.86% <30.13%> (-2.29%) ⬇️
6.1-m5n.metal 79.85% <30.13%> (-2.29%) ⬇️
6.1-m6a.metal 79.05% <30.13%> (-2.40%) ⬇️
6.1-m6g.metal 77.44% <30.75%> (-2.03%) ⬇️
6.1-m6i.metal 79.84% <30.13%> (-2.29%) ⬇️
6.1-m7g.metal 77.44% <30.75%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zulinx86 zulinx86 force-pushed the bindgen branch 3 times, most recently from 1f22919 to ceb3ea6 Compare March 22, 2024 21:58
@zulinx86 zulinx86 marked this pull request as ready for review March 22, 2024 21:58
`bindgen.sh` was broken for the following reasons:
- `--size_t-is-usize` was removed since it became default behavior.
- `*_gen` crates were moved under `vmm` crate and patching `iff.rs` no
  longer works.
- `io_uring` crate was moved under `vmm` crate and the bindings path
  became wrong.

Fix `bindgen.sh` and regenerates bindings.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
@zulinx86 zulinx86 force-pushed the bindgen branch 2 times, most recently from 2f4a45e to f2e37bb Compare March 22, 2024 22:00
@zulinx86 zulinx86 changed the title fix(tool): Fix bindgen.sh Fix bindgen.sh and remove TODO Mar 22, 2024
@zulinx86 zulinx86 changed the title Fix bindgen.sh and remove TODO Fix bindgen.sh and solve TODO Mar 22, 2024
Generates Rust code from `prctl.h` and removes TODO.

Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
@zulinx86 zulinx86 self-assigned this Mar 22, 2024
@zulinx86 zulinx86 added rust Pull requests that update Rust code Type: Fix Indicates a fix to existing code Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed rust Pull requests that update Rust code labels Mar 22, 2024
Copy link
Contributor

@pb8o pb8o left a comment

Choose a reason for hiding this comment

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

Re: the layout... should we keep all the gen stuff together in one crate, or spread over many crates?

@@ -0,0 +1,26 @@
From 8faa5d38ebe9b6bd27fda01dd8f54d3d91583166 Mon Sep 17 00:00:00 2001
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm this is a git format-patch patch... I'm not sure it is 100% compatible with patch. Could we use git am below to apply this patch?

@roypat
Copy link
Contributor

roypat commented Mar 25, 2024

Huh, I didn't realize the bindings are included in the coverage reports. Should we exclude them? The bindgen generated unit tests are not a inside of test modules, so their lines are included in coverage reports, but since they have assert!s, all the assertion messages are considered "uncovered", which is nonsense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Fix Indicates a fix to existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants