From: Thomas Bertschinger <tahbertschinger@gmail.com>
To: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Trevor Gross <tmgross@umich.edu>,
linux-bcachefs@vger.kernel.org, bfoster@redhat.com,
Miguel Ojeda <ojeda@kernel.org>,
rust-for-linux@vger.kernel.org,
Benno Lossin <benno.lossin@proton.me>
Subject: packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)
Date: Sun, 21 Jan 2024 09:11:24 -0700 [thread overview]
Message-ID: <20240121161124.GA151023@fedora-laptop> (raw)
In-Reply-To: <gpokpnqgj6c2pcmtpiu46jy3kb3tom4xkp3fykg26zx6e3c2qe@zrnl2653qwsk>
On Fri, Jan 19, 2024 at 04:37:51PM -0500, Kent Overstreet wrote:
> On Fri, Jan 19, 2024 at 01:05:17PM -0600, Trevor Gross wrote:
> > On Mon, Jan 15, 2024 at 1:22 PM Kent Overstreet
> > <kent.overstreet@linux.dev> wrote:
> > > The main thing that needs to be sorted out is that we require a patched
> > > version of bindgen (since rustc can't yet handle types that are both
> > > packed and aligned); we need to talk to the rust-for-linux people about
> > > whether they'll be ok with switching the kernel to the patched bindgen
> > > until we can get a proper fix into rustc.
> >
> > Do you have a link to the patches needed? We are wondering if this is
> > something that could be upstreamed.
>
> The current workaround is to just drop #[repr(align)] if the type is
> both packed and aligned - but that leads to rustc and gcc disagreeing on
> the memory layout of certain types; it only works as long as we're not
> mutating types where this matters from rust code.
I made a script to compare the size and alignment of bcachefs structs
in C vs. in Rust generated by the patched, lossy bindgen. All sizes
were the same, but the following types had different alignment:
bkey (8 in C, 4 in Rust)
bch_extent_crc32 (8 in C, 4 in Rust)
bch_extent_ptr (8 in C, 1 in Rust)
I didn't explicitly compare the offsets of all struct members between
Rust and C as I couldn't come up with a trivial way to automate this.
However, I think that if the C struct is packed, and the Rust and C
structs have the same size, you can conclude that all member offsets
are the same. This is because Rust could not increase the offset of a
member (or else the Rust size would be larger), it could not decrease
the offset of a member (because it's already packed), and it could not
reorder the members (because #[repr(C)] prohibits this in Rust).
Given the above logic, I think it's safe to conclude that if a struct
is packed in C, and the size and alignment are the same between C and
Rust, then they are fully ABI compatible. Does anyone see an error in
my reasoning?
Taking a look at struct bkey, the alignment difference arises from a
semantic difference between gcc's __attribute__((packed, aligned(N)))
and rustc's #[repr(C, packed(N))]. In Rust,
> By specifying this attribute, the alignment of the struct would be
> the smaller of the specified packing and the default alignment of the
> struct. [1]
We can get the same alignment for struct bkey in Rust by modifying
the code from bindgen to use align(8) instead of packed(8). On my
x86_64 Linux system, this results in a struct with the same size,
alignment, and offsets of all members as the original C struct. This
works because the struct happens to be packed already, so the
#[repr(packed)] attribute is superfluous. I can't say if this works in
any other environment with different architecture, endianness, etc. But
introducing an automated test for comparing the size and alignment of
bindgen-generated structs could help with validating other environments.
In general, given a C structure like
struct A {
...
} __packed __aligned(N);
I think a Rust structure can be created with the same size, alignment,
and member offsets in the following cases:
(1) #[repr(packed(N))] will have the same layout if N is <= the
structure's default alignment and all members with a default
alignment >= N naturally have an offset that is a multiple of N.
Also, no member can have an alignment attribute as rustc forbids
this [2].
(2) #[repr(align(N))] will have the same layout if the structure's size
without the packed attribute is equal to the sum of the size of all
its members (i.e., it is "naturally packed"), and the structure's
default alignment is <= N.
(I haven't formally proven these rules and I welcome counterexamples if
someone can spot an error in my logic.)
An example of a struct that I think the current rustc cannot represent:
struct rust_cannot_represent_this {
u8 a;
u32 b;
} __packed __aligned(2);
Luckily, I do not think bcachefs has any structs like this.
TL;DR -- even without a rustc fix to support packed and aligned structs,
I think it is possible in many cases to create Rust structs with the
same memory layout, although it may take some manual effort. Perhaps
bindgen could be enhanced with awareness of rule (2) from above, to
automatically cover more cases than it does now?
Of course, a fix to rustc is the best option. It would avoid the need
for error-prone manual work. Specifying both #[repr(packed, align(N))]
also communicates semantic intent more than specifying just one even
when the result is the same. But we could be waiting a while for that
fix to arrive.
[1] https://rust-lang.github.io/rfcs/1399-repr-pack.html
[2] https://github.com/rust-lang/rust/issues/100743
- Thomas Bertschinger
next prev parent reply other threads:[~2024-01-21 16:11 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240115052451.145611-1-tahbertschinger@gmail.com>
[not found] ` <pawxsqxbjncrwtxytattvgizwrtmkis6suokgkb6wfm5xvsnhd@njjz4ywheu2a>
[not found] ` <20240115175509.GA156208@fedora-laptop>
[not found] ` <iiraqelv6wtwxcdf2yjtjt26ghejngfantjx7d4mztav27qu7y@gmiztxamveq3>
[not found] ` <20240115191022.GC156208@fedora-laptop>
2024-01-15 19:22 ` [PATCH TOOLS 0/2] convert main() from C to Rust Kent Overstreet
2024-01-19 19:05 ` Trevor Gross
2024-01-19 21:37 ` Kent Overstreet
2024-01-21 16:11 ` Thomas Bertschinger [this message]
2024-01-21 18:19 ` packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust) Kent Overstreet
2024-01-21 19:32 ` Thomas Bertschinger
2024-01-22 2:47 ` Thomas Bertschinger
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=20240121161124.GA151023@fedora-laptop \
--to=tahbertschinger@gmail.com \
--cc=benno.lossin@proton.me \
--cc=bfoster@redhat.com \
--cc=kent.overstreet@linux.dev \
--cc=linux-bcachefs@vger.kernel.org \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=tmgross@umich.edu \
/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).