From: Thomas Bertschinger <tahbertschinger@gmail.com>
To: kent.overstreet@linux.dev, linux-bcachefs@vger.kernel.org,
bfoster@redhat.com
Cc: Thomas Bertschinger <tahbertschinger@gmail.com>
Subject: [PATCH] bch_bindgen: fix packed and aligned structs on i686, ppc64
Date: Sat, 17 Feb 2024 11:20:25 -0700 [thread overview]
Message-ID: <20240217182025.10698-1-tahbertschinger@gmail.com> (raw)
This patch addresses build issues with bch_bindgen on architectures
where the natural alignment of u64 is 4 instead of 8, such as i686 and
ppc64. The issue arises from rustc's refusing to compile structs with a
"packed" attribute that have members with an explicit "align(N)"
attribute.
rust-bindgen generally does not place "align(N)" attributes on types
when it is redundant with the type's natural alignment. There are a few
types in bcachefs with "__aligned(8)" in C where the type's natural
alignment is 8 except on architectures like those mentioned above where
it is 4. rust-bindgen places the "align(8)" attribute on these types,
for those architectures. The affected types include:
- bch_csum
- bch_sb_layout
- bch_ioctl_data_progress
bch_ioctl_data_progress, and all types that depend on it, are not used
on the Rust side currently and bindings are only generated for them
because they are covered by `.allowlist_type("bch_.*")` in
bch_bindgen/build.rs. This patch resolves the build failure for this
type by excluding it from bch_bindgen. If/when accessing this type in
Rust is needed, a decision can be made then about how to represent its
layout in Rust.
bch_csum and bch_sb_layout, and types that depend on them, are currently
used in Rust so they can't be excluded. This patch addresses these types
by stripping off the "packed" attribute in Rust on types that embed
these types, since that attribute happens to not be necessary for proper
layout.
Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
---
As noted in the TODO item in the comments below, it would be nice to
make arch-specific modifications only occur when compiling for that
target arch. This isn't strictly required: the layout of the affected
types remains correct on x86_64 and aarch64 (this can be verified by
running `cargo test --manifest-path bch_bindgen/Cargo.toml).
It is likely possible to do this by referencing some enviroment
variables set by Cargo for the target arch, but I want to test that more
fully before I submit a patch that adds that in. In the meantime, this
patch as-is should hopefully resolve the build failures and hopefully
the impact to already working arches is acceptable.
bch_bindgen/build.rs | 45 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 37 insertions(+), 8 deletions(-)
diff --git a/bch_bindgen/build.rs b/bch_bindgen/build.rs
index 1baa266..0329f1a 100644
--- a/bch_bindgen/build.rs
+++ b/bch_bindgen/build.rs
@@ -64,6 +64,7 @@ fn main() {
.allowlist_function("printbuf.*")
.blocklist_type("rhash_lock_head")
.blocklist_type("srcu_struct")
+ .blocklist_type("bch_ioctl_data.*")
.allowlist_var("BCH_.*")
.allowlist_var("KEY_SPEC_.*")
.allowlist_var("Fix753_.*")
@@ -121,14 +122,42 @@ fn main() {
}
// rustc has a limitation where it does not allow structs with a "packed" attribute to contain a
-// member with an "align(N)" attribute. There is one type in bcachefs with this issue:
-// struct btree_node.
+// member with an "align(N)" attribute. There are a few types in bcachefs with this problem. We can
+// "fix" these types by stripping off "packed" from the outer type, or "align(N)" from the inner
+// type. For all of the affected types, stripping "packed" from the outer type happens to preserve
+// the same layout in Rust as in C.
//
-// Luckily, it happens that this type does not need the "packed(N)" attribute in Rust to have the
-// same ABI as C, so we can strip it off the bindgen output.
+// Some types are only affected on attributes on architectures where the natural alignment of u64
+// is 4 instead of 8, for example i686 or ppc64: struct bch_csum and struct bch_sb_layout have
+// "align(8)" added on such architecutres. These types are included by several "packed" types:
+// - bch_extent_crc128
+// - jset
+// - btree_node_entry
+// - bch_sb
+//
+// TODO: find a way to conditionally include arch-specific modifications when compiling for that
+// target arch. Regular conditional compilation won't work here since build scripts are always
+// compiled for the host arch, not the target arch, so that won't work when cross-compiling.
fn packed_and_align_fix(bindings: std::string::String) -> std::string::String {
- bindings.replace(
- "#[repr(C, packed(8))]\npub struct btree_node {",
- "#[repr(C, align(8))]\npub struct btree_node {",
- )
+ bindings
+ .replace(
+ "#[repr(C, packed(8))]\npub struct btree_node {",
+ "#[repr(C, align(8))]\npub struct btree_node {",
+ )
+ .replace(
+ "#[repr(C, packed(8))]\n#[derive(Debug, Default, Copy, Clone)]\npub struct bch_extent_crc128 {",
+ "#[repr(C, align(8))]\n#[derive(Debug, Default, Copy, Clone)]\npub struct bch_extent_crc128 {",
+ )
+ .replace(
+ "#[repr(C, packed(8))]\npub struct jset {",
+ "#[repr(C, align(8))]\npub struct jset {",
+ )
+ .replace(
+ "#[repr(C, packed(8))]\npub struct btree_node_entry {",
+ "#[repr(C, align(8))]\npub struct btree_node_entry {",
+ )
+ .replace(
+ "#[repr(C, packed(8))]\npub struct bch_sb {",
+ "#[repr(C, align(8))]\npub struct bch_sb {",
+ )
}
--
2.43.0
reply other threads:[~2024-02-17 18:20 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240217182025.10698-1-tahbertschinger@gmail.com \
--to=tahbertschinger@gmail.com \
--cc=bfoster@redhat.com \
--cc=kent.overstreet@linux.dev \
--cc=linux-bcachefs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).