Allow wrapper function generation for functional macros #2578

Abestanis · 2023-07-07T14:08:48Z

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 · 2023-07-07T14:09:12Z

r? @emilio

bors-servo · 2023-07-10T20:53:06Z

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

bors-servo · 2023-07-14T21:53:48Z

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

pvdrz · 2023-07-18T22:00:34Z

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 · 2023-07-18T22:32:35Z

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 · 2023-07-19T18:58:00Z

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

pvdrz · 2023-07-19T19:19:51Z

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

pvdrz

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

pvdrz · 2023-07-19T19:26:38Z

bindgen-cli/options.rs

@@ -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> {


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.

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?

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

pvdrz · 2023-07-19T19:31:15Z

bindgen/codegen/mod.rs

@@ -4787,6 +4810,7 @@ pub(crate) mod utils {

        let mut code = Vec::new();

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


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.

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?

pvdrz · 2023-07-19T19:45:20Z

bindgen-tests/tests/headers/function_macros.h

@@ -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)"


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.

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 · 2023-10-04T04:18:14Z

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

Abestanis · 2023-10-06T10:45:11Z

@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 · 2023-10-06T15:46:49Z

@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 · 2023-10-06T18:01:24Z

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 · 2023-10-06T18:37:44Z

@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 · 2024-02-07T18:12:11Z

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

bors-servo · 2024-02-19T16:20:54Z

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

reitermarkus · 2024-02-21T16:26:57Z

@Abestanis, I have now rebased #2369.

Abestanis · 2024-02-21T22:40:16Z

@reitermarkus Thank you! ❤️

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

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

Abestanis force-pushed the feature/function_macros branch from 3e82041 to 5091cdb Compare July 17, 2023 09:46

pvdrz requested changes Jul 19, 2023

View reviewed changes

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

Abestanis added 8 commits February 7, 2024 16:21

Generate wrapping functions for functional macros

2ad5e7c

Add tests for macro wrap function generation

b43c620

Add changes to Changelog

5ca0f26

Make sure macro wrapper functions have the correct link name

a5463fc

Make the macro-function argument experimental

3b1372c

Rename native_code_generation_path to wrapper_code_generation_path

54d1ad7

Don't use let...else statement

10a0ac7

Rename wrap_static_fns_suffix and use it for functional macros

26f18f5

Abestanis force-pushed the feature/function_macros branch from 74f7bef to 26f18f5 Compare February 7, 2024 15:53

CGMossa mentioned this pull request May 10, 2024

Implement wrappers for function-like macros extendr/libR-sys#139

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow wrapper function generation for functional macros #2578

Allow wrapper function generation for functional macros #2578

Abestanis commented Jul 7, 2023 •

edited

Abestanis commented Jul 7, 2023

bors-servo commented Jul 10, 2023

bors-servo commented Jul 14, 2023

pvdrz commented Jul 18, 2023

Abestanis commented Jul 18, 2023

bors-servo commented Jul 19, 2023

pvdrz commented Jul 19, 2023

pvdrz left a comment

pvdrz Jul 19, 2023

Abestanis Oct 6, 2023

Abestanis Feb 7, 2024

pvdrz Jul 19, 2023

Abestanis Oct 6, 2023

pvdrz Jul 19, 2023

Abestanis Oct 6, 2023

reitermarkus commented Oct 4, 2023

Abestanis commented Oct 6, 2023 •

edited

Abestanis commented Oct 6, 2023

reitermarkus commented Oct 6, 2023 •

edited

reitermarkus commented Oct 6, 2023

Abestanis commented Feb 7, 2024

bors-servo commented Feb 19, 2024

reitermarkus commented Feb 21, 2024

Abestanis commented Feb 21, 2024

		@@ -4787,6 +4810,7 @@ pub(crate) mod utils {

		let mut code = Vec::new();

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

		@@ -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)"

Allow wrapper function generation for functional macros #2578

Are you sure you want to change the base?

Allow wrapper function generation for functional macros #2578

Conversation

Abestanis commented Jul 7, 2023 • edited

Example

Abestanis commented Jul 7, 2023

bors-servo commented Jul 10, 2023

bors-servo commented Jul 14, 2023

pvdrz commented Jul 18, 2023

Abestanis commented Jul 18, 2023

bors-servo commented Jul 19, 2023

pvdrz commented Jul 19, 2023

pvdrz left a comment

Choose a reason for hiding this comment

pvdrz Jul 19, 2023

Choose a reason for hiding this comment

Abestanis Oct 6, 2023

Choose a reason for hiding this comment

Abestanis Feb 7, 2024

Choose a reason for hiding this comment

pvdrz Jul 19, 2023

Choose a reason for hiding this comment

Abestanis Oct 6, 2023

Choose a reason for hiding this comment

pvdrz Jul 19, 2023

Choose a reason for hiding this comment

Abestanis Oct 6, 2023

Choose a reason for hiding this comment

reitermarkus commented Oct 4, 2023

Abestanis commented Oct 6, 2023 • edited

Abestanis commented Oct 6, 2023

reitermarkus commented Oct 6, 2023 • edited

reitermarkus commented Oct 6, 2023

Abestanis commented Feb 7, 2024

bors-servo commented Feb 19, 2024

reitermarkus commented Feb 21, 2024

Abestanis commented Feb 21, 2024

Abestanis commented Jul 7, 2023 •

edited

Abestanis commented Oct 6, 2023 •

edited

reitermarkus commented Oct 6, 2023 •

edited