Workaround for expansion of function-like macros #2779

jbaublitz · 2024-03-11T16:39:54Z

Related to #753

@ojeda Let me know if you have any thoughts on this. Compared to without the fallback (1s on the contants in the kernel header) I managed to get the compilation times from 2m to 8s to 3.5s. There may be additional optimizations we can do but it seems like it's definitely trending in the right direction.

ojeda

3.5s sounds quite usable! Thanks John!

Some comments below...

bindgen/ir/context.rs

bindgen/clang.rs

bindgen/options/mod.rs

bindgen/clang.rs

bindgen/ir/var.rs

bindgen/clang.rs

bindgen/ir/var.rs

emilio

This looks neat. I'm a bit concerned about just using a constant file name in the cwd for this, but that's not unfixable (we could even use a proper temporary file from the tempfile crate or something). No need to fix it right now.

emilio · 2024-03-12T22:45:15Z

bindgen/ir/var.rs

+            .open(&file)
+            .ok()?
+            .write_all(contents.as_bytes())
+            .ok()?;


I wonder, do we really need to write this file to disk? Can we somehow abuse the CXUnsavedFile API to do this in memory in both code paths?

Otherwise, should we delete it?

Also, it's a bit unfortunate that if the user happens to have a valuable thing there (unlikely) we'd throw it away.

Please take a look at my most recent addition of --clang-macro-fallback-build-dir. That might address your concern.

bindgen/ir/var.rs

bindgen/ir/context.rs

bindgen/clang.rs

jbaublitz · 2024-03-13T14:27:41Z

I've done a quick pass to fix the low hanging fruit. Will try to get back to this later today to resolve the rest of the concerns and follow up on any outstanding discussions.

jbaublitz · 2024-03-14T18:07:23Z

@ojeda @emilio I think this is ready for a second pass! I think I've addressed all of the concerns raised above.

emilio

Looks good with that simplification, but can we add a test for this? Let me know if you need any help with it.

emilio · 2024-03-19T09:54:50Z

bindgen/ir/var.rs

+        return None;
+    }
+
+    let file = format!(


The translation unit has the file path already, so this could be moved to try_ensure_fallback_translation_unit, and just use ftu.file_path below, right?

This depends a little bit on how you want to see this implemented. On one hand, we could make the member of FallbackTranslationUnit crate public, but I think we'd require a clone because otherwise when we pass it in, we'd have a mutable reference taken for .reparse() and an immutable reference taken for &ftu.file_path. We could also change the API slightly so that .reparse() only takes file contents as it's only operating on a single file and we may not need the flexibility of supporting reparsing multiple different files. I'd personally prefer the second approach.

I've made the changes to reflect the second option. Let me know what you think.

jbaublitz · 2024-03-19T18:15:53Z

@emilio What would you like in terms of the test? An integration or unit test or both?

emilio · 2024-03-19T22:42:12Z

@emilio What would you like in terms of the test? An integration or unit test or both?

Integration should be fine. Just using the new flags from a regular test (see the tests/headers directory and bindgen-flags: directives). I think it should be straight-forward but let me know if you hit any issues or weirdness. Thanks!

bindgen/ir/context.rs

jbaublitz · 2024-03-20T00:00:30Z

Okay I think I've addressed everything!

emilio · 2024-03-20T00:16:43Z

It seems clang 9.0 doesn't like your test and the new test fails there. Let me know if you have the appetite to chase that down (shouldn't be hard by pulling a pre-built clang for Linux amd64, and pointing to it with LIBCLANG_PATH).

Otherwise I guess we can document the option as best effort / not working on older libclang versions (totally fine), and silence the failure there by adding the empty (well with the #[allow(...)]) expectation file to bindgen-tests/tests/expectations/tests/libclang-9/.

jbaublitz · 2024-03-20T14:52:21Z

I may need a little bit of help in terms of the explanation but I can reproduce this reusing a precompiled header from 9.x on the latest version and vice versa. I'm wondering if there's a way there's a lingering precompiled header in the test suite somehow. This also raises the question of whether we should also be deleting the precompiled headers because debugging this would be incredibly difficult for users unfamiliar with the code and I expect you'll bump into in increase in issues with this feature if we don't.

jbaublitz · 2024-03-20T23:15:59Z

Okay, I managed to track it down to Clang_Cursor_Evaluate. In Clang 9, this returns a NULL pointer when trying to evaluate a Cursor over Cursors with kinds IntegerLiteral and BinaryOperator. I'm not sure if this is a bug or not, but I think it may be best to ignore for the time being. I'll add the file you recommended.

emilio

This might need a rebase (might be able to do that automatically, we'll see), but looks good to me, thanks!

This commit resolves an issue where macros that evaluate to a constant but have a function like macro in the macro body would not be properly expanded by cexpr. This adds an opt-in option to use Clang on intermediary files to evaluate the macros one by one. This is opt-in largely because of the compile time implications.

SeleDreams · 2024-04-27T14:41:34Z

@jbaublitz I was able to test this workaround, but it doesn't seem to be working in my case.
I am trying to make a wrapper around the blocksds libnds library (meant for making DS homebrews)

this is a part of the macros that register the interrupts

#include <nds/ndstypes.h>

// Values allowed for REG_IE and REG_IF
#define IRQ_VBLANK          BIT(0)  ///< Vertical blank interrupt mask
#define IRQ_HBLANK          BIT(1)  ///< Horizontal blank interrupt mask
#define IRQ_VCOUNT          BIT(2)  ///< Vcount match interrupt mask
#define IRQ_TIMER0          BIT(3)  ///< Timer 0 interrupt mask
#define IRQ_TIMER1          BIT(4)  ///< Timer 1 interrupt mask
#define IRQ_TIMER2          BIT(5)  ///< Timer 2 interrupt mask
#define IRQ_TIMER3          BIT(6)  ///< Timer 3 interrupt mask
#ifdef ARM7
#define IRQ_NETWORK         BIT(7)  ///< Serial/RTC interrupt mask (ARM7) (deprecated name)
#define IRQ_RTC             BIT(7)  ///< Serial/RTC interrupt mask (ARM7)
#endif
#define IRQ_DMA0            BIT(8)  ///< DMA 0 interrupt mask
#define IRQ_DMA1            BIT(9)  ///< DMA 1 interrupt mask
#define IRQ_DMA2            BIT(10) ///< DMA 2 interrupt mask
#define IRQ_DMA3            BIT(11) ///< DMA 3 interrupt mask
#define IRQ_KEYS            BIT(12) ///< Keypad interrupt mask
#define IRQ_CART            BIT(13) ///< GBA cartridge interrupt mask
#define IRQ_IPC_SYNC        BIT(16) ///< IPC sync interrupt mask

and BIT is a macro defined in ndstypes

/// Returns a number with the nth bit set.
#define BIT(n) (1 << (n))

none of the IRQ defines end up set in the wrapper

let bindings = bindgen::Builder::default()
        // The input header we would like to generate
        // bindings for.
        .header("wrapper.h")
        .wrap_static_fns(true)
        .wrap_static_fns_path("src/arm7_bindings.c")
        .use_core()
        .trust_clang_mangling(false)
        .layout_tests(false)
        .ctypes_prefix("::libc")
        .prepend_enum_name(false)
        .blocklist_type("__builtin_va_list")
        .blocklist_type("__va_list")
        .derive_default(true)
        // Tell cargo to invalidate the built crate whenever any of the
        // included header files changed.
        .parse_callbacks(Box::new(bindgen::CargoCallbacks::new()))
        .clang_arg(format!("-I{}/libs/libnds/include",blocksds_path))
        .clang_arg(format!("-I{}/libs/dswifi/include",blocksds_path))
        .clang_arg(format!("-I{}/libs/maxmod/include",blocksds_path))
        .clang_arg(format!("-isystem{}/toolchain/gcc-arm-none-eabi/arm-none-eabi/include",wonderful_path))
        .clang_arg("-DARM7")
        .clang_macro_fallback()
        // Finish the builder and generate the bindings.
        .generate()
        // Unwrap the Result and panic on failure.
        .expect("Unable to generate bindings");

jbaublitz · 2024-04-28T03:42:12Z

@SeleDreams I'm not able to reproduce the behavior you're seeing. With:

#define BIT(n) (1 << (n))

#define IRQ_VBLANK          BIT(0)  ///< Vertical blank interrupt mask
#define IRQ_HBLANK          BIT(1)  ///< Horizontal blank interrupt mask
#define IRQ_VCOUNT          BIT(2)  ///< Vcount match interrupt mask
#define IRQ_TIMER0          BIT(3)  ///< Timer 0 interrupt mask
#define IRQ_TIMER1          BIT(4)  ///< Timer 1 interrupt mask
#define IRQ_TIMER2          BIT(5)  ///< Timer 2 interrupt mask
#define IRQ_TIMER3          BIT(6)  ///< Timer 3 interrupt mask
#ifdef ARM7
#define IRQ_NETWORK         BIT(7)  ///< Serial/RTC interrupt mask (ARM7) (deprecated name)
#define IRQ_RTC             BIT(7)  ///< Serial/RTC interrupt mask (ARM7)
#endif
#define IRQ_DMA0            BIT(8)  ///< DMA 0 interrupt mask
#define IRQ_DMA1            BIT(9)  ///< DMA 1 interrupt mask
#define IRQ_DMA2            BIT(10) ///< DMA 2 interrupt mask
#define IRQ_DMA3            BIT(11) ///< DMA 3 interrupt mask
#define IRQ_KEYS            BIT(12) ///< Keypad interrupt mask
#define IRQ_CART            BIT(13) ///< GBA cartridge interrupt mask
#define IRQ_IPC_SYNC        BIT(16) ///< IPC sync interrupt mask

in testheader.h and

./target/debug/bindgen --clang-macro-fallback $HOME/testheader.h

I get:

/* automatically generated by rust-bindgen 0.69.4 */

pub const IRQ_VBLANK: u32 = 1;
pub const IRQ_HBLANK: u32 = 2;
pub const IRQ_VCOUNT: u32 = 4;
pub const IRQ_TIMER0: u32 = 8;
pub const IRQ_TIMER1: u32 = 16;
pub const IRQ_TIMER2: u32 = 32;
pub const IRQ_TIMER3: u32 = 64;
pub const IRQ_DMA0: u32 = 256;
pub const IRQ_DMA1: u32 = 512;
pub const IRQ_DMA2: u32 = 1024;
pub const IRQ_DMA3: u32 = 2048;
pub const IRQ_KEYS: u32 = 4096;
pub const IRQ_CART: u32 = 8192;
pub const IRQ_IPC_SYNC: u32 = 65536;

What version of Clang are you using? Another thing to consider is that if the directory that contains the headers aren't writable, you may need to specify --clang-macro-fallback-build-dir as a temporary writable directory to store the precompiled headers in.

SeleDreams · 2024-04-28T07:18:20Z

The directory is writable since I do see the temp file get created. I wonder if the issue is caused by BIT being defined in another header Le dim. 28 avr. 2024 à 05:42, John Baublitz ***@***.***> a écrit :

…

@SeleDreams <https://github.com/SeleDreams> I'm not able to reproduce the behavior you're seeing. With: #define BIT(n) (1 << (n)) #define IRQ_VBLANK BIT(0) ///< Vertical blank interrupt mask #define IRQ_HBLANK BIT(1) ///< Horizontal blank interrupt mask #define IRQ_VCOUNT BIT(2) ///< Vcount match interrupt mask #define IRQ_TIMER0 BIT(3) ///< Timer 0 interrupt mask #define IRQ_TIMER1 BIT(4) ///< Timer 1 interrupt mask #define IRQ_TIMER2 BIT(5) ///< Timer 2 interrupt mask #define IRQ_TIMER3 BIT(6) ///< Timer 3 interrupt mask #ifdef ARM7 #define IRQ_NETWORK BIT(7) ///< Serial/RTC interrupt mask (ARM7) (deprecated name) #define IRQ_RTC BIT(7) ///< Serial/RTC interrupt mask (ARM7) #endif #define IRQ_DMA0 BIT(8) ///< DMA 0 interrupt mask #define IRQ_DMA1 BIT(9) ///< DMA 1 interrupt mask #define IRQ_DMA2 BIT(10) ///< DMA 2 interrupt mask #define IRQ_DMA3 BIT(11) ///< DMA 3 interrupt mask #define IRQ_KEYS BIT(12) ///< Keypad interrupt mask #define IRQ_CART BIT(13) ///< GBA cartridge interrupt mask #define IRQ_IPC_SYNC BIT(16) ///< IPC sync interrupt mask in testheader.h and ./target/debug/bindgen --clang-macro-fallback $HOME/testheader.h I get: /* automatically generated by rust-bindgen 0.69.4 */ pub const IRQ_VBLANK: u32 = 1; pub const IRQ_HBLANK: u32 = 2; pub const IRQ_VCOUNT: u32 = 4; pub const IRQ_TIMER0: u32 = 8; pub const IRQ_TIMER1: u32 = 16; pub const IRQ_TIMER2: u32 = 32; pub const IRQ_TIMER3: u32 = 64; pub const IRQ_DMA0: u32 = 256; pub const IRQ_DMA1: u32 = 512; pub const IRQ_DMA2: u32 = 1024; pub const IRQ_DMA3: u32 = 2048; pub const IRQ_KEYS: u32 = 4096; pub const IRQ_CART: u32 = 8192; pub const IRQ_IPC_SYNC: u32 = 65536; What version of Clang are you using? Another thing to consider is that if the directory that contains the headers aren't writable, you may need to specify --clang-macro-fallback-build-dir as a temporary writable directory to store the precompiled headers in. — Reply to this email directly, view it on GitHub <#2779 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4UF4LW6RDIYNOM6D4DWNLY7RV2TAVCNFSM6AAAAABEQVC3USVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBRGMYTENBXGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

SeleDreams · 2024-04-28T07:19:19Z

Also, does it use the system wide clang installation ? if that's the case clang --version clang version 17.0.6 Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin

SeleDreams · 2024-04-28T07:46:13Z

@jbaublitz after testing, I found something interesting.
If I directly include nds.h like

let bindings = bindgen::Builder::default()
        // The input header we would like to generate
        // bindings for.
        .header(format!("{}/libs/libnds/include/nds.h",blocksds_path))
        .wrap_static_fns(true)
        .wrap_static_fns_path("src/arm9_bindings.c")
        .use_core()
        .trust_clang_mangling(false)
        .prepend_enum_name(false)
        .clang_arg("-mfloat-abi=soft")
        // Tell cargo to invalidate the built crate whenever any of the
        // included header files changed.
        .clang_arg(format!("-I{}/libs/libnds/include",blocksds_path))
        .clang_arg(format!("-I{}/libs/dswifi/include",blocksds_path))
        .clang_arg(format!("-I{}/libs/maxmod/include",blocksds_path))
        .clang_arg(format!("-isystem{}/toolchain/gcc-arm-none-eabi/arm-none-eabi/include",wonderful_path))
        .clang_arg("-DARM9")
        .clang_arg("-D__NDS__")
        .clang_macro_fallback()
        .wrap_unsafe_ops(true)
        // Finish the builder and generate the bindings.
        .generate()
        // Unwrap the Result and panic on failure.
        .expect("Unable to generate bindings");

it works, however, if I include a wrapper.h with the content

#include <nds.h>
#ifdef ARM7
#include <dswifi7.h>
#include <maxmod7.h>
#elif ARM9
#include <dswifi9.h>
#include <maxmod9.h>
#endif

it doesn't work, although it did include the rest of nds.h, it didn't import the macros
that might be a bug

edit:
that seems to occur also when I do that

let bindings = bindgen::Builder::default()
        // The input header we would like to generate
        // bindings for.
        .header(format!("{}/libs/libnds/include/nds.h",blocksds_path))
        .header(format!("{}/libs/dswifi/include/dswifi9.h",blocksds_path))
        .header(format!("{}/libs/maxmod/include/maxmod9.h",blocksds_path))

so the issue seems caused by the fact to import other headers than just nds.h

it also occurs when wrapper.h only includes nds.h
this is the library for reference
https://github.com/blocksds/libnds/tree/master

jbaublitz · 2024-04-28T15:30:09Z

@SeleDreams I just did some research and what I'm seeing is that Clang only supports one precompiled header at a time. I'm working on a fix now.

ojeda added the rust-for-linux Issues relevant to the Rust for Linux project label Mar 11, 2024

ojeda mentioned this pull request Mar 11, 2024

bindgen wanted features & bugfixes Rust-for-Linux/linux#353

Open

27 tasks

jbaublitz force-pushed the issue-rust-bindgen-753 branch 2 times, most recently from 104a714 to 3990e2f Compare March 11, 2024 16:53

jbaublitz mentioned this pull request Mar 11, 2024

Prototype of bindgen workaround usage stratis-storage/libcryptsetup-rs#348

Open

ojeda reviewed Mar 11, 2024

View reviewed changes

emilio reviewed Mar 12, 2024

View reviewed changes

jbaublitz force-pushed the issue-rust-bindgen-753 branch from 3990e2f to 104bcfe Compare March 13, 2024 14:25

jbaublitz force-pushed the issue-rust-bindgen-753 branch 3 times, most recently from 3a52ac5 to 6911401 Compare March 14, 2024 15:46

jbaublitz changed the title ~~Workaround for expansion of funcion-like macros~~ Workaround for expansion of function-like macros Mar 14, 2024

jbaublitz force-pushed the issue-rust-bindgen-753 branch 4 times, most recently from 10036f1 to 390062a Compare March 14, 2024 18:00

emilio approved these changes Mar 19, 2024

View reviewed changes

jbaublitz force-pushed the issue-rust-bindgen-753 branch 2 times, most recently from 0e553e3 to b5df566 Compare March 19, 2024 18:32

emilio reviewed Mar 19, 2024

View reviewed changes

bindgen/ir/context.rs Outdated Show resolved Hide resolved

jbaublitz force-pushed the issue-rust-bindgen-753 branch 3 times, most recently from b71bb6f to 4f319fa Compare March 19, 2024 23:59

jbaublitz force-pushed the issue-rust-bindgen-753 branch from 4f319fa to 9532f2e Compare March 20, 2024 22:07

jbaublitz force-pushed the issue-rust-bindgen-753 branch 2 times, most recently from 6c0044f to 9ded89d Compare March 20, 2024 23:28

emilio approved these changes Apr 1, 2024

View reviewed changes

emilio enabled auto-merge (rebase) April 1, 2024 20:00

emilio force-pushed the issue-rust-bindgen-753 branch from 9ded89d to f4d25f6 Compare April 1, 2024 20:00

emilio force-pushed the issue-rust-bindgen-753 branch from f4d25f6 to f77b3de Compare April 1, 2024 20:09

auto-merge was automatically disabled April 1, 2024 20:21
Merge queue setting changed

emilio added this pull request to the merge queue Apr 1, 2024

Merged via the queue into rust-lang:main with commit 6e42ccf Apr 1, 2024
33 checks passed

This was referenced Apr 29, 2024

UINTMAX64 can never be generated #2822

Open

Switch to single precompiled header in macro fallback #2823

Merged

jbaublitz deleted the issue-rust-bindgen-753 branch August 13, 2024 13:17

pvdrz mentioned this pull request Aug 21, 2024

Bindgen skips macros with token concatenation. #2402

Closed

ivmarkov mentioned this pull request Sep 22, 2024

esp32c2 use example http_sw_server report err ESP_ERR_HTTPD_TASK esp-rs/esp-idf-svc#489

Closed

aesteve-rh mentioned this pull request Nov 22, 2024

Bump bindgen version Gnurou/v4l2r#40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround for expansion of function-like macros #2779

Workaround for expansion of function-like macros #2779

Workaround for expansion of function-like macros #2779

Workaround for expansion of function-like macros #2779

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment