Repository: rust-lang/rust-bindgen
Pull request #2823 (2024-04-29 14:40:34)

Switch to single precompiled header in macro fallback
Fix for issue reported [here](https://github.com/rust-lang/rust-bindgen/pull/2779#issuecomment-2081376090).

@SeleDreams Could you also confirm that this resolves your problem?

SeleDreams: > Fix for issue reported [here](https://github.com/rust-lang/rust-bindgen/pull/2779#issuecomment-2081376090).
> 
> @SeleDreams Could you also confirm that this resolves your problem?

I tested, seems like it still doesn't generate the IRQ defines when I have other headers included
jbaublitz: @SeleDreams Can you provide me with a minimal reproducer to reproduce your problem with the headers you're using? I'm no longer able to reproduce the problem with this PR in the setup that originally helped me confirm that there was a bug.
SeleDreams: I'll try to do a reproduction case as soon as possible.
Right now with my great luck I have an X server issue on the linux machine
I work on so I can't do it right now

Le lun. 29 avr. 2024 à 20:02, John Baublitz ***@***.***> a
écrit :

> @SeleDreams <https://github.com/SeleDreams> Can you provide me with a
> minimal reproducer to reproduce your problem with the headers you're using?
> I'm no longer able to reproduce the problem with this PR in the setup that
> originally helped me confirm that there was a bug.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/rust-lang/rust-bindgen/pull/2823#issuecomment-2083340511>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AD4UF4PB56GDOHDE2TOYJHDY72DNHAVCNFSM6AAAAABG6PCDBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBTGM2DANJRGE>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>

jbaublitz: > This looks reasonable but I guess we should sort out why it's still not working for @SeleDreams? Or do you want this merged now?

I would like to figure out what's going on with @SeleDreams before we merge this as they reported the problem originally so I'd like to make sure it works in their use case too. If it's a setup problem, I'll help debug and if there are two bugs, I'll fix the second one. :)

> 
> A test for this would also be amazing, but if it's too hard to set up lmk.

How would you generally suggest I go about testing multiple headers? Is there infrastructure for that?


jbaublitz: > I'll try to do a reproduction case as soon as possible. Right now with my great luck I have an X server issue on the linux machine I work on so I can't do it right now Le lun. 29 avr. 2024 à 20:02, John Baublitz ***@***.***> a écrit :
> […](#)
> @SeleDreams <https://github.com/SeleDreams> Can you provide me with a minimal reproducer to reproduce your problem with the headers you're using? I'm no longer able to reproduce the problem with this PR in the setup that originally helped me confirm that there was a bug. — Reply to this email directly, view it on GitHub <[#2823 (comment)](https://github.com/rust-lang/rust-bindgen/pull/2823#issuecomment-2083340511)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD4UF4PB56GDOHDE2TOYJHDY72DNHAVCNFSM6AAAAABG6PCDBSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBTGM2DANJRGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Okay sounds good! Just let me know when you can provide the reproducer, and I'll take a look. Basically I just want to rule out a setup problem because since bindgen doesn't have a lot of error reporting infrastructure yet (I think it's planned), setup problems will look exactly the same as a bug: bindings for the constants are just not generated. If it's a bug, I'll fix it. If not, I'll try to point you to exactly what's going on.
SeleDreams: @jbaublitz  So, I was able to make a reproduction project, however, for some weird reason, it seems to delete the wrapper.h as well everytime I build the project which in itself could be another bug
https://github.com/SeleDreams/reproduction_test

jbaublitz: @SeleDreams Thanks so much for the report and taking the time to craft a reproducer! It appears there were two bugs. One was that Clang only supports one precompiled header at a time which I resolved, but the other is that `-I` flags were not getting passed from `.clang_arg()` calls to the header precompilation. The reason I didn't bump into this is that I was testing on headers in system folders, but for headers that are in nonstandard places (like in your case) these were not being picked up.

The resolution I've come to is forwarding all `-I` and `-isystem` flags. @emilio Let me know if you think other flags should be forwarded too.
jbaublitz: I've added a test that tests both multiple headers and also the `-I` parameters being forwarded.
jbaublitz: > Is there any good reason not to forward _all_ flags? Think of a macro that desugars to something like `sizeof(int)`, if you don't pass the right flags (like `--target` and so) you might end up with the wrong constant value, right? Same with `sizeof(enum foo)` if you don't pass `-fshort-enums` or what not... Probably countless examples too.

The reason I didn't forward all flags is that generally there's a header provided at the end of the clang args in my tests which causes the precompilation to fail. If you'd prefer, I can just remove the last argument instead of exclusively passing the include directives.
emilio: Yes. Excluding what we don't need seems like a better approach imho
jbaublitz: @emilio I've taken the approach of filtering out any argument that matches the elements in `input_headers` or `-include` based on how bindgen generates c_args for the main translation unit. Let me know if you have any issues with this approach.

@SeleDreams Once the code review is done, do you mind testing your actual setup and confirming the problem is resolved?

Patch:
diff --git a/bindgen/clang.rs b/bindgen/clang.rs
index 08d33815..26c02ace 100644
--- a/bindgen/clang.rs
+++ b/bindgen/clang.rs
@@ -1908,7 +1908,8 @@ impl Drop for TranslationUnit {
 /// Translation unit used for macro fallback parsing
 pub(crate) struct FallbackTranslationUnit {
     file_path: String,
-    pch_paths: Vec<String>,
+    header_path: String,
+    pch_path: String,
     idx: Box<Index>,
     tu: TranslationUnit,
 }
@@ -1923,7 +1924,8 @@ impl FallbackTranslationUnit {
     /// Create a new fallback translation unit
     pub(crate) fn new(
         file: String,
-        pch_paths: Vec<String>,
+        header_path: String,
+        pch_path: String,
         c_args: &[Box<str>],
     ) -> Option<Self> {
         // Create empty file
@@ -1944,7 +1946,8 @@ impl FallbackTranslationUnit {
         )?;
         Some(FallbackTranslationUnit {
             file_path: file,
-            pch_paths,
+            header_path,
+            pch_path,
             tu: f_translation_unit,
             idx: f_index,
         })
@@ -1982,9 +1985,8 @@ impl FallbackTranslationUnit {
 impl Drop for FallbackTranslationUnit {
     fn drop(&mut self) {
         let _ = std::fs::remove_file(&self.file_path);
-        for pch in self.pch_paths.iter() {
-            let _ = std::fs::remove_file(pch);
-        }
+        let _ = std::fs::remove_file(&self.header_path);
+        let _ = std::fs::remove_file(&self.pch_path);
     }
 }
 
diff --git a/bindgen/ir/context.rs b/bindgen/ir/context.rs
index 2e608b07..a1536935 100644
--- a/bindgen/ir/context.rs
+++ b/bindgen/ir/context.rs
@@ -29,6 +29,8 @@ use quote::ToTokens;
 use std::borrow::Cow;
 use std::cell::{Cell, RefCell};
 use std::collections::{BTreeSet, HashMap as StdHashMap};
+use std::fs::OpenOptions;
+use std::io::Write;
 use std::mem;
 use std::path::Path;
 
@@ -2081,55 +2083,91 @@ If you encounter an error missing from this list, please file an issue or a PR!"
 
             let index = clang::Index::new(false, false);
 
-            let mut c_args = Vec::new();
-            let mut pch_paths = Vec::new();
-            for input_header in self.options().input_headers.iter() {
+            let mut header_names_to_compile = Vec::new();
+            let mut header_paths = Vec::new();
+            let mut header_contents = String::new();
+            for input_header in self.options.input_headers.iter() {
                 let path = Path::new(input_header.as_ref());
-                let header_name = path
-                    .file_name()
-                    .and_then(|hn| hn.to_str())
-                    .map(|s| s.to_owned());
-                let header_path = path
-                    .parent()
-                    .and_then(|hp| hp.to_str())
-                    .map(|s| s.to_owned());
-
-                let (header, pch) = if let (Some(ref hp), Some(hn)) =
-                    (header_path, header_name)
-                {
-                    let header_path = if hp.is_empty() { "." } else { hp };
-                    let header = format!("{header_path}/{hn}");
-                    let pch_path = if let Some(ref path) =
-                        self.options().clang_macro_fallback_build_dir
-                    {
-                        path.as_os_str().to_str()?
+                if let Some(header_path) = path.parent() {
+                    if header_path == Path::new("") {
+                        header_paths.push(".");
                     } else {
-                        header_path
-                    };
-                    (header, format!("{pch_path}/{hn}.pch"))
+                        header_paths.push(header_path.as_os_str().to_str()?);
+                    }
                 } else {
-                    return None;
-                };
-
-                let mut tu = clang::TranslationUnit::parse(
-                    &index,
-                    &header,
-                    &[
-                        "-x".to_owned().into_boxed_str(),
-                        "c-header".to_owned().into_boxed_str(),
-                    ],
-                    &[],
-                    clang_sys::CXTranslationUnit_ForSerialization,
-                )?;
-                tu.save(&pch).ok()?;
-
-                c_args.push("-include-pch".to_string().into_boxed_str());
-                c_args.push(pch.clone().into_boxed_str());
-                pch_paths.push(pch);
+                    header_paths.push(".");
+                }
+                let header_name = path.file_name()?.to_str()?;
+                header_names_to_compile
+                    .push(header_name.split(".h").next()?.to_string());
+                header_contents +=
+                    format!("\n#include <{header_name}>").as_str();
             }
+            let header_to_precompile = format!(
+                "{}/{}",
+                match self.options().clang_macro_fallback_build_dir {
+                    Some(ref path) => path.as_os_str().to_str()?,
+                    None => ".",
+                },
+                header_names_to_compile.join("-") + "-precompile.h"
+            );
+            let pch = header_to_precompile.clone() + ".pch";
+
+            let mut header_to_precompile_file = OpenOptions::new()
+                .create(true)
+                .truncate(true)
+                .write(true)
+                .open(&header_to_precompile)
+                .ok()?;
+            header_to_precompile_file
+                .write_all(header_contents.as_bytes())
+                .ok()?;
 
+            let mut c_args = Vec::new();
+            c_args.push("-x".to_string().into_boxed_str());
+            c_args.push("c-header".to_string().into_boxed_str());
+            for header_path in header_paths {
+                c_args.push(format!("-I{header_path}").into_boxed_str());
+            }
+            c_args.extend(
+                self.options
+                    .clang_args
+                    .iter()
+                    .filter(|next| {
+                        !self.options.input_headers.contains(next) &&
+                            next.as_ref() != "-include"
+                    })
+                    .cloned(),
+            );
+            let mut tu = clang::TranslationUnit::parse(
+                &index,
+                &header_to_precompile,
+                &c_args,
+                &[],
+                clang_sys::CXTranslationUnit_ForSerialization,
+            )?;
+            tu.save(&pch).ok()?;
+
+            let mut c_args = vec![
+                "-include-pch".to_string().into_boxed_str(),
+                pch.clone().into_boxed_str(),
+            ];
+            c_args.extend(
+                self.options
+                    .clang_args
+                    .clone()
+                    .iter()
+                    .filter(|next| {
+                        !self.options.input_headers.contains(next) &&
+                            next.as_ref() != "-include"
+                    })
+                    .cloned(),
+            );
             self.fallback_tu = Some(clang::FallbackTranslationUnit::new(
-                file, pch_paths, &c_args,
+                file,
+                header_to_precompile,
+                pch,
+                &c_args,
             )?);
         }
 


Test patch:
diff --git a/bindgen-tests/tests/expectations/tests/libclang-9/macro_fallback_non_system_dir.rs b/bindgen-tests/tests/expectations/tests/libclang-9/macro_fallback_non_system_dir.rs
new file mode 100644
index 00000000..8f5c4ba2
--- /dev/null
+++ b/bindgen-tests/tests/expectations/tests/libclang-9/macro_fallback_non_system_dir.rs
@@ -0,0 +1,1 @@
+pub const NEGATIVE: i32 = -1;
diff --git a/bindgen-tests/tests/expectations/tests/test_macro_fallback_non_system_dir.rs b/bindgen-tests/tests/expectations/tests/test_macro_fallback_non_system_dir.rs
new file mode 100644
index 00000000..bf9739f3
--- /dev/null
+++ b/bindgen-tests/tests/expectations/tests/test_macro_fallback_non_system_dir.rs
@@ -0,0 +1,6 @@
+pub const CONST: u32 = 5;
+pub const OTHER_CONST: u32 = 6;
+pub const LARGE_CONST: u32 = 1536;
+pub const THE_CONST: u32 = 28;
+pub const MY_CONST: u32 = 69;
+pub const NEGATIVE: i32 = -1;
diff --git a/bindgen-tests/tests/headers/issue-753.h b/bindgen-tests/tests/headers/issue-753.h
index 94bb8e1c..3a6c8252 100644
--- a/bindgen-tests/tests/headers/issue-753.h
+++ b/bindgen-tests/tests/headers/issue-753.h
@@ -1,7 +1,12 @@
 // bindgen-flags: --clang-macro-fallback
 
+#ifndef ISSUE_753_H
+#define ISSUE_753_H
+
 #define UINT32_C(c) c ## U
 
 #define CONST UINT32_C(5)
 #define OTHER_CONST UINT32_C(6)
 #define LARGE_CONST UINT32_C(6 << 8)
+
+#endif
diff --git a/bindgen-tests/tests/macro_fallback_test_headers/another_header.h b/bindgen-tests/tests/macro_fallback_test_headers/another_header.h
new file mode 100644
index 00000000..b0c40eb4
--- /dev/null
+++ b/bindgen-tests/tests/macro_fallback_test_headers/another_header.h
@@ -0,0 +1,10 @@
+#ifndef ANOTHER_HEADER_H
+#define ANOTHER_HEADER_H
+
+#include <issue-753.h>
+
+#define SHOULD_NOT_GENERATE UINT64_C(~0)
+#define MY_CONST UINT32_C(69)
+#define NEGATIVE ~0
+
+#endif
diff --git a/bindgen-tests/tests/macro_fallback_test_headers/one_header.h b/bindgen-tests/tests/macro_fallback_test_headers/one_header.h
new file mode 100644
index 00000000..5058814b
--- /dev/null
+++ b/bindgen-tests/tests/macro_fallback_test_headers/one_header.h
@@ -0,0 +1,8 @@
+#ifndef ONE_HEADER_H
+#define ONE_HEADER_H
+
+#include <issue-753.h>
+
+#define THE_CONST UINT32_C(28)
+
+#endif
diff --git a/bindgen-tests/tests/tests.rs b/bindgen-tests/tests/tests.rs
index e6c038a0..14988e46 100644
--- a/bindgen-tests/tests/tests.rs
+++ b/bindgen-tests/tests/tests.rs
@@ -565,6 +565,61 @@ fn test_mixed_header_and_header_contents() {
     }
 }
 
+#[test]
+fn test_macro_fallback_non_system_dir() {
+    let actual = builder()
+        .header(concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/tests/macro_fallback_test_headers/one_header.h"
+        ))
+        .header(concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/tests/macro_fallback_test_headers/another_header.h"
+        ))
+        .clang_macro_fallback()
+        .clang_arg(format!("-I{}/tests/headers", env!("CARGO_MANIFEST_DIR")))
+        .generate()
+        .unwrap()
+        .to_string();
+
+    let actual = format_code(actual).unwrap();
+
+    let (expected_filename, expected) = match clang_version().parsed {
+        Some((9, _)) => {
+            let expected_filename = concat!(
+                env!("CARGO_MANIFEST_DIR"),
+                "/tests/expectations/tests/libclang-9/macro_fallback_non_system_dir.rs",
+            );
+            let expected = include_str!(concat!(
+                env!("CARGO_MANIFEST_DIR"),
+                "/tests/expectations/tests/libclang-9/macro_fallback_non_system_dir.rs",
+            ));
+            (expected_filename, expected)
+        }
+        _ => {
+            let expected_filename = concat!(
+                env!("CARGO_MANIFEST_DIR"),
+                "/tests/expectations/tests/test_macro_fallback_non_system_dir.rs",
+            );
+            let expected = include_str!(concat!(
+                env!("CARGO_MANIFEST_DIR"),
+                "/tests/expectations/tests/test_macro_fallback_non_system_dir.rs",
+            ));
+            (expected_filename, expected)
+        }
+    };
+    let expected = format_code(expected).unwrap();
+    if expected != actual {
+        error_diff_mismatch(
+            &actual,
+            &expected,
+            None,
+            Path::new(expected_filename),
+        )
+        .unwrap();
+    }
+}
+
 #[test]
 // Doesn't support executing sh file on Windows.
 // We may want to implement it in Rust so that we support all systems.