Workaround for expansion of function-like macros

[jbaublitz]

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...

[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]

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.

[jbaublitz]

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

[jbaublitz]

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]

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

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?

[jbaublitz]

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.

[jbaublitz]

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

[jbaublitz]

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

[emilio]

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

[jbaublitz]

Okay I think I've addressed everything!

[emilio]

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]

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]

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!

[SeleDreams]

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

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

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]

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]

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

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