rust-bindgen icon indicating copy to clipboard operation
rust-bindgen copied to clipboard

Support complex macros.

Open reitermarkus opened this issue 2 years ago • 25 comments

This supersedes https://github.com/rust-lang/rust-bindgen/pull/2268 which aimed to support casts in macros.

The cmacro crate I developed over the last months allows parsing casts in variable-like macros and even more complex macros, such as function-like macros and nested macros.

Macro parsing has been split from Var into a separate MacroDef type, since macros need to be parsed last. Variables can be evaluated at parse time, macros on the other hand can only be fully evaluated after all types, variables, functions and other macros have been parsed. This allows getting the correct final value and is needed for parsing nested macros.

reitermarkus avatar Dec 08 '22 07:12 reitermarkus

:umbrella: The latest upstream changes (presumably 5875949e8e598fa9b0e3a018b3547815e2654fc8) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Jan 06 '23 18:01 bors-servo

Can someone please take a look at this PR? It's been over three months without a review.

reitermarkus avatar Mar 13 '23 15:03 reitermarkus

:wave: Hi! Sorry for not giving you an answer in all this time. Somehow I missed this and wasn't aware of it.

I will properly review this PR as I think it might solve many issues regarding the type that is assigned to these define macros when they get translated as constants. Bindgen has depended for a long time on rust-cexpr but having someone breathe new life to this part of bindgen is good IMHO.

That being said, we still should figure out the logistics of maintenance due to bindgen using your crate and I don't have the whole picture in my head so I'll defer that part to @emilio

pvdrz avatar Mar 14 '23 17:03 pvdrz

Also random question. Is token concatenation using ## supported?

pvdrz avatar Mar 16 '23 02:03 pvdrz

Yes, concatenation (ident ## ident) as well as stringification (#ident) are supported. I think the only problem is Rust's concat_idents! does not (yet) support concatenating numbers (i.e. ident ## 1).

reitermarkus avatar Mar 16 '23 10:03 reitermarkus

Yes, concatenation (ident ## ident) as well as stringification (#ident) are supported. I think the only problem is Rust's concat_idents! does not (yet) support concatenating numbers (i.e. ident ## 1).

Hmm how would directives like this be translated then?

#define UL(X) X ## UL
#define ONE UL(1)

pvdrz avatar Mar 16 '23 14:03 pvdrz

I think this might actually expand ONE to 1 as c_ulong since there is special treatment for concatenating with a literal suffix like UL.

reitermarkus avatar Mar 17 '23 02:03 reitermarkus

Is should be working now, there was a bug that treated 1 ## U ## LL different than 1 ## LL # U which prevented some variants from being parsed.

reitermarkus avatar Mar 17 '23 06:03 reitermarkus

Nevermind. The UL macro is generated correctly as

macro_rules! __cmacro__UL {
  ($X:ident) => {
    concat_idents!($X, UL)
  };
}

ONE currently generates

pub const ONE: _ = concat_idents!(X , UL);

so some special handling in code generation needs to be done as well in addition to doing it in the parser.

reitermarkus avatar Mar 17 '23 06:03 reitermarkus

ONE should now correctly generate

pub const ONE: _ = 1;

reitermarkus avatar Mar 17 '23 20:03 reitermarkus

Hi @reitermarkus. I haven't had enough time to review the most recent changes but I suspect we need to figure out a way to avoid breaking the --rust-target flag with this feature as cmacro is able to emit arbitrary Rust code. I suggest two options depending on your priorities:

  • We could put all these changes behind the "experimental" feature. In that way we can merge this faster and incrementally fix any regressions caused by the code generation of cmacro.
  • Be sure that cmacro will emit valid code for the 1.30 version of the compiler (which is the oldest version we haven't deprecated).

In either case we need to be able to test that different versions of rustc can compile the code emitted by bindgen which means fixing https://github.com/rust-lang/rust-bindgen/issues/2277.

PD: I really want to merge this PR as it would fix a lot of weird issues we have with preprocessing directives.

pvdrz avatar Mar 29 '23 22:03 pvdrz

I guess we can also add some support for --rust-target in cmacro so we can either not generate code for older versions (e.g. for function-like macros) or alter code generation.

reitermarkus avatar Mar 31 '23 15:03 reitermarkus

I have now put the code generation for function-like macros behind the experimental feature.

Variable-like macros should already always generate the correct code.

reitermarkus avatar May 16 '23 16:05 reitermarkus

@pvdrz, please have another look here. Probably best to review https://github.com/rust-lang/rust-bindgen/pull/2543 first though.

reitermarkus avatar Jun 03 '23 11:06 reitermarkus

:umbrella: The latest upstream changes (presumably 2034a0fda98cb8a5180d4f14b8c0d7949c6cb146) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo avatar Jun 15 '23 20:06 bors-servo

I'm wondering what the current status is here, is this PR blocked waiting on a review?

jschwe avatar Aug 08 '23 11:08 jschwe

This PR is blocked (or superseded) by https://github.com/rust-lang/rust-bindgen/pull/2561.

pvdrz avatar Aug 08 '23 16:08 pvdrz

@pvdrz Are you sure that's the right issue? On first glance it doesn't look the same.

alex avatar Aug 08 '23 20:08 alex

@alex, it's blocked by https://github.com/rust-lang/rust-bindgen/pull/2561 at the moment, i.e. part of this PR depends on the sorting being fixed.

reitermarkus avatar Aug 08 '23 21:08 reitermarkus

Note that this PR also includes the same changes.

reitermarkus avatar Aug 08 '23 21:08 reitermarkus

Ah, got it, I see how it's blocked. It was the superseded that confused me.

alex avatar Aug 08 '23 21:08 alex

So further testing/reviewing of https://github.com/rust-lang/rust-bindgen/pull/2561 would be appreciated to get this moving again.

reitermarkus avatar Aug 08 '23 21:08 reitermarkus

@reitermarkus, this is really exciting because it would resolve a downstream bug I ran into with bssl-sys. The review comments above looked positive, any idea when it might be merged?

overhacked avatar May 13 '24 18:05 overhacked

@overhacked, thanks for testing.

any idea when it might be merged?

No idea when this will be merged, I already split some of the changes into separate PRs to make them easier to review, but all of them seem to also have stalled.

reitermarkus avatar May 27 '24 20:05 reitermarkus