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: Re: packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)
Date: Sun, 21 Jan 2024 19:47:11 -0700 [thread overview]
Message-ID: <20240122024711.GC151023@fedora-laptop> (raw)
In-Reply-To: <20240121193253.GB151023@fedora-laptop>
On Sun, Jan 21, 2024 at 12:32:53PM -0700, Thomas Bertschinger wrote:
> On Sun, Jan 21, 2024 at 01:19:24PM -0500, Kent Overstreet wrote:
> > On Sun, Jan 21, 2024 at 09:11:24AM -0700, Thomas Bertschinger wrote:
> > > 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:
> > >
> > That right there is really good news. If we can add that script to the
> > tests in bcachefs-tools, we'll already be in better shape than we were.
> >
> > I wonder if it would be possible to upstream that check into bindgen.
>
> I did this with an awk script that grabs the structs from the bcachefs
> C headers and outputs code in Rust and C to dump the sizes and
> alignments. I then manually added calls to the generated functions into
> the bcachefs utility and diffed the output. I'll try to get this into a
> form more suitable for an automated test, and hopefully submit a patch
> to bcachefs-tools soon...
So after looking into bindgen more... it turns out that bindgen already
does this! All you have to do is run "cargo test" from the
bcachefs-tools/bch_bindgen/ directory.
Reading the documentation helps :)
The bindgen tests basically agree with my findings, that struct bkey
gets the wrong alignment (4 instead of 8). There's one other problem
that follows from this, btree_iter.k (a struct bkey) gets the wrong
offset.
Manually adjusting struct bkey to use #[repr(align(8))] as described
previously fixes both of these problems.
The other problem types mentioned earlier, bch_extent_crc32 and
bch_extent_ptr, are handled manually outside of bindgen so they don't
show up in the bindgen test. However, they can be moved under bindgen
control and fixed with the same technique, using align(8) instead of
packed(8).
This works immediately for bch_extent_crc32 but bch_extent_ptr is a
little trickier. It is a member of struct btree_node which has a
"packed" attribute so we run into rustc disallowing embedding aligned
types within packed types. Luckily, replacing the packed(8) attribute
on struct btree_node with align(8) appears unproblematic.
Given all that, I think it should be possible to move forward with an
unpatched bindgen as long as we're OK with treating a few types
specially. We can move all of the above types under bindgen control (so
that they are covered by the automatic tests) and post-process the
bindgen output, as suggested in this comment [1], and use align(8)
instead of packed(8) for these types.
What do you think?
[1] https://github.com/koverstreet/bcachefs-tools/issues/202#issuecomment-1886962791
- Thomas Bertschinger
prev parent reply other threads:[~2024-01-22 2:47 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 ` packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust) Thomas Bertschinger
2024-01-21 18:19 ` Kent Overstreet
2024-01-21 19:32 ` Thomas Bertschinger
2024-01-22 2:47 ` Thomas Bertschinger [this message]
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=20240122024711.GC151023@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).