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
