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

Compile time layout tests #2787

Merged
merged 1 commit into from
Apr 1, 2024
Merged

Conversation

GKFX
Copy link
Contributor

@GKFX GKFX commented Mar 24, 2024

This commit emits layout tests as compile-time assertions, rather than as unit tests. This allows them to be run in situations where running unit tests is unfeasible, i.e. cross-compiled code and nostd targets.

Fixes #2786. @ojeda - I noticed you tagged the feature request for Rust-for-Linux.

bindgen/codegen/mod.rs Outdated Show resolved Hide resolved
@emilio
Copy link
Contributor

emilio commented Mar 26, 2024

This looks great, though I think it's a requirement to make it the default to be able to tell how the values differ... Can we use format!() in the message or something along those lines?

@emilio
Copy link
Contributor

emilio commented Mar 26, 2024

(or does the assert macro print that already?)

@pvdrz
Copy link
Contributor

pvdrz commented Mar 26, 2024

There's no way to print such thing on const context AFAIK

@emilio
Copy link
Contributor

emilio commented Mar 26, 2024

I haven't been able to play with this yet but maybe we can abuse slice indexing to at least print the differences between actual and expected? E.g for [0u8][X - Y] or something like that which would panic if X was different to Y, and I would expect the compiler to print the bad index?

@pvdrz
Copy link
Contributor

pvdrz commented Mar 27, 2024

oh that's so hacky... I love it!

@GKFX GKFX requested a review from pvdrz March 29, 2024 16:35
@GKFX
Copy link
Contributor Author

GKFX commented Mar 29, 2024

Using slice indexing works. I've put the string as the expression in the array rather than 0u8 so it's visible.

@GKFX GKFX force-pushed the compile-time-layout-tests branch from cbb817f to d508b91 Compare March 29, 2024 19:16
@emilio emilio merged commit 4f78afa into rust-lang:main Apr 1, 2024
33 checks passed
@GKFX GKFX deleted the compile-time-layout-tests branch April 7, 2024 19:12
@gabiganam
Copy link
Contributor

gabiganam commented Aug 19, 2024

The new code generated by this change triggers a false-positive unnecessary operation warning from rust-clippy.
Seems like the issue was already reported: rust-lang/rust-clippy#12817
And fixed: rust-lang/rust-clippy#12840
But wasn't released yet.

@Arnavion
Copy link

It also increased MSRV of the generated code to 1.77.0 which is the one that stabilized offset_of!

@GKFX
Copy link
Contributor Author

GKFX commented Aug 22, 2024

The MSRV of the generated code is configurable with Builder::rust_target. If using Bindgen 0.70 and Rust < 1.77.0 then the relevant target will need to be set.

@Arnavion
Copy link

Ah yes, sorry for the noise.

plaes added a commit to plaes/nrf-sdc that referenced this pull request Oct 22, 2024
Biggest change change introduced in 0.70 is the compile-time
layout tests to to check and validate layouts. Previously these
tests were implemented as unit tests which made it somewhat
difficult to run on cross-compiled targets.

More info:
* rust-lang/rust-bindgen#2786
* rust-lang/rust-bindgen#2787
plaes added a commit to plaes/nrf-sdc that referenced this pull request Oct 22, 2024
Biggest change change introduced in bindgen 0.70 is the compile-time
layout tests to to check and validate layouts. Previously these
tests were implemented as unit tests which made it somewhat
difficult to run on cross-compiled targets.

Also, drop `rustfmt_bindings()` call had been enabled by default for
a while and was removed/deprecated in bindgen 0.65.x releases.

More info:
* rust-lang/rust-bindgen#2786
* rust-lang/rust-bindgen#2787
@AlexTMjugador
Copy link
Contributor

AlexTMjugador commented Nov 28, 2024

A seemingly-overlooked side effect of moving layout tests to constant expressions is that C code which gets translated to different bindings in different platforms may no longer compile on platforms different than the one the bindings were generated in. This is especially inconvenient when the platform-specific differences in size and alignment do not affect correctness.

For instance, this problem has caused Windows builds to fail for a project I maintain when using bindings generated under Linux. In this case, the discrepancies stem from the long type having different sizes on Windows and Linux, which is inconsequential for the purposes of my project, since such numeric types are referred to by their C aliases. Generating the bindings during the build for the current target plaform is not that practically feasible either, because that slows down compilation significantly and introduces additional build-time requirements.

As @GKFX noted, configuring bindgen to target Rust 1.73 restores the previous codegen behavior for size and alignment tests. However, discovering this workaround was difficult, as I had to dig through the repository to locate this PR, and I think it's a bad long-term solution. This workaround means the generated bindings won't ever be able to take advantage of features introduced in newer Rust versions, and it's reasonable to expect that bindgen maintainers may eventually drop support for targeting older Rust versions once doing so becomes enough of a burden.

Would it be feasible to make this behavior toggleable via an independent switch or, at the very least, document it more thoroughly? This could save others from encountering similar problems.

@GKFX
Copy link
Contributor Author

GKFX commented Nov 28, 2024

Generating bindings on one platform and using them on another is inherently risky, and I personally feel that the layout tests are doing their job here by alerting you to that fact. The difference between longs may have worked out OK but that doesn't mean everything else will.

If you do expect the layout tests to fail I would disable them entirely with Builder::layout_tests rather than using the older version and just not running them.

@AlexTMjugador
Copy link
Contributor

Thank you for the tip about disabling layout tests, it's indeed a much better solution for use cases like mine! I was initially reluctant to use it because I wanted to have layout test coverage for Linux and for the bindings that don't change between platforms, but I ended up not running such tests on Windows anyway because they failed, so your comment made me realize I was not compromising on much 👍

AlexTMjugador added a commit to ComunidadAylas/vorbis-rs that referenced this pull request Nov 28, 2024
They are mainly useful to catch unexpected layout discrepancies across
platforms, but we were already expecting such discrepancies on Windows,
due to the bindings being pregenerated on Linux.

It'd be great to still have these tests on Linux, but with current
versions of bindgen that's not doable without resorting to increasing
tech debt. (See
rust-lang/rust-bindgen#2787 (comment))
@pvdrz
Copy link
Contributor

pvdrz commented Nov 28, 2024

Generating bindings on one platform and using them on another is inherently risky, and I personally feel that the layout tests are doing their job here by alerting you to that fact. The difference between longs may have worked out OK but that doesn't mean everything else will.

I agree wholeheartedly with this. If you want to support different platforms, I think you should consider cfg gating your bindings in one way or another.

@AlexTMjugador
Copy link
Contributor

AlexTMjugador commented Nov 28, 2024

I think you should consider cfg gating your bindings in one way or another.

I agree that's the most correct solution in general, but in my case doing so bloats the crate size and reduces its portability (realistically, I cannot be bothered to pregenerate bindings for every platform users may want to target, and generating bindings on build is not ideal) for no gain, as the only risk with the generated bindings for my project I can see are layout tests not being portable. I already took care of excluding types and functions that referred to libc internals and the like, for example.

@emilio
Copy link
Contributor

emilio commented Nov 28, 2024

We allow to disable layout tests altogether don't we? Maybe generate one for the architecture you test on with tests, one elsewhere without?

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.

Feature request: compile-time layout tests
7 participants