Re: [PATCH TOOLS 0/2] convert main() from C to Rust

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Kent Overstreet]

On Mon, Jan 15, 2024 at 12:10:22PM -0700, Thomas Bertschinger wrote:
> On Mon, Jan 15, 2024 at 01:45:22PM -0500, Kent Overstreet wrote:
> > On Mon, Jan 15, 2024 at 10:55:09AM -0700, Thomas Bertschinger wrote:
> > > I'm leaning towards keeping Make as the primary build tool for now and
> > > perhaps a future patch can move towards using Cargo. But other opinions
> > > are welcome.
> > 
> > Let's hold off on that for now.
> > 
> > I'm planning on pullling the Rust code in bch_bindgen into the kernel
> > soon, and I want to see how that interacts with the kernel build system.
> > If there's potential for commonality with the kernel build system, we
> > might want to take advantage of that.
> > 
> > (Also, if you're interested in that project, let me know! Getting that
> > ready for the next merge window is a high priority item, but I have a
> > lot of high priority items at the moment... :)
> 
> Sure, I would definitely be interested in helping out with that. I don't
> want to make any promises regarding timeline, since this combines two
> worlds--Rust, and Linux Kernel development--that are both still very
> new to me. But I can certainly give it an honest try :) as long as you,
> or someone else experienced, can help with any questions I might run
> into.

Of course :)

Join the IRC channel - irc.oftc.net#bcache, we've got quite a few people
acquainted with Rust there.

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.

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Trevor Gross]

On Mon, Jan 15, 2024 at 1:22 PM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
>
> On Mon, Jan 15, 2024 at 12:10:22PM -0700, Thomas Bertschinger wrote:
> > On Mon, Jan 15, 2024 at 01:45:22PM -0500, Kent Overstreet wrote:
> > > On Mon, Jan 15, 2024 at 10:55:09AM -0700, Thomas Bertschinger wrote:
> > > > I'm leaning towards keeping Make as the primary build tool for now and
> > > > perhaps a future patch can move towards using Cargo. But other opinions
> > > > are welcome.
> > >
> > > Let's hold off on that for now.
> > >
> > > I'm planning on pullling the Rust code in bch_bindgen into the kernel
> > > soon, and I want to see how that interacts with the kernel build system.
> > > If there's potential for commonality with the kernel build system, we
> > > might want to take advantage of that.
> > >
> > > (Also, if you're interested in that project, let me know! Getting that
> > > ready for the next merge window is a high priority item, but I have a
> > > lot of high priority items at the moment... :)
> >
> > Sure, I would definitely be interested in helping out with that. I don't
> > want to make any promises regarding timeline, since this combines two
> > worlds--Rust, and Linux Kernel development--that are both still very
> > new to me. But I can certainly give it an honest try :) as long as you,
> > or someone else experienced, can help with any questions I might run
> > into.
>
> Of course :)
>
> Join the IRC channel - irc.oftc.net#bcache, we've got quite a few people
> acquainted with Rust there.
>
> 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.

Miguel found [1] which seems to be the rustc fix but I am curious how
you work around this.

[1]: https://github.com/rust-lang/rust/issues/59154#issuecomment-1899563996

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Kent Overstreet]

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:
> >
> > On Mon, Jan 15, 2024 at 12:10:22PM -0700, Thomas Bertschinger wrote:
> > > On Mon, Jan 15, 2024 at 01:45:22PM -0500, Kent Overstreet wrote:
> > > > On Mon, Jan 15, 2024 at 10:55:09AM -0700, Thomas Bertschinger wrote:
> > > > > I'm leaning towards keeping Make as the primary build tool for now and
> > > > > perhaps a future patch can move towards using Cargo. But other opinions
> > > > > are welcome.
> > > >
> > > > Let's hold off on that for now.
> > > >
> > > > I'm planning on pullling the Rust code in bch_bindgen into the kernel
> > > > soon, and I want to see how that interacts with the kernel build system.
> > > > If there's potential for commonality with the kernel build system, we
> > > > might want to take advantage of that.
> > > >
> > > > (Also, if you're interested in that project, let me know! Getting that
> > > > ready for the next merge window is a high priority item, but I have a
> > > > lot of high priority items at the moment... :)
> > >
> > > Sure, I would definitely be interested in helping out with that. I don't
> > > want to make any promises regarding timeline, since this combines two
> > > worlds--Rust, and Linux Kernel development--that are both still very
> > > new to me. But I can certainly give it an honest try :) as long as you,
> > > or someone else experienced, can help with any questions I might run
> > > into.
> >
> > Of course :)
> >
> > Join the IRC channel - irc.oftc.net#bcache, we've got quite a few people
> > acquainted with Rust there.
> >
> > 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.

packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)

[Thomas Bertschinger]

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

Re: packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)

[Kent Overstreet]

On Sun, Jan 21, 2024 at 09:11:24AM -0700, Thomas Bertschinger wrote:
> 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)

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.

Also, note: the differing alignment isn't a correctness issue, but it is
definitely a performance issue on arm - the compiler will then have to
generate code to handle misaligned loads and stores.

> 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?

Agreed. It'd be great to have an easy way of checking member offsets as
well, like we were discussing on IRC, but I don't think it's necessary.

> 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.)

That feels right to me; and if we can get your test committed I won't be
too concerned about getting a formal proof :)

> 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.

*nod*

> 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

Thanks for the writeup, this really helps.

Agreed that if at all possible we should aim for better support from
rustc, and forwarding this to the rustc people might help get this the
necessary attention before we dive in with patching bindgen.

Another thought - the interface between bindgen and rustc feels a bit
fragile here, since the C attributes and Rust reprs don't exactly match.
Can we come up with something better?

I wonder if we could get support in rustc for specifying the offset of
each member directly?

Also - you tracked down the difference between microsoft and gcc
__attribute__((align)), let me try to recap (and tell me if I get it
wrong): with gcc, __align() works like any naturally aligned type, but
Microsoft's version overrides __packed on a containing structure.

I think there's a strong argument to be made that Microsoft is the weird
one here and Rustc should just provide the gcc behaviour when mixing
packed and align...

Re: packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)

[Thomas Bertschinger]

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...

Enhancing bindgen to do something like this automatically also sounds
like a good idea but it will take me longer to figure out how to do
that. I think it could also be good to incorporate explicit member
offset checks with this. Handling that with an awk script or similar
seems challenging to me with how bindgen mangles bitfield names.

> Also - you tracked down the difference between microsoft and gcc
> __attribute__((align)), let me try to recap (and tell me if I get it
> wrong): with gcc, __align() works like any naturally aligned type, but
> Microsoft's version overrides __packed on a containing structure.
> 
> I think there's a strong argument to be made that Microsoft is the weird
> one here and Rustc should just provide the gcc behaviour when mixing
> packed and align...

I don't have access to a Microsoft environment to test anything, so
everything that I think I know about Microsoft semantics is based on what
I've read on various rust-lang threads, like [1][2]

With that disclaimer, from what I can tell it seems like Microsoft's
semantics are strictly less expressive than gcc's. I think gcc can
represent any struct that Microsoft can (adding padding by hand
may be necessary in some cases, but it's always possible), whereas
there are structures representable in gcc that cannot be represented
with the Microsoft semantics as presented in the cited rust-lang
threads. (Given that I haven't tested the MS compiler, I could be
totally wrong about this.)

[1] https://github.com/rust-lang/rust/issues/59154#issuecomment-476408300
[2] https://github.com/rust-lang/rust/issues/33158

- Thomas Bertschinger

Re: packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)

[Thomas Bertschinger]

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