Re: [PATCH BCACHEFS-TOOLS] use upstream bindgen; fix packed and aligned types

[Martin Rodriguez Reboredo <yakoyoku@gmail.com>]

On 1/22/24 11:36, Thomas Bertschinger wrote:
> bcachefs-tools has been using a patched bindgen to work around a
> limitation of rustc that prevents compiling structs with
> both #[repr(packed(N)] and #[repr(align(N)] attributes. The patch:
> e8168ceda507 "codegen: Don't generate conflicting packed() and align()
> representation hints." discards the "align" attribute in cases where
> bindgen produces a type with both.
> 
> This may be correct for some types, but it turns out that for each
> bcachefs type with this problem, keeping the "align" attribute and
> discarding the "packed" attribute generates a type with the same ABI as
> the original C type.
> 
> This can be tested automatically by running:
> $ cargo test --manifest-path bch_bindgen/Cargo.toml
> in the bcachefs-tools tree.
> 
> There has been pressure recently to start using upstream bindgen; both
> externally, from distribution maintainers who want to build
> bcachefs-tools with standard dependencies, and internally, in order to
> enable using Rust for bcachefs in-kernel.
> 
> This patch updates bcachefs-tools to use upstream bindgen. It works
> around the rustc limitation with a post-processing step in the bindgen
> build that adjusts the attributes to include "#[repr(C, align(N))]" and
> exclude #[repr(packed(N)] only for the 4 types that need it.
> 
> Some types that had been manually implemented in
> bch_bindgen/src/bcachefs.rs are now automatically generated by bindgen,
> so that they will be covered by the ABI compatibility testing mentioned
> above.
> 
> I intentionally targeted the post-processing to the exact 4 types with
> the issue currently, so that any changes to bcachefs that result in this
> issue appearing for a new type will require manual intervention. I
> figured any such changes should require careful consideration.
> 
> Ideally, bindgen can be updated to handle situations where "align(N)"
> is needed and "packed(N)" can be safely discarded. If a patch for this
> is accepted into bindgen, the post-processing hack can be removed.
> 
> Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
> ---
> [...]

You really got lucky that those types didn't require packing in
Rust. LGTM.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>