Rust dependency updates & Debian

[paravoid]

bcachefs-tools in Debian disables the Rust parts, due to a few missing crates from Debian. I just reported this to the Debian maintainer (Debian #1060256), and looked a little bit into what it would take to bring this all to Debian. Most of the work is already done, and the bits remaining is on them, but perhaps you as upstream can help with a few parts?

1. Cargo.toml specifies a dependency on the parse-display crate. As far as I can tell, that's not really used and is thus a spurious dependency? (This matters for Debian, because it's not currently packaged).
2. While looking into (1), I also tried cargo-machete, which (no idea if correct) also identified:

bcachefs-rust -- /tmp/bcachefs-tools/rust-src/Cargo.toml:
	chrono
	getset
	parse-display
bch_bindgen -- /tmp/bcachefs-tools/rust-src/bch_bindgen/Cargo.toml:
	chrono
	colored
	gag
	libc
	udev

3. Debian ships with rpassword v7.3.1. There is an API change, that I believe should be:

-        rpassword::read_password_from_tty(Some("Enter passphrase: "))?
+        rpassword::prompt_password("Enter passphrase: ")?

4. Debian ships with newer versions of clap_complete (4.3.2 -> 4.4.3), itertools (0.9 -> 0.10), errno (0.2 -> 0.3), and we would package the latest version of udev (0.7.0 -> 0.8.0). Changing Cargo.toml to these versions seemed to result in a successful build, but I haven't tested this from a functional perspective at all.
5. Debian ships with older versions of memoffset (0.8.0 -> 0.6.5), paste (1.0.11 -> 1.0.7). Smoke-tested as above.
6. I see that bch_bindgen depends on a fork of rust-bindgen. As far as I can tell, the only difference is a cherry-pick of __attribute__((aligned(2), packed)) generates both repr(packed) and repr(aligned) rust-lang/rust-bindgen#2240, which I've just bumped. Perhaps we can cherry-pick that in Debian -- let me know if that will not be sufficient.

[koverstreet]

A patch for this stuff would be appreciated.

That bch_bindgen patch should not be pulled into Debian - the patch results in silent miscompilation, since rustc doesn't actually support packed and aligned structs - that needs to be fixed in rustc.

[paravoid]

Thanks for the quick response!

A patch for this stuff would be appreciated.

#203, #204, #205. I may or may not know what I'm doing, please review with care :)

That bch_bindgen patch should not be pulled into Debian - the patch results in silent miscompilation, since rustc doesn't actually support packed and aligned structs - that needs to be fixed in rustc.

Ah, I just read that issue more closely. That's unfortunate :/ Any ideas on how to workaround this? It'd be quite the hard sell to ship a fork of bindgen in Debian...

[koverstreet]

This is the workaround - I realize it's not ideal for you guys, but it's a tricky situation and it needs to be addressed correctly.

Here's the bug that needs to be addressed to handle this properly: rust-lang/rust#59154 (comment)

[paravoid]

This is the workaround - I realize it's not ideal for you guys, but it's a tricky situation and it needs to be addressed correctly.

Here's the bug that needs to be addressed to handle this properly: rust-lang/rust#59154 (comment)

Thanks, that's helpful! From the bug report you linked and associated ones, it doesn't seem there is consensus on whether this constitutes a problem or what a fix could look like... I'm concerned that by the time this gets fixed (if at all), rolled out in a new rustc version, then that new version making it to Debian etc., it's going to be a few years.

It'd be nice to not get stuck with no mount.bcachefs in Debian for years to come, so I'm hoping for some other kind of workaround!

Looking at the generated bindgen code, it looks like the difference is effectively just one occurence of:

@@ -5924,7 +5924,6 @@
     );
 }
 #[repr(C, packed(8))]
-#[repr(align(8))]
 #[derive(Debug, Default, Copy, Clone)]
 pub struct bkey {
     pub u64s: __u8,

This may sound (very) dirty, but perhaps a way to address this would be for build.rs to postprocess the output with a .replace("#[repr(C, packed(8))]\n#[repr(align(8))]", "#[repr(C, packed(8))]")?

[xhebox]

.replace("#[repr(C, packed(8))]\n#[repr(align(8))]", "#[repr(C, packed(8))]")

I need a very new rust-bindgen since I am using clang17. Thus I reverted that bch_bindgen patch. Then I need a new method to address this issue, and I can confirm that your suggestion works:

@@ -92,8 +92,8 @@
         .parse_callbacks(Box::new(Fix753 {}))
         .generate()
         .expect("BindGen Generation Failiure: [libbcachefs_wrapper]");
-    bindings
-        .write_to_file(out_dir.join("bcachefs.rs"))
+    std::fs::write(out_dir.join("bcachefs.rs"),
+         bindings.to_string().replace("#[repr(C, packed(8))]\n#[repr(align(8))]", "#[repr(C, packed(8))]"))
         .expect("Writing to output file failed for: `bcachefs.rs`");

     let keyutils = pkg_config::probe_library("libkeyutils").expect("Failed to find keyutils lib");

[koverstreet]

So, to reiterate: simply stripping out the align() directive is incorrect, since it causes Rust and C to generate types with different sizes - the align means sizeof() will be rounded up to a multiple of the alignment.

However - there is a workaround we could do in bindgen - bindgen could insert padding bytes when required.

This won't be as good as actually getting support for #[repr(align)] and #[repr(packed)] into rustc, since with the align directive the compiler will know that some members won't be misaligned (or will be misaligned in more restricted ways), and can generate better code.

But it is a workaround that wouldn't result in miscompiling code.

[paravoid]

With 7717a43 having been merged (thanks @bertschingert!), I think everything mentioned here has been resolved. Thanks everyone!