Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Add option to generate safe and unsafe conversions for rustified enums #2908

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add option to generate safe and unsafe conversions for rustified enums #2908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bindgen-integration/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ fn setup_macro_test() {
.enable_cxx_namespaces()
.default_enum_style(EnumVariation::Rust {
non_exhaustive: false,
safe_conversion: false,
unsafe_conversion: false,
})
.raw_line("pub use self::root::*;")
.raw_line("extern { fn my_prefixed_function_to_remove(i: i32); }")
Expand Down
100 changes: 100 additions & 0 deletions bindgen-tests/tests/expectations/tests/issue-2646.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions bindgen-tests/tests/headers/issue-2646.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// bindgen-flags: --rustified-enum 'Plain.*' --rustified-enum 'TryFromRaw.*=try_from_raw' --rustified-enum='FromRawUnchecked.*=from_raw_unchecked' --rustified-enum='Both.*=try_from_raw,from_raw_unchecked' --rustified-enum 'NonExhaustive.*=non_exhaustive'

enum Plain {
Plain1,
Plain2,
Plain3
};

enum TryFromRaw {
TFR1 = -1,
TFR2 = 5,
TFR3
};

enum FromRawUnchecked {
FRU1 = 6,
FRU2 = 10,
FRU3 = 11,
};

enum Both {
Both1,
Both2 = -1,
Both3,
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a good idea to add a few functions here, e.g. enum Plain f(enum Plain); and so on, to double-check the generated type in expectations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, it appears after adding some tests that it is converting it to the type that will result in undefined behavior. I'll have to take a look at how to avoid that. I think the code to handle that is in a place I haven't touched, so I'll have to track down where that's happening.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I feared, yeah. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So is what you're recommending only doing this in the case of TryFrom generation like with the constants?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean -- do you have an example?

Copy link
Contributor

@pvdrz pvdrz Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, one alternative to roll this change slowly would be to add another flag like --use-enum-ctypes-on-fns or --return-enum-ctypes (name is a WIP 🤣) that when enabled, makes all functions return the ctype instead of the rustified enum. We could keep it disabled by default and eventually enable it by default as unsoundness is a pretty good basis for it.

@jbaublitz @ojeda what do you think?

Copy link
Contributor

@ojeda ojeda Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've been thinking something: Let's say we have two functions (...)

How would a user call takes_plain with a value that is not in the Rust-side enum? i.e. a C function may need to be used with more values too. Of course, perhaps the bindgen user should avoid this kind of enum in this case.

Copy link
Contributor

@pvdrz pvdrz Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would a user call takes_plain with a value that is not in the Rust-side enum? i.e. a C function may need to be use with more values too.

I think the only case where a Rust user would call takes_plain with a value outside the rustified enum would be if there is a common pattern in the C library like:

// This value could be outside of `Plain`
enum Plain value = returns_plain();
// But that's OK because this function already assumes that this could happen 
// and preemptively checks that internally
takes_plain(value);

So doing that from the rust side would be weird as you'd have:

unsafe extern "C" {
   fn returns_plain() -> Plain_ctype;
   fn takes_plain(plain: Plain);
}
// This value could be outside of `Plain`
let value: Plain_ctype = unsafe { returns_plain() };
// But here, we have to do an unsafe transmute that might cause UB 
// even though the call to the C function would be fine.
unsafe { takes_plain(std::mem::transmute(value)) };

However, if you just use the Plain_ctype everywhere, that's not cumbersome anymore

unsafe extern "C" {
   fn returns_plain() -> Plain_ctype;
   fn takes_plain(plain: Plain_ctype);
}
// This value could be outside of `Plain`
let value: Plain_ctype = unsafe { returns_plain() };
// No weird transmute
unsafe { takes_plain(value) };

Of course, perhaps the bindgen user should avoid this kind of enum in this case.

Yes maybe in that case you'd be better not using --rustified-enum at all. However, I could see a case where you'd want to offer both the rustified enum and safe wrappers to those functions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(My previous comment was intended to reply to your "two functions", rather than the --use-enum-ctypes-on-fns one; our messages crossed :)

However, if you just use the Plain_ctype everywhere, that's not cumbersome anymore

Exactly, that is why I think using the Rust enum for parameters may not be great, unless the expectation is that users avoid it in some cases. So they would probably still not use it as their "default" or "catch all" case, i.e. --... *.

However, I could see a case where you'd want to offer both the rustified enum and safe wrappers to those functions.

Hmm... What do you mean by safe wrappers? i.e. the raw functions would still need to be marked unsafe, even if we took Rust enums, so the user would still need to wrap it.

Having said that, I would like to eventually have a [[safe]] attribute on the C side that allows us to do things like that -- for the kernel for instance, we are now at the point were starting to add annotations on the C side for several things may be OK for some subsystems at least. Similarly, we could have a [[enum ...]] thing that gives bindgen this information without having to maintain the lists outside the source code. I don't know if nowadays libclang preserves unknown attributes -- if so, we could already do it without having to modify and wait for Clang.

The other extreme is to have an "easy-to-use" mode, where by default all is provided: the raw C type and its constants, plus a Rustified enum type, plus a non-exhaustive one, plus the conversion functions (fallible, infallible and unsafe) to convert to both of those... Then users can use whatever they think it is best for each enum, without having to know about the bindgen kinds, and it is sound. Then when they are sure how they use each enum, they can restrict it to one kind via the arguments (or via attributes if we ever get there) so that they avoid bindgen generating code that will not be needed. So it would be easy to start using, and allow people to customize later. But it would be more "expensive" by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(My previous comment was intended to reply to your "two functions", rather than the --use-enum-ctypes-on-fns one; our messages crossed :)

However, if you just use the Plain_ctype everywhere, that's not cumbersome anymore

Exactly, that is why I think using the Rust enum for parameters may not be great, unless the expectation is that users avoid it in some cases. So they would probably still not use it as their "default" or "catch all" case, i.e. --... *.

Yeah I think that the extra convenience of using the rustified enum would only work if you want to expose the enum itself. And if you want to expose safe bindings for functions that use the c enum, then you have to do those yourself accordingly to the invariants of the library itself.

However, I could see a case where you'd want to offer both the rustified enum and safe wrappers to those functions.

Hmm... What do you mean by safe wrappers? i.e. the raw functions would still need to be marked unsafe, even if we took Rust enums, so the user would still need to wrap it.

That's true, even if the function takes the rustified enum then you still have to figure out its safety requirements and expose it accordingly. So you'd still have to do things like

mod bindings {
    unsafe extern "C" {
        pub fn takes_plain(plain: Plain);
    }
}

/// Safety: It is safe, believe me.
pub fn takes_plain(plain: Plain) {
    unsafe { bindings::takes_plain(plain) }
}

So, having to add an extra plain.to_raw_ctype() or whatever to obtain the C value seems like little effort. Maybe @emilio or @kulp have a better idea on why bindings of functions that take C enums become functions that take Rustified enums instead of using the integer type directly as they have been around for longer.

Having said that, I would like to eventually have a [[safe]] attribute on the C side that allows us to do things like that -- for the kernel for instance, we are now at the point were starting to add annotations on the C side for several things may be OK for some subsystems at least. Similarly, we could have a [[enum ...]] thing that gives bindgen this information without having to maintain the lists outside the source code. I don't know if nowadays libclang preserves unknown attributes -- if so, we could already do it without having to modify and wait for Clang.

That sounds like a sensible addition. In the meantime, it is already possible to embed html-like tags in the comments of the headers and bindgen can read those to introduce custom behavior.

The other extreme is to have an "easy-to-use" mode, where by default all is provided: the raw C type and its constants, plus a Rustified enum type, plus a non-exhaustive one, plus the conversion functions (fallible, infallible and unsafe) to convert to both of those... Then users can use whatever they think it is best for each enum, without having to know about the bindgen kinds, and it is sound. Then when they are sure how they use each enum, they can restrict it to one kind via the arguments (or via attributes if we ever get there) so that they avoid bindgen generating code that will not be needed. So it would be easy to start using, and allow people to customize later. But it would be more "expensive" by default.

Yeah ideally you should be able to decide if a C enum should be translated in different ways without interference between each representation. I think the API would need some bike-shedding, specially from the CLI side where there isn't much flexibility. Maybe the idea of having a bindgen.toml file where we can store more structured information would be useful here as you could do something like

[[enum_style]]
regex = "Foo"
rustified = { generate = true, try_from_raw = true, from_raw_unchecked = true }
constified = true

Then anything that matches the Foo regex would get all of those immediately. Maybe a CLI equivalent could be --enum-style "Foo=rustified(try_from_raw,from_raw_unchecked),constified"

It would also be nice to be able to replace the multitude of --newtype-enum --newtype-global-enum, --constified-enum, etc, by a single option that can be extended. This would also give @jbaublitz some flexibility as it wouldn't be necessary to include the raw type constants with this feature as those would be enabled automatically if you pass the extra constified style.

enum NonExhaustive {
Ex1,
Ex2,
};
Loading
Loading