Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Bindgen should be configured to use union + ManuallyDrop #1137

Open
workingjubilee opened this issue Jan 15, 2025 · 4 comments
Open

Bindgen should be configured to use union + ManuallyDrop #1137

workingjubilee opened this issue Jan 15, 2025 · 4 comments
Labels
• toolchain Related to `rustc`, `bindgen`, `rustdoc`, LLVM, Clippy...

Comments

@workingjubilee
Copy link

workingjubilee commented Jan 15, 2025

re: Richardhongyu/rfl_empirical_tools#1
re: https://www.usenix.org/system/files/atc24-li-hongyu.pdf

Researchers see that bindgen currently uses the __BindgenUnionField and then make logically incorrect accusations about the (in)capabilities of rustc. Accusations that nonetheless survive peer review because they were not reviewed by Rust ABI experts, precious few of those there are, because most of those people are not considered acclaimed academes. At least, not in the halls of "computer science".

These researchers should be rebutted by adjusting the configuration accordingly, or else new subissues should be filed reflecting the challenges involved in doing so against rust-bindgen and/or rustc, until this issue is resolved.

@ojeda ojeda added the • toolchain Related to `rustc`, `bindgen`, `rustdoc`, LLVM, Clippy... label Jan 16, 2025
@ojeda
Copy link
Member

ojeda commented Jan 16, 2025

Leaving aside the paper, as long as something is an improvement for the kernel, then we should consider it.

If I understand correctly, is the suggestion to use --default-non-copy-union-style manually_drop? (--rust-target is already there, and even when it wasn't, bindgen's default is the highest, so that bit should be fine already).

Enabling that indeed makes bindgen use union with ManuallyDrop for some cases, though not for all, including bpf_array:

struct bpf_array {
    union {
        struct {
            struct {} __empty_value;
            char value[];
        };
    };
};

Cc @pvdrz

@Richardhongyu
Copy link

Richardhongyu commented Jan 16, 2025

Leaving aside the paper, as long as something is an improvement for the kernel, then we should consider it.

If I understand correctly, is the suggestion to use --default-non-copy-union-style manually_drop? (--rust-target is already there, and even when it wasn't, bindgen's default is the highest, so that bit should be fine already).

Enabling that indeed makes bindgen use union with ManuallyDrop for some cases, though not for all, including bpf_array:

struct bpf_array {
union {
struct {
struct {} __empty_value;
char value[];
};
};
};
Cc @pvdrz

I found this is related to the rule of zero length.

Any union with this pattern will not use a rust union.

	union {
		DECLARE_FLEX_ARRAY(struct foo, foo_name);
		DECLARE_FLEX_ARRAY(struct bar, bar_name);
	};

@tamird
Copy link

tamird commented Feb 15, 2025

I filed rust-lang/rust-bindgen#3130 upstream.

@workingjubilee
Copy link
Author

If I understand correctly, is the suggestion to use --default-non-copy-union-style manually_drop? (--rust-target is already there, and even when it wasn't, bindgen's default is the highest, so that bit should be fine already).

Yes.

Obviously if there's a bug in bindgen's support that doesn't work out that can be fixed.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
• toolchain Related to `rustc`, `bindgen`, `rustdoc`, LLVM, Clippy...
Development

No branches or pull requests

4 participants