Skip to content

Generate bindgen files on build#236

Open
slp wants to merge 3 commits into
rust-vmm:mainfrom
slp:bindgen-on-build
Open

Generate bindgen files on build#236
slp wants to merge 3 commits into
rust-vmm:mainfrom
slp:bindgen-on-build

Conversation

@slp
Copy link
Copy Markdown

@slp slp commented Jun 5, 2026

Summary of the PR

Carrying pre-generated bindgen files is problematic because different targets may require different translation of types.

Drop bootparam.rs, elf.rs and start_info.rs, and have them generated on build using the minimized Linux headers imported on the previous commit.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

slp added 3 commits June 5, 2026 12:26
Import a minimized version of bootparam.h, elf.h and start_info.h
to be used by bindgen to generate the respective Rust files.

Signed-off-by: Sergio Lopez <slp@redhat.com>
Carrying pre-generated bindgen files is problematic because
different targets may require different translation of types.

Drop bootparam.rs, elf.rs and start_info.rs, and have them generated
on build using the Linux headers imported on the previous commit.

Signed-off-by: Sergio Lopez <slp@redhat.com>
Since we now run bindgen on build, add a build requirements section
indicating that clang is required to build this crate.

Signed-off-by: Sergio Lopez <slp@redhat.com>
Copy link
Copy Markdown
Collaborator

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

I really don't think we should do this. I think it breaks cross compilation and makes big assumptions about the environment the build is run in.

@rbradford
Copy link
Copy Markdown
Collaborator

I'm also confused about your comment about different translation of types - these are x86-64 specific files - with the C ABI there is only one way that these types can be defined on that platform.

Can you share the real world problem you're trying to fix with this?

@slp
Copy link
Copy Markdown
Author

slp commented Jun 5, 2026

I really don't think we should do this. I think it breaks cross compilation and makes big assumptions about the environment the build is run in.

Much on the contrary, this doesn't break cross compilation, but fixes it. In fact, this is the recommended way of using bindgen: https://rust-lang.github.io/rust-bindgen/library-usage.html.

The only assumption about the build environment is that clang-devel is installed, as the required Linux headers are vendored.

I'm also confused about your comment about different translation of types - these are x86-64 specific files - with the C ABI there is only one way that these types can be defined on that platform.

Can you share the real world problem you're trying to fix with this?

The prime example of this is the discrepancy between the size of unsigned long, being 64 bit on Linux but 32 bit on Windows. There are other more subtle potential issues such as different padding and alignment requirements.

Running bindgen on build, along with the simplified headers imported in the first commit, ensures compatibility across arches and OSes.

@rbradford
Copy link
Copy Markdown
Collaborator

The prime example of this is the discrepancy between the size of unsigned long, being 64 bit on Linux but 32 bit on Windows. There are other more subtle potential issues such as different padding and alignment requirements.

Running bindgen on build, along with the simplified headers imported in the first commit, ensures compatibility across arches and OSes.

I think if you changed the size values then the kernel wouldn't boot as Linux guest assumes those fixed sizes

@slp
Copy link
Copy Markdown
Author

slp commented Jun 5, 2026

The prime example of this is the discrepancy between the size of unsigned long, being 64 bit on Linux but 32 bit on Windows. There are other more subtle potential issues such as different padding and alignment requirements.
Running bindgen on build, along with the simplified headers imported in the first commit, ensures compatibility across arches and OSes.

I think if you changed the size values then the kernel wouldn't boot as Linux guest assumes those fixed sizes

Indeed, that's why we need unsigned long to be c_ulong on Linux and c_ulonglong on Windows.

The alternative to this PR would be manually fixing the files generated by bindgen to address every cross-os/cross-arch issue we find.

@stefano-garzarella
Copy link
Copy Markdown
Member

IIRC in vm-virtio we did something similar but then reverted it with rust-vmm/vm-virtio#348 Not sure if those scripts can help here too.

@rbradford
Copy link
Copy Markdown
Collaborator

What about just making them "native rust" (i know we would still need the #[repr(C)] rather than bindgen e.g. use u8 and u64 vs the ffi versions. I don't think these are ever going to change.

@slp
Copy link
Copy Markdown
Author

slp commented Jun 5, 2026

IIRC in vm-virtio we did something similar but then reverted it with rust-vmm/vm-virtio#348 Not sure if those scripts can help here too.

Kind of funny this one: "because users have found the requirement to have an up to date libclang at build time to be onerous". xD

What about just making them "native rust" (i know we would still need the #[repr(C)] rather than bindgen e.g. use u8 and u64 vs the ffi versions. I don't think these are ever going to change.

Yeah, that would work (and I guess it'd be less onerous ;-P). Let me redo this PR.

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.

3 participants