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.

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

Allow wrapper function generation for functional macros #2578

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Abestanis
Copy link

@Abestanis Abestanis commented Jul 7, 2023

This allows to register functional macros for which a wrapper function will be generated. Currently it is not possible to call functional macros at all. This makes it possible.

Macros don't have a defined argument or return types, which is why they have to be specified explicitly. Some macros can be called with different argument types. This patch only supports creating a wrapper function for one specific set of argument and return types. Wrappers are only generated for configured macros, all others are ignored.

Example

With this change we can expose the following C macros:

#define MACRO_WITH_NO_ARGUMENTS printf("Test")
#define MACRO_WITH_NO_ARGUMENTS_AND_RETURN_VALUE() 0
#define MACRO_WITH_ONE_ARGUMENT_AND_RETURN_VALUE(value) ((float) (value))
#define MACRO_WITH_TWO_ARGUMENTS_AND_RETURN_VALUE(condition, value) ((condition) ? ((float) (value)) : 0)

To generate wrappers for these macros, we have to eiter configure them via command line arguments:

--macro-function "MACRO_WITH_NO_ARGUMENTS"
--macro-function "u8 MACRO_WITH_NO_ARGUMENTS_AND_RETURN_VALUE"
--macro-function "f32 MACRO_WITH_ONE_ARGUMENT_AND_RETURN_VALUE(u32)"
--macro-function "f32 MACRO_WITH_TWO_ARGUMENTS_AND_RETURN_VALUE(bool, u32)"

… or via the Builder:

Builder::default()
     .macro_function("MACRO_WITH_NO_ARGUMENTS", FunctionType::new::<(), ()>())
     .macro_function("MACRO_WITH_NO_ARGUMENTS_AND_RETURN_VALUE", FunctionType::new::<u8, ()>())
     .macro_function("MACRO_WITH_ONE_ARGUMENT_AND_RETURN_VALUE", FunctionType::new::<f32, u32>())
     .macro_function("MACRO_WITH_TWO_ARGUMENTS_AND_RETURN_VALUE", FunctionType::new::<f32, (bool, u32)>())

On the Rust side we can then call the macros as normal functions:

unsafe {
    ffi::MACRO_WITH_NO_ARGUMENTS();
    let value = ffi::MACRO_WITH_NO_ARGUMENTS_AND_RETURN_VALUE();
    let value = ffi::MACRO_WITH_ONE_ARGUMENT_AND_RETURN_VALUE(42);
    let value = ffi::MACRO_WITH_TWO_ARGUMENTS_AND_RETURN_VALUE(true, 42);
}

@Abestanis
Copy link
Author

r? @emilio

@Abestanis Abestanis force-pushed the feature/function_macros branch 2 times, most recently from 39eb5fd to f8c06fd Compare July 7, 2023 14:21
@bors-servo
Copy link

☔ The latest upstream changes (presumably f44a665) made this pull request unmergeable. Please resolve the merge conflicts.

@Abestanis Abestanis force-pushed the feature/function_macros branch 3 times, most recently from e7f5b7f to 3e82041 Compare July 11, 2023 13:31
@bors-servo
Copy link

☔ The latest upstream changes (presumably 251dec9) made this pull request unmergeable. Please resolve the merge conflicts.

@pvdrz
Copy link
Contributor

pvdrz commented Jul 18, 2023

I like this feature but I think it needs more bikeshedding.

One issue I see is that having to write the name of the macro itself and the type makes it dangerously close to just creating a wrapper by hand.

If I had a macro in input.h:

#define INC(value) (value + 1)

I could very well create a wrapper.h with these contents:

#include "input.h"

int inc(int val) {
    return INC(val)
}

And pass it to bindgen. You could argue that just having to do --macro-function="int INC(int)" is shorter but it doesn't feel that much shorter to be honest and the amount of complexity adding to bindgen to do it doesn't feel like a good trade-off to me.

@Abestanis
Copy link
Author

Writing the wrapper manually in a header and passing it to bindgen does create a binding, but the function still needs to be included in the final compilation unit, so we also need a source file that includes the header and add it to the build. Otherwise the function referenced by bindgen is not actually present in the library.

I'm also not a fan of having to specify every macro function type, but I'm not aware of any solution on how to avoid doing that and at least this way I can avoid writing C code, which is why this feature is worth it in my opinion.

@bors-servo
Copy link

☔ The latest upstream changes (presumably 1d2b579) made this pull request unmergeable. Please resolve the merge conflicts.

@pvdrz
Copy link
Contributor

pvdrz commented Jul 19, 2023

Writing the wrapper manually in a header and passing it to bindgen does create a binding, but the function still needs to be included in the final compilation unit, so we also need a source file that includes the header and add it to the build. Otherwise the function referenced by bindgen is not actually present in the library.

That's a good point, so basically by piggybacking on the --wrap-static-fns video we get the source C file so the user only has to compile it instead of writing it by hand. That's pretty neat!

I'm also not a fan of having to specify every macro function type, but I'm not aware of any solution on how to avoid doing that and at least this way I can avoid writing C code, which is why this feature is worth it in my opinion.

Yeah I don't have a better solution for this either. I'll give you a proper review in a moment

Copy link
Contributor

@pvdrz pvdrz left a comment

Choose a reason for hiding this comment

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

I'm done with the review. Other than the comments I wrote in the changes I think we might need a different name for this feature as ParseCallbacks::func_macro sounds almost like the same, maybe a more explicit --wrap-func-macro is better? IDK. The other thing I'd ask you is avoid editing the style of the CHANGELOG.md so you don't get more merge conflicts.

bindgen-cli/options.rs Show resolved Hide resolved
bindgen-cli/options.rs Outdated Show resolved Hide resolved
@@ -1084,3 +1136,28 @@ where

Ok((builder, output, verbose))
}

/// Parse a [`CTypeKind`] from the given command line `input`.
fn parse_c_type(input: &str) -> Result<CTypeKind, String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really necessary? It might sound lazy but to be honest it might be easier to just let the user type whatever they want as a type and propagate those strings until they are written to the wrapper code file. This also gives you the "advantage" of supporting any type the user wants as long as it is defined in the headers.

Copy link
Author

Choose a reason for hiding this comment

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

Well, we need do do some kind of parsing because we need to generate not only a function definition but also the function body where we call the macro. I don't believe there is a nice way to create the macro invocation without separating the argument types from the definition and removing the return type, but maybe I'm missing something?

Copy link
Author

Choose a reason for hiding this comment

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

I am actually a bit stuck on this. I am trying to support calling these macros with C types, which can't be referenced from build scripts:

typedef struct {
    int inner;
} my_type;

#define ACCESS_INNER(value) value.inner

I want to be able to specify that the ACCESS_INNER macro takes a my_type. To do that I was trying to switch everything over to just accept strings, but in Function::parse I need to generate a Function object which eventually needs a TypeKind for the return type and the argument types.

So my question is, is there a way to construct a Function that doesn't require me to parse the types, so I can just pass the strings through?

bindgen/codegen/mod.rs Outdated Show resolved Hide resolved
@@ -4787,6 +4810,7 @@ pub(crate) mod utils {

let mut code = Vec::new();

writeln!(code, "#include <stdint.h>\n#include <stdbool.h>")?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't add this here as it would break people's code that don't have these libraries or have their system path messed up (like in some windows casses I've seen). This goes in the same direction as treating the argument and return types provided by the user be just strings, if they use bool they have to include it in the headers themselves.

Copy link
Author

Choose a reason for hiding this comment

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

Hm, I agree that unconditionally adding those is not a good idea, but the problem is that people can't include the headers themselves, since the wrapper C file only includes the header file where the macro is defined over which the user usually has no control and which might not have the the required headers included.

Should we add an additional option allow passing in include files for the generated C code? Or do you have a better idea?

@@ -0,0 +1,15 @@
// bindgen-flags: --macro-function "SIMPLE" --macro-function "INDIRECT_SIMPLE" --macro-function "f32 COMPLEX(u32)" --macro-function "f32 INDIRECT_COMPLEX(u32)" --macro-function "f32 CONDITIONAL_COMPLEX(bool, u32)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I have mixed feelings about the syntax for the --macro-function argument. Usually when we have these options where you need to match an item by name and need extra info for it we use the --option=<ITEM_NAME>=<EXTRA_STUFF> syntax like in the --override-abi case.

Maybe having --macro-function=<MACRO_NAME>=<WRAPPER_TYPE> is a bit more in style with the rest of bindgen.

This also opens the gates to <MACRO_NAME> being a regex in the future.

Copy link
Author

Choose a reason for hiding this comment

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

Hm, the benefit of the the current representation is that is matches the macro definition with the rust types for argument names. For me that looks very readable, but we can try to match the existing command line style.

Did you have an idea for the <EXTRA_STUFF> syntax? Should we try to match the syntax used with the builder pattern like this?
--macro-function=SIMPLE --macro-function=COMPLEX="f32 (u32)" --macro-function=INDIRECT_COMPLEX="f32 (u32)" --macro-function=CONDITIONAL_COMPLEX="f32 (bool, u32)"

@reitermarkus
Copy link
Contributor

@Abestanis, does #2369 not solve your use case?

@Abestanis
Copy link
Author

Abestanis commented Oct 6, 2023

@reitermarkus #2369 is very interesting and incredibly cool. Thank you for putting so much effort into it. I can indeed use it for my specific usecase of the one specific macro I am trying to make accessible, but I think there is an issue with the approach of trying to replicate the C macro in Rust:

C and macros have a low of quirks and Rust is different to C and macros in many ways. There will always be some macro which can't yet be represented by #2369 and needs some new feature. Specifically when it comes to types and operations between types, C is a lot more lenient (because of implicit type conversions) while Rust basically disallows most operations between types. This means that many macros loose their ability to be called with different types.

I think the following (terrible, but) valid macro is going to be difficult to represent with #2369, even though I'm sure there are workarounds for this:

#define WEIRD_IS_ONE(x) (x == (uint16_t) 1 || x == (char) '1')

The wrapping approach supports all macro types out of the box, as long as the arguments and return types can be sent across ffi.

In my specific use case I am trying to make the following macro available:

typedef long BaseType_t;
#define pdFALSE ( ( BaseType_t ) 0 )
#define portEND_SWITCHING_ISR( xSwitchRequired ) do { if( xSwitchRequired != pdFALSE ) portYIELD(); } while( 0 )
#define portYIELD_FROM_ISR( x ) portEND_SWITCHING_ISR( x )

With the wrapping approach, I can define a wrapper like this

bindgen::Builder::default()
    .macro_function("portYIELD_FROM_ISR", FunctionType::new::<(), bool>())
    .generate();

and call it like this:

pub fn yield_from_isr(should_do_isr_context_switch: bool) {
    unsafe { ffi::portYIELD_FROM_ISR(should_do_isr_context_switch) }
}

Even though xSwitchRequired and pdFALSE have different types, C allows the comparison to work as expected.

With #2369, the following binding macro is generated:

pub use __cmacro__portEND_SWITCHING_ISR as portEND_SWITCHING_ISR;
#[doc(hidden)]
#[macro_export]
macro_rules! __cmacro__portYIELD_FROM_ISR {
    ($ x : expr) => {
        if $x != 0u8 as BaseType_t {
            vPortYield();
        }
    };
}

I can not call this macro with a bool argument type any more, because a <bool> != <cty::c_long> is not allowed in Rust.
Instead I have to work around this issue by explicitly casting the only allowed argument type:

pub fn yield_from_isr(should_do_isr_context_switch: bool) {
    unsafe { ffi::portYIELD_FROM_ISR!(BaseType_t::from(should_do_isr_context_switch)) }
}

So I still think the wrapping approach has some value and could exist together with #2369, but we have to make sure that they don't interfere with each other.

@Abestanis Abestanis force-pushed the feature/function_macros branch 3 times, most recently from 316f7c9 to 74f7bef Compare October 6, 2023 15:24
@Abestanis
Copy link
Author

@pvdrz Sorry for the long period of silence, I have addressed your feedback.

we might need a different name for this feature as ParseCallbacks::func_macro sounds almost like the same, maybe a more explicit --wrap-func-macro is better?

I'm open for better names, although the ParseCallbacks::func_macro is referring to the exact same concept of a functional macro, so I don't think the two are in conflict. Both allow handling functional macros in some way.

The other thing I'd ask you is avoid editing the style of the CHANGELOG.md so you don't get more merge conflicts.

Yeah, sorry about that, that was not a good idea. I removed all the formatting changes there except the trailing whitespaces, because my editor really wants to remove them when saving. I hope it's ok to leave these changes in there, but If needed I can try to remove them from the commit.

@reitermarkus
Copy link
Contributor

reitermarkus commented Oct 6, 2023

In my specific use case I am trying to make the following macro available:

typedef long BaseType_t;
#define pdFALSE ( ( BaseType_t ) 0 )
#define portEND_SWITCHING_ISR( xSwitchRequired ) do { if( xSwitchRequired != pdFALSE ) portYIELD(); } while( 0 )
#define portYIELD_FROM_ISR( x ) portEND_SWITCHING_ISR( x )

FreeRTOS is exactly what started my PR, actually. You may want to take a look at my fork of freertos-rust here, which is using the cmacro branch of my bindgen fork to generate (almost) all FreeRTOS bindings: https://github.com/reitermarkus/freertos-rust

But yes, the portEND_SWITCHING_ISR macro cannot be generated easily since it in turn calls the portYIELD macro, which in turn wants to write to a register.

The portYIELD macro is actually working fine, I even have a test for it here:

https://github.com/reitermarkus/cmacro-rs/blob/00849e0f5fda2c2de22bff73049faf53ebff54c9/tests/fixtures/assign_pointer.h

@reitermarkus
Copy link
Contributor

@Abestanis, I just found a bug that prevented the portEND_SWITCHING_ISR from being parsed correctly and hence from being generated, it should be possible now with my PR.

@Abestanis
Copy link
Author

@reitermarkus I would like to give your MR another try, but it seems like it is no longer compiling.

@bors-servo
Copy link

☔ The latest upstream changes (presumably 285eb56) made this pull request unmergeable. Please resolve the merge conflicts.

@reitermarkus
Copy link
Contributor

@Abestanis, I have now rebased #2369.

@Abestanis
Copy link
Author

@reitermarkus Thank you! ❤️

@CGMossa
Copy link
Contributor

CGMossa commented Jun 10, 2024

This looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants